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,
