On Sat, Feb 6, 2021 at 5:47 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Sat, Feb 6, 2021 at 3:26 PM Petr Jelinek <pjmo...@pjmodos.net> wrote: > > > > On 06/02/2021 07:29, Amit Kapila wrote: > > > On Fri, Feb 5, 2021 at 6:45 PM Euler Taveira <eu...@eulerto.com> wrote: > > >> - replorigin_drop(roident, true); > > >> + replorigin_drop_by_name(name, false /* missing_ok */ , true /* nowait > > >> */ ); > > >> > > >> A modern IDE would certainly show you the function definition that > > >> allows you > > >> to check what each parameter value is without having to go back and > > >> forth. I > > >> saw a few occurrences of this pattern in the source code and IMO it > > >> could be > > >> used when it is not obvious what that value means. Booleans are easier to > > >> figure out, however, sometimes integer and text are not. > > >> > > > Fair enough, removed in the attached patch. > > > > > > To be fair the logical replication framework is full of these comments > > so it's pretty natural to add them to new code as well, but I agree with > > Euler that it's unnecessary with any reasonable development tooling. > > > > The patch as posted looks good to me, > > > > Thanks, but today again testing this API, I observed that we can still > get "tuple concurrently deleted" because we are releasing the lock on > ReplicationOriginRelationId at the end of API replorigin_drop_by_name. > So there is no guarantee that invalidation reaches other backend doing > the same operation. I think we need to keep the lock till the end of > xact as we do in other drop operations (see DropTableSpace, dropdb). >
Fixed the problem as mentioned above in the attached. -- With Regards, Amit Kapila.
v5-0001-Make-pg_replication_origin_drop-safe-against-con.patch
Description: Binary data
