Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3864.400.21\))
Subject: Re: Bug: wrong relname in RemoveSubscriptionRel() error detail
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CAA4eK1KNGMqQbp7qkKjvVnbQVSR14pmg_WVqpaAzaKKSiN_zFQ@mail.gmail.com>
Date: Tue, 31 Mar 2026 15:53:46 +0800
Cc: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>,
 Nisha Moond <nisha.moond412@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <C5E09856-0BA1-41FB-83F9-FD5758224909@gmail.com>
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>
 <CAA4eK1KNGMqQbp7qkKjvVnbQVSR14pmg_WVqpaAzaKKSiN_zFQ@mail.gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/C5E09856-0BA1-41FB-83F9-FD5758224909%40gmail.com>
Precedence: bulk


> On Mar 30, 2026, at 20:19, Amit Kapila <amit.kapila16@gmail.com> =
wrote:
>=20
> On Mon, Mar 30, 2026 at 1:44=E2=80=AFPM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
>>=20
>> 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:
>>>>=20
>>>> On Fri, Mar 27, 2026 at 3:22=E2=80=AFPM Chao Li =
<li.evan.chao@gmail.com> wrote:
>>>>> This looks like a first-day bug introducing by ce0fdbf, so I think =
it=E2=80=99s worth
>>>>> back-patching.
>>>>>=20
>>>> I tried reproducing the said bug on HEAD, but it doesn=E2=80=99t =
seem to exist
>>>> in the current code.
>>>>=20
>>>> 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
>>>>=20
>>>> 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);
>>>>=20
>>>> It can only happen with a hypothetical future caller of
>>>> RemoveSubscriptionRel(InvalidOid, InvalidOid). And in that case, =
using
>>>> "subrel->srrelid" would be correct.
>>>>=20
>>>> 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.
>>>>=20
>>>> --
>>>> Thanks,
>>>> Nisha
>>>=20
>>> Hi Nisha,
>>>=20
>>> Thanks for your review.
>>>=20
>>> I think one current call site may have been overlooked. In =
DropSubscription(),
>>> we have:
>>> ```
>>>    /* Remove any associated relation synchronization states. */
>>>    RemoveSubscriptionRel(subid, InvalidOid);
>>> ```
>>=20
>> 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.
>>=20
>>>=20
>>> I agree this is an edge-case bug and may be difficult to reproduce =
in practice.
>>> But from the function=E2=80=99s 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.
>>=20
>> 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.
>>=20
>=20
> 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
> --=20
> With Regards,
> Amit Kapila.

Hi Amit,

Yes, I agree there is no current use case that would trigger this bug.

Looking at the function header comment again:
```
/*
* Drop subscription relation mapping. These can be for a particular
* subscription, or for a particular relation, or both.
*/
void
RemoveSubscriptionRel(Oid subid, Oid relid)
```

Looks like the function comment does not clearly say that both subid and =
relid being InvalidOid is a supported case. To me, =E2=80=9Ca particular =
subscription=E2=80=9D or =E2=80=9Ca particular relation=E2=80=9D reads =
as a specific valid OID, so the comment is really silent about the case =
where both are invalid.

If we consider subid =3D=3D InvalidOid && relid =3D=3D InvalidOid to be =
invalid usage, then I think it would be better to make that explicit, =
for example by documenting it and adding an Assert. Otherwise, the =
current behavior can still lead to a misleading error detail in that =
path.

=46rom the code readability perspective, I also think subrel->srrelid is =
a better choice here regardless. This error is reporting the relation =
whose table synchronization is actually in progress, and that relation =
comes from the current mapping row, not from the input argument used as =
a filter. So using subrel->srrelid makes the code and the error =
construction easier to understand.

Given that there is no known practical case that triggers it today, I =
agree that back-patching is not needed.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/