public inbox for [email protected]
help / color / mirror / Atom feedFrom: Vishal Prasanna <[email protected]>
To: kurodahayato <[email protected]>
Cc: pgsql-bugs <[email protected]>
Subject: RE: [BUG] Assert failure in ReorderBufferReturnTXN during logical decoding due to leaked specinsert change
Date: Wed, 25 Feb 2026 22:02:54 +0530
Message-ID: <[email protected]> (raw)
In-Reply-To: <OS9PR01MB121492243C9366FBDF8334947F577A@OS9PR01MB12149.jpnprd01.prod.outlook.com>
References: <[email protected]>
<OS9PR01MB121492243C9366FBDF8334947F577A@OS9PR01MB12149.jpnprd01.prod.outlook.com>
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)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: RE: [BUG] Assert failure in ReorderBufferReturnTXN during logical decoding due to leaked specinsert change
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox