Subject: Re: Rename PageData to XLogPageData

From: Heikki Linnakangas <[email-masked]>
Date: Wed, 2 Oct 2024 20:02:06 +0300

On 02/10/2024 14:30, Peter Eisentraut wrote:
> I was fiddling a bit with making some Page-related APIs const-proof, 
> which might involve changing something like "Page p" to "const PageData 
> *p", but I was surprised that a type PageData exists but it's an 
> unrelated type local to generic_xlog.c.

Good find

> This patch renames that type to a more specific name XLogPageData.  This 
> makes room for possibly adding another PageData type with the earlier 
> meaning, but that's not done here.  But I think even without that, this 
> patch is a useful little cleanup that makes the code more consistent and 
> clear.

+1 for renaming, but -1 on XLogPageData. That sounds like a WAL page, 
see XLogPageHeaderData for example. I'd suggest GenericXLogPageData or 
just GenericPerPageData or something.

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

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 3 Oct 2024 13:31:19 +0200

On 02.10.24 19:02, Heikki Linnakangas wrote:
>> This patch renames that type to a more specific name XLogPageData.  
>> This makes room for possibly adding another PageData type with the 
>> earlier meaning, but that's not done here.  But I think even without 
>> that, this patch is a useful little cleanup that makes the code more 
>> consistent and clear.
> 

Sounds good.  Here is an updated version.

From 988094723e5b98ef89c5bdacde665cd51bc0e766 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email-masked]>
Date: Thu, 3 Oct 2024 13:14:33 +0200
Subject: [PATCH v2] Rename PageData to GenericXLogPageData

In the PostgreSQL C type naming schema, the type PageData should be
what the pointer of type Page points to.  But in this case it's
actually an unrelated type local to generic_xlog.c.  Rename that to a
more specific name.  This makes room to possible add a PageData type
with the mentioned meaning, but this is not done here.

Reviewed-by: Heikki Linnakangas <[email-masked]>
Discussion: https://www.postgresql.org/message-id/flat/001d457e-c118-4219-8132-e1846c2ae3c9%40eisentraut.org
---
 src/backend/access/transam/generic_xlog.c | 24 +++++++++++------------
 src/tools/pgindent/typedefs.list          |  2 +-
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/src/backend/access/transam/generic_xlog.c b/src/backend/access/transam/generic_xlog.c
index e8522781631..1b3a059f7a3 100644
--- a/src/backend/access/transam/generic_xlog.c
+++ b/src/backend/access/transam/generic_xlog.c
@@ -55,7 +55,7 @@ typedef struct
 	char	   *image;			/* copy of page image for modification, do not
 								 * do it in-place to have aligned memory chunk */
 	char		delta[MAX_DELTA_SIZE];	/* delta between page images */
-} PageData;
+} GenericXLogPageData;
 
 /*
  * State of generic xlog record construction.  Must be allocated at an I/O
@@ -66,17 +66,17 @@ struct GenericXLogState
 	/* Page images (properly aligned, must be first) */
 	PGIOAlignedBlock images[MAX_GENERIC_XLOG_PAGES];
 	/* Info about each page, see above */
-	PageData	pages[MAX_GENERIC_XLOG_PAGES];
+	GenericXLogPageData pages[MAX_GENERIC_XLOG_PAGES];
 	bool		isLogged;
 };
 
