Horton

RE: Is this a problem in GenericXLogFinish()?

Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>

View thread View on postgr.es
From: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
To: 'Alexander Lakhin' <exclusion@gmail.com>, 'Amit Kapila' <amit.kapila16@gmail.com>
Cc: 'Michael Paquier' <michael@paquier.xyz>, 'Robert Haas' <robertmhaas@gmail.com>, 'Jeff Davis' <pgsql@j-davis.com>, 'Heikki Linnakangas' <hlinnaka@iki.fi>, "'pgsql-hackers@postgresql.org'" <pgsql-hackers@postgresql.org>
Date: 2023-11-30T07:28:05Z
Lists: pgsql-hackers

Attachments

Dear Alexander,

> 
> Good catch, thank you for reporting! I will investigate more about it and post my
> analysis.
>

Again, good catch. Here is my analysis and fix patch.
I think it is sufficient to add an initialization for writebuf.

In the reported case, neither is_prim_bucket_same_wrt nor is_prev_bucket_same_wrt
is set in the WAL record, and ntups is also zero. This means that the wbuf is not
written in the WAL record on primary side (see [1]).
Also, there are no reasons to read (and lock) other buffer page because we do
not modify it. Based on them, I think that just initializing as InvalidBuffer is sufficient.


I want to add a test case for it as well. I've tested on my env and found that proposed
tuples seems sufficient.
(We must fill the primary bucket, so initial 500 is needed. Also, we must add
many dead pages to lead squeeze operation. Final page seems OK to smaller value.)

I compared the execution time before/after patching, and they are not so different
(1077 ms -> 1125 ms).

How do you think?

[1]:
```
		else if (xlrec.is_prim_bucket_same_wrt || xlrec.is_prev_bucket_same_wrt)
		{
			uint8		wbuf_flags;

			/*
			 * A write buffer needs to be registered even if no tuples are
			 * added to it to ensure that we can acquire a cleanup lock on it
			 * if it is the same as primary bucket buffer or update the
			 * nextblkno if it is same as the previous bucket buffer.
			 */
			Assert(xlrec.ntups == 0);

			wbuf_flags = REGBUF_STANDARD;
			if (!xlrec.is_prev_bucket_same_wrt)
				wbuf_flags |= REGBUF_NO_CHANGE;
			XLogRegisterBuffer(1, wbuf, wbuf_flags);
		}
```

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

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.