public inbox for [email protected]
help / color / mirror / Atom feedRE: [BUG] Assert failure in ReorderBufferReturnTXN during logical decoding due to leaked specinsert change
2+ messages / 2 participants
[nested] [flat]
* RE: [BUG] Assert failure in ReorderBufferReturnTXN during logical decoding due to leaked specinsert change
@ 2026-02-25 16:32 Vishal Prasanna <[email protected]>
2026-02-25 16:57 ` Re: [BUG] Assert failure in ReorderBufferReturnTXN during logical decoding due to leaked specinsert change Álvaro Herrera <[email protected]>
0 siblings, 1 reply; 2+ messages in thread
From: Vishal Prasanna @ 2026-02-25 16:32 UTC (permalink / raw)
To: kurodahayato <[email protected]>; +Cc: pgsql-bugs <[email protected]>
Hi,
Thanks for the response.
> Anyway, I agreed that the leak could happen across all versions.
> How about fixing the issue as shown below? Thought?
> 1. Add an Assert() for PG16-.
Yeah, Adding `Assert(txn->size == 0)` to PG 14, 15, 16 would ensure everything
is cleaned up before freeing the `ReorderBufferTXN`. Consider adding this assert as well.
> 2. Make sure specinsert is released for all supported versions.
In PG 14 to 17, `ReorderBufferReturnChange()` is used to free the `ReorderBufferChange`.
In PG 18, this function was renamed to `ReorderBufferFreeChange()`.
Refer:
PG 14, 15, 16, 17: PG14-17-Fix-specinsert-leak-in-ReorderBufferProcessTXN-error-path.patch
PG 18: PG18-Fix-specinsert-leak-in-ReorderBufferProcessTXN-error-path.patch
The above changes cover the `specinsert` cleanup in the error path for all supported versions.
> 3. Consider a workload that could reproduce on PG18 and master.
> 3-1. If found, the test code can be added for all versions.
> 3-2. Otherwise, the test code can be added for PG17-.
Found a workload that can reproduce the issue across all supported versions, except PG 14.
For PG 14, since row_filter is not supported, so we can go with the 'publication does not exist' error instead.
Refer: PG14-Test-specinsert-cleanup-in-ReorderBufferProcessTXN-error-path.patch
For PG 15 - 18, using the row_filter option we can cause an error in the logical decoder.
Refer: PG15-18-Test-specinsert-cleanup-in-ReorderBufferProcessTXN-error-path.patch
>> Additional Suggestion:
>> Currently in `PG_CATCH` block, `specinsert` is only freed in the `ERRCODE_TRANSACTION_ROLLBACK` branch
>> for streaming or prepared transactions, via `ReorderBufferResetTXN()` at line 2691.
>> Would it make sense to move the freeing of `specinsert` before the if/else branch,
>> so that it is always freed regardless of the error path? This would avoid duplication and ensure
>> that `specinsert` is always cleaned up.
> It looks OK for me. In this case an argument should be reduced from
> ReorderBufferResetTXN(), right? It is harmless because the function is a static one.
Yes, the `specinsert` is no longer needed in `ReorderBufferResetTXN()`. Updated the patch where `specinsert` cleanup
is now handled in the `PG_CATCH()` block of `ReorderBufferProcessTXN()`, so it is always freed before the if/else branch.
Refer:
PG14-17-Fix-specinsert-leak-in-ReorderBufferProcessTXN-error-path.patch
PG18-Fix-specinsert-leak-in-ReorderBufferProcessTXN-error-path.patch
Regards,
Vishal Prasanna
Zoho Corporation
Attachments:
[application/octet-stream] PG14-17-Fix-specinsert-leak-in-ReorderBufferProcessTXN-error-path.patch (2.2K, 3-PG14-17-Fix-specinsert-leak-in-ReorderBufferProcessTXN-error-path.patch)
download | inline diff:
From 66f3e22b29a61a1817e5482b335fb387c873955b Mon Sep 17 00:00:00 2001
From: Vishal Prasanna <[email protected]>
Date: Wed, 25 Feb 2026 15:52:22 +0530
Subject: [PATCH] Fix specinsert leak in ReorderBufferProcessTXN error path
---
.../replication/logical/reorderbuffer.c | 20 +++++++++----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 4bd1f7af061..51ac4d04f25 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2081,8 +2081,7 @@ static void
ReorderBufferResetTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
Snapshot snapshot_now,
CommandId command_id,
- XLogRecPtr last_lsn,
- ReorderBufferChange *specinsert)
+ XLogRecPtr last_lsn)
{
/* Discard the changes that we just streamed */
ReorderBufferTruncateTXN(rb, txn, rbtxn_prepared(txn));
@@ -2090,13 +2089,6 @@ ReorderBufferResetTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
/* Free all resources allocated for toast reconstruction */
ReorderBufferToastReset(rb, txn);
- /* Return the spec insert change if it is not NULL */
- if (specinsert != NULL)
- {
- ReorderBufferReturnChange(rb, specinsert, true);
- specinsert = NULL;
- }
-
/*
* For the streaming case, stop the stream and remember the command ID and
* snapshot for the streaming run.
@@ -2663,6 +2655,13 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
if (using_subtxn)
RollbackAndReleaseCurrentSubTransaction();
+ /* Free the specinsert change before freeing the ReorderBufferTXN */
+ if (specinsert != NULL)
+ {
+ ReorderBufferReturnChange(rb, specinsert, true);
+ specinsert = NULL;
+ }
+
/*
* The error code ERRCODE_TRANSACTION_ROLLBACK indicates a concurrent
* abort of the (sub)transaction we are streaming or preparing. We
@@ -2689,8 +2688,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
/* Reset the TXN so that it is allowed to stream remaining data. */
ReorderBufferResetTXN(rb, txn, snapshot_now,
- command_id, prev_lsn,
- specinsert);
+ command_id, prev_lsn);
}
else
{
--
2.50.1 (Apple Git-155)
[application/octet-stream] PG14-Test-specinsert-cleanup-in-ReorderBufferProcessTXN-error-path.patch (2.1K, 4-PG14-Test-specinsert-cleanup-in-ReorderBufferProcessTXN-error-path.patch)
download | inline diff:
From 3b781f72bdd8c6a897b7c6b1265c1ad41581f21a Mon Sep 17 00:00:00 2001
From: Vishal Prasanna <[email protected]>
Date: Wed, 25 Feb 2026 20:39:54 +0530
Subject: [PATCH 1/2] Test specinsert cleanup in ReorderBufferProcessTXN error path
---
src/test/subscription/t/100_bugs.pl | 40 ++++++++++++++++++++++++++++-
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index 235227c7727..c195fd33bce 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -6,7 +6,7 @@ use strict;
use warnings;
use PostgresNode;
use TestLib;
-use Test::More tests => 11;
+use Test::More tests => 12;
# Bug #15114
@@ -392,3 +392,41 @@ like(
$node_publisher->safe_psql('postgres', "DROP DATABASE regress_db");
$node_publisher->stop('fast');
+
+# Ensure logical decoder doesn't crash if error occurs
+# while processing an INSERT ... ON CONFLICT statement
+$node_publisher = get_new_node('logical_decoder');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+$node_publisher->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE tab_upsert (a INT PRIMARY KEY, b INT);
+ SELECT * FROM pg_create_logical_replication_slot('upsert_slot', 'pgoutput');
+ INSERT INTO tab_upsert (a, b) VALUES (1, 1)
+ ON CONFLICT(a) DO UPDATE SET b = excluded.b;
+));
+
+# Decode the changes without a publication and
+# verify that the logical decoder doesn't crash.
+($ret, $stdout, $stderr) = $node_publisher->psql(
+ 'postgres', qq(
+ SELECT *
+ FROM pg_logical_slot_peek_binary_changes(
+ 'upsert_slot',
+ NULL,
+ NULL,
+ 'proto_version', '1',
+ 'publication_names', 'pub_that_does_not_exist'
+ );
+));
+
+ok( $stderr =~ qr/publication "pub_that_does_not_exist" does not exist/,
+ 'peek logical changes with non-existent publication throws error'
+);
+
+# Clean up
+$node_publisher->safe_psql('postgres', "SELECT pg_drop_replication_slot('upsert_slot')");
+$node_publisher->safe_psql('postgres', "DROP TABLE tab_upsert");
+
+$node_publisher->stop('fast');
--
2.50.1 (Apple Git-155)
[application/octet-stream] PG15-18-Test-specinsert-cleanup-in-ReorderBufferProcessTXN-error-path.patch (2.2K, 5-PG15-18-Test-specinsert-cleanup-in-ReorderBufferProcessTXN-error-path.patch)
download | inline diff:
From bb4d6db02d79828cc7665690a9272b71970ead65 Mon Sep 17 00:00:00 2001
From: Vishal Prasanna <[email protected]>
Date: Wed, 25 Feb 2026 15:17:25 +0530
Subject: [PATCH 1/2] Test specinsert cleanup in ReorderBufferProcessTXN error path
---
src/test/subscription/t/100_bugs.pl | 41 +++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index 50223054918..4344eb581da 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -605,4 +605,45 @@ $node_publisher->safe_psql('postgres', "DROP DATABASE regress_db");
$node_publisher->stop('fast');
+# Ensure logical decoder doesn't crash if error occurs
+# while processing an INSERT ... ON CONFLICT statement.
+$node_publisher = PostgreSQL::Test::Cluster->new('logical_decoder');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+# The publication row filter WHERE ((a / 0) > 0) will trigger a division by zero error.
+$node_publisher->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE tab_upsert (a INT PRIMARY KEY, b INT);
+ CREATE PUBLICATION pub_rowfilter_error FOR TABLE tab_upsert WHERE ((a / 0) > 0);
+ SELECT * FROM pg_create_logical_replication_slot('upsert_slot', 'pgoutput');
+ INSERT INTO tab_upsert (a, b) VALUES (1, 1)
+ ON CONFLICT(a) DO UPDATE SET b = excluded.b;
+));
+
+# Decode the changes with a publication whose row filter causes a
+# division by zero error, and verify that the logical decoder doesn't crash.
+($ret, $stdout, $stderr) = $node_publisher->psql(
+ 'postgres', qq(
+ SELECT *
+ FROM pg_logical_slot_peek_binary_changes(
+ 'upsert_slot',
+ NULL,
+ NULL,
+ 'proto_version', '1',
+ 'publication_names', 'pub_rowfilter_error'
+ );
+));
+
+ok( $stderr =~ qr/division by zero/,
+ 'peek logical changes with row filter causing division by zero throws error'
+);
+
+# Clean up
+$node_publisher->safe_psql('postgres', "SELECT pg_drop_replication_slot('upsert_slot')");
+$node_publisher->safe_psql('postgres', "DROP PUBLICATION pub_rowfilter_error");
+$node_publisher->safe_psql('postgres', "DROP TABLE tab_upsert");
+
+$node_publisher->stop('fast');
+
done_testing();
--
2.50.1 (Apple Git-155)
[application/octet-stream] PG18-Fix-specinsert-leak-in-ReorderBufferProcessTXN-error-path.patch (2.2K, 6-PG18-Fix-specinsert-leak-in-ReorderBufferProcessTXN-error-path.patch)
download | inline diff:
From 3aaefccb77c0a47926ab8567ec28682c3ed136a0 Mon Sep 17 00:00:00 2001
From: Vishal Prasanna <[email protected]>
Date: Wed, 25 Feb 2026 15:29:50 +0530
Subject: [PATCH 2/2] Fix specinsert leak in ReorderBufferProcessTXN error path
---
.../replication/logical/reorderbuffer.c | 20 +++++++++----------
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 11139a910b8..5dce51f2ee4 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2164,8 +2164,7 @@ static void
ReorderBufferResetTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
Snapshot snapshot_now,
CommandId command_id,
- XLogRecPtr last_lsn,
- ReorderBufferChange *specinsert)
+ XLogRecPtr last_lsn)
{
/* Discard the changes that we just streamed */
ReorderBufferTruncateTXN(rb, txn, rbtxn_is_prepared(txn));
@@ -2173,13 +2172,6 @@ ReorderBufferResetTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
/* Free all resources allocated for toast reconstruction */
ReorderBufferToastReset(rb, txn);
- /* Return the spec insert change if it is not NULL */
- if (specinsert != NULL)
- {
- ReorderBufferFreeChange(rb, specinsert, true);
- specinsert = NULL;
- }
-
/*
* For the streaming case, stop the stream and remember the command ID and
* snapshot for the streaming run.
@@ -2753,6 +2745,13 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
if (using_subtxn)
RollbackAndReleaseCurrentSubTransaction();
+ /* Free the specinsert change before freeing the ReorderBufferTXN */
+ if (specinsert != NULL)
+ {
+ ReorderBufferFreeChange(rb, specinsert, true);
+ specinsert = NULL;
+ }
+
/*
* The error code ERRCODE_TRANSACTION_ROLLBACK indicates a concurrent
* abort of the (sub)transaction we are streaming or preparing. We
@@ -2786,8 +2785,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
/* Reset the TXN so that it is allowed to stream remaining data. */
ReorderBufferResetTXN(rb, txn, snapshot_now,
- command_id, prev_lsn,
- specinsert);
+ command_id, prev_lsn);
}
else
{
--
2.50.1 (Apple Git-155)
^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: [BUG] Assert failure in ReorderBufferReturnTXN during logical decoding due to leaked specinsert change
2026-02-25 16:32 RE: [BUG] Assert failure in ReorderBufferReturnTXN during logical decoding due to leaked specinsert change Vishal Prasanna <[email protected]>
@ 2026-02-25 16:57 ` Álvaro Herrera <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: Álvaro Herrera @ 2026-02-25 16:57 UTC (permalink / raw)
To: Vishal Prasanna <[email protected]>; +Cc: kurodahayato <[email protected]>; pgsql-bugs <[email protected]>
On 2026-Feb-25, Vishal Prasanna wrote:
> > It looks OK for me. In this case an argument should be reduced from
> > ReorderBufferResetTXN(), right? It is harmless because the function is a static one.
>
> Yes, the `specinsert` is no longer needed in
> `ReorderBufferResetTXN()`. Updated the patch where `specinsert`
> cleanup is now handled in the `PG_CATCH()` block of
> `ReorderBufferProcessTXN()`, so it is always freed before the if/else
> branch.
Please don't do this. Changing the argument list of an exported
function is an ABI break. That's an OK change to do in branch master
(to keep the interface clean), but for released branches it is not
welcome, because it causes problems for users that have extensions that
call the function and were compiled with its older definition.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2026-02-25 16:57 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-02-25 16:32 RE: [BUG] Assert failure in ReorderBufferReturnTXN during logical decoding due to leaked specinsert change Vishal Prasanna <[email protected]>
2026-02-25 16:57 ` Álvaro Herrera <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox