From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
cc: 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>,
        Masahiko Sawada <sawada.mshk@gmail.com>
Subject: Re: Intermittent buildfarm failures on wrasse
In-reply-to: <202204191637.eldwa2exvguw@alvherre.pgsql>
References: <202204191637.eldwa2exvguw@alvherre.pgsql>
Comments: In-reply-to Alvaro Herrera <alvherre@alvh.no-ip.org>
	message dated "Tue, 19 Apr 2022 18:37:13 +0200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <2571200.1650392946.1@sss.pgh.pa.us>
Date: Tue, 19 Apr 2022 14:29:06 -0400
Message-ID: <2571201.1650392946@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/2571201.1650392946%40sss.pgh.pa.us>
Precedence: bulk

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2022-Apr-15, Tom Lane wrote:
>> 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.

> Hmm, maybe a better use of that define is to use to select which flags
> to copy, rather than to ensure we they are the only ones set.  What
> about this?

Yeah, I thought about that too, but figured that the author probably
had a reason for writing the assertion the way it was.  If we want
to redefine PROC_COPYABLE_FLAGS as "flags associated with xmin",
that's fine by me.  But I'd suggest that both the name of the macro
and the comment for it in proc.h should be revised to match that
definition.

Another point is that as you've coded it, the code doesn't so much
copy those flags as union them with whatever the recipient had,
which seems wrong.  I could go with either

    Assert(!(MyProc->statusFlags & PROC_XMIN_FLAGS));
    MyProc->statusFlags |= (proc->statusFlags & PROC_XMIN_FLAGS);

or

    MyProc->statusFlags = (MyProc->statusFlags & ~PROC_XMIN_FLAGS) |
                          (proc->statusFlags & PROC_XMIN_FLAGS);

Perhaps the latter is more future-proof.

			regards, tom lane