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

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