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: 
 <CABdArM6ZsUa1Xh06FaF9dK+ECxHe4NGOXspRxOu9SgivvcKiAw@mail.gmail.com>
Date: Mon, 30 Mar 2026 14:33:00 +0800
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <DD53A97B-AF71-4101-8B2B-2006623B3B9A@gmail.com>
References: <B7B16777-AC54-413A-BECE-A814E90D395B@gmail.com>
 <CABdArM6ZsUa1Xh06FaF9dK+ECxHe4NGOXspRxOu9SgivvcKiAw@mail.gmail.com>
To: Nisha Moond <nisha.moond412@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/DD53A97B-AF71-4101-8B2B-2006623B3B9A%40gmail.com>
Precedence: bulk


> 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:
>>=20
>> Hi,
>>=20
>> 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 =3D 0;
>>=20
>>        rel =3D table_open(SubscriptionRelRelationId, =
RowExclusiveLock);
>>=20
>>        if (OidIsValid(subid))
>>        {
>>                ScanKeyInit(&skey[nkeys++],
>>                                        =
Anum_pg_subscription_rel_srsubid,
>>                                        BTEqualStrategyNumber,
>>                                        F_OIDEQ,
>>                                        ObjectIdGetDatum(subid));
>>        }
>>=20
>>        if (OidIsValid(relid))
>>        {
>>                ScanKeyInit(&skey[nkeys++],
>>                                        =
Anum_pg_subscription_rel_srrelid,
>>                                        BTEqualStrategyNumber,
>>                                        F_OIDEQ,
>>                                        ObjectIdGetDatum(relid));
>>        }
>>=20
>>        /* Do the search and delete what we found. */
>>        scan =3D table_beginscan_catalog(rel, nkeys, skey);
>>        while (HeapTupleIsValid(tup =3D heap_getnext(scan, =
ForwardScanDirection)))
>>        {
>>                Form_pg_subscription_rel subrel;
>>=20
>>                subrel =3D (Form_pg_subscription_rel) GETSTRUCT(tup);
>>=20
>>                if (!OidIsValid(subid) &&
>>                        subrel->srsubstate !=3D SUBREL_STATE_READY &&
>>                        get_rel_relkind(subrel->srrelid) !=3D =
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), // <=3D=3D=3D=3D=3D=3D bug is =
here
>>=20
>> ```
>>=20
>> 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.
>>=20
>> This looks like a first-day bug introducing by ce0fdbf, so I think =
it=E2=80=99s worth back-patching.
>>=20
>=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

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=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.

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