Re: Logical Replication of sequences

vignesh C <vignesh21@gmail.com>

From: vignesh C <vignesh21@gmail.com>
To: Peter Smith <smithpb2250@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Masahiko Sawada <sawada.mshk@gmail.com>, Shlok Kyal <shlok.kyal.oss@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, Peter Eisentraut <peter@eisentraut.org>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Euler Taveira <euler@eulerto.com>, Michael Paquier <michael@paquier.xyz>, "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, "Jonathan S. Katz" <jkatz@postgresql.org>
Date: 2025-05-16T08:46:19Z
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, 15 May 2025 at 12:27, Peter Smith <smithpb2250@gmail.com> wrote:
>
> Hi Vignesh.
>
> Some minor review comments for the patches in set v20250514.
>
> ======
>
> Patch 0001.
>
> 1.1
> For function 'pg_sequence_state', the DOCS call the 2nd parameter
> 'sequence_name', but the pg_proc.dat file calls it 'seq_name'. Should
> these be made the same?
>
> ////////////////////
>
> Patch 0004.
>
> pg_sequence_state:
>
> 4.1
>
> - errmsg("sequence \"%s.%s\" does not exist",
> + errmsg("logical replication sequence \"%s.%s\" does not exist",
>
> Why isn't this change already be done in an early patch when this
> function was first implemented?
>
> ~~~
>
> copy_sequences:
>
> 4.2
>
> +/*
> + * Copy existing data of sequnces from the publisher.
> + *
>
> Typo: "sequnces"
>
> ~~~
>
> 4.3
> +{
> + int total_seq = list_length(remotesequences);
> + int curr_seq = 0;
> +
> +/*
> + * We batch synchronize multiple sequences per transaction, because the
> + * alternative of synchronizing each sequence individually incurs overhead of
> + * starting and committing transactions repeatedly. On the other hand, we want
> + * to avoid keeping this batch transaction open for extended periods so it is
> + * currently limited to 100 sequences per batch.
> + */
> +#define MAX_SEQUENCES_SYNC_PER_BATCH 100
>
> Wrong indent for block comment.
>
> ~~~
>
> 4.4
> + if (res->status != WALRCV_OK_TUPLES)
> + ereport(ERROR,
> + errcode(ERRCODE_CONNECTION_FAILURE),
> + errmsg("could not receive list of sequences information from the
> publisher: %s",
> +    res->err));
>
> Should this say /sequences information/sequence information/
>
> ~~~
>
> 4.5
> + ereport(LOG,
> + errmsg("Logical replication sequence synchronization - total
> unsynchronized: %d, attempted in this batch: %d; succeeded in this
> batch: %d; mismatched in this batch: %d for subscription: \"%s\"",
> +    total_seq, batch_seq_count, batch_success_count,
> +    batch_mismatch_count, get_subscription_name(subid, false)));
> +
>
>
> This errmsg seems backwards. I think it should be expressed like the
> other one immediately above. Also I think the information can be made
> shorter -- e.g. no need to say "in this batch" multiple times.
>
> SUGGESTION
> "Logical replication sequence synchronization for subscription \"%s\"
> - total unsynchronized: %d; batch #%d = %d attempted, %d succeeded, %d
> mismatched"

Thanks for the comments, these are handled in the attached v20250516
version patch.

Regards,
Vignesh