Horton

Thread

Commits

  1. Fix relptr's encoding of the base address.

  2. Suppress compiler warning in relptr_store().

  1. pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Justin Pryzby <pryzby@telsasoft.com> — 2022-05-19T19:38:39Z

    ./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().

  2. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-05-19T21:16:03Z

    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

  3. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-05-20T03:00:14Z

    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 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.

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

  4. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-05-31T05:05:45Z

    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 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.
> 
> 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

  5. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Robert Haas <robertmhaas@gmail.com> — 2022-05-31T19:57:14Z

    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

  6. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-05-31T20:10:05Z

    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

  7. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Thomas Munro <thomas.munro@gmail.com> — 2022-05-31T20:32:07Z

    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.

  8. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Robert Haas <robertmhaas@gmail.com> — 2022-05-31T20:58:24Z

    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

  9. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Robert Haas <robertmhaas@gmail.com> — 2022-05-31T21:01:04Z

    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

  10. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-05-31T21:09:27Z

    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

  11. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Thomas Munro <thomas.munro@gmail.com> — 2022-05-31T21:26:30Z

    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?

  12. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-05-31T21:52:09Z

    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

  13. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Robert Haas <robertmhaas@gmail.com> — 2022-05-31T22:04:36Z

    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

  14. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-05-31T22:14:13Z

    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

  15. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Robert Haas <robertmhaas@gmail.com> — 2022-05-31T22:29:14Z

    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

  16. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-05-31T22:39:23Z

    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

  17. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-06-01T02:42:01Z

    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

  18. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-06-01T02:51:33Z

    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

  19. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2022-06-01T02:53:12Z

    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

  20. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    John Naylor <john.naylor@enterprisedb.com> — 2022-06-22T02:49:50Z

    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

  21. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-06-22T02:54:47Z

    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

  22. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Thomas Munro <thomas.munro@gmail.com> — 2022-06-22T04:24:06Z

    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.

  23. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Thomas Munro <thomas.munro@gmail.com> — 2022-06-22T04:33:44Z

    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.

  24. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Robert Haas <robertmhaas@gmail.com> — 2022-06-22T14:09:28Z

    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

  25. Re: pg15b1: FailedAssertion("val > base", File: "...src/include/utils/relptr.h", Line: 67, PID: 30485)

    Thomas Munro <thomas.munro@gmail.com> — 2022-06-27T01:14:08Z

    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.