Re: pg_replication_origin_drop API potential race condition

On Sat, Feb 6, 2021 at 5:47 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Sat, Feb 6, 2021 at 3:26 PM Petr Jelinek <pjmodos(at)pjmodos(dot)net> wrote:
> >
> > On 06/02/2021 07:29, Amit Kapila wrote:
> > > On Fri, Feb 5, 2021 at 6:45 PM Euler Taveira <euler(at)eulerto(dot)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.