pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feed[bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
28+ messages / 7 participants
[nested] [flat]
* [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2024-08-08 05:07 Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
0 siblings, 1 reply; 28+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2024-08-08 05:07 UTC (permalink / raw)
To: 'pgsql-hackers@lists.postgresql.org' <pgsql-hackers@lists.postgresql.org>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; shveta.malik@gmail.com <shveta.malik@gmail.com>
Dear hackers,
This thread forks from [1]. Here can be used to discuss second item.
Below part contains the same statements written in [1], but I did copy-and-paste
just in case. Attached patch is almost the same but bit modified based on the comment
from Amit [2] - an unrelated change is removed.
Found issue
=====
When the subscriber enables two-phase commit but doesn't set max_prepared_transaction >0
and a transaction is prepared on the publisher, the apply worker reports an ERROR
on the subscriber. After that, the prepared transaction is not replayed, which
means it's lost forever. Attached script can emulate the situation.
--
ERROR: prepared transactions are disabled
HINT: Set "max_prepared_transactions" to a nonzero value.
--
The reason is that we advanced the origin progress when aborting the
transaction as well (RecordTransactionAbort->replorigin_session_advance). So,
after setting replorigin_session_origin_lsn, if any ERROR happens when preparing
the transaction, the transaction aborts which incorrectly advances the origin lsn.
An easiest fix is to reset session replication origin before calling the
RecordTransactionAbort(). I think this can happen when 1) LogicalRepApplyLoop()
raises an ERROR or 2) apply worker exits. Attached patch can fix the issue on HEAD.
[1]: https://www.postgresql.org/message-id/TYAPR01MB5692FA4926754B91E9D7B5F0F5AA2%40TYAPR01MB5692.jpnprd0...
[2]: https://www.postgresql.org/message-id/CAA4eK1L-r8OKGdBwC6AeXSibrjr9xKsg8LjGpX_PDR5Go-A9TA%40mail.gma...
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Attachments:
[application/octet-stream] v2-0001-Prevent-origin-progress-advancement-if-failed-to-.patch (4.1K, ../../TYAPR01MB5692FAC23BE40C69DA8ED4AFF5B92@TYAPR01MB5692.jpnprd01.prod.outlook.com/2-v2-0001-Prevent-origin-progress-advancement-if-failed-to-.patch)
download | inline diff:
From eb4e9737040beb08e4a2567643dee2dfe02c4e8b Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Tue, 23 Jul 2024 08:40:29 +0000
Subject: [PATCH v2] Prevent origin progress advancement if failed to apply a
transaction
The origin progress is advanced when aborting a transaction, intented to
signify the successful streaming and application of the ROLLBACK from the
publisher to the subscriber in streaming parallel mode. But when an error
occurred during the commit or prepare after setting
replorigin_session_origin_lsn, the origin progress was advanced as well which
is unexpected. This led to skipped transactions that were not replicated again.
Fix it by resetting replorigin_session_origin_lsn in case of error.
Originally reported by Hou Zhijie
---
src/backend/replication/logical/worker.c | 28 ++++++++++++++++++++++++
src/test/subscription/t/021_twophase.pl | 14 +++++++++++-
2 files changed, 41 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 6dc54c7283..70e4113644 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -4409,6 +4409,14 @@ start_apply(XLogRecPtr origin_startpos)
}
PG_CATCH();
{
+ /*
+ * Reset the origin data to prevent the advancement of origin progress
+ * if the transaction failed to apply.
+ */
+ replorigin_session_origin = InvalidRepOriginId;
+ replorigin_session_origin_lsn = InvalidXLogRecPtr;
+ replorigin_session_origin_timestamp = 0;
+
if (MySubscription->disableonerr)
DisableSubscriptionAndExit();
else
@@ -4619,6 +4627,19 @@ InitializeLogRepWorker(void)
CommitTransactionCommand();
}
+/*
+ * Reset the origin state. This is needed to prevent the advancement of origin
+ * progress if the transaction failed to apply.
+ */
+static void
+replorigin_reset(int code, Datum arg)
+{
+ replorigin_session_origin = InvalidRepOriginId;
+ replorigin_session_origin_lsn = InvalidXLogRecPtr;
+ replorigin_session_origin_timestamp = 0;
+}
+
+
/* Common function to setup the leader apply or tablesync worker. */
void
SetupApplyOrSyncWorker(int worker_slot)
@@ -4647,6 +4668,13 @@ SetupApplyOrSyncWorker(int worker_slot)
InitializeLogRepWorker();
+ /*
+ * Register a callback to reset the origin state before aborting the
+ * transaction in ShutdownPostgres(). This is to prevent the advancement
+ * of origin progress if the transaction failed to apply.
+ */
+ before_shmem_exit(replorigin_reset, (Datum) 0);
+
/* Connect to the origin and start the replication. */
elog(DEBUG1, "connecting to publisher using connection string \"%s\"",
MySubscription->conninfo);
diff --git a/src/test/subscription/t/021_twophase.pl b/src/test/subscription/t/021_twophase.pl
index 5e50f1af33..19147f31e2 100644
--- a/src/test/subscription/t/021_twophase.pl
+++ b/src/test/subscription/t/021_twophase.pl
@@ -23,7 +23,7 @@ $node_publisher->start;
my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
$node_subscriber->init;
$node_subscriber->append_conf('postgresql.conf',
- qq(max_prepared_transactions = 10));
+ qq(max_prepared_transactions = 0));
$node_subscriber->start;
# Create some pre-existing content on publisher
@@ -67,12 +67,24 @@ $node_subscriber->poll_query_until('postgres', $twophase_query)
# then COMMIT PREPARED
###############################
+# Save the log location, to see the failure of the application
+my $log_location = -s $node_subscriber->logfile;
+
$node_publisher->safe_psql(
'postgres', "
BEGIN;
INSERT INTO tab_full VALUES (11);
PREPARE TRANSACTION 'test_prepared_tab_full';");
+# Confirm the ERROR is reported becasue max_prepared_transactions is zero
+$node_subscriber->wait_for_log(
+ qr/ERROR: ( [A-Z0-9]+:)? prepared transactions are disabled/);
+
+# Set max_prepared_transactions to correct value to resume the replication
+$node_subscriber->append_conf('postgresql.conf',
+ qq(max_prepared_transactions = 10));
+$node_subscriber->restart;
+
$node_publisher->wait_for_catchup($appname);
# check that transaction is in prepared state on subscriber
--
2.43.0
[application/octet-stream] test_2pc.sh (1.6K, ../../TYAPR01MB5692FAC23BE40C69DA8ED4AFF5B92@TYAPR01MB5692.jpnprd01.prod.outlook.com/3-test_2pc.sh)
download
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2024-08-08 06:33 Amit Kapila <amit.kapila16@gmail.com>
parent: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
0 siblings, 2 replies; 28+ messages in thread
From: Amit Kapila @ 2024-08-08 06:33 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; +Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; shveta.malik@gmail.com <shveta.malik@gmail.com>
On Thu, Aug 8, 2024 at 10:37 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
...
>
> An easiest fix is to reset session replication origin before calling the
> RecordTransactionAbort(). I think this can happen when 1) LogicalRepApplyLoop()
> raises an ERROR or 2) apply worker exits. Attached patch can fix the issue on HEAD.
>
Few comments:
=============
*
@@ -4409,6 +4409,14 @@ start_apply(XLogRecPtr origin_startpos)
}
PG_CATCH();
{
+ /*
+ * Reset the origin data to prevent the advancement of origin progress
+ * if the transaction failed to apply.
+ */
+ replorigin_session_origin = InvalidRepOriginId;
+ replorigin_session_origin_lsn = InvalidXLogRecPtr;
+ replorigin_session_origin_timestamp = 0;
Can't we call replorigin_reset() instead here?
*
+ /*
+ * Register a callback to reset the origin state before aborting the
+ * transaction in ShutdownPostgres(). This is to prevent the advancement
+ * of origin progress if the transaction failed to apply.
+ */
+ before_shmem_exit(replorigin_reset, (Datum) 0);
I think we need this despite resetting the origin-related variables in
PG_CATCH block to handle FATAL error cases, right? If so, can we use
PG_ENSURE_ERROR_CLEANUP() instead of PG_CATCH()?
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2024-08-08 10:01 shveta malik <shveta.malik@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 1 reply; 28+ messages in thread
From: shveta malik @ 2024-08-08 10:01 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; shveta malik <shveta.malik@gmail.com>
On Thu, Aug 8, 2024 at 12:03 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Aug 8, 2024 at 10:37 AM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> ...
> >
> > An easiest fix is to reset session replication origin before calling the
> > RecordTransactionAbort(). I think this can happen when 1) LogicalRepApplyLoop()
> > raises an ERROR or 2) apply worker exits. Attached patch can fix the issue on HEAD.
> >
>
> Few comments:
> =============
> *
> @@ -4409,6 +4409,14 @@ start_apply(XLogRecPtr origin_startpos)
> }
> PG_CATCH();
> {
> + /*
> + * Reset the origin data to prevent the advancement of origin progress
> + * if the transaction failed to apply.
> + */
> + replorigin_session_origin = InvalidRepOriginId;
> + replorigin_session_origin_lsn = InvalidXLogRecPtr;
> + replorigin_session_origin_timestamp = 0;
>
> Can't we call replorigin_reset() instead here?
>
> *
> + /*
> + * Register a callback to reset the origin state before aborting the
> + * transaction in ShutdownPostgres(). This is to prevent the advancement
> + * of origin progress if the transaction failed to apply.
> + */
> + before_shmem_exit(replorigin_reset, (Datum) 0);
>
> I think we need this despite resetting the origin-related variables in
> PG_CATCH block to handle FATAL error cases, right? If so, can we use
> PG_ENSURE_ERROR_CLEANUP() instead of PG_CATCH()?
+1
Basic tests work fine on this patch. Just thinking out loud,
SetupApplyOrSyncWorker() is called for table-sync worker as well and
IIUC tablesync worker does not deal with 2PC txns. So do we even need
to register replorigin_reset() for tablesync worker as well? If we may
hit such an issue in general, then perhaps we need it in table-sync
worker otherwise not. It needs some investigation. Thoughts?
thanks
Shveta
^ permalink raw reply [nested|flat] 28+ messages in thread
* RE: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2024-08-08 10:11 Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
parent: shveta malik <shveta.malik@gmail.com>
0 siblings, 2 replies; 28+ messages in thread
From: Zhijie Hou (Fujitsu) @ 2024-08-08 10:11 UTC (permalink / raw)
To: shveta malik <shveta.malik@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
On Thursday, August 8, 2024 6:01 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Thu, Aug 8, 2024 at 12:03 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >
> > On Thu, Aug 8, 2024 at 10:37 AM Hayato Kuroda (Fujitsu)
> > <kuroda.hayato@fujitsu.com> wrote:
> > >
> > ...
> > >
> > > An easiest fix is to reset session replication origin before calling
> > > the RecordTransactionAbort(). I think this can happen when 1)
> > > LogicalRepApplyLoop() raises an ERROR or 2) apply worker exits.
> Attached patch can fix the issue on HEAD.
> > >
> >
> > Few comments:
> > =============
> > *
> > @@ -4409,6 +4409,14 @@ start_apply(XLogRecPtr origin_startpos)
> > }
> > PG_CATCH();
> > {
> > + /*
> > + * Reset the origin data to prevent the advancement of origin
> > + progress
> > + * if the transaction failed to apply.
> > + */
> > + replorigin_session_origin = InvalidRepOriginId;
> > + replorigin_session_origin_lsn = InvalidXLogRecPtr;
> > + replorigin_session_origin_timestamp = 0;
> >
> > Can't we call replorigin_reset() instead here?
> >
> > *
> > + /*
> > + * Register a callback to reset the origin state before aborting the
> > + * transaction in ShutdownPostgres(). This is to prevent the
> > + advancement
> > + * of origin progress if the transaction failed to apply.
> > + */
> > + before_shmem_exit(replorigin_reset, (Datum) 0);
> >
> > I think we need this despite resetting the origin-related variables in
> > PG_CATCH block to handle FATAL error cases, right? If so, can we use
> > PG_ENSURE_ERROR_CLEANUP() instead of PG_CATCH()?
>
> +1
>
> Basic tests work fine on this patch. Just thinking out loud,
> SetupApplyOrSyncWorker() is called for table-sync worker as well and IIUC
> tablesync worker does not deal with 2PC txns. So do we even need to register
> replorigin_reset() for tablesync worker as well? If we may hit such an issue in
> general, then perhaps we need it in table-sync worker otherwise not. It
> needs some investigation. Thoughts?
I think this is a general issue that can occur not only due to 2PC. IIUC, this
problem should arise if any ERROR arises after setting the
replorigin_session_origin_lsn but before the CommitTransactionCommand is
completed. If so, I think we should register it for tablesync worker as well.
Best Regards,
Hou zj
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2024-08-08 12:38 Amit Kapila <amit.kapila16@gmail.com>
parent: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
1 sibling, 1 reply; 28+ messages in thread
From: Amit Kapila @ 2024-08-08 12:38 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; +Cc: shveta malik <shveta.malik@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
On Thu, Aug 8, 2024 at 3:41 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> On Thursday, August 8, 2024 6:01 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Thu, Aug 8, 2024 at 12:03 PM Amit Kapila <amit.kapila16@gmail.com>
> > wrote:
> > >
> > > On Thu, Aug 8, 2024 at 10:37 AM Hayato Kuroda (Fujitsu)
> > > <kuroda.hayato@fujitsu.com> wrote:
> > > >
> > > ...
> > > >
> > > > An easiest fix is to reset session replication origin before calling
> > > > the RecordTransactionAbort(). I think this can happen when 1)
> > > > LogicalRepApplyLoop() raises an ERROR or 2) apply worker exits.
> > Attached patch can fix the issue on HEAD.
> > > >
> > >
> > > Few comments:
> > > =============
> > > *
> > > @@ -4409,6 +4409,14 @@ start_apply(XLogRecPtr origin_startpos)
> > > }
> > > PG_CATCH();
> > > {
> > > + /*
> > > + * Reset the origin data to prevent the advancement of origin
> > > + progress
> > > + * if the transaction failed to apply.
> > > + */
> > > + replorigin_session_origin = InvalidRepOriginId;
> > > + replorigin_session_origin_lsn = InvalidXLogRecPtr;
> > > + replorigin_session_origin_timestamp = 0;
> > >
> > > Can't we call replorigin_reset() instead here?
> > >
> > > *
> > > + /*
> > > + * Register a callback to reset the origin state before aborting the
> > > + * transaction in ShutdownPostgres(). This is to prevent the
> > > + advancement
> > > + * of origin progress if the transaction failed to apply.
> > > + */
> > > + before_shmem_exit(replorigin_reset, (Datum) 0);
> > >
> > > I think we need this despite resetting the origin-related variables in
> > > PG_CATCH block to handle FATAL error cases, right? If so, can we use
> > > PG_ENSURE_ERROR_CLEANUP() instead of PG_CATCH()?
> >
> > +1
> >
> > Basic tests work fine on this patch. Just thinking out loud,
> > SetupApplyOrSyncWorker() is called for table-sync worker as well and IIUC
> > tablesync worker does not deal with 2PC txns. So do we even need to register
> > replorigin_reset() for tablesync worker as well? If we may hit such an issue in
> > general, then perhaps we need it in table-sync worker otherwise not. It
> > needs some investigation. Thoughts?
>
> I think this is a general issue that can occur not only due to 2PC. IIUC, this
> problem should arise if any ERROR arises after setting the
> replorigin_session_origin_lsn but before the CommitTransactionCommand is
> completed. If so, I think we should register it for tablesync worker as well.
>
As pointed out earlier, won't using PG_ENSURE_ERROR_CLEANUP() instead
of PG_CATCH() be enough?
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2024-08-09 04:04 shveta malik <shveta.malik@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 28+ messages in thread
From: shveta malik @ 2024-08-09 04:04 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; shveta malik <shveta.malik@gmail.com>
On Thu, Aug 8, 2024 at 6:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Aug 8, 2024 at 3:41 PM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Thursday, August 8, 2024 6:01 PM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > On Thu, Aug 8, 2024 at 12:03 PM Amit Kapila <amit.kapila16@gmail.com>
> > > wrote:
> > > >
> > > > On Thu, Aug 8, 2024 at 10:37 AM Hayato Kuroda (Fujitsu)
> > > > <kuroda.hayato@fujitsu.com> wrote:
> > > > >
> > > > ...
> > > > >
> > > > > An easiest fix is to reset session replication origin before calling
> > > > > the RecordTransactionAbort(). I think this can happen when 1)
> > > > > LogicalRepApplyLoop() raises an ERROR or 2) apply worker exits.
> > > Attached patch can fix the issue on HEAD.
> > > > >
> > > >
> > > > Few comments:
> > > > =============
> > > > *
> > > > @@ -4409,6 +4409,14 @@ start_apply(XLogRecPtr origin_startpos)
> > > > }
> > > > PG_CATCH();
> > > > {
> > > > + /*
> > > > + * Reset the origin data to prevent the advancement of origin
> > > > + progress
> > > > + * if the transaction failed to apply.
> > > > + */
> > > > + replorigin_session_origin = InvalidRepOriginId;
> > > > + replorigin_session_origin_lsn = InvalidXLogRecPtr;
> > > > + replorigin_session_origin_timestamp = 0;
> > > >
> > > > Can't we call replorigin_reset() instead here?
> > > >
> > > > *
> > > > + /*
> > > > + * Register a callback to reset the origin state before aborting the
> > > > + * transaction in ShutdownPostgres(). This is to prevent the
> > > > + advancement
> > > > + * of origin progress if the transaction failed to apply.
> > > > + */
> > > > + before_shmem_exit(replorigin_reset, (Datum) 0);
> > > >
> > > > I think we need this despite resetting the origin-related variables in
> > > > PG_CATCH block to handle FATAL error cases, right? If so, can we use
> > > > PG_ENSURE_ERROR_CLEANUP() instead of PG_CATCH()?
> > >
> > > +1
> > >
> > > Basic tests work fine on this patch. Just thinking out loud,
> > > SetupApplyOrSyncWorker() is called for table-sync worker as well and IIUC
> > > tablesync worker does not deal with 2PC txns. So do we even need to register
> > > replorigin_reset() for tablesync worker as well? If we may hit such an issue in
> > > general, then perhaps we need it in table-sync worker otherwise not. It
> > > needs some investigation. Thoughts?
> >
> > I think this is a general issue that can occur not only due to 2PC. IIUC, this
> > problem should arise if any ERROR arises after setting the
> > replorigin_session_origin_lsn but before the CommitTransactionCommand is
> > completed. If so, I think we should register it for tablesync worker as well.
> >
>
> As pointed out earlier, won't using PG_ENSURE_ERROR_CLEANUP() instead
> of PG_CATCH() be enough?
Yes, I think it should suffice. IIUC, we are going to change
'replorigin_session_origin_lsn' only in start_apply() and not before
that, and thus ensuring its reset during any ERROR or FATAL in
start_apply() is good enough. And I guess we don't want this
origin-reset to be called during regular shutdown, isn't it? But
registering it through before_shmem_exit() will make the
reset-function to be called during normal shutdown as well.
And to answer my previous question (as Hou-San also pointed out), we
do need it in table-sync worker as well. So a change in start_apply
will make sure the fix is valid for both apply and tablesync worker.
thanks
Shveta
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2024-08-09 05:49 Amit Kapila <amit.kapila16@gmail.com>
parent: shveta malik <shveta.malik@gmail.com>
0 siblings, 0 replies; 28+ messages in thread
From: Amit Kapila @ 2024-08-09 05:49 UTC (permalink / raw)
To: shveta malik <shveta.malik@gmail.com>; +Cc: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
On Fri, Aug 9, 2024 at 9:35 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Thu, Aug 8, 2024 at 6:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > >
> > > I think this is a general issue that can occur not only due to 2PC. IIUC, this
> > > problem should arise if any ERROR arises after setting the
> > > replorigin_session_origin_lsn but before the CommitTransactionCommand is
> > > completed. If so, I think we should register it for tablesync worker as well.
> > >
> >
> > As pointed out earlier, won't using PG_ENSURE_ERROR_CLEANUP() instead
> > of PG_CATCH() be enough?
>
> Yes, I think it should suffice. IIUC, we are going to change
> 'replorigin_session_origin_lsn' only in start_apply() and not before
> that, and thus ensuring its reset during any ERROR or FATAL in
> start_apply() is good enough.
>
Right, I also think so.
> And I guess we don't want this
> origin-reset to be called during regular shutdown, isn't it?
>
Agreed. OTOH, there was no harm even if such a reset function is invoked.
> But
> registering it through before_shmem_exit() will make the
> reset-function to be called during normal shutdown as well.
>
True and unless I am missing something we won't need it. I would like
to hear the opinions of Hou-San and Kuroda-San on the same.
> And to answer my previous question (as Hou-San also pointed out), we
> do need it in table-sync worker as well. So a change in start_apply
> will make sure the fix is valid for both apply and tablesync worker.
>
As table-sync workers can also apply transactions after the initial
copy, we need it for table-sync during its apply phase.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 28+ messages in thread
* RE: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2024-08-09 09:09 Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 1 reply; 28+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2024-08-09 09:09 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; shveta.malik@gmail.com <shveta.malik@gmail.com>; Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; +Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Dear Amit, Shveta, Hou,
Thanks for giving many comments! I've updated the patch.
> @@ -4409,6 +4409,14 @@ start_apply(XLogRecPtr origin_startpos)
> }
> PG_CATCH();
> {
> + /*
> + * Reset the origin data to prevent the advancement of origin progress
> + * if the transaction failed to apply.
> + */
> + replorigin_session_origin = InvalidRepOriginId;
> + replorigin_session_origin_lsn = InvalidXLogRecPtr;
> + replorigin_session_origin_timestamp = 0;
>
> Can't we call replorigin_reset() instead here?
I didn't use the function because arguments of calling function looked strange,
but ideally I can. Fixed.
> + /*
> + * Register a callback to reset the origin state before aborting the
> + * transaction in ShutdownPostgres(). This is to prevent the advancement
> + * of origin progress if the transaction failed to apply.
> + */
> + before_shmem_exit(replorigin_reset, (Datum) 0);
>
> I think we need this despite resetting the origin-related variables in
> PG_CATCH block to handle FATAL error cases, right? If so, can we use
> PG_ENSURE_ERROR_CLEANUP() instead of PG_CATCH()?
There are two reasons to add a shmem-exit callback. One is to support a FATAL,
another one is to support the case that user does the shutdown request while
applying changes. In this case, I think ShutdownPostgres() can be called so that
the session origin may advance.
However, I think we cannot use PG_ENSURE_ERROR_CLEANUP()/PG_END_ENSURE_ERROR_CLEANUP
macros here. According to codes, it assumes that any before-shmem callbacks are
not registered within the block because the cleanup function is registered and canceled
within the macro. LogicalRepApplyLoop() can register the function when
it handles COMMIT PREPARED message so it breaks the rule.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Attachments:
[application/octet-stream] v3-0001-Prevent-origin-progress-advancement-if-failed-to-.patch (4.3K, ../../TYAPR01MB5692AE79A51187517DF84A43F5BA2@TYAPR01MB5692.jpnprd01.prod.outlook.com/2-v3-0001-Prevent-origin-progress-advancement-if-failed-to-.patch)
download | inline diff:
From ec22f424ede4cc828d79363502be38c04d43ac58 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Tue, 23 Jul 2024 08:40:29 +0000
Subject: [PATCH v3] Prevent origin progress advancement if failed to apply a
transaction
The origin progress is advanced when aborting a transaction, intented to
signify the successful streaming and application of the ROLLBACK from the
publisher to the subscriber in streaming parallel mode. But when an error
occurred during the commit or prepare after setting
replorigin_session_origin_lsn, the origin progress was advanced as well which
is unexpected. This led to skipped transactions that were not replicated again.
Fix it by resetting replorigin_session_origin_lsn in case of error.
Originally reported by Hou Zhijie
---
src/backend/replication/logical/worker.c | 27 ++++++++++++++++++++++++
src/test/subscription/t/021_twophase.pl | 14 +++++++++++-
2 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 245e9be6f2..c2849c8bfa 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -413,6 +413,8 @@ static inline void reset_apply_error_context_info(void);
static TransApplyAction get_transaction_apply_action(TransactionId xid,
ParallelApplyWorkerInfo **winfo);
+static void replorigin_reset(int code, Datum arg);
+
/*
* Form the origin name for the subscription.
*
@@ -4432,6 +4434,12 @@ start_apply(XLogRecPtr origin_startpos)
}
PG_CATCH();
{
+ /*
+ * Reset the origin data to prevent the advancement of origin progress
+ * if the transaction failed to apply.
+ */
+ replorigin_reset(0, (Datum) 0);
+
if (MySubscription->disableonerr)
DisableSubscriptionAndExit();
else
@@ -4642,6 +4650,18 @@ InitializeLogRepWorker(void)
CommitTransactionCommand();
}
+/*
+ * Reset the origin state. This is needed to prevent the advancement of origin
+ * progress if the transaction failed to apply.
+ */
+static void
+replorigin_reset(int code, Datum arg)
+{
+ replorigin_session_origin = InvalidRepOriginId;
+ replorigin_session_origin_lsn = InvalidXLogRecPtr;
+ replorigin_session_origin_timestamp = 0;
+}
+
/* Common function to setup the leader apply or tablesync worker. */
void
SetupApplyOrSyncWorker(int worker_slot)
@@ -4670,6 +4690,13 @@ SetupApplyOrSyncWorker(int worker_slot)
InitializeLogRepWorker();
+ /*
+ * Register a callback to reset the origin state before aborting the
+ * transaction in ShutdownPostgres(). This is to prevent the advancement
+ * of origin progress if the transaction failed to apply.
+ */
+ before_shmem_exit(replorigin_reset, (Datum) 0);
+
/* Connect to the origin and start the replication. */
elog(DEBUG1, "connecting to publisher using connection string \"%s\"",
MySubscription->conninfo);
diff --git a/src/test/subscription/t/021_twophase.pl b/src/test/subscription/t/021_twophase.pl
index 5e50f1af33..19147f31e2 100644
--- a/src/test/subscription/t/021_twophase.pl
+++ b/src/test/subscription/t/021_twophase.pl
@@ -23,7 +23,7 @@ $node_publisher->start;
my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
$node_subscriber->init;
$node_subscriber->append_conf('postgresql.conf',
- qq(max_prepared_transactions = 10));
+ qq(max_prepared_transactions = 0));
$node_subscriber->start;
# Create some pre-existing content on publisher
@@ -67,12 +67,24 @@ $node_subscriber->poll_query_until('postgres', $twophase_query)
# then COMMIT PREPARED
###############################
+# Save the log location, to see the failure of the application
+my $log_location = -s $node_subscriber->logfile;
+
$node_publisher->safe_psql(
'postgres', "
BEGIN;
INSERT INTO tab_full VALUES (11);
PREPARE TRANSACTION 'test_prepared_tab_full';");
+# Confirm the ERROR is reported becasue max_prepared_transactions is zero
+$node_subscriber->wait_for_log(
+ qr/ERROR: ( [A-Z0-9]+:)? prepared transactions are disabled/);
+
+# Set max_prepared_transactions to correct value to resume the replication
+$node_subscriber->append_conf('postgresql.conf',
+ qq(max_prepared_transactions = 10));
+$node_subscriber->restart;
+
$node_publisher->wait_for_catchup($appname);
# check that transaction is in prepared state on subscriber
--
2.43.0
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2024-08-12 10:06 shveta malik <shveta.malik@gmail.com>
parent: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
0 siblings, 2 replies; 28+ messages in thread
From: shveta malik @ 2024-08-12 10:06 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; shveta malik <shveta.malik@gmail.com>
On Fri, Aug 9, 2024 at 2:39 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Amit, Shveta, Hou,
>
> Thanks for giving many comments! I've updated the patch.
>
> > @@ -4409,6 +4409,14 @@ start_apply(XLogRecPtr origin_startpos)
> > }
> > PG_CATCH();
> > {
> > + /*
> > + * Reset the origin data to prevent the advancement of origin progress
> > + * if the transaction failed to apply.
> > + */
> > + replorigin_session_origin = InvalidRepOriginId;
> > + replorigin_session_origin_lsn = InvalidXLogRecPtr;
> > + replorigin_session_origin_timestamp = 0;
> >
> > Can't we call replorigin_reset() instead here?
>
> I didn't use the function because arguments of calling function looked strange,
> but ideally I can. Fixed.
>
> > + /*
> > + * Register a callback to reset the origin state before aborting the
> > + * transaction in ShutdownPostgres(). This is to prevent the advancement
> > + * of origin progress if the transaction failed to apply.
> > + */
> > + before_shmem_exit(replorigin_reset, (Datum) 0);
> >
> > I think we need this despite resetting the origin-related variables in
> > PG_CATCH block to handle FATAL error cases, right? If so, can we use
> > PG_ENSURE_ERROR_CLEANUP() instead of PG_CATCH()?
>
> There are two reasons to add a shmem-exit callback. One is to support a FATAL,
> another one is to support the case that user does the shutdown request while
> applying changes. In this case, I think ShutdownPostgres() can be called so that
> the session origin may advance.
Agree that we need the 'reset' during shutdown flow as well. Details at [1]
> However, I think we cannot use PG_ENSURE_ERROR_CLEANUP()/PG_END_ENSURE_ERROR_CLEANUP
> macros here. According to codes, it assumes that any before-shmem callbacks are
> not registered within the block because the cleanup function is registered and canceled
> within the macro. LogicalRepApplyLoop() can register the function when
> it handles COMMIT PREPARED message so it breaks the rule.
Yes, on reanalyzing, we can not use PG_ENSURE_ERROR_CLEANUP in this
flow due to the limitation of cancel_before_shmem_exit() that it can
cancel only the last registered callback, while in our flow we have
other callbacks also registered after we register our reset one.
[1]
Shutdown analysis:
I did a test where we make apply worker wait for say 0sec right after
it updates 'replorigin_session_origin_lsn' in
apply_handle_prepare_internal() (say it at code-point1). During this
wait, we triggered a subscriber shutdown.Under normal circumstances,
everything works fine: after the wait, the apply worker processes the
SIGTERM (via LogicalRepApplyLoop-->ProcessInterrupts()) only after the
prepare phase is complete, meaning the PREPARE LSN is flushed, and the
origin LSN is correctly advanced in EndPrepare() before the worker
shuts down. But, if we insert a LOG statement between code-point1 and
EndPrepare(), the apply worker processes the SIGTERM during the LOG
operation, as errfinish() triggers CHECK_FOR_INTERRUPTS at the end,
which causes the origin LSN to be incorrectly advanced during
shutdown. And thus the subsequent COMMIT PREPARED on the publisher
results in ERROR on subscriber; as the 'PREPARE' is lost on the
subscriber and is not resent by the publisher. ERROR: prepared
transaction with identifier "pg_gid_16403_757" does not exist
A similar problem can also occur without introducing any additional
LOG statements, but by simply setting log_min_messages=debug5. This
causes the apply worker to output a few DEBUG messages upon receiving
a shutdown signal (after code-point1) before it reaches EndPrepare().
As a result, it ends up processing the SIGTERM (during logging)and
invoking AbortOutOfAnyTransaction(), which incorrectly advances the
origin LSN.
thanks
Shveta
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2024-08-12 10:10 shveta malik <shveta.malik@gmail.com>
parent: shveta malik <shveta.malik@gmail.com>
1 sibling, 0 replies; 28+ messages in thread
From: shveta malik @ 2024-08-12 10:10 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; shveta malik <shveta.malik@gmail.com>
On Mon, Aug 12, 2024 at 3:36 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Fri, Aug 9, 2024 at 2:39 PM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > Dear Amit, Shveta, Hou,
> >
> > Thanks for giving many comments! I've updated the patch.
> >
> > > @@ -4409,6 +4409,14 @@ start_apply(XLogRecPtr origin_startpos)
> > > }
> > > PG_CATCH();
> > > {
> > > + /*
> > > + * Reset the origin data to prevent the advancement of origin progress
> > > + * if the transaction failed to apply.
> > > + */
> > > + replorigin_session_origin = InvalidRepOriginId;
> > > + replorigin_session_origin_lsn = InvalidXLogRecPtr;
> > > + replorigin_session_origin_timestamp = 0;
> > >
> > > Can't we call replorigin_reset() instead here?
> >
> > I didn't use the function because arguments of calling function looked strange,
> > but ideally I can. Fixed.
> >
> > > + /*
> > > + * Register a callback to reset the origin state before aborting the
> > > + * transaction in ShutdownPostgres(). This is to prevent the advancement
> > > + * of origin progress if the transaction failed to apply.
> > > + */
> > > + before_shmem_exit(replorigin_reset, (Datum) 0);
> > >
> > > I think we need this despite resetting the origin-related variables in
> > > PG_CATCH block to handle FATAL error cases, right? If so, can we use
> > > PG_ENSURE_ERROR_CLEANUP() instead of PG_CATCH()?
> >
> > There are two reasons to add a shmem-exit callback. One is to support a FATAL,
> > another one is to support the case that user does the shutdown request while
> > applying changes. In this case, I think ShutdownPostgres() can be called so that
> > the session origin may advance.
>
> Agree that we need the 'reset' during shutdown flow as well. Details at [1]
>
> > However, I think we cannot use PG_ENSURE_ERROR_CLEANUP()/PG_END_ENSURE_ERROR_CLEANUP
> > macros here. According to codes, it assumes that any before-shmem callbacks are
> > not registered within the block because the cleanup function is registered and canceled
> > within the macro. LogicalRepApplyLoop() can register the function when
> > it handles COMMIT PREPARED message so it breaks the rule.
>
> Yes, on reanalyzing, we can not use PG_ENSURE_ERROR_CLEANUP in this
> flow due to the limitation of cancel_before_shmem_exit() that it can
> cancel only the last registered callback, while in our flow we have
> other callbacks also registered after we register our reset one.
>
> [1]
> Shutdown analysis:
>
> I did a test where we make apply worker wait for say 0sec right after
Correction here: 0sec -->10sec
> it updates 'replorigin_session_origin_lsn' in
> apply_handle_prepare_internal() (say it at code-point1). During this
> wait, we triggered a subscriber shutdown.Under normal circumstances,
> everything works fine: after the wait, the apply worker processes the
> SIGTERM (via LogicalRepApplyLoop-->ProcessInterrupts()) only after the
> prepare phase is complete, meaning the PREPARE LSN is flushed, and the
> origin LSN is correctly advanced in EndPrepare() before the worker
> shuts down. But, if we insert a LOG statement between code-point1 and
> EndPrepare(), the apply worker processes the SIGTERM during the LOG
> operation, as errfinish() triggers CHECK_FOR_INTERRUPTS at the end,
> which causes the origin LSN to be incorrectly advanced during
> shutdown. And thus the subsequent COMMIT PREPARED on the publisher
> results in ERROR on subscriber; as the 'PREPARE' is lost on the
> subscriber and is not resent by the publisher. ERROR: prepared
> transaction with identifier "pg_gid_16403_757" does not exist
>
> A similar problem can also occur without introducing any additional
> LOG statements, but by simply setting log_min_messages=debug5. This
> causes the apply worker to output a few DEBUG messages upon receiving
> a shutdown signal (after code-point1) before it reaches EndPrepare().
> As a result, it ends up processing the SIGTERM (during logging)and
> invoking AbortOutOfAnyTransaction(), which incorrectly advances the
> origin LSN.
>
> thanks
> Shveta
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2024-08-13 04:18 Amit Kapila <amit.kapila16@gmail.com>
parent: shveta malik <shveta.malik@gmail.com>
1 sibling, 2 replies; 28+ messages in thread
From: Amit Kapila @ 2024-08-13 04:18 UTC (permalink / raw)
To: shveta malik <shveta.malik@gmail.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
On Mon, Aug 12, 2024 at 3:37 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Fri, Aug 9, 2024 at 2:39 PM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> >
> > > + /*
> > > + * Register a callback to reset the origin state before aborting the
> > > + * transaction in ShutdownPostgres(). This is to prevent the advancement
> > > + * of origin progress if the transaction failed to apply.
> > > + */
> > > + before_shmem_exit(replorigin_reset, (Datum) 0);
> > >
> > > I think we need this despite resetting the origin-related variables in
> > > PG_CATCH block to handle FATAL error cases, right? If so, can we use
> > > PG_ENSURE_ERROR_CLEANUP() instead of PG_CATCH()?
> >
> > There are two reasons to add a shmem-exit callback. One is to support a FATAL,
> > another one is to support the case that user does the shutdown request while
> > applying changes. In this case, I think ShutdownPostgres() can be called so that
> > the session origin may advance.
>
> Agree that we need the 'reset' during shutdown flow as well. Details at [1]
>
Thanks for the detailed analysis. I agree with your analysis that we
need to reset the origin information for the shutdown path to avoid it
being advanced incorrectly. However, the patch doesn't have sufficient
comments to explain why we need to reset it for both the ERROR and
Shutdown paths. Can we improve the comments in the patch?
Also, for the ERROR path, can we reset the origin information in
apply_error_callback()?
BTW, this needs to be backpatched till 16 when it has been introduced
by the parallel apply feature as part of commit 216a7848. So, can we
test this patch in back-branches as well?
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2024-08-14 04:55 shveta malik <shveta.malik@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 1 reply; 28+ messages in thread
From: shveta malik @ 2024-08-14 04:55 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; shveta malik <shveta.malik@gmail.com>
On Tue, Aug 13, 2024 at 9:48 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Aug 12, 2024 at 3:37 PM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > On Fri, Aug 9, 2024 at 2:39 PM Hayato Kuroda (Fujitsu)
> > <kuroda.hayato@fujitsu.com> wrote:
> > >
> > >
> > > > + /*
> > > > + * Register a callback to reset the origin state before aborting the
> > > > + * transaction in ShutdownPostgres(). This is to prevent the advancement
> > > > + * of origin progress if the transaction failed to apply.
> > > > + */
> > > > + before_shmem_exit(replorigin_reset, (Datum) 0);
> > > >
> > > > I think we need this despite resetting the origin-related variables in
> > > > PG_CATCH block to handle FATAL error cases, right? If so, can we use
> > > > PG_ENSURE_ERROR_CLEANUP() instead of PG_CATCH()?
> > >
> > > There are two reasons to add a shmem-exit callback. One is to support a FATAL,
> > > another one is to support the case that user does the shutdown request while
> > > applying changes. In this case, I think ShutdownPostgres() can be called so that
> > > the session origin may advance.
> >
> > Agree that we need the 'reset' during shutdown flow as well. Details at [1]
> >
>
> Thanks for the detailed analysis. I agree with your analysis that we
> need to reset the origin information for the shutdown path to avoid it
> being advanced incorrectly. However, the patch doesn't have sufficient
> comments to explain why we need to reset it for both the ERROR and
> Shutdown paths. Can we improve the comments in the patch?
>
> Also, for the ERROR path, can we reset the origin information in
> apply_error_callback()?
Please find v4 attached. Addressed comments in that.
Manual testing done on v4:
1) Error and Fatal case
2) Shutdown after replorigin_session_origin_lsn was set in
apply_handle_prepare() and before EndPrepare was called.
2a) with log_min_messages=debug5. This will result in processing
of shutdown signal by errfinish() before PREPARE is over.
2b) with default log_min_messages. This will result in processing
of shutdown signal by LogicalRepApplyLoop() after ongoing PREPARE is
over.
>
> BTW, this needs to be backpatched till 16 when it has been introduced
> by the parallel apply feature as part of commit 216a7848. So, can we
> test this patch in back-branches as well?
>
Sure, will do next.
thanks
Shveta
Attachments:
[application/octet-stream] v4-0001-Prevent-origin-progress-advancement-if-failed-to-.patch (6.5K, ../../CAJpy0uAa3M9sC3GWTUA+YPTnYxEcorFt_7g6rXz05jNcAGdjgQ@mail.gmail.com/2-v4-0001-Prevent-origin-progress-advancement-if-failed-to-.patch)
download | inline diff:
From 7ec18bfbee7c841fb237d49ff823045cd755e51c Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Tue, 23 Jul 2024 08:40:29 +0000
Subject: [PATCH v4] Prevent origin progress advancement if failed to apply a
transaction
The origin progress is advanced when aborting a transaction, intented to
signify the successful streaming and application of the ROLLBACK from the
publisher to the subscriber in streaming parallel mode. But when an error
occurred during the commit or prepare after setting
replorigin_session_origin_lsn, the origin progress was advanced as well which
is unexpected. This led to skipped transactions that were not replicated again.
Fix it by resetting replorigin_session_origin_lsn in case of error.
Originally reported by Hou Zhijie
---
src/backend/replication/logical/worker.c | 46 ++++++++++++++++++++++++
src/backend/utils/error/elog.c | 17 +++++++++
src/include/utils/elog.h | 1 +
src/test/subscription/t/021_twophase.pl | 14 +++++++-
4 files changed, 77 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 6dc54c7283..837e63ea45 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -4619,6 +4619,19 @@ InitializeLogRepWorker(void)
CommitTransactionCommand();
}
+/*
+ * Reset the origin state. This is needed to prevent the advancement of origin
+ * progress if the transaction failed to apply or a shutdown was triggered
+ * before worker could commit the transaction.
+ */
+static void
+replorigin_reset(int code, Datum arg)
+{
+ replorigin_session_origin = InvalidRepOriginId;
+ replorigin_session_origin_lsn = InvalidXLogRecPtr;
+ replorigin_session_origin_timestamp = 0;
+}
+
/* Common function to setup the leader apply or tablesync worker. */
void
SetupApplyOrSyncWorker(int worker_slot)
@@ -4647,6 +4660,25 @@ SetupApplyOrSyncWorker(int worker_slot)
InitializeLogRepWorker();
+ /*
+ * Register a callback to reset the origin state before aborting the
+ * transaction in ShutdownPostgres(). This is necessary to handle
+ * situations where a user shuts down a subscriber while the worker is
+ * applying changes. In such cases, the worker may shut down before
+ * committing the transaction. If the worker does not reset the origin
+ * state information before calling AbortOutOfAnyTransaction() during
+ * shutdown, the origin LSN will advance incorrectly, resulting in the
+ * loss of that transaction, as it will not be replayed when the
+ * subscriber is restarted.
+ *
+ * Since errfinish includes CHECK_FOR_INTERRUPTS(), any LOG or DEBUG
+ * statements placed in the code after the origin state is set may process
+ * a shutdown signal before committing the current apply operation, which
+ * could lead to above explained situation.
+ *
+ */
+ before_shmem_exit(replorigin_reset, (Datum) 0);
+
/* Connect to the origin and start the replication. */
elog(DEBUG1, "connecting to publisher using connection string \"%s\"",
MySubscription->conninfo);
@@ -4873,12 +4905,26 @@ void
apply_error_callback(void *arg)
{
ApplyErrorCallbackArg *errarg = &apply_error_callback_arg;
+ int elevel;
if (apply_error_callback_arg.command == 0)
return;
Assert(errarg->origin_name);
+ elevel = geterrlevel();
+
+ /*
+ * Reset the origin state to prevent the advancement of origin progress if
+ * the transaction fails to apply. It is crucial to reset this information
+ * before calling AbortOutOfAnyTransaction(). Otherwise, the abort will
+ * incorrectly advance the origin based on the current origin state for
+ * which the worker could not perform the commit. This will result in
+ * transaction loss, as that transaction will never be replayed.
+ */
+ if (elevel >= ERROR)
+ replorigin_reset(0, (Datum) 0);
+
if (errarg->rel == NULL)
{
if (!TransactionIdIsValid(errarg->remote_xid))
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 943d8588f3..e26b9f3a66 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -1568,6 +1568,23 @@ geterrcode(void)
return edata->sqlerrcode;
}
+/*
+ * geterrlevel --- return the currently set SQLSTATE error level
+ *
+ * This is only intended for use in error callback subroutines, since there
+ * is no other place outside elog.c where the concept is meaningful.
+ */
+int
+geterrlevel(void)
+{
+ ErrorData *edata = &errordata[errordata_stack_depth];
+
+ /* we don't bother incrementing recursion_depth */
+ CHECK_STACK_DEPTH();
+
+ return edata->elevel;
+}
+
/*
* geterrposition --- return the currently set error position (0 if none)
*
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 054dd2bf62..e54eca5b48 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -226,6 +226,7 @@ extern int internalerrquery(const char *query);
extern int err_generic_string(int field, const char *str);
extern int geterrcode(void);
+extern int geterrlevel(void);
extern int geterrposition(void);
extern int getinternalerrposition(void);
diff --git a/src/test/subscription/t/021_twophase.pl b/src/test/subscription/t/021_twophase.pl
index 5e50f1af33..19147f31e2 100644
--- a/src/test/subscription/t/021_twophase.pl
+++ b/src/test/subscription/t/021_twophase.pl
@@ -23,7 +23,7 @@ $node_publisher->start;
my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
$node_subscriber->init;
$node_subscriber->append_conf('postgresql.conf',
- qq(max_prepared_transactions = 10));
+ qq(max_prepared_transactions = 0));
$node_subscriber->start;
# Create some pre-existing content on publisher
@@ -67,12 +67,24 @@ $node_subscriber->poll_query_until('postgres', $twophase_query)
# then COMMIT PREPARED
###############################
+# Save the log location, to see the failure of the application
+my $log_location = -s $node_subscriber->logfile;
+
$node_publisher->safe_psql(
'postgres', "
BEGIN;
INSERT INTO tab_full VALUES (11);
PREPARE TRANSACTION 'test_prepared_tab_full';");
+# Confirm the ERROR is reported becasue max_prepared_transactions is zero
+$node_subscriber->wait_for_log(
+ qr/ERROR: ( [A-Z0-9]+:)? prepared transactions are disabled/);
+
+# Set max_prepared_transactions to correct value to resume the replication
+$node_subscriber->append_conf('postgresql.conf',
+ qq(max_prepared_transactions = 10));
+$node_subscriber->restart;
+
$node_publisher->wait_for_catchup($appname);
# check that transaction is in prepared state on subscriber
--
2.34.1
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2024-08-14 09:23 shveta malik <shveta.malik@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
1 sibling, 0 replies; 28+ messages in thread
From: shveta malik @ 2024-08-14 09:23 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; shveta malik <shveta.malik@gmail.com>
On Tue, Aug 13, 2024 at 9:48 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> BTW, this needs to be backpatched till 16 when it has been introduced
> by the parallel apply feature as part of commit 216a7848. So, can we
> test this patch in back-branches as well?
>
I was able to reproduce the problem on REL_16_STABLE and REL_17_STABLE
through both the flows (shutdown and apply-error). The patch v4 fixes
the issues on both.
thanks
Shveta
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2024-08-20 06:06 Amit Kapila <amit.kapila16@gmail.com>
parent: shveta malik <shveta.malik@gmail.com>
0 siblings, 1 reply; 28+ messages in thread
From: Amit Kapila @ 2024-08-20 06:06 UTC (permalink / raw)
To: shveta malik <shveta.malik@gmail.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
On Wed, Aug 14, 2024 at 10:26 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> >
> > Thanks for the detailed analysis. I agree with your analysis that we
> > need to reset the origin information for the shutdown path to avoid it
> > being advanced incorrectly. However, the patch doesn't have sufficient
> > comments to explain why we need to reset it for both the ERROR and
> > Shutdown paths. Can we improve the comments in the patch?
> >
> > Also, for the ERROR path, can we reset the origin information in
> > apply_error_callback()?
>
> Please find v4 attached. Addressed comments in that.
>
The patch looks mostly good to me. I have slightly changed a few of
the comments in the attached. What do you think of the attached?
--
With Regards,
Amit Kapila.
Attachments:
[application/octet-stream] v5-0001-Don-t-advance-origin-during-apply-failure.patch (5.7K, ../../CAA4eK1K=hFfrB1rW5Mbd5aTGBP_yo1nKb-0jdib842Z9bTz_Dw@mail.gmail.com/2-v5-0001-Don-t-advance-origin-during-apply-failure.patch)
download | inline diff:
From 295bcde464c3dda541da3e36ed440389a380d769 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Tue, 23 Jul 2024 08:40:29 +0000
Subject: [PATCH v5] Don't advance origin during apply failure.
We advance origin progress during abort on successful streaming and
application of ROLLBACK in parallel streaming mode. But the origin
shouldn't be advanced during an error or unsuccessful apply due to
shutdown. Otherwise, it will result in a transaction loss as such a
transaction won't be sent again by the server.
Reported-by: Hou Zhijie
Author: Hayato Kuroda and Shveta Malik
Reviewed-by: Amit Kapila
Backpatch-through: 16
Discussion: https://postgr.es/m/TYAPR01MB5692FAC23BE40C69DA8ED4AFF5B92@TYAPR01MB5692.jpnprd01.prod.outlook.com
---
src/backend/replication/logical/worker.c | 35 ++++++++++++++++++++++++
src/backend/utils/error/elog.c | 17 ++++++++++++
src/include/utils/elog.h | 1 +
src/test/subscription/t/021_twophase.pl | 14 +++++++++-
4 files changed, 66 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index cdea6295d8..bdb8fc6482 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -4713,6 +4713,17 @@ InitializeLogRepWorker(void)
CommitTransactionCommand();
}
+/*
+ * Reset the origin state.
+ */
+static void
+replorigin_reset(int code, Datum arg)
+{
+ replorigin_session_origin = InvalidRepOriginId;
+ replorigin_session_origin_lsn = InvalidXLogRecPtr;
+ replorigin_session_origin_timestamp = 0;
+}
+
/* Common function to setup the leader apply or tablesync worker. */
void
SetupApplyOrSyncWorker(int worker_slot)
@@ -4741,6 +4752,19 @@ SetupApplyOrSyncWorker(int worker_slot)
InitializeLogRepWorker();
+ /*
+ * Register a callback to reset the origin state before aborting any
+ * pending transaction during shutdown (see ShutdownPostgres()). This will
+ * avoid origin advancement for an in-complete transaction which could
+ * otherwise lead its loss as such a transaction won't be sent by the
+ * server again.
+ *
+ * Note that even a LOG or DEBUG statement placed after setting the origin
+ * state may process a shutdown signal before committing the current apply
+ * operation. So, it is important to register such a callback here.
+ */
+ before_shmem_exit(replorigin_reset, (Datum) 0);
+
/* Connect to the origin and start the replication. */
elog(DEBUG1, "connecting to publisher using connection string \"%s\"",
MySubscription->conninfo);
@@ -4967,12 +4991,23 @@ void
apply_error_callback(void *arg)
{
ApplyErrorCallbackArg *errarg = &apply_error_callback_arg;
+ int elevel;
if (apply_error_callback_arg.command == 0)
return;
Assert(errarg->origin_name);
+ elevel = geterrlevel();
+
+ /*
+ * Reset the origin state to prevent the advancement of origin progress if
+ * we fail to apply. Otherwise, this will result in transaction loss as
+ * that transaction won't be sent again by the server.
+ */
+ if (elevel >= ERROR)
+ replorigin_reset(0, (Datum) 0);
+
if (errarg->rel == NULL)
{
if (!TransactionIdIsValid(errarg->remote_xid))
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 943d8588f3..5cbb5b5416 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -1568,6 +1568,23 @@ geterrcode(void)
return edata->sqlerrcode;
}
+/*
+ * geterrlevel --- return the currently set error level
+ *
+ * This is only intended for use in error callback subroutines, since there
+ * is no other place outside elog.c where the concept is meaningful.
+ */
+int
+geterrlevel(void)
+{
+ ErrorData *edata = &errordata[errordata_stack_depth];
+
+ /* we don't bother incrementing recursion_depth */
+ CHECK_STACK_DEPTH();
+
+ return edata->elevel;
+}
+
/*
* geterrposition --- return the currently set error position (0 if none)
*
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 054dd2bf62..e54eca5b48 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -226,6 +226,7 @@ extern int internalerrquery(const char *query);
extern int err_generic_string(int field, const char *str);
extern int geterrcode(void);
+extern int geterrlevel(void);
extern int geterrposition(void);
extern int getinternalerrposition(void);
diff --git a/src/test/subscription/t/021_twophase.pl b/src/test/subscription/t/021_twophase.pl
index 5e50f1af33..19147f31e2 100644
--- a/src/test/subscription/t/021_twophase.pl
+++ b/src/test/subscription/t/021_twophase.pl
@@ -23,7 +23,7 @@ $node_publisher->start;
my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
$node_subscriber->init;
$node_subscriber->append_conf('postgresql.conf',
- qq(max_prepared_transactions = 10));
+ qq(max_prepared_transactions = 0));
$node_subscriber->start;
# Create some pre-existing content on publisher
@@ -67,12 +67,24 @@ $node_subscriber->poll_query_until('postgres', $twophase_query)
# then COMMIT PREPARED
###############################
+# Save the log location, to see the failure of the application
+my $log_location = -s $node_subscriber->logfile;
+
$node_publisher->safe_psql(
'postgres', "
BEGIN;
INSERT INTO tab_full VALUES (11);
PREPARE TRANSACTION 'test_prepared_tab_full';");
+# Confirm the ERROR is reported becasue max_prepared_transactions is zero
+$node_subscriber->wait_for_log(
+ qr/ERROR: ( [A-Z0-9]+:)? prepared transactions are disabled/);
+
+# Set max_prepared_transactions to correct value to resume the replication
+$node_subscriber->append_conf('postgresql.conf',
+ qq(max_prepared_transactions = 10));
+$node_subscriber->restart;
+
$node_publisher->wait_for_catchup($appname);
# check that transaction is in prepared state on subscriber
--
2.28.0.windows.1
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2024-08-20 08:30 shveta malik <shveta.malik@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 28+ messages in thread
From: shveta malik @ 2024-08-20 08:30 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; shveta malik <shveta.malik@gmail.com>
On Tue, Aug 20, 2024 at 11:36 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Aug 14, 2024 at 10:26 AM shveta malik <shveta.malik@gmail.com> wrote:
> >
> > >
> > > Thanks for the detailed analysis. I agree with your analysis that we
> > > need to reset the origin information for the shutdown path to avoid it
> > > being advanced incorrectly. However, the patch doesn't have sufficient
> > > comments to explain why we need to reset it for both the ERROR and
> > > Shutdown paths. Can we improve the comments in the patch?
> > >
> > > Also, for the ERROR path, can we reset the origin information in
> > > apply_error_callback()?
> >
> > Please find v4 attached. Addressed comments in that.
> >
>
> The patch looks mostly good to me. I have slightly changed a few of
> the comments in the attached. What do you think of the attached?
>
Looks good to me. Please find backported patches attached.
thanks
Shveta
Attachments:
[application/octet-stream] v1-0001-Don-t-advance-origin-during-apply-failure_PG17.patch (5.7K, ../../CAJpy0uA+yBtU0kUKpioy9hvUXriEnnz871zP1dirK5ba2x7V4w@mail.gmail.com/2-v1-0001-Don-t-advance-origin-during-apply-failure_PG17.patch)
download | inline diff:
From 089abd1ddd9c9e7ccf2520fd9d9638f99715b6cc Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Tue, 23 Jul 2024 08:40:29 +0000
Subject: [PATCH v1] Don't advance origin during apply failure.
We advance origin progress during abort on successful streaming and
application of ROLLBACK in parallel streaming mode. But the origin
shouldn't be advanced during an error or unsuccessful apply due to
shutdown. Otherwise, it will result in a transaction loss as such a
transaction won't be sent again by the server.
Reported-by: Hou Zhijie
Author: Hayato Kuroda and Shveta Malik
Reviewed-by: Amit Kapila
Backpatch-through: 16
Discussion: https://postgr.es/m/TYAPR01MB5692FAC23BE40C69DA8ED4AFF5B92@TYAPR01MB5692.jpnprd01.prod.outlook.com
---
src/backend/replication/logical/worker.c | 35 ++++++++++++++++++++++++
src/backend/utils/error/elog.c | 17 ++++++++++++
src/include/utils/elog.h | 1 +
src/test/subscription/t/021_twophase.pl | 14 +++++++++-
4 files changed, 66 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index b5a80fe3e8..d091a1dd27 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -4639,6 +4639,17 @@ InitializeLogRepWorker(void)
CommitTransactionCommand();
}
+/*
+ * Reset the origin state.
+ */
+static void
+replorigin_reset(int code, Datum arg)
+{
+ replorigin_session_origin = InvalidRepOriginId;
+ replorigin_session_origin_lsn = InvalidXLogRecPtr;
+ replorigin_session_origin_timestamp = 0;
+}
+
/* Common function to setup the leader apply or tablesync worker. */
void
SetupApplyOrSyncWorker(int worker_slot)
@@ -4667,6 +4678,19 @@ SetupApplyOrSyncWorker(int worker_slot)
InitializeLogRepWorker();
+ /*
+ * Register a callback to reset the origin state before aborting any
+ * pending transaction during shutdown (see ShutdownPostgres()). This will
+ * avoid origin advancement for an in-complete transaction which could
+ * otherwise lead to its loss as such a transaction won't be sent by the
+ * server again.
+ *
+ * Note that even a LOG or DEBUG statement placed after setting the origin
+ * state may process a shutdown signal before committing the current apply
+ * operation. So, it is important to register such a callback here.
+ */
+ before_shmem_exit(replorigin_reset, (Datum) 0);
+
/* Connect to the origin and start the replication. */
elog(DEBUG1, "connecting to publisher using connection string \"%s\"",
MySubscription->conninfo);
@@ -4893,12 +4917,23 @@ void
apply_error_callback(void *arg)
{
ApplyErrorCallbackArg *errarg = &apply_error_callback_arg;
+ int elevel;
if (apply_error_callback_arg.command == 0)
return;
Assert(errarg->origin_name);
+ elevel = geterrlevel();
+
+ /*
+ * Reset the origin state to prevent the advancement of origin progress if
+ * we fail to apply. Otherwise, this will result in transaction loss as
+ * that transaction won't be sent again by the server.
+ */
+ if (elevel >= ERROR)
+ replorigin_reset(0, (Datum) 0);
+
if (errarg->rel == NULL)
{
if (!TransactionIdIsValid(errarg->remote_xid))
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index d1d1632bdd..b924b524d0 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -1568,6 +1568,23 @@ geterrcode(void)
return edata->sqlerrcode;
}
+/*
+ * geterrlevel --- return the currently set error level
+ *
+ * This is only intended for use in error callback subroutines, since there
+ * is no other place outside elog.c where the concept is meaningful.
+ */
+int
+geterrlevel(void)
+{
+ ErrorData *edata = &errordata[errordata_stack_depth];
+
+ /* we don't bother incrementing recursion_depth */
+ CHECK_STACK_DEPTH();
+
+ return edata->elevel;
+}
+
/*
* geterrposition --- return the currently set error position (0 if none)
*
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 054dd2bf62..e54eca5b48 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -226,6 +226,7 @@ extern int internalerrquery(const char *query);
extern int err_generic_string(int field, const char *str);
extern int geterrcode(void);
+extern int geterrlevel(void);
extern int geterrposition(void);
extern int getinternalerrposition(void);
diff --git a/src/test/subscription/t/021_twophase.pl b/src/test/subscription/t/021_twophase.pl
index 9437cd4c3b..e635be74c6 100644
--- a/src/test/subscription/t/021_twophase.pl
+++ b/src/test/subscription/t/021_twophase.pl
@@ -23,7 +23,7 @@ $node_publisher->start;
my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
$node_subscriber->init;
$node_subscriber->append_conf('postgresql.conf',
- qq(max_prepared_transactions = 10));
+ qq(max_prepared_transactions = 0));
$node_subscriber->start;
# Create some pre-existing content on publisher
@@ -67,12 +67,24 @@ $node_subscriber->poll_query_until('postgres', $twophase_query)
# then COMMIT PREPARED
###############################
+# Save the log location, to see the failure of the application
+my $log_location = -s $node_subscriber->logfile;
+
$node_publisher->safe_psql(
'postgres', "
BEGIN;
INSERT INTO tab_full VALUES (11);
PREPARE TRANSACTION 'test_prepared_tab_full';");
+# Confirm the ERROR is reported becasue max_prepared_transactions is zero
+$node_subscriber->wait_for_log(
+ qr/ERROR: ( [A-Z0-9]+:)? prepared transactions are disabled/);
+
+# Set max_prepared_transactions to correct value to resume the replication
+$node_subscriber->append_conf('postgresql.conf',
+ qq(max_prepared_transactions = 10));
+$node_subscriber->restart;
+
$node_publisher->wait_for_catchup($appname);
# check that transaction is in prepared state on subscriber
--
2.34.1
[application/octet-stream] v1-0001-Don-t-advance-origin-during-apply-failure_PG16.patch (5.7K, ../../CAJpy0uA+yBtU0kUKpioy9hvUXriEnnz871zP1dirK5ba2x7V4w@mail.gmail.com/3-v1-0001-Don-t-advance-origin-during-apply-failure_PG16.patch)
download | inline diff:
From 0a76f73a39776315bbea653c0a533edd06edcc13 Mon Sep 17 00:00:00 2001
From: Shveta Malik <shveta.malik@gmail.com>
Date: Wed, 14 Aug 2024 13:02:31 +0530
Subject: [PATCH v1] Don't advance origin during apply failure.
We advance origin progress during abort on successful streaming and
application of ROLLBACK in parallel streaming mode. But the origin
shouldn't be advanced during an error or unsuccessful apply due to
shutdown. Otherwise, it will result in a transaction loss as such a
transaction won't be sent again by the server.
Reported-by: Hou Zhijie
Author: Hayato Kuroda and Shveta Malik
Reviewed-by: Amit Kapila
Backpatch-through: 16
Discussion: https://postgr.es/m/TYAPR01MB5692FAC23BE40C69DA8ED4AFF5B92@TYAPR01MB5692.jpnprd01.prod.outlook.com
---
src/backend/replication/logical/worker.c | 35 ++++++++++++++++++++++++
src/backend/utils/error/elog.c | 17 ++++++++++++
src/include/utils/elog.h | 1 +
src/test/subscription/t/021_twophase.pl | 14 +++++++++-
4 files changed, 66 insertions(+), 1 deletion(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 832b1cf764..18f86c73bd 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -4404,6 +4404,17 @@ start_table_sync(XLogRecPtr *origin_startpos, char **myslotname)
pfree(syncslotname);
}
+/*
+ * Reset the origin state.
+ */
+static void
+replorigin_reset(int code, Datum arg)
+{
+ replorigin_session_origin = InvalidRepOriginId;
+ replorigin_session_origin_lsn = InvalidXLogRecPtr;
+ replorigin_session_origin_timestamp = 0;
+}
+
/*
* Run the apply loop with error handling. Disable the subscription,
* if necessary.
@@ -4553,6 +4564,19 @@ ApplyWorkerMain(Datum main_arg)
InitializeApplyWorker();
+ /*
+ * Register a callback to reset the origin state before aborting any
+ * pending transaction during shutdown (see ShutdownPostgres()). This will
+ * avoid origin advancement for an in-complete transaction which could
+ * otherwise lead to its loss as such a transaction won't be sent by the
+ * server again.
+ *
+ * Note that even a LOG or DEBUG statement placed after setting the origin
+ * state may process a shutdown signal before committing the current apply
+ * operation. So, it is important to register such a callback here.
+ */
+ before_shmem_exit(replorigin_reset, (Datum) 0);
+
InitializingApplyWorker = false;
/* Connect to the origin and start the replication. */
@@ -4916,12 +4940,23 @@ void
apply_error_callback(void *arg)
{
ApplyErrorCallbackArg *errarg = &apply_error_callback_arg;
+ int elevel;
if (apply_error_callback_arg.command == 0)
return;
Assert(errarg->origin_name);
+ elevel = geterrlevel();
+
+ /*
+ * Reset the origin state to prevent the advancement of origin progress if
+ * we fail to apply. Otherwise, this will result in transaction loss as
+ * that transaction won't be sent again by the server.
+ */
+ if (elevel >= ERROR)
+ replorigin_reset(0, (Datum) 0);
+
if (errarg->rel == NULL)
{
if (!TransactionIdIsValid(errarg->remote_xid))
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 7112fb0006..3347f24d4a 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -1571,6 +1571,23 @@ geterrcode(void)
return edata->sqlerrcode;
}
+/*
+ * geterrlevel --- return the currently set SQLSTATE error level
+ *
+ * This is only intended for use in error callback subroutines, since there
+ * is no other place outside elog.c where the concept is meaningful.
+ */
+int
+geterrlevel(void)
+{
+ ErrorData *edata = &errordata[errordata_stack_depth];
+
+ /* we don't bother incrementing recursion_depth */
+ CHECK_STACK_DEPTH();
+
+ return edata->elevel;
+}
+
/*
* geterrposition --- return the currently set error position (0 if none)
*
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 0292e88b4f..8bb55e5b3c 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -226,6 +226,7 @@ extern int internalerrquery(const char *query);
extern int err_generic_string(int field, const char *str);
extern int geterrcode(void);
+extern int geterrlevel(void);
extern int geterrposition(void);
extern int getinternalerrposition(void);
diff --git a/src/test/subscription/t/021_twophase.pl b/src/test/subscription/t/021_twophase.pl
index 8ce4cfc983..822932c1db 100644
--- a/src/test/subscription/t/021_twophase.pl
+++ b/src/test/subscription/t/021_twophase.pl
@@ -23,7 +23,7 @@ $node_publisher->start;
my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
$node_subscriber->init(allows_streaming => 'logical');
$node_subscriber->append_conf('postgresql.conf',
- qq(max_prepared_transactions = 10));
+ qq(max_prepared_transactions = 0));
$node_subscriber->start;
# Create some pre-existing content on publisher
@@ -67,12 +67,24 @@ $node_subscriber->poll_query_until('postgres', $twophase_query)
# then COMMIT PREPARED
###############################
+# Save the log location, to see the failure of the application
+my $log_location = -s $node_subscriber->logfile;
+
$node_publisher->safe_psql(
'postgres', "
BEGIN;
INSERT INTO tab_full VALUES (11);
PREPARE TRANSACTION 'test_prepared_tab_full';");
+# Confirm the ERROR is reported becasue max_prepared_transactions is zero
+$node_subscriber->wait_for_log(
+ qr/ERROR: ( [A-Z0-9]+:)? prepared transactions are disabled/);
+
+# Set max_prepared_transactions to correct value to resume the replication
+$node_subscriber->append_conf('postgresql.conf',
+ qq(max_prepared_transactions = 10));
+$node_subscriber->restart;
+
$node_publisher->wait_for_catchup($appname);
# check that transaction is in prepared state on subscriber
--
2.34.1
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2024-08-21 09:57 Amit Kapila <amit.kapila16@gmail.com>
parent: shveta malik <shveta.malik@gmail.com>
0 siblings, 1 reply; 28+ messages in thread
From: Amit Kapila @ 2024-08-21 09:57 UTC (permalink / raw)
To: shveta malik <shveta.malik@gmail.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
On Tue, Aug 20, 2024 at 2:01 PM shveta malik <shveta.malik@gmail.com> wrote:
>
> On Tue, Aug 20, 2024 at 11:36 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Wed, Aug 14, 2024 at 10:26 AM shveta malik <shveta.malik@gmail.com> wrote:
> > >
> > > >
> > > > Thanks for the detailed analysis. I agree with your analysis that we
> > > > need to reset the origin information for the shutdown path to avoid it
> > > > being advanced incorrectly. However, the patch doesn't have sufficient
> > > > comments to explain why we need to reset it for both the ERROR and
> > > > Shutdown paths. Can we improve the comments in the patch?
> > > >
> > > > Also, for the ERROR path, can we reset the origin information in
> > > > apply_error_callback()?
> > >
> > > Please find v4 attached. Addressed comments in that.
> > >
> >
> > The patch looks mostly good to me. I have slightly changed a few of
> > the comments in the attached. What do you think of the attached?
> >
>
> Looks good to me. Please find backported patches attached.
>
Pushed.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2024-11-07 19:23 Tom Lane <tgl@sss.pgh.pa.us>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 28+ messages in thread
From: Tom Lane @ 2024-11-07 19:23 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: shveta malik <shveta.malik@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Amit Kapila <amit.kapila16@gmail.com> writes:
> On Tue, Aug 20, 2024 at 2:01 PM shveta malik <shveta.malik@gmail.com> wrote:
>> Looks good to me. Please find backported patches attached.
> Pushed.
I came across this commit while preparing release notes, and I'm
worried about whether it doesn't create more problems than it solves.
The intent stated in the thread subject is to prevent an apply worker
from advancing past a prepared transaction if the subscriber side
doesn't permit prepared transactions. However, it appears to me that
the committed patch doesn't permit an apply worker to advance past
any failing transaction whatsoever. Was any thought given to how
a DBA would get out of such a situation and get replication flowing
again? In the prepared-xact case, it's at least clear that you
could increase max_prepared_transactions and restart the subscriber
installation. In the general case, it's not very obvious that you'd
even know what the problem is let alone have an easy way to fix it.
In other words: I thought the original design here was to
intentionally ignore apply errors and keep going, on the theory that
that was better than blocking replication altogether. This commit
has reversed that decision, on the strength of little or no
discussion AFAICS. Are we really ready to push this into minor
releases of stable branches? Is it a good idea even on HEAD?
regards, tom lane
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2024-11-08 03:05 Amit Kapila <amit.kapila16@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 28+ messages in thread
From: Amit Kapila @ 2024-11-08 03:05 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: shveta malik <shveta.malik@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
On Fri, Nov 8, 2024 at 12:53 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I came across this commit while preparing release notes, and I'm
> worried about whether it doesn't create more problems than it solves.
> The intent stated in the thread subject is to prevent an apply worker
> from advancing past a prepared transaction if the subscriber side
> doesn't permit prepared transactions. However, it appears to me that
> the committed patch doesn't permit an apply worker to advance past
> any failing transaction whatsoever. Was any thought given to how
> a DBA would get out of such a situation and get replication flowing
> again? In the prepared-xact case, it's at least clear that you
> could increase max_prepared_transactions and restart the subscriber
> installation. In the general case, it's not very obvious that you'd
> even know what the problem is let alone have an easy way to fix it.
>
This is by design, so we don't let the apply worker proceed in case of
any ERROR. For example, the apply worker keeps retrying to apply the
transaction when there is a unique key violation error while applying
(which could be due to the subscriber side having a unique key defined
but the publisher doesn't or the subscriber already has a row with the
same value). We need manual intervention to continue the replication.
To do that, she can create a subscription with the option
'disable_on_error'. Then apply worker will stop on ERROR instead of
retrying. Then, she can either manually remove a conflicting row or
use ALTER SUBSCRIPTION ... SKIP ... to get past the conflicting/error
transaction. Alternatively, the transaction can also be skipped by
calling the pg_replication_origin_advance() function. To use SKIP or
origin_advance function, in the ERROR log we print the LSN (CONTEXT:
processing remote data for replication origin "pg_16395" during
"INSERT" for replication target relation "public.test" in transaction
725 finished at 0/14C0378). She needs to use LSN value 0/14C0378 to
skip the error transaction. We have explained this in the docs [1].
> In other words: I thought the original design here was to
> intentionally ignore apply errors and keep going, on the theory that
> that was better than blocking replication altogether
>
No, that was not the intention because otherwise, we will silently
create inconsistency on the subscriber side.
. This commit
> has reversed that decision, on the strength of little or no
> discussion AFAICS. Are we really ready to push this into minor
> releases of stable branches? Is it a good idea even on HEAD?
>
I hope the explanation above addresses your concern.
[1] - https://www.postgresql.org/docs/devel/logical-replication-conflicts.html
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 28+ messages in thread
* RE: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2025-12-22 09:01 Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
parent: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
1 sibling, 2 replies; 28+ messages in thread
From: Zhijie Hou (Fujitsu) @ 2025-12-22 09:01 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; +Cc: 'pgsql-hackers@lists.postgresql.org' <pgsql-hackers@lists.postgresql.org>; shveta malik <shveta.malik@gmail.com>
Hi,
When reviewing some parallel apply related codes, I noticed a bug in the
parallel apply worker, similar to the issue discussed in this thread.
The issue is that the logical replication parallel apply worker may erroneously
advance the origin progress during an error or unsuccessful apply. This can lead
to transaction loss, as these transactions will not be resent by the server.
Commit 3f28b2fc addressed a similar issue in both the apply worker and table
sync worker.
The original fix involved registering a before_shmem_exit callback to reset the
origin information, preventing the worker from advancing it during transaction
abortion on shutdown. The attached patch registers the same callback for the
parallel apply worker, ensuring consistent behavior across all workers.
Best Regards,
Hou zj
Attachments:
[application/octet-stream] v1-0001-Fix-unexpected-origin-advancement-during-parallel.patch (4.6K, ../../TY4PR01MB169078771FB31B395AB496A6B94B4A@TY4PR01MB16907.jpnprd01.prod.outlook.com/2-v1-0001-Fix-unexpected-origin-advancement-during-parallel.patch)
download | inline diff:
From 0d626e462fbf25f4973de5b4771835f57ed5e56d Mon Sep 17 00:00:00 2001
From: Zhijie Hou <houzj.fnst@fujitsu.com>
Date: Mon, 22 Dec 2025 14:01:08 +0800
Subject: [PATCH v1] Fix unexpected origin advancement during parallel apply
failure
The logical replication parallel apply worker may erroneously advance the origin
progress during an error or unsuccessful apply. This can lead to transaction
loss, as these transactions will not be resent by the server.
Commit 3f28b2fc addressed a similar issue in both the apply worker and table
sync worker, by registering a before_shmem_exit callback to reset the origin
information, preventing the worker from advancing it during transaction abortion
on shutdown. This commit registers the same callback for the parallel apply
worker, ensuring consistent behavior across all workers.
---
src/backend/replication/logical/worker.c | 30 +++++++++-------
.../subscription/t/023_twophase_stream.pl | 34 +++++++++++++++++++
2 files changed, 51 insertions(+), 13 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index fc64476a9ef..e009c0c9d48 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -5849,6 +5849,23 @@ InitializeLogRepWorker(void)
MySubscription->name));
CommitTransactionCommand();
+
+ /*
+ * Register a callback to reset the origin state before aborting any
+ * pending transaction during shutdown (see ShutdownPostgres()). This will
+ * avoid origin advancement for an in-complete transaction which could
+ * otherwise lead to its loss as such a transaction won't be sent by the
+ * server again.
+ *
+ * Note that even a LOG or DEBUG statement placed after setting the origin
+ * state may process a shutdown signal before committing the current apply
+ * operation. So, it is important to register such a callback here.
+ *
+ * Register this callback here to ensure that all types of logical
+ * replication workers that set up origins and apply remote transactions
+ * are protected.
+ */
+ before_shmem_exit(replorigin_reset, (Datum) 0);
}
/*
@@ -5892,19 +5909,6 @@ SetupApplyOrSyncWorker(int worker_slot)
InitializeLogRepWorker();
- /*
- * Register a callback to reset the origin state before aborting any
- * pending transaction during shutdown (see ShutdownPostgres()). This will
- * avoid origin advancement for an in-complete transaction which could
- * otherwise lead to its loss as such a transaction won't be sent by the
- * server again.
- *
- * Note that even a LOG or DEBUG statement placed after setting the origin
- * state may process a shutdown signal before committing the current apply
- * operation. So, it is important to register such a callback here.
- */
- before_shmem_exit(replorigin_reset, (Datum) 0);
-
/* Connect to the origin and start the replication. */
elog(DEBUG1, "connecting to publisher using connection string \"%s\"",
MySubscription->conninfo);
diff --git a/src/test/subscription/t/023_twophase_stream.pl b/src/test/subscription/t/023_twophase_stream.pl
index e01347ca699..9b9f189308e 100644
--- a/src/test/subscription/t/023_twophase_stream.pl
+++ b/src/test/subscription/t/023_twophase_stream.pl
@@ -429,6 +429,40 @@ $result =
$node_subscriber->safe_psql('postgres', "SELECT count(*) FROM test_tab_2");
is($result, qq(1), 'transaction is committed on subscriber');
+# Test the ability to re-apply a transaction when a parallel apply worker fails
+# to prepare the transaction due to insufficient max_prepared_transactions
+# setting.
+$node_subscriber->append_conf('postgresql.conf',
+ qq(max_prepared_transactions = 0));
+$node_subscriber->restart;
+
+$node_publisher->safe_psql(
+ 'postgres', q{
+ BEGIN;
+ INSERT INTO test_tab_2 values(2);
+ PREPARE TRANSACTION 'xact';
+ COMMIT PREPARED 'xact';
+ });
+
+$offset = -s $node_subscriber->logfile;
+
+# Confirm the ERROR is reported because max_prepared_transactions is zero
+$node_subscriber->wait_for_log(
+ qr/ERROR: ( [A-Z0-9]+:)? prepared transactions are disabled/,
+ $offset);
+
+# Set max_prepared_transactions to correct value to resume the replication
+$node_subscriber->append_conf('postgresql.conf',
+ qq(max_prepared_transactions = 10));
+$node_subscriber->restart;
+
+$node_publisher->wait_for_catchup($appname);
+
+# Check that transaction is committed on subscriber
+$result =
+ $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM test_tab_2");
+is($result, qq(2), 'transaction is committed on subscriber after retrying');
+
###############################
# check all the cleanup
###############################
--
2.51.1.windows.1
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2025-12-22 09:28 Chao Li <li.evan.chao@gmail.com>
parent: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
1 sibling, 1 reply; 28+ messages in thread
From: Chao Li @ 2025-12-22 09:28 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; shveta malik <shveta.malik@gmail.com>
> On Dec 22, 2025, at 17:01, Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote:
>
> Hi,
>
> When reviewing some parallel apply related codes, I noticed a bug in the
> parallel apply worker, similar to the issue discussed in this thread.
>
> The issue is that the logical replication parallel apply worker may erroneously
> advance the origin progress during an error or unsuccessful apply. This can lead
> to transaction loss, as these transactions will not be resent by the server.
> Commit 3f28b2fc addressed a similar issue in both the apply worker and table
> sync worker.
>
> The original fix involved registering a before_shmem_exit callback to reset the
> origin information, preventing the worker from advancing it during transaction
> abortion on shutdown. The attached patch registers the same callback for the
> parallel apply worker, ensuring consistent behavior across all workers.
>
> Best Regards,
> Hou zj
> <v1-0001-Fix-unexpected-origin-advancement-during-parallel.patch>
So, ParallelApplyWorkerMain() only calls InitializeLogRepWorker() but SetupApplyOrSyncWorker(), by moving before_shmem_exit() into InitializeLogRepWorker(), ParallelApplyWorkerMain()() gets the exit callback.
LGTM.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2025-12-22 11:09 Chao Li <li.evan.chao@gmail.com>
parent: Chao Li <li.evan.chao@gmail.com>
0 siblings, 0 replies; 28+ messages in thread
From: Chao Li @ 2025-12-22 11:09 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; shveta malik <shveta.malik@gmail.com>
> On Dec 22, 2025, at 17:28, Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
>> On Dec 22, 2025, at 17:01, Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com> wrote:
>>
>> Hi,
>>
>> When reviewing some parallel apply related codes, I noticed a bug in the
>> parallel apply worker, similar to the issue discussed in this thread.
>>
>> The issue is that the logical replication parallel apply worker may erroneously
>> advance the origin progress during an error or unsuccessful apply. This can lead
>> to transaction loss, as these transactions will not be resent by the server.
>> Commit 3f28b2fc addressed a similar issue in both the apply worker and table
>> sync worker.
>>
>> The original fix involved registering a before_shmem_exit callback to reset the
>> origin information, preventing the worker from advancing it during transaction
>> abortion on shutdown. The attached patch registers the same callback for the
>> parallel apply worker, ensuring consistent behavior across all workers.
>>
>> Best Regards,
>> Hou zj
>> <v1-0001-Fix-unexpected-origin-advancement-during-parallel.patch>
>
> So, ParallelApplyWorkerMain() only calls InitializeLogRepWorker() but SetupApplyOrSyncWorker(), by moving before_shmem_exit() into InitializeLogRepWorker(), ParallelApplyWorkerMain()() gets the exit callback.
>
> LGTM.
>
I just noticed a nitpick while reviewing the other your patch:
```
+ * avoid origin advancement for an in-complete transaction which could
```
“In-complete” should be just “incomplete”.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2025-12-23 04:21 Amit Kapila <amit.kapila16@gmail.com>
parent: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
1 sibling, 1 reply; 28+ messages in thread
From: Amit Kapila @ 2025-12-23 04:21 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; shveta malik <shveta.malik@gmail.com>
On Mon, Dec 22, 2025 at 2:31 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> When reviewing some parallel apply related codes, I noticed a bug in the
> parallel apply worker, similar to the issue discussed in this thread.
>
> The issue is that the logical replication parallel apply worker may erroneously
> advance the origin progress during an error or unsuccessful apply. This can lead
> to transaction loss, as these transactions will not be resent by the server.
> Commit 3f28b2fc addressed a similar issue in both the apply worker and table
> sync worker.
>
> The original fix involved registering a before_shmem_exit callback to reset the
> origin information, preventing the worker from advancing it during transaction
> abortion on shutdown. The attached patch registers the same callback for the
> parallel apply worker, ensuring consistent behavior across all workers.
>
Thanks for reporting the issue and patch.
+# Test the ability to re-apply a transaction when a parallel apply worker fails
+# to prepare the transaction due to insufficient max_prepared_transactions
+# setting.
+$node_subscriber->append_conf('postgresql.conf',
How does the test ensure that error is raised by parallel apply
worker? I see that in the previous test, we set
'debug_logical_replication_streaming = immediate', so that should help
to invoke parallel apply worker. But is there a more direct way to
ensure the same? Can we test for LOG like: "ERROR: logical
replication parallel apply worker exited due to error"?
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 28+ messages in thread
* RE: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2025-12-23 06:42 Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 2 replies; 28+ messages in thread
From: Zhijie Hou (Fujitsu) @ 2025-12-23 06:42 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; shveta malik <shveta.malik@gmail.com>
On Tuesday, December 23, 2025 12:21 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Mon, Dec 22, 2025 at 2:31 PM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
> >
> > When reviewing some parallel apply related codes, I noticed a bug in
> > the parallel apply worker, similar to the issue discussed in this thread.
> >
> > The issue is that the logical replication parallel apply worker may
> > erroneously advance the origin progress during an error or
> > unsuccessful apply. This can lead to transaction loss, as these transactions
> will not be resent by the server.
> > Commit 3f28b2fc addressed a similar issue in both the apply worker and
> > table sync worker.
> >
> > The original fix involved registering a before_shmem_exit callback to
> > reset the origin information, preventing the worker from advancing it
> > during transaction abortion on shutdown. The attached patch registers
> > the same callback for the parallel apply worker, ensuring consistent
> behavior across all workers.
> >
>
> Thanks for reporting the issue and patch.
>
> +# Test the ability to re-apply a transaction when a parallel apply
> +worker fails # to prepare the transaction due to insufficient
> +max_prepared_transactions # setting.
> +$node_subscriber->append_conf('postgresql.conf',
>
> How does the test ensure that error is raised by parallel apply worker? I see
> that in the previous test, we set 'debug_logical_replication_streaming =
> immediate', so that should help to invoke parallel apply worker. But is there a
> more direct way to ensure the same? Can we test for LOG like: "ERROR:
> logical replication parallel apply worker exited due to error"?
OK, I have added a general log test for "ERROR .. logical replication parallel
apply worker ..." to ensure that it's the parallel apply worker that failed to
apply the transaction.
And I also addressed the comments by Li Chao.
Here are the updated patches for all branches.
Best Regards,
Hou zj
Attachments:
[application/octet-stream] v2_PG16-0001-Fix-unexpected-origin-advancement-during-par.patch (5.0K, ../../TY4PR01MB1690740E94C11F6EA9072EDDD94B5A@TY4PR01MB16907.jpnprd01.prod.outlook.com/2-v2_PG16-0001-Fix-unexpected-origin-advancement-during-par.patch)
download | inline diff:
From 3cf0f657f4506f6710fe032c4f43553be262e89d Mon Sep 17 00:00:00 2001
From: Zhijie Hou <houzj.fnst@fujitsu.com>
Date: Tue, 23 Dec 2025 13:38:37 +0800
Subject: [PATCH v2_PG16] Fix unexpected origin advancement during parallel
apply failure
The logical replication parallel apply worker may erroneously advance the origin
progress during an error or unsuccessful apply. This can lead to transaction
loss, as these transactions will not be resent by the server.
Commit 3f28b2fc addressed a similar issue in both the apply worker and table
sync worker, by registering a before_shmem_exit callback to reset the origin
information, preventing the worker from advancing it during transaction abortion
on shutdown. This commit registers the same callback for the parallel apply
worker, ensuring consistent behavior across all workers.
---
src/backend/replication/logical/worker.c | 30 +++++++------
.../subscription/t/023_twophase_stream.pl | 45 +++++++++++++++++++
2 files changed, 62 insertions(+), 13 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 9b5c641941f..d6bbffd7c8d 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -4544,6 +4544,23 @@ InitializeApplyWorker(void)
MySubscription->name)));
CommitTransactionCommand();
+
+ /*
+ * Register a callback to reset the origin state before aborting any
+ * pending transaction during shutdown (see ShutdownPostgres()). This will
+ * avoid origin advancement for an incomplete transaction which could
+ * otherwise lead to its loss as such a transaction won't be sent by the
+ * server again.
+ *
+ * Note that even a LOG or DEBUG statement placed after setting the origin
+ * state may process a shutdown signal before committing the current apply
+ * operation. So, it is important to register such a callback here.
+ *
+ * Register this callback here to ensure that all types of logical
+ * replication workers that set up origins and apply remote transactions
+ * are protected.
+ */
+ before_shmem_exit(replorigin_reset, (Datum) 0);
}
/* Logical Replication Apply worker entry point */
@@ -4581,19 +4598,6 @@ ApplyWorkerMain(Datum main_arg)
InitializeApplyWorker();
- /*
- * Register a callback to reset the origin state before aborting any
- * pending transaction during shutdown (see ShutdownPostgres()). This will
- * avoid origin advancement for an in-complete transaction which could
- * otherwise lead to its loss as such a transaction won't be sent by the
- * server again.
- *
- * Note that even a LOG or DEBUG statement placed after setting the origin
- * state may process a shutdown signal before committing the current apply
- * operation. So, it is important to register such a callback here.
- */
- before_shmem_exit(replorigin_reset, (Datum) 0);
-
InitializingApplyWorker = false;
/* Connect to the origin and start the replication. */
diff --git a/src/test/subscription/t/023_twophase_stream.pl b/src/test/subscription/t/023_twophase_stream.pl
index 0303807846e..39ad688a7bd 100644
--- a/src/test/subscription/t/023_twophase_stream.pl
+++ b/src/test/subscription/t/023_twophase_stream.pl
@@ -429,6 +429,51 @@ $result =
$node_subscriber->safe_psql('postgres', "SELECT count(*) FROM test_tab_2");
is($result, qq(1), 'transaction is committed on subscriber');
+# Test the ability to re-apply a transaction when a parallel apply worker fails
+# to prepare the transaction due to insufficient max_prepared_transactions
+# setting.
+$node_subscriber->append_conf(
+ 'postgresql.conf', qq(
+max_prepared_transactions = 0
+debug_logical_replication_streaming = buffered
+));
+$node_subscriber->restart;
+
+$node_publisher->safe_psql(
+ 'postgres', q{
+ BEGIN;
+ INSERT INTO test_tab_2 values(2);
+ PREPARE TRANSACTION 'xact';
+ COMMIT PREPARED 'xact';
+ });
+
+$offset = -s $node_subscriber->logfile;
+
+# Confirm the ERROR is reported because max_prepared_transactions is zero
+$node_subscriber->wait_for_log(
+ qr/ERROR: ( [A-Z0-9]+:)? prepared transactions are disabled/,
+ $offset);
+
+# Confirm that the parallel apply worker has encountered an error. The check
+# focuses on the worker type as a keyword, since the error message content may
+# differ based on whether the leader initially detected the parallel apply
+# worker's failure or received a signal from it.
+$node_subscriber->wait_for_log(
+ qr/ERROR: .*logical replication parallel apply worker.*/,
+ $offset);
+
+# Set max_prepared_transactions to correct value to resume the replication
+$node_subscriber->append_conf('postgresql.conf',
+ qq(max_prepared_transactions = 10));
+$node_subscriber->restart;
+
+$node_publisher->wait_for_catchup($appname);
+
+# Check that transaction is committed on subscriber
+$result =
+ $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM test_tab_2");
+is($result, qq(2), 'transaction is committed on subscriber after retrying');
+
###############################
# check all the cleanup
###############################
--
2.51.1.windows.1
[application/octet-stream] v2_PG17_PG18-0001-Fix-unexpected-origin-advancement-durin.patch (5.0K, ../../TY4PR01MB1690740E94C11F6EA9072EDDD94B5A@TY4PR01MB16907.jpnprd01.prod.outlook.com/3-v2_PG17_PG18-0001-Fix-unexpected-origin-advancement-durin.patch)
download | inline diff:
From 823f182da91667002da0848e3622b03e056b8120 Mon Sep 17 00:00:00 2001
From: Zhijie Hou <houzj.fnst@fujitsu.com>
Date: Tue, 23 Dec 2025 13:45:16 +0800
Subject: [PATCH v2_PG17_PG18] Fix unexpected origin advancement during
parallel apply failure
The logical replication parallel apply worker may erroneously advance the origin
progress during an error or unsuccessful apply. This can lead to transaction
loss, as these transactions will not be resent by the server.
Commit 3f28b2fc addressed a similar issue in both the apply worker and table
sync worker, by registering a before_shmem_exit callback to reset the origin
information, preventing the worker from advancing it during transaction abortion
on shutdown. This commit registers the same callback for the parallel apply
worker, ensuring consistent behavior across all workers.
---
src/backend/replication/logical/worker.c | 30 +++++++------
.../subscription/t/023_twophase_stream.pl | 45 +++++++++++++++++++
2 files changed, 62 insertions(+), 13 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index f77adfffc83..313d3a82660 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -4673,6 +4673,23 @@ InitializeLogRepWorker(void)
MySubscription->name)));
CommitTransactionCommand();
+
+ /*
+ * Register a callback to reset the origin state before aborting any
+ * pending transaction during shutdown (see ShutdownPostgres()). This will
+ * avoid origin advancement for an incomplete transaction which could
+ * otherwise lead to its loss as such a transaction won't be sent by the
+ * server again.
+ *
+ * Note that even a LOG or DEBUG statement placed after setting the origin
+ * state may process a shutdown signal before committing the current apply
+ * operation. So, it is important to register such a callback here.
+ *
+ * Register this callback here to ensure that all types of logical
+ * replication workers that set up origins and apply remote transactions
+ * are protected.
+ */
+ before_shmem_exit(replorigin_reset, (Datum) 0);
}
/*
@@ -4714,19 +4731,6 @@ SetupApplyOrSyncWorker(int worker_slot)
InitializeLogRepWorker();
- /*
- * Register a callback to reset the origin state before aborting any
- * pending transaction during shutdown (see ShutdownPostgres()). This will
- * avoid origin advancement for an in-complete transaction which could
- * otherwise lead to its loss as such a transaction won't be sent by the
- * server again.
- *
- * Note that even a LOG or DEBUG statement placed after setting the origin
- * state may process a shutdown signal before committing the current apply
- * operation. So, it is important to register such a callback here.
- */
- before_shmem_exit(replorigin_reset, (Datum) 0);
-
/* Connect to the origin and start the replication. */
elog(DEBUG1, "connecting to publisher using connection string \"%s\"",
MySubscription->conninfo);
diff --git a/src/test/subscription/t/023_twophase_stream.pl b/src/test/subscription/t/023_twophase_stream.pl
index 23c762e8adf..5a5516ba787 100644
--- a/src/test/subscription/t/023_twophase_stream.pl
+++ b/src/test/subscription/t/023_twophase_stream.pl
@@ -429,6 +429,51 @@ $result =
$node_subscriber->safe_psql('postgres', "SELECT count(*) FROM test_tab_2");
is($result, qq(1), 'transaction is committed on subscriber');
+# Test the ability to re-apply a transaction when a parallel apply worker fails
+# to prepare the transaction due to insufficient max_prepared_transactions
+# setting.
+$node_subscriber->append_conf(
+ 'postgresql.conf', qq(
+max_prepared_transactions = 0
+debug_logical_replication_streaming = buffered
+));
+$node_subscriber->restart;
+
+$node_publisher->safe_psql(
+ 'postgres', q{
+ BEGIN;
+ INSERT INTO test_tab_2 values(2);
+ PREPARE TRANSACTION 'xact';
+ COMMIT PREPARED 'xact';
+ });
+
+$offset = -s $node_subscriber->logfile;
+
+# Confirm the ERROR is reported because max_prepared_transactions is zero
+$node_subscriber->wait_for_log(
+ qr/ERROR: ( [A-Z0-9]+:)? prepared transactions are disabled/,
+ $offset);
+
+# Confirm that the parallel apply worker has encountered an error. The check
+# focuses on the worker type as a keyword, since the error message content may
+# differ based on whether the leader initially detected the parallel apply
+# worker's failure or received a signal from it.
+$node_subscriber->wait_for_log(
+ qr/ERROR: .*logical replication parallel apply worker.*/,
+ $offset);
+
+# Set max_prepared_transactions to correct value to resume the replication
+$node_subscriber->append_conf('postgresql.conf',
+ qq(max_prepared_transactions = 10));
+$node_subscriber->restart;
+
+$node_publisher->wait_for_catchup($appname);
+
+# Check that transaction is committed on subscriber
+$result =
+ $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM test_tab_2");
+is($result, qq(2), 'transaction is committed on subscriber after retrying');
+
###############################
# check all the cleanup
###############################
--
2.51.1.windows.1
[application/octet-stream] v2_HEAD-0001-Fix-unexpected-origin-advancement-during-par.patch (5.0K, ../../TY4PR01MB1690740E94C11F6EA9072EDDD94B5A@TY4PR01MB16907.jpnprd01.prod.outlook.com/4-v2_HEAD-0001-Fix-unexpected-origin-advancement-during-par.patch)
download | inline diff:
From a6cb317116e4710c7989306848dca2610761d944 Mon Sep 17 00:00:00 2001
From: Zhijie Hou <houzj.fnst@fujitsu.com>
Date: Mon, 22 Dec 2025 14:01:08 +0800
Subject: [PATCH v2_HEAD] Fix unexpected origin advancement during parallel
apply failure
The logical replication parallel apply worker may erroneously advance the origin
progress during an error or unsuccessful apply. This can lead to transaction
loss, as these transactions will not be resent by the server.
Commit 3f28b2fc addressed a similar issue in both the apply worker and table
sync worker, by registering a before_shmem_exit callback to reset the origin
information, preventing the worker from advancing it during transaction abortion
on shutdown. This commit registers the same callback for the parallel apply
worker, ensuring consistent behavior across all workers.
---
src/backend/replication/logical/worker.c | 30 +++++++------
.../subscription/t/023_twophase_stream.pl | 45 +++++++++++++++++++
2 files changed, 62 insertions(+), 13 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index d1ee0261c64..718408bb599 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -5849,6 +5849,23 @@ InitializeLogRepWorker(void)
MySubscription->name));
CommitTransactionCommand();
+
+ /*
+ * Register a callback to reset the origin state before aborting any
+ * pending transaction during shutdown (see ShutdownPostgres()). This will
+ * avoid origin advancement for an incomplete transaction which could
+ * otherwise lead to its loss as such a transaction won't be sent by the
+ * server again.
+ *
+ * Note that even a LOG or DEBUG statement placed after setting the origin
+ * state may process a shutdown signal before committing the current apply
+ * operation. So, it is important to register such a callback here.
+ *
+ * Register this callback here to ensure that all types of logical
+ * replication workers that set up origins and apply remote transactions
+ * are protected.
+ */
+ before_shmem_exit(replorigin_reset, (Datum) 0);
}
/*
@@ -5892,19 +5909,6 @@ SetupApplyOrSyncWorker(int worker_slot)
InitializeLogRepWorker();
- /*
- * Register a callback to reset the origin state before aborting any
- * pending transaction during shutdown (see ShutdownPostgres()). This will
- * avoid origin advancement for an in-complete transaction which could
- * otherwise lead to its loss as such a transaction won't be sent by the
- * server again.
- *
- * Note that even a LOG or DEBUG statement placed after setting the origin
- * state may process a shutdown signal before committing the current apply
- * operation. So, it is important to register such a callback here.
- */
- before_shmem_exit(replorigin_reset, (Datum) 0);
-
/* Connect to the origin and start the replication. */
elog(DEBUG1, "connecting to publisher using connection string \"%s\"",
MySubscription->conninfo);
diff --git a/src/test/subscription/t/023_twophase_stream.pl b/src/test/subscription/t/023_twophase_stream.pl
index e01347ca699..dc629425daa 100644
--- a/src/test/subscription/t/023_twophase_stream.pl
+++ b/src/test/subscription/t/023_twophase_stream.pl
@@ -429,6 +429,51 @@ $result =
$node_subscriber->safe_psql('postgres', "SELECT count(*) FROM test_tab_2");
is($result, qq(1), 'transaction is committed on subscriber');
+# Test the ability to re-apply a transaction when a parallel apply worker fails
+# to prepare the transaction due to insufficient max_prepared_transactions
+# setting.
+$node_subscriber->append_conf(
+ 'postgresql.conf', qq(
+max_prepared_transactions = 0
+debug_logical_replication_streaming = buffered
+));
+$node_subscriber->restart;
+
+$node_publisher->safe_psql(
+ 'postgres', q{
+ BEGIN;
+ INSERT INTO test_tab_2 values(2);
+ PREPARE TRANSACTION 'xact';
+ COMMIT PREPARED 'xact';
+ });
+
+$offset = -s $node_subscriber->logfile;
+
+# Confirm the ERROR is reported because max_prepared_transactions is zero
+$node_subscriber->wait_for_log(
+ qr/ERROR: ( [A-Z0-9]+:)? prepared transactions are disabled/,
+ $offset);
+
+# Confirm that the parallel apply worker has encountered an error. The check
+# focuses on the worker type as a keyword, since the error message content may
+# differ based on whether the leader initially detected the parallel apply
+# worker's failure or received a signal from it.
+$node_subscriber->wait_for_log(
+ qr/ERROR: .*logical replication parallel apply worker.*/,
+ $offset);
+
+# Set max_prepared_transactions to correct value to resume the replication
+$node_subscriber->append_conf('postgresql.conf',
+ qq(max_prepared_transactions = 10));
+$node_subscriber->restart;
+
+$node_publisher->wait_for_catchup($appname);
+
+# Check that transaction is committed on subscriber
+$result =
+ $node_subscriber->safe_psql('postgres', "SELECT count(*) FROM test_tab_2");
+is($result, qq(2), 'transaction is committed on subscriber after retrying');
+
###############################
# check all the cleanup
###############################
--
2.51.1.windows.1
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2025-12-24 06:28 Amit Kapila <amit.kapila16@gmail.com>
parent: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
1 sibling, 0 replies; 28+ messages in thread
From: Amit Kapila @ 2025-12-24 06:28 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; shveta malik <shveta.malik@gmail.com>
On Tue, Dec 23, 2025 at 12:12 PM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> Here are the updated patches for all branches.
>
Pushed.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2026-08-05 05:00 Alexander Lakhin <exclusion@gmail.com>
parent: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
1 sibling, 1 reply; 28+ messages in thread
From: Alexander Lakhin @ 2026-08-05 05:00 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; Amit Kapila <amit.kapila16@gmail.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; shveta malik <shveta.malik@gmail.com>
Hello hackers,
23.12.2025 08:42, Zhijie Hou (Fujitsu) wrote:
> On Tuesday, December 23, 2025 12:21 PM Amit Kapila<amit.kapila16@gmail.com> wrote:
>> +# Test the ability to re-apply a transaction when a parallel apply
>> +worker fails # to prepare the transaction due to insufficient
>> +max_prepared_transactions # setting.
>> +$node_subscriber->append_conf('postgresql.conf',
>>
>> How does the test ensure that error is raised by parallel apply worker? I see
>> that in the previous test, we set 'debug_logical_replication_streaming =
>> immediate', so that should help to invoke parallel apply worker. But is there a
>> more direct way to ensure the same? Can we test for LOG like: "ERROR:
>> logical replication parallel apply worker exited due to error"?
> OK, I have added a general log test for "ERROR .. logical replication parallel
> apply worker ..." to ensure that it's the parallel apply worker that failed to
> apply the transaction.
Buildfarm animal olingo discovered a way to break this test, [1]:
400/402 subscription - postgresql:subscription/023_twophase_stream ERROR 230.82s (exit status 255 or 0xff)
[22:24:32.246](0.650s) ok 29 - transaction is committed on subscriber
### Restarting node "subscriber"
# Running: pg_ctl --wait --pgdata
/home/bf/bf-build/olingo/REL_19_STABLE/pgsql.build/testrun/subscription/023_twophase_stream/data/t_023_twophase_stream_subscriber_data/pgdata
--log
/home/bf/bf-build/olingo/REL_19_STABLE/pgsql.build/testrun/subscription/023_twophase_stream/log/023_twophase_stream_subscriber.log
restart
waiting for server to shut down.... done
server stopped
waiting for server to start.... done
server started
# Postmaster PID for node "subscriber" is 61596
[22:27:45.401](193.155s) # die: timed out waiting for file
/home/bf/bf-build/olingo/REL_19_STABLE/pgsql.build/testrun/subscription/023_twophase_stream/log/023_twophase_stream_subscriber.log
contents to match: (?^:ERROR: .*logical replication parallel apply worker.*) at
/home/bf/bf-build/olingo/REL_19_STABLE/pgsql/src/test/subscription/t/023_twophase_stream.pl line 460.
[22:27:45.401](0.001s) 1..29
timed out waiting for file
/home/bf/bf-build/olingo/REL_19_STABLE/pgsql.build/testrun/subscription/023_twophase_stream/log/023_twophase_stream_subscriber.log
contents to match: (?^:ERROR: .*logical replication parallel apply worker.*) at
/home/bf/bf-build/olingo/REL_19_STABLE/pgsql/src/test/subscription/t/023_twophase_stream.pl line 460.
# Postmaster PID for node "publisher" is 54199
### Stopping node "publisher" using mode immediate
pgsql.build/testrun/subscription/023_twophase_stream/log/023_twophase_stream_subscriber.log
2026-08-03 22:24:33.209 CEST [62186][logical replication parallel worker][120/4:730] ERROR: prepared transactions are
disabled
2026-08-03 22:24:33.209 CEST [62186][logical replication parallel worker][120/4:730] HINT: Set
"max_prepared_transactions" to a nonzero value.
2026-08-03 22:24:33.209 CEST [62186][logical replication parallel worker][120/4:730] CONTEXT: processing remote data
for replication origin "pg_16397" during message type "STREAM PREPARE" in transaction 719, finished at 0/01810A18
2026-08-03 22:24:33.211 CEST [61742][logical replication apply worker][119/0:0] ERROR: logical replication parallel
apply worker exited due to error
2026-08-03 22:24:33.211 CEST [61742][logical replication apply worker][119/0:0] CONTEXT: processing remote data for
replication origin "pg_16397" during message type "STREAM PREPARE" in transaction 719, finished at 0/01810A18
logical replication parallel apply worker
processing remote data for replication origin "pg_16397" during message type "STREAM PREPARE" in transaction 719,
finished at 0/01810A18
2026-08-03 22:24:33.227 CEST [61596][postmaster][:0] LOG: background worker "logical replication parallel worker" (PID
62186) exited with exit code 1
2026-08-03 22:24:33.250 CEST [62213][logical replication apply worker][121/2:0] LOG: logical replication apply worker
for subscription "tap_sub" has started
2026-08-03 22:24:33.267 CEST [61596][postmaster][:0] LOG: background worker "logical replication apply worker" (PID
61742) exited with exit code 1
2026-08-03 22:24:33.552 CEST [62213][logical replication apply worker][121/0:0] ERROR: prepared transactions are disabled
2026-08-03 22:24:33.552 CEST [62213][logical replication apply worker][121/0:0] HINT: Set "max_prepared_transactions"
to a nonzero value.
2026-08-03 22:24:33.552 CEST [62213][logical replication apply worker][121/0:0] CONTEXT: processing remote data for
replication origin "pg_16397" during message type "PREPARE" in transaction 719, finished at 0/01810A18
2026-08-03 22:24:33.569 CEST [61596][postmaster][:0] LOG: background worker "logical replication apply worker" (PID
62213) exited with exit code 1
...
2026-08-03 22:27:44.655 CEST [61596][postmaster][:0] LOG: background worker "logical replication apply worker" (PID
183059) exited with exit code 1
2026-08-03 22:27:45.113 CEST [183345][logical replication apply worker][123/262:0] LOG: logical replication apply
worker for subscription "tap_sub" has started
2026-08-03 22:27:45.154 CEST [183345][logical replication apply worker][123/0:0] ERROR: prepared transactions are disabled
2026-08-03 22:27:45.154 CEST [183345][logical replication apply worker][123/0:0] HINT: Set "max_prepared_transactions"
to a nonzero value.
2026-08-03 22:27:45.154 CEST [183345][logical replication apply worker][123/0:0] CONTEXT: processing remote data for
replication origin "pg_16397" during message type "PREPARE" in transaction 719, finished at 0/01810A18
2026-08-03 22:27:45.192 CEST [61596][postmaster][:0] LOG: background worker "logical replication apply worker" (PID
183345) exited with exit code 1
2026-08-03 22:27:45.565 CEST [61596][postmaster][:0] LOG: received immediate shutdown request
That is, there is only one "ERROR: logical replication parallel apply
worker exited due to error" in the log, all the following errors are
produced not by parallel worker, but by a leader worker...
I'm able to reproduce this failure with:
--- a/src/test/subscription/t/023_twophase_stream.pl
+++ b/src/test/subscription/t/023_twophase_stream.pl
@@ -445,2 +445,3 @@ $node_publisher->safe_psql(
INSERT INTO test_tab_2 values(2);
+ SELECT pg_sleep(0.5);
PREPARE TRANSACTION 'xact';
Could you have a look, please?
[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=olingo&dt=2026-08-03%2020%3A13%3A19
Best regards,
Alexander
^ permalink raw reply [nested|flat] 28+ messages in thread
* RE: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2026-08-05 05:25 Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
parent: Alexander Lakhin <exclusion@gmail.com>
0 siblings, 1 reply; 28+ messages in thread
From: Zhijie Hou (Fujitsu) @ 2026-08-05 05:25 UTC (permalink / raw)
To: Alexander Lakhin <exclusion@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; shveta malik <shveta.malik@gmail.com>
Hi,
On Wednesday, August 5, 2026 1:00 PM Alexander Lakhin <exclusion@gmail.com> wrote:
>
> Buildfarm animal olingo discovered a way to break this test, [1]:
> 400/402 subscription - postgresql:subscription/023_twophase_stream
> ERROR 230.82s (exit status 255 or 0xff)
>
> pgsql.build/testrun/subscription/023_twophase_stream/log/023_twophase_s
> tream_subscriber.log
> ...
> That is, there is only one "ERROR: logical replication parallel apply worker
> exited due to error" in the log, all the following errors are produced not by
> parallel worker, but by a leader worker...
>
> I'm able to reproduce this failure with:
> --- a/src/test/subscription/t/023_twophase_stream.pl
> +++ b/src/test/subscription/t/023_twophase_stream.pl
> @@ -445,2 +445,3 @@ $node_publisher->safe_psql(
> INSERT INTO test_tab_2 values(2);
> + SELECT pg_sleep(0.5);
> PREPARE TRANSACTION 'xact';
>
> Could you have a look, please?
Thanks for reporting this. I confirmed that this is a race condition in the test
- the log offset is captured after the publisher's prepared transaction runs, so
the expected ERROR may have already been written before the offset was recorded.
This would cause the subsequent search to miss the message. To fix it, we can
simply get the offset before the prepared transaction runs.
The patch fixes the timeout on my machine (after adding the mentioned pg_sleep(0.5)).
Best Regards,
Zhijie Hou
Attachments:
[application/octet-stream] v1-0001-Fix-BF-failure-for-023_twophase_stream.patch (1.6K, ../../TY4PR01MB177187B1A2B34DC0F1010EBDF94D32@TY4PR01MB17718.jpnprd01.prod.outlook.com/2-v1-0001-Fix-BF-failure-for-023_twophase_stream.patch)
download | inline diff:
From e8730d8c04b61467be89d201b8c518b8bfb1ad7a Mon Sep 17 00:00:00 2001
From: Zhijie Hou <houzj.fnst@fujitsu.com>
Date: Wed, 5 Aug 2026 13:14:40 +0800
Subject: [PATCH v1] Fix BF failure for 023_twophase_stream
The tap-test checks that the parallel apply worker reports an ERROR because
max_prepared_transactions is zero. However, the log offset was captured after
the publisher's prepared transaction had already run, so the expected ERROR
might have been written to the log before the offset was recorded. This caused
the subsequent wait_for_log() call to miss the message and time out. Fix by
capturing the offset before the prepared transaction runs.
Author: Zhijie Hou <houzj.fnst@fujitsu.com>
Reported-by: Alexander Lakhin <exclusion@gmail.com>
---
src/test/subscription/t/023_twophase_stream.pl | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/test/subscription/t/023_twophase_stream.pl b/src/test/subscription/t/023_twophase_stream.pl
index e9e0f753f7c..fbb2dffbd95 100644
--- a/src/test/subscription/t/023_twophase_stream.pl
+++ b/src/test/subscription/t/023_twophase_stream.pl
@@ -439,6 +439,8 @@ debug_logical_replication_streaming = buffered
));
$node_subscriber->restart;
+$offset = -s $node_subscriber->logfile;
+
$node_publisher->safe_psql(
'postgres', q{
BEGIN;
@@ -447,8 +449,6 @@ $node_publisher->safe_psql(
COMMIT PREPARED 'xact';
});
-$offset = -s $node_subscriber->logfile;
-
# Confirm the ERROR is reported because max_prepared_transactions is zero
$node_subscriber->wait_for_log(
qr/ERROR: ( [A-Z0-9]+:)? prepared transactions are disabled/, $offset);
--
2.43.0
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2026-08-06 09:12 Amit Kapila <amit.kapila16@gmail.com>
parent: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
0 siblings, 1 reply; 28+ messages in thread
From: Amit Kapila @ 2026-08-06 09:12 UTC (permalink / raw)
To: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; +Cc: Alexander Lakhin <exclusion@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; shveta malik <shveta.malik@gmail.com>
On Wed, Aug 5, 2026 at 10:55 AM Zhijie Hou (Fujitsu)
<houzj.fnst@fujitsu.com> wrote:
>
> Thanks for reporting this. I confirmed that this is a race condition in the test
> - the log offset is captured after the publisher's prepared transaction runs, so
> the expected ERROR may have already been written before the offset was recorded.
> This would cause the subsequent search to miss the message. To fix it, we can
> simply get the offset before the prepared transaction runs.
>
> The patch fixes the timeout on my machine (after adding the mentioned pg_sleep(0.5)).
>
The patch looks good to me, so I pushed.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 28+ messages in thread
* Re: [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber
@ 2026-08-07 05:00 Alexander Lakhin <exclusion@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 0 replies; 28+ messages in thread
From: Alexander Lakhin @ 2026-08-07 05:00 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>; shveta malik <shveta.malik@gmail.com>
Hello Amit and Zhijie,
06.08.2026 12:12, Amit Kapila wrote:
> On Wed, Aug 5, 2026 at 10:55 AM Zhijie Hou (Fujitsu)
> <houzj.fnst@fujitsu.com> wrote:
>> The patch fixes the timeout on my machine (after adding the mentioned pg_sleep(0.5)).
> The patch looks good to me, so I pushed.
Thank you for fixing the test!
Best regards,
Alexander
^ permalink raw reply [nested|flat] 28+ messages in thread
end of thread, other threads:[~2026-08-07 05:00 UTC | newest]
Thread overview: 28+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-08-08 05:07 [bug fix] prepared transaction might be lost when max_prepared_transactions is zero on the subscriber Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
2024-08-08 06:33 ` Amit Kapila <amit.kapila16@gmail.com>
2024-08-08 10:01 ` shveta malik <shveta.malik@gmail.com>
2024-08-08 10:11 ` Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2024-08-08 12:38 ` Amit Kapila <amit.kapila16@gmail.com>
2024-08-09 04:04 ` shveta malik <shveta.malik@gmail.com>
2024-08-09 05:49 ` Amit Kapila <amit.kapila16@gmail.com>
2025-12-22 09:01 ` Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2025-12-22 09:28 ` Chao Li <li.evan.chao@gmail.com>
2025-12-22 11:09 ` Chao Li <li.evan.chao@gmail.com>
2025-12-23 04:21 ` Amit Kapila <amit.kapila16@gmail.com>
2025-12-23 06:42 ` Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2025-12-24 06:28 ` Amit Kapila <amit.kapila16@gmail.com>
2026-08-05 05:00 ` Alexander Lakhin <exclusion@gmail.com>
2026-08-05 05:25 ` Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
2026-08-06 09:12 ` Amit Kapila <amit.kapila16@gmail.com>
2026-08-07 05:00 ` Alexander Lakhin <exclusion@gmail.com>
2024-08-09 09:09 ` Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
2024-08-12 10:06 ` shveta malik <shveta.malik@gmail.com>
2024-08-12 10:10 ` shveta malik <shveta.malik@gmail.com>
2024-08-13 04:18 ` Amit Kapila <amit.kapila16@gmail.com>
2024-08-14 04:55 ` shveta malik <shveta.malik@gmail.com>
2024-08-20 06:06 ` Amit Kapila <amit.kapila16@gmail.com>
2024-08-20 08:30 ` shveta malik <shveta.malik@gmail.com>
2024-08-21 09:57 ` Amit Kapila <amit.kapila16@gmail.com>
2024-11-07 19:23 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-11-08 03:05 ` Amit Kapila <amit.kapila16@gmail.com>
2024-08-14 09:23 ` shveta malik <shveta.malik@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox