Fix inconsistencies with code and beautify xlog structures description and fin hash_xlog.h
Hi hackers!
While doing some xlog-related task at my job I noticed inconsistency
between [0]https://git.postgresql.org/cgit/postgresql.git/tree/src/include/access/hash_xlog.h?id=567d27e8e2b752743626eb259ba75ecdc936eaf3#n147 & [1]https://git.postgresql.org/cgit/postgresql.git/tree/src/backend/access/hash/hash_xlog.c?id=567d27e8e2b752743626eb259ba75ecdc936eaf3#n830. hash_xlog.h xl_hash_squeeze_page structure
description mismatches with actual code.
PFA fixing issue.
I also allowed myself to standardise description of
xl_hash_move_page_contents and xl_hash_init_bitmap_page, so
xl_hash_delete, xl_hash_init_bitmap_page and
xl_hash_move_page_contents describes their zeroth backup block akin to
each other.
[0]: https://git.postgresql.org/cgit/postgresql.git/tree/src/include/access/hash_xlog.h?id=567d27e8e2b752743626eb259ba75ecdc936eaf3#n147
[1]: https://git.postgresql.org/cgit/postgresql.git/tree/src/backend/access/hash/hash_xlog.c?id=567d27e8e2b752743626eb259ba75ecdc936eaf3#n830
--
Best regards,
Kirill Reshke
Attachments:
v1-0001-Fix-comments-in-xlog.h.patchapplication/octet-stream; name=v1-0001-Fix-comments-in-xlog.h.patchDownload
From f7fc411b888ef42d39827e8621d4dfdd2a26a828 Mon Sep 17 00:00:00 2001
From: reshke <reshke@qavm-273b4667.qemu>
Date: Fri, 5 Sep 2025 09:08:27 +0300
Subject: [PATCH v1] Fix comments in xlog.h
---
src/include/access/hash_xlog.h | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/src/include/access/hash_xlog.h b/src/include/access/hash_xlog.h
index 6fe97de4d6..5d4671dc4c 100644
--- a/src/include/access/hash_xlog.h
+++ b/src/include/access/hash_xlog.h
@@ -129,7 +129,7 @@ typedef struct xl_hash_split_complete
*
* This data record is used for XLOG_HASH_MOVE_PAGE_CONTENTS
*
- * Backup Blk 0: bucket page
+ * Backup Blk 0: primary bucket page
* Backup Blk 1: page containing moved tuples
* Backup Blk 2: page from which tuples will be removed
*/
@@ -149,12 +149,13 @@ typedef struct xl_hash_move_page_contents
*
* This data record is used for XLOG_HASH_SQUEEZE_PAGE
*
- * Backup Blk 0: page containing tuples moved from freed overflow page
- * Backup Blk 1: freed overflow page
- * Backup Blk 2: page previous to the freed overflow page
- * Backup Blk 3: page next to the freed overflow page
- * Backup Blk 4: bitmap page containing info of freed overflow page
- * Backup Blk 5: meta page
+ * Backup Blk 0: primary bucket page
+ * Backup Blk 1: page containing tuples moved from freed overflow page
+ * Backup Blk 2: freed overflow page
+ * Backup Blk 3: page previous to the freed overflow page
+ * Backup Blk 4: page next to the freed overflow page
+ * Backup Blk 5: bitmap page containing info of freed overflow page
+ * Backup Blk 6: meta page
*/
typedef struct xl_hash_squeeze_page
{
@@ -245,7 +246,7 @@ typedef struct xl_hash_init_bitmap_page
*
* This data record is used for XLOG_HASH_VACUUM_ONE_PAGE
*
- * Backup Blk 0: bucket page
+ * Backup Blk 0: primary bucket page
* Backup Blk 1: meta page
*/
typedef struct xl_hash_vacuum_one_page
--
2.43.0
On Fri, 5 Sept 2025 at 11:17, I wrote:
I also allowed myself to standardise description of
xl_hash_move_page_contents and xl_hash_init_bitmap_page, so
xl_hash_delete, xl_hash_init_bitmap_page and
xl_hash_move_page_contents describes their zeroth backup block akin to
each other.
Ahh... sorry for the noise, this part is wrong.
PFA v2
--
Best regards,
Kirill Reshke
Attachments:
v2-0001-Fix-comments-in-xlog.h.patchapplication/octet-stream; name=v2-0001-Fix-comments-in-xlog.h.patchDownload
From bf69cabcba8cde920881b7025621620cd098c4d0 Mon Sep 17 00:00:00 2001
From: reshke <reshke@qavm-273b4667.qemu>
Date: Fri, 5 Sep 2025 09:08:27 +0300
Subject: [PATCH v2] Fix comments in xlog.h
---
src/include/access/hash_xlog.h | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/include/access/hash_xlog.h b/src/include/access/hash_xlog.h
index 6fe97de4d6..6816b80b07 100644
--- a/src/include/access/hash_xlog.h
+++ b/src/include/access/hash_xlog.h
@@ -149,12 +149,13 @@ typedef struct xl_hash_move_page_contents
*
* This data record is used for XLOG_HASH_SQUEEZE_PAGE
*
- * Backup Blk 0: page containing tuples moved from freed overflow page
- * Backup Blk 1: freed overflow page
- * Backup Blk 2: page previous to the freed overflow page
- * Backup Blk 3: page next to the freed overflow page
- * Backup Blk 4: bitmap page containing info of freed overflow page
- * Backup Blk 5: meta page
+ * Backup Blk 0: bucket page
+ * Backup Blk 1: page containing tuples moved from freed overflow page
+ * Backup Blk 2: freed overflow page
+ * Backup Blk 3: page previous to the freed overflow page
+ * Backup Blk 4: page next to the freed overflow page
+ * Backup Blk 5: bitmap page containing info of freed overflow page
+ * Backup Blk 6: meta page
*/
typedef struct xl_hash_squeeze_page
{
--
2.43.0
On Sep 5, 2025, at 14:24, Kirill Reshke <reshkekirill@gmail.com> wrote:
--
Best regards,
Kirill Reshke
<v2-0001-Fix-comments-in-xlog.h.patch>
LGTM. SlruRecentlyUsed() is now an inline function.
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On Sep 5, 2025, at 16:29, Chao Li <li.evan.chao@gmail.com> wrote:
On Sep 5, 2025, at 14:24, Kirill Reshke <reshkekirill@gmail.com> wrote:
--
Best regards,
Kirill Reshke
<v2-0001-Fix-comments-in-xlog.h.patch>LGTM. SlruRecentlyUsed() is now an inline function.
After pulling master, I just found this patch has been pushed.
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
On 5 Sep 2025, at 11:24, Kirill Reshke <reshkekirill@gmail.com> wrote:
PFA v2
Proposed change is correct and in line with a description of e.g. XLOG_HASH_MOVE_PAGE_CONTENTS.
I'd note that block is for lock only and record does not include image, but other such cases are not reported in comments.
Best regards, Andrey Borodin.
On Wed, Sep 10, 2025 at 05:36:48PM +0500, Andrey Borodin wrote:
Proposed change is correct and in line with a description of
e.g. XLOG_HASH_MOVE_PAGE_CONTENTS.
I'd note that block is for lock only and record does not include
image, but other such cases are not reported in comments.
Right, the case of block 0 being registered in a MOVE_PAGE_CONTENTS
record to ensure that a cleanup lock is taken is documented in
_hash_squeezebucket()@hashovfl.c, for SQUEEZE_PAGE, or for DELETE.
Squeeze can also do that for buffer 1.
On top of your suggestions, I have double-checked the rest of the hash
records, and they seem to be in line with hash_xlog.h. Will fix,
thanks.
--
Michael