On Thu, Oct 26, 2023 at 6:08 PM Hayato Kuroda (Fujitsu) <kuroda.hay...@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. > 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? regards, Ajin Cherian Fujitsu Australia
