Cleanup: PGProc->links doesn't need to be the first field anymore
pgproc.h has this:
struct PGPROC
{
/* proc->links MUST BE FIRST IN STRUCT (see ProcSleep,ProcWakeup,etc) */
dlist_node links; /* list link if process is in a list */
dlist_head *procgloballist; /* procglobal list that owns this PGPROC */
...
I don't see any particular reason for 'links' to be the first field. We
used to do things like "proc = (PGPROC *) waitQueue->links.next", but
since commit 5764f611e1, this has been a "dlist", and dlist_container()
can handle the list link being anywhere in the struct.
I tried moving it and ran the regression tests. That revealed one place
where we still don't use dlist_container:
if (!dlist_is_empty(procgloballist))
{
MyProc = (PGPROC *) dlist_pop_head_node(procgloballist);
...
I believe that was just an oversight. Trivial patch attached.
--
Heikki Linnakangas
Neon (https://neon.tech)
Attachments:
0001-Lift-limitation-that-PGPROC-links-must-be-the-first-.patchtext/x-patch; charset=UTF-8; name=0001-Lift-limitation-that-PGPROC-links-must-be-the-first-.patchDownload
From b3aad7430f1c300951bd9a4e51bb62daee7bcf2f Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date: Thu, 4 Jul 2024 01:38:58 +0300
Subject: [PATCH 1/1] Lift limitation that PGPROC->links must be the first
field
Since commit 5764f611e1, we've been using the ilist.h functions for
handling the linked list. There's no need for 'links' to be the first
element of the struct anymore, except for one call in InitProcess
where we used a straight cast from the 'dlist_node *' to PGPROC *,
without the dlist_container() macro. That was just an oversight in
commit 5764f611e1, fix it.
There no imminent need to move 'links' from being the first field, but
let's be tidy.
---
src/backend/storage/lmgr/proc.c | 2 +-
src/include/storage/proc.h | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index ce29da9012..1b23efb26f 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -330,7 +330,7 @@ InitProcess(void)
if (!dlist_is_empty(procgloballist))
{
- MyProc = (PGPROC *) dlist_pop_head_node(procgloballist);
+ MyProc = dlist_container(PGPROC, links, dlist_pop_head_node(procgloballist));
SpinLockRelease(ProcStructLock);
}
else
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 9488bf1857..7d3fc2bfa6 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -161,7 +161,6 @@ typedef enum
*/
struct PGPROC
{
- /* proc->links MUST BE FIRST IN STRUCT (see ProcSleep,ProcWakeup,etc) */
dlist_node links; /* list link if process is in a list */
dlist_head *procgloballist; /* procglobal list that owns this PGPROC */
--
2.39.2
Hi Heikki,
I tried moving it and ran the regression tests. That revealed one place
where we still don't use dlist_container:if (!dlist_is_empty(procgloballist))
{
MyProc = (PGPROC *) dlist_pop_head_node(procgloballist);
...I believe that was just an oversight. Trivial patch attached.
I tested your patch. LGTM.
PGPROC is exposed to third-party code, but since we don't change the
structure this time, the extensions will not be affected.
--
Best regards,
Aleksander Alekseev
Hi,
On 2024-07-04 01:54:18 +0300, Heikki Linnakangas wrote:
pgproc.h has this:
struct PGPROC
{
/* proc->links MUST BE FIRST IN STRUCT (see ProcSleep,ProcWakeup,etc) */
dlist_node links; /* list link if process is in a list */
dlist_head *procgloballist; /* procglobal list that owns this PGPROC */
...I don't see any particular reason for 'links' to be the first field. We used
to do things like "proc = (PGPROC *) waitQueue->links.next", but since
commit 5764f611e1, this has been a "dlist", and dlist_container() can handle
the list link being anywhere in the struct.
Indeed.
I tried moving it and ran the regression tests. That revealed one place
where we still don't use dlist_container:if (!dlist_is_empty(procgloballist))
{
MyProc = (PGPROC *) dlist_pop_head_node(procgloballist);
...I believe that was just an oversight. Trivial patch attached.
Oops. Yes, I clearly should have used dlist_container() here.
+1
Greetings,
Andres Freund
On 04/07/2024 23:20, Andres Freund wrote:
On 2024-07-04 01:54:18 +0300, Heikki Linnakangas wrote:
I believe that was just an oversight. Trivial patch attached.
Oops. Yes, I clearly should have used dlist_container() here.
Committed, thanks.
--
Heikki Linnakangas
Neon (https://neon.tech)