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