Re: new heapcheck contrib module

> On Oct 22, 2020, at 1:23 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> ... btw, having now looked more closely at get_xid_status(), I wonder
> how come there aren't more compilers bitching about it, because it
> is very very obviously broken.  In particular, the case of
> requesting status for an xid that is BootstrapTransactionId or
> FrozenTransactionId *will* fall through to perform
> FullTransactionIdPrecedesOrEquals with an uninitialized fxid.
>
> The fact that most compilers seem to fail to notice that is quite scary.
> I suppose it has something to do with FullTransactionId being a struct,
> which makes me wonder if that choice was quite as wise as we thought.
>
> Meanwhile, so far as this code goes, I wonder why you don't just change it
> to always set that value, ie
>
>     XidBoundsViolation result;
>     FullTransactionId fxid;
>     FullTransactionId clog_horizon;
>
> +    fxid = FullTransactionIdFromXidAndCtx(xid, ctx);
> +
>     /* Quick check for special xids */
>     if (!TransactionIdIsValid(xid))
>         result = XID_INVALID;
>     else if (xid == BootstrapTransactionId || xid == FrozenTransactionId)
>         result = XID_BOUNDS_OK;
>     else
>     {
>         /* Check if the xid is within bounds */
> -        fxid = FullTransactionIdFromXidAndCtx(xid, ctx);
>         if (!fxid_in_cached_range(fxid, ctx))
>         {

Yeah, I reached the same conclusion before submitting the fix upthread.  I structured it a bit differently, but I
believefxid will now always get set before being used, though sometimes the function returns before doing either. 

I had the same thought about compilers not catching that, too.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company