On Wed, Nov 22, 2023 at 10:02 AM Zhijie Hou (Fujitsu) <houzj.f...@fujitsu.com> wrote: > > On Tuesday, November 21, 2023 5:33 PM shveta malik <shveta.ma...@gmail.com> > wrote: > > > > > > v37 fails to apply to HEAD due to a recent commit e83aa9f92fdd, rebased the > > patches. PFA v37_2 patches. > > Thanks for updating the patches. > > I'd like to discuss one issue related to the correct handling of failover flag > when executing ALTER SUBSCRIPTION SET (slot_name = 'new_slot')". > > Since the command intends to use a new slot on the primary, the new slot needs > to reflect the "failover" state that the subscription currently has. If the > failoverstate of the Subscription is LOGICALREP_FAILOVER_STATE_ENABLED, then I > can reset it to LOGICALREP_FAILOVER_STATE_PENDING and allow the apply worker > to > handle it the way it is handled today (just like two_phase handling). > > But if the failoverstate is LOGICALREP_FAILOVER_STATE_DISABLED, the original > idea is to call walrcv_alter_slot and alter the slot from the "ALTER > SUBSCRIPTION" handling backend itself. This works if the slot is currently > disabled. But the " ALTER SUBSCRIPTION SET (slot_name = 'new_slot')" command > is > supported even if the subscription is enabled. If the subscription is enabled, > then calling walrcv_alter_slot() fails because the slot is still acquired by > apply worker. > > So, I am thinking do we need a new mechanism to change the failover flag to > false on an enabled subscription ? For example, we could call > walrcv_alter_slot > on startup of apply worker if AllTablesyncsReady(), for both true and false > values of failover flag. This way, every time apply worker is started, it > calls > walrcv_alter_slot to set the failover flag on the primary. >
I think for the false case, we need to execute walrcv_alter_slot() every time at the start of apply worker and it doesn't sound like an ideal way to achieve it. > Or we could just document that it is user's responsibility to match the > failover > property in case it changes the slot_name. > Personally, I think we should document this behavior instead of complicating the patch and the user anyway has a way to achieve it. -- With Regards, Amit Kapila.