Thread: Fix slot synchronization with two_phase decoding enabled

Fix slot synchronization with two_phase decoding enabled

From

"Zhijie Hou (Fujitsu)"

Date:

25 March, 08:35:32

Hi,

When testing the slot synchronization with logical replication slots that
enabled two_phase decoding, I found that transactions prepared before two-phase
decoding is enabled may fail to replicate to the subscriber after being
committed on a promoted standby following a failover.

To reproduce this issue, please follow these steps (also detailed in the
attached TAP test, v1-0001):

1. sub: create a subscription with (two_phase = false)
2. primary (pub): prepare a txn A.
3. sub: alter subscription set (two_phase = true) and wait for the logical slot to
   be synced to standby.
4. primary (pub): stop primary, promote the standby and let the subscriber use
   the promoted standby as publisher.
5. promoted standby (pub): COMMIT PREPARED A;
6. sub: the apply worker will report the following ERROR because it didn't
   receive the PREPARE.
   ERROR:  prepared transaction with identifier "pg_gid_16387_752" does not exist

I think the root cause of this issue is that the two_phase_at field of the
slot, which indicates the LSN from which two-phase decoding is enabled (used to
prevent duplicate data transmission for prepared transactions), is not
synchronized to the standby server.

In step 3, transaction A is not immediately replicated because it occurred
before enabling two-phase decoding. Thus, the prepared transaction should only
be replicated after decoding the final COMMIT PREPARED, as referenced in
ReorderBufferFinishPrepared(). However, due to the invalid two_phase_at on the
standby, the prepared transaction fails to send at that time.

This problem arises after the support for altering the two-phase option
(1462aad). Previously, two-phase was only enabled during slot creation, which
wait for all prepared transactions to finish (via ... -> SnapBuildWaitSnapshot)
before reaching a consistent state, so the bug didn't exist.

To address the issue, I propose synchronizing the two_phase_at field to the
standby server, as implemented in the attached patches. As mentioned
earlier,this bug exists only for PG18 so we do not need to back patch.

v1-0001: Tap test to reproduce the issue

    I place this patch as the first one so that reviewers can run it
    independently to reproduce the issue. Once the problem is thoroughly
    understood and the fix proves stable, the patches can be integrated.

v1-0002: Display two_phase_at in the pg_replication_slots view
v1-0003: Sync the two_phase_at field of a replication slot to the standby

An alternative approach might be modifying ALTER_REPLICATION_SLOT to wait
for all prepared transactions to commit when enabling two-phase. However, this
appears inelegant and less user friendly.

Best Regards,
Hou zj

On Thu, Mar 27, 2025 at 2:29 PM Amit Kapila wrote:

> 
> On Tue, Mar 25, 2025 at 12:1 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >
> > On Tue, Mar 25, 2025 at 11:05 AM Zhijie Hou (Fujitsu)
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > Hi,
> > >
> > > When testing the slot synchronization with logical replication slots that
> > > enabled two_phase decoding, I found that transactions prepared before
> two-phase
> > > decoding is enabled may fail to replicate to the subscriber after being
> > > committed on a promoted standby following a failover.
> > >
> > > To reproduce this issue, please follow these steps (also detailed in the
> > > attached TAP test, v1-0001):
> > >
> > > 1. sub: create a subscription with (two_phase = false)
> > > 2. primary (pub): prepare a txn A.
> > > 3. sub: alter subscription set (two_phase = true) and wait for the logical
> slot to
> > >    be synced to standby.
> > > 4. primary (pub): stop primary, promote the standby and let the subscriber
> use
> > >    the promoted standby as publisher.
> > > 5. promoted standby (pub): COMMIT PREPARED A;
> > > 6. sub: the apply worker will report the following ERROR because it didn't
> > >    receive the PREPARE.
> > >    ERROR:  prepared transaction with identifier "pg_gid_16387_752"
> does not exist
> > >
> > > I think the root cause of this issue is that the two_phase_at field of the
> > > slot, which indicates the LSN from which two-phase decoding is enabled
> (used to
> > > prevent duplicate data transmission for prepared transactions), is not
> > > synchronized to the standby server.
> > >
> > > In step 3, transaction A is not immediately replicated because it occurred
> > > before enabling two-phase decoding. Thus, the prepared transaction
> should only
> > > be replicated after decoding the final COMMIT PREPARED, as referenced
> in
> > > ReorderBufferFinishPrepared(). However, due to the invalid two_phase_at
> on the
> > > standby, the prepared transaction fails to send at that time.
> > >
> > > This problem arises after the support for altering the two-phase option
> > > (1462aad).
> > >
> 
> I suspect that this can happen in PG17 as well, but I need to think
> more about it to make a reproducible test case.

After further analysis, I was able to reproduce the same issue [1] in
PG 17.

However, since the proposed fix requires catalog changes and the issue is not a
security risk significant enough to justify changing the catalog in back
branches, we cannot back-patch the same solution. Following off-list
discussions with Amit and Kuroda-san, we are considering disallowing enabling
failover and two-phase decoding together for a replication slot, as suggested
in attachment 0002.

Another idea considered is to prevent the slot that enables two-phase decoding
from being synced to standby. IOW, this means displaying the failover field as
false in the view, if there is any possibility that transactions prepared
before the two_phase_at position exist (e.g., if restart_lsn is less than
two_phase_at). However, implementing this change would require additional
explanations to users for this new behavior, which seems tricky.