-static void writeFragment(PageData *pageData, OffsetNumber offset,
+static void writeFragment(GenericXLogPageData *pageData, OffsetNumber offset,
 						  OffsetNumber length, const char *data);
-static void computeRegionDelta(PageData *pageData,
+static void computeRegionDelta(GenericXLogPageData *pageData,
 							   const char *curpage, const char *targetpage,
 							   int targetStart, int targetEnd,
 							   int validStart, int validEnd);
-static void computeDelta(PageData *pageData, Page curpage, Page targetpage);
+static void computeDelta(GenericXLogPageData *pageData, Page curpage, Page targetpage);
 static void applyPageRedo(Page page, const char *delta, Size deltaSize);

@@ -87,7 +87,7 @@ static void applyPageRedo(Page page, const char *delta, Size deltaSize);
  * actual data (of length length).
  */
 static void
-writeFragment(PageData *pageData, OffsetNumber offset, OffsetNumber length,
+writeFragment(GenericXLogPageData *pageData, OffsetNumber offset, OffsetNumber length,
 			  const char *data)
 {
 	char	   *ptr = pageData->delta + pageData->deltaLen;
@@ -118,7 +118,7 @@ writeFragment(PageData *pageData, OffsetNumber offset, OffsetNumber length,
  * about the data-matching loops.
  */
 static void
-computeRegionDelta(PageData *pageData,
+computeRegionDelta(GenericXLogPageData *pageData,
 				   const char *curpage, const char *targetpage,
 				   int targetStart, int targetEnd,
 				   int validStart, int validEnd)
@@ -225,7 +225,7 @@ computeRegionDelta(PageData *pageData,
  * and store it in pageData's delta field.
  */
 static void
-computeDelta(PageData *pageData, Page curpage, Page targetpage)
+computeDelta(GenericXLogPageData *pageData, Page curpage, Page targetpage)
 {
 	int			targetLower = ((PageHeader) targetpage)->pd_lower,
 				targetUpper = ((PageHeader) targetpage)->pd_upper,
@@ -303,7 +303,7 @@ GenericXLogRegisterBuffer(GenericXLogState *state, Buffer buffer, int flags)
 	/* Search array for existing entry or first unused slot */
 	for (block_id = 0; block_id < MAX_GENERIC_XLOG_PAGES; block_id++)
 	{
-		PageData   *page = &state->pages[block_id];
+		GenericXLogPageData *page = &state->pages[block_id];
 
 		if (BufferIsInvalid(page->buffer))
 		{
@@ -352,7 +352,7 @@ GenericXLogFinish(GenericXLogState *state)
 		 */
 		for (i = 0; i < MAX_GENERIC_XLOG_PAGES; i++)
 		{
-			PageData   *pageData = &state->pages[i];
+			GenericXLogPageData *pageData = &state->pages[i];
 			Page		page;
 			PageHeader	pageHeader;
 
@@ -401,7 +401,7 @@ GenericXLogFinish(GenericXLogState *state)
 		/* Set LSN */
 		for (i = 0; i < MAX_GENERIC_XLOG_PAGES; i++)
 		{
-			PageData   *pageData = &state->pages[i];
+			GenericXLogPageData *pageData = &state->pages[i];
 
 			if (BufferIsInvalid(pageData->buffer))
 				continue;
@@ -415,7 +415,7 @@ GenericXLogFinish(GenericXLogState *state)
 		START_CRIT_SECTION();
 		for (i = 0; i < MAX_GENERIC_XLOG_PAGES; i++)
 		{
-			PageData   *pageData = &state->pages[i];
+			GenericXLogPageData *pageData = &state->pages[i];
 
 			if (BufferIsInvalid(pageData->buffer))
 				continue;
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 5fabb127d7e..c4de597b1fa 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1003,6 +1003,7 @@ GenerationBlock
 GenerationContext
 GenerationPointer
 GenericCosts
+GenericXLogPageData
 GenericXLogState
 GeqoPrivateData
 GetForeignJoinPaths_function
@@ -1975,7 +1976,6 @@ PX_Combo
 PX_HMAC
 PX_MD
 Page
-PageData
 PageGistNSN
 PageHeader
 PageHeaderData

base-commit: 19531968e84557693576928b3184ccc549aa44c8
-- 
2.46.2

---

From: Michael Paquier <[email-masked]>
Date: Fri, 4 Oct 2024 12:55:45 +0900

> Sounds good.  Here is an updated version.

Good idea.  What you have sent here looks good to me.
--
Michael

---

From: Peter Eisentraut <[email-masked]>
Date: Fri, 4 Oct 2024 13:02:04 +0200

> Good idea.  What you have sent here looks good to me.

committed