On Tue, Apr 22, 2025 at 3:00 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Mon, Apr 21, 2025 at 8:44 AM Zhijie Hou (Fujitsu) > <houzj.f...@fujitsu.com> wrote: > > > > On Sat, Apr 19, 2025 at 2:19 AM Masahiko Sawada wrote: > > > > > > On Tue, Apr 8, 2025 at 10:14 PM Zhijie Hou (Fujitsu) > > > <houzj.f...@fujitsu.com> wrote: > > > > > > > > > > > > ---------- > > > > Approach 2 > > > > ---------- > > > > > > > > Instead of disallowing the use of two-phase and failover together, a > > > > more > > > > flexible strategy could be only restrict failover for slots with > > > > two-phase > > > > enabled when there's a possibility of existing prepared transactions > > > > before > > > the > > > > two_phase_at that are not yet replicated. During slot creation with > > > two-phase > > > > and failover, we could check for any decoded prepared transactions when > > > > determining the decoding start point (DecodingContextFindStartpoint). > > > > For > > > > subsequent attempts to alter failover to true, we ensure that > > > > two_phase_at is > > > > less than restart_lsn, indicating that all prepared transactions have > > > > been > > > > committed and replicated, thus the bug would not happen. > > > > > > > > pros: > > > > > > > > This method minimizes restrictions for users. Especially during slot > > > > creation > > > > with (two_phase=on, failover=on), as it’s uncommon for transactions to > > > prepare > > > > during consistent snapshot creation, the restriction becomes almost > > > > unnoticeable. > > > > > > I think this approach can work for the transactions that are prepared > > > while the slot is created. But if I understand the problem correctly, > > > while the initial table sync is performing, the slot's two_phase is > > > still false, so we need to deal with the transactions that are > > > prepared during the initial table sync too. What do you think? > > > > > > > Yes, I agree that we need to restrict this case too. Given that we haven't > > started decoding when setting two_phase=true during CreateDecodingContext() > > after tablesync, we could check prepared transactions afterwards during > > decoding. This could involve reporting an ERROR when skipping a prepared > > transaction during decoding if its prepare LSN is less than two_phase_at. > > > > It will make it difficult for users to detect it as this happens at a > later point of time. > > > Alternatively, a simpler method would be to prevent this situation entirely > > during the CREATE SUBSCRIPTION command. For example, we could restrict slots > > created with failover set to true and twophase is later modified to true > > after > > tablesync. Although the simpler check is more user-visible, it may offer > > less > > flexibility. > > > > I agree with your point, but OTOH, I am also afraid of adding too many > smart checks in the back-branch. If we follow what you say here, then > users have the following ways in PG17 to enable both failover and > two_phase. (a) During Create Subscription, users can set both > 'failover' and 'two_phase', if 'copy_data' is false, or (b), if > 'copy_data' is true, during Create Subscription, then users can enable > 'two_phase' and wait for it to be enabled. Then use Alter Subscription > to set 'failover'.
Yet another idea would be to disallow enabling both two_phase and failover at CREATE SUBSCRIPTION regardless of copy_data value and to add check when enabling failover for the two_phase-enabled-slots. For example, users who want to enable both need to do two steps: 1. CREATE SUBSCRIPTION with two_phase = true and failover = false. 2. ALTER SUBSCRIPTION with failover = true. At ALTER SUBSCRIPTION with failover = true, the subscriber checks if the two_phase states is ready (and possibly check if the slot's two_phase has been enabled too), otherwise raises an ERROR. Then, when the publisher enables the failover for the two_phase-enabled-slot up on walrcv_alter_slot() request, it checks the slot's restart_lsn has passed slot's two_phase_at, otherwise raise an error with the message like "the slot need to consume change upto %X/%X to enable failover". Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
