public inbox for [email protected]
help / color / mirror / Atom feedFrom: Zhijie Hou (Fujitsu) <[email protected]>
To: Chao Li <[email protected]>
To: Nisha Moond <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: RE: Bug: wrong relname in RemoveSubscriptionRel() error detail
Date: Mon, 30 Mar 2026 08:14:07 +0000
Message-ID: <TY4PR01MB1690715C2DD73955A5B8C1BD99452A@TY4PR01MB16907.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CABdArM6ZsUa1Xh06FaF9dK+ECxHe4NGOXspRxOu9SgivvcKiAw@mail.gmail.com>
<[email protected]>
On Monday, March 30, 2026 2:33 PM Chao Li <[email protected]> wrote:
> > On Mar 30, 2026, at 14:16, Nisha Moond <[email protected]>
> wrote:
> >
> > On Fri, Mar 27, 2026 at 3:22 PM Chao Li <[email protected]> wrote:
> > > This looks like a first-day bug introducing by ce0fdbf, so I think it’s worth
> > > back-patching.
> > >
> > I tried reproducing the said bug on HEAD, but it doesn’t seem to exist
> > in the current code.
> >
> > To hit the mentioned error, the subid has to be invalid -
> > if (!OidIsValid(subid) && <==
> > And currently, the only path that uses an invalid subid is via
> > heap_drop_with_catalog() :
> > …
> > /*
> > * Remove any associated relation synchronization states.
> > */
> > RemoveSubscriptionRel(InvalidOid, relid);
> > …
> >
> > 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, using
> > "subrel->srrelid" would be correct.
> >
> > So this doesn’t appear to be a real issue in the current code, and
> > doesn’t 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 DropSubscription(),
> we have:
> ```
> /* Remove any associated relation synchronization states. */
> RemoveSubscriptionRel(subid, InvalidOid);
> ```
This won't trigger the bug either, since it passes a valid subscription OID to
the function, while the function only reports an error when an invalid OID is
passed.
>
> I agree this is an edge-case bug and may be difficult to reproduce in practice.
> But from the function’s semantics, it seems clear to me that the 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. I'm slightly unsure, however, whether
it's worth backpatching, since this is purely a theoretical issue at the moment.
Best Regards,
Hou zj
view thread (6+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: RE: Bug: wrong relname in RemoveSubscriptionRel() error detail
In-Reply-To: <TY4PR01MB1690715C2DD73955A5B8C1BD99452A@TY4PR01MB16907.jpnprd01.prod.outlook.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox