Re: Is this a problem in GenericXLogFinish()?

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)