Horton

Re: HOT chain validation in verify_heapam()

Aleksander Alekseev <aleksander@timescale.com>

View thread View on postgr.es
From: Aleksander Alekseev <aleksander@timescale.com>
To: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Cc: Himanshu Upadhyaya <upadhyaya.himanshu@gmail.com>, Robert Haas <robertmhaas@gmail.com>, Andres Freund <andres@anarazel.de>, Peter Geoghegan <pg@bowt.ie>, Andrew Dunstan <andrew@dunslane.net>, Mark Dilger <mark.dilger@enterprisedb.com>
Date: 2022-09-06T13:38:01Z
Lists: pgsql-hackers 
Hi hackers,

> Please correct me if I'm wrong, but don't we have a race condition here:
>
> ```
> +            if ((TransactionIdDidAbort(pred_xmin) ||
> TransactionIdIsInProgress(pred_xmin))
> +                && !TransactionIdEquals(pred_xmin, curr_xmin))
>              {
> ```
>
> The scenario that concerns me is the following:
>
> 1. TransactionIdDidAbort(pred_xmin) returns false
> 2. The transaction aborts
> 3. TransactionIdIsInProgress(pred_xmin) returns false
> 4. (false || false) gives us false. An error is reported, although
> actually the condition should have been true.

It looks like I had a slight brain fade here.

In order to report a false error either TransactionIdDidAbort() or
TransactionIdIsInProgress() should return true and
TransactionIdEquals() should be false. So actually under rare
conditions the error will NOT be reported while it should. Other than
that we seem to be safe from the concurrency perspective, unless I'm
missing something again.

Personally I don't have a strong opinion on whether we should bother
about this scenario. Probably an explicit comment will not hurt.

Also I suggest checking TransactionIdEquals() first though since it's cheaper.

-- 
Best regards,
Aleksander Alekseev

Commits

  1. amcheck: Generalize one of the recently-added update chain checks.

  2. amcheck: Tighten up validation of redirect line pointers.

  3. amcheck: Fix verify_heapam for tuples where xmin or xmax is 0.

  4. amcheck: Fix a few bugs in new update chain validation.

  5. Fix new test case to work on (some?) big-endian architectures.

  6. Don't test HEAP_XMAX_INVALID when freezing xmax.