On 26/09/2023 22:32, Jeff Davis wrote:
On Mon, 2023-09-25 at 13:04 +0300, Heikki Linnakangas wrote:
Yes, that's a problem.
Patch attached. I rearranged the code a bit to follow the expected
pattern of: write, mark dirty, WAL, set LSN. I think computing the
deltas could also be moved earlier, outside of the critical section,
but I'm not sure that would be useful.
Looks correct. You now loop through all the block IDs three times,
however. I wonder if that is measurably slower, but even if it's not,
was there a reason you wanted to move the XLogRegisterBuffer() calls to
a separate loop?
Do you have a suggestion for any kind of test addition, or should we
just review carefully?
I wish we had an assertion for that. XLogInsert() could assert that
the page is already marked dirty, for example.
Unfortunately that's not always the case, e.g. log_newpage_range().
Hmm, I'm sure there are exceptions but log_newpage_range() actually
seems to be doing the right thing; it calls MarkBufferDirty() before
XLogInsert(). It only calls it after XLogRegisterBuffer() though, and I
concur that XLogRegisterBuffer() would be the logical place for the
assertion. We could tighten this up, require that you call
MarkBufferDirty() before XLogRegisterBuffer(), and fix all the callers.
--
Heikki Linnakangas
Neon (https://neon.tech)