> 
> In the meantime, I have a few minor comments on the proposed patches:
> 1.
> ##################################################
>  # Promote the standby1 to primary. Confirm that:
>  # a) the slot 'lsub1_slot' and 'snap_test_slot' are retained on the new primary
>  # b) logical replication for regress_mysub1 is resumed successfully
> after failover
>  # c) changes can be consumed from the synced slot 'snap_test_slot'
>  ##################################################
> -$standby1->start;
>  $primary->wait_for_replay_catchup($standby1);
> 
>  # Capture the time before the standby is promoted
> @@ -885,6 +940,15 @@ $standby1->wait_for_catchup('regress_mysub1');
>  is($subscriber1->safe_psql('postgres', q{SELECT count(*) FROM tab_int;}),
>   "20", 'data replicated from the new primary');
> 
> +# Commit the prepared transaction
> +$standby1->safe_psql('postgres',
> + "COMMIT PREPARED 'test_twophase_slotsync';");
> +$standby1->wait_for_catchup('regress_mysub1');
> +
> +# Confirm that the prepared transaction is replicated to the subscriber
> +is($subscriber1->safe_psql('postgres', q{SELECT count(*) FROM tab_int;}),
> + "21", 'prepared data replicated from the new primary');
> 
> The commentary above this test should include information about
> verifying the replication of previously prepared transactions after
> promotion. Also, it would be better if confirm the commit prepared
> before confirming the new Insert is replicated after promotion.
> 
> 2.
> @@ -249,6 +250,7 @@ update_local_synced_slot(RemoteSlot *remote_slot,
> Oid remote_dbid,
>   SpinLockAcquire(&slot->mutex);
>   slot->data.restart_lsn = remote_slot->restart_lsn;
>   slot->data.confirmed_flush = remote_slot->confirmed_lsn;
> + slot->data.two_phase_at = remote_slot->two_phase_at;
> 
> Why do we need to update the two_phase_at here when the patch does it
> later in this function when local and remote values don't match?

Thanks for the comments, they have been addressed in V2.

[1]
- pub: created a slot 'sub' with two_phase=false, then prepared a transaction
- pub: after some activity, advanced the confirmed_flush_lsn of 'sub', so it is
  greater than prepared txn lsn.
- sub: create subscription with (slot_name='sub', create_slot=false, failover =
 true, two_phase=true, copy_data=false); two_phase_at will be set to the same
 as confirmed_flush_lsn which is greater than the prepared transaction.
- stop the primary and promote the standby.
- commit the prepared transaction on standby, the following error will be
  reported on subscriber:

LOG:  logical replication apply worker for subscription "sub2" has started
ERROR:  prepared transaction with identifier "pg_gid_16398_764" does not exist.


Best Regards,
Hou zj

Attachment

Re: Fix slot synchronization with two_phase decoding enabled

From

Amit Kapila

Date:

01 April, 06:20:03

On Mon, Mar 31, 2025 at 5:04 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Thu, Mar 27, 2025 at 2:29 PM Amit Kapila wrote:
>
> >
> > I suspect that this can happen in PG17 as well, but I need to think
> > more about it to make a reproducible test case.
>
> After further analysis, I was able to reproduce the same issue [1] in
> PG 17.
>
> However, since the proposed fix requires catalog changes and the issue is not a
> security risk significant enough to justify changing the catalog in back
> branches, we cannot back-patch the same solution.
>

Agreed. In the past, as in commit b6e39ca92e, we have backported a
catalog-modifying commit, but that is for a CVE. Users need to follow
manual steps as explained in 9.6.4 release notes [1], which would be
cumbersome for them. This is not a security issue, so we shouldn't
backpatch a catalog modifying commit following past.

> Following off-list
> discussions with Amit and Kuroda-san, we are considering disallowing enabling
> failover and two-phase decoding together for a replication slot, as suggested
> in attachment 0002.
>
> Another idea considered is to prevent the slot that enables two-phase decoding
> from being synced to standby. IOW, this means displaying the failover field as
> false in the view, if there is any possibility that transactions prepared
> before the two_phase_at position exist (e.g., if restart_lsn is less than
> two_phase_at). However, implementing this change would require additional
> explanations to users for this new behavior, which seems tricky.
>

I find it tricky to explain to users. We need to say that sometimes
the slots won't be synced even if the failover is set to true. Users
can verify that by checking slot properties on the publisher. Also, on
the subscriber, the failover flag in the subscription may still be
true unless we do more engineering to make it false. So, I prefer to
simply disallow setting failover and two_phase together. We need to
recommend to users in release notes for 17 that they need to disable
failover for subscriptions where two_phase is enabled or re-create the
subscriptions with two_phase=false and failover=true. Users may not
like it, but I think it is better than getting a complaint that after
promotion of standby the data to subscriber is not getting replicated.

[1] - https://www.postgresql.org/docs/9.6/release-9-6-4.html

--
With Regards,
Amit Kapila.

Re: Fix slot synchronization with two_phase decoding enabled

From

Amit Kapila

Date:

01 April, 09:09:10

On Mon, Mar 31, 2025 at 5:04 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> Thanks for the comments, they have been addressed in V2.
>

In the PG17 patch, we only have a check preventing failover and
two_phase in CreateSubscription. Don't we need a similar check for
AlterSubscription?

Apart from the above, I have a few suggestions for changes in docs,
error messages, and comments for both versions. See attached.

--
With Regards,
Amit Kapila.

Attachment

RE: Fix slot synchronization with two_phase decoding enabled

From

"Zhijie Hou (Fujitsu)"

Date:

01 April, 13:58:24

On Tue, Apr 1, 2025 at 2:09 PM Amit Kapila wrote:

> 
> On Mon, Mar 31, 2025 at 5:0 PM Zhijie Hou (Fujitsu) 
> <houzj.fnst@fujitsu.com> wrote:
> >
> > Thanks for the comments, they have been addressed in V2.
> >
> 
> In the PG17 patch, we only have a check preventing failover and 
> two_phase in CreateSubscription. Don't we need a similar check for AlterSubscription?
> 
> Apart from the above, I have a few suggestions for changes in docs, 
> error messages, and comments for both versions. See attached.

Thanks for the comments.

Here is the V3 patch set which addressed all the comments.

I also added a testcase for ALTER SUB in 0002 as suggested by
Kuroda-san off-list.

Additionally, I found that a test failed in PG17 following this
patch because it involved creating a subscription with both failover and
two-phase enabled. Since that test was designed to test the upgrade of
replication slots and is not directly related to failover functionality, I
decided to disable the failover option for test case.

Best Regards,
Hou zj

Attachment

Re: Fix slot synchronization with two_phase decoding enabled

From

Amit Kapila

Date:

02 April, 07:40:44

On Tue, Apr 1, 2025 at 4:28 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> Here is the V3 patch set which addressed all the comments.
>

Comment 0n 0001
<literal>NULL</literal> for logical slots where
+       <structfield>two_phase</structfield> is false and physical slots.
+      </para></entry>

change above to:
<literal>NULL</literal> for logical slots where
<structfield>two_phase</structfield> is false and for physical slots.

Comment on 0002
+# Create a subscription with two_phase enabled
+$subscriber1->safe_psql('postgres',
+ "CREATE SUBSCRIPTION regress_mysub2 CONNECTION '$publisher_connstr'
PUBLICATION regress_mypub WITH (slot_name = lsub1_slot, create_slot =
false, enabled = false, two_phase = true);"
+);
+
+# Enable failover for the subscription
+($result, $stdout, $stderr) = $subscriber1->psql('postgres',
+ "ALTER SUBSCRIPTION regress_mysub2 SET (failover = true)");
+ok( $stderr =~ /ERROR:  cannot enable failover for a two_phase
enabled subscription/,
+ "Enabling failover is not allowed for a two_phase enabled subscription");

Is there a need for this test to be in .pl file? Can't we add it in .sql file?

Apart from the above, I have made minor modifications to the PG17
patch in the attached.

--
With Regards,
Amit Kapila.

Attachment

v3-0002-amit.1.patch.txt

RE: Fix slot synchronization with two_phase decoding enabled

From

"Zhijie Hou (Fujitsu)"

Date:

02 April, 08:55:37

On Wed, Apr 2, 2025 at 12:41 PM Amit Kapila wrote:

> 
> On Tue, Apr 1, 2025 at 4:28 PM Zhijie Hou (Fujitsu) 
> <houzj.fnst@fujitsu.com>
> wrote:
> >
> > Here is the V3 patch set which addressed all the comments.
> >
> 
> Comment 0n 0001
> <literal>NULL</literal> for logical slots where
> +       <structfield>two_phase</structfield> is false and physical slots.
> +      </para></entry>
> 
> change above to:
> <literal>NULL</literal> for logical slots where 
> <structfield>two_phase</structfield> is false and for physical slots.

Changed.

> 
> Comment on 0002
> +# Create a subscription with two_phase enabled 
> +$subscriber1->safe_psql('postgres',
> + "CREATE SUBSCRIPTION regress_mysub2 CONNECTION
> '$publisher_connstr'
> PUBLICATION regress_mypub WITH (slot_name = lsub1_slot, create_slot = 
> false, enabled = false, two_phase = true);"
> +);
> +
> +# Enable failover for the subscription ($result, $stdout, $stderr) = 
> +$subscriber1->psql('postgres',  "ALTER SUBSCRIPTION regress_mysub2 
> +SET (failover = true)"); ok( $stderr =~
> +/ERROR:  cannot enable failover for a two_phase
> enabled subscription/,
> + "Enabling failover is not allowed for a two_phase enabled 
> + subscription");
> 
> Is there a need for this test to be in .pl file? Can't we add it in .sql file?

Right, I moved the test into subscription.sql

> Apart from the above, I have made minor modifications to the PG17 
> patch in the attached.

Here is V4 patch set which addressed above comments and passed
pgindent test.

Best Regards,
Hou zj

HI,

Please find the patches attached for all three approaches.

On Wed, Apr 9, 2025 at 10:45 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Thu, Apr 3, 2025 at 3:16 PM Zhijie Hou (Fujitsu) wrote:
> >
> > On Thu, Apr 3, 2025 at 1:38 PM Masahiko Sawada wrote:
> >
> > >
> > > On Wed, Apr 2, 2025 at 7:58 PM Amit Kapila <amit.kapila16@gmail.com>
> > > wrote:
> > > >
> > > > On Thu, Apr 3, 2025 at 7:50 AM Zhijie Hou (Fujitsu)
> > > > <houzj.fnst@fujitsu.com> wrote:
> > > > >
> > > > > On Thu, Apr 3, 2025 at 3:30 AM Masahiko Sawada wrote:
> > > > >
> > > > > >
> > > > > > On Wed, Apr 2, 2025 at 6:33 AM Zhijie Hou (Fujitsu)
> > > > > > <houzj.fnst@fujitsu.com> wrote:
> > > > > >
> > > > > > Thank you for the explanation! I agree that the issue happens in
> > > > > > these
> > > cases.
> > > > > >
> > > > > > As another idea, I wonder if we could somehow defer to make the
> > > > > > synced slot as 'sync-ready' until we can ensure that the slot
> > > > > > doesn't have any transactions that are prepared before the point
> > > > > > of enabling two_phase. For example, when the slotsync worker
> > > > > > fetches the remote slot, it remembers the confirmed_flush_lsn
> > > > > > (say
> > > > > > LSN-1) if the local slot's two_phase becomes true or the local
> > > > > > slot is newly created with enabling two_phase, and then it makes
> > > > > > the slot 'sync-ready' once it confirmed that the slot's
> > > > > > restart_lsn passed
> > > LSN-1. Does it work?
> > > > >
> > > > > Thanks for the idea!
> > > > >
> > > > > We considered a similar approach in [1] to confirm there is no
> > > > > prepared transactions before two_phase_at, but the issue is that
> > > > > when the two_phase flag is switched from 'false' to 'true' (as in
> > > > > the case with (copy_data=true, failover=true, two_phase=true)). In
> > > > > this case, the slot may have already been marked as sync-ready
> > > > > before the two_phase flag is enabled, as slotsync is unaware of
> > > > > potential
> > > future changes to the two_phase flag.
> > > >
> > > > This can happen because when copy_data is true, tablesync can take a
> > > > long time to complete the sync and in the meantime, slot without a
> > > > two_phase flag would have been synced to standby. Such a slot would
> > > > be marked as sync-ready even if we follow the calculation proposed
> > > > by Sawada-san. Note that we enable two_phase once all the tables are
> > > > in ready state (See run_apply_worker() and comments atop worker.c
> > > > (TWO_PHASE TRANSACTIONS)).
> > >
> > > Right. It doesn't make sense to make the slot not-sync-ready and then
> > > back to sync-ready.
> > >
> > > While I agree with the approach for HEAD and it seems difficult to
> > > find a solution, I'm concerned that disallowing to use both failover
> > > and two_phase in a minor release would affect users much. Users who
> > > are already using that combination might end up needing to re-think
> > > their system architecture. So I'm trying to narrow down use cases
> > > where we're going to prohibit or to find workarounds.
>
> We analyzed more for the backbranch fix, and here is a summary of different
> approaches that could be used for PG17.
>
> ----------
> Approach 1
> ----------
>
> This is the original approach implemented in V4 patch.
>
> In this approach, we entirely disallow enabling both failover and the two-phase
> feature together for a replication slot or subscription.
>
> pros:
>
> This restriction is simple to implement and easy for users to comprehend.
>
> cons:
>
> Users would be unable to use the two-phase feature in conjunction with
> failover.
>
> Following the upgrade to a new release with this fix, existing subscriptions
> that have both failover and two-phase enabled would require manual re-creation,
> which is time-consuming.
>

Patch "v5_aprch_1-0001" implements the above Approach 1.

> ----------
> 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.
>
> Users are not always forced to re-create subscriptions post-upgrade.
>
> cons:
>
> The logic involved for (restart_lsn > two_phase_at) might be less intuitive for
> users.
>
> After upgrading, it's recommended that users verify whether two_phase_at for a
> source slot is less than restart_lsn of the synced slot. If it isn't, users
> should be careful when using the synced slot on a standby. This might be
> complex to understand.
>

Patch "v5_aprch_2-0001" implements the above Approach 2.

> ----------
> Approach 3
> ----------
>
> This approach is similar to Approach 2 but simplifies some aspects by avoiding
> checks for prepared transactions during slot creation. It disallows enabling
> both failover and twophase during slot creation, but permits altering failover
> to true once ensured that no prepared transaction exists (restart_lsn >
> two_phase_at).
>
> pros:
>
> Like Approach 2, this reduces user restrictions compared to completely
> disallowing failover and two-phase usage together.
>
> Users are not always forced to re-create subscriptions post-upgrade.
>
> cons:
>
> User still could not enable failover and twophase when creating a slot.
>
> Similar to approach 1, the check (restart_lsn > two_phase_at) might be less
> intuitive for users.
>
> After upgrading, it's recommended that users verify whether two_phase_at for a
> source slot is less than restart_lsn of the synced slot. If it isn't, users
> should be careful when using the synced slot on a standby. This might be
> complex to understand.
>

Patch "v5_aprch_3-0001" implements the above Approach 3.

Thanks Hou-san for implementing approach-2 and providing the patch.

--
Thanks,
Nisha

Attachment

RE: Fix slot synchronization with two_phase decoding enabled

From

"Zhijie Hou (Fujitsu)"

Date:

17 April, 15:44:16

Hi,

The recently added tests for slotsync on two-phase enabled slots failed[1] due
to a broken assertion triggered while decoding the COMMIT PREPARED record on
the promoted standby.

-----
Analysis
-----

if ((txn->final_lsn < two_phase_at) && is_commit)
{
/*
* txn must have been marked as a prepared transaction and skipped but
* not sent a prepare. Also, the prepare info must have been updated
* in txn even if we skip prepare.
*/
Assert((txn->txn_flags & RBTXN_PREPARE_STATUS_MASK) ==
(RBTXN_IS_PREPARED | RBTXN_SKIPPED_PREPARE));

I think the issue stem from the PREPARED transaction, which was skipped on the
primary, not being skipped on the promoted standby. The root cause appears to
be the latest 'confirmed_flush' (0/6005118) of the source slot not being synced
to the standby. This results in the confirmed_flush (0/6004F90) of the synced
slot being less than the synced two_phase_at field (0/6005118). Consequently,
the PREPARE record's LSN exceeds the synced confirmed_flush during
decoding, causing it to be incorrectly decoded and sent to the subscriber. Thus, when
decoding COMMIT PREPARED, the PREPARE record is found before two_phase_at but
wasn't skipped.

-- The LOGs that proves the confirmed_flush is not synced:

decoding on original slot (primary):
LOG: 0/6004F90 has been already streamed, forwarding to 0/6005118
...
DETAIL: Streaming transactions committing after 0/6005118, reading WAL from 0/60049C8.

decoding on synced slot (promted standby) - failed run:
DETAIL: Streaming transactions committing after 0/6004F90, reading WAL from 0/60049C8.

decoding on synced slot (promted standby) - success run:
LOG: 0/6004F90 has been already streamed, forwarding to 0/6005118
...
DETAIL: Streaming transactions committing after 0/6005118, reading WAL from 0/60049C8.
--

The logs above clearly show that during the test failure, the starting position
(6004F90) was less than that of a successful run. Additionally, it was lower
than the starting position of the remote slot on the primary, indicating a
failure to synchronize confirmed_lsn.

The reason confirmed_flush isn't synced should stem from the following check,
which skip the syncing of the confirmed_flush value. I also reproduced the
assertion failure by creating a scenario that leads to sync omission due to
this check:

/*
* Don't overwrite if we already have a newer catalog_xmin and
* restart_lsn.
*/
if (remote_slot->restart_lsn < slot->data.restart_lsn ||
TransactionIdPrecedes(remote_slot->catalog_xmin,
slot->data.catalog_xmin))
{

This check triggered because the synced catalog_xmin surpassed that of the
source slot (It can be seen from the log that the restart_lsn was synced to the
same value) ). This situation arises due to several factors:

On the publisher(primary), the slot may retain an older catalog_xmin and
restart_lsn because it is being consumed by a walsender. In this scenario, if
the apply worker does not immediately confirm that changes have been flushed,
the walsender advances the catalog_xmin/restart_lsn slowly. It sets an old
value for candidate_catalog_xmin/candidate_restart_lsn and would not update
them until the apply worker confirms via LogicalConfirmReceivedLocation.

However, the slotsync worker has the opportunity to begin WAL reading from a
more recent point, potentially advancing catalog_xmin/restart_lsn to newer
values.

And it's also possible to advance only catalog_xmin while keep restart_lsn
unchanged, by starting a transaction before the running_xact record. In this
case, it would pass builder->last_serialized_snapshot as restart_lsn to
LogicalIncreaseRestartDecodingForSlot(), but last_serialized_snapshot would
point to the position of the last running_xact record instead of the current
one, or the value would be NULL if no snapshot has been serialized before, so
it would not advance restart_lsn. The advancement of catalog_xmin is not
blocked by running transaction, as it would either use the XID of the running
transaction or the oldestRunningXid as candidate.

-----
Reproduction
-----

Accompanying this analysis is a script to reproduce the issue. In these
scripts, two running_xacts were logged post-slot creation on the publisher.
Below is a detailed progression of restart_lsn/catalog_xmin advancement
observed on both the publisher and standby:

The process is : slot creation -> log running_xact -> prepare a txn -> log
running_xact

The process of advancing the catalog_xmin/restart_lsn on publisher:

slot creation
...
1. 0/301B880 - running_xacts (no running txn, oldestrunningxid = 755)

got new catalog xmin 755 at 0/301B880
LOG: got new restart lsn 0/301B880 at 0/301B880
2. 0/301B8B8 - PREPAPRE a TRANSACTION.
3. 0/301BA20 - running_xacts (no running txn, oldestrunningxid = 756)

failed to increase restart lsn: proposed 0/301B880, after 0/301BA20,
current candidate 0/301B880, current after 0/301B880, flushed up to
0/301B810

final slot data:

catalog_xmin | 755
restart_lsn | 0/301B880
confirmed_flush_lsn | 0/301BAC8

The process of advancing the catalog_xmin/restart_lsn on standby (during slot
sync):

slot creation
...
1. 0/301B880 - running_xacts (no running txn, oldestrunningxid = 755)

building consistent snapshot, so will skip advancing the catalog_xmin/restart_lsn
...
2. 0/301BA20 - running_xacts (no running txn, oldestrunningxid = 756)

got new catalog xmin 756 at 0/301BA20

cannot get new restart_lsn as running txn exists and didn't serialize
snapshot yet.

first synced slot data:

catalog_xmin | 756
restart_lsn | 0/301B880
confirmed_flush_lsn | 0/301BAC8

0001-Add-a-test.patch

Re: Fix slot synchronization with two_phase decoding enabled

From

shveta malik

Date:

22 April, 12:53:05

On Fri, Apr 11, 2025 at 1:03 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
>
> Patch "v5_aprch_3-0001" implements the above Approach 3.
>
> Thanks Hou-san for implementing approach-2 and providing the patch.
>

I find Approach 2 better than Approach1. Yet to review Approach3.
Please find my initial comments:

Approach#1:

1)
When slot is created with failover enabled and later we try to create
a subscription using that pre-created slot with two-phase enabled, it
does not error out. But it silently retains two_phase of the existing
slot as false. Please check if an error is possible in this case too.

Approach #2:

1)
+ ReorderBufferSkipPrepare(reorder, parsed.twophase_xid);

In xact_decode, why do we have a new call to ReorderBufferSkipPrepare,
can you please add some comments here?

2)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("\"%s\" and \"%s\" are mutually exclusive options when \"%s\"
is specified",

So like approach 1, here as well we disallow creating subscriptions
with both failover and two_phase enabled when create_slot and
copy_data is true? But users can alter the sub later to allow failover
for two-phase enabled slot provided there are no pending PREPARE txns
which are not yet consumed by the subscriber. Is my understanding
correct? Can you please tell all the ways using which the user can
enable both failover and two_phase together? The patch msg is not that
clear. It will be good to add such details in patch message.

3)

