Re: Intermittent buildfarm failures on wrasse - Mailing list pgsql-hackers

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

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 |= PROC_AFFECTS_ALL_HORIZONS;
+    ProcGlobal->statusFlags[MyProc->pgxactoff] = MyProc->statusFlags;
+    LWLockRelease(ProcArrayLock);
+
     /* Initialize empty timestamp buffer for lag tracking. */
     lag_tracker = MemoryContextAllocZero(TopMemoryContext, sizeof(LagTracker));
 }
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 == InvalidOid || proc->databaseId == MyDatabaseId ||
-            proc->databaseId == 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, PGPROC *proc)
         /* Install xmin */
         MyProc->xmin = TransactionXmin = xmin;

-        /* 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;
+        /* walsender cheats by passing proc == MyProc, don't check 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;
+        }

         result = 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 \