Subject: RE: [BUG] Assert failure in ReorderBufferReturnTXN during logical decoding due to leaked specinsert change

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Thu, 26 Feb 2026 03:16:32 +0000

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

---

From: Vishal Prasanna <[email-masked]>
Date: Thu, 26 Feb 2026 11:34:02 +0530

Dear Hayato, Álvaro,

> 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

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 3 Mar 2026 07:33:13 +0000

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

---

From: Vishal Prasanna <[email-masked]>
Date: Mon, 09 Mar 2026 02:44:03 +0530

Hi Hayato,

> 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.

> 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.

> atop the branch. Can you create it as well?

created a separate patch for master.

> 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

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Mon, 9 Mar 2026 04:52:56 +0000

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

---

From: Vishal Prasanna <[email-masked]>
Date: Mon, 09 Mar 2026 15:09:51 +0530

Hi Hayato,

 > 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