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) 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers