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-30T10:49:46Z
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

Attachments

On Thu, Oct 30, 2025 at 8:12 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> 3.
> +static void
> +report_sequence_errors(StringInfo insuffperm_seqs, StringInfo mismatched_seqs,
> +    StringInfo insuffperm_seqs)
>
> Perhaps this function should also have an assertion:
>
> Assert(insuffperm_seqs->len || mismatched_seqs->len || insuffperm_seqs->len);
>

I don't think such an assertion is helpful. As of now, this is called
from only one place where we ensure that one of the conditions
mentioned in assert is true but in future even if we call that without
any condition being true, it will just be a harmless call.

Other review comments:
===================
1. In copy_sequences, we start a transaction before getting the
changes from the publisher and keep using it till we copy all
sequences. If we don't need to retain lock while querying from a
publisher, then can't we even commit the xact before that and start a
new one for the copy_sequence part? Is it possible that we fetch the
required sequence information in LogicalRepSyncSequences()?

2. Isn't it better to add some comments as to why we didn't decide to
retain locks on required sequences while querying the publisher, and
rather re-validate them later?

3. To avoid a lot of parameters in get_remote_sequence_info and
validate_sequence(), can we have one function call say
get_and_validate_seq_info()? It will retrieve only the parameters
required by copy_sequence.

4.
validate_sequence()
{
...
+ /* Sequence was concurrently invalidated? */
+ if (!seqinfo->entry_valid)
+ {
+ ReleaseSysCache(tup);
+ return COPYSEQ_SKIPPED;
+ }

So, if the sequence is renamed concurrently, we get the cause as
SKIPPED but shouldn't it be COPYSEQ_MISMATCH? Is it possible to
compare the local sequence name with the remote name in
validate_sequence() after taking lock on sequence relation? If so, we
can return the state as COPYSEQ_MISMATCH.

Apart from the above, I have modified comments at various places in
the patch, see attached.

-- 
With Regards,
Amit Kapila.