Обсуждение: 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)
Вложения
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)