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

Dear Vishal,

Thanks for reporting the issue.

Your analysis looks correct. ReorderBufferProcessTXN() rarely fails, thus the
issue may be missed. Looks like the code path you pointed out exists from the
beginning, but I found your workload could reproduce only on PG17.

Regarding the PG16 and earlier versions, the problematic Assert() in
ReorderBufferReturnTXN() does not exist. The commit dbed2e3 added it, and
it was backpatched till PG17. We deallocate ReorderBufferTXN even if the size
is not zero for such versions.

Regarding PG18 and master, the provided workload did not cause an ERROR.
7c99dc5 changed the behavior to report a WARNING instead.

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-.
2. Make sure specinsert is released 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-.

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

Best regards,
Hayato Kuroda
FUJITSU LIMITED