+ /*
+ * Do not allow users to enable the failover and two_phase options together
+ * when create_slot is specified.
+ *
+ * See comments atop the similar check in DecodingContextFindStartpoint()
+ * for a detailed reason.
+ */
+ if (!opts.create_slot && opts.twophase && opts.failover)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("\"%s\" and \"%s\" are mutually exclusive options when \"%s\"
is specified",
+

Is the check '!opts.create_slot' correct? The error msg and comment
says otherwise.

thanks
Shveta

Re: Fix slot synchronization with two_phase decoding enabled

From

Amit Kapila

Date:

22 April, 13:00:01

On Mon, Apr 21, 2025 at 8:44 AM Zhijie Hou (Fujitsu)
<houzj.fnst@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.fnst@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'.


--
With Regards,
Amit Kapila.

Re: Fix slot synchronization with two_phase decoding enabled

From

Nisha Moond

Date:

23 April, 14:23:55

On Tue, Apr 22, 2025 at 3:23 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Fri, Apr 11, 2025 at 1:03 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> >
> > Patch "v5_aprch_3-0001" implements the above Approach 3.
> >
> > Thanks Hou-san for implementing approach-2 and providing the patch.
> >
>
> I find Approach 2 better than Approach1. Yet to review Approach3.
> Please find my initial comments:
>

Thanks for the review! I’ve addressed the comments for Approach 2, as
it seems the most suitable. We can revisit the others if needed.

>
>
> Approach #2:
>
> 1)
> + ReorderBufferSkipPrepare(reorder, parsed.twophase_xid);
>
> In xact_decode, why do we have a new call to ReorderBufferSkipPrepare,
> can you please add some comments here?
>

Done.

> 2)
> + ereport(ERROR,
> + errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("\"%s\" and \"%s\" are mutually exclusive options when \"%s\"
> is specified",
>
> So like approach 1, here as well we disallow creating subscriptions
> with both failover and two_phase enabled when create_slot and
> copy_data is true? But users can alter the sub later to allow failover
> for two-phase enabled slot provided there are no pending PREPARE txns
> which are not yet consumed by the subscriber. Is my understanding
> correct? Can you please tell all the ways using which the user can
> enable both failover and two_phase together? The patch msg is not that
> clear. It will be good to add such details in patch message.
>

We allow creating subscriptions in this scenario as long as no
prepared transactions exist before the two_phase_at. Similarly,
altering a subscription to set failover = true is permitted, provided
the slot's restart_lsn is greater than two_phase_at.
Amit has suggested the ways at [1]  in which users can enable both
failover and two_phase together.
I've updated the commit message to include more details about the
conditions enforced by the fix.

> 3)
>
> + /*
> + * Do not allow users to enable the failover and two_phase options together
> + * when create_slot is specified.
> + *
> + * See comments atop the similar check in DecodingContextFindStartpoint()
> + * for a detailed reason.
> + */
> + if (!opts.create_slot && opts.twophase && opts.failover)
> + ereport(ERROR,
> + errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("\"%s\" and \"%s\" are mutually exclusive options when \"%s\"
> is specified",
> +
>
> Is the check '!opts.create_slot' correct? The error msg and comment
> says otherwise.
>

Corrected the check as it should be checking if copy_data is
specified. Thanks for the input!

Please find the attached v6 patch for approach-2 fix on PG17.

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

--
Thanks,
Nisha

Attachment

v6-0001-PG17-Approach-2-Fix-slot-synchronization-for-two_.patch

Re: Fix slot synchronization with two_phase decoding enabled

From

Masahiko Sawada

Date:

23 April, 20:33:44

On Tue, Apr 22, 2025 at 3:00 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Apr 21, 2025 at 8:44 AM Zhijie Hou (Fujitsu)
> <houzj.fnst@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.fnst@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

Re: Fix slot synchronization with two_phase decoding enabled

From

Amit Kapila

Date:

24 April, 09:57:52

On Wed, Apr 23, 2025 at 11:04 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Apr 22, 2025 at 3:00 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Apr 21, 2025 at 8:44 AM Zhijie Hou (Fujitsu)
> > <houzj.fnst@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.fnst@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".
>

This should further simplify the checks with an additional restriction
during the CREATE SUBSCRIPTION time. I am in favor of it because I
want the code in this area to be as much same in HEAD and backbranches
as possible.

Please note that the PG17 also suffers from data loss after promotion
due to a bug in fast-forward mode as described in email [1]. So, we
should try to get that fixed as well.

Thanks for your feedback.

[1] -
https://www.postgresql.org/message-id/OS0PR01MB57163087F86621D44D9A72BF94BB2%40OS0PR01MB5716.jpnprd01.prod.outlook.com

--
With Regards,
Amit Kapila.

Re: Fix slot synchronization with two_phase decoding enabled

From

Nisha Moond

Date:

24 April, 12:24:04

On Thu, Apr 24, 2025 at 12:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Apr 23, 2025 at 11:04 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Tue, Apr 22, 2025 at 3:00 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Mon, Apr 21, 2025 at 8:44 AM Zhijie Hou (Fujitsu)
> > > <houzj.fnst@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.fnst@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".
> >
>
> This should further simplify the checks with an additional restriction
> during the CREATE SUBSCRIPTION time. I am in favor of it because I
> want the code in this area to be as much same in HEAD and backbranches
> as possible.
>

Please find the updated patch for Approach 3, which implements the
idea suggested by Swada-san above.

--
Thanks,
Nisha

Attachment

v7-0001-PG17-Approach-3-Fix-slot-synchronization-for-two_.patch

Re: Fix slot synchronization with two_phase decoding enabled

From

shveta malik

Date:

24 April, 13:27:22

On Thu, Apr 24, 2025 at 2:54 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> Please find the updated patch for Approach 3, which implements the
> idea suggested by Swada-san above.
>

Thank You for the patch.

1)

CreateDecodingContext:

  if (ctx->twophase && !slot->data.two_phase)
  {
+ /*
+ * Do not allow two-phase decoding for failover enabled slots.
+ *
+ * See comments atop the similar check in ReplicationSlotCreate() for
+ * a detailed reason.
+ */
+ if (slot->data.failover)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot enable two-phase decoding for failover enabled slot \"%s\"",
+ NameStr(slot->data.name))));
+

Can you please add tetscase to cover this scenario?

2)
We shall update create-sub documents as well for these mutually
exclusive options. Review other pages (alter-sub, create-slot) as well
for any required change.

3)
+##################################################
+# Test that the failover option can be enabled for a two_phase enabled
+# subscription.
+##################################################

Suggestion: 'Test that the failover option can be enabled for a two_phase
enabled subscription only through Alter Subscription (failover=true)'


thanks
Shveta

Re: Fix slot synchronization with two_phase decoding enabled

From

Masahiko Sawada

Date:

24 April, 20:48:58

On Thu, Apr 24, 2025 at 2:24 AM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> On Thu, Apr 24, 2025 at 12:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Apr 23, 2025 at 11:04 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > On Tue, Apr 22, 2025 at 3:00 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > On Mon, Apr 21, 2025 at 8:44 AM Zhijie Hou (Fujitsu)
> > > > <houzj.fnst@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.fnst@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".
> > >
> >
> > This should further simplify the checks with an additional restriction
> > during the CREATE SUBSCRIPTION time. I am in favor of it because I
> > want the code in this area to be as much same in HEAD and backbranches
> > as possible.
> >
>
> Please find the updated patch for Approach 3, which implements the
> idea suggested by Swada-san above.

Thank you for the patch! I think we need to update the documentation
as well. I'll also review the patch shortly so could you please
prepare the documentation changes?

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: Fix slot synchronization with two_phase decoding enabled

From

Masahiko Sawada

Date:

25 April, 03:31:48

On Thu, Apr 24, 2025 at 10:48 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Thu, Apr 24, 2025 at 2:24 AM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > On Thu, Apr 24, 2025 at 12:28 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Apr 23, 2025 at 11:04 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > > >
> > > > On Tue, Apr 22, 2025 at 3:00 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > >
> > > > > On Mon, Apr 21, 2025 at 8:44 AM Zhijie Hou (Fujitsu)
> > > > > <houzj.fnst@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.fnst@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".
> > > >
> > >
> > > This should further simplify the checks with an additional restriction
> > > during the CREATE SUBSCRIPTION time. I am in favor of it because I
> > > want the code in this area to be as much same in HEAD and backbranches
> > > as possible.
> > >
> >
> > Please find the updated patch for Approach 3, which implements the
> > idea suggested by Swada-san above.
>
> Thank you for the patch! I think we need to update the documentation
> as well. I'll also review the patch shortly so could you please
> prepare the documentation changes?

Here are reviews comments on v7 patch:

+   /*
+    * Do not allow users to enable the failover and two_phase options
+    * together.
+    *
+    * See comments atop the similar check in ReplicationSlotCreate() for a
+    * detailed reason.
+    */
+   if (opts.twophase && opts.failover)
+       ereport(ERROR,
+               errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+               errmsg("\"%s\" and \"%s\" are mutually exclusive options",
+                      "failover", "two_phase"));

I think we can reword the error message to something like "cannot
enable both failover and two_phase options at CREATE SUBSCRIPTION",
and we can give a errhint that failover can be enabled after two_phase
state is ready.

---
+       if (two_phase && !IsSyncingReplicationSlots())
+           ereport(ERROR,
+                   errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                   errmsg("\"%s\" and \"%s\" are mutually exclusive options",
+                          "failover", "two_phase"));

Similar to the above comment, how about rewriting it to something like
"cannot enable both failover and two_phase during replication slot
creation".

I realized that users who create a logical slot using
pg_create_logical_replication_slot() would not be able to enable both
options at slot creation, and there is no easy way to enable the
failover after two_phase-enabled-slot creation. Users would need to
use ALTER_REPLICATION_SLOT replication command, which seems
unrealistics for users to use. On the other hand, if we allow creating
a logical slot with enabling failover and two_phase using SQL API,
there is still a chance for this bug to occur. Would it be worth
considering that if a logical slot is created with enabling failover
and two_phase using SQL API, we create the slot with only
two_phase=true, then advance the slot until the slot satisfies
restart_lsn >= two_phase_at, and then enable the failover?

---
+                   if (sub->twophasestate ==
LOGICALREP_TWOPHASE_STATE_PENDING &&
+                       opts.failover)
+                       ereport(ERROR,
+                               errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                               errmsg("cannot enable failover for a
subscription with a pending two_phase state"));
+

I think ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE is a more appropriate
error code.

---
+   if (failover && MyReplicationSlot->data.two_phase &&
+       MyReplicationSlot->data.restart_lsn <
MyReplicationSlot->data.two_phase_at)
+       ereport(ERROR,
+               errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+               errmsg("cannot enable failover for a two-phase enabled
replication slot"),
+               errdetail("The slot need to consume change upto %X/%X
to enable failover.",
+
LSN_FORMAT_ARGS(MyReplicationSlot->data.two_phase_at)));

Same as above comment with regards to the error code.

---
We need regression tests for replication slot creation SQL API too.

---
+# Confirm that the failover flag on the slot has been turned on
+is( $publisher->safe_psql(
+       'postgres',
+       q{SELECT failover from pg_replication_slots WHERE slot_name =
'lsub1_slot';}
+   ),
+   "t",
+   'logical slot has failover true on the publisher');

It should check 'lsub2_slot' instead.

---
+# Create a subscription with two_phase enabled
+$subscriber1->safe_psql('postgres',
+   "CREATE SUBSCRIPTION regress_mysub2 CONNECTION
'$publisher_connstr' PUBLICATION regress_mypub WITH (create_slot =
false, copy_data = false, enabled = false, two_phase = true);"
+);
+
+# Enable failover for the subscription with two_phase in pending state
+($result, $stdout, $stderr) = $subscriber1->psql('postgres',
+   "ALTER SUBSCRIPTION regress_mysub2 SET (failover = true)");
+ok( $stderr =~ /ERROR:  cannot enable failover for a subscription
with a pending two_phase state/,
+   "Enabling failover is not allowed for a two_phase pending subscription");
+

I think it would be good to have more tests where we make the
two_phase state ready and execute 'ALTER SUBSCRIPTION SET (failover =
true)' successfully.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: Fix slot synchronization with two_phase decoding enabled

From

Amit Kapila

Date:

25 April, 13:43:12

On Fri, Apr 25, 2025 at 6:02 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> I realized that users who create a logical slot using
> pg_create_logical_replication_slot() would not be able to enable both
> options at slot creation, and there is no easy way to enable the
> failover after two_phase-enabled-slot creation. Users would need to
> use ALTER_REPLICATION_SLOT replication command, which seems
> unrealistics for users to use. On the other hand, if we allow creating
> a logical slot with enabling failover and two_phase using SQL API,
> there is still a chance for this bug to occur. Would it be worth
> considering that if a logical slot is created with enabling failover
> and two_phase using SQL API, we create the slot with only
> two_phase=true, then advance the slot until the slot satisfies
> restart_lsn >= two_phase_at, and then enable the failover?
>

This means we either need to maintain somewhere that user has provided
failover flag till restart_lsn >= two_phase_at or and then set
failover flag in the slot or initially mark it but enable the
functionality of failover when we reach the condition restart_lsn >=
two_phase_at. Both seem to have different kinds of problems. The first
idea seems to have an issue with persistence, which means we can lose
track of the flag after the restart. The second can mislead the user
for a long period in cases where prepare and commit have a large time
gap. I feel this will introduce complexity either in the form of code
or in giving the information to the user.

--
With Regards,
Amit Kapila.

Re: Fix slot synchronization with two_phase decoding enabled

From

Nisha Moond

Date:

25 April, 15:23:38

On Thu, Apr 24, 2025 at 3:57 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Thu, Apr 24, 2025 at 2:54 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > Please find the updated patch for Approach 3, which implements the
> > idea suggested by Swada-san above.
> >
>
> Thank You for the patch.
>
> 1)
>
> CreateDecodingContext:
>
>   if (ctx->twophase && !slot->data.two_phase)
>   {
> + /*
> + * Do not allow two-phase decoding for failover enabled slots.
> + *
> + * See comments atop the similar check in ReplicationSlotCreate() for
> + * a detailed reason.
> + */
> + if (slot->data.failover)
> + ereport(ERROR,
> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("cannot enable two-phase decoding for failover enabled slot \"%s\"",
> + NameStr(slot->data.name))));
> +
>
> Can you please add tetscase to cover this scenario?
>

Added a test for the above scenario.

> 2)
> We shall update create-sub documents as well for these mutually
> exclusive options. Review other pages (alter-sub, create-slot) as well
> for any required change.
>

