Horton

RE: Synchronizing slots from primary to standby

Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>

View thread View on postgr.es
From: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
To: 'Ajin Cherian' <itsajin@gmail.com>
Cc: shveta malik <shveta.malik@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>, Peter Smith <smithpb2250@gmail.com>, "Drouvot, Bertrand" <bertranddrouvot.pg@gmail.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>, Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: 2023-10-31T01:46:09Z
Lists: pgsql-hackers 
Dear Ajin,

Thanks for your reply!

> On Thu, Oct 26, 2023 at 6:08 PM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > Dear Shveta,
> >
> > > PFA v25 patch set. The changes are:
> >
> > Thanks for making the patch! It seems that there are lots of comments, so
> > I can put some high-level comments for 0001.
> > Sorry if there are duplicated comments.
> >
> > 1.
> > The patch seemed not to consider the case that failover option between
> replication
> > slot and subscription were different. Currently slot option will be overwritten
> > by subscription one.
> >
> > Actually, I'm not sure what specification is better. Regarding the two_phase,
> > 2PC will be decoded only when the both of settings are true. Should we follow?
> >
> 
> But this is the intention, we want the Alter subscription to be able
> to change the failover behaviour
> of the slot.

I had not understood how two_phase is enabled. I found that slot->data.two_phase
is overwritten in CreateDecodingContext(), so the failover option now follows two_phase, right?
(I think the overwritten of data.failover should be also done at CreateDecodingContext()).

> > 2.
> > Currently ctx->failover is set only in the pgoutput_startup(), but not sure it is
> OK.
> > Can we change the parameter in CreateDecodingContext() or similar functions?
> >
> > Because IIUC it means that only slots which have pgoutput can wait. Other
> > output plugins must understand the change and set faliover flag as well -
> > I felt it is not good. E.g., you might miss to enable the parameter in
> test_decoding.
> >
> > Regarding the two_phase parameter, setting on plugin layer is good because it
> > quite affects the output. As for the failover, it is not related with the
> > content so that all of slots should be enabled.
> >
> > I think CreateDecodingContext or StartupDecodingContext() is the common
> path.
> > Or, is it the out-of-scope for now?
> 
> Currently, the failover field is part of the options list in the
> StartReplicationCmd. This gives some
> level of flexibility such that only plugins that are interested in
> this need to handle it. The options list
> is only deparsed by plugins.  If we move it to outside of the options list,
> this sort of changes the protocol for START_REPLICATION and will
> impact all plugins.
>  But I agree to your larger point that, we need to do it in such a way that
> other plugins do not unintentionally change the 'failover' behaviour
> of the originally created slot.
> Maybe I can code it in such a way that, only if the failover option is
> specified in the list of options
> passed as part of START_REPLICATION  will it change the original slot
> created 'failover' flag by adding
> another flag "failover_opt_given". Plugins that set this, will be able
> to change the failover flag of the slot,
> while plugins that do not support this will not set this and the
> failover flag of the created slot will remain.
> What do you think?

May be OK, but I came up with a corner case that external plugins have a streaming
option 'failover'. What should be? Has the option been reserved?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

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