Re: Bug: wrong relname in RemoveSubscriptionRel() error detail

On Fri, Mar 27, 2026 at 3:22 PM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> 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