Updated docs where I felt this new change should be mentioned. Please
let me know if I missed any place.

> 3)
> +##################################################
> +# Test that the failover option can be enabled for a two_phase enabled
> +# subscription.
> +##################################################
>
> Suggestion: 'Test that the failover option can be enabled for a two_phase
> enabled subscription only through Alter Subscription (failover=true)'
>
>

Done.
~~~

Please find the attached v8 patch with above comments addressed.
This version includes the documentation updates suggested by
Sawada-san at [1], and incorporates his feedback from [2] as well.

[1] https://www.postgresql.org/message-id/CAD21AoD08Nb588fAJ%2BJd5xRxtAT8yWnOfZ3zq-K5sto9b3ntsA%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAD21AoD08Nb588fAJ%2BJd5xRxtAT8yWnOfZ3zq-K5sto9b3ntsA%40mail.gmail.com

--
Thanks,
Nisha

Attachment

v8-0001-PG17-Approach-3-Fix-slot-synchronization-for-two_.patch

Re: Fix slot synchronization with two_phase decoding enabled

From

Masahiko Sawada

Date:

25 April, 19:26:40

On Fri, Apr 25, 2025 at 3:43 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Apr 25, 2025 at 6:02 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I realized that users who create a logical slot using
> > pg_create_logical_replication_slot() would not be able to enable both
> > options at slot creation, and there is no easy way to enable the
> > failover after two_phase-enabled-slot creation. Users would need to
> > use ALTER_REPLICATION_SLOT replication command, which seems
> > unrealistics for users to use. On the other hand, if we allow creating
> > a logical slot with enabling failover and two_phase using SQL API,
> > there is still a chance for this bug to occur. Would it be worth
> > considering that if a logical slot is created with enabling failover
> > and two_phase using SQL API, we create the slot with only
> > two_phase=true, then advance the slot until the slot satisfies
> > restart_lsn >= two_phase_at, and then enable the failover?
> >
>
> This means we either need to maintain somewhere that user has provided
> failover flag till restart_lsn >= two_phase_at or and then set
> failover flag in the slot

I was thinking of this idea.

> or initially mark it but enable the
> functionality of failover when we reach the condition restart_lsn >=
> two_phase_at.

IIUC the slot could be synchronized to the standby as soon as we
complete DecodingContextFindStartpoint() for a failover-enabled slot.
So we would need some mechanisms to make sure that the slot is not
synchronized while we're waiting to reach the condition restart_lsn >=
two_phase_at even if the failover is enabled.

> Both seem to have different kinds of problems. The first
> idea seems to have an issue with persistence, which means we can lose
> track of the flag after the restart.

I think we can do this series of operations while the slot is not
persistent, that is the slot is still RS_EPHEMERAL.

> The second can mislead the user
> for a long period in cases where prepare and commit have a large time
> gap. I feel this will introduce complexity either in the form of code
> or in giving the information to the user.

Agreed. Both ways introduce complexity so we need to consider the
user-unfriendliness (by not having a proper way to enable failover for
the two_phase-enabled-slot using SQL API) vs. risk (of introducing
complexity).

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: Fix slot synchronization with two_phase decoding enabled

From

Amit Kapila

Date:

26 April, 15:07:38

On Fri, Apr 25, 2025 at 9:57 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Fri, Apr 25, 2025 at 3:43 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Fri, Apr 25, 2025 at 6:02 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > I realized that users who create a logical slot using
> > > pg_create_logical_replication_slot() would not be able to enable both
> > > options at slot creation, and there is no easy way to enable the
> > > failover after two_phase-enabled-slot creation. Users would need to
> > > use ALTER_REPLICATION_SLOT replication command, which seems
> > > unrealistics for users to use. On the other hand, if we allow creating
> > > a logical slot with enabling failover and two_phase using SQL API,
> > > there is still a chance for this bug to occur. Would it be worth
> > > considering that if a logical slot is created with enabling failover
> > > and two_phase using SQL API, we create the slot with only
> > > two_phase=true, then advance the slot until the slot satisfies
> > > restart_lsn >= two_phase_at, and then enable the failover?
> > >
> >
> > This means we either need to maintain somewhere that user has provided
> > failover flag till restart_lsn >= two_phase_at or and then set
> > failover flag in the slot
>
> I was thinking of this idea.
>
> > or initially mark it but enable the
> > functionality of failover when we reach the condition restart_lsn >=
> > two_phase_at.
>
> IIUC the slot could be synchronized to the standby as soon as we
> complete DecodingContextFindStartpoint() for a failover-enabled slot.
> So we would need some mechanisms to make sure that the slot is not
> synchronized while we're waiting to reach the condition restart_lsn >=
> two_phase_at even if the failover is enabled.
>

So, then we need any state or persistent flag for this.

> > Both seem to have different kinds of problems. The first
> > idea seems to have an issue with persistence, which means we can lose
> > track of the flag after the restart.
>
> I think we can do this series of operations while the slot is not
> persistent, that is the slot is still RS_EPHEMERAL.
>

But we still need a persistent flag to indicate such slots shouldn't
be synced to standby till we reach the condition restart_lsn >=
two_phase_at.

> > The second can mislead the user
> > for a long period in cases where prepare and commit have a large time
> > gap. I feel this will introduce complexity either in the form of code
> > or in giving the information to the user.
>
> Agreed. Both ways introduce complexity so we need to consider the
> user-unfriendliness (by not having a proper way to enable failover for
> the two_phase-enabled-slot using SQL API) vs. risk (of introducing
> complexity).
>

Right, to me it sounds risky to provide such functionality for SQL API
in the back branch.

--
With Regards,
Amit Kapila.

Re: Fix slot synchronization with two_phase decoding enabled

From

shveta malik

Date:

28 April, 09:33:52

On Fri, Apr 25, 2025 at 5:53 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> Please find the attached v8 patch with above comments addressed.
> This version includes the documentation updates suggested by
> Sawada-san at [1], and incorporates his feedback from [2] as well.
>

Thanks for the patches.

1)
Regarding documents, these are the details added:
 a) pg_create_logical_replication_slot  says "The parameters twophase
and failover cannot be enabled together when creating a replication
slot."
 b) CREATE_REPLICATION_SLOT says TWO_PHASE: This option is mutually
exclusive with failover.
 c) CREATE SUBSCRIPTION doc says: The options failover and twophase
cannot be enabled together when creating a subscription.
 d) ALTER SUB says: When two_phase is in the "pending" state, setting
failover = true is not permitted. Once two_phase is "enabled",
failover = true can be set only if the slot's restart_lsn has advanced
beyond its two_phase_at value.

The individual points mentioned are correct. But nowhere we get
complete picture as in how user can enable failover and twophase
together  when setting up a slot and subscription. I think we can add
brief notes in a,b and c to state how user can actually enable these 2
together.
Or can add this detail in single page and let other pages refer/point
to that page. Thoughts?

2)

ReplicationSlotCreate():

+ if (two_phase && !IsSyncingReplicationSlots())
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot enable both \"%s\" and \"%s\" options during
replication slot creation",
+    "failover", "two_phase"),
+ errhint("The \"%s\" option can be enabled after \"%s\" state is ready.",
+ "failover", "two_phase"));

I think  errhint here is not correct. We can not alter failover of
replication slot without /o doing create-sub first and then performing
alter-sub. So either we should mention complete details or remove this
hint, otherwise it is confusing.

3)
CreateSubscription:

+ if (opts.twophase && opts.failover)
+ ereport(ERROR,
+ errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot enable both \"%s\" and \"%s\" options at CREATE SUBSCRIPTION",
+    "failover", "two_phase"),
+ errhint("The \"%s\" option can be enabled after  \"%s\" state is ready.",
+ "failover", "two_phase"));

Shall we mention "using Alter Sub" in the errhint?

4)

AlterSubscription:

+ if (sub->twophasestate == LOGICALREP_TWOPHASE_STATE_PENDING &&
+ opts.failover)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("cannot enable failover for a subscription with a pending
two_phase state"));
+state"));

Shall we mention errhint here too?

thanks
Shveta

RE: Fix slot synchronization with two_phase decoding enabled

From

"Zhijie Hou (Fujitsu)"

Date:

28 April, 11:56:52

On Fri, Apr 18, 2025 at 12:29 PM Amit Kapila wrote:
> 
> On Thu, Apr 17, 2025 at 6:14 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
> >
> > -----
> > Fix
> > -----
> >
> > I think we should keep the confirmed_flush even if the previous synced
> > restart_lsn/catalog_xmin is newer. Attachments include a patch for the
> same.
> >
> 
> This will fix the case we are facing but adds a new rule for slot synchronization.
> Can we think of a simpler way to fix this by avoiding updating other slot fields
> (like two_phase, two_phase_at) if restart_lsn or catalog_xmin of the local slot
> is ahead of the remote slot?

Since the fix for xmin advancement during fast-forward decoding has been pushed
(commit aaf9e95), I'm attaching the V2 patch to address the assert failure by
avoiding updating other slot fields (like two_phase, two_phase_at) if
restart_lsn or catalog_xmin of the local slot is ahead.

After commit aaf9e95, it's less likely for the synced slot's restart/catalog to
advance ahead of the source slot (The tests in
040_standby_failover_slots_sync.pl would not trigger it anymore). However, this
can still occur in certain scenarios.

Here is an example scenario with the steps involved:

1. Create a failover replication slot named 'logicalslot' on primary
  and acquire it in the walsender.

2. Log two standby snapshots on primary:
   - `0/3000428` - `running_xacts` (no running transactions)
   - `0/3000460` - `running_xacts` (no running transactions)

3. The walsender sets `0/3000428` as the `candidate_restart_lsn`, skipping the
   second `running_xacts` because `candidate_restart_lsn` is already valid.

4. After receiving a reply from the apply worker, the walsender assigns
   `0/3000428` to `restart_lsn`. At this point, the replication slot
   'logicalslot' has `restart_lsn` set to `0/3000428`.

5. On the standby, execute `pg_sync_replication_slots()` to synchronize
   'logicalslot'.

6. During synchronization, with the initial `restart_lsn` at `0/3000428`, the
   sync slot reaches a consistent point at this position. As a result, it does
   not use this consistent point as `candidate_restart_lsn` (refer to
   `SnapBuildProcessRunningXacts()`).

7. The sync process identifies the second standby snapshot at `0/3000460` and
   uses its LSN as `candidate_restart_lsn`, eventually updating it to
   `restart_lsn`.

8. Now, the synced slot holds `restart_lsn` at `0/3000460`, which is ahead of
   the remote slot on the primary server.

So, if a user prepares a txn and changes the two_phase setting to true after
the steps mentioned above, the value for two_phase_at might get synced to the
standby while skipping the sync of the latest confirmed_flush_lsn. So I think
it's still necessary to fix the assert failure.

BTW, this fix is only for HEAD as back-branches do not sync
two_phase_at field.

Best Regards,
Hou zj

Attachment

v2-0001-Fix-assertion-failure-when-decoding-synced-two-ph.patch

Re: Fix slot synchronization with two_phase decoding enabled

From

shveta malik

Date:

28 April, 12:33:05

On Mon, Apr 28, 2025 at 2:27 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Fri, Apr 18, 2025 at 12:29 PM Amit Kapila wrote:
> >
> > On Thu, Apr 17, 2025 at 6:14 PM Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
> > >
> > > -----
> > > Fix
> > > -----
> > >
> > > I think we should keep the confirmed_flush even if the previous synced
> > > restart_lsn/catalog_xmin is newer. Attachments include a patch for the
> > same.
> > >
> >
> > This will fix the case we are facing but adds a new rule for slot synchronization.
> > Can we think of a simpler way to fix this by avoiding updating other slot fields
> > (like two_phase, two_phase_at) if restart_lsn or catalog_xmin of the local slot
> > is ahead of the remote slot?
>
> Since the fix for xmin advancement during fast-forward decoding has been pushed
> (commit aaf9e95), I'm attaching the V2 patch to address the assert failure by
> avoiding updating other slot fields (like two_phase, two_phase_at) if
> restart_lsn or catalog_xmin of the local slot is ahead.
>

The patch looks good to me. We can have minor comment tweak:

+ * Syncing only two_phase_at, without also syncing the latest
+ * confirmed_lsn, might lead to transactions between the old
+ * confirmed_lsn and two_phase_at being unexpectedly decoded and sent
+ * to the subscriber.

We can append "following a promotion".

thanks
Shveta

Re: Fix slot synchronization with two_phase decoding enabled

From

Nisha Moond

Date:

28 April, 14:03:02

On Mon, Apr 28, 2025 at 12:04 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Fri, Apr 25, 2025 at 5:53 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > Please find the attached v8 patch with above comments addressed.
> > This version includes the documentation updates suggested by
> > Sawada-san at [1], and incorporates his feedback from [2] as well.
> >
>
> Thanks for the patches.
>
>
> 1)
> Regarding documents, these are the details added:
>  a) pg_create_logical_replication_slot  says "The parameters twophase
> and failover cannot be enabled together when creating a replication
> slot."
>  b) CREATE_REPLICATION_SLOT says TWO_PHASE: This option is mutually
> exclusive with failover.
>  c) CREATE SUBSCRIPTION doc says: The options failover and twophase
> cannot be enabled together when creating a subscription.
>  d) ALTER SUB says: When two_phase is in the "pending" state, setting
> failover = true is not permitted. Once two_phase is "enabled",
> failover = true can be set only if the slot's restart_lsn has advanced
> beyond its two_phase_at value.
>
> The individual points mentioned are correct. But nowhere we get
> complete picture as in how user can enable failover and twophase
> together  when setting up a slot and subscription. I think we can add
> brief notes in a,b and c to state how user can actually enable these 2
> together.
> Or can add this detail in single page and let other pages refer/point
> to that page. Thoughts?
>

