public inbox for [email protected]
help / color / mirror / Atom feedRE: [BUG] Assert failure in ReorderBufferReturnTXN during logical decoding due to leaked specinsert change
6+ messages / 2 participants
[nested] [flat]
* RE: [BUG] Assert failure in ReorderBufferReturnTXN during logical decoding due to leaked specinsert change
@ 2026-02-26 03:16 Hayato Kuroda (Fujitsu) <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2026-02-26 03:16 UTC (permalink / raw)
To: 'Álvaro Herrera' <[email protected]>; Vishal Prasanna <[email protected]>; +Cc: pgsql-bugs <[email protected]>
Dear Álvaro, Vishal,
> > 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.
To confirm, ReorderBufferResetTXN() seems a static function and proposed patch
does not modify *.h files. So they do not break the ABI and OK to remove, right?
Best regards,
Hayato Kuroda
FUJITSU LIMITED
^ permalink raw reply [nested|flat] 6+ messages in thread
* RE: [BUG] Assert failure in ReorderBufferReturnTXN during logical decoding due to leaked specinsert change
@ 2026-02-26 06:04 Vishal Prasanna <[email protected]>
parent: Hayato Kuroda (Fujitsu) <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Vishal Prasanna @ 2026-02-26 06:04 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: "'Álvaro Herrera'" <[email protected]>; pgsql-bugs <[email protected]>
Dear Hayato, Álvaro,
>> 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.
>
> To confirm, ReorderBufferResetTXN() seems a static function and proposed patch
> does not modify *.h files. So they do not break the ABI and OK to remove, right?
```
File: reorderbuffer.c
2156: /*
2157: * Helper function for ReorderBufferProcessTXN to handle the concurrent
2158: * abort of the streaming transaction. This resets the TXN such that it
2159: * can be used to stream the remaining data of transaction being processed.
2160: * This can happen when the subtransaction is aborted and we still want to
2161: * continue processing the main or other subtransactions data.
2162: */
2163: static void
2164: ReorderBufferResetTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
```
Confirmed, `ReorderBufferResetTXN()` is a static function used only by ReorderBufferProcessTXN().
It is not exposed outside reorderbuffer.c, and the patch does not affect any public exported function.
Regards,
Vishal Prasanna
Zoho Corporation
^ permalink raw reply [nested|flat] 6+ messages in thread
* RE: [BUG] Assert failure in ReorderBufferReturnTXN during logical decoding due to leaked specinsert change
@ 2026-03-03 07:33 Hayato Kuroda (Fujitsu) <[email protected]>
parent: Vishal Prasanna <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2026-03-03 07:33 UTC (permalink / raw)
To: 'Vishal Prasanna' <[email protected]>; +Cc: "'Álvaro Herrera'" <[email protected]>; pgsql-bugs <[email protected]>
Dear Vishal,
Thanks for the patch.
> 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.
Per my understanding, for PG16-, the provided workload cannot cause an assertion
failure because it misses the Assert() in the ReorderBufferReturnTXN(), right?
Adding the line is essential, otherwise the test could pass even without the fix.
Below contains more comments. I mainly checked on PG18, so something might not
be suitable for others.
01.
I think it is better to combine tests actual code patches into one. Because they
would be done when patches are committed.
02.
This issue can happen even on HEAD, but PG18-Fix-specinsert... cannot be applied
atop the branch. Can you create it as well?
03. 100_bugs.pl
Other tests start from the comment like "The bug...", but it does not follow.
Can we update?
04. 100_bugs.pl
Can we just rotate a log instead of starting new instance? It might be faster.
05. 100_bugs.pl for PG
```
# The publication row filter WHERE ((a / 0) > 0) will trigger a division by zero error.
```
I think the comment can be improved like:
Create a publication with the zero-division row filter. It always throws an
ERROR before publishing changes, when the filter is evaluated.
Please see attached my top-up patch for PG18, it addresses comments 03-05.
Best regards,
Hayato Kuroda
FUJITSU LIMITED
Attachments:
[application/octet-stream] kuroda.diffs (1.0K, 2-kuroda.diffs)
download
^ permalink raw reply [nested|flat] 6+ messages in thread
* RE: [BUG] Assert failure in ReorderBufferReturnTXN during logical decoding due to leaked specinsert change
@ 2026-03-08 21:14 Vishal Prasanna <[email protected]>
parent: Hayato Kuroda (Fujitsu) <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Vishal Prasanna @ 2026-03-08 21:14 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: "'Álvaro Herrera'" <[email protected]>; pgsql-bugs <[email protected]>
Hi Hayato,
>> 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.
>
> Per my understanding, for PG16-, the provided workload cannot cause an assertion
> failure because it misses the Assert() in the ReorderBufferReturnTXN(), right?
> Adding the line is essential, otherwise the test could pass even without the fix.
Right. Added `Assert(txn->size == 0)` in `ReorderBufferReturnTXN()` for PG14, 15, 16 patches.
> 01.
> I think it is better to combine tests actual code patches into one. Because they
> would be done when patches are committed.
combined the fix and test into a single patch. since some changes are version specific
(like add assert check for PG14-16, different function names from PG17 -> 18, test case)
separate patches are provided per version. PG15 and PG16 share the same patch.
> 02.
> This issue can happen even on HEAD, but PG18-Fix-specinsert... cannot be applied
> atop the branch. Can you create it as well?
created a separate patch for master.
> 03. 100_bugs.pl
>
> Other tests start from the comment like "The bug...", but it does not follow.
> Can we update?
>
> 04. 100_bugs.pl
>
> Can we just rotate a log instead of starting new instance? It might be faster.
>
> 05. 100_bugs.pl for PG
>
> ```
> # The publication row filter WHERE ((a / 0) > 0) will trigger a division by zero error.
> ```
>
> I think the comment can be improved like:
>
> Create a publication with the zero-division row filter. It always throws an
> ERROR before publishing changes, when the filter is evaluated.
>
> Please see attached my top-up patch for PG18, it addresses comments 03-05.
Applied your suggestions across all versions. Thanks.
Overall:
PG14: Assert check + fix + test (uses 'pub does not exist' error)
PG15-16: Assert check + fix + test (uses row_filter error)
PG17: fix + test (uses row_filter error)
PG18, master: fix + test (uses row_filter error)
Regards,
Vishal Prasanna
Zoho Corporation
Attachments:
[application/octet-stream] master-Fix-specinsert-leak-in-ReorderBufferProcessTXN-error-path.patch (4.2K, 2-master-Fix-specinsert-leak-in-ReorderBufferProcessTXN-error-path.patch)
download | inline diff:
From 3bdb3b1afc0d8c1c647606c954023ad3552d305f Mon Sep 17 00:00:00 2001
From: Vishal Prasanna <[email protected]>
Date: Mon, 9 Mar 2026 01:11:30 +0530
Subject: [PATCH] Fix specinsert leak in ReorderBufferProcessTXN error path
---
.../replication/logical/reorderbuffer.c | 20 ++++-----
src/test/subscription/t/100_bugs.pl | 45 +++++++++++++++++++
2 files changed, 54 insertions(+), 11 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 4c230bcc8e4..c20c2635ab3 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2166,8 +2166,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));
@@ -2175,13 +2174,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.
@@ -2765,6 +2757,13 @@ ReorderBufferProcessTXN(ReorderBuffer *rb, ReorderBufferTXN *txn,
CurrentResourceOwner = cowner;
}
+ /* 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
@@ -2798,8 +2797,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
{
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index a23035e23fe..31dc63ae8c4 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -605,4 +605,49 @@ $node_publisher->safe_psql('postgres', "DROP DATABASE regress_db");
$node_publisher->stop('fast');
+# https://postgr.es/m/19c7623e882.4080fd5426212.311756747309556767%40zohocorp.com
+
+# The bug was that when an ERROR was raised while processing an INSERT ... ON
+# CONFLICT statement, the decoded change misses to be free'd. This can cause an
+# assertion failure if enabled.
+
+$node_publisher->rotate_logfile();
+$node_publisher->start();
+
+# Create a publication with the zero-division row filter. It always throws an
+# ERROR before publishing changes, when the filter is evaluated.
+$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] PG14-Fix-specinsert-leak-in-ReorderBufferProcessTXN-error-path.patch (4.3K, 3-PG14-Fix-specinsert-leak-in-ReorderBufferProcessTXN-error-path.patch)
download | inline diff:
From b90143addb125dc180f23b65936999402591cc7b Mon Sep 17 00:00:00 2001
From: Vishal Prasanna <[email protected]>
Date: Sun, 8 Mar 2026 23:29:27 +0530
Subject: [PATCH] Fix specinsert leak in ReorderBufferProcessTXN error path
---
.../replication/logical/reorderbuffer.c | 23 +++++-----
src/test/subscription/t/100_bugs.pl | 43 ++++++++++++++++++-
2 files changed, 54 insertions(+), 12 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index b708877d965..04d877d67ee 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -474,6 +474,9 @@ ReorderBufferReturnTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
/* Reset the toast hash */
ReorderBufferToastReset(rb, txn);
+ /* All changes must be deallocated */
+ Assert(txn->size == 0);
+
pfree(txn);
}
@@ -2029,8 +2032,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));
@@ -2038,13 +2040,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.
@@ -2589,6 +2584,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
@@ -2615,8 +2617,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
{
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index 235227c7727..81ff79fce46 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,44 @@ like(
$node_publisher->safe_psql('postgres', "DROP DATABASE regress_db");
$node_publisher->stop('fast');
+
+# https://postgr.es/m/19c7623e882.4080fd5426212.311756747309556767%40zohocorp.com
+
+# The bug was that when an ERROR was raised while processing an INSERT ... ON
+# CONFLICT statement, the decoded change misses to be free'd. This can cause an
+# assertion failure if enabled.
+
+$node_publisher->rotate_logfile();
+$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-16-Fix-specinsert-leak-in-ReorderBufferProcessTXN-error-path.patch (4.5K, 4-PG15-16-Fix-specinsert-leak-in-ReorderBufferProcessTXN-error-path.patch)
download | inline diff:
From 6cfbfa82b8a31c365de29395758558e7a50d3f46 Mon Sep 17 00:00:00 2001
From: Vishal Prasanna <[email protected]>
Date: Sun, 8 Mar 2026 23:49:30 +0530
Subject: [PATCH] Fix specinsert leak in ReorderBufferProcessTXN error path
---
.../replication/logical/reorderbuffer.c | 23 +++++-----
src/test/subscription/t/100_bugs.pl | 45 +++++++++++++++++++
2 files changed, 57 insertions(+), 11 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 59efa73930f..2fad1c1cf38 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -478,6 +478,9 @@ ReorderBufferReturnTXN(ReorderBuffer *rb, ReorderBufferTXN *txn)
/* Reset the toast hash */
ReorderBufferToastReset(rb, txn);
+ /* All changes must be deallocated */
+ Assert(txn->size == 0);
+
pfree(txn);
}
@@ -2033,8 +2036,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));
@@ -2042,13 +2044,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.
@@ -2592,6 +2587,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
@@ -2618,8 +2620,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
{
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index 061eb2a32c1..eee297efa2b 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -448,4 +448,49 @@ $node_publisher->safe_psql('postgres', "DROP DATABASE regress_db");
$node_publisher->stop('fast');
+# https://postgr.es/m/19c7623e882.4080fd5426212.311756747309556767%40zohocorp.com
+
+# The bug was that when an ERROR was raised while processing an INSERT ... ON
+# CONFLICT statement, the decoded change misses to be free'd. This can cause an
+# assertion failure if enabled.
+
+$node_publisher->rotate_logfile();
+$node_publisher->start();
+
+# Create a publication with the zero-division row filter. It always throws an
+# ERROR before publishing changes, when the filter is evaluated.
+$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] PG17-Fix-specinsert-leak-in-ReorderBufferProcessTXN-error-path.patch (4.3K, 5-PG17-Fix-specinsert-leak-in-ReorderBufferProcessTXN-error-path.patch)
download | inline diff:
From dd95d7488fc6b6dcb2964504ea5e28d566839060 Mon Sep 17 00:00:00 2001
From: Vishal Prasanna <[email protected]>
Date: Mon, 9 Mar 2026 00:19:29 +0530
Subject: [PATCH] Fix specinsert leak in ReorderBufferProcessTXN error path
---
.../replication/logical/reorderbuffer.c | 20 ++++-----
src/test/subscription/t/100_bugs.pl | 45 +++++++++++++++++++
2 files changed, 54 insertions(+), 11 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 4bd1f7af061..5134abe9359 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
{
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index 17accd11d93..7d804e5ff9c 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -597,4 +597,49 @@ $node_publisher->safe_psql('postgres', "DROP DATABASE regress_db");
$node_publisher->stop('fast');
+# https://postgr.es/m/19c7623e882.4080fd5426212.311756747309556767%40zohocorp.com
+
+# The bug was that when an ERROR was raised while processing an INSERT ... ON
+# CONFLICT statement, the decoded change misses to be free'd. This can cause an
+# assertion failure if enabled.
+
+$node_publisher->rotate_logfile();
+$node_publisher->start();
+
+# Create a publication with the zero-division row filter. It always throws an
+# ERROR before publishing changes, when the filter is evaluated.
+$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 (4.3K, 6-PG18-Fix-specinsert-leak-in-ReorderBufferProcessTXN-error-path.patch)
download | inline diff:
From 64deae15dc57ac7083fd5d1967f0b8a83e648b56 Mon Sep 17 00:00:00 2001
From: Vishal Prasanna <[email protected]>
Date: Mon, 9 Mar 2026 00:46:12 +0530
Subject: [PATCH] Fix specinsert leak in ReorderBufferProcessTXN error path
---
.../replication/logical/reorderbuffer.c | 20 ++++-----
src/test/subscription/t/100_bugs.pl | 45 +++++++++++++++++++
2 files changed, 54 insertions(+), 11 deletions(-)
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.c
index 11139a910b8..57e1ae2441b 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
{
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index 50223054918..df6b2e1296d 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -605,4 +605,49 @@ $node_publisher->safe_psql('postgres', "DROP DATABASE regress_db");
$node_publisher->stop('fast');
+# https://postgr.es/m/19c7623e882.4080fd5426212.311756747309556767%40zohocorp.com
+
+# The bug was that when an ERROR was raised while processing an INSERT ... ON
+# CONFLICT statement, the decoded change misses to be free'd. This can cause an
+# assertion failure if enabled.
+
+$node_publisher->rotate_logfile();
+$node_publisher->start();
+
+# Create a publication with the zero-division row filter. It always throws an
+# ERROR before publishing changes, when the filter is evaluated.
+$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)
^ permalink raw reply [nested|flat] 6+ messages in thread
* RE: [BUG] Assert failure in ReorderBufferReturnTXN during logical decoding due to leaked specinsert change
@ 2026-03-09 04:52 Hayato Kuroda (Fujitsu) <[email protected]>
parent: Vishal Prasanna <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Hayato Kuroda (Fujitsu) @ 2026-03-09 04:52 UTC (permalink / raw)
To: 'Vishal Prasanna' <[email protected]>; +Cc: "'Álvaro Herrera'" <[email protected]>; pgsql-bugs <[email protected]>
Dear Vishal,
Thanks for updating the patch. They look good to me except one minor point.
```
@@ -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;
+ }
```
In PG17-, we seem to use the term "return" to deallocate the change. Should we follow that?
I have no strong opinion for it.
I have no comments anymore. I registered your patch in commitfest [1]: and marked as
"Ready for Committer" not to forget. If needed you can register your name as author.
[1]: https://commitfest.postgresql.org/patch/6569/
Best regards,
Hayato Kuroda
FUJITSU LIMITED
^ permalink raw reply [nested|flat] 6+ messages in thread
* RE: [BUG] Assert failure in ReorderBufferReturnTXN during logical decoding due to leaked specinsert change
@ 2026-03-09 09:39 Vishal Prasanna <[email protected]>
parent: Hayato Kuroda (Fujitsu) <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Vishal Prasanna @ 2026-03-09 09:39 UTC (permalink / raw)
To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: "'Álvaro Herrera'" <[email protected]>; pgsql-bugs <[email protected]>
Hi Hayato,
> ```
> @@ -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;
> + }
> ```
>
> In PG17-, we seem to use the term "return" to deallocate the change. Should we follow that?
> I have no strong opinion for it.
Internally, `ReorderBufferReturnChange()` frees the change, which is why comment uses "Free".
Either term is fine for me.
Thanks for the review and for registering the patch.
Regards,
Vishal Prasanna
Zoho Corporation
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2026-03-09 09:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-02-26 03:16 RE: [BUG] Assert failure in ReorderBufferReturnTXN during logical decoding due to leaked specinsert change Hayato Kuroda (Fujitsu) <[email protected]>
2026-02-26 06:04 ` Vishal Prasanna <[email protected]>
2026-03-03 07:33 ` Hayato Kuroda (Fujitsu) <[email protected]>
2026-03-08 21:14 ` Vishal Prasanna <[email protected]>
2026-03-09 04:52 ` Hayato Kuroda (Fujitsu) <[email protected]>
2026-03-09 09:39 ` Vishal Prasanna <[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