Re: Recursive ReceiveSharedInvalidMessages not safe

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?

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

> c) The ProcessMessageList() calls access msg->rc.id to test for the type
> of the existing message. That's not nice.

Huh?

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

regards, tom lane