From: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Cc: Shawn McCoy <shawn.the.mccoy@gmail.com>
Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>
Cc: drewwcallahan@gmail.com <drewwcallahan@gmail.com>
Cc: scott@meads.us <scott@meads.us>
Subject: RE: Disabled logical replication origin session causes primary key errors
Date: Wed, 23 Apr 2025 01:41:20 +0000
Message-ID: <OSCPR01MB149668436272E7ADE05202A1AF5BA2@OSCPR01MB14966.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAD21AoDhBDJcWsmsS4Du8E6Vzk6krxu65sCbpGggBxLHj3-8eA@mail.gmail.com>
References: <CALsgZNCGARa2mcYNVTSj9uoPcJo-tPuWUGECReKpNgTpo31_Pw@mail.gmail.com>
<CAD21AoA1K8pD-90B-KW_8ifRRQU7_nOBZTo3We6ENpu+uCSdUw@mail.gmail.com>
<CAA4eK1KQpQjPs_LaF=zUwJxa0vEQ5eiaYNh7f+TyCT4NqZJQ9Q@mail.gmail.com>
<OSCPR01MB14966B223A32363FF8D253D0EF5B82@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<CAD21AoCcO1QknzcumBNT-HWnb5cKkszzDQv2+Yv71sj9mk75xg@mail.gmail.com>
<OSCPR01MB149665D61D6C16D0F4AA66B5BF5BB2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<CALDaNm1F6jLdFfHA6t+KdFsf1J8qtAXVW3XGgeVFa6CegKWNvQ@mail.gmail.com>
<OSCPR01MB14966C633C0C7CDB7CC7D844BF5BB2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
<CAD21AoDhBDJcWsmsS4Du8E6Vzk6krxu65sCbpGggBxLHj3-8eA@mail.gmail.com>
Dear Sawada-san,
Thanks for giving comments!
> +# The bug was that the replication origin wasn’t updated whe
> +# apply_error_callback() was called with elevel >= ERROR, and the apply
> worker
> +# continued running afterward.
>
> I think it would be better to mention the fact that the problem
> happened when an error was caught for instance by a plpgsql function.
> How about rewriting it as follows?
>
> # The bug was that when an ERROR was caught, for instance by a
> PL/pgSQL function,
> # the apply worker reset the replication origin but continued processing
> # subsequent changes. This behavior resulted in a failure to update
> the replication
> # origin during further apply operations.
I tried to describe the internal reasons of bugs, but yours did reported facts.
+1, replaced.
> ---
> +# Define an after-trigger function for the table insert. It can be fired even
> +# by the apply worker and always raises an exception. This situation allows
> +# worker continue after apply_error_callback() is called with elevel = ERROR.
> +$node_subscriber->safe_psql(
> + 'postgres', q{
> +CREATE FUNCTION handle_exception_trigger()
> +RETURNS TRIGGER AS $$
> +BEGIN
> + BEGIN
> + -- Raise an exception
> + RAISE EXCEPTION 'This is a test exception';
> + EXCEPTION
> + WHEN OTHERS THEN
> + RETURN NEW;
> + END;
> +
> + RETURN NEW;
> +END;
> +$$ LANGUAGE plpgsql;
> +});
> +
> +$node_subscriber->safe_psql(
> + 'postgres', q{
> +CREATE TRIGGER silent_exception_trigger
> +AFTER INSERT OR UPDATE ON t1
> +FOR EACH ROW
> +EXECUTE FUNCTION handle_exception_trigger();
> +
> +ALTER TABLE t1 ENABLE ALWAYS TRIGGER silent_exception_trigger;
> +});
>
> How about rewriting the comment as follows?
>
> # Create an AFTER INSERT trigger on the table that raises and subsequently
> # handles an exception. Subsequent insertions will trigger this exception,
> # causing the apply worker to invoke its error callback with an ERROR. However,
> # since the error is caught within the trigger, the apply worker will continue
> # processing changes.
Fixed.
> And can we execute these queries in one safe_psql() call?
Yes possible. I intentionally separated to make it clearer, but I did not have
strong opinions. Fixed.
> ---
> +# Obtain current remote_lsn value to check its advancement later
> +my $remote_lsn = $node_subscriber->safe_psql('postgres',
> + "SELECT remote_lsn FROM
> pg_catalog.pg_replication_origin_status os, pg_catalog.pg_subscription
> s WHERE os.external_id = 'pg_' || s.oid AND s.subname = 'regress_sub'"
> +);
>
> It seems to make sense to me to get the remote_lsn value just before
> executing INSERT after creating the trigger.
Moved.
> Is it a conventional way to always use schema-qualified catalogs names
> in regression tests? Looking at other tests in src/test/subscription,
> there are only three cases:
>
> % git grep pg_catalog src/test/subscription/t
> src/test/subscription/t/001_rep_changes.pl: FROM
> pg_catalog.pg_stat_io
> src/test/subscription/t/020_messages.pl: "SELECT COUNT(*) FROM
> pg_catalog.pg_replication_slots WHERE slot_name = 'tap_sub' AND
> active='f'",
> src/test/subscription/t/029_on_error.pl: "SELECT subenabled = false
> FROM pg_catalog.pg_subscription WHERE subname = 'sub'"
>
> ISTM that we don't necessarily need to make the catalog name schema-qualified.
I referred parts of Cluster.pm and 040_standby_failover_slots_sync.pl, and they
had "pg_catalog" prefix. After considering more, instances would be created within
the test and we ensure to connect to them - we can assume they are safe, OK to
remove it.
> ---
> We might want to stop both the publisher and the subscriber at the end
> of the tests.
Opps, added.
Attached patch could pass tests on ,my env, and pgperltidy said OK.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Attachments:
[application/octet-stream] v6-PG16-PG17-0001-Fix-oversight-3f28b2f.patch (5.4K, ../OSCPR01MB149668436272E7ADE05202A1AF5BA2@OSCPR01MB14966.jpnprd01.prod.outlook.com/2-v6-PG16-PG17-0001-Fix-oversight-3f28b2f.patch)
download | inline diff:
From 94e3522510daf853b57a1039f7db2086e3344363 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Tue, 22 Apr 2025 18:57:05 +0900
Subject: [PATCH v6-PG16-PG17] Fix oversight 3f28b2f
3f28b2f introduced replorigin_reset() in apply_error_callback() in case of
elevel >= ERROR to ensure the replication origin is released from the session
before exiting the process. However, in some cases, apply_error_callback() can
be called with ERROR, but the apply worker can continue applying the logical
replication. If this happens, the worker applies changes without setting the
replication origin, which can lead to data duplication.
This commit moves replorigin_reset() to PG_CATCH() part in start_apply(). The
path can be reached only by the exiting workers
---
src/backend/replication/logical/worker.c | 21 ++++---
src/test/subscription/t/100_bugs.pl | 76 ++++++++++++++++++++++++
2 files changed, 86 insertions(+), 11 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.cindex 65e22306c48..db09978697f 100644--- a/src/backend/replication/logical/worker.c+++ b/src/backend/replication/logical/worker.c@@ -416,6 +416,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.
*
@@ -4441,6 +4443,14 @@ start_apply(XLogRecPtr origin_startpos)
}
PG_CATCH();
{
+ /*+ * 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.+ */+ replorigin_reset(0, (Datum) 0);+
if (MySubscription->disableonerr)
DisableSubscriptionAndExit();
else
@@ -4928,23 +4938,12 @@ 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/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.plindex cb36ca7b16b..3c00e2f8867 100644--- a/src/test/subscription/t/100_bugs.pl+++ b/src/test/subscription/t/100_bugs.pl@@ -490,4 +490,80 @@ is( $result, qq(2|f
$node_publisher->stop('fast');
$node_subscriber->stop('fast');
+# The bug was that when an ERROR was caught, for instance by a PL/pgSQL+# function, the apply worker reset the replication origin but continued+# processing subsequent changes. This behavior resulted in a failure to update+# the replication origin during further apply operations.++$node_publisher->rotate_logfile();+$node_publisher->start();++$node_subscriber->rotate_logfile();+$node_subscriber->start();++# Set up a publication with a table+$node_publisher->safe_psql(+ 'postgres', qq(+ CREATE TABLE t1 (a int);+ CREATE PUBLICATION regress_pub FOR TABLE t1;+));++# Set up a subscription which subscribes the publication+$node_subscriber->safe_psql(+ 'postgres', qq(+ CREATE TABLE t1 (a int);+ CREATE SUBSCRIPTION regress_sub CONNECTION '$publisher_connstr' PUBLICATION regress_pub;+));++$node_subscriber->wait_for_subscription_sync($node_publisher, 'regress_sub');++# Create an AFTER INSERT trigger on the table that raises and subsequently+# handles an exception. Subsequent insertions will trigger this exception,+# causing the apply worker to invoke its error callback with an ERROR. However,+# since the error is caught within the trigger, the apply worker will continue+# processing changes.+$node_subscriber->safe_psql(+ 'postgres', q{+CREATE FUNCTION handle_exception_trigger()+RETURNS TRIGGER AS $$+BEGIN+ BEGIN+ -- Raise an exception+ RAISE EXCEPTION 'This is a test exception';+ EXCEPTION+ WHEN OTHERS THEN+ RETURN NEW;+ END;++ RETURN NEW;+END;+$$ LANGUAGE plpgsql;++CREATE TRIGGER silent_exception_trigger+AFTER INSERT OR UPDATE ON t1+FOR EACH ROW+EXECUTE FUNCTION handle_exception_trigger();++ALTER TABLE t1 ENABLE ALWAYS TRIGGER silent_exception_trigger;+});++# Obtain current remote_lsn value to check its advancement later+my $remote_lsn = $node_subscriber->safe_psql('postgres',+ "SELECT remote_lsn FROM pg_replication_origin_status os, pg_subscription s WHERE os.external_id = 'pg_' || s.oid AND s.subname = 'regress_sub'"+);++# Insert a tuple to replicate changes+$node_publisher->safe_psql('postgres', "INSERT INTO t1 VALUES (1);");+$node_publisher->wait_for_catchup('regress_sub');++# Confirms the origin can be advanced+$result = $node_subscriber->safe_psql('postgres',+ "SELECT remote_lsn > '$remote_lsn' FROM pg_replication_origin_status os, pg_subscription s WHERE os.external_id = 'pg_' || s.oid AND s.subname = 'regress_sub'"+);+is($result, 't',+ 'remote_lsn has advanced for apply worker raising an exception');++$node_publisher->stop('fast');+$node_subscriber->stop('fast');+
done_testing();
--
2.43.5
[application/octet-stream] v6-HEAD-0001-Fix-oversight-3f28b2f.patch (6.6K, ../OSCPR01MB149668436272E7ADE05202A1AF5BA2@OSCPR01MB14966.jpnprd01.prod.outlook.com/3-v6-HEAD-0001-Fix-oversight-3f28b2f.patch)
download | inline diff:
From 9939ae1e5eac420f62137a119680e1bf5a1c8f48 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Mon, 21 Apr 2025 15:16:31 +0900
Subject: [PATCH v6-HEAD] Fix oversight 3f28b2f
3f28b2f introduced replorigin_reset() in apply_error_callback() in case of
elevel >= ERROR to ensure the replication origin is released from the session
before exiting the process. However, in some cases, apply_error_callback() can
be called with ERROR, but the apply worker can continue applying the logical
replication. If this happens, the worker applies changes without setting the
replication origin, which can lead to data duplication.
This commit moves replorigin_reset() to PG_CATCH() part in start_apply(). The
path can be reached only by the exiting workers.
---
src/backend/replication/logical/worker.c | 21 ++++---
src/backend/utils/error/elog.c | 17 ------
src/include/utils/elog.h | 1 -
src/test/subscription/t/100_bugs.pl | 76 ++++++++++++++++++++++++
4 files changed, 86 insertions(+), 29 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.cindex 5ce596f4576..4151a4b2a96 100644--- a/src/backend/replication/logical/worker.c+++ b/src/backend/replication/logical/worker.c@@ -414,6 +414,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.
*
@@ -4516,6 +4518,14 @@ start_apply(XLogRecPtr origin_startpos)
}
PG_CATCH();
{
+ /*+ * 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.+ */+ replorigin_reset(0, (Datum) 0);+
if (MySubscription->disableonerr)
DisableSubscriptionAndExit();
else
@@ -5004,23 +5014,12 @@ 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.cindex d6299633ab7..47af743990f 100644--- a/src/backend/utils/error/elog.c+++ b/src/backend/utils/error/elog.c@@ -1590,23 +1590,6 @@ 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.hindex a5313c5d2d5..5eac0e16970 100644--- a/src/include/utils/elog.h+++ b/src/include/utils/elog.h@@ -227,7 +227,6 @@ 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/100_bugs.pl b/src/test/subscription/t/100_bugs.plindex b3924ca4b09..6249abdf3f7 100644--- a/src/test/subscription/t/100_bugs.pl+++ b/src/test/subscription/t/100_bugs.pl@@ -498,4 +498,80 @@ is( $node_publisher->psql(
$node_publisher->stop('fast');
$node_subscriber->stop('fast');
+# The bug was that when an ERROR was caught, for instance by a PL/pgSQL+# function, the apply worker reset the replication origin but continued+# processing subsequent changes. This behavior resulted in a failure to update+# the replication origin during further apply operations.++$node_publisher->rotate_logfile();+$node_publisher->start();++$node_subscriber->rotate_logfile();+$node_subscriber->start();++# Set up a publication with a table+$node_publisher->safe_psql(+ 'postgres', qq(+ CREATE TABLE t1 (a int);+ CREATE PUBLICATION regress_pub FOR TABLE t1;+));++# Set up a subscription which subscribes the publication+$node_subscriber->safe_psql(+ 'postgres', qq(+ CREATE TABLE t1 (a int);+ CREATE SUBSCRIPTION regress_sub CONNECTION '$publisher_connstr' PUBLICATION regress_pub;+));++$node_subscriber->wait_for_subscription_sync($node_publisher, 'regress_sub');++# Create an AFTER INSERT trigger on the table that raises and subsequently+# handles an exception. Subsequent insertions will trigger this exception,+# causing the apply worker to invoke its error callback with an ERROR. However,+# since the error is caught within the trigger, the apply worker will continue+# processing changes.+$node_subscriber->safe_psql(+ 'postgres', q{+CREATE FUNCTION handle_exception_trigger()+RETURNS TRIGGER AS $$+BEGIN+ BEGIN+ -- Raise an exception+ RAISE EXCEPTION 'This is a test exception';+ EXCEPTION+ WHEN OTHERS THEN+ RETURN NEW;+ END;++ RETURN NEW;+END;+$$ LANGUAGE plpgsql;++CREATE TRIGGER silent_exception_trigger+AFTER INSERT OR UPDATE ON t1+FOR EACH ROW+EXECUTE FUNCTION handle_exception_trigger();++ALTER TABLE t1 ENABLE ALWAYS TRIGGER silent_exception_trigger;+});++# Obtain current remote_lsn value to check its advancement later+my $remote_lsn = $node_subscriber->safe_psql('postgres',+ "SELECT remote_lsn FROM pg_replication_origin_status os, pg_subscription s WHERE os.external_id = 'pg_' || s.oid AND s.subname = 'regress_sub'"+);++# Insert a tuple to replicate changes+$node_publisher->safe_psql('postgres', "INSERT INTO t1 VALUES (1);");+$node_publisher->wait_for_catchup('regress_sub');++# Confirms the origin can be advanced+$result = $node_subscriber->safe_psql('postgres',+ "SELECT remote_lsn > '$remote_lsn' FROM pg_replication_origin_status os, pg_subscription s WHERE os.external_id = 'pg_' || s.oid AND s.subname = 'regress_sub'"+);+is($result, 't',+ 'remote_lsn has advanced for apply worker raising an exception');++$node_publisher->stop('fast');+$node_subscriber->stop('fast');+
done_testing();
--
2.43.5
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: kuroda.hayato@fujitsu.com, sawada.mshk@gmail.com, vignesh21@gmail.com, amit.kapila16@gmail.com, shawn.the.mccoy@gmail.com, pgsql-bugs@lists.postgresql.org, drewwcallahan@gmail.com, scott@meads.us
Subject: RE: Disabled logical replication origin session causes primary key errors
In-Reply-To: <OSCPR01MB149668436272E7ADE05202A1AF5BA2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
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