On Sun, Apr 06, 2025 at 11:00:54AM -0700, Noah Misch wrote:
> I pushed that as commit 8e7e672 (2024-10-25). I now think DELAY_CHKPT_START
> is superfluous here, per this proc.h comment:
>
> * (In the
> * extremely common case where the data being modified is in shared buffers
> * and we acquire an exclusive content lock on the relevant buffers before
> * writing WAL, this mechanism is not needed, because phase 2 will block
> * until we release the content lock and then flush the modified data to
> * disk.)
>
> heap_inplace_update_and_unlock() meets those conditions. Its closest
> precedent is XLogSaveBufferForHint(), which does need DELAY_CHKPT_START due to
> having only BUFFER_LOCK_SHARED.
True so far, but ...
> heap_inplace_update_and_unlock() has
> BUFFER_LOCK_EXCLUSIVE, so it doesn't need DELAY_CHKPT_START.
... not so, because we've not yet done MarkBufferDirty(). transam/README
cites SyncOneBuffer(), which says:
* Check whether buffer needs writing.
*
* We can make this check without taking the buffer content lock so long
* as we mark pages dirty in access methods *before* logging changes with
* XLogInsert(): if someone marks the buffer dirty just after our check we
* don't worry because our checkpoint.redo points before log record for
* upcoming changes and so we are not required to write such dirty buffer.
The attached patch updates the aforementioned proc.h comment and the
heap_inplace_update_and_unlock() comment that my last message proposed.
