public inbox for [email protected]  

help / color / mirror / Atom feed

From: 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 14:33:00 +0800
Message-ID: <[email protected]> (raw)
In-Reply-To: <CABdArM6ZsUa1Xh06FaF9dK+ECxHe4NGOXspRxOu9SgivvcKiAw@mail.gmail.com>
References: <[email protected]>
	<CABdArM6ZsUa1Xh06FaF9dK+ECxHe4NGOXspRxOu9SgivvcKiAw@mail.gmail.com>



> 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/

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]
  Subject: Re: Bug: wrong relname in RemoveSubscriptionRel() error detail
  In-Reply-To: <[email protected]>

* 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