Re: Possible dereference null return (src/backend/replication/logical/reorderbuffer.c)

At Wed, 10 Feb 2021 20:12:38 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
> Hi,
>
> Per Coverity.
>
> If xid is a subtransaction, the setup of base snapshot on the top-level
> transaction,
> can be not optional, otherwise a Dereference null return value
> (NULL_RETURNS) can be raised.
>
> Patch suggestion to fix this.
>
> diff --git a/src/backend/replication/logical/reorderbuffer.c
> b/src/backend/replication/logical/reorderbuffer.c
> index 5a62ab8bbc..3c6a81f716 100644
> --- a/src/backend/replication/logical/reorderbuffer.c
> +++ b/src/backend/replication/logical/reorderbuffer.c
> @@ -2993,8 +2993,8 @@ ReorderBufferSetBaseSnapshot(ReorderBuffer *rb,
> TransactionId xid,
>   */
>   txn = ReorderBufferTXNByXid(rb, xid, true, &is_new, lsn, true);
>   if (rbtxn_is_known_subxact(txn))
> - txn = ReorderBufferTXNByXid(rb, txn->toplevel_xid, false,
> - NULL, InvalidXLogRecPtr, false);
> + txn = ReorderBufferTXNByXid(rb, txn->toplevel_xid, true,
> + NULL, InvalidXLogRecPtr, true);
>   Assert(txn->base_snapshot == NULL);

If the return from the first call is a subtransaction, the second call
always obtain the top transaction.  If the top transaction actualy did
not exist, it's rather the correct behavior to cause SEGV, than
creating a bogus rbtxn. THus it is wrong to set create=true and
create_as_top=true.  We could change the assertion like Assert (txn &&
txn->base_snapshot) to make things clearer.

It does not make sense to generate a SEGV on purpose and 

assertion would not solve why this happens in a production environment.
Better to report an error if the second call fails.