I have updated the docs with a brief explanation of how both options
can be enabled together for points (a), (b), and (c).
Let’s wait for others’ opinions on whether this detail should stay
consolidated on one page, with other pages referring to it.

> 2)
>
> ReplicationSlotCreate():
>
> + if (two_phase && !IsSyncingReplicationSlots())
> + ereport(ERROR,
> + errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("cannot enable both \"%s\" and \"%s\" options during
> replication slot creation",
> +    "failover", "two_phase"),
> + errhint("The \"%s\" option can be enabled after \"%s\" state is ready.",
> + "failover", "two_phase"));
>
> I think  errhint here is not correct. We can not alter failover of
> replication slot without /o doing create-sub first and then performing
> alter-sub. So either we should mention complete details or remove this
> hint, otherwise it is confusing.
>

Removed the hint. I feel this explanation will be too long for the
HINT and also docs have the details.
~~~

Please find the v9 patch, addressing the above and all other comments as well.

--
Thanks,
Nisha

Attachment

v9-0001-PG17-Approach-3-Fix-slot-synchronization-for-two_.patch

RE: Fix slot synchronization with two_phase decoding enabled

From

"Zhijie Hou (Fujitsu)"

Date:

28 April, 14:33:12

On Mon, Apr 28, 2025 at 5:33 PM shveta malik wrote:
> 
> On Mon, Apr 28, 2025 at 2:27 PM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Fri, Apr 18, 2025 at 12:29 PM Amit Kapila wrote:
> > >
> > > On Thu, Apr 17, 2025 at 6:14 PM Zhijie Hou (Fujitsu)
> > > <houzj.fnst@fujitsu.com>
> > > >
> > > > -----
> > > > Fix
> > > > -----
> > > >
> > > > I think we should keep the confirmed_flush even if the previous
> > > > synced restart_lsn/catalog_xmin is newer. Attachments include a
> > > > patch for the
> > > same.
> > > >
> > >
> > > This will fix the case we are facing but adds a new rule for slot
> synchronization.
> > > Can we think of a simpler way to fix this by avoiding updating other
> > > slot fields (like two_phase, two_phase_at) if restart_lsn or
> > > catalog_xmin of the local slot is ahead of the remote slot?
> >
> > Since the fix for xmin advancement during fast-forward decoding has
> > been pushed (commit aaf9e95), I'm attaching the V2 patch to address
> > the assert failure by avoiding updating other slot fields (like
> > two_phase, two_phase_at) if restart_lsn or catalog_xmin of the local slot is
> ahead.
> >
> 
> The patch looks good to me. We can have minor comment tweak:
> 
> + * Syncing only two_phase_at, without also syncing the latest
> + * confirmed_lsn, might lead to transactions between the old
> + * confirmed_lsn and two_phase_at being unexpectedly decoded and sent
> + * to the subscriber.
> 
> We can append "following a promotion".

Thanks for reviewing. Here is V3 patch that addressed it.

BTW, I also did some tests to confirm the catalog_xmin could
still be ahead in some case, and here is an example:

1. Create a failover replication slot named 'logicalslot' on primary
  and acquire it in the walsender.

2. Log two standby snapshots on primary. Before logging, call txid_current()
 To assign a xid, so that each standby snapshot will hold a new xid in its oldestrunningXid field:
   - txid_current();
   - `0/3000420` - `running_xacts` (no running transactions, oldestrunningXid = 755)
   - txid_current();
   - `0/3000488` - `running_xacts` (no running transactions, oldestrunningXid = 756)

3. The walsender sets `0/3000420` as the `candidate_restart_lsn`, 755 as
   `candidate_catalog_xmin`, skipping the second `running_xacts` because
   `candidate_restart_lsn`/`candidate_catalog_xmin` is already valid.

4. After receiving a reply from the apply worker, the walsender assigns
   `0/3000420` to `restart_lsn`, `755` to `catalog_xmin`. At this point, the
   replication slot 'logicalslot' has `restart_lsn` set to `0/3000420`,
   `catalog_xmin` set to `755`.

5. On the standby, execute `pg_sync_replication_slots()` to synchronize
   'logicalslot'.

6. During synchronization, with the initial `restart_lsn` at `0/3000420`, the
   sync slot reaches a consistent point at this position. As a result, it does
   not update `candidate_restart_lsn` and `candidate_catalog_xmin` at
   consistent point (refer to `SnapBuildProcessRunningXacts()`).

7. The sync process identifies the second standby snapshot at `0/3000488` and
   uses its LSN as `candidate_restart_lsn`, and use the oldestrunningXid `756`
   as `candidate_catalog_xmin`, eventually updating it to `restart_lsn` and
   `catalog_xmin`.

8. Now, the synced slot holds `restart_lsn` at `0/3000488`, `catalog_xmin` at
   `756`, which are all ahead of the remote slot on the primary server.

Attaching a script to reproduce the same.

Note that, to reproduce this stably, we'd better modify the value of
LOG_SNAPSHOT_INTERVAL_MS in bgwriter.c to a bigger number to avoid
unexpected xl_running_xacts logging.

Best Regards,
Hou zj

On Mon, Apr 28, 2025 at 7:33 PM Zhijie Hou (Fujitsu) wrote:
> 
> Thanks for reviewing. Here is V3 patch that addressed it.
> 
> BTW, I also did some tests to confirm the catalog_xmin could still be 
> ahead in some case, and here is an example:
> 
> 1. Create a failover replication slot named 'logicalslot' on primary
>   and acquire it in the walsender.
> 
> 2. Log two standby snapshots on primary. Before logging, call 
> txid_current() To assign a xid, so that each standby snapshot will 
> hold a new xid in its oldestrunningXid field:
>    - txid_current();
>    - `0/3000420` - `running_xacts` (no running transactions, 
> oldestrunningXid = 755)
>    - txid_current();
>    - `0/3000488` - `running_xacts` (no running transactions, 
> oldestrunningXid = 756)
> 
> 3. The walsender sets `0/3000420` as the `candidate_restart_lsn`, 755 as
>    `candidate_catalog_xmin`, skipping the second `running_xacts` because
>    `candidate_restart_lsn`/`candidate_catalog_xmin` is already valid.
> 
> 4. After receiving a reply from the apply worker, the walsender assigns
>    `0/3000420` to `restart_lsn`, `755` to `catalog_xmin`. At this point, the
>    replication slot 'logicalslot' has `restart_lsn` set to `0/3000420`,
>    `catalog_xmin` set to `755`.
> 
> 5. On the standby, execute `pg_sync_replication_slots()` to synchronize
>    'logicalslot'.
> 
> 6. During synchronization, with the initial `restart_lsn` at `0/3000420`, the
>    sync slot reaches a consistent point at this position. As a result, it does
>    not update `candidate_restart_lsn` and `candidate_catalog_xmin` at
>    consistent point (refer to `SnapBuildProcessRunningXacts()`).
> 
> 7. The sync process identifies the second standby snapshot at 
> `0/3000488` and
>    uses its LSN as `candidate_restart_lsn`, and use the 
> oldestrunningXid `756`
>    as `candidate_catalog_xmin`, eventually updating it to `restart_lsn` and
>    `catalog_xmin`.
> 
> 8. Now, the synced slot holds `restart_lsn` at `0/3000488`, `catalog_xmin` at
>    `756`, which are all ahead of the remote slot on the primary server.
> 
> Attaching a script to reproduce the same.
> 
> Note that, to reproduce this stably, we'd better modify the value of 
> LOG_SNAPSHOT_INTERVAL_MS in bgwriter.c to a bigger number to avoid 
> unexpected xl_running_xacts logging.

In addition to above steps, for those interested in reproducing the specific
scenario where two_phase_at advances past the synced confirmed_flush, I'm
attaching a new script. This script can reproduce the issue after applying the
injection points I provided.

Best Regards,
Hou zj

On Tue, Apr 29, 2025 at 2:51 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Mon, Apr 28, 2025 at 4:33 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > Please find the v9 patch, addressing the above and all other comments as well.
> >
>
> Thanks for the patch.
>
> 1)
>
> +          The default is false. This option cannot be set together with
> +          <literal>two_phase</literal> when creating the slot. However, once
> +          the slot is created with <literal>two_phase=true</literal>, the
> +          <literal>failover</literal> can be set to true after the
> +          <literal>restart_lsn</literal> has advanced beyond its
> +          <literal>two_phase_at</literal> value
>
> As the user is not aware of two_phase_at, we shall change all the docs
> to have a more user friendly explanation.
>

Done.

> 2)
>  # Confirm that the invalidated slot has been dropped.
>  $standby1->wait_for_log(
> - qr/dropped replication slot "lsub1_slot" of database with OID
> [0-9]+/, $log_offset);
> + qr/dropped replication slot "lsub1_slot" of database with OID [0-9]+/,
> + $log_offset);
>
> I think this change is not needed.
>

The perltidy is auto-correcting it now, though it didn't for earlier
patches. I've kept the original for now.
~~~

Thanks for review, please find the updated patch v10.

--
Thanks,
Nisha

Attachment

v10-0001-PG17-Approach-3-Fix-slot-synchronization-for-two.patch

Re: Fix slot synchronization with two_phase decoding enabled

From

Masahiko Sawada

Date:

30 April, 03:14:38

On Tue, Apr 29, 2025 at 5:00 AM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> On Tue, Apr 29, 2025 at 2:51 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Mon, Apr 28, 2025 at 4:33 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> > >
> > > Please find the v9 patch, addressing the above and all other comments as well.
> > >
> >
> > Thanks for the patch.
> >
> > 1)
> >
> > +          The default is false. This option cannot be set together with
> > +          <literal>two_phase</literal> when creating the slot. However, once
> > +          the slot is created with <literal>two_phase=true</literal>, the
> > +          <literal>failover</literal> can be set to true after the
> > +          <literal>restart_lsn</literal> has advanced beyond its
> > +          <literal>two_phase_at</literal> value
> >
> > As the user is not aware of two_phase_at, we shall change all the docs
> > to have a more user friendly explanation.
> >
>
> Done.
>
> > 2)
> >  # Confirm that the invalidated slot has been dropped.
> >  $standby1->wait_for_log(
> > - qr/dropped replication slot "lsub1_slot" of database with OID
> > [0-9]+/, $log_offset);
> > + qr/dropped replication slot "lsub1_slot" of database with OID [0-9]+/,
> > + $log_offset);
> >
> > I think this change is not needed.
> >
>
> The perltidy is auto-correcting it now, though it didn't for earlier
> patches. I've kept the original for now.
> ~~~
>
> Thanks for review, please find the updated patch v10.

Thank you for updating the patch! Here are some comments on v10.

We refer to the following comment in ReplicationSlotCreate() from many places:

/*
 * Do not allow users to enable both failover and two_phase for slots.
 *
 * This is because the two_phase_at field of a slot, which tracks the
 * LSN, from which two-phase decoding starts, is not synchronized to
 * standby servers. Without two_phase_at, the logical decoding might
 * incorrectly identify prepared transaction as already replicated to
 * the subscriber after promotion of standby server, causing them to
 * be skipped.
 *
 * However, both failover and two_phase enabled slots can be created
 * during slot synchronization because we need to retain the same
 * values as the remote slot.
 */

I think this is an important implementation restriction, so how about
describing the overall idea of combination of two_phase and failover
in the comment atop slotsync.c or worker.c? It would be easier to
find.

---
+   if (failover && MyReplicationSlot->data.two_phase &&
+       MyReplicationSlot->data.restart_lsn <
MyReplicationSlot->data.two_phase_at)
+       ereport(ERROR,
+               errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+               errmsg("cannot enable failover for a two-phase enabled
replication slot"),
+               errdetail("The slot need to consume change upto %X/%X
to enable failover.",
+
LSN_FORMAT_ARGS(MyReplicationSlot->data.two_phase_at)));

I think it's better to mention the reason in the error message why we
cannot enable two-phase in this situation. How about:

-               errmsg("cannot enable failover for a two-phase enabled
replication slot"),
+               errmsg("cannot enable failover for a two-phase enabled
replication slot due to unconsumed changes"),

---
+       ereport(ERROR,
+               errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+               errmsg("cannot enable both \"%s\" and \"%s\" options
at CREATE SUBSCRIPTION",
+                      "failover", "two_phase"),
+               errhint("The \"%s\" option can be enabled after \"%s\"
state is ready using ALTER SUBSCRIPTION.",
+                       "failover", "two_phase"));

How about rewriting the errhint message to:

-               errhint("The \"%s\" option can be enabled after \"%s\"
state is ready using ALTER SUBSCRIPTION.",
-                       "failover", "two_phase"));
+               errhint("Use ALTER SUBSCRIPTION ... SET (failover =
true) to enable \"%s\" after two_phase state is ready",
+                       "failover"));

