pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedDisabled logical replication origin session causes primary key errors
17+ messages / 6 participants
[nested] [flat]
* Disabled logical replication origin session causes primary key errors
@ 2025-04-18 21:22 Shawn McCoy <shawn.the.mccoy@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Shawn McCoy @ 2025-04-18 21:22 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: swada.mshk@gmail.com, drewwcallahan@gmail.com, "scott@meads.us" <scott@meads.us>
Hello,
We have discovered a recent regression in the Origin handling of logical
replication apply workers. We have found the cause of the issue was due to
the worker resetting its local origin session information during the
processing of an error that is
silently handled allowing the worker to continue. We suspect this is caused
by the recent change made in the following thread,
https://www.postgresql.org/message-id/TYAPR01MB5692FAC23BE40C69DA8ED4AFF5B92@TYAPR01MB5692.jpnprd01....
.
The logical replication apply worker will originally setup the origin
correctly. However, on the first insert will call into the trigger which
will raise an exception. This exception will execute the error callback
that resets the origin session state. The exception will then be silently
handled, returning execution back to the apply worker. In the second
reproduction, a function based index is used with the same result.
At this point, the apply worker can continue to commit these changes, but
has cleared all local origin session state. As a result, we will not update
our remote to local LSN mapping of the origin. Allowing for duplicate data
to be applied.
This was tested and observed in at least these versions:
PostgreSQL 16.8
PostgreSQL 17.4
We provide a simple reproduction of the issue below in 2 separate use-cases.
Regards,
Shawn McCoy, Drew Callahan, Scott Mead
NOTE: wal_level = logical
Publication Session:
CREATE TABLE t1 (a INT);
CREATE PUBLICATION my_publication FOR TABLE t1;
Subscription Session:
CREATE TABLE t1 (a INT);
CREATE SUBSCRIPTION my_subscription
CONNECTION '<conninfo>'
PUBLICATION my_publication;
CREATE OR REPLACE FUNCTION handle_exception_trigger()
RETURNS TRIGGER AS $$
BEGIN
BEGIN
-- Raise an exception
RAISE EXCEPTION 'This is a test exception';
EXCEPTION
WHEN OTHERS THEN
-- Silently handle the exception
-- You can optionally log it if needed
-- RAISE NOTICE 'Caught exception: %', SQLERRM;
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();
-- Just force it to always trigger for simplicity
ALTER TABLE t1 ENABLE ALWAYS TRIGGER silent_exception_trigger;
-- Mark down the current value
SELECT * from pg_replication_origin_status;
Publication Session:
INSERT INTO t1 VALUES (1);
INSERT INTO t1 VALUES (2);
INSERT INTO t1 VALUES (3);
Subscription Session:
-- All 3 changes should be present
SELECT * FROM t1;
-- The new value will be equivalent to the original value
-- even though three new changes have been applied from the remote
SELECT * from pg_replication_origin_status;
Using a function based GIST index with a pgbench workload
NOTE: wal_level = logical
PUBLISHER:
1. setup a small pgbench with:
pgbench -i -s 10 postgres
2. set up table with an index based function to invoke the problem with origin and publication for this table and the modified pgbench_history table with a serial based primary key.
CREATE TABLE t1 (a text primary key);
CREATE or REPLACE FUNCTION cidr_from_text (a text)
RETURNS cidr as $$
BEGIN
RETURN CAST($1 as cidr);
EXCEPTION
WHEN INVALID_TEXT_REPRESENTATION THEN
RETURN NULL;
END;
$$ LANGUAGE PLPGSQL IMMUTABLE;
CREATE INDEX my_functional_index ON t1 using GIST (public.cidr_from_text(a) inet_ops) WHERE public.cidr_from_text(a) IS NOT NULL;
ALTER TABLE pgbench_history ADD COLUMN key SERIAL;
ALTER TABLE pgbench_history ADD PRIMARY KEY (key);
CREATE PUBLICATION my_publication FOR TABLE t1, pgbench_history;
CREATE USER repl_user WITH REPLICATION PASSWORD '<your_password>';
GRANT CONNECT ON DATABASE postgres TO repl_user;
GRANT SELECT ON ALL TABLES IN SCHEMA public TO repl_user;
SUBSCRIBER:
1. create tables on subscriber and setup subscription
CREATE TABLE t1 (a text primary key);
CREATE or REPLACE FUNCTION cidr_from_text (a text)
RETURNS cidr as $$
BEGIN
RETURN CAST($1 as cidr);
EXCEPTION
WHEN INVALID_TEXT_REPRESENTATION THEN
RETURN NULL;
END;
$$ LANGUAGE PLPGSQL IMMUTABLE;
CREATE INDEX my_functional_index ON t1 using GIST (public.cidr_from_text(a) inet_ops) WHERE public.cidr_from_text(a) IS NOT NULL;
CREATE TABLE public.pgbench_history (
tid integer,
bid integer,
aid integer,
delta integer,
mtime timestamp without time zone,
filler character(22)
);
ALTER TABLE pgbench_history ADD COLUMN key SERIAL;
ALTER TABLE pgbench_history ADD PRIMARY KEY (key);
CREATE SUBSCRIPTION my_subscription CONNECTION '<conninfo>' PUBLICATION my_publication;
This will need multiple sessions.
Session 1 on publisher:
1. start the pgbench workload
pgbench -c 5 -T 180 -n -s 5 postgres
Session 2 on publisher:
1. A good value and a bad value that will enter the exception clause
insert into t1 values ('192.168/24');
insert into t1 values ('bad_value_1');
Session 3 on subscriber:
1. Check origin status, it will have stopped advancing
select * from pg_replication_origin_status;
select * from pg_replication_origin_status;
2. Disable subscription
alter subscription my_subscription disable;
3. Enable subscription
alter subscription my_subscription enable;
4. View the postgresql.log and observe duplicate key violations and replication is now broken
2025-04-09 12:38:18.734 MDT [39789] LOG: logical replication worker for subscription "my_subscription" will stop because the subscription was disabled
2025-04-09 12:38:22.511 MDT [39859] LOG: logical replication apply worker for subscription "my_subscription" has started
2025-04-09 12:38:23.764 MDT [39859] ERROR: duplicate key value violates unique constraint "pgbench_history_pkey"
2025-04-09 12:38:23.764 MDT [39859] DETAIL: Key (key)=(276668) already exists.
2025-04-09 12:38:23.764 MDT [39859] CONTEXT: processing remote data for replication origin "pg_16527" during message type "INSERT" for replication target relation "public.pgbench_history" in transaction 4631967, finished at 1/8F25C2E0
2025-04-09 12:38:23.767 MDT [17454] LOG: background worker "logical replication worker" (PID 39859) exited with exit code 1
Attachments:
[text/plain] repro_local_origin_stuck.txt (1.3K, ../../CALsgZNCGARa2mcYNVTSj9uoPcJo-tPuWUGECReKpNgTpo31_Pw@mail.gmail.com/3-repro_local_origin_stuck.txt)
download | inline:
NOTE: wal_level = logical
Publication Session:
CREATE TABLE t1 (a INT);
CREATE PUBLICATION my_publication FOR TABLE t1;
Subscription Session:
CREATE TABLE t1 (a INT);
CREATE SUBSCRIPTION my_subscription
CONNECTION '<conninfo>'
PUBLICATION my_publication;
CREATE OR REPLACE FUNCTION handle_exception_trigger()
RETURNS TRIGGER AS $$
BEGIN
BEGIN
-- Raise an exception
RAISE EXCEPTION 'This is a test exception';
EXCEPTION
WHEN OTHERS THEN
-- Silently handle the exception
-- You can optionally log it if needed
-- RAISE NOTICE 'Caught exception: %', SQLERRM;
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();
-- Just force it to always trigger for simplicity
ALTER TABLE t1 ENABLE ALWAYS TRIGGER silent_exception_trigger;
-- Mark down the current value
SELECT * from pg_replication_origin_status;
Publication Session:
INSERT INTO t1 VALUES (1);
INSERT INTO t1 VALUES (2);
INSERT INTO t1 VALUES (3);
Subscription Session:
-- All 3 changes should be present
SELECT * FROM t1;
-- The new value will be equivalent to the original value
-- even though three new changes have been applied from the remote
SELECT * from pg_replication_origin_status;
[text/plain] repro_local_origin_stuck_causing_duplicate_key.txt (3.4K, ../../CALsgZNCGARa2mcYNVTSj9uoPcJo-tPuWUGECReKpNgTpo31_Pw@mail.gmail.com/4-repro_local_origin_stuck_causing_duplicate_key.txt)
download | inline:
Using a function based GIST index with a pgbench workload
NOTE: wal_level = logical
PUBLISHER:
1. setup a small pgbench with:
pgbench -i -s 10 postgres
2. set up table with an index based function to invoke the problem with origin and publication for this table and the modified pgbench_history table with a serial based primary key.
CREATE TABLE t1 (a text primary key);
CREATE or REPLACE FUNCTION cidr_from_text (a text)
RETURNS cidr as $$
BEGIN
RETURN CAST($1 as cidr);
EXCEPTION
WHEN INVALID_TEXT_REPRESENTATION THEN
RETURN NULL;
END;
$$ LANGUAGE PLPGSQL IMMUTABLE;
CREATE INDEX my_functional_index ON t1 using GIST (public.cidr_from_text(a) inet_ops) WHERE public.cidr_from_text(a) IS NOT NULL;
ALTER TABLE pgbench_history ADD COLUMN key SERIAL;
ALTER TABLE pgbench_history ADD PRIMARY KEY (key);
CREATE PUBLICATION my_publication FOR TABLE t1, pgbench_history;
CREATE USER repl_user WITH REPLICATION PASSWORD '<your_password>';
GRANT CONNECT ON DATABASE postgres TO repl_user;
GRANT SELECT ON ALL TABLES IN SCHEMA public TO repl_user;
SUBSCRIBER:
1. create tables on subscriber and setup subscription
CREATE TABLE t1 (a text primary key);
CREATE or REPLACE FUNCTION cidr_from_text (a text)
RETURNS cidr as $$
BEGIN
RETURN CAST($1 as cidr);
EXCEPTION
WHEN INVALID_TEXT_REPRESENTATION THEN
RETURN NULL;
END;
$$ LANGUAGE PLPGSQL IMMUTABLE;
CREATE INDEX my_functional_index ON t1 using GIST (public.cidr_from_text(a) inet_ops) WHERE public.cidr_from_text(a) IS NOT NULL;
CREATE TABLE public.pgbench_history (
tid integer,
bid integer,
aid integer,
delta integer,
mtime timestamp without time zone,
filler character(22)
);
ALTER TABLE pgbench_history ADD COLUMN key SERIAL;
ALTER TABLE pgbench_history ADD PRIMARY KEY (key);
CREATE SUBSCRIPTION my_subscription CONNECTION '<conninfo>' PUBLICATION my_publication;
This will need multiple sessions.
Session 1 on publisher:
1. start the pgbench workload
pgbench -c 5 -T 180 -n -s 5 postgres
Session 2 on publisher:
1. A good value and a bad value that will enter the exception clause
insert into t1 values ('192.168/24');
insert into t1 values ('bad_value_1');
Session 3 on subscriber:
1. Check origin status, it will have stopped advancing
select * from pg_replication_origin_status;
select * from pg_replication_origin_status;
2. Disable subscription
alter subscription my_subscription disable;
3. Enable subscription
alter subscription my_subscription enable;
4. View the postgresql.log and observe duplicate key violations and replication is now broken
2025-04-09 12:38:18.734 MDT [39789] LOG: logical replication worker for subscription "my_subscription" will stop because the subscription was disabled
2025-04-09 12:38:22.511 MDT [39859] LOG: logical replication apply worker for subscription "my_subscription" has started
2025-04-09 12:38:23.764 MDT [39859] ERROR: duplicate key value violates unique constraint "pgbench_history_pkey"
2025-04-09 12:38:23.764 MDT [39859] DETAIL: Key (key)=(276668) already exists.
2025-04-09 12:38:23.764 MDT [39859] CONTEXT: processing remote data for replication origin "pg_16527" during message type "INSERT" for replication target relation "public.pgbench_history" in transaction 4631967, finished at 1/8F25C2E0
2025-04-09 12:38:23.767 MDT [17454] LOG: background worker "logical replication worker" (PID 39859) exited with exit code 1
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Disabled logical replication origin session causes primary key errors
@ 2025-04-20 05:53 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Shawn McCoy <shawn.the.mccoy@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Masahiko Sawada @ 2025-04-20 05:53 UTC (permalink / raw)
To: Shawn McCoy <shawn.the.mccoy@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org, drewwcallahan@gmail.com, "scott@meads.us" <scott@meads.us>; Amit Kapila <amit.kapila16@gmail.com>
Hi,
Thank you for the report and sharing the reproducers.
On Fri, Apr 18, 2025 at 2:22 PM Shawn McCoy <shawn.the.mccoy@gmail.com> wrote:
>
> Hello,
>
> We have discovered a recent regression in the Origin handling of logical replication apply workers. We have found the cause of the issue was due to the worker resetting its local origin session information during the processing of an error that is
> silently handled allowing the worker to continue. We suspect this is caused by the recent change made in the following thread, https://www.postgresql.org/message-id/TYAPR01MB5692FAC23BE40C69DA8ED4AFF5B92@TYAPR01MB5692.jpnprd01.....
>
> The logical replication apply worker will originally setup the origin correctly. However, on the first insert will call into the trigger which will raise an exception. This exception will execute the error callback that resets the origin session state. The exception will then be silently handled, returning execution back to the apply worker. In the second reproduction, a function based index is used with the same result.
>
> At this point, the apply worker can continue to commit these changes, but has cleared all local origin session state. As a result, we will not update our remote to local LSN mapping of the origin. Allowing for duplicate data to be applied.
I agree with your analysis. When the subscriber restarts the logical
replication, changes that happened since the last acknowledged LSN
would be replicated again.
With commit 3f28b2fcac33f, we reset the replication origin in
apply_error_callback() but I guess moving it to the PG_CATCH() block
in start_apply() might work.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Disabled logical replication origin session causes primary key errors
@ 2025-04-21 05:17 Amit Kapila <amit.kapila16@gmail.com>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Amit Kapila @ 2025-04-21 05:17 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: Shawn McCoy <shawn.the.mccoy@gmail.com>; pgsql-bugs@lists.postgresql.org, drewwcallahan@gmail.com, "scott@meads.us" <scott@meads.us>
On Sun, Apr 20, 2025 at 11:24 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> Thank you for the report and sharing the reproducers.
>
> On Fri, Apr 18, 2025 at 2:22 PM Shawn McCoy <shawn.the.mccoy@gmail.com> wrote:
> >
> > Hello,
> >
> > We have discovered a recent regression in the Origin handling of logical replication apply workers. We have found the cause of the issue was due to the worker resetting its local origin session information during the processing of an error that is
> > silently handled allowing the worker to continue. We suspect this is caused by the recent change made in the following thread, https://www.postgresql.org/message-id/TYAPR01MB5692FAC23BE40C69DA8ED4AFF5B92@TYAPR01MB5692.jpnprd01.....
> >
> > The logical replication apply worker will originally setup the origin correctly. However, on the first insert will call into the trigger which will raise an exception. This exception will execute the error callback that resets the origin session state. The exception will then be silently handled, returning execution back to the apply worker. In the second reproduction, a function based index is used with the same result.
> >
> > At this point, the apply worker can continue to commit these changes, but has cleared all local origin session state. As a result, we will not update our remote to local LSN mapping of the origin. Allowing for duplicate data to be applied.
>
> I agree with your analysis. When the subscriber restarts the logical
> replication, changes that happened since the last acknowledged LSN
> would be replicated again.
>
> With commit 3f28b2fcac33f, we reset the replication origin in
> apply_error_callback() but I guess moving it to the PG_CATCH() block
> in start_apply() might work.
>
Right. We have wrongly assumed in that commit that the apply worker
will exit after an ERROR, but as shown by this case, the ERROR could
be silently handled. So, +1, for moving replication origin reset to
PG_CATCH in start_apply.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 17+ messages in thread
* RE: Disabled logical replication origin session causes primary key errors
@ 2025-04-21 10:08 Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 2 replies; 17+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2025-04-21 10:08 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; Masahiko Sawada <sawada.mshk@gmail.com>; Shawn McCoy <shawn.the.mccoy@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; drewwcallahan@gmail.com <drewwcallahan@gmail.com>; scott@meads.us <scott@meads.us>
Dear members,
Thanks for reporting the issue.
> Right. We have wrongly assumed in that commit that the apply worker
> will exit after an ERROR, but as shown by this case, the ERROR could
> be silently handled. So, +1, for moving replication origin reset to
> PG_CATCH in start_apply.
I was an author of original commit, so let me take initiative. When I was working
for 3f28b2fcac, I could not find path which ERROR is reported but worker can
survive so that I added replorigin_reset() in apply_error_callback(). The reported
case, however, the exception could be raised but the insert itself is committed.
In this case worker can continue working.
Attached patches have proposed changes. I did 1) meson test, 2) workloads provided
in [1], and 3) manual tests done in original thread [2], and all of them could be
passed. The version is 2 because of the self-reviewing.
One note is that geterrlevel() is removed for HEAD patch but retained for PG16/PG17.
The function is exported, and APIs cannot be changed in back branches.
How do you feel?
[1]: https://www.postgresql.org/message-id/flat/CALsgZNCGARa2mcYNVTSj9uoPcJo-tPuWUGECReKpNgTpo31_Pw%40mai...
[2]: https://www.postgresql.org/message-id/CAJpy0uAa3M9sC3GWTUA%2BYPTnYxEcorFt_7g6rXz05jNcAGdjgQ%40mail.g...
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Attachments:
[application/octet-stream] v2-HEAD-0001-Fix-oversight-3f28b2f.patch (3.7K, ../../OSCPR01MB14966B223A32363FF8D253D0EF5B82@OSCPR01MB14966.jpnprd01.prod.outlook.com/2-v2-HEAD-0001-Fix-oversight-3f28b2f.patch)
download | inline diff:
From b6305cf030b469e52aa68cfd5c4ea9976647686d 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 v2-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 | 20 +++++++++-----------
src/backend/utils/error/elog.c | 17 -----------------
src/include/utils/elog.h | 1 -
3 files changed, 9 insertions(+), 29 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 5ce596f4576..1fd04f05e6a 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,13 @@ 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 +5013,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.c
index 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.h
index 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);
--
2.43.5
[application/octet-stream] v2-PG16-PG17-0001-Fix-oversight-3f28b2f.patch (2.4K, ../../OSCPR01MB14966B223A32363FF8D253D0EF5B82@OSCPR01MB14966.jpnprd01.prod.outlook.com/3-v2-PG16-PG17-0001-Fix-oversight-3f28b2f.patch)
download | inline diff:
From fa8c3494720935dc37497f07f28c4468eddd8535 Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Mon, 21 Apr 2025 17:05:29 +0900
Subject: [PATCH v2-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 | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 65e22306c48..1f7fa1949a9 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,13 @@ 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 +4937,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))
--
2.43.5
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Disabled logical replication origin session causes primary key errors
@ 2025-04-21 16:17 vignesh C <vignesh21@gmail.com>
parent: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
1 sibling, 1 reply; 17+ messages in thread
From: vignesh C @ 2025-04-21 16:17 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Masahiko Sawada <sawada.mshk@gmail.com>; Shawn McCoy <shawn.the.mccoy@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; drewwcallahan@gmail.com <drewwcallahan@gmail.com>; scott@meads.us <scott@meads.us>
On Mon, 21 Apr 2025 at 15:38, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear members,
>
> Thanks for reporting the issue.
>
> > Right. We have wrongly assumed in that commit that the apply worker
> > will exit after an ERROR, but as shown by this case, the ERROR could
> > be silently handled. So, +1, for moving replication origin reset to
> > PG_CATCH in start_apply.
>
> I was an author of original commit, so let me take initiative. When I was working
> for 3f28b2fcac, I could not find path which ERROR is reported but worker can
> survive so that I added replorigin_reset() in apply_error_callback(). The reported
> case, however, the exception could be raised but the insert itself is committed.
> In this case worker can continue working.
>
> Attached patches have proposed changes. I did 1) meson test, 2) workloads provided
> in [1], and 3) manual tests done in original thread [2], and all of them could be
> passed. The version is 2 because of the self-reviewing.
>
> One note is that geterrlevel() is removed for HEAD patch but retained for PG16/PG17.
> The function is exported, and APIs cannot be changed in back branches.
>
> How do you feel?
I was able to reproduce the issue with the steps suggested by Shawn
and your patch fixes the issue. Your suggested changes look good. One
thought, do you feel we should include a test for this.
Regards,
Vignesh
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Disabled logical replication origin session causes primary key errors
@ 2025-04-21 17:00 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
1 sibling, 1 reply; 17+ messages in thread
From: Masahiko Sawada @ 2025-04-21 17:00 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Shawn McCoy <shawn.the.mccoy@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; drewwcallahan@gmail.com <drewwcallahan@gmail.com>; scott@meads.us <scott@meads.us>
On Mon, Apr 21, 2025 at 3:08 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear members,
>
> Thanks for reporting the issue.
>
> > Right. We have wrongly assumed in that commit that the apply worker
> > will exit after an ERROR, but as shown by this case, the ERROR could
> > be silently handled. So, +1, for moving replication origin reset to
> > PG_CATCH in start_apply.
>
> I was an author of original commit, so let me take initiative. When I was working
> for 3f28b2fcac, I could not find path which ERROR is reported but worker can
> survive so that I added replorigin_reset() in apply_error_callback(). The reported
> case, however, the exception could be raised but the insert itself is committed.
> In this case worker can continue working.
>
> Attached patches have proposed changes. I did 1) meson test, 2) workloads provided
> in [1], and 3) manual tests done in original thread [2], and all of them could be
> passed. The version is 2 because of the self-reviewing.
>
> One note is that geterrlevel() is removed for HEAD patch but retained for PG16/PG17.
> The function is exported, and APIs cannot be changed in back branches.
Thank you for the patch. The changes look reasonable to me. Can we add
some regression tests for that?
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Disabled logical replication origin session causes primary key errors
@ 2025-04-22 02:35 Amit Kapila <amit.kapila16@gmail.com>
parent: vignesh C <vignesh21@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Amit Kapila @ 2025-04-22 02:35 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; Masahiko Sawada <sawada.mshk@gmail.com>; Shawn McCoy <shawn.the.mccoy@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; drewwcallahan@gmail.com <drewwcallahan@gmail.com>; scott@meads.us <scott@meads.us>
On Mon, Apr 21, 2025 at 9:47 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Mon, 21 Apr 2025 at 15:38, Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > Dear members,
> >
> > Thanks for reporting the issue.
> >
> > > Right. We have wrongly assumed in that commit that the apply worker
> > > will exit after an ERROR, but as shown by this case, the ERROR could
> > > be silently handled. So, +1, for moving replication origin reset to
> > > PG_CATCH in start_apply.
> >
> > I was an author of original commit, so let me take initiative. When I was working
> > for 3f28b2fcac, I could not find path which ERROR is reported but worker can
> > survive so that I added replorigin_reset() in apply_error_callback(). The reported
> > case, however, the exception could be raised but the insert itself is committed.
> > In this case worker can continue working.
> >
> > Attached patches have proposed changes. I did 1) meson test, 2) workloads provided
> > in [1], and 3) manual tests done in original thread [2], and all of them could be
> > passed. The version is 2 because of the self-reviewing.
> >
> > One note is that geterrlevel() is removed for HEAD patch but retained for PG16/PG17.
> > The function is exported, and APIs cannot be changed in back branches.
> >
> > How do you feel?
>
> I was able to reproduce the issue with the steps suggested by Shawn
> and your patch fixes the issue.
>
Thanks, Vignesh, for testing the fix. Shawn, it would be good if you
could confirm the fix at your end as well.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 17+ messages in thread
* RE: Disabled logical replication origin session causes primary key errors
@ 2025-04-22 09:06 Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2025-04-22 09:06 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Shawn McCoy <shawn.the.mccoy@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; drewwcallahan@gmail.com <drewwcallahan@gmail.com>; scott@meads.us <scott@meads.us>
Dear Sawada-san,
> Thank you for the patch. The changes look reasonable to me. Can we add
> some regression tests for that?
>
Yeah it should be added. I think it is enough to add the first scenario. Done in
100_bugs.pl.
I also ran pgindent/pgperltidy. Because of self-reviewing the version became v4.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Attachments:
[application/octet-stream] v4-HEAD-0001-Fix-oversight-3f28b2f.patch (6.6K, ../../OSCPR01MB149665D61D6C16D0F4AA66B5BF5BB2@OSCPR01MB14966.jpnprd01.prod.outlook.com/2-v4-HEAD-0001-Fix-oversight-3f28b2f.patch)
download | inline diff:
From 4d5e3e89ecefa421ab4e7fa947d07c942c2ba453 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 v4-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 | 77 ++++++++++++++++++++++++
4 files changed, 87 insertions(+), 29 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 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.c
index 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.h
index 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.pl
index b3924ca4b09..7229454d58c 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -498,4 +498,81 @@ is( $node_publisher->psql(
$node_publisher->stop('fast');
$node_subscriber->stop('fast');
+# 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.
+
+$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');
+
+# Insert tuples and confirms replication works well
+$node_publisher->safe_psql('postgres', "INSERT INTO t1 VALUES (1);");
+
+$node_publisher->wait_for_catchup('regress_sub');
+
+# 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'"
+);
+
+# 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;
+});
+
+# Insert a tuple to continue logical replication
+$node_publisher->safe_psql('postgres', "INSERT INTO t1 VALUES (2);");
+
+# Confirms the origin can be advanced
+ok( $node_subscriber->poll_query_until(
+ 'postgres',
+ "SELECT remote_lsn > '$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'",
+ 't')
+ or die "Timed out while waiting for replication origin to be updated");
+
done_testing();
--
2.43.5
[application/octet-stream] v4-PG16-17-0001-Fix-oversight-3f28b2f.patch (5.3K, ../../OSCPR01MB149665D61D6C16D0F4AA66B5BF5BB2@OSCPR01MB14966.jpnprd01.prod.outlook.com/3-v4-PG16-17-0001-Fix-oversight-3f28b2f.patch)
download | inline diff:
From fe9c9aeda2d9882fc6da03cb4b19a1a7374f05db Mon Sep 17 00:00:00 2001
From: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Date: Mon, 21 Apr 2025 17:05:29 +0900
Subject: [PATCH v4-PG16-17] 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 | 77 ++++++++++++++++++++++++
2 files changed, 87 insertions(+), 11 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 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.pl
index cb36ca7b16b..b43811ce7be 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -490,4 +490,81 @@ is( $result, qq(2|f
$node_publisher->stop('fast');
$node_subscriber->stop('fast');
+# 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.
+
+$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');
+
+# Insert tuples and confirms replication works well
+$node_publisher->safe_psql('postgres', "INSERT INTO t1 VALUES (1);");
+
+$node_publisher->wait_for_catchup('regress_sub');
+
+# 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'"
+);
+
+# 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;
+});
+
+# Insert a tuple to continue logical replication
+$node_publisher->safe_psql('postgres', "INSERT INTO t1 VALUES (2);");
+
+# Confirms the origin can be advanced
+ok( $node_subscriber->poll_query_until(
+ 'postgres',
+ "SELECT remote_lsn > '$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'",
+ 't')
+ or die "Timed out while waiting for replication origin to be updated");
+
done_testing();
--
2.43.5
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Disabled logical replication origin session causes primary key errors
@ 2025-04-22 09:36 vignesh C <vignesh21@gmail.com>
parent: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
0 siblings, 1 reply; 17+ messages in thread
From: vignesh C @ 2025-04-22 09:36 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; +Cc: Masahiko Sawada <sawada.mshk@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; Shawn McCoy <shawn.the.mccoy@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; drewwcallahan@gmail.com <drewwcallahan@gmail.com>; scott@meads.us <scott@meads.us>
On Tue, 22 Apr 2025 at 14:36, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Sawada-san,
>
> > Thank you for the patch. The changes look reasonable to me. Can we add
> > some regression tests for that?
> >
>
> Yeah it should be added. I think it is enough to add the first scenario. Done in
> 100_bugs.pl.
> I also ran pgindent/pgperltidy. Because of self-reviewing the version became v4.
Couple of minor suggestions in the test:
1) I felt this is not required for this test as it has been verified many times:
+
+# Insert tuples and confirms replication works well
+$node_publisher->safe_psql('postgres', "INSERT INTO t1 VALUES (1);");
+
+$node_publisher->wait_for_catchup('regress_sub');
2) How about we change:
+# Confirms the origin can be advanced
+ok( $node_subscriber->poll_query_until(
+ 'postgres',
+ "SELECT remote_lsn > '$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'",
+ 't')
+ or die "Timed out while waiting for replication origin to be
updated");
to:
$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_catalog.pg_replication_origin_status os, pg_catalog.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');
In that way, we need not wait on poll_query_until.
Regards,
Vignesh
^ permalink raw reply [nested|flat] 17+ messages in thread
* RE: Disabled logical replication origin session causes primary key errors
@ 2025-04-22 11:00 Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
parent: vignesh C <vignesh21@gmail.com>
0 siblings, 2 replies; 17+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2025-04-22 11:00 UTC (permalink / raw)
To: vignesh C <vignesh21@gmail.com>; +Cc: Masahiko Sawada <sawada.mshk@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; Shawn McCoy <shawn.the.mccoy@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; drewwcallahan@gmail.com <drewwcallahan@gmail.com>; scott@meads.us <scott@meads.us>
Dear Vignesh,
> Couple of minor suggestions in the test:
> 1) I felt this is not required for this test as it has been verified many times:
> +
> +# Insert tuples and confirms replication works well
> +$node_publisher->safe_psql('postgres', "INSERT INTO t1 VALUES (1);");
> +
> +$node_publisher->wait_for_catchup('regress_sub');
I added it to ensure remote_lsn to the valid value, but yes it is not mandatory.
> 2) How about we change:
> +# Confirms the origin can be advanced
> +ok( $node_subscriber->poll_query_until(
> + 'postgres',
> + "SELECT remote_lsn > '$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'",
> + 't')
> + or die "Timed out while waiting for replication origin to be
> updated");
>
> to:
> $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_catalog.pg_replication_origin_status os, pg_catalog.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');
>
> In that way, we need not wait on poll_query_until.
I intentionally used poll_query_until(), but I have no strong opinions.
Modified.
PSA new patches.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Attachments:
[application/octet-stream] v5-PG16-PG17-0001-Fix-oversight-3f28b2f.patch (5.2K, ../../OSCPR01MB14966C633C0C7CDB7CC7D844BF5BB2@OSCPR01MB14966.jpnprd01.prod.outlook.com/2-v5-PG16-PG17-0001-Fix-oversight-3f28b2f.patch)
download | inline diff:
From cc355240f19089a2cc1d971f1d068681952385b6 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 v5-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 | 73 ++++++++++++++++++++++++
2 files changed, 83 insertions(+), 11 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 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.pl
index cb36ca7b16b..bb3bf1277fc 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -490,4 +490,77 @@ is( $result, qq(2|f
$node_publisher->stop('fast');
$node_subscriber->stop('fast');
+# 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.
+
+$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');
+
+# 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'"
+);
+
+# 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;
+});
+
+# 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_catalog.pg_replication_origin_status os, pg_catalog.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');
+
done_testing();
--
2.43.5
[application/octet-stream] v5-HEAD-0001-Fix-oversight-3f28b2f.patch (6.4K, ../../OSCPR01MB14966C633C0C7CDB7CC7D844BF5BB2@OSCPR01MB14966.jpnprd01.prod.outlook.com/3-v5-HEAD-0001-Fix-oversight-3f28b2f.patch)
download | inline diff:
From 50f339b5790ed0a19c1464ff6955a89292613d8c 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 v5-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 | 73 ++++++++++++++++++++++++
4 files changed, 83 insertions(+), 29 deletions(-)
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 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.c
index 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.h
index 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.pl
index b3924ca4b09..137bc375bcc 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -498,4 +498,77 @@ is( $node_publisher->psql(
$node_publisher->stop('fast');
$node_subscriber->stop('fast');
+# 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.
+
+$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');
+
+# 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'"
+);
+
+# 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;
+});
+
+# 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_catalog.pg_replication_origin_status os, pg_catalog.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');
+
done_testing();
--
2.43.5
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Disabled logical replication origin session causes primary key errors
@ 2025-04-22 13:37 vignesh C <vignesh21@gmail.com>
parent: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
1 sibling, 0 replies; 17+ messages in thread
From: vignesh C @ 2025-04-22 13:37 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; +Cc: Masahiko Sawada <sawada.mshk@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; Shawn McCoy <shawn.the.mccoy@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; drewwcallahan@gmail.com <drewwcallahan@gmail.com>; scott@meads.us <scott@meads.us>
On Tue, 22 Apr 2025 at 16:31, Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Vignesh,
>
> > Couple of minor suggestions in the test:
> > 1) I felt this is not required for this test as it has been verified many times:
> > +
> > +# Insert tuples and confirms replication works well
> > +$node_publisher->safe_psql('postgres', "INSERT INTO t1 VALUES (1);");
> > +
> > +$node_publisher->wait_for_catchup('regress_sub');
>
> I added it to ensure remote_lsn to the valid value, but yes it is not mandatory.
>
> > 2) How about we change:
> > +# Confirms the origin can be advanced
> > +ok( $node_subscriber->poll_query_until(
> > + 'postgres',
> > + "SELECT remote_lsn > '$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'",
> > + 't')
> > + or die "Timed out while waiting for replication origin to be
> > updated");
> >
> > to:
> > $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_catalog.pg_replication_origin_status os, pg_catalog.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');
> >
> > In that way, we need not wait on poll_query_until.
>
> I intentionally used poll_query_until(), but I have no strong opinions.
> Modified.
>
> PSA new patches.
Thanks for the updated patch. I’ve reviewed and verified the changes
across the HEAD, PG17, and PG16 branches, and everything is working as
expected. I have no further comments.
Regards,
Vignesh
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Disabled logical replication origin session causes primary key errors
@ 2025-04-22 18:10 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
1 sibling, 1 reply; 17+ messages in thread
From: Masahiko Sawada @ 2025-04-22 18:10 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; +Cc: vignesh C <vignesh21@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; Shawn McCoy <shawn.the.mccoy@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; drewwcallahan@gmail.com <drewwcallahan@gmail.com>; scott@meads.us <scott@meads.us>
On Tue, Apr 22, 2025 at 4:01 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> Dear Vignesh,
>
> > Couple of minor suggestions in the test:
> > 1) I felt this is not required for this test as it has been verified many times:
> > +
> > +# Insert tuples and confirms replication works well
> > +$node_publisher->safe_psql('postgres', "INSERT INTO t1 VALUES (1);");
> > +
> > +$node_publisher->wait_for_catchup('regress_sub');
>
> I added it to ensure remote_lsn to the valid value, but yes it is not mandatory.
>
> > 2) How about we change:
> > +# Confirms the origin can be advanced
> > +ok( $node_subscriber->poll_query_until(
> > + 'postgres',
> > + "SELECT remote_lsn > '$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'",
> > + 't')
> > + or die "Timed out while waiting for replication origin to be
> > updated");
> >
> > to:
> > $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_catalog.pg_replication_origin_status os, pg_catalog.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');
> >
> > In that way, we need not wait on poll_query_until.
>
> I intentionally used poll_query_until(), but I have no strong opinions.
> Modified.
>
> PSA new patches.
Thank you for updating the patch! Here are some 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.
---
+# 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.
And can we execute these queries in one safe_psql() call?
---
+# 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.
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.
---
We might want to stop both the publisher and the subscriber at the end
of the tests.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Disabled logical replication origin session causes primary key errors
@ 2025-04-22 20:10 Shawn McCoy <shawn.the.mccoy@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 0 replies; 17+ messages in thread
From: Shawn McCoy @ 2025-04-22 20:10 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: vignesh C <vignesh21@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; Masahiko Sawada <sawada.mshk@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; drewwcallahan@gmail.com <drewwcallahan@gmail.com>; scott@meads.us <scott@meads.us>
> Thanks, Vignesh, for testing the fix. Shawn, it would be good if you
> could confirm the fix at your end as well.
I tested the patch and confirmed on versions 16.8, 17.4 and HEAD that
the pgbench / functional index version repro is indeed fixed.
Thanks,
Shawn
^ permalink raw reply [nested|flat] 17+ messages in thread
* RE: Disabled logical replication origin session causes primary key errors
@ 2025-04-23 01:41 Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2025-04-23 01:41 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: vignesh C <vignesh21@gmail.com>; Amit Kapila <amit.kapila16@gmail.com>; Shawn McCoy <shawn.the.mccoy@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; drewwcallahan@gmail.com <drewwcallahan@gmail.com>; scott@meads.us <scott@meads.us>
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.c
index 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.pl
index 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.c
index 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.c
index 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.h
index 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.pl
index 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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Disabled logical replication origin session causes primary key errors
@ 2025-04-23 09:01 Amit Kapila <amit.kapila16@gmail.com>
parent: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
0 siblings, 1 reply; 17+ messages in thread
From: Amit Kapila @ 2025-04-23 09:01 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; +Cc: Masahiko Sawada <sawada.mshk@gmail.com>; vignesh C <vignesh21@gmail.com>; Shawn McCoy <shawn.the.mccoy@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; drewwcallahan@gmail.com <drewwcallahan@gmail.com>; scott@meads.us <scott@meads.us>
On Wed, Apr 23, 2025 at 7:11 AM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > +# 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.
>
Pushed the patch after slightly changing the comments.
--
With Regards,
Amit Kapila.
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Disabled logical replication origin session causes primary key errors
@ 2025-04-23 15:58 Masahiko Sawada <sawada.mshk@gmail.com>
parent: Amit Kapila <amit.kapila16@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Masahiko Sawada @ 2025-04-23 15:58 UTC (permalink / raw)
To: Amit Kapila <amit.kapila16@gmail.com>; +Cc: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; vignesh C <vignesh21@gmail.com>; Shawn McCoy <shawn.the.mccoy@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; drewwcallahan@gmail.com <drewwcallahan@gmail.com>; scott@meads.us <scott@meads.us>
On Wed, Apr 23, 2025 at 2:01 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Apr 23, 2025 at 7:11 AM Hayato Kuroda (Fujitsu)
> <kuroda.hayato@fujitsu.com> wrote:
> >
> > > +# 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.
> >
>
> Pushed the patch after slightly changing the comments.
Thank you!
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: Disabled logical replication origin session causes primary key errors
@ 2025-04-24 02:25 Michael Paquier <michael@paquier.xyz>
parent: Masahiko Sawada <sawada.mshk@gmail.com>
0 siblings, 0 replies; 17+ messages in thread
From: Michael Paquier @ 2025-04-24 02:25 UTC (permalink / raw)
To: Masahiko Sawada <sawada.mshk@gmail.com>; +Cc: Amit Kapila <amit.kapila16@gmail.com>; Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>; vignesh C <vignesh21@gmail.com>; Shawn McCoy <shawn.the.mccoy@gmail.com>; pgsql-bugs@lists.postgresql.org <pgsql-bugs@lists.postgresql.org>; drewwcallahan@gmail.com <drewwcallahan@gmail.com>; scott@meads.us <scott@meads.us>
On Wed, Apr 23, 2025 at 08:58:27AM -0700, Masahiko Sawada wrote:
> On Wed, Apr 23, 2025 at 2:01 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>> Pushed the patch after slightly changing the comments.
>
> Thank you!
+1.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../aAmhNzyKEpFVpjgC@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 17+ messages in thread
end of thread, other threads:[~2025-04-24 02:25 UTC | newest]
Thread overview: 17+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-04-18 21:22 Disabled logical replication origin session causes primary key errors Shawn McCoy <shawn.the.mccoy@gmail.com>
2025-04-20 05:53 ` Masahiko Sawada <sawada.mshk@gmail.com>
2025-04-21 05:17 ` Amit Kapila <amit.kapila16@gmail.com>
2025-04-21 10:08 ` Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
2025-04-21 16:17 ` vignesh C <vignesh21@gmail.com>
2025-04-22 02:35 ` Amit Kapila <amit.kapila16@gmail.com>
2025-04-22 20:10 ` Shawn McCoy <shawn.the.mccoy@gmail.com>
2025-04-21 17:00 ` Masahiko Sawada <sawada.mshk@gmail.com>
2025-04-22 09:06 ` Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
2025-04-22 09:36 ` vignesh C <vignesh21@gmail.com>
2025-04-22 11:00 ` Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
2025-04-22 13:37 ` vignesh C <vignesh21@gmail.com>
2025-04-22 18:10 ` Masahiko Sawada <sawada.mshk@gmail.com>
2025-04-23 01:41 ` Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
2025-04-23 09:01 ` Amit Kapila <amit.kapila16@gmail.com>
2025-04-23 15:58 ` Masahiko Sawada <sawada.mshk@gmail.com>
2025-04-24 02:25 ` Michael Paquier <michael@paquier.xyz>
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