From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: pgsql-hackers@lists.postgresql.org, Noah Misch <noah@leadboat.com>,
        Peter Geoghegan <pg@bowt.ie>, David Rowley <dgrowleyml@gmail.com>
Subject: Re: Intermittent buildfarm failures on wrasse
In-reply-to: <20220415165850.rh7byj7ep5jmzuzb@alap3.anarazel.de>
References: 
 <CAH2-WznXXkkfE_y9ieaHkEQj31HeMAqJBtJPmDY+EpyVg1QywQ@mail.gmail.com>
 <1570894.1649986347@sss.pgh.pa.us>
 <20220415015249.kfsqxujrf2bn2wz6@alap3.anarazel.de>
 <20220415024515.GA862547@rfd.leadboat.com>
 <20220415051205.6abhlilh6fnjgohr@alap3.anarazel.de>
 <20220415052125.GD862547@rfd.leadboat.com> <1637671.1650032132@sss.pgh.pa.us>
 <1643686.1650036220@sss.pgh.pa.us>
 <A11045E8-18D2-4BBB-8CF1-80DC6920ED73@anarazel.de>
 <1650516.1650040612@sss.pgh.pa.us>
 <20220415165850.rh7byj7ep5jmzuzb@alap3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Fri, 15 Apr 2022 09:58:50 -0700"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1737780.1650046441.0@sss.pgh.pa.us>
Date: Fri, 15 Apr 2022 14:14:47 -0400
Message-ID: <1737834.1650046487@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1737834.1650046487%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1737780.1650046441.1@sss.pgh.pa.us>

Andres Freund <andres@anarazel.de> writes:
> On 2022-04-15 12:36:52 -0400, Tom Lane wrote:
>> Yeah, I was also thinking about a flag in PGPROC being a more reliable
>> way to do this.  Is there anything besides walsenders that should set
>> that flag?

> Not that I can think of. It's only because of hs_feedback that we need
> to.  I guess it's possible that somebody build some extension that needs
> something similar, but then they'd need to set that flag...

Here's a WIP patch for that.  The only exciting thing in it is that
because of some undocumented cowboy programming in walsender.c, the
	Assert((proc->statusFlags & (~PROC_COPYABLE_FLAGS)) == 0);
in ProcArrayInstallRestoredXmin fires unless we skip that.

I could use some help filling in the XXX comments, because it's far
from clear to me *why* walsenders need this to happen.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="cleaner-walsender-handling-wip.patch";
	charset="us-ascii"
Content-ID: <1737780.1650046441.2@sss.pgh.pa.us>
Content-Description: cleaner-walsender-handling-wip.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/replication/walsender.c b/src/backend/replication=
/walsender.c
index be40261393..a04a4a8e5d 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -285,6 +285,15 @@ InitWalSender(void)
 	MarkPostmasterChildWalSender();
 	SendPostmasterSignal(PMSIGNAL_ADVANCE_STATE_MACHINE);
 =

+	/*
+	 * Also, mark ourselves in PGPROC as affecting vacuum horizons in all
+	 * databases.  XXX explain why
+	 */
+	LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+	MyProc->statusFlags |=3D PROC_AFFECTS_ALL_HORIZONS;
+	ProcGlobal->statusFlags[MyProc->pgxactoff] =3D MyProc->statusFlags;
+	LWLockRelease(ProcArrayLock);
+
 	/* Initialize empty timestamp buffer for lag tracking. */
 	lag_tracker =3D MemoryContextAllocZero(TopMemoryContext, sizeof(LagTrack=
er));
 }
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc=
/procarray.c
index f6e98aae29..8b867bd56c 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -1821,10 +1821,12 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h)
 		 * to prune still needed data away). If the current backend never
 		 * connects to a database that is harmless, because
 		 * data_oldest_nonremovable will never be utilized.
+		 *
+		 * XXX explain PROC_AFFECTS_ALL_HORIZONS, too
 		 */
 		if (in_recovery ||
 			MyDatabaseId =3D=3D InvalidOid || proc->databaseId =3D=3D MyDatabaseId=
 ||
-			proc->databaseId =3D=3D 0)	/* always include WalSender */
+			(statusFlags & PROC_AFFECTS_ALL_HORIZONS))
 		{
 			/*
 			 * We can ignore this backend if it's running CREATE INDEX
@@ -2681,10 +2683,14 @@ ProcArrayInstallRestoredXmin(TransactionId xmin, P=
GPROC *proc)
 		/* Install xmin */
 		MyProc->xmin =3D TransactionXmin =3D xmin;
 =

-		/* Flags being copied must be valid copy-able flags. */
-		Assert((proc->statusFlags & (~PROC_COPYABLE_FLAGS)) =3D=3D 0);
-		MyProc->statusFlags =3D proc->statusFlags;
-		ProcGlobal->statusFlags[MyProc->pgxactoff] =3D MyProc->statusFlags;
+		/* walsender cheats by passing proc =3D=3D MyProc, don't check flags */
+		if (proc !=3D MyProc)
+		{
+			/* Flags being copied must be valid copy-able flags. */
+			Assert((proc->statusFlags & (~PROC_COPYABLE_FLAGS)) =3D=3D 0);
+			MyProc->statusFlags =3D proc->statusFlags;
+			ProcGlobal->statusFlags[MyProc->pgxactoff] =3D MyProc->statusFlags;
+		}
 =

 		result =3D true;
 	}
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 809b74db5e..e132665938 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -60,6 +60,9 @@ struct XidCache
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
 #define		PROC_IN_LOGICAL_DECODING	0x10	/* currently doing logical
 												 * decoding outside xact */
+#define		PROC_AFFECTS_ALL_HORIZONS	0x20	/* proc's xmin must be
+												 * included in vacuum horizons
+												 * in all databases */
 =

 /* flags reset at EOXact */
 #define		PROC_VACUUM_STATE_MASK \

------- =_aaaaaaaaaa0--