Re: new heapcheck contrib module

> On Oct 22, 2020, at 1:23 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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 believe fxid 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