Re: Recursive ReceiveSharedInvalidMessages not safe

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?

Will do.

> > b) we right/left shift -1 in a signed int by 16 in
> > CacheInvalidateSmgr/LocalExecuteInvalidationMessage(). IIRC that's
> > implementation defined behaviour.
>
> 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;

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

> > After far, far too much confused head scratching, code reading, random
> > elog()s et al I found out that this is just because of a deficiency in
> > valgrind's undefinedness tracking. [...]
> > Unfortunately this cannot precisely be caught by valgrind's
> > suppressions. Thus I'd like to add memset(SharedInvalidationMessage msg,
> > 0) in AddCatcacheInvalidationMessage() et al. to suppress these
> > warnings. Imo we can just add them unconditionally, but if somebody else
> > prefers we can add #ifdef USE_VALGRIND around them.
>
> I'd be okay with USE_VALGRIND. I'm not particularly hot on adding a
> memset for everybody just to make valgrind less confused. Especially
> since that's really going to hide any problems, not fix them.

I can't see it having any measureable overhead. Even old compilers will
optimize the memset() away and just initialize the padding... But
anyway, USE_VALGRIND is fine with me.

Greetings,

Andres Freund

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