On Thu, Jul 2, 2026 at 5:30 PM Dilip Kumar <[email protected]> wrote: > > On Thu, Jul 2, 2026 at 4:48 PM Shlok Kyal <[email protected]> wrote: > > > > On Thu, 2 Jul 2026 at 13:39, Dilip Kumar <[email protected]> wrote: > > > > > > On Wed, Jul 1, 2026 at 6:55 PM Dilip Kumar <[email protected]> wrote: > > > > > > > > On Wed, Jul 1, 2026 at 1:16 PM shveta malik <[email protected]> > > > > wrote: > > > > > > > > > > On Wed, Jul 1, 2026 at 12:45 PM Dilip Kumar <[email protected]> > > > > > wrote: > > > > > > > > > > > > Thanks I have fixed these comments and also merged the docs. > > > > > > > > > > Thanks. Sharing the version with pgindent run on v61. > > > > > > > > Here is the updated patch version. It fixes offlist review comments > > > > provided by Amit and Shveta, bumps the catversion and adds authors and > > > > reviewers details. > > > > > > I rebased the remaining patches on top of HEAD. So far, I have run > > > pgindent and completed the doc merge for 0001. The 0002 and 0003 are > > > just rebased. > > > > Hi Dilip, > > > > While testing I hit the following assert: > > + if (OidIsValid(relid)) > > + { > > + /* Existing table from upgrade */ > > + Assert(IsBinaryUpgrade); > > + } > > > > Steps to reproduce: > > 1. Suppose initially we have a logical replication setup and the > > subscriber is created with 'conflict_log_destination = log' > > 2. Now we have two psql sessions of the subscriber node. Attach GDB to > > first session and add breakpoint to line: > > AlterSubscription->LockSharedObject(SubscriptionRelationId, subid, 0, > > AccessExclusiveLock); > > 3.Execute 'ALTER SUBSCRIPTION sub1 SET (conflict_log_destination = > > 'all')' on the first session. It will stop at the breakpoint. > > 4. Now execute 'ALTER SUBSCRIPTION sub1 SET (conflict_log_destination > > = 'all');' on the second session. The execution on the second session > > is successful and a conflict log table is created. > > 5. Continue the execution of the first session. This will hit the > > above Assert condition. > > > > I think this happens because for the first session, we have the stale > > copy of 'sub'. > > While the first session is stopped at the breakpoint, the second > > session successfully executes ALTER SUBSCRIPTION and creates the > > conflict log table. When the first session resumes, it still sees the > > old value of old_dest (i.e CONFLICT_LOG_DEST_LOG), so > > 'opts.conflictlogdest != old_dest' evaluates to true and > > 'alter_sub_conflict_log_dest()' is invoked. > > Since the second session has already created the conflict log table > > and 'want_table' is true, the code finds an existing conflict log > > table '(OidIsValid(relid))', causing the assert to fire. > > In short, if I understand correctly, the issue is that altering a > subscription that tries to create the table might hit this assert > because we assume that if we are switching from 'log' to 'table' or > 'all', there should not be an existing table except in binary upgrade > mode. This happens because we read the destination information from > subscription cache while checking the table status in real time. In > fact in other cases, it might cause alter subscription to error out if > the table is created while we are creating the table. Therefore, we > need to handle concurrent table creation during ALTER SUBSCRIPTION. > One option is: if the table exists and it's not a binary upgrade, > should we just error out? >
No, I feel it should be Assert only. IMO, we should re-fetch 'sub' beofre CLT creation or move first 'sub' fetching itself post LockSharedObject if there is nothing blocking it. Thoughts? thanks Shveta
