Horton

Thread

Commits

  1. Code review for shm_toc.h/.c.

  2. Don't be so trusting that shm_toc_lookup() will always succeed.

  1. shm_toc_lookup API

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-05T15:19:41Z

    shm_toc_lookup() returns NULL if it fails to find the desired key.
Out of the 30 or so call sites, there is exactly one that has any
use for that.  Some of the rest have Asserts that they get back
a non-null result, but the majority just blithely dereference the
pointer.  I do not find this cool at all; given that we're accessing
a shared data structure, we should assign more than zero probability
to the idea that we might not find what we expect to find when we
expect to find it.

I think that shm_toc_lookup() ought to be made to throw elog(ERROR) on an
unexpected failure.  To satisfy the one caller that doesn't want an error,
we could either add a "bool noError" parameter to it, or split it into two
functions shm_toc_lookup() and shm_toc_lookup_noerror().  The latter would
require touching less code, but the former is probably more like what we'd
have had if this were designed more carefully to begin with.

Any preferences?

			regards, tom lane

  2. Re: shm_toc_lookup API

    Robert Haas <robertmhaas@gmail.com> — 2017-06-05T15:36:40Z

    On Mon, Jun 5, 2017 at 11:19 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> shm_toc_lookup() returns NULL if it fails to find the desired key.
> Out of the 30 or so call sites, there is exactly one that has any
> use for that.  Some of the rest have Asserts that they get back
> a non-null result, but the majority just blithely dereference the
> pointer.  I do not find this cool at all; given that we're accessing
> a shared data structure, we should assign more than zero probability
> to the idea that we might not find what we expect to find when we
> expect to find it.
>
> I think that shm_toc_lookup() ought to be made to throw elog(ERROR) on an
> unexpected failure.  To satisfy the one caller that doesn't want an error,
> we could either add a "bool noError" parameter to it, or split it into two
> functions shm_toc_lookup() and shm_toc_lookup_noerror().  The latter would
> require touching less code, but the former is probably more like what we'd
> have had if this were designed more carefully to begin with.

Either is OK with me.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  3. Re: shm_toc_lookup API

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-05T16:19:01Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jun 5, 2017 at 11:19 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think that shm_toc_lookup() ought to be made to throw elog(ERROR) on an
>> unexpected failure.  To satisfy the one caller that doesn't want an error,
>> we could either add a "bool noError" parameter to it, or split it into two
>> functions shm_toc_lookup() and shm_toc_lookup_noerror().  The latter would
>> require touching less code, but the former is probably more like what we'd
>> have had if this were designed more carefully to begin with.

> Either is OK with me.

Done with the extra parameter.  If we discover a reason to back-patch
this, we'd likely need to do it the other way in back branches, but
for now I'll refrain.

While I'm looking at this ... seems like there's a pretty basic coding-
rule violation here, namely that shm_toc_lookup() thinks it can read
toc->toc_nentry without any sort of locking.  Since that field is declared
Size, this amounts to an assumption that 64-bit reads are atomic, which
is not per project practice.

In practice it probably can't fail even if 64-bit reads aren't atomic,
simply because we'll never have enough entries in a shm_toc to make the
high-order half ever change.  But that just begs the question why the
field is declared Size rather than int.  I think we should make it the
latter.

I am also thinking that most of the shm_toc functions need to have the
toc pointers declared as "volatile *", but particularly shm_toc_lookup.
That read_barrier call might prevent the hardware from reordering
accesses, but I don't think it stops the compiler from doing so.

These problems are probably just latent, because it looks to me like
in our current usage no process would ever be doing lookups on shm_tocs
that are being changed concurrently.  But they'll bite us someday.

			regards, tom lane

  4. Re: shm_toc_lookup API

    Robert Haas <robertmhaas@gmail.com> — 2017-06-05T17:10:44Z

    On Mon, Jun 5, 2017 at 12:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> While I'm looking at this ... seems like there's a pretty basic coding-
> rule violation here, namely that shm_toc_lookup() thinks it can read
> toc->toc_nentry without any sort of locking.  Since that field is declared
> Size, this amounts to an assumption that 64-bit reads are atomic, which
> is not per project practice.
>
> In practice it probably can't fail even if 64-bit reads aren't atomic,
> simply because we'll never have enough entries in a shm_toc to make the
> high-order half ever change.  But that just begs the question why the
> field is declared Size rather than int.  I think we should make it the
> latter.

