Horton

Re: Synchronizing slots from primary to standby

Peter Smith <smithpb2250@gmail.com>

View thread View on postgr.es
From: Peter Smith <smithpb2250@gmail.com>
To: shveta malik <shveta.malik@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Bertrand Drouvot <bertranddrouvot.pg@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>, Dilip Kumar <dilipbalaut@gmail.com>, Nisha Moond <nisha.moond412@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>, Peter Eisentraut <peter.eisentraut@enterprisedb.com>, Bruce Momjian <bruce@momjian.us>, Ashutosh Sharma <ashu.coek88@gmail.com>, Andres Freund <andres@anarazel.de>, pgsql-hackers <pgsql-hackers@postgresql.org>, Ajin Cherian <itsajin@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: 2024-01-23T04:14:42Z
Lists: pgsql-hackers 
Here are some review comments for v65-0002

======
0. General - GUCs in messages

I think it would be better for the GUC names to all be quoted. It's
not a rule (yet), but OTOH it seems to be the consensus most people
want. See [1].

This might impact the following messages:

0.1
+ ereport(ERROR,
+ errmsg("could not fetch primary_slot_name \"%s\" info from the"
+    " primary server: %s", PrimarySlotName, res->err));

SUGGESTION
errmsg("could not fetch primary server slot \"%s\" info from the
primary server: %s", ...)
errhint("Check if \"primary_slot_name\" is configured correctly.");

~~~

0.2
+ if (!tuplestore_gettupleslot(res->tuplestore, true, false, tupslot))
+ elog(ERROR, "failed to fetch primary_slot_name tuple");

SUGGESTION
elog(ERROR, "failed to fetch tuple for the primary server slot
specified by \"primary_slot_name\"");

~~~

0.3
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ /* translator: second %s is a GUC variable name */
+ errdetail("The primary server slot \"%s\" specified by %s is not valid.",
+   PrimarySlotName, "primary_slot_name"));

/specified by %s/specified by \"%s\"/

~~~

0.4
+ ereport(ERROR,
+ /* translator: %s is a GUC variable name */
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ errhint("%s must be defined.", "primary_slot_name"));

/%s must be defined./\"%s\" must be defined./

~~~

0.5
+ ereport(ERROR,
+ /* translator: %s is a GUC variable name */
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ errhint("%s must be enabled.", "hot_standby_feedback"));

/%s must be enabled./\"%s\" must be enabled./

~~~

0.6
+ ereport(ERROR,
+ /* translator: %s is a GUC variable name */
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ errhint("wal_level must be >= logical."));

errhint("\"wal_level\" must be >= logical."))

~~~

0.7
+ if (PrimaryConnInfo == NULL || strcmp(PrimaryConnInfo, "") == 0)
+ ereport(ERROR,
+ /* translator: %s is a GUC variable name */
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ errhint("%s must be defined.", "primary_conninfo"));

/%s must be defined./\"%s\" must be defined./

~~~

0.8
+ ereport(ERROR,
+
+ /*
+ * translator: 'dbname' is a specific option; %s is a GUC variable
+ * name
+ */
+ errmsg("exiting from slot synchronization due to bad configuration"),
+ errhint("'dbname' must be specified in %s.", "primary_conninfo"));

/must be specified in %s./must be specified in \"%s\"./

~~~

0.9
+ ereport(LOG,
+ errmsg("skipping slot synchronization"),
+ errdetail("enable_syncslot is disabled."));

errdetail("\"enable_syncslot\" is disabled."));

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

1.
+/* Min and Max sleep time for slot sync worker */
+#define MIN_WORKER_NAPTIME_MS  200
+#define MAX_WORKER_NAPTIME_MS  30000 /* 30s */
+
+/*
+ * Sleep time in ms between slot-sync cycles.
+ * See wait_for_slot_activity() for how we adjust this
+ */
+static long sleep_ms = MIN_WORKER_NAPTIME_MS;

These all belong together, so I think they share a combined comment like:

SUGGESTION
The sleep time (ms) between slot-sync cycles varies dynamically
(within a MIN/MAX range) according to slot activity. See
wait_for_slot_activity() for details.

