Re: sync request forward function ForwardSyncRequest() might hang for some time in a corner case?

On Thu, May 27, 2021 at 9:59 PM Paul Guo <paulguo(at)gmail(dot)com> wrote:
>
> > It seems like a really unlikely scenario, but maybe possible if you
> > use a lot of unlogged tables maybe (as you could eventually
> > dirty/evict more than NBuffers buffers without triggering enough WALs
> > activity to trigger a checkpoint with any sane checkpoint
> > configuration).
>
> RegisterSyncRequest() handles SYNC_UNLINK_REQUEST and
> SYNC_FORGET_REQUEST scenarios, besides the usual SYNC_REQUEST type for
> buffer sync.

I know, but the checkpointer can hold up to NBuffers requests, so I
highly doubt that you can end up filling the buffer with those.

> > > ForwardSyncRequest():
> > >
> > > if (CheckpointerShmem->checkpointer_pid == 0 ||
> > > (CheckpointerShmem->num_requests >= CheckpointerShmem->max_requests &&
> > > !CompactCheckpointerRequestQueue()))
> > > {
> > > /*
> > > * Count the subset of writes where backends have to do their own
> > > * fsync
> > > */
> > > if (!AmBackgroundWriterProcess())
> > > CheckpointerShmem->num_backend_fsync++;
> > > LWLockRelease(CheckpointerCommLock);
> > > return false;
> > > }
> > >
> > > One fix is to add below similar code in RegisterSyncRequest(), trigger
> > > a checkpoint for the scenario.
> > >
> > > // checkpointer_triggered: variable for one trigger only.
> > > if (!ret && retryOnError && ProcGlobal->checkpointerLatch &&
> > > !checkpointer_triggered)
> > > SetLatch(ProcGlobal->checkpointerLatch);
> > >
> > > Any comments?
> >
> > It looks like you intended to set the checkpointer_triggered var but
>
> Yes this is just pseduo code.
>
> > didn't. Also this will wake up the checkpointer but won't force a
> > checkpoint (unlike RequestCheckpoint()). It may be a good thing
>
> I do not expect an immediate checkpoint. AbsorbSyncRequests()
> is enough since after that RegisterSyncRequest() could finish.

You said "trigger a checkpoint", which sounded more like forcing a
checkpointer rather than waking up the checkpointer so that it can
absorb the pending requests, so it seems worth to mention what it
would really do.