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>, Shlok Kyal <shlok.kyal.oss@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, Peter Eisentraut <peter@eisentraut.org>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Tomas Vondra <tomas.vondra@enterprisedb.com>, 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-08-20T01:56:52Z
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, Here are my review comments for the latest patchset

v20240819-0001. No changes. No comments.
v20240819-0002. No changes. No comments.
v20240819-0003. See below.
v20240819-0004. See below.
v20240819-0005. No changes. No comments.

///////////////////////

PATCH v20240819-0003

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

3.1.
+typedef enum
+{
+ SYNC_RELATION_STATE_NEEDS_REBUILD,
+ SYNC_RELATION_STATE_REBUILD_STARTED,
+ SYNC_RELATION_STATE_VALID,
+} SyncingRelationsState;
+
+static SyncingRelationsState relation_states_validity =
SYNC_RELATION_STATE_NEEDS_REBUILD;

There is some muddle of singular/plural names here. The
typedef/values/var should all match:

e.g. It could be like:
SYNC_RELATION_STATE_xxx --> SYNC_RELATION_STATES_xxx
SyncingRelationsState --> SyncRelationStates

But, a more radical change might be better.

typedef enum
{
RELATION_STATES_SYNC_NEEDED,
RELATION_STATES_SYNC_STARTED,
RELATION_STATES_SYNCED,
} SyncRelationStates;

~~~

3.2. GENERAL refactoring

I don't think all of the functions moved into syncutil.c truly belong there.

This new module was introduced to be for common/util functions for
tablesync and sequencesync, but with each patchset, it has been
sucking in more and more functions that maybe do not quite belong
here.

For example, AFAIK these below have logic that is *solely* for TABLES
(not for SEQUENCES). Perhaps it was convenient to dump them here
because they are statically called, but I felt they still logically
belong in tablesync.c:
- process_syncing_tables_for_sync(XLogRecPtr current_lsn)
- process_syncing_tables_for_apply(XLogRecPtr current_lsn)
- AllTablesyncsReady(void)

~~~

3.3.
+static bool
+FetchRelationStates(bool *started_tx)
+{

If this function can remain static then the name should change to be
like fetch_table_states, right?

======
src/include/replication/worker_internal.h

3.4.
+extern bool wait_for_relation_state_change(Oid relid, char expected_state);

If this previously static function will be exposed now (it may not
need to be if some other functions are returned tablesync.c) then the
function name should also be changed, right?

////////////////////////

PATCH v20240819-0004

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

4.1 GENERAL refactoring

(this is similar to review comment #3.2 above)

Functions like below have logic that is *solely* for SEQUENCES (not
for TABLES). I felt they logically belong in sequencesync.c, not here.
- process_syncing_sequences_for_apply(void)

~~~

FetchRelationStates:
nit - the comment change about "not-READY tables" (instead of
relations) should be already in patch 0003.

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