MIME-Version: 1.0
References: <B7B16777-AC54-413A-BECE-A814E90D395B@gmail.com>
 <CABdArM6ZsUa1Xh06FaF9dK+ECxHe4NGOXspRxOu9SgivvcKiAw@mail.gmail.com>
 <DD53A97B-AF71-4101-8B2B-2006623B3B9A@gmail.com>
 <TY4PR01MB1690715C2DD73955A5B8C1BD99452A@TY4PR01MB16907.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <TY4PR01MB1690715C2DD73955A5B8C1BD99452A@TY4PR01MB16907.jpnprd01.prod.outlook.com>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Mon, 30 Mar 2026 17:49:41 +0530
Message-ID: 
 <CAA4eK1KNGMqQbp7qkKjvVnbQVSR14pmg_WVqpaAzaKKSiN_zFQ@mail.gmail.com>
Subject: Re: Bug: wrong relname in RemoveSubscriptionRel() error detail
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Cc: Chao Li <li.evan.chao@gmail.com>, Nisha Moond <nisha.moond412@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1KNGMqQbp7qkKjvVnbQVSR14pmg_WVqpaAzaKKSiN_zFQ%40mail.gmail.com>
Precedence: bulk

On Mon, Mar 30, 2026 at 1:44=E2=80=AFPM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Monday, March 30, 2026 2:33 PM Chao Li <li.evan.chao@gmail.com> wrote:
> > > On Mar 30, 2026, at 14:16, Nisha Moond <nisha.moond412@gmail.com>
> > wrote:
> > >
> > > On Fri, Mar 27, 2026 at 3:22=E2=80=AFPM Chao Li <li.evan.chao@gmail.c=
om> wrote:
> > > > This looks like a first-day bug introducing by ce0fdbf, so I think =
it=E2=80=99s worth
> > > > back-patching.
> > > >
> > > I tried reproducing the said bug on HEAD, but it doesn=E2=80=99t seem=
 to exist
> > > in the current code.
> > >
> > > To hit the mentioned error, the subid has to be invalid -
> > >   if (!OidIsValid(subid) &&  <=3D=3D
> > > And currently, the only path that uses an invalid subid is via
> > > heap_drop_with_catalog() :
> > > =E2=80=A6
> > > /*
> > >   * Remove any associated relation synchronization states.
> > >   */
> > >  RemoveSubscriptionRel(InvalidOid, relid);
> > > =E2=80=A6
> > >
> > > But here relid is always a valid OID (it's the table being dropped).
> > > The corresponding pg_class row is deleted after
> > > RemoveSubscriptionRel(), i.e. via a later call to
> > > DeleteRelationTuple(relid);
> > >
> > > It can only happen with a hypothetical future caller of
> > > RemoveSubscriptionRel(InvalidOid, InvalidOid). And in that case, usin=
g
> > > "subrel->srrelid" would be correct.
> > >
> > > So this doesn=E2=80=99t appear to be a real issue in the current code=
, and
> > > doesn=E2=80=99t look like a bug to fix now. IMO, if such a caller is =
added in
> > > the future, we can add a guard at that point.
> > >
> > > --
> > > Thanks,
> > > Nisha
> >
> > Hi Nisha,
> >
> > Thanks for your review.
> >
> > I think one current call site may have been overlooked. In DropSubscrip=
tion(),
> > we have:
> > ```
> >     /* Remove any associated relation synchronization states. */
> >     RemoveSubscriptionRel(subid, InvalidOid);
> > ```
>
> This won't trigger the bug either, since it passes a valid subscription O=
ID to
> the function, while the function only reports an error when an invalid OI=
D is
> passed.
>
> >
> > I agree this is an edge-case bug and may be difficult to reproduce in p=
ractice.
> > But from the function=E2=80=99s semantics, it seems clear to me that th=
e wrong
> > relation OID is used in the error detail, regardless of how easy it is =
to trigger
> > today.
>
> Since this is a public function, I think it should be OK to fix it as it'=
s good
> to make the function future-proof anyway.
>

Even if it is exposed, it is not clear to me in which case one would
like to use it the way it can lead to a problem. I feel unless we have
some concrete case it may not be beneficial to change it.

--=20
With Regards,
Amit Kapila.