Yeah.  I think a shm_toc with more than 2^10 entries would probably
perform badly enough that somebody would rewrite this entire module,
so we don't really need to worry about having more than 2^31.
Changing to int (or uint32) seems fine.

> I am also thinking that most of the shm_toc functions need to have the
> toc pointers declared as "volatile *", but particularly shm_toc_lookup.
> That read_barrier call might prevent the hardware from reordering
> accesses, but I don't think it stops the compiler from doing so.

If it doesn't prevent both the hardware and the compiler from
reordering, it's broken.  See the comments for pg_read_barrier() in
atomics.h.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  5. Re: shm_toc_lookup API

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-05T18:57:10Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jun 5, 2017 at 12:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> In practice it probably can't fail even if 64-bit reads aren't atomic,
>> simply because we'll never have enough entries in a shm_toc to make the
>> high-order half ever change.  But that just begs the question why the
>> field is declared Size rather than int.  I think we should make it the
>> latter.

> Yeah.  I think a shm_toc with more than 2^10 entries would probably
> perform badly enough that somebody would rewrite this entire module,
> so we don't really need to worry about having more than 2^31.
> Changing to int (or uint32) seems fine.

Done with uint32.

>> I am also thinking that most of the shm_toc functions need to have the
>> toc pointers declared as "volatile *", but particularly shm_toc_lookup.

(actually, they do already use volatile pointers, except for shm_toc_lookup)

>> That read_barrier call might prevent the hardware from reordering
>> accesses, but I don't think it stops the compiler from doing so.

> If it doesn't prevent both the hardware and the compiler from
> reordering, it's broken.  See the comments for pg_read_barrier() in
> atomics.h.

Meh.  Without volatile, I think that the compiler would be within its
rights to elide the nentry local variable and re-fetch toc->toc_nentry
each time through the loop.  It'd be unlikely to do so, granted, but
I'm not convinced that pg_read_barrier() would prevent that.

However, as long as the write barrier in shm_toc_insert does what it's
supposed to, I think we'd be safe even if that happened.  So probably
it's a moot point.

			regards, tom lane

  6. Re: shm_toc_lookup API

    Andres Freund <andres@anarazel.de> — 2017-06-05T19:02:49Z

    On 2017-06-05 14:57:10 -0400, Tom Lane wrote:
> > If it doesn't prevent both the hardware and the compiler from
> > reordering, it's broken.  See the comments for pg_read_barrier() in
> > atomics.h.
> 
> Meh.  Without volatile, I think that the compiler would be within its
> rights to elide the nentry local variable and re-fetch toc->toc_nentry
> each time through the loop.

I don't think that's true. Excerption from the docs of the macros:
About pg_read_barrier()
 * A read barrier must act as a compiler barrier, and in addition must
About pg_compiler_barrier():
 * A compiler barrier need not (and preferably should not) emit any actual
 * machine code, but must act as an optimization fence: the compiler must not
 * reorder loads or stores to main memory around the barrier.  However, the
 * CPU may still reorder loads or stores at runtime, if the architecture's
 * memory model permits this.
 */

Given that I don't see how it'd be permissible to elide the local
variable.  Are you saying that's permitted, or that our implementations
don't guarantee that?

- Andres

  7. Re: shm_toc_lookup API

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-05T19:06:06Z

    Andres Freund <andres@anarazel.de> writes:
> On 2017-06-05 14:57:10 -0400, Tom Lane wrote:
>> Meh.  Without volatile, I think that the compiler would be within its
>> rights to elide the nentry local variable and re-fetch toc->toc_nentry
>> each time through the loop.

> I don't think that's true.

Perhaps not, but I'm not quite convinced.  Anyway it doesn't matter,
at least not here.

			regards, tom lane

  8. Re: shm_toc_lookup API

    Andres Freund <andres@anarazel.de> — 2017-06-05T19:10:05Z

    On 2017-06-05 15:06:06 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2017-06-05 14:57:10 -0400, Tom Lane wrote:
> >> Meh.  Without volatile, I think that the compiler would be within its
> >> rights to elide the nentry local variable and re-fetch toc->toc_nentry
> >> each time through the loop.
> 
> > I don't think that's true.
> 
> Perhaps not, but I'm not quite convinced.  Anyway it doesn't matter,
> at least not here.

If not, we have a bunch of bugs in other files.  So if there are
concerns, we should better resolve them.

- Andres