---
@@ -29073,9 +29073,13 @@ postgres=# SELECT '0/0'::pg_lsn +
pd.segment_number * ps.setting::int + :offset
         <parameter>failover</parameter>, when set to true,
         specifies that this slot is enabled to be synced to the
         standbys so that logical replication can be resumed after
-        failover. A call to this function has the same effect as
-        the replication protocol command
-        <literal>CREATE_REPLICATION_SLOT ... LOGICAL</literal>.
+        failover. The parameters <parameter>twophase</parameter> and
+        <parameter>failover</parameter> cannot be enabled together when
+        creating a replication slot. However, a slot created with
<parameter>twophase</parameter>
+        enabled can later have <parameter>failover</parameter> set to true via
+        <command>ALTER SUBSCRIPTION</command>, if a subscription is using this
+        slot. A call to this function has the same effect as the replication
+        protocol command <literal>CREATE_REPLICATION_SLOT ...
LOGICAL</literal>.
        </para></entry>
       </row>

Can we also mention ALTER_REPLICATION to enable the failover for slots
that are not used by subscriptions?

---
+# Also wait for two-phase to be enabled
+$subscriber1->poll_query_until(
+   'postgres', qq[
+   SELECT count(1) = 0 FROM pg_subscription WHERE subname =
'regress_mysub2' and subtwophasestate NOT IN ('e');]
+) or die "Timed out while waiting for subscriber to enable twophase";

Since we check the twophasestate of the specific slot, how about doing
something like:

+ok( $subscriber1->poll_query_until(
+   'postgres',
+       qq[SELECT subtwophasestate FROM pg_subscription WHERE subname
= 'regress_mysub2';],
+   'e',
+    ),
+    "Timed out while waiting for subscriber to enable twophase");

---
+
+# Also wait for two-phase to be enabled
+$subscriber1->poll_query_until(
+   'postgres', qq[
+   SELECT count(1) = 0 FROM pg_subscription WHERE subname =
'regress_mysub2' and subtwophasestate NOT IN ('e');]
+) or die "Timed out while waiting for subscriber to enable twophase";
+
+# Try to enable the failover for the subscription, should give error
+($result, $stdout, $stderr) = $subscriber1->psql(
+   'postgres',
+   "ALTER SUBSCRIPTION regress_mysub2 DISABLE;
+    ALTER SUBSCRIPTION regress_mysub2 SET (failover = true);");
+ok( $stderr =~
+     qr/ERROR:  could not alter replication slot "lsub2_slot": ERROR:
 cannot enable failover for a two-phase enabled replication slot/,
+   "failover can't be enabled if restart_lsn < two_phase_at on a
two_phase subscription."
+);

I think it's possible between two tests that the walsender consumes
some changes (e.g. generated by autovacuums) then the second check
fails (i.e., ALTER SUBSCRIPTION SET (failover = ture) succeeds).

---
+# Test that SQL API disallows slot creation with both two_phase and
failover enabled
+($result, $stdout, $stderr) = $publisher->psql('postgres',
+   "SELECT pg_create_logical_replication_slot('regress_mysub3',
'pgoutput', false, true, true);"
+);
+ok( $stderr =~
+     /ERROR:  cannot enable both "failover" and "two_phase" options
during replication slot creation/,
+   "cannot create slot with both two_phase and failover enabled");

Isn't this test already covered by test_decoding/sql/slot.sql?

I've attached a patch that contains comment changes I mentioned above.
Please incorporate it if you agree with them.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Attachment

comment_improvement_v10.patch

RE: Fix slot synchronization with two_phase decoding enabled

From

"Zhijie Hou (Fujitsu)"

Date:

30 April, 12:37:49

On Tue, Apr 29, 2025 at 6:57 PM Amit Kapila wrote:
> 
> On Tue, Apr 29, 2025 at 3:01 AM Masahiko Sawada
> <sawada.mshk@gmail.com> wrote:
> >
> > Your fix looks good to me. Is it worth considering putting an
> > assertion to verify if new two_phase_at is equal to or greater than
> > confirmed_lsn (or at least it doesn't go backward), when syncing
> > two_phase_at?
> >
> 
> Yeah, it makes sense. But the condition should be reverse (two_phase_at
> should be less than or equal to confirmed_flush). I have done that, changed a
> few comments, and committed the patch.

I noticed a BF failure[1] related to the fix, where the recently added assertion
Assert(slot->data.two_phase_at <= slot->data.confirmed_flush) was broken. After
examining the logs and code, I couldn't identify any functionality issues. Here is
my analysis:

AFAICS, the last slotsync cycle should have retrieved the latest confirmed_flush and
two_phase_at from the source slot, both expected to be 0/6005150. Here are
some details:

The standby's log[1] shows the source slot's two_phase_at as 0/6005150.
Meanwhile, the publisher's log reveals that the source slot's confirmed_flush
was already 0/6005150 before the last slotsync. Therefore, the last slotsync
cycle should have fetched confirmed_flush: 0/6005150, two_phase_at: 0/6005150.

If we assume remote_slot->confirmed_lsn during the last sync cycle is
0/6005150, then either the local slot's confirmed_lsn had already been updated
to this value in a prior sync, leading it to skip the update in the last cycle
(in which case, the assertion shouldn't be broken), or it should enter the slot
update branch to set the synced slot to 0/6005150 (see
update_local_synced_slot() for details). Thus, theoretically, the assertion
wouldn't fail.

Beyond functionality problems, another possibility might be the CPU reordered
memory access, causing the assertion to execute before updating
confirmed_flush. However, we lack the information needed to verify this, so one
idea is to add some DEBUG message in update_local_synced_slot() to facilitate
tracking if the issue recurs.

I'll continue to investigate, but any suggestions or insights would be greatly
appreciated.

[1]
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=calliphoridae&dt=2025-04-29%2009%3A11%3A31&stg=recovery-check

[2]
2025-04-29 09:18:05.641 UTC [1023004][client backend][0/2:0] LOG:  statement: SELECT two_phase AND '0/6005150' =
two_phase_atfrom pg_replication_slots WHERE slot_name = 'lsub1_slot' AND synced AND NOT temporary;

[3]
2025-04-29 09:18:05.487 UTC [1022913][walsender][29/0:0] LOG:  0/6004FC8 has been already streamed, forwarding to
0/6005150
2025-04-29 09:18:05.487 UTC [1022913][walsender][29/0:0] STATEMENT:  START_REPLICATION SLOT "lsub1_slot" LOGICAL
0/6004FC8(proto_version '4', streaming 'parallel', two_phase 'on', origin 'any', publication_names '"regress_mypub"')

2025-04-29 09:18:05.489 UTC [1022911][client backend][:0] LOG:  disconnection: session time: 0:00:00.005 user=bf
database=postgreshost=[local]

2025-04-29 09:18:05.516 UTC [1022913][walsender][29/0:0] LOG:  starting logical decoding for slot "lsub1_slot"
2025-04-29 09:18:05.516 UTC [1022913][walsender][29/0:0] DETAIL:  Streaming transactions committing after 0/6005150,
readingWAL from 0/6004A00.

2025-04-29 09:18:05.516 UTC [1022913][walsender][29/0:0] STATEMENT:  START_REPLICATION SLOT "lsub1_slot" LOGICAL
0/6004FC8(proto_version '4', streaming 'parallel', two_phase 'on', origin 'any', publication_names '"regress_mypub"')

2025-04-29 09:18:05.518 UTC [1022687][client backend][9/19:0] LOG:  statement: SELECT slot_name, plugin,
confirmed_flush_lsn,restart_lsn, catalog_xmin, two_phase, two_phase_at, failover, database, invalidation_reason FROM
pg_catalog.pg_replication_slotsWHERE failover and NOT temporary

2025-04-29 09:18:05.524 UTC [1022928][not initialized][:0] LOG:  connection received: host=[local]

Best Regards,
Hou zj

Re: Fix slot synchronization with two_phase decoding enabled

From

Masahiko Sawada

Date:

01 May, 01:26:47

On Wed, Apr 30, 2025 at 2:38 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Tue, Apr 29, 2025 at 6:57 PM Amit Kapila wrote:
> >
> > On Tue, Apr 29, 2025 at 3:01 AM Masahiko Sawada
> > <sawada.mshk@gmail.com> wrote:
> > >
> > > Your fix looks good to me. Is it worth considering putting an
> > > assertion to verify if new two_phase_at is equal to or greater than
> > > confirmed_lsn (or at least it doesn't go backward), when syncing
> > > two_phase_at?
> > >
> >
> > Yeah, it makes sense. But the condition should be reverse (two_phase_at
> > should be less than or equal to confirmed_flush). I have done that, changed a
> > few comments, and committed the patch.
>
> I noticed a BF failure[1] related to the fix, where the recently added assertion
> Assert(slot->data.two_phase_at <= slot->data.confirmed_flush) was broken. After
> examining the logs and code, I couldn't identify any functionality issues.

Yeah, that's weird.

> AFAICS, the last slotsync cycle should have retrieved the latest confirmed_flush and
> two_phase_at from the source slot, both expected to be 0/6005150. Here are
> some details:
>
> The standby's log[1] shows the source slot's two_phase_at as 0/6005150.
> Meanwhile, the publisher's log reveals that the source slot's confirmed_flush
> was already 0/6005150 before the last slotsync. Therefore, the last slotsync
> cycle should have fetched confirmed_flush: 0/6005150, two_phase_at: 0/6005150.
>
> If we assume remote_slot->confirmed_lsn during the last sync cycle is
> 0/6005150, then either the local slot's confirmed_lsn had already been updated
> to this value in a prior sync, leading it to skip the update in the last cycle
> (in which case, the assertion shouldn't be broken), or it should enter the slot
> update branch to set the synced slot to 0/6005150 (see
> update_local_synced_slot() for details). Thus, theoretically, the assertion
> wouldn't fail.

Agreed with your analysis. After enabling the subscription with
two_phase=true, the primary server has the following logs that
indicate that logical decoding started from 0/6005150, not 0/6004FC8.
Given that the slot's two_phase was enabled at this time, the slot's
confirmed_flush and two_phase_at were 0/6005150.

2025-04-29 09:18:05.487 UTC [1022913][walsender][29/0:0] LOG:
received replication command: START_REPLICATION SLOT "lsub1_slot"
LOGICAL 0/6004FC8 (proto_version '4', streaming 'parallel', two_phase
'on', origin 'any', publication_names '"regress_mypub"')
2025-04-29 09:18:05.487 UTC [1022913][walsender][29/0:0] STATEMENT:
START_REPLICATION SLOT "lsub1_slot" LOGICAL 0/6004FC8 (proto_version
'4', streaming 'parallel', two_phase 'on', origin 'any',
publication_names '"regress_mypub"')
2025-04-29 09:18:05.487 UTC [1022913][walsender][29/0:0] LOG:
acquired logical replication slot "lsub1_slot"
2025-04-29 09:18:05.487 UTC [1022913][walsender][29/0:0] STATEMENT:
START_REPLICATION SLOT "lsub1_slot" LOGICAL 0/6004FC8 (proto_version
'4', streaming 'parallel', two_phase 'on', origin 'any',
publication_names '"regress_mypub"')
2025-04-29 09:18:05.487 UTC [1022913][walsender][29/0:0] LOG:
0/6004FC8 has been already streamed, forwarding to 0/6005150
2025-04-29 09:18:05.487 UTC [1022913][walsender][29/0:0] STATEMENT:
START_REPLICATION SLOT "lsub1_slot" LOGICAL 0/6004FC8 (proto_version
'4', streaming 'parallel', two_phase 'on', origin 'any',
publication_names '"regress_mypub"')
2025-04-29 09:18:05.516 UTC [1022913][walsender][29/0:0] LOG:
starting logical decoding for slot "lsub1_slot"
2025-04-29 09:18:05.516 UTC [1022913][walsender][29/0:0] DETAIL:
Streaming transactions committing after 0/6005150, reading WAL from
0/6004A00.
2025-04-29 09:18:05.516 UTC [1022913][walsender][29/0:0] STATEMENT:
START_REPLICATION SLOT "lsub1_slot" LOGICAL 0/6004FC8 (proto_version
'4', streaming 'parallel', two_phase 'on', origin 'any',
publication_names '"regress_mypub"')

>
> Beyond functionality problems, another possibility might be the CPU reordered
> memory access, causing the assertion to execute before updating
> confirmed_flush.

Not sure CPU reordered memory access could matter in this case but I
don't have other ideas of possible causes.

> However, we lack the information needed to verify this, so one
> idea is to add some DEBUG message in update_local_synced_slot() to facilitate
> tracking if the issue recurs.

That would be a good idea.

Also, it's unrelated to this issue but probably we are better off
doing this assertion check only when enabling two_phase. The assertion
is currently checked even when disabling the two_phase, which seems
not to make sense, and we don't clear two_phase_at value when
disabling two_phase by ReplicationSlotAlter().

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: Fix slot synchronization with two_phase decoding enabled

From

Nisha Moond

Date:

02 May, 10:27:25

On Wed, Apr 30, 2025 at 5:45 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Tue, Apr 29, 2025 at 5:00 AM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> Thank you for updating the patch! Here are some comments on v10.
>

Thanks for reviewing the patch!

> ---
> +
> +# Also wait for two-phase to be enabled
> +$subscriber1->poll_query_until(
> +   'postgres', qq[
> +   SELECT count(1) = 0 FROM pg_subscription WHERE subname =
> 'regress_mysub2' and subtwophasestate NOT IN ('e');]
> +) or die "Timed out while waiting for subscriber to enable twophase";
> +
> +# Try to enable the failover for the subscription, should give error
> +($result, $stdout, $stderr) = $subscriber1->psql(
> +   'postgres',
> +   "ALTER SUBSCRIPTION regress_mysub2 DISABLE;
> +    ALTER SUBSCRIPTION regress_mysub2 SET (failover = true);");
> +ok( $stderr =~
> +     qr/ERROR:  could not alter replication slot "lsub2_slot": ERROR:
>  cannot enable failover for a two-phase enabled replication slot/,
> +   "failover can't be enabled if restart_lsn < two_phase_at on a
> two_phase subscription."
> +);
>
> I think it's possible between two tests that the walsender consumes
> some changes (e.g. generated by autovacuums) then the second check
> fails (i.e., ALTER SUBSCRIPTION SET (failover = ture) succeeds).
>

Yes, this is a possibility. To account for this negative test case
where restart_lsn < two_phase_at, I updated the test to attempt
altering a two_phase-enabled slot without any associated subscription.

> ---
> +# Test that SQL API disallows slot creation with both two_phase and
> failover enabled
> +($result, $stdout, $stderr) = $publisher->psql('postgres',
> +   "SELECT pg_create_logical_replication_slot('regress_mysub3',
> 'pgoutput', false, true, true);"
> +);
> +ok( $stderr =~
> +     /ERROR:  cannot enable both "failover" and "two_phase" options
> during replication slot creation/,
> +   "cannot create slot with both two_phase and failover enabled");
>
> Isn't this test already covered by test_decoding/sql/slot.sql?
>

Yes, it is covered in slot.sql, hence removed from here.

> I've attached a patch that contains comment changes I mentioned above.
> Please incorporate it if you agree with them.
>

I have incorporated the suggested changes and updated a couple more
places accordingly.
~~~

Please find the v11 patch addressing the above points and all other
comments. I have also optimized the test by reducing the number of
subscriptions and slots.

--
Thanks,
Nisha

Attachment

v11-0001-PG17-Approach-3-Fix-slot-synchronization-for-two.patch

Re: Fix slot synchronization with two_phase decoding enabled

From

shveta malik

Date:

02 May, 12:35:15

On Fri, May 2, 2025 at 12:57 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
>
> Please find the v11 patch addressing the above points and all other
> comments. I have also optimized the test by reducing the number of
> subscriptions and slots.
>

Thanks for the patch. Few comments:

1)

pg_upgrade/t/003_logical_slots.pl:

- "SELECT slot_name, two_phase, failover FROM pg_replication_slots");
-is($result, qq(regress_sub|t|t), 'check the slot exists on new cluster');
+ "SELECT slot_name, two_phase FROM pg_replication_slots");
+is($result, qq(regress_sub|t), 'check the slot exists on new cluster');

With this change, the failover property test during upgrade is
removed. Shall we add it again either using ALTER SUB to enable
failover and two_phase together or a new subscription with failover
alone?

2)
+          The default is false. This option cannot be set together with
+          <literal>failover</literal> when creating a logical replication slot.
+          However, once the slot is created with <literal>two_phase =
true</literal>,
+          <literal>failover</literal> can be set to true after the slot has
+          consumed all transactions up to the point where two-phase decoding
+          was enabled.


