Horton

Re: Is this a problem in GenericXLogFinish()?

Heikki Linnakangas <hlinnaka@iki.fi>

View thread View on postgr.es
From: Heikki Linnakangas <hlinnaka@iki.fi>
To: Jeff Davis <pgsql@j-davis.com>, pgsql-hackers@postgresql.org
Date: 2023-09-26T21:14:26Z
Lists: pgsql-hackers 
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)

Commits

  1. Fix inconsistency with replay of hash squeeze record for clean buffers

  2. Set LSN for wbuf in _hash_freeovflpage() iff wbuf is modified.

  3. Fix an uninitialized access in hash_xlog_squeeze_page().

  4. Use REGBUF_NO_CHANGE at one more place in the hash index.

  5. Assert that buffers are marked dirty before XLogRegisterBuffer().

  6. Fix bug in GenericXLogFinish().

  7. Improve hash index bucket split behavior.