Re: Logical Replication of sequences

Peter Smith <smithpb2250@gmail.com>

From: Peter Smith <smithpb2250@gmail.com>

To: vignesh C <vignesh21@gmail.com>

Cc: Masahiko Sawada <sawada.mshk@gmail.com>, shveta malik <shveta.malik@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>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Hou, Zhijie/侯志杰 <houzj.fnst@fujitsu.com>, "Jonathan S. Katz" <jkatz@postgresql.org>

Date: 2024-12-18T23:28:25Z

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 →

Doc: Add documentation for sequence synchronization.
- 55cefadde874 19 (unreleased) landed
Remove unused assignment in CREATE PUBLICATION grammar.
- bfb7419b0bbe 19 (unreleased) landed
Add seq_sync_error_count to subscription statistics.
- f6a4c498dcf6 19 (unreleased) landed
Fix few issues in commit 5509055d69.
- 5a4eba558aa7 19 (unreleased) landed
Add sequence synchronization for logical replication.
- 5509055d6956 19 (unreleased) landed
Add worker type argument to logical replication worker functions.
- 3e8e05596a02 19 (unreleased) landed
Introduce "REFRESH SEQUENCES" for subscriptions.
- f0b3573c3aac 19 (unreleased) landed
Refactor logical worker synchronization code into a separate file.
- 41c674d2e31e 19 (unreleased) landed
Standardize use of REFRESH PUBLICATION in code and messages.
- 2436b8c047ff 19 (unreleased) landed
Add "ALL SEQUENCES" support to publications.
- 96b378497346 19 (unreleased) landed
Expose sequence page LSN via pg_get_sequence_data.
- b93172ca59f4 19 (unreleased) landed
Resume conflict-relevant data retention automatically.
- 0d48d393d465 19 (unreleased) cited
Post-commit review fixes for 228c370868.
- 1f7e9ba3ac4e 19 (unreleased) cited
Generate GUC tables from .dat file
- 63599896545c 19 (unreleased) cited

Hi Vignesh,

Here are some review comments for the patch v20241211-0003.

======
src/backend/replication/logical/syncutils.c

1.
+typedef enum
+{
+ SYNC_RELATIONS_STATE_NEEDS_REBUILD,
+ SYNC_RELATIONS_STATE_REBUILD_STARTED,
+ SYNC_RELATIONS_STATE_VALID,
+} SyncingRelationsState;
+

Even though the patch's intent was only to "move" this from
tablsync.c, this enum probably deserved a comment describing its
purpose.

~~~

2.
+List    *table_states_not_ready = NIL;

Maybe it is convenient to move this, but there is something about this
list being exposed out of a "utils" module back to the tablesync.c
module that seems a bit strange. Would it make more sense for this to
be the other way around e.g. declared in the tablesync.c but exposed
to the syncutils.c? (and then similarly in subsequent patch 0004 the
sequence_states_not_ready would belong in the sequencesync.c)

~~~

3.
+/*
+ * Process possible state change(s) of tables that are being synchronized.
+ */
+void
+SyncProcessRelations(XLogRecPtr current_lsn)
+{

IIUC there was a deliberate effort to rename some comments and
functions to say "relations" instead of "tables". AFAICT that was done
to encompass the SEQUENCES which can fit under the umbrella of
"relations". Anyway, it becomes confusing sometimes when there is a
mismatch. For example, here is a function (relations) with a function
comment (tables).

~~~

4.
+/*
+ * Common code to fetch the up-to-date sync state info into the static lists.
+ *
+ * Returns true if subscription has 1 or more tables, else false.
+ *
+ * Note: If this function started the transaction (indicated by the parameter)
+ * then it is the caller's responsibility to commit it.
+ */
+bool
+FetchRelationStates(bool *started_tx)

Here is another place where the function name is "relations", but the
function comment refers to "tables".

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