Suggestion:  all transactions --> all the changes.

thanks
Shveta

Re: Fix slot synchronization with two_phase decoding enabled

From

Tom Lane

Date:

03 May, 20:23:51

BF member mule just showed what seems an identical failure [1]:

2025-05-03 17:46:20.088 CEST [24308:3] LOG:  database system is ready to accept read-only connections
2025-05-03 17:46:20.091 CEST [24321:1] LOG:  slot sync worker started
2025-05-03 17:46:20.100 CEST [24322:1] LOG:  started streaming WAL from primary at 0/6000000 on timeline 1
2025-05-03 17:46:20.117 CEST [24321:2] LOG:  starting logical decoding for slot "lsub1_slot"
2025-05-03 17:46:20.117 CEST [24321:3] DETAIL:  Streaming transactions committing after 0/60049C8, reading WAL from
0/5000048.
2025-05-03 17:46:20.117 CEST [24321:4] LOG:  logical decoding found consistent point at 0/5000048
2025-05-03 17:46:20.117 CEST [24321:5] DETAIL:  There are no running transactions.
TRAP: failed Assert("slot->data.two_phase_at <= slot->data.confirmed_flush"), File: "slotsync.c", Line: 311, PID: 24321
postgres: standby1: slotsync worker (ExceptionalCondition+0x59)[0x556c3b8cb3e9]
postgres: standby1: slotsync worker (+0x4cb100)[0x556c3b706100]
postgres: standby1: slotsync worker (+0x4cba4c)[0x556c3b706a4c]
postgres: standby1: slotsync worker (ReplSlotSyncWorkerMain+0x258)[0x556c3b707598]
postgres: standby1: slotsync worker (postmaster_child_launch+0x102)[0x556c3b6d6962]
postgres: standby1: slotsync worker (+0x49daea)[0x556c3b6d8aea]
postgres: standby1: slotsync worker (+0x49fb3d)[0x556c3b6dab3d]
postgres: standby1: slotsync worker (PostmasterMain+0xd3f)[0x556c3b6dc04f]
postgres: standby1: slotsync worker (main+0x1ca)[0x556c3b3bee1a]
/lib/x86_64-linux-gnu/libc.so.6(+0x2724a)[0x7fe08da4824a]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x85)[0x7fe08da48305]
postgres: standby1: slotsync worker (_start+0x21)[0x556c3b3bf421]
2025-05-03 17:46:20.337 CEST [24308:4] LOG:  slot sync worker process (PID 24321) was terminated by signal 6: Aborted

            regards, tom lane

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mule&dt=2025-05-03%2015%3A30%3A02

Re: Fix slot synchronization with two_phase decoding enabled

From

Masahiko Sawada

Date:

04 May, 12:03:07

On Sat, May 3, 2025 at 10:23 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> BF member mule just showed what seems an identical failure [1]:
>
> 2025-05-03 17:46:20.088 CEST [24308:3] LOG:  database system is ready to accept read-only connections
> 2025-05-03 17:46:20.091 CEST [24321:1] LOG:  slot sync worker started
> 2025-05-03 17:46:20.100 CEST [24322:1] LOG:  started streaming WAL from primary at 0/6000000 on timeline 1
> 2025-05-03 17:46:20.117 CEST [24321:2] LOG:  starting logical decoding for slot "lsub1_slot"
> 2025-05-03 17:46:20.117 CEST [24321:3] DETAIL:  Streaming transactions committing after 0/60049C8, reading WAL from
0/5000048.
> 2025-05-03 17:46:20.117 CEST [24321:4] LOG:  logical decoding found consistent point at 0/5000048
> 2025-05-03 17:46:20.117 CEST [24321:5] DETAIL:  There are no running transactions.
> TRAP: failed Assert("slot->data.two_phase_at <= slot->data.confirmed_flush"), File: "slotsync.c", Line: 311, PID:
24321
> postgres: standby1: slotsync worker (ExceptionalCondition+0x59)[0x556c3b8cb3e9]
> postgres: standby1: slotsync worker (+0x4cb100)[0x556c3b706100]
> postgres: standby1: slotsync worker (+0x4cba4c)[0x556c3b706a4c]
> postgres: standby1: slotsync worker (ReplSlotSyncWorkerMain+0x258)[0x556c3b707598]
> postgres: standby1: slotsync worker (postmaster_child_launch+0x102)[0x556c3b6d6962]
> postgres: standby1: slotsync worker (+0x49daea)[0x556c3b6d8aea]
> postgres: standby1: slotsync worker (+0x49fb3d)[0x556c3b6dab3d]
> postgres: standby1: slotsync worker (PostmasterMain+0xd3f)[0x556c3b6dc04f]
> postgres: standby1: slotsync worker (main+0x1ca)[0x556c3b3bee1a]
> /lib/x86_64-linux-gnu/libc.so.6(+0x2724a)[0x7fe08da4824a]
> /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x85)[0x7fe08da48305]
> postgres: standby1: slotsync worker (_start+0x21)[0x556c3b3bf421]
> 2025-05-03 17:46:20.337 CEST [24308:4] LOG:  slot sync worker process (PID 24321) was terminated by signal 6: Aborted
>

While I cannot be entirely certain of my analysis, I believe the root
cause might be related to the backward movement of the confirmed_flush
LSN. The following scenario seems possible:

1. The walsender enables the two_phase and sets two_phase_at (which
should be the same as confirmed_flush).
2. The slot's confirmed_flush regresses for some reason.
3. The slotsync worker retrieves the remote slot information and
enables two_phase for the local slot.

If I recall correctly, we previously discussed[1] how a slot's
confirmed_flush LSN could move backward depending on the feedback
messages received from the downstream system. Based on the following
primary server's logs, it appears possible that the subscriber sent
feedback LSNs older than the slot's confirmed_flush, consequently
causing the confirmed_flush to move backward. However, I should note
that this is currently a hypothesis, as I haven't yet been able to
reproduce and verify this scenario.

Regards,

[1] https://www.postgresql.org/message-id/85fff40e-148b-4e86-b921-b4b846289132%40vondra.me

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: Fix slot synchronization with two_phase decoding enabled

From

Nisha Moond

Date:

05 May, 12:59:21

On Fri, May 2, 2025 at 3:05 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Fri, May 2, 2025 at 12:57 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> >
> > Please find the v11 patch addressing the above points and all other
> > comments. I have also optimized the test by reducing the number of
> > subscriptions and slots.
> >
>
> Thanks for the patch. Few comments:
>
> 1)
>
> pg_upgrade/t/003_logical_slots.pl:
>
> - "SELECT slot_name, two_phase, failover FROM pg_replication_slots");
> -is($result, qq(regress_sub|t|t), 'check the slot exists on new cluster');
> + "SELECT slot_name, two_phase FROM pg_replication_slots");
> +is($result, qq(regress_sub|t), 'check the slot exists on new cluster');
>
> With this change, the failover property test during upgrade is
> removed. Shall we add it again either using ALTER SUB to enable
> failover and two_phase together or a new subscription with failover
> alone?
>

If ALTER SUB is used to set failover for the existing subscription,
then pg_upgrade will fail while trying to create a slot with both
failover and two_phase on the upgraded node $newpub.
So, I've implemented the other suggestion by adding a separate
pub-subs to verify the failover property.

> 2)
> +          The default is false. This option cannot be set together with
> +          <literal>failover</literal> when creating a logical replication slot.
> +          However, once the slot is created with <literal>two_phase =
> true</literal>,
> +          <literal>failover</literal> can be set to true after the slot has
> +          consumed all transactions up to the point where two-phase decoding
> +          was enabled.
>
>
> Suggestion:  all transactions --> all the changes.
>

Done.
~~~

Please find the v12 patch with above suggested changes.

--
Thanks,
Nisha

Attachment

v12-0001-PG17-Approach-3-Fix-slot-synchronization-for-two.patch

Re: Fix slot synchronization with two_phase decoding enabled

From

Amit Kapila

Date:

05 May, 13:59:18

On Sun, May 4, 2025 at 2:33 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> While I cannot be entirely certain of my analysis, I believe the root
> cause might be related to the backward movement of the confirmed_flush
> LSN. The following scenario seems possible:
>
> 1. The walsender enables the two_phase and sets two_phase_at (which
> should be the same as confirmed_flush).
> 2. The slot's confirmed_flush regresses for some reason.
> 3. The slotsync worker retrieves the remote slot information and
> enables two_phase for the local slot.
>

Yes, this is possible. Here is my theory as to how it can happen in
the current case. In the failed test, after the primary has prepared a
transaction, the transaction won't be replicated to the subscriber as
two_phase was not enabled for the slot. However, subsequent keepalive
messages can send the latest WAL location to the subscriber and get
the confirmation of the same from the subscriber without its origin
being moved. Now, after we restart the apply worker (due to
disable/enable for a subscription), it will use the previous
origin_lsn to temporarily move back the confirmed flush LSN as
explained in one of the previous emails in another thread [1]. During
this temporary movement of confirm flush LSN, the slotsync worker
fetches the two_phase_at and confirm_flush_lsn values, leading to the
assertion failure. We see this issue intermittently because it depends
on the timing of slotsync worker's request to fetch the slot's value.

If this theory is correct, then we need something on the lines of what
Vignesh proposed in email [2] (Confirm_flush_dont_allow_backward) to
fix it.

[1]: https://www.postgresql.org/message-id/CAA4eK1%2BzWQwOe5G8zCYGvErnaXh5%2BDbyg_A1Z3uywSf_4%3DT0UA%40mail.gmail.com
[2]: https://www.postgresql.org/message-id/CALDaNm3hgow2%2BoEov5jBk4iYP5eQrUCF1yZtW7%2BdV3J__p4KLQ%40mail.gmail.com

--
With Regards,
Amit Kapila.

Re: Fix slot synchronization with two_phase decoding enabled

From

