Removing redundant check for transaction in progress in check_safe_enum_use

Started by Zhihong Yuover 4 years ago2 messages

zyu@yugabyte.com

over 4 years ago

1 attachment(s)

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.

Please see the simple patch for removing the redundant check.

Thanks

Attachments:

txn-in-progress-enum.patchapplication/octet-stream; name=txn-in-progress-enum.patchDownload

diff --git a/src/backend/utils/adt/enum.c b/src/backend/utils/adt/enum.c
index 0d892132a8..621d470c08 100644
--- a/src/backend/utils/adt/enum.c
+++ b/src/backend/utils/adt/enum.c
@@ -77,8 +77,7 @@ check_safe_enum_use(HeapTuple enumval_tup)
 	 * into syscache; but just in case not, let's check the xmin directly.
 	 */
 	xmin = HeapTupleHeaderGetXmin(enumval_tup->t_data);
-	if (!TransactionIdIsInProgress(xmin) &&
-		TransactionIdDidCommit(xmin))
+	if (TransactionIdDidCommit(xmin))
 		return;
 
 	/*

Matthias van de Meent

boekewurm+postgres@gmail.com

over 4 years ago

In reply to: Zhihong Yu (#1)

Re: Removing redundant check for transaction in progress in check_safe_enum_use

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