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). -- With Regards, Amit Kapila.
