Horton

RE: Synchronizing slots from primary to standby

Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>

View thread View on postgr.es
From: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Masahiko Sawada <sawada.mshk@gmail.com>, shveta malik <shveta.malik@gmail.com>, Bertrand Drouvot <bertranddrouvot.pg@gmail.com>, Peter Smith <smithpb2250@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-10T12:23:14Z
Lists: pgsql-hackers

Attachments

On Wednesday, January 10, 2024 2:26 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> 
> On Tue, Jan 9, 2024 at 5:44 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
> wrote:
> >
> comments on 0002

Thanks for the comments !

> 
> 1.
> +/* Worker's nap time in case of regular activity on the primary server */
> +#define WORKER_DEFAULT_NAPTIME_MS                   10L /* 10 ms */
> +
> +/* Worker's nap time in case of no-activity on the primary server */
> +#define WORKER_INACTIVITY_NAPTIME_MS                10000L /* 10 sec
> */
> 
> Instead of directly switching between 10ms to 10s shouldn't we increase the
> nap time gradually?  I mean it can go beyond 10 sec as well but instead of
> directly switching from 10ms to 10 sec we can increase it every time with some
> multiplier and keep a max limit up to which it can grow.  Although we can reset
> back to 10ms directly as soon as we observe some activity.

Agreed. I changed the strategy similar to what we do in the walsummarizer.

> 
> 2.
> SlotSyncWorkerCtxStruct add this to typedefs. list file
> 
> 3.
> +/*
> + * Update local slot metadata as per remote_slot's positions  */ static
> +void local_slot_update(RemoteSlot *remote_slot) {
> +Assert(MyReplicationSlot->data.invalidated == RS_INVAL_NONE);
> +
> + LogicalConfirmReceivedLocation(remote_slot->confirmed_lsn);
> + LogicalIncreaseXminForSlot(remote_slot->confirmed_lsn,
> +    remote_slot->catalog_xmin);
> + LogicalIncreaseRestartDecodingForSlot(remote_slot->confirmed_lsn,
> +   remote_slot->restart_lsn);
> +}
> 
> IIUC on the standby we just want to overwrite what we get from primary no? If
> so why we are using those APIs that are meant for the actual decoding slots
> where it needs to take certain logical decisions instead of mere overwriting?

I think we don't have a strong reason to use these APIs, but it was convenient to
use these APIs as they can take care of updating the slots info and will call
functions like, ReplicationSlotsComputeRequiredXmin,
ReplicationSlotsComputeRequiredLSN internally. Or do you prefer directly overwriting
the fields and call these manually ?

> 
> 4.
> +/*
> + * Helper function for drop_obsolete_slots()
> + *
> + * Drops synced slot identified by the passed in name.
> + */
> +static void
> +drop_synced_slots_internal(const char *name, bool nowait)
> 
> Suggestion to add one line to explain no wait in the header

The 'nowait' flag is not necessary now, so removed.

> 
> 5.
> +/*
> + * Helper function to check if local_slot is present in remote_slots list.
> + *
> + * It also checks if logical slot is locally invalidated i.e.
> +invalidated on
> + * the standby but valid on the primary server. If found so, it sets
> + * locally_invalidated to true.
> + */
> 
> Instead of saying "but valid on the primary server" better to mention it in the
> remote_slots list, because here this function is just checking the remote_slots
> list regardless of whether the list came from.  Mentioning primary seems like it
> might fetch directly from the primary in this function so this is a bit confusing.

Adjusted.

> 
> 6.
> +/*
> + * Check that all necessary GUCs for slot synchronization are set
> + * appropriately. If not, raise an ERROR.
> + */
> +static void
> +validate_slotsync_parameters(char **dbname)
> 
> 
> The function name just says 'validate_slotsync_parameters' but it also gets the
> dbname so I think it better we change the name accordingly also instead of
> passing dbname as a parameter just return it directly.
> There
> is no need to pass this extra parameter and make the function return void.

Renamed.

> 
> 7.
> + tupslot = MakeSingleTupleTableSlot(res->tupledesc,
> + &TTSOpsMinimalTuple); tuple_ok =
> + tuplestore_gettupleslot(res->tuplestore, true, false, tupslot);
> + Assert(tuple_ok); /* It must return one tuple */
> 
> Comments say 'It must return one tuple' but asserting just for at least one tuple
> shouldn't we enhance assert so that it checks that we got exactly one tuple?

Changed to use tuplestore_tuple_count.

> 
> 8.
> /* No need to check further, return that we are cascading standby */
> + *am_cascading_standby = true;
> 
> we are not returning immediately we are just setting am_cascading_standby to
> true so adjust comments accordingly

Adjusted.

> 
> 9.
> + /* No need to check further, return that we are cascading standby */
> + *am_cascading_standby = true; } else {
> + /* We are a normal standby. */
> 
> Single-line comments do not follow the uniform pattern for the full stop, either
> use a full stop for all single-line comments or none, at least follow the same rule
> in a file or nearby comments.

Adjusted.

> 
> 10.
> + errmsg("exiting from slot synchronization due to bad configuration"),
> + errhint("%s must be defined.", "primary_slot_name"));
> 
> Why we are using the constant string "primary_slot_name" as a variable in this
> error formatting?

It was suggested to make it friendly to the translator, as the GUC doesn't needs to be translated
and it can avoid adding multiple similar message to be translated.

> 
> 11.
> + /*
> + * Hot_standby_feedback must be enabled to cooperate with the physical
> + * replication slot, which allows informing the primary about the xmin
> + and
> + * catalog_xmin values on the standby.
> 
> I do not like capitalizing the first letter of the 'hot_standby_feedback' which is a
> GUC parameter

Changed.

Here is the V59 patch set which addressed above comments and comments from Peter[1].

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

Best Regards,
Hou zj

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).