Horton

Re: Logical Replication of sequences

vignesh C <vignesh21@gmail.com>

View thread
From: vignesh C <vignesh21@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Peter Smith <smithpb2250@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-30T16:30:15Z
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, 30 Oct 2025 at 16:20, Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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()?

Change it to fetching it 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?

Added comments for the same

> 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.

Modified

> 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.

Modified

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

Thanks, I have merged them.

The attached v20251030 version patch has the changes for the same.
I have also addressed Peter's comments from [1] and Hou-san's comments from [2].

[1] - https://www.postgresql.org/message-id/CAHut%2BPt73A8XNnBSiF5wEfqM4mT6ofVwW9BXw3oUjbGsaYQN_g%40mail.gmail.com
[2] - https://www.postgresql.org/message-id/TY4PR01MB16907CAA300EE41232FB0BA8194FAA%40TY4PR01MB16907.jpnprd01.prod.outlook.com

Regards,
Vignesh