Re: Recursive ReceiveSharedInvalidMessages not safe

On 2014-05-05 18:50:59 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2014-05-05 15:41:22 -0400, Tom Lane wrote:
> >> Looks all right to me. Yeah, the right shift might have undefined
> >> high-order bits, but we don't care because we're storing the result
> >> into an int16.
>
> > Doesn't at the very least
> > rnode.backend = (msg->sm.backend_hi << 16) | (int) msg->sm.backend_lo;
> > have to be
> > rnode.backend = ((int) msg->sm.backend_hi << 16) | (int) msg->sm.backend_lo;
>
> A promotion to int (or wider) is implicit in any arithmetic operation,
> last I checked the C standard. The "(int)" on the other side isn't
> necessary either.

Done in the attached patch.

> We might actually be better off casting both sides to unsigned int,
> just to enforce that the left shifting is done with unsigned semantics.
>
> >>> c) The ProcessMessageList() calls access msg->rc.id to test for the type
> >>> of the existing message. That's not nice.
>
> >> Huh?
>
> > The checks should access msg->id, not msg->rc.id...
>
> Meh. If they're not the same thing, we've got big trouble anyway.
> But if you want to change it as a cosmetic thing, no objection.

Changed as well.

On 2014-05-05 15:41:22 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > a) SICleanupQueue() sometimes releases and reacquires the write lock
> > held on the outside. That's pretty damn fragile, not to mention
> > ugly. Even slight reformulations of the code in SIInsertDataEntries()
> > can break this... Can we please extend the comment in the latter that
> > to mention the lock dropping explicitly?
>
> Want to write a better comment?

Edited the comment and, in a perhaps debatable fashion, moved some
variable declarations + volatile into the retry loop for some added
cozyness. If the compiler inlines the cleanup function it could very
well decide to fetch some of the variables only once.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services