Re: [PATCH] Support automatic sequence replication

> On Mar 13, 2026, at 15:13, Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote:
>
> On Monday, March 9, 2026 11:13 AM shveta malik <shveta.malik@gmail.com> wrote:
>>
>> No major concerns on 001, just a few trivial things. Do these only if you feel
>> okay about these.
>>
>
> Thanks for the reviews. I've updated the patch set addressing all comments.
>
> In 0001, aside from addressing comments in [1][2][3][4], I've changed the
> logic to delay updating the page LSN for each sequence in
> pg_subscription_rel. Frequent catalog updates would generate many
> invalidations, degrading the performance of the apply worker which
> relies on cached data from pg_subscription_rel.
>
> 0002 adds caching of sequence information for the current subscription in the
> sequence sync worker. The cache is invalidated immediately when
> pg_subscription_rel is modified. Concurrent changes to sequence names or
> namespaces are detected before synchronization, as the worker verifies the
> sequence data at sync time.
>
> 0003 (formerly 0002) modifies REFRESH SEQUENCES to synchronize sequence values
> directly without launching a worker.
>
> [1] https://www.postgresql.org/message-id/02EDB3D2-4E5A-4EDE-BADF-3DF62D707831%40gmail.com
> [2]
https://www.postgresql.org/message-id/OS9PR01MB12149E4614DA95963670772EEF579A%40OS9PR01MB12149.jpnprd01.prod.outlook.com
> [3] https://www.postgresql.org/message-id/CAJpy0uAmEkjsBS6RxPv9iDcK2kfJ5%3Dbq4Mq1zMCQtaYFoDfbbQ%40mail.gmail.com
> [4] https://www.postgresql.org/message-id/CAJpy0uC0T_tp62zxJN_2d_A%3DYpvf14ebjGFepckeJugW5OHOyA%40mail.gmail.com
>
> Best Regards,
> Hou zj
>
<v12-0003-Synchronize-sequences-directly-in-REFRESH-SEQUEN.patch><v12-0001-Support-automatic-sequence-replication.patch><v12-0002-Cache-sequence-information-in-the-sequence-sync-.patch>

I reviewed v12 again. 0001 looks good. A few comments on 0002 and 0003.

1 - 0002
```
+    /*
+     * Setup callback for syscache so that we know when something changes in
+     * the subscription relation state.
+     */
+    CacheRegisterSyscacheCallback(SUBSCRIPTIONRELMAP,
+                                  invalidate_syncing_sequence_infos,
+                                  (Datum) 0);
```

I wonder if SUBSCRIPTIONRELMAP should be SUBSCRIPTIONREL?

2 - 0003
```
+        /*
+         * Use the current memory context for synchronization. Since this should
+         * be short-lived command context that will be cleaned up automatically,
+         * we can simply assign it as the synchronization context.
+         */
+        SequenceSyncContext = CurrentMemoryContext;
```

I think it’s still better to create a memory context from CurrentMemoryContext for SequenceSyncContext, and destroy it
aftercopy_sequence. 

Today, this is only on the SQL command path, CurrentMemoryContext is supposed to be short-lived. But
AlterSubSyncSequences()might be called somewhere else in future, then we could not predict what would be
CurrentMemoryContext.

3 - 0003
```
    "output the wanring for the missing sequence regress_s4”);
```

Typo: wanring -> warning

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/