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
