Re: Support for N synchronous standby servers - take 2

On Tue, Sep 15, 2015 at 3:19 PM, Thomas Munro
<thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> I got the following error from clang-602.0.53 on my Mac:
>
> walsender.c:1955:11: error: passing 'char volatile[8192]' to parameter of
> type 'void *' discards qualifiers
> [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
> memcpy(walsnd->name, application_name,
> strlen(application_name));
> ^~~~~~~~~~~~
>
> I think your memcpy and explicit null termination could be replaced with
> strcpy, or maybe something to limit buffer overrun damage in case of sizing
> bugs elsewhere. But to get rid of that warning you'd still need to cast
> away volatile... I note that you do that in SyncRepGetQuorumRecPtr when you
> read the string with strcmp. But is that actually safe, with respect to
> load/store reordering around spinlock operations? Do we actually need
> volatile-preserving cstring copy and compare functions for this type of
> thing?

Maybe volatile isn't even needed here at all. I have asked that
question separately here:

http://www.postgresql.org/message-id/CAEepm=2f-N5MD+xYYyO=yBpC9SoOdCdrdiKia9_oLTSiu1uBtA@mail.gmail.com

In SyncRepGetQuorumRecPtr you have strcmp(node->name, (char *)
walsnd->name): that might be more problematic. I'm not sure about
casting away volatile (it's probably fine at least in practice), but
it's accessing walsnd without the the spinlock. The existing
syncrep.c code already did that sort of thing (and I haven't had time
to grok the thinking behind that yet), but I think you may be upping
the ante here by doing non-atomic reads with strcmp (whereas the code
in master always read single word values). Imagine if you hit a slot
that was being set up by InitWalSenderSlot concurrently, and memcpy
was in the process of writing the name. strcmp would read garbage,
maybe even off the end of the buffer because there is no terminator
yet. That may be incredibly unlikely, but it seems fishy. Or I may
have misunderstood the synchronisation at work here completely :-)

--
Thomas Munro
http://www.enterprisedb.com