MIME-Version: 1.0
References: <B7B16777-AC54-413A-BECE-A814E90D395B@gmail.com>
In-Reply-To: <B7B16777-AC54-413A-BECE-A814E90D395B@gmail.com>
From: Nisha Moond <nisha.moond412@gmail.com>
Date: Mon, 30 Mar 2026 11:46:10 +0530
Message-ID: 
 <CABdArM6ZsUa1Xh06FaF9dK+ECxHe4NGOXspRxOu9SgivvcKiAw@mail.gmail.com>
Subject: Re: Bug: wrong relname in RemoveSubscriptionRel() error detail
To: Chao Li <li.evan.chao@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CABdArM6ZsUa1Xh06FaF9dK%2BECxHe4NGOXspRxOu9SgivvcKiAw%40mail.gmail.com>
Precedence: bulk

On Fri, Mar 27, 2026 at 3:22=E2=80=AFPM Chao Li <li.evan.chao@gmail.com> wr=
ote:
>
> Hi,
>
> I just noticed a wrong relation name in the error detail emitted by Remov=
eSubscriptionRel():
> ```
> *
>  * 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;
>
>         rel =3D 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 =3D table_beginscan_catalog(rel, nkeys, skey);
>         while (HeapTupleIsValid(tup =3D heap_getnext(scan, ForwardScanDir=
ection)))
>         {
>                 Form_pg_subscription_rel subrel;
>
>                 subrel =3D (Form_pg_subscription_rel) GETSTRUCT(tup);
>
>                 if (!OidIsValid(subid) &&
>                         subrel->srsubstate !=3D SUBREL_STATE_READY &&
>                         get_rel_relkind(subrel->srrelid) !=3D RELKIND_SEQ=
UENCE)
>                 {
>                         ereport(ERROR,
>                                         (errcode(ERRCODE_INVALID_PARAMETE=
R_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(r=
elid), subrel->srsubstate), // <=3D=3D=3D=3D=3D=3D 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=E2=
=80=99s worth back-patching.
>

I tried reproducing the said bug on HEAD, but it doesn=E2=80=99t seem to ex=
ist
in the current code.

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

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

--
Thanks,
Nisha