Re: Is this a problem in GenericXLogFinish()?

On 10/10/2023 22:57, Jeff Davis wrote:
> On Thu, 2023-09-28 at 12:05 -0700, Jeff Davis wrote:
>> Also, I ran into some problems with GIN that might require a bit more
>> refactoring in ginPlaceToPage(). Perhaps we could consider
>> REGBUF_NO_CHANGE a general bypass of the Assert(BufferIsDirty()), and
>> use it temporarily until we have a chance to analyze/refactor each of
>> the callers that need it.
> 
> For the Assert, I have attached a new patch for v17.

Thanks for working on this!

> +/*
> + * BufferIsDirty
> + *
> + *        Checks if buffer is already dirty.
> + *
> + * Buffer must be pinned and exclusive-locked.  (If caller does not hold
> + * exclusive lock, then the result may be stale before it's returned.)
> + */
> +bool
> +BufferIsDirty(Buffer buffer)
> +{
> +    BufferDesc *bufHdr;
> +
> +    if (BufferIsLocal(buffer))
> +    {
> +        int bufid = -buffer - 1;
> +        bufHdr = GetLocalBufferDescriptor(bufid);
> +    }
> +    else
> +    {
> +        bufHdr = GetBufferDescriptor(buffer - 1);
> +    }
> +
> +    Assert(BufferIsPinned(buffer));
> +    Assert(LWLockHeldByMeInMode(BufferDescriptorGetContentLock(bufHdr),
> +                                LW_EXCLUSIVE));
> +
> +    return pg_atomic_read_u32(&bufHdr->state) & BM_DIRTY;
> +}
The comment suggests that you don't need to hold an exclusive lock when 
you call this, but there's an assertion that you do.

Is it a new requirement that you must hold an exclusive lock on the 
buffer when you call XLogRegisterBuffer()? I think that's reasonable.

I thought if that might be a problem when you WAL-log a page when you 
set hint bits on it and checksums are enabled. Hint bits can be set 
holding just a share lock. But it uses XLogSaveBufferForHint(), which 
calls XLogRegisterBlock() instead of XLogRegisterBuffer()

There is a comment above XLogSaveBufferForHint() that implies that 
XLogRegisterBuffer() requires you to hold an exclusive-lock but I don't 
see that spelled out explicitly in XLogRegisterBuffer() itself. Maybe 
add an assertion for that in XLogRegisterBuffer() to make it more explicit.

> --- a/src/backend/access/transam/xloginsert.c
> +++ b/src/backend/access/transam/xloginsert.c
> @@ -246,6 +246,7 @@ XLogRegisterBuffer(uint8 block_id, Buffer buffer, uint8 flags)
>  
>      /* NO_IMAGE doesn't make sense with FORCE_IMAGE */
>      Assert(!((flags & REGBUF_FORCE_IMAGE) && (flags & (REGBUF_NO_IMAGE))));
> +    Assert((flags & REGBUF_CLEAN_OK) || BufferIsDirty(buffer));
>      Assert(begininsert_called);
>  
>      if (block_id >= max_registered_block_id)

I'd suggest a comment here to explain what's wrong if someone hits this 
assertion. Something like "The buffer must be marked as dirty before 
registering, unless REGBUF_CLEAN_OK is set to indicate that the buffer 
is not being modified".

> I changed the name of the flag to REGBUF_CLEAN_OK, because for some of
> the callsites it was not clear to me whether the page is always
> unchanged or sometimes unchanged. In other words, REGBUF_CLEAN_OK is a
> way to bypass the Assert for callsites where we either know that we
> actually want to register an unchanged page, or for callsites where we
> don't know if the page is changed or not.

Ok. A flag to explicitly mark that the page is not modified would 
actually be nice for pg_rewind. It could ignore such page references. 
Not that it matters much in practice, since those records are so rare. 
And for that, we'd need to include the flag in the WAL record too. So I 
think this is fine.

> If we commit this, ideally someone would look into the places where
> REGBUF_CLEAN_OK is used and make sure that it's not a bug, and perhaps
> refactor so that it would benefit from the Assert. But refactoring
> those places is outside of the scope of this patch.

About that: you added the flag to a couple of XLogRegisterBuffer() calls 
in GIN, because they call MarkBufferDirty() only after 
XLogRegisterBuffer(). Those would be easy to refactor so I'd suggest 
doing that now.

> It sounds like we have no intention to change the hash index code, so
> are we OK if this flag just lasts forever? Do you still think it offers
> a useful check?

Yeah, I think this is a useful assertion. It might catch some bugs in 
extensions too.

-- 
Heikki Linnakangas
Neon (https://neon.tech)