Re: Fix possible dereference null pointer (src/backend/replication/logical/reorderbuffer.c)

On Sat, Apr 13, 2024 at 12:46 PM Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
> I don't much like adding extra runtime checks for "can't happen"
> scenarios either. Assertions would be more clear, but in this case the
> code would just segfault trying to dereference the NULL pointer, which
> is fine for a "can't happen" scenario.
>

Isn't the existing assertion (Assert(!create || txn != NULL);) in
ReorderBufferTXNByXid() sufficient to handle this case?

> Looking closer, when we identify an XID as a subtransaction, we:
> - assign toplevel_xid,
> - set RBTXN_IS_SUBXACT, and
> - assign toptxn pointer.
>
> ISTM the 'toplevel_xid' and RBTXN_IS_SUBXACT are redundant.
> 'toplevel_xid' is only used in those two calls that do
> ReorderBufferTXNByXid(rb, txn->toplevel_xid,...), and you could replace
> those by following the 'toptxn' pointer directly. And RBTXN_IS_SUBXACT
> is redundant with (toptxn != NULL). So here's a patch to remove
> 'toplevel_xid' and RBTXN_IS_SUBXACT altogether.
>

Good idea. I don't see a problem with this idea.

@@ -1135,8 +1133,6 @@ static void
ReorderBufferTransferSnapToParent(ReorderBufferTXN *txn,
ReorderBufferTXN *subtxn)
{
- Assert(subtxn->toplevel_xid == txn->xid);

Is there a benefit in converting this assertion using toptxn?

--
With Regards,
Amit Kapila.