On 2014-05-05 15:41:22 -0400, Tom Lane wrote:
> Andres Freund <and...@2ndquadrant.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) 
have to be
                rnode.backend = ((int) msg->sm.backend_hi << 16) | (int) 

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


Andres Freund

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

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to