public inbox for [email protected]
help / color / mirror / Atom feedBug: wrong relname in RemoveSubscriptionRel() error detail
6+ messages / 4 participants
[nested] [flat]
* Bug: wrong relname in RemoveSubscriptionRel() error detail
@ 2026-03-27 09:51 Chao Li <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Chao Li @ 2026-03-27 09:51 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>
Hi,
I just noticed a wrong relation name in the error detail emitted by RemoveSubscriptionRel():
```
*
* Drop subscription relation mapping. These can be for a particular
* subscription, or for a particular relation, or both.
*/
void
RemoveSubscriptionRel(Oid subid, Oid relid)
{
Relation rel;
TableScanDesc scan;
ScanKeyData skey[2];
HeapTuple tup;
int nkeys = 0;
rel = table_open(SubscriptionRelRelationId, RowExclusiveLock);
if (OidIsValid(subid))
{
ScanKeyInit(&skey[nkeys++],
Anum_pg_subscription_rel_srsubid,
BTEqualStrategyNumber,
F_OIDEQ,
ObjectIdGetDatum(subid));
}
if (OidIsValid(relid))
{
ScanKeyInit(&skey[nkeys++],
Anum_pg_subscription_rel_srrelid,
BTEqualStrategyNumber,
F_OIDEQ,
ObjectIdGetDatum(relid));
}
/* Do the search and delete what we found. */
scan = table_beginscan_catalog(rel, nkeys, skey);
while (HeapTupleIsValid(tup = heap_getnext(scan, ForwardScanDirection)))
{
Form_pg_subscription_rel subrel;
subrel = (Form_pg_subscription_rel) GETSTRUCT(tup);
if (!OidIsValid(subid) &&
subrel->srsubstate != SUBREL_STATE_READY &&
get_rel_relkind(subrel->srrelid) != RELKIND_SEQUENCE)
{
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("could not drop relation mapping for subscription \"%s\"",
get_subscription_name(subrel->srsubid, false)),
errdetail("Table synchronization for relation \"%s\" is in progress and is in state \"%c\".",
get_rel_name(relid), subrel->srsubstate), // <====== bug is here
```
In the error detail, it uses relid to get the relation name. But at that point we are iterating over subrel, and the function argument relid can be InvalidOid. So the error detail should use subrel->srrelid instead.
This looks like a first-day bug introducing by ce0fdbf, so I think it’s worth back-patching.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
Attachments:
[application/octet-stream] v1-0001-Fix-wrong-relname-in-RemoveSubscriptionRel-error-.patch (1.1K, 2-v1-0001-Fix-wrong-relname-in-RemoveSubscriptionRel-error-.patch)
download | inline diff:
From c49dc0495607c05c6ac8e15cc0300f91bacf0f6c Mon Sep 17 00:00:00 2001
From: "Chao Li (Evan)" <[email protected]>
Date: Fri, 27 Mar 2026 17:39:30 +0800
Subject: [PATCH v1] Fix wrong relname in RemoveSubscriptionRel() error detail
Author: Chao Li <[email protected]>
Reviewed-by:
Discussion: https://postgr.es/m/
---
src/backend/catalog/pg_subscription.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 5a733585490..0ee00d91d83 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -525,7 +525,7 @@ RemoveSubscriptionRel(Oid subid, Oid relid)
errmsg("could not drop relation mapping for subscription \"%s\"",
get_subscription_name(subrel->srsubid, false)),
errdetail("Table synchronization for relation \"%s\" is in progress and is in state \"%c\".",
- get_rel_name(relid), subrel->srsubstate),
+ get_rel_name(subrel->srrelid), subrel->srsubstate),
/*
* translator: first %s is a SQL ALTER command and second %s is a
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Bug: wrong relname in RemoveSubscriptionRel() error detail
@ 2026-03-30 06:16 Nisha Moond <[email protected]>
parent: Chao Li <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Nisha Moond @ 2026-03-30 06:16 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
On Fri, Mar 27, 2026 at 3:22 PM Chao Li <[email protected]> wrote:
>
> Hi,
>
> I just noticed a wrong relation name in the error detail emitted by RemoveSubscriptionRel():
> ```
> *
> * Drop subscription relation mapping. These can be for a particular
> * subscription, or for a particular relation, or both.
> */
> void
> RemoveSubscriptionRel(Oid subid, Oid relid)
> {
> Relation rel;
> TableScanDesc scan;
> ScanKeyData skey[2];
> HeapTuple tup;
> int nkeys = 0;
>
> rel = table_open(SubscriptionRelRelationId, RowExclusiveLock);
>
> if (OidIsValid(subid))
> {
> ScanKeyInit(&skey[nkeys++],
> Anum_pg_subscription_rel_srsubid,
> BTEqualStrategyNumber,
> F_OIDEQ,
> ObjectIdGetDatum(subid));
> }
>
> if (OidIsValid(relid))
> {
> ScanKeyInit(&skey[nkeys++],
> Anum_pg_subscription_rel_srrelid,
> BTEqualStrategyNumber,
> F_OIDEQ,
> ObjectIdGetDatum(relid));
> }
>
> /* Do the search and delete what we found. */
> scan = table_beginscan_catalog(rel, nkeys, skey);
> while (HeapTupleIsValid(tup = heap_getnext(scan, ForwardScanDirection)))
> {
> Form_pg_subscription_rel subrel;
>
> subrel = (Form_pg_subscription_rel) GETSTRUCT(tup);
>
> if (!OidIsValid(subid) &&
> subrel->srsubstate != SUBREL_STATE_READY &&
> get_rel_relkind(subrel->srrelid) != RELKIND_SEQUENCE)
> {
> ereport(ERROR,
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> errmsg("could not drop relation mapping for subscription \"%s\"",
> get_subscription_name(subrel->srsubid, false)),
> errdetail("Table synchronization for relation \"%s\" is in progress and is in state \"%c\".",
> get_rel_name(relid), subrel->srsubstate), // <====== bug is here
>
> ```
>
> In the error detail, it uses relid to get the relation name. But at that point we are iterating over subrel, and the function argument relid can be InvalidOid. So the error detail should use subrel->srrelid instead.
>
> 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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Bug: wrong relname in RemoveSubscriptionRel() error detail
@ 2026-03-30 06:33 Chao Li <[email protected]>
parent: Nisha Moond <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Chao Li @ 2026-03-30 06:33 UTC (permalink / raw)
To: Nisha Moond <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
> 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:
>>
>> Hi,
>>
>> I just noticed a wrong relation name in the error detail emitted by RemoveSubscriptionRel():
>> ```
>> *
>> * Drop subscription relation mapping. These can be for a particular
>> * subscription, or for a particular relation, or both.
>> */
>> void
>> RemoveSubscriptionRel(Oid subid, Oid relid)
>> {
>> Relation rel;
>> TableScanDesc scan;
>> ScanKeyData skey[2];
>> HeapTuple tup;
>> int nkeys = 0;
>>
>> rel = table_open(SubscriptionRelRelationId, RowExclusiveLock);
>>
>> if (OidIsValid(subid))
>> {
>> ScanKeyInit(&skey[nkeys++],
>> Anum_pg_subscription_rel_srsubid,
>> BTEqualStrategyNumber,
>> F_OIDEQ,
>> ObjectIdGetDatum(subid));
>> }
>>
>> if (OidIsValid(relid))
>> {
>> ScanKeyInit(&skey[nkeys++],
>> Anum_pg_subscription_rel_srrelid,
>> BTEqualStrategyNumber,
>> F_OIDEQ,
>> ObjectIdGetDatum(relid));
>> }
>>
>> /* Do the search and delete what we found. */
>> scan = table_beginscan_catalog(rel, nkeys, skey);
>> while (HeapTupleIsValid(tup = heap_getnext(scan, ForwardScanDirection)))
>> {
>> Form_pg_subscription_rel subrel;
>>
>> subrel = (Form_pg_subscription_rel) GETSTRUCT(tup);
>>
>> if (!OidIsValid(subid) &&
>> subrel->srsubstate != SUBREL_STATE_READY &&
>> get_rel_relkind(subrel->srrelid) != RELKIND_SEQUENCE)
>> {
>> ereport(ERROR,
>> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>> errmsg("could not drop relation mapping for subscription \"%s\"",
>> get_subscription_name(subrel->srsubid, false)),
>> errdetail("Table synchronization for relation \"%s\" is in progress and is in state \"%c\".",
>> get_rel_name(relid), subrel->srsubstate), // <====== bug is here
>>
>> ```
>>
>> In the error detail, it uses relid to get the relation name. But at that point we are iterating over subrel, and the function argument relid can be InvalidOid. So the error detail should use subrel->srrelid instead.
>>
>> 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);
```
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.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 6+ messages in thread
* RE: Bug: wrong relname in RemoveSubscriptionRel() error detail
@ 2026-03-30 08:14 Zhijie Hou (Fujitsu) <[email protected]>
parent: Chao Li <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Zhijie Hou (Fujitsu) @ 2026-03-30 08:14 UTC (permalink / raw)
To: Chao Li <[email protected]>; Nisha Moond <[email protected]>; +Cc: PostgreSQL Hackers <[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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Bug: wrong relname in RemoveSubscriptionRel() error detail
@ 2026-03-30 12:19 Amit Kapila <[email protected]>
parent: Zhijie Hou (Fujitsu) <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Amit Kapila @ 2026-03-30 12:19 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <[email protected]>; +Cc: Chao Li <[email protected]>; Nisha Moond <[email protected]>; PostgreSQL Hackers <[email protected]>
On Mon, Mar 30, 2026 at 1:44 PM Zhijie Hou (Fujitsu)
<[email protected]> wrote:
>
> 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.
>
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.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Bug: wrong relname in RemoveSubscriptionRel() error detail
@ 2026-03-31 07:53 Chao Li <[email protected]>
parent: Amit Kapila <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Chao Li @ 2026-03-31 07:53 UTC (permalink / raw)
To: Amit Kapila <[email protected]>; +Cc: Zhijie Hou (Fujitsu) <[email protected]>; Nisha Moond <[email protected]>; PostgreSQL Hackers <[email protected]>
> On Mar 30, 2026, at 20:19, Amit Kapila <[email protected]> wrote:
>
> On Mon, Mar 30, 2026 at 1:44 PM Zhijie Hou (Fujitsu)
> <[email protected]> wrote:
>>
>> 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.
>>
>
> 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.
>
> --
> 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, “a particular subscription” or “a particular relation” reads as a specific valid OID, so the comment is really silent about the case where both are invalid.
If we consider subid == InvalidOid && relid == 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.
From 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/
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2026-03-31 07:53 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-03-27 09:51 Bug: wrong relname in RemoveSubscriptionRel() error detail Chao Li <[email protected]>
2026-03-30 06:16 ` Nisha Moond <[email protected]>
2026-03-30 06:33 ` Chao Li <[email protected]>
2026-03-30 08:14 ` Zhijie Hou (Fujitsu) <[email protected]>
2026-03-30 12:19 ` Amit Kapila <[email protected]>
2026-03-31 07:53 ` Chao Li <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox