Hi Soumyadeep and Ashwin,
Thanks for looking!
On Sun, Jul 18, 2021 at 6:58 AM Soumyadeep Chakraborty
<soumyadeep2007@gmail.com> wrote:
> You might have missed a spot to initialize SERIALIZABLE_XACT->pgprocno in
> InitPredicateLocks(), so:
>
> + PredXact->OldCommittedSxact->pgprocno = INVALID_PGPROCNO;
The magic OldCommittedSxact shouldn't be the target of a "signal", but
this is definitely tidier. Thanks.
> Slightly tangential: we should add a comment to PGPROC.pgprocno, for more
> immediate understandability:
>
> + int pgprocno; /* index of this PGPROC in ProcGlobal->allProcs */
I wonder why we need this member anyway, when you can compute it from
the address... #define GetPGProcNumber(p) ((p) - ProcGlobal->allProcs)
or something like that? Kinda wonder why we don't use
GetPGProcByNumber() in more places instead of open-coding access to
ProcGlobal->allProcs, too...
> Also, why don't we take the opportunity to get rid of SERIALIZABLEXACT->pid? We
> took a stab. Attached is v2 of your patch with these changes.
SERIALIZABLEXACT objects can live longer than the backends that
created them. They hang around to sabotage other transactions' plans,
depending on what else they overlapped with before they committed.
With that change, the pg_locks view might show the pid of some
unrelated session that moves into the same PGPROC.
It's only an "informational" pid, and pids are imperfect information
anyway because (1) they are themselves recycled, and (2) they won't be
interesting in a hypothetical multi-threaded future. One solution
would be to hide the pids from the view after the backend disconnects
(somehow -- add a generation number?), but they're also still kinda
useful, despite the weaknesses. I wonder what the ideal way would be
to refer to sessions, anyway, including those that are no longer
active; perhaps we could invent a new "session ID" concept.