Horton

Re: Is this a problem in GenericXLogFinish()?

Michael Paquier <michael@paquier.xyz>

View thread View on postgr.es
From: Michael Paquier <michael@paquier.xyz>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>, Jeff Davis <pgsql@j-davis.com>, Alexander Lakhin <exclusion@gmail.com>, Heikki Linnakangas <hlinnaka@iki.fi>, pgsql-hackers@postgresql.org
Date: 2023-11-01T07:24:07Z
Lists: pgsql-hackers 
On Mon, Oct 30, 2023 at 03:54:39PM +0530, Amit Kapila wrote:
> On Sat, Oct 28, 2023 at 4:30 PM Michael Paquier <michael@paquier.xyz> wrote:
>> On Sat, Oct 28, 2023 at 03:45:13PM +0530, Amit Kapila wrote:
>> > Yes, we need it to exclude any concurrent in-progress scans that could
>> > return incorrect tuples during bucket squeeze operation.
>>
>> Thanks.  So I assume that we should just set REGBUF_NO_CHANGE when the
>> primary and write buffers are the same and there are no tuples to
>> move.  Say with something like the attached?
> 
> What if the primary and write buffer are not the same but ntups is
> zero? Won't we hit the assertion again in that case?

The code tells that it should be able to handle such a case, so this
would mean to set REGBUF_NO_CHANGE only when we have no tuples to
move:
-        XLogRegisterBuffer(1, wbuf, REGBUF_STANDARD);
+        /*
+         * A cleanup lock is still required on the write buffer even
+         * if there are no tuples to move, so it needs to be registered
+         * in this case.
+         */
+        wbuf_flags = REGBUF_STANDARD;
+        if (xlrec.ntups == 0)
+            wbuf_flags |= REGBUF_NO_CHANGE;
+        XLogRegisterBuffer(1, wbuf, wbuf_flags);

Anyway, there is still a hole here: the regression tests have zero
coverage for the case where there are no tuples to move if
!is_prim_bucket_same_wrt.  There are only two queries that stress the
squeeze path with no tuples, both use is_prim_bucket_same_wrt:
INSERT INTO hash_split_heap SELECT a/2 FROM generate_series(1, 25000) a;
VACUUM hash_split_heap;

Perhaps this should have more coverage to make sure that all these
scenarios are covered at replay (likely with 027_stream_regress.pl)?
The case posted by Alexander at [1] falls under the same category
(is_prim_bucket_same_wrt with no tuples).

[1]: https://www.postgresql.org/message-id/f045c8f7-ee24-ead6-3679-c04a43d21351@gmail.com
--
Michael

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.