Re: pg_replication_origin_drop API potential race condition

On Fri, Feb 5, 2021 at 1:50 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Fri, Feb 5, 2021 at 6:02 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Fri, Feb 5, 2021 at 9:46 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> > >
> > > PSA patch updated per above suggestions.
> > >
> >
> > Thanks, I have tested your patch and before the patch, I was getting
> > errors like "tuple concurrently deleted" or "cache lookup failed for
> > replication origin with oid 1" and after the patch, I am getting
> > "replication origin "origin-1" does not exist" which is clearly better
> > and user-friendly.
> >
> > Before Patch
> > postgres=# select pg_replication_origin_drop('origin-1');
> > ERROR: tuple concurrently deleted
> > postgres=# select pg_replication_origin_drop('origin-1');
> > ERROR: cache lookup failed for replication origin with oid 1
> >
> > After Patch
> > postgres=# select pg_replication_origin_drop('origin-1');
> > ERROR: replication origin "origin-1" does not exist
> >
> > I wonder why you haven't changed the usage of the existing
> > replorigin_drop in the code? I have changed the same, added few
> > comments, ran pgindent, and updated the commit message in the
> > attached.
>
> You are right.
>
> The goal of this patch was to fix pg_replication_origin_drop, but
> while focussed on fixing that, I forgot the same call pattern was also
> in the DropSubscription.
>
> >
> > I am not completely whether we should retire replorigin_drop or just
> > keep it for backward compatibility? What do you think? Anybody else
> > has any opinion?
>
> It is still good code, but just not being used atm.
>
> I don't know what is the PG convention for dead code - to remove it
> immedaitely at first sight, or to leave it lying around if it still
> might have future usefulness?
>

I am mostly worried about the extensions outside pg-core. For example,
on a quick search, it seems there seem to be a few such usages in
pglogical [1][2]. Then, I see a similar usage pattern (search by name
and then drop) in one of the pglogical [3].

[1] - https://github.com/2ndQuadrant/pglogical/issues/160
[2] - https://github.com/2ndQuadrant/pglogical/issues/124
[3] - https://github.com/2ndQuadrant/pglogical/blob/REL2_x_STABLE/pglogical_functions.c

--
With Regards,
Amit Kapila.