~~~

2. update_and_persist_slot

+ /* First time slot update, the function must return true */
+ if(!local_slot_update(remote_slot))
+ elog(ERROR, "failed to update slot");

Missing whitespace after 'if'

~~~

3. synchronize_one_slot

+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("exiting from slot synchronization because same"
+    " name slot \"%s\" already exists on standby",
+    remote_slot->name),
+ errdetail("A user-created slot with the same name as"
+   " failover slot already exists on the standby."));


3a.
/on standby/on the standby/

~

3b.
Now the errmsg is changed, the errdetail doesn't seem so useful. Isn't
it repeating pretty much the same information as in the errmsg?

======
src/backend/replication/walsender.c

4. GetStandbyFlushRecPtr

 /*
- * Returns the latest point in WAL that has been safely flushed to disk, and
- * can be sent to the standby. This should only be called when in recovery,
- * ie. we're streaming to a cascaded standby.
+ * Returns the latest point in WAL that has been safely flushed to disk.
+ * This should only be called when in recovery.
+ *

Since it says "This should only be called when in recovery", should
there also be a check for that (e.g. RecoveryInProgress) in the added
Assert?

======
src/include/replication/walreceiver.h

5.
 typedef char *(*walrcv_identify_system_fn) (WalReceiverConn *conn,
  TimeLineID *primary_tli);
+/*
+ * walrcv_get_dbname_from_conninfo_fn
+ *
+ * Returns the dbid from the primary_conninfo
+ */
+typedef char *(*walrcv_get_dbname_from_conninfo_fn) (const char *conninfo);

It looks like a blank line that previously existed has been lost.

======
[1] https://www.postgresql.org/message-id/CAHut%2BPsf3NewXbsFKY88Qn1ON1_dMD6343MuWdMiiM2Ds9a_wA%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia

Commits

  1. Doc: Add the new section "Logical Replication Failover".

  2. Fix the review comments and a bug in the slot sync code.

  3. Fix a test in failover slots regression test.

  4. Fix the intermittent buildfarm failures in 040_standby_failover_slots_sync.

  5. Split XLogCtl->LogwrtResult into separate struct members

  6. Ensure that the sync slots reach a consistent state after promotion without losing data.

  7. Introduce a new GUC 'standby_slot_names'.

  8. Fix BF failure introduced by commit b3f6b14cf4.

  9. Fixups for commit 93db6cbda0.

  10. Fix BF failure in commit 93db6cbda0.

  11. Add a new slot sync worker to synchronize logical slots.

  12. Improve ERROR/LOG messages added by commits ddd5f4f54a and 7a424ece48.

  13. Disable autovacuum on primary in 040_standby_failover_slots_sync test.

  14. Fix the incorrect format specifier used in commit 7a424ece48.

  15. Change the LOG level in 040_standby_failover_slots_sync.pl to DEBUG2.

  16. Add more LOG and DEBUG messages for slot synchronization.

  17. Another try to fix BF failure introduced in commit ddd5f4f54a.

  18. Fix BF introduced in commit ddd5f4f54a.

  19. Add a slot synchronization function.

  20. Clean-ups for 776621a5e4 and 7329240437.

  21. Enhance libpqrcv APIs to support slot synchronization.

  22. Add a failover option to subscriptions.

  23. Allow setting failover property in the replication command.

  24. Allow to enable failover property for replication slots via SQL API.

  25. Add macros for looping through a List without a ListCell.

  26. Track conflict_reason in pg_replication_slots.

  27. Allow upgrades to preserve the full subscription's state.

  28. Add support for incremental backup.

  29. Simplify some logic in CreateReplicationSlot()

  30. Fix uninitialized access to InitialRunningXacts during decoding after ERROR.

  31. Flexible options for CREATE_REPLICATION_SLOT.

  32. Empty search_path in logical replication apply worker and walsender.

  33. Fix oldest xmin and LSN computation across repslots after advancing

  34. Require C99 (and thus MSCV 2013 upwards).