Re: pg_replication_origin_drop API potential race condition

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?
Personally, I would leave it, if only because it seems a less radical
change from the current HEAD code to keep the existing function
signature.

------
Kind Regards,
Peter Smith.
Fujitsu Australia