I'm doubtful that that'd be a good idea. It'd add a lot of contention on the
CheckpointerShmem->ckpt_lck that's acquired as part of
FirstCallSinceLastCheckpoint. On a system with a lot of IO that'd probably
noticeable.
I always look forward to your comments Andres.
I don't see a reason to have a lock in FirstCallSinceLastCheckpoint()
diff --git a/src/backend/postmaster/checkpointer.c b/src/backend/postmaster/checkpointer.c
index 580c7944119..c91c0350e78 100644
--- a/src/backend/postmaster/checkpointer.c
+++ b/src/backend/postmaster/checkpointer.c
@@ -1517,9 +1517,7 @@ FirstCallSinceLastCheckpoint(void)
int new_done;
bool FirstCall = false;
- SpinLockAcquire(&CheckpointerShmem->ckpt_lck);
new_done = CheckpointerShmem->ckpt_done;
- SpinLockRelease(&CheckpointerShmem->ckpt_lck);
if (new_done != ckpt_done)
FirstCall = true;
I don't think any approach that adds an acuisition of a central lock around
every IO is going to make sense here.
I think Alexandre's patch has the same issue, unfortunately.
Unfortunately, yes. I didn't notice how complex ReceiveSharedInvalidMessages
is, SIGetDataEntries is hiding some LWLocks.
I'm sure we could make FirstCallSinceLastCheckpoint() not require a
spinlock.
race condition hehe
But I wonder if that's quite the right design. Doing work for every
single iteration of various loops (bgwriter, io worker) doesn't really seem
right to me for something that's as rare as this.
Well, from my previous read of Nazir's patch he will check only when the worker
goes idle, at most once per checkpoint.
Having aux processes participate in sinval doesn't really clearly seem like
the right thing either. There are a lot of messages that they never benefit
from. And manual filtering in the receive function like Alexandre added
doesn't really seem like a good way of addressing that.
Noted
Maybe we should just drop having SHAREDINVALSMGR_ID and only use
PROCSIGNAL_BARRIER_SMGRRELEASE?
Closing all the files because one was dropped doesn't seem right either.
bool
ProcessBarrierSmgrRelease(void)
{
smgrreleaseall();
return true;
}
And, apparently, that is prone to the same issue as shihao pointed out earlier.
smgrreleaseall() will turn a file leak into a slow memory memory leak.
Regards,
Alexandre
