From: Tom Lane <tgl@sss.pgh.pa.us>
To: Robert Haas <robertmhaas@gmail.com>
Cc: pgsql-hackers@postgresql.org <pgsql-hackers@postgresql.org>
Subject: Re: shm_toc_lookup API
Date: Mon, 05 Jun 2017 12:19:01 -0400
Message-ID: <16984.1496679541@sss.pgh.pa.us> (raw)
In-Reply-To: <CA+TgmoZ_GAT21HLZsw=whXaj3uN_=bez420cRLP0149RPtvS_Q@mail.gmail.com>
References: <9697.1496675981@sss.pgh.pa.us>
<CA+TgmoZ_GAT21HLZsw=whXaj3uN_=bez420cRLP0149RPtvS_Q@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, robertmhaas@gmail.com
Subject: Re: shm_toc_lookup API
In-Reply-To: <16984.1496679541@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox