Re: Improve heavyweight locks instead of building new lock managers?

Hi,

Some of the discussions about improving the locking code, in particular
the group locking / deadlock detector discussion with Robert, again made
me look at lock.c. While looking at how much work it'd be to a) remove
the PROCLOCK hashtable b) move more of the group locking logic into
lock.c, rather than smarts in deadlock.c, I got sidetracked by all the
verbose and hard to read SHM_QUEUE code.

Here's a *draft* patch series for removing all use of SHM_QUEUE, and
subsequently removing SHM_QUEUE. There's a fair bit of polish needed,
but I do think it makes the code considerably easier to read
(particularly for predicate.c). The diffstat is nice too:

src/include/lib/ilist.h | 132 +++++++++++++++++----
src/include/replication/walsender_private.h | 3 +-
src/include/storage/lock.h | 10 +-
src/include/storage/predicate_internals.h | 49 +++-----
src/include/storage/proc.h | 18 +--
src/include/storage/shmem.h | 22 ----
src/backend/access/transam/twophase.c | 4 +-
src/backend/lib/ilist.c | 8 +-
src/backend/replication/syncrep.c | 89 ++++++--------
src/backend/replication/walsender.c | 2 +-
src/backend/storage/ipc/Makefile | 1 -
src/backend/storage/ipc/shmqueue.c | 190 ------------------------------
src/backend/storage/lmgr/deadlock.c | 76 +++++-------
src/backend/storage/lmgr/lock.c | 129 ++++++++------------
src/backend/storage/lmgr/predicate.c | 692 +++++++++++++++++++++++++++++++++++------------------------------------------------------------------------
src/backend/storage/lmgr/proc.c | 197 +++++++++++++------------------
16 files changed, 569 insertions(+), 1053 deletions(-)

I don't want to invest a lot of time into this if there's not some
agreement that this is a good direction to go into. So I'd appreciate a
few cursory looks before spending more time.

Overview:
0001: Add additional prev/next & detached node functions to ilist.
I think the additional prev/next accessors are nice. I am less
convinced by the 'detached' stuff, but it's used by some SHM_QUEUE
users. I don't want to make the plain dlist_delete reset the node's
prev/next pointers, it's not needed in the vast majority of cases...

0002: Use dlists instead of SHM_QUEUE for heavyweight locks.
I mostly removed the odd reliance on PGPROC.links needing to be the
first struct member - seems odd.

I think we should rename PROC_QUEUE.links, elsewhere that's used for
list membership nodes, so it's imo confusing/odd.

0003: Use dlist for syncrep queue.
This seems fairly simple to me.

0004: Use dlists for predicate locking.
Unfortunately pretty large. I think it's a huge improvement, but it's
also subtle code. Wonder if there's something better to do here wrt
OnConflict_CheckForSerializationFailure?

0005: Remove now unused SHMQueue*.
0006: Remove PROC_QUEUE.size.
I'm not sure whether this is a a good idea. I was looking primarily at
that because I thought it'd allow us to remove PROC_QUEUE as a whole
if we wanted to. But as PROC_QUEUE.size doesn't really seem to buy us
much, we should perhaps just do something roughly like in the patch?

Greetings,

Andres Freund