Re: Removing redundant check for transaction in progress in check_safe_enum_use

On Sun, 4 Jul 2021, 03:40 Zhihong Yu, <zyu(at)yugabyte(dot)com> wrote:
>
> Hi,
> I was looking at :
> Relax transactional restrictions on ALTER TYPE ... ADD VALUE (redux).
>
> In check_safe_enum_use():
>
> + if (!TransactionIdIsInProgress(xmin) &&
> + TransactionIdDidCommit(xmin))
> + return;
>
> Since the condition would be true only when TransactionIdDidCommit() returns true, I think the call to TransactionIdIsInProgress is not needed.
> If transaction for xmin is committed, the transaction cannot be in progress at the same time.

I'm not sure that removing the !TransactionIdIsInProgress-check is
correct. The comment in heapam_visibility.c:13 explains that we need
to check TransactionIdIsInProgress before TransactionIdDidCommit in
non-MVCC snapshots, and I'm fairly certain that check_safe_enum_use()
is not guaranteed to run only in MVCC snapshots (at least its
documentation does not warn against non-MVCC snapshots).

Kind regards,

Matthias van de Meent