On Thu, Jul 2, 2026 at 5:38 PM Bertrand Drouvot <[email protected]> wrote: > > Hi hackers, > > while playing with the new ALTER SUBSCRIPTION parameter added in a5918fddf10, > I realized that the subscription is not re-read once we acquire the lock in > AlterSubscription(). > > This pre-existing issue is now more visible after a5918fddf10: > > 1/ two concurrent ALTER SUBSCRIPTION SET (conflict_log_destination = 'table') > could result in the second session attempting to create an already-existing > conflict log table, producing a confusing "relation already exists" error: > > ERROR: relation "pg_conflict_log_24614" already exists > > It's confusing because ALTER SUBSCRIPTION SET (conflict_log_destination = > 'table') > would not report an error if the conflict table already exists (and no > concurrent > ALTER is running). > > 2/ a concurrent DROP followed by the ALTER would emit a NOTICE about creating > the > conflict log table before failing with "referenced subscription was > concurrently > dropped". That sounds like a weird messaging: > > NOTICE: created conflict log table "pg_conflict.pg_conflict_log_24620" for > subscription "mysub" > ERROR: referenced subscription was concurrently dropped > > The attached fixes it by: > > - Re-reading the subscription tuple after LockSharedObject() and refreshing > the > Subscription struct. > - Moving the local variable assignments to after the re-read. > - Re-checking the password_required privilege restriction after the re-read. > > Remarks: > > 1/ not re-checking password_required after the re-read would still produce a > "tuple concurrently updated" error, but re-checking it allows us to display a > better error message. > > 2/ the ownership check is intentionally not re-done after the lock because > AlterSubscriptionOwner() does not take AccessExclusiveLock on the subscription > object: it only takes RowExclusiveLock on the pg_subscription catalog table. > This means ownership can change regardless of our lock, making a re-check > after > lock acquisition pointless. The existing "tuple concurrently updated" error > from > CatalogTupleUpdate() already provides a protection if ownership changes > concurrently. > > 3/ the "privileges" checks are still also done before the lock acquisition > because > we don't want to lock an object we don't have privileges on. >
Thanks Bertrand, yeah this seems like a valid issue, and I agree we need to reread the subscription after acquiring the object lock. -- Regards, Dilip Kumar Google
