Re: Recursive ReceiveSharedInvalidMessages not safe

Andres Freund <andres@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?

> 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