Masahiko Sawada

Date:

05 May, 21:55:46

On Mon, May 5, 2025 at 3:59 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Sun, May 4, 2025 at 2:33 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > While I cannot be entirely certain of my analysis, I believe the root
> > cause might be related to the backward movement of the confirmed_flush
> > LSN. The following scenario seems possible:
> >
> > 1. The walsender enables the two_phase and sets two_phase_at (which
> > should be the same as confirmed_flush).
> > 2. The slot's confirmed_flush regresses for some reason.
> > 3. The slotsync worker retrieves the remote slot information and
> > enables two_phase for the local slot.
> >
>
> Yes, this is possible. Here is my theory as to how it can happen in
> the current case. In the failed test, after the primary has prepared a
> transaction, the transaction won't be replicated to the subscriber as
> two_phase was not enabled for the slot. However, subsequent keepalive
> messages can send the latest WAL location to the subscriber and get
> the confirmation of the same from the subscriber without its origin
> being moved. Now, after we restart the apply worker (due to
> disable/enable for a subscription), it will use the previous
> origin_lsn to temporarily move back the confirmed flush LSN as
> explained in one of the previous emails in another thread [1]. During
> this temporary movement of confirm flush LSN, the slotsync worker
> fetches the two_phase_at and confirm_flush_lsn values, leading to the
> assertion failure. We see this issue intermittently because it depends
> on the timing of slotsync worker's request to fetch the slot's value.
>
> If this theory is correct, then we need something on the lines of what
> Vignesh proposed in email [2] (Confirm_flush_dont_allow_backward) to
> fix it.

This theory seems plausible. According to the thread you shared, we
didn't address the issue of backward movement in the slot's
confirmed_flush LSN. I think that we should carefully consider whether
this behavior is acceptable in the first place. If we decide to
maintain this behavior, we would need to modify the slotsync to deal
with such scenarios (note we have already ensured that the synced
slot's confirmed_flush cannot regress). However, I'm concerned about a
case like where the server crashes immediately after persisting the
retreated confirmed_flush LSN (along with restart_lsn or xmin updates)
to disk. In such a case, wouldn't the walsender potentially send
duplicate data? If so, we would need a patch like Vignesh proposed.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

Re: Fix slot synchronization with two_phase decoding enabled

From

Amit Kapila

Date:

06 May, 07:04:44

On Tue, May 6, 2025 at 12:26 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Mon, May 5, 2025 at 3:59 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Sun, May 4, 2025 at 2:33 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> > >
> > > While I cannot be entirely certain of my analysis, I believe the root
> > > cause might be related to the backward movement of the confirmed_flush
> > > LSN. The following scenario seems possible:
> > >
> > > 1. The walsender enables the two_phase and sets two_phase_at (which
> > > should be the same as confirmed_flush).
> > > 2. The slot's confirmed_flush regresses for some reason.
> > > 3. The slotsync worker retrieves the remote slot information and
> > > enables two_phase for the local slot.
> > >
> >
> > Yes, this is possible. Here is my theory as to how it can happen in
> > the current case. In the failed test, after the primary has prepared a
> > transaction, the transaction won't be replicated to the subscriber as
> > two_phase was not enabled for the slot. However, subsequent keepalive
> > messages can send the latest WAL location to the subscriber and get
> > the confirmation of the same from the subscriber without its origin
> > being moved. Now, after we restart the apply worker (due to
> > disable/enable for a subscription), it will use the previous
> > origin_lsn to temporarily move back the confirmed flush LSN as
> > explained in one of the previous emails in another thread [1]. During
> > this temporary movement of confirm flush LSN, the slotsync worker
> > fetches the two_phase_at and confirm_flush_lsn values, leading to the
> > assertion failure. We see this issue intermittently because it depends
> > on the timing of slotsync worker's request to fetch the slot's value.
> >
> > If this theory is correct, then we need something on the lines of what
> > Vignesh proposed in email [2] (Confirm_flush_dont_allow_backward) to
> > fix it.
>
> This theory seems plausible. According to the thread you shared, we
> didn't address the issue of backward movement in the slot's
> confirmed_flush LSN. I think that we should carefully consider whether
> this behavior is acceptable in the first place. If we decide to
> maintain this behavior, we would need to modify the slotsync to deal
> with such scenarios (note we have already ensured that the synced
> slot's confirmed_flush cannot regress). However, I'm concerned about a
> case like where the server crashes immediately after persisting the
> retreated confirmed_flush LSN (along with restart_lsn or xmin updates)
> to disk. In such a case, wouldn't the walsender potentially send
> duplicate data?
>

Yes that could happen but last time when we tried we couldn't
reproduce such a case. However, we will try some more to see if we can
reproduce a problem even without slotsync. Having said that, we will
also try to reproduce the failure with slotsync in the test where BF
is showing failure to confirm the above theory.

>
 If so, we would need a patch like Vignesh proposed.
>

Right, I feel that is the right thing to do, even if we could prove
that it can lead to failure in slotsync.

--
With Regards,
Amit Kapila.

Re: Fix slot synchronization with two_phase decoding enabled

From

Masahiko Sawada

Date:

06 May, 08:21:41

On Mon, May 5, 2025 at 2:59 AM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> On Fri, May 2, 2025 at 3:05 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Fri, May 2, 2025 at 12:57 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> > >
> > >
> > > Please find the v11 patch addressing the above points and all other
> > > comments. I have also optimized the test by reducing the number of
> > > subscriptions and slots.
> > >
> >
> > Thanks for the patch. Few comments:
> >
> > 1)
> >
> > pg_upgrade/t/003_logical_slots.pl:
> >
> > - "SELECT slot_name, two_phase, failover FROM pg_replication_slots");
> > -is($result, qq(regress_sub|t|t), 'check the slot exists on new cluster');
> > + "SELECT slot_name, two_phase FROM pg_replication_slots");
> > +is($result, qq(regress_sub|t), 'check the slot exists on new cluster');
> >
> > With this change, the failover property test during upgrade is
> > removed. Shall we add it again either using ALTER SUB to enable
> > failover and two_phase together or a new subscription with failover
> > alone?
> >
>
> If ALTER SUB is used to set failover for the existing subscription,
> then pg_upgrade will fail while trying to create a slot with both
> failover and two_phase on the upgraded node $newpub.
> So, I've implemented the other suggestion by adding a separate
> pub-subs to verify the failover property.
>
> > 2)
> > +          The default is false. This option cannot be set together with
> > +          <literal>failover</literal> when creating a logical replication slot.
> > +          However, once the slot is created with <literal>two_phase =
> > true</literal>,
> > +          <literal>failover</literal> can be set to true after the slot has
> > +          consumed all transactions up to the point where two-phase decoding
> > +          was enabled.
> >
> >
> > Suggestion:  all transactions --> all the changes.
> >
>
> Done.
> ~~~
>
> Please find the v12 patch with above suggested changes.

Thank you for updating the patch! I'll review it and share comments.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

RE: Fix slot synchronization with two_phase decoding enabled

From

"Zhijie Hou (Fujitsu)"

Date:

06 May, 14:22:27

On Mon, May 5, 2025 at 6:59 PM Amit Kapila wrote:
> 
> On Sun, May 4, 2025 at 2:33 PM Masahiko Sawada
> <sawada.mshk@gmail.com> wrote:
> >
> > While I cannot be entirely certain of my analysis, I believe the root
> > cause might be related to the backward movement of the confirmed_flush
> > LSN. The following scenario seems possible:
> >
> > 1. The walsender enables the two_phase and sets two_phase_at (which
> > should be the same as confirmed_flush).
> > 2. The slot's confirmed_flush regresses for some reason.
> > 3. The slotsync worker retrieves the remote slot information and
> > enables two_phase for the local slot.
> >
> 
> Yes, this is possible. Here is my theory as to how it can happen in the current
> case. In the failed test, after the primary has prepared a transaction, the
> transaction won't be replicated to the subscriber as two_phase was not
> enabled for the slot. However, subsequent keepalive messages can send the
> latest WAL location to the subscriber and get the confirmation of the same from
> the subscriber without its origin being moved. Now, after we restart the apply
> worker (due to disable/enable for a subscription), it will use the previous
> origin_lsn to temporarily move back the confirmed flush LSN as explained in
> one of the previous emails in another thread [1]. During this temporary
> movement of confirm flush LSN, the slotsync worker fetches the two_phase_at
> and confirm_flush_lsn values, leading to the assertion failure. We see this
> issue intermittently because it depends on the timing of slotsync worker's
> request to fetch the slot's value.

Based on this theory, I can reproduce the BF failure in the 040 tap-test on
HEAD after applying the 0001 patch. This is achieved by using the injection
point to stop the walsender from sending a keepalive before receiving the old
origin position from the apply worker, ensuring the confirmed_flush
consistently moves backward before slotsync.

Additionally, I've reproduced the duplicate data issue on HEAD without slotsync
using the attached script (after applying the injection point patch). This
issue arises if we immediately disable the subscription after the
confirm_flush_lsn moves backward, preventing the walsender from advancing the
confirm_flush_lsn.

In this case, if a prepared transaction exists before two_phase_at, then after
re-enabling the subscription, it will replicate that prepared transaction when
decoding the PREPARE record and replicate that again when decoding the COMMIT
PREPARED record. In such cases, the apply worker keeps reporting the error:

ERROR: transaction identifier "pg_gid_16387_755" is already in use.

Apart from above, we're investigating whether the same issue can occur in
back-branches and will share the results once ready.

Best Regards,
Hou zj

Attachment

Re: Fix slot synchronization with two_phase decoding enabled

From

shveta malik

Date:

07 May, 08:01:55

On Mon, May 5, 2025 at 3:29 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
> Please find the v12 patch with above suggested changes.
>

Thanks for the patch. Few comments for doc changes:

1)
func.sgml - pg_create_logical_replication_slot:

+        failover. The parameters <parameter>twophase</parameter> and
+        <parameter>failover</parameter> cannot be enabled together when
+        creating a replication slot. However, a slot created with
<parameter>twophase</parameter>
+        enabled can later have <parameter>failover</parameter> set to true
+        either using <command>ALTER_REPLICATION_SLOT</command> or via
+        <command>ALTER SUBSCRIPTION</command> if a subscription is using this
+        slot.

Will it be better if we make a NOTE for this? Also can we create links for
<command>ALTER SUBSCRIPTION</command> and
<command>ALTER_REPLICATION_SLOT</command>?

2)
protocol.sgml:

+          The default is false. This option cannot be set together with
+          <literal>failover</literal> when creating a logical replication slot.
+          However, once the slot is created with <literal>two_phase =
true</literal>,
+          <literal>failover</literal> can be set to true after the slot has
+          consumed all the changes up to the point where two-phase decoding
+          was enabled.

Can we make a new paragraph for the new restriction added. Same for
other new restriction in this page.

3)
create_subscription.sgml:

+          cannot be enabled together when creating a subscription. However, a
+          <literal>two_phase</literal> enabled subscription can later have
+          <literal>failover</literal> set to true via <command>ALTER
SUBSCRIPTION</command>,

via-->using

thanks
Shveta

Re: Fix slot synchronization with two_phase decoding enabled

From

Nisha Moond

Date:

07 May, 14:06:41

On Wed, May 7, 2025 at 10:32 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Mon, May 5, 2025 at 3:29 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
> >
> > Please find the v12 patch with above suggested changes.
> >
>
> Thanks for the patch. Few comments for doc changes:
>
> 1)
> func.sgml - pg_create_logical_replication_slot:
>
> +        failover. The parameters <parameter>twophase</parameter> and
> +        <parameter>failover</parameter> cannot be enabled together when
> +        creating a replication slot. However, a slot created with
> <parameter>twophase</parameter>
> +        enabled can later have <parameter>failover</parameter> set to true
> +        either using <command>ALTER_REPLICATION_SLOT</command> or via
> +        <command>ALTER SUBSCRIPTION</command> if a subscription is using this
> +        slot.
>
> Will it be better if we make a NOTE for this? Also can we create links for
> <command>ALTER SUBSCRIPTION</command> and
> <command>ALTER_REPLICATION_SLOT</command>?
>
> 2)
> protocol.sgml:
>
> +          The default is false. This option cannot be set together with
> +          <literal>failover</literal> when creating a logical replication slot.
> +          However, once the slot is created with <literal>two_phase =
> true</literal>,
> +          <literal>failover</literal> can be set to true after the slot has
> +          consumed all the changes up to the point where two-phase decoding
> +          was enabled.
>
> Can we make a new paragraph for the new restriction added. Same for
> other new restriction in this page.
>
> 3)
> create_subscription.sgml:
>
> +          cannot be enabled together when creating a subscription. However, a
> +          <literal>two_phase</literal> enabled subscription can later have
> +          <literal>failover</literal> set to true via <command>ALTER
> SUBSCRIPTION</command>,
>
> via-->using
>

Attached is the v13 patch with the above comments addressed.

--
Thanks,
Nisha

Attachment

v13-0001-PG17-Approach-3-Fix-slot-synchronization-for-two.patch

Re: Fix slot synchronization with two_phase decoding enabled

From

shveta malik

Date:

08 May, 09:05:20

On Wed, May 7, 2025 at 4:36 PM Nisha Moond <nisha.moond412@gmail.com> wrote:
>
>
> Attached is the v13 patch with the above comments addressed.
>
> --

Thanks for the patch.

I think we can have the restriction mentioned under the 'two_phase'
section as well along with the 'failover' section in the CREATE
SUBSCRIPTION doc, similar to what we have in CREATE_REPLICATION_SLOT.

Apart from this, I have no more comments.

thanks
Shveta