Re: pg_replication_origin_drop API potential race condition - Mailing list pgsql-hackers

From Amit Kapila
Subject Re: pg_replication_origin_drop API potential race condition
Date
Msg-id CAA4eK1KgX7+Bc_ZgD+1gderLPXRLKTfwfrpfg8grFv=Ux9RwVw@mail.gmail.com
Whole thread Raw
In response to Re: pg_replication_origin_drop API potential race condition  (Peter Smith <smithpb2250@gmail.com>)
List pgsql-hackers
On Fri, Feb 5, 2021 at 1:50 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Fri, Feb 5, 2021 at 6:02 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Feb 5, 2021 at 9:46 AM Peter Smith <smithpb2250@gmail.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.



pgsql-hackers by date:

Previous
From: Amul Sul
Date:
Subject: Re: Correct comment in StartupXLOG().
Next
From: Fujii Masao
Date:
Subject: Re: adding wait_start column to pg_locks