On 2024-Mar-03, Tom Lane wrote:
> This is certainly simpler, but I notice that it holds the current
> LWLock across the line
>
> ptr = (MultiXactMember *) palloc(length * sizeof(MultiXactMember));
>
> where the old code did not. Could the palloc take long enough that
> holding the lock is bad?
Hmm, I guess most of the time it shouldn't be much of a problem (if the
length is small so we can palloc without malloc'ing); but it could be in
the worst case. But the fix is simple: just release the lock before.
There's no correctness argument for holding it all the way down. I was
just confused about how the original code worked.
> Also, with this coding the "lock = NULL;" assignment just before
> "goto retry" is a dead store. Not sure if Coverity or other static
> analyzers would whine about that.
Oh, right. I removed that.
On 2024-Mar-04, Dilip Kumar wrote:
> I am not sure about the other changes, I mean that makes the code much
> simpler but now we are not releasing the 'MultiXactOffsetCtl' related
> bank lock, and later in the following loop, we are comparing that lock
> against 'MultiXactMemberCtl' related bank lock. This makes code
> simpler because now in the loop we are sure that we are always holding
> the lock but I do not like comparing the bank locks for 2 different
> SLRUs, although there is no problem as there would not be a common
> lock address,
True. This can be addressed in the same way Tom's first comment is:
just release the lock before entering the second loop, and setting lock
to NULL. This brings the code to a similar state than before, except
that the additional LWLock * variables are in a tighter scope. That's
in 0001.
Now, I had a look at the other users of slru.c and noticed in subtrans.c
that StartupSUBTRANS we have some duplicate code that I think could be
rewritten by making the "while" block test the condition at the end
instead of at the start; changed that in 0002. I'll leave this one for
later, because I want to add some test code for it -- right now it's
pretty much test-uncovered code.
I also looked at slru.c for uses of shared->bank_locks and noticed a
couple that could be made simpler. That's 0003 here.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"If it is not right, do not do it.
If it is not true, do not say it." (Marcus Aurelius, Meditations)