Date: Sat, 14 May 2022 16:53:00 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Andres Freund <andres@anarazel.de>,
	pgsql-hackers@lists.postgresql.org, Noah Misch <noah@leadboat.com>,
	Peter Geoghegan <pg@bowt.ie>, David Rowley <dgrowleyml@gmail.com>,
	Amit Kapila <amit.kapila16@gmail.com>
Subject: Re: Intermittent buildfarm failures on wrasse
Message-ID: <202205141453.skp37viold2x@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="zgl76stv2tlmzxb2"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CAD21AoAsuDJE2u3_yFKr7J3jdim-iZg2oYdUzKZCpkhPJHsxFw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/202205141453.skp37viold2x%40alvherre.pgsql>
Precedence: bulk


--zgl76stv2tlmzxb2
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2022-Apr-20, Masahiko Sawada wrote:

> >     MyProc->statusFlags = (MyProc->statusFlags & ~PROC_XMIN_FLAGS) |
> >                           (proc->statusFlags & PROC_XMIN_FLAGS);
> >
> > Perhaps the latter is more future-proof.

> Copying only xmin-related flags in this way also makes sense to me and
> there is no problem at least for now. A note would be that when we
> introduce a new flag that needs to be copied in the future, we need to
> make sure to add it to PROC_XMIN_FLAGS so it is copied. Otherwise a
> similar issue we fixed by 0f0cfb494004befb0f6e could happen again.

OK, done this way -- patch attached.

Reading the comment I wrote about it, I wonder if flags
PROC_AFFECTS_ALL_HORIZONS and PROC_IN_LOGICAL_DECODING should also be
included.  I think the only reason we don't care at this point is that
walsenders (logical or otherwise) do not engage in snapshot copying.
But if we were to implement usage of parallel workers sharing a common
snapshot to do table sync in parallel, then it ISTM it would be
important to copy at least the latter.  Not sure there are any cases
were we might care about the former.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Every machine is a smoke machine if you operate it wrong enough."
https://twitter.com/libseybieda/status/1541673325781196801

--zgl76stv2tlmzxb2
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v2-0001-Repurpose-PROC_COPYABLE_FLAGS-as-PROC_XMIN_FLAGS.patch"

From 95bd3bf62992987e1d6e078520ff76133248579e Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Sat, 14 May 2022 16:51:23 +0200
Subject: [PATCH v2] Repurpose PROC_COPYABLE_FLAGS as PROC_XMIN_FLAGS

This is a slight, convenient semantics change from what commit
0f0cfb494004 introduced that lets us simplify the coding in the one
place where it is used.
---
 src/backend/storage/ipc/procarray.c | 17 +++++++----------
 src/include/storage/proc.h          |  7 +++----
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index ca22336e35..cd58c5faf0 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -2685,17 +2685,14 @@ ProcArrayInstallRestoredXmin(TransactionId xmin, PGPROC *proc)
 		TransactionIdIsNormal(xid) &&
 		TransactionIdPrecedesOrEquals(xid, xmin))
 	{
-		/* Install xmin */
+		/*
+		 * Install xmin and propagate the statusFlags that affect how the
+		 * value is interpreted by vacuum.
+		 */
 		MyProc->xmin = TransactionXmin = xmin;
-
-		/* walsender cheats by passing proc == MyProc, don't check its flags */
-		if (proc != MyProc)
-		{
-			/* Flags being copied must be valid copy-able flags. */
-			Assert((proc->statusFlags & (~PROC_COPYABLE_FLAGS)) == 0);
-			MyProc->statusFlags = proc->statusFlags;
-			ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
-		}
+		MyProc->statusFlags = (MyProc->statusFlags & ~PROC_XMIN_FLAGS) |
+			(proc->statusFlags & PROC_XMIN_FLAGS);
+		ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
 
 		result = true;
 	}
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 15be21c00a..2579e619eb 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -69,11 +69,10 @@ struct XidCache
 	(PROC_IN_VACUUM | PROC_IN_SAFE_IC | PROC_VACUUM_FOR_WRAPAROUND)
 
 /*
- * Flags that are valid to copy from another proc, the parallel leader
- * process in practice.  Currently, flags that are set during parallel
- * vacuum and parallel index creation are allowed.
+ * Xmin-related flags. Make sure any flags that affect how the process' Xmin
+ * value is interpreted by VACUUM are included here.
  */
-#define		PROC_COPYABLE_FLAGS (PROC_IN_VACUUM | PROC_IN_SAFE_IC)
+#define		PROC_XMIN_FLAGS (PROC_IN_VACUUM | PROC_IN_SAFE_IC)
 
 /*
  * We allow a small number of "weak" relation locks (AccessShareLock,
-- 
2.30.2


--zgl76stv2tlmzxb2--