Re: HOT chain validation in verify_heapam()

On Wed, Sep 7, 2022 at 12:11 AM Robert Haas <robertmhaas@gmail.com> wrote:

>
> I think the check should be written like this:
>
> !TransactionIdEquals(pred_xmin, curr_xmin) &&
> !TransctionIdDidCommit(pred_xmin)
>
> The TransactionIdEquals check should be done first for the reason you
> state: it's cheaper.
>
> I think that we shouldn't be using TransactionIdDidAbort() at all,
> because it can sometimes return false even when the transaction
> actually did abort. See test_lockmode_for_conflict() and
> TransactionIdIsInProgress() for examples of logic that copes with
> this. What's happening here is that TransactionIdDidAbort doesn't ever
> get called if the system crashes while a transaction is running. So we
> can use TransactionIdDidAbort() only as an optimization: if it returns
> true, the transaction is definitely aborted, but if it returns false,
> we have to check whether it's still running. If not, it aborted
> anyway.
>
> TransactionIdDidCommit() does not have the same issue. A transaction
> can abort without updating CLOG if the system crashes, but it can
> never commit without updating CLOG. If the transaction didn't commit,
> then it is either aborted or still in progress (and we don't care
> which, because neither is an error here).
>
> As to whether the existing formulation of the test has an error
> condition, you're generally right that we should test
> TransactionIdIsInProgress() before TransactionIdDidCommit/Abort,
> because we during commit or abort, we first set the status in CLOG -
> which is queried by TransactionIdDidCommit/Abort - and only afterward
> update the procarray - which is queried by TransactionIdIsInProgress.
> So normally TransactionIdIsInProgress should be checked first, and
> TransactionIdDidCommit/Abort should only be checked if it returns
> false, at which point we know that the return values of the latter
> calls can't ever change. Possibly there is an argument for including
> the TransactionIdInProgress check here too:
>
> !TransactionIdEquals(pred_xmin, curr_xmin) &&
> (TransactionIdIsInProgress(pred_xmin) ||
> !TransctionIdDidCommit(pred_xmin))
>
> ...but I don't think it could change the answer. Most places that
> check TransactionIdIsInProgress() first are concerned with MVCC
> semantics, and here we are not. I think the only effects of including
> or excluding the TransactionIdIsInProgress() test are (1) performance,
> in that searching the procarray might avoid expense if it's cheaper
> than searching clog, or add expense if the reverse is true and (2)
> slightly changing the time at which we're first able to detect this
> form of corruption. I am inclined to prefer the simpler form of the
> test without TransactionIdIsInProgress() unless someone can say why we
> shouldn't go that route.
>
> Done, updated in the v3 patch.


-- 
Regards,
Himanshu Upadhyaya
EnterpriseDB: http://www.enterprisedb.com