Re: BufferAlloc: don't take two simultaneous locks

В Вс, 06/02/2022 в 19:34 +0300, Michail Nikolaev пишет:
> Hello, Yura.
>
> A one additional moment:
>
> > 1332: Assert((oldFlags & (BM_PIN_COUNT_WAITER | BM_IO_IN_PROGRESS)) == 0);
> > 1333: CLEAR_BUFFERTAG(buf->tag);
> > 1334: buf_state &= ~(BUF_FLAG_MASK | BUF_USAGECOUNT_MASK);
> > 1335: UnlockBufHdr(buf, buf_state);
>
> I think there is no sense to unlock buffer here because it will be
> locked after a few moments (and no one is able to find it somehow). Of
> course, it should be unlocked in case of collision.

UnlockBufHdr actually writes buf_state. Until it called, buffer
is in intermediate state and it is ... locked.

We have to write state with BM_TAG_VALID cleared before we
call BufTableDelete and release oldPartitionLock to maintain
consistency.

Perhaps, it could be cheated, and there is no harm to skip state
write at this point. But I'm not so confident to do it.

>
> BTW, I still think is better to introduce some kind of
> hash_update_hash_key and use it.
>
> It may look like this:
>
> // should be called with oldPartitionLock acquired
> // newPartitionLock hold on return
> // oldPartitionLock and newPartitionLock are not taken at the same time
> // if newKeyPtr is present - existingEntry is removed
> bool hash_update_hash_key_or_remove(
> HTAB *hashp,
> void *existingEntry,
> const void *newKeyPtr,
> uint32 newHashValue,
> LWLock *oldPartitionLock,
> LWLock *newPartitionLock
> );

Interesting suggestion, thanks. I'll think about.
It has downside of bringing LWLock knowdlege to dynahash.c .
But otherwise looks smart.

---------

regards,
Yura Sokolov