On Mon, Mar 30, 2026 at 1:44 PM Zhijie Hou (Fujitsu) <[email protected]> wrote: > > On Monday, March 30, 2026 2:33 PM Chao Li <[email protected]> wrote: > > > 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: > > > > 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); > > ``` > > This won't trigger the bug either, since it passes a valid subscription OID to > the function, while the function only reports an error when an invalid OID is > passed. > > > > > 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. > > Since this is a public function, I think it should be OK to fix it as it's > good > to make the function future-proof anyway. >
Even if it is exposed, it is not clear to me in which case one would like to use it the way it can lead to a problem. I feel unless we have some concrete case it may not be beneficial to change it. -- With Regards, Amit Kapila.
