Dear Fujii Masao,
Thank you for the review of the patch. Based on your comments I propose
a new version of the patch.
> +#include "storage/buf_internals.h"
> This include should be placed in alphabetical order.
I removed the include of the header file. I'm not sure, that it is a good
way to expose buffer internals here. Instead, I implemented a new function
BufferGetRefCount to be used in standby.c.
> + uint32 buf_refcount = BUF_STATE_GET_REFCOUNT(buf_state);
> <snip>
> + if (buf_refcount > 1)
> + continue;
>
> Wouldn't it be better to check explicitly whether we're still the waiter,
> instead of using BUF_STATE_GET_REFCOUNT()?
>
> (buf_state & BM_PIN_COUNT_WAITER) != 0 &&
> bufHdr->wait_backend_pgprocno == MyProcNumber
This is a precondition for the ResolveRecoveryConflictWithBufferPin function
that the current process should be a waiter process. See LockBufferForCleanup
where this state is set before the call of ResolveRecoveryConflictWithBufferPin.
I believe, there is no sense to check the waiter state because it doesn't
change in this function. Instead, I think, it is enough to just check for
refcount. One of the suggestions is to add an assert to check that the current
process is a waiter process, but I'm not sure about it. Let me know please if
I missed anything.
> The current control flow in the loop feels a bit hard to follow.
> Would something like the following be simpler?
I reorganized the control flow as well.
I also added a new tap-test as a part of the patch. I did some changes in the
tap test to make it stable. Let me know, please, if it should be in a separate
commit.
With best regards,
Vitaly
