On Wed, Jun 22, 2022 at 4:24 PM Thomas Munro <thomas.mu...@gmail.com> wrote:
> On Wed, Jun 22, 2022 at 2:54 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
> > John Naylor <john.nay...@enterprisedb.com> writes:
> > > On Wed, Jun 1, 2022 at 2:57 AM Robert Haas <robertmh...@gmail.com> wrote:
> > >> ... So we can fix this by:
> > >> 1. Using a relative pointer value other than 0 to represent a null
> > >> pointer. Andres suggested (Size) -1.
> > >> 2. Not storing the free page manager for the DSM in the main shared
> > >> memory segment at byte offset 0.
>
> For the record, the third idea proposed was to use 1 for the first
> byte, so that 0 is reserved for NULL and works with memset(0). Here's
> an attempt at that.
... erm, though, duh, I forgot to adjust Assert(val > base). One more time.
From ba1661745ad57c69d0d9f881913d337504b5e870 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.mu...@gmail.com>
Date: Wed, 22 Jun 2022 16:18:53 +1200
Subject: [PATCH v2] Fix relptr's encoding of NULL.
Use 0 for NULL, and use 1-based offset for non-NULL values. Also fix
macro hygiene in passing (ie missing/misplaced parentheses), and remove
open-coded access to the raw value from freepage.c/.h.
---
src/backend/utils/mmgr/freepage.c | 6 +++---
src/include/utils/freepage.h | 4 ++--
src/include/utils/relptr.h | 15 +++++++++------
3 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/src/backend/utils/mmgr/freepage.c b/src/backend/utils/mmgr/freepage.c
index b26a295a4e..dcf246faf1 100644
--- a/src/backend/utils/mmgr/freepage.c
+++ b/src/backend/utils/mmgr/freepage.c
@@ -434,7 +434,7 @@ FreePageManagerDump(FreePageManager *fpm)
/* Dump general stuff. */
appendStringInfo(&buf, "metadata: self %zu max contiguous pages = %zu\n",
- fpm->self.relptr_off, fpm->contiguous_pages);
+ relptr_offset(fpm->self), fpm->contiguous_pages);
/* Dump btree. */
if (fpm->btree_depth > 0)
@@ -1269,7 +1269,7 @@ FreePageManagerDumpBtree(FreePageManager *fpm, FreePageBtree *btp,
if (btp->hdr.magic == FREE_PAGE_INTERNAL_MAGIC)
appendStringInfo(buf, " %zu->%zu",
btp->u.internal_key[index].first_page,
- btp->u.internal_key[index].child.relptr_off / FPM_PAGE_SIZE);
+ relptr_offset(btp->u.internal_key[index].child) / FPM_PAGE_SIZE);
else
appendStringInfo(buf, " %zu(%zu)",
btp->u.leaf_key[index].first_page,
@@ -1859,7 +1859,7 @@ FreePagePopSpanLeader(FreePageManager *fpm, Size pageno)
{
Size f = Min(span->npages, FPM_NUM_FREELISTS) - 1;
- Assert(fpm->freelist[f].relptr_off == pageno * FPM_PAGE_SIZE);
+ Assert(relptr_offset(fpm->freelist[f]) == pageno * FPM_PAGE_SIZE);
relptr_copy(fpm->freelist[f], span->next);
}
}
diff --git a/src/include/utils/freepage.h b/src/include/utils/freepage.h
index 08064b10f9..e69b2804ec 100644
--- a/src/include/utils/freepage.h
+++ b/src/include/utils/freepage.h
@@ -78,11 +78,11 @@ struct FreePageManager
#define fpm_pointer_is_page_aligned(base, ptr) \
(((Size) (((char *) (ptr)) - (base))) % FPM_PAGE_SIZE == 0)
#define fpm_relptr_is_page_aligned(base, relptr) \
- ((relptr).relptr_off % FPM_PAGE_SIZE == 0)
+ (relptr_offset(relptr) % FPM_PAGE_SIZE == 0)
/* Macro to find base address of the segment containing a FreePageManager. */
#define fpm_segment_base(fpm) \
- (((char *) fpm) - fpm->self.relptr_off)
+ (((char *) fpm) - relptr_offset(fpm->self))
/* Macro to access a FreePageManager's largest consecutive run of pages. */
#define fpm_largest(fpm) \
diff --git a/src/include/utils/relptr.h b/src/include/utils/relptr.h
index cc80a7200d..9364dd6694 100644
--- a/src/include/utils/relptr.h
+++ b/src/include/utils/relptr.h
@@ -42,7 +42,7 @@
#define relptr_access(base, rp) \
(AssertVariableIsOfTypeMacro(base, char *), \
(__typeof__((rp).relptr_type)) ((rp).relptr_off == 0 ? NULL : \
- (base + (rp).relptr_off)))
+ (base) + (rp).relptr_off - 1))
#else
/*
* If we don't have __builtin_types_compatible_p, assume we might not have
@@ -50,12 +50,15 @@
*/
#define relptr_access(base, rp) \
(AssertVariableIsOfTypeMacro(base, char *), \
- (void *) ((rp).relptr_off == 0 ? NULL : (base + (rp).relptr_off)))
+ (void *) ((rp).relptr_off == 0 ? NULL : (base) + (rp).relptr_off - 1))
#endif
#define relptr_is_null(rp) \
((rp).relptr_off == 0)
+#define relptr_offset(rp) \
+ ((rp).relptr_off - 1)
+
/* We use this inline to avoid double eval of "val" in relptr_store */
static inline Size
relptr_store_eval(char *base, char *val)
@@ -64,8 +67,8 @@ relptr_store_eval(char *base, char *val)
return 0;
else
{
- Assert(val > base);
- return val - base;
+ Assert(val >= base);
+ return val - base + 1;
}
}
@@ -73,7 +76,7 @@ relptr_store_eval(char *base, char *val)
#define relptr_store(base, rp, val) \
(AssertVariableIsOfTypeMacro(base, char *), \
AssertVariableIsOfTypeMacro(val, __typeof__((rp).relptr_type)), \
- (rp).relptr_off = relptr_store_eval(base, (char *) (val)))
+ (rp).relptr_off = relptr_store_eval((base), (char *) (val)))
#else
/*
* If we don't have __builtin_types_compatible_p, assume we might not have
@@ -81,7 +84,7 @@ relptr_store_eval(char *base, char *val)
*/
#define relptr_store(base, rp, val) \
(AssertVariableIsOfTypeMacro(base, char *), \
- (rp).relptr_off = relptr_store_eval(base, (char *) (val)))
+ (rp).relptr_off = relptr_store_eval((base), (char *) (val)))
#endif
#define relptr_copy(rp1, rp2) \
--
2.35.1
