On Sun, 4 Jul 2021, 03:40 Zhihong Yu, <z...@yugabyte.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
