Horton

Re: Logical Replication of sequences

Peter Smith <smithpb2250@gmail.com>

View thread
From: Peter Smith <smithpb2250@gmail.com>
To: vignesh C <vignesh21@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-15T06:57:00Z
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

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"

======
Kind Regards,
Peter Smith.
Fujitsu Australia