pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)
./tmp_install/usr/local/pgsql/bin/postgres -D ./src/test/regress/tmp_check/data -c min_dynamic_shared_memory=1MB
TRAP: FailedAssertion("val > base", File: "../../../../src/include/utils/relptr.h", Line: 67, PID: 21912)
./tmp_install/usr/local/pgsql/bin/postgres(ExceptionalCondition+0xa0)[0x55af5c9c463e]
./tmp_install/usr/local/pgsql/bin/postgres(FreePageManagerInitialize+0x94)[0x55af5c9f4478]
./tmp_install/usr/local/pgsql/bin/postgres(dsm_shmem_init+0x87)[0x55af5c841532]
./tmp_install/usr/local/pgsql/bin/postgres(CreateSharedMemoryAndSemaphores+0x8d)[0x55af5c843f30]
./tmp_install/usr/local/pgsql/bin/postgres(+0x41805c)[0x55af5c7c605c]
./tmp_install/usr/local/pgsql/bin/postgres(PostmasterMain+0x959)[0x55af5c7ca8e7]
./tmp_install/usr/local/pgsql/bin/postgres(main+0x229)[0x55af5c70af7f]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xe7)[0x7f736d6e1b97]
./tmp_install/usr/local/pgsql/bin/postgres(_start+0x2a)[0x55af5c4794fa]
It looks like this may be pre-existing problem exposed by
commit e07d4ddc55fdcf82082950b3eb0cd8f728284c9d
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sat Mar 26 14:29:29 2022 -0400
Suppress compiler warning in relptr_store().
Justin Pryzby <pryzby@telsasoft.com> writes:
./tmp_install/usr/local/pgsql/bin/postgres -D ./src/test/regress/tmp_check/data -c min_dynamic_shared_memory=1MB
TRAP: FailedAssertion("val > base", File: "../../../../src/include/utils/relptr.h", Line: 67, PID: 21912)
Yeah, I see it too.
It looks like this may be pre-existing problem exposed by
commit e07d4ddc55fdcf82082950b3eb0cd8f728284c9d
Agreed. Here I see
#5 FreePageManagerInitialize (fpm=fpm@entry=0x7f34b3ddd300,
base=base@entry=0x7f34b3ddd300 "") at freepage.c:187
#6 0x000000000082423e in dsm_shmem_init () at dsm.c:473
so that where we do
relptr_store(base, fpm->self, fpm);
the "relative" pointer value would have to be zero, making the case
indistinguishable from a NULL pointer. We can either change the
caller so that these addresses aren't the same, or give up the
ability to store NULL in relptrs ... doesn't seem like a hard call.
One interesting question I didn't look into is why it takes a nondefault
value of min_dynamic_shared_memory to expose this bug.
regards, tom lane
At Thu, 19 May 2022 17:16:03 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
Justin Pryzby <pryzby@telsasoft.com> writes:
./tmp_install/usr/local/pgsql/bin/postgres -D ./src/test/regress/tmp_check/data -c min_dynamic_shared_memory=1MB
TRAP: FailedAssertion("val > base", File: "../../../../src/include/utils/relptr.h", Line: 67, PID: 21912)Yeah, I see it too.
It looks like this may be pre-existing problem exposed by
commit e07d4ddc55fdcf82082950b3eb0cd8f728284c9dAgreed. Here I see
#5 FreePageManagerInitialize (fpm=fpm@entry=0x7f34b3ddd300,
base=base@entry=0x7f34b3ddd300 "") at freepage.c:187
#6 0x000000000082423e in dsm_shmem_init () at dsm.c:473so that where we do
relptr_store(base, fpm->self, fpm);
the "relative" pointer value would have to be zero, making the case
indistinguishable from a NULL pointer. We can either change the
caller so that these addresses aren't the same, or give up the
ability to store NULL in relptrs ... doesn't seem like a hard call.One interesting question I didn't look into is why it takes a nondefault
value of min_dynamic_shared_memory to expose this bug.
The path is taken only when a valid value is given to the
parameter. If we don't use preallocated dsm, it is initialized
elsewhere. In those cases the first bytes of the base address (the
second parameter of FreePageManagerInitialize) are used for
dsa_segment_header so the relptr won't be zero (!= NULL).
It can be silenced by wasting the first MAXALIGN bytes of
dsm_main_space_begin..
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
At Fri, 20 May 2022 12:00:14 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
At Thu, 19 May 2022 17:16:03 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
Justin Pryzby <pryzby@telsasoft.com> writes:
./tmp_install/usr/local/pgsql/bin/postgres -D ./src/test/regress/tmp_check/data -c min_dynamic_shared_memory=1MB
TRAP: FailedAssertion("val > base", File: "../../../../src/include/utils/relptr.h", Line: 67, PID: 21912)Yeah, I see it too.
It looks like this may be pre-existing problem exposed by
commit e07d4ddc55fdcf82082950b3eb0cd8f728284c9dAgreed. Here I see
#5 FreePageManagerInitialize (fpm=fpm@entry=0x7f34b3ddd300,
base=base@entry=0x7f34b3ddd300 "") at freepage.c:187
#6 0x000000000082423e in dsm_shmem_init () at dsm.c:473so that where we do
relptr_store(base, fpm->self, fpm);
the "relative" pointer value would have to be zero, making the case
indistinguishable from a NULL pointer. We can either change the
caller so that these addresses aren't the same, or give up the
ability to store NULL in relptrs ... doesn't seem like a hard call.One interesting question I didn't look into is why it takes a nondefault
value of min_dynamic_shared_memory to expose this bug.The path is taken only when a valid value is given to the
parameter. If we don't use preallocated dsm, it is initialized
elsewhere. In those cases the first bytes of the base address (the
second parameter of FreePageManagerInitialize) are used for
dsa_segment_header so the relptr won't be zero (!= NULL).It can be silenced by wasting the first MAXALIGN bytes of
dsm_main_space_begin..
Actually, that change doesn't result in wasting of usable memory size
since the change doesn't move the first effective page.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
allow-setting-min_dynamic_shared_memory.patchtext/x-patch; charset=us-asciiDownload
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index 9d86bbe872..c67134ec27 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -461,12 +461,19 @@ dsm_shmem_init(void)
dsm_main_space_begin = ShmemInitStruct("Preallocated DSM", size, &found);
if (!found)
{
- FreePageManager *fpm = (FreePageManager *) dsm_main_space_begin;
+ /*
+ * fpm cannot be the same with dsm_main_space_begin due to the
+ * restriction imposed by FreePageManagerInitialize() due to
+ * relptr. Add MAXALIGN(1) to fpm to live with that restriction.
+ */
+ int gap = MAXALIGN(1);
+ FreePageManager *fpm =
+ (FreePageManager *) ((char *)dsm_main_space_begin + gap);
size_t first_page = 0;
size_t pages;
/* Reserve space for the FreePageManager. */
- while (first_page * FPM_PAGE_SIZE < sizeof(FreePageManager))
+ while (first_page * FPM_PAGE_SIZE < sizeof(FreePageManager) + gap)
++first_page;
/* Initialize it and give it all the rest of the space. */
diff --git a/src/test/modules/test_misc/t/004_check_min_dsm_size.pl b/src/test/modules/test_misc/t/004_check_min_dsm_size.pl
new file mode 100644
index 0000000000..025dca33e4
--- /dev/null
+++ b/src/test/modules/test_misc/t/004_check_min_dsm_size.pl
@@ -0,0 +1,14 @@
+# Tests of min_dynamic_shared_memory
+
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More;
+
+my $node = PostgreSQL::Test::Cluster->new('main');
+$node->init;
+$node->append_conf('postgresql.conf', 'min_dynamic_shared_memory = 1MB');
+$node->start;
+pass('check if server can start with min_dynamic_shared_memory');
+done_testing();
On Thu, May 19, 2022 at 11:00 PM Kyotaro Horiguchi
<horikyota.ntt@gmail.com> wrote:
The path is taken only when a valid value is given to the
parameter. If we don't use preallocated dsm, it is initialized
elsewhere. In those cases the first bytes of the base address (the
second parameter of FreePageManagerInitialize) are used for
dsa_segment_header so the relptr won't be zero (!= NULL).It can be silenced by wasting the first MAXALIGN bytes of
dsm_main_space_begin..
Yeah, so when I created this stuff in the first place, I figured that
it wasn't a problem if we reserved relptr == 0 to mean a NULL pointer,
because you would never have a relative pointer pointing to the
beginning of a DSM, because it would probably always start with a
dsm_toc. But when Thomas made it so that DSM allocations could happen
in the main shared memory segment, that ceased to be true. This
example happened not to break because we never use relptr_access() on
fpm->self. We do use fpm_segment_base(), but that accidentally fails
to break, because instead of using relptr_access() it drills right
through the abstraction and doesn't have any kind of special case for
0. 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.
3. Dropping the assertion while loudly singing "la la la la la la".
--
Robert Haas
EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
Yeah, so when I created this stuff in the first place, I figured that
it wasn't a problem if we reserved relptr == 0 to mean a NULL pointer,
because you would never have a relative pointer pointing to the
beginning of a DSM, because it would probably always start with a
dsm_toc. But when Thomas made it so that DSM allocations could happen
in the main shared memory segment, that ceased to be true. This
example happened not to break because we never use relptr_access() on
fpm->self. We do use fpm_segment_base(), but that accidentally fails
to break, because instead of using relptr_access() it drills right
through the abstraction and doesn't have any kind of special case for
0.
Seems like that in itself is a a lousy idea. Either the code should
respect the abstraction, or it shouldn't be declaring the variable
as a relptr in the first place.
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.
3. Dropping the assertion while loudly singing "la la la la la la".
I'm definitely down on #3, because that just leaves the ambiguity
in place to bite somewhere else in future. #1 would work as long
as nobody expects memset-to-zero to produce null relptrs, but that
doesn't seem very nice either.
On the whole, wasting MAXALIGN worth of memory seems like the least bad
alternative, but I wonder if we ought to do it right here as opposed
to somewhere in the DSM code proper. Why is this DSM space not like
other DSM spaces in starting with a TOC?
regards, tom lane
On Wed, Jun 1, 2022 at 8:10 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
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.
3. Dropping the assertion while loudly singing "la la la la la la".I'm definitely down on #3, because that just leaves the ambiguity
in place to bite somewhere else in future. #1 would work as long
as nobody expects memset-to-zero to produce null relptrs, but that
doesn't seem very nice either.On the whole, wasting MAXALIGN worth of memory seems like the least bad
alternative, but I wonder if we ought to do it right here as opposed
to somewhere in the DSM code proper. Why is this DSM space not like
other DSM spaces in starting with a TOC?
This FPM isn't in a DSM. (It happens to have DSMs *inside it*,
because I'm using it as a separate DSM allocator: instead of making
them with dsm_impl.c mechanisms, this one recycles space from the main
shmem area). I view FPM as a reusable 4kb page-based memory allocator
that could have many potential uses, not as a thing that must live
inside another thing with a TOC. The fact that it uses the relptr
thing makes it possible to use FPM inside DSMs too, but that doesn't
mean it has to be used inside a DSM.
I vote for #1.
On Tue, May 31, 2022 at 4:10 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Seems like that in itself is a a lousy idea. Either the code should
respect the abstraction, or it shouldn't be declaring the variable
as a relptr in the first place.
Yep. I think it should be respecting the abstraction, but the 2016
version of me failed to realize the issue when committing 13e14a78ea1.
Hindsight is 20-20, perhaps.
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.
3. Dropping the assertion while loudly singing "la la la la la la".I'm definitely down on #3, because that just leaves the ambiguity
in place to bite somewhere else in future. #1 would work as long
as nobody expects memset-to-zero to produce null relptrs, but that
doesn't seem very nice either.
Well, that's a good point that I hadn't considered, actually. I was
thinking I'd only picked 0 as the value out of adherence to
convention, but I might have had this in mind too, at the time.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Tue, May 31, 2022 at 4:32 PM Thomas Munro <thomas.munro@gmail.com> wrote:
This FPM isn't in a DSM. (It happens to have DSMs *inside it*,
because I'm using it as a separate DSM allocator: instead of making
them with dsm_impl.c mechanisms, this one recycles space from the main
shmem area). I view FPM as a reusable 4kb page-based memory allocator
that could have many potential uses, not as a thing that must live
inside another thing with a TOC. The fact that it uses the relptr
thing makes it possible to use FPM inside DSMs too, but that doesn't
mean it has to be used inside a DSM.
Could it use something other than its own address as the base address?
One way to do this would be to put it at the *end* of the
"Preallocated DSM" space, rather than the beginning.
--
Robert Haas
EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
Could it use something other than its own address as the base address?
Hmm, maybe we could make something of that idea ...
One way to do this would be to put it at the *end* of the
"Preallocated DSM" space, rather than the beginning.
... but that way doesn't sound good. Doesn't it just move the
problem to the first object allocated inside the FPM?
regards, tom lane
On Wed, Jun 1, 2022 at 9:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
Could it use something other than its own address as the base address?
Hmm, maybe we could make something of that idea ...
One way to do this would be to put it at the *end* of the
"Preallocated DSM" space, rather than the beginning.... but that way doesn't sound good. Doesn't it just move the
problem to the first object allocated inside the FPM?
Count we make the relptrs 1-based, so that 0 is reserved as a sentinel
that has the nice memset(0) property?
Thomas Munro <thomas.munro@gmail.com> writes:
Count we make the relptrs 1-based, so that 0 is reserved as a sentinel
that has the nice memset(0) property?
Hm ... almost. A +1 offset would mean that zero is ambiguous with a
pointer to the byte just before the relptr. Maybe that case never
arises in practice, but now that we've seen this problem I'm not real
comfortable with such an assumption. But how about a -1 offset?
Then zero would be ambiguous with a pointer to the second byte of the
relptr, and I think I *am* prepared to assume that that has no use-cases.
The other advantage of such a definition is that it'd help flush out
anybody breaking the relptr abstraction ;-)
regards, tom lane
On Tue, May 31, 2022 at 5:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@gmail.com> writes:
Count we make the relptrs 1-based, so that 0 is reserved as a sentinel
that has the nice memset(0) property?Hm ... almost. A +1 offset would mean that zero is ambiguous with a
pointer to the byte just before the relptr. Maybe that case never
arises in practice, but now that we've seen this problem I'm not real
comfortable with such an assumption. But how about a -1 offset?
Then zero would be ambiguous with a pointer to the second byte of the
relptr, and I think I *am* prepared to assume that that has no use-cases.The other advantage of such a definition is that it'd help flush out
anybody breaking the relptr abstraction ;-)
Seems backwards to me. A relative pointer is supposed to point to
something inside some range of memory, like a DSM gment -- it can
never be legally used to point to anything outside that segment. So it
seems to me that you could perfectly legally point to the second byte
of the segment, but never to the -1'th byte.
--
Robert Haas
EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
Seems backwards to me. A relative pointer is supposed to point to
something inside some range of memory, like a DSM gment -- it can
never be legally used to point to anything outside that segment. So it
seems to me that you could perfectly legally point to the second byte
of the segment, but never to the -1'th byte.
Okay, I was thinking about it slightly wrong: relptr is defined as an
offset relative to some base address, not to its own address. As long
as you're prepared to assume that the base address really is the start
of the addressable area, then yeah the above argument works.
However, now that I've corrected that mistaken image ... I wonder if
it could make sense to redefine relptr as self-relative? That ought
to provide some notational savings since you'd only need to carry
around the relptr's own address not that plus a base address.
Probably not something to consider for v15 though.
regards, tom lane
On Tue, May 31, 2022 at 6:14 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
However, now that I've corrected that mistaken image ... I wonder if
it could make sense to redefine relptr as self-relative? That ought
to provide some notational savings since you'd only need to carry
around the relptr's own address not that plus a base address.
Probably not something to consider for v15 though.
I think that would be pretty hard to make work, since copying around a
relative pointer would change its meaning. Code like "relptr_foo x =
*y" would be broken, for example, but the compiler would not complain.
Or maybe I misunderstand your idea?
Also keep in mind that the major use case here is DSM segments, which
can be mapped at different addresses in different processes. Mainly,
we expect to store relative pointers in the segment to other things in
the same segment. Sometimes, we might read the values from there into
local variables - or maybe global variables - in code that is
accessing those DSM segments for some purpose.
There is little use for a relative pointer that can access all of the
address space that exists. For that, it is better to just as a regular
pointer.
--
Robert Haas
EDB: http://www.enterprisedb.com
Robert Haas <robertmhaas@gmail.com> writes:
On Tue, May 31, 2022 at 6:14 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
However, now that I've corrected that mistaken image ... I wonder if
it could make sense to redefine relptr as self-relative? That ought
to provide some notational savings since you'd only need to carry
around the relptr's own address not that plus a base address.
Probably not something to consider for v15 though.
I think that would be pretty hard to make work, since copying around a
relative pointer would change its meaning. Code like "relptr_foo x =
*y" would be broken, for example, but the compiler would not complain.
Sure, but the current definition is far from error-proof as well:
nothing stops you from using the wrong base address with a relptr's
value. Anyway, it's just idle speculation at this point.
regards, tom lane
At Tue, 31 May 2022 16:10:05 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
tgl> Robert Haas <robertmhaas@gmail.com> writes:
tgl> > Yeah, so when I created this stuff in the first place, I figured that
tgl> > it wasn't a problem if we reserved relptr == 0 to mean a NULL pointer,
tgl> > because you would never have a relative pointer pointing to the
tgl> > beginning of a DSM, because it would probably always start with a
tgl> > dsm_toc. But when Thomas made it so that DSM allocations could happen
tgl> > in the main shared memory segment, that ceased to be true. This
tgl> > example happened not to break because we never use relptr_access() on
tgl> > fpm->self. We do use fpm_segment_base(), but that accidentally fails
tgl> > to break, because instead of using relptr_access() it drills right
tgl> > through the abstraction and doesn't have any kind of special case for
tgl> > 0.
tgl>
tgl> Seems like that in itself is a a lousy idea. Either the code should
tgl> respect the abstraction, or it shouldn't be declaring the variable
tgl> as a relptr in the first place.
tgl>
tgl> > So we can fix this by:
tgl> > 1. Using a relative pointer value other than 0 to represent a null
tgl> > pointer. Andres suggested (Size) -1.
tgl> > 2. Not storing the free page manager for the DSM in the main shared
tgl> > memory segment at byte offset 0.
tgl> > 3. Dropping the assertion while loudly singing "la la la la la la".
tgl>
tgl> I'm definitely down on #3, because that just leaves the ambiguity
tgl> in place to bite somewhere else in future. #1 would work as long
tgl> as nobody expects memset-to-zero to produce null relptrs, but that
tgl> doesn't seem very nice either.
tgl>
tgl> On the whole, wasting MAXALIGN worth of memory seems like the least bad
tgl> alternative, but I wonder if we ought to do it right here as opposed
tgl> to somewhere in the DSM code proper. Why is this DSM space not like
tgl> other DSM spaces in starting with a TOC?
tgl>
tgl> regards, tom lane
At Tue, 31 May 2022 15:57:14 -0400, Robert Haas <robertmhaas@gmail.com> wrote in
1. Using a relative pointer value other than 0 to represent a null
pointer. Andres suggested (Size) -1.
I thought that relptr as a part of DSM so the use of offset=0 is
somewhat illegal. But I like this. We can fix this by this
modification. I think ((Size) -1) is natural to signal something
special. (I see glibc uses "(size_t) -1".)
2. Not storing the free page manager for the DSM in the main shared
memory segment at byte offset 0.
3. Dropping the assertion while loudly singing "la la la la la la".
reagards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Attachments:
make-relptr-allow-zero-address.patchtext/x-patch; charset=us-asciiDownload
diff --git a/src/include/utils/relptr.h b/src/include/utils/relptr.h
index cc80a7200d..c6d39a1360 100644
--- a/src/include/utils/relptr.h
+++ b/src/include/utils/relptr.h
@@ -41,7 +41,7 @@
#ifdef HAVE__BUILTIN_TYPES_COMPATIBLE_P
#define relptr_access(base, rp) \
(AssertVariableIsOfTypeMacro(base, char *), \
- (__typeof__((rp).relptr_type)) ((rp).relptr_off == 0 ? NULL : \
+ (__typeof__((rp).relptr_type)) ((rp).relptr_off == ((Size) -1) ? NULL : \
(base + (rp).relptr_off)))
#else
/*
@@ -50,21 +50,21 @@
*/
#define relptr_access(base, rp) \
(AssertVariableIsOfTypeMacro(base, char *), \
- (void *) ((rp).relptr_off == 0 ? NULL : (base + (rp).relptr_off)))
+ (void *) ((rp).relptr_off == ((Size) -1) ? NULL : (base + (rp).relptr_off)))
#endif
#define relptr_is_null(rp) \
- ((rp).relptr_off == 0)
+ ((rp).relptr_off == ((Size) -1))
/* We use this inline to avoid double eval of "val" in relptr_store */
static inline Size
relptr_store_eval(char *base, char *val)
{
if (val == NULL)
- return 0;
+ return ((Size) -1);
else
{
- Assert(val > base);
+ Assert(val >= base);
return val - base;
}
}
At Wed, 01 Jun 2022 11:42:01 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in
me> At Tue, 31 May 2022 16:10:05 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
me> tgl> Robert Haas <robertmhaas@gmail.com> writes:
me> tgl> > Yeah, so when I created this stuff in the first place, I figured that
me> tgl> > it wasn't a problem if we reserved relptr == 0 to mean a NULL pointer,
Mmm. Sorry. It's just an accidental shooting.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
On Wed, Jun 1, 2022 at 2:57 AM Robert Haas <robertmhaas@gmail.com> wrote:
We do use fpm_segment_base(), but that accidentally fails
to break, because instead of using relptr_access() it drills right
through the abstraction and doesn't have any kind of special case for
0. 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.
Hi all,
For this open item, the above two ideas were discussed as a short-term
fix, and my reading of the thread is that the other proposals are too
invasive at this point in the cycle. Both of them have a draft patch
in the thread. #2, i.e. wasting MAXALIGN of space, seems the simplest
and most localized. Any thoughts on pulling the trigger on either of
these two approaches?
--
John Naylor
EDB: http://www.enterprisedb.com
John Naylor <john.naylor@enterprisedb.com> writes:
On Wed, Jun 1, 2022 at 2:57 AM Robert Haas <robertmhaas@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 this open item, the above two ideas were discussed as a short-term
fix, and my reading of the thread is that the other proposals are too
invasive at this point in the cycle. Both of them have a draft patch
in the thread. #2, i.e. wasting MAXALIGN of space, seems the simplest
and most localized. Any thoughts on pulling the trigger on either of
these two approaches?
I'm still of the opinion that 0 == NULL is a good property to have,
so I vote for #2.
regards, tom lane
On Wed, Jun 22, 2022 at 2:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
John Naylor <john.naylor@enterprisedb.com> writes:
On Wed, Jun 1, 2022 at 2:57 AM Robert Haas <robertmhaas@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.
Attachments:
0001-Fix-relptr-s-encoding-of-NULL.patchtext/x-patch; charset=US-ASCII; name=0001-Fix-relptr-s-encoding-of-NULL.patchDownload
From 8b6b1e765b4dd23b968563aab1a01fa8b7c5a463 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@gmail.com>
Date: Wed, 22 Jun 2022 16:18:53 +1200
Subject: [PATCH] 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 | 13 ++++++++-----
3 files changed, 13 insertions(+), 10 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..79f4f0095d 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)
@@ -65,7 +68,7 @@ relptr_store_eval(char *base, char *val)
else
{
Assert(val > base);
- return 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
On Wed, Jun 22, 2022 at 4:24 PM Thomas Munro <thomas.munro@gmail.com> wrote:
On Wed, Jun 22, 2022 at 2:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
John Naylor <john.naylor@enterprisedb.com> writes:
On Wed, Jun 1, 2022 at 2:57 AM Robert Haas <robertmhaas@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.
Attachments:
v2-0001-Fix-relptr-s-encoding-of-NULL.patchtext/x-patch; charset=US-ASCII; name=v2-0001-Fix-relptr-s-encoding-of-NULL.patchDownload
From ba1661745ad57c69d0d9f881913d337504b5e870 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@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
On Wed, Jun 22, 2022 at 12:34 AM Thomas Munro <thomas.munro@gmail.com> wrote:
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.
I like this idea and think this might have the side benefit of making
it harder to get away with accessing relptr_off directly.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Thu, Jun 23, 2022 at 2:09 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Jun 22, 2022 at 12:34 AM Thomas Munro <thomas.munro@gmail.com> wrote:
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.
I like this idea and think this might have the side benefit of making
it harder to get away with accessing relptr_off directly.
Thanks. Pushed, and back-patched to 14, where
min_dynamic_shared_memory arrived.
I wondered in passing if the stuff about relptr_declare() was still
needed to avoid confusing pgindent, since we tweaked the indent code a
bit for macros that take a typename, but it seems that it still
mangles "relptr(FooBar) some_struct_member;", putting extra whitespace
in front of it. Hmmph.