On Tue, 4 Aug 2026 at 11:25, Bertrand Drouvot
<[email protected]> wrote:
>
> Hi,
>
> On Mon, Jul 06, 2026 at 02:53:39PM +0000, Bertrand Drouvot wrote:
> > Hi,
> >
> > On Mon, Jul 06, 2026 at 03:07:24PM +0530, Amit Kapila wrote:
> >
> > DROP SUBSCRIPTION however has its own dedicated code path and does not go
> > through
> > get_object_address(): 0003 adds the retry loop for it. And if DROP already
> > uses
> > the retry loop then ALTER should probably use it too (also done in 0003 and
> > 0004).
>
> Mandatory rebase attached.
>
Hi, I reviewed 0001 and 0002 patches.
0001 LGTM.
Some comments for 0002 patch:
+ /* DROP hook for the subscription being removed */
+ InvokeObjectDropHook(SubscriptionRelationId, subid, 0);
+
+ /*
+ * Re-read the subscription tuple after acquiring the lock. A concurrent
+ * ALTER or DROP may have committed before we acquired the lock.
+ */
+ tup = SearchSysCache1(SUBSCRIPTIONOID, ObjectIdGetDatum(subid));
+
+ if (!HeapTupleIsValid(tup))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("subscription \"%s\" does not exist",
+ stmt->subname)));
+
1. Here if 'tup' is invalid we are not checking the 'stmt->missing_ok' flag.
Should we only throw an error if 'stmt->missing' is false?
Otherwise 'DROP SUBSCRIPTION IF EXISTS' can throw an error like:
postgres=# DROP SUBSCRIPTION IF EXISTS sub1;
ERROR: subscription "sub1" does not exist
2. Should the function 'InvokeObjectDropHook' be called after the
check in 'if (!HeapTupleIsValid(tup))'?
If 'tup' is not valid, an error is thrown, and in this case, calling
the function 'InvokeObjectDropHook' is unnecessary.
Thanks,
Shlok Kyal
