Horton

Re: Logical Replication of sequences

Amit Kapila <amit.kapila16@gmail.com>

View thread
From: Amit Kapila <amit.kapila16@gmail.com>
To: Peter Smith <smithpb2250@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>, Chao Li <li.evan.chao@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, Dilip Kumar <dilipbalaut@gmail.com>, shveta malik <shveta.malik@gmail.com>, Shlok Kyal <shlok.kyal.oss@gmail.com>, Nisha Moond <nisha.moond412@gmail.com>, Peter Eisentraut <peter@eisentraut.org>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Euler Taveira <euler@eulerto.com>, Michael Paquier <michael@paquier.xyz>, "Jonathan S. Katz" <jkatz@postgresql.org>
Date: 2025-10-31T04:40:52Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Doc: Add documentation for sequence synchronization.

  2. Remove unused assignment in CREATE PUBLICATION grammar.

  3. Add seq_sync_error_count to subscription statistics.

  4. Fix few issues in commit 5509055d69.

  5. Add sequence synchronization for logical replication.

  6. Add worker type argument to logical replication worker functions.

  7. Introduce "REFRESH SEQUENCES" for subscriptions.

  8. Refactor logical worker synchronization code into a separate file.

  9. Standardize use of REFRESH PUBLICATION in code and messages.

  10. Add "ALL SEQUENCES" support to publications.

  11. Expose sequence page LSN via pg_get_sequence_data.

  12. Resume conflict-relevant data retention automatically.

  13. Post-commit review fixes for 228c370868.

  14. Generate GUC tables from .dat file

On Fri, Oct 31, 2025 at 7:34 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> ======
> .../replication/logical/sequencesync.c
>
> 2.
> + * A single sequencesync worker is responsible for synchronizing all sequences
> + * in INIT state in pg_subscription_rel. It begins by retrieving the list of
> + * sequences flagged for synchronization. These sequences are then processed
> + * in batches, allowing multiple entries to be synchronized within a single
> + * transaction. The worker fetches the current sequence values and page LSNs
> + * from the remote publisher, updates the corresponding sequences on the local
> + * subscriber, and finally marks each sequence as READY upon successful
> + * synchronization.
> + *
>
> Those first 2 sentences seem repetitive because AFAIK "in INIT state"
> and "flagged for synchronization" are exactly the same thing.
>
> SUGGESTION
> A single sequencesync worker is responsible for synchronizing all
> sequences. It begins by retrieving the list of sequences that are
> flagged for needing synchronization (e.g. those with INIT state).
>

/e.g/i.e. We don't have multiple such states, so let's be specific.

> ~~~
>
> 4.
> Now that this function is in 2 parts, I think each part should be
> clearly identified with comments, something like:
>
> PART 1:
> /*
>  * 1. Extract sequence information from the tuple slot received from the
>  * publisher
>  */
>
> PART 2:
> /*
>  * 2. Compare the remote sequence definition to the local sequence definition,
>  * and report any discrepancies.
>  */
>

I don't see the need for such explicit comments as the same is
apparent from the code.

> ~~~
>
> 5.
> + seqinfo_local = (LogicalRepSequenceInfo *) list_nth(seqinfos, seqidx);
> + seqinfo_local->found_on_pub = true;
> + *seqinfo = seqinfo_local;
>
> Is this separate `seqinfo_local` variable needed? It seems always
> unconditionally assigned to the parameter, so you might as well just
> do without the extra variable. Maybe just rename the parameter as
> `seqinfo_local`?
>

We can do without a local variable as well but it appears neat to
modify and use local variable. I think it is a matter of personal
choice, so either way is fine but I would prefer using local variable
for this.

>
> 12.
> There is still a yet-to-be-implemented test combination as previously
> reported [1, comment #3], right?
>

I don't think adding similar negative tests adds value. So, we can skip those.

-- 
With Regards,
Amit Kapila.