Thread: Should pointers to PGPROC be volatile-qualified?

There are a bunch of places where we do things like
    PGPROC       *proc = arrayP->procs[index];    /* Fetch xid just once - see GetNewTransactionId */    TransactionId
pxid= proc->xid;
 
    ... use pxid several times ...

In the case where "use pxid" involves a function call, this is probably
safe enough, because the compiler can't assume that the called function
won't access and modify the PGPROC.  However, if "use pxid" is straight
line code, I am suddenly wondering if the C compiler could think it can
get away with loading proc->xid more than once instead of expending a
register on it.  As far as it knows there isn't any way for the value
to change underneath it.

The correct fix for this is probably to make the fetch happen through a
volatile-qualified pointer, eg
    volatile PGPROC *proc = arrayP->procs[index];    /* Fetch xid just once - see GetNewTransactionId */
TransactionIdpxid = proc->xid;
 

I'm wondering how far to go with that.  In the extreme we could try to
make MyProc and all other PGPROC pointers volatile-qualified; is that
overkill?

Or maybe I'm worried over nothing.  I can't recall any bug reports that
seem like they could be tied to such a thing, and given that we can only
set, not clear, MyProc->xid and xmin without exclusive lock, there might
not actually be a bug here.  But it seems a bit risky.

Comments?  Does anyone think the C standard forbids what I'm worried
about?
        regards, tom lane

Tom Lane wrote:

>Comments?  Does anyone think the C standard forbids what I'm worried
>about?
>  
>

My understanding of the C spec is that it explicitly *allows* for 
exactly what you're afraid of.  It's even possible if the uses include 
function calls, as the compiler might inline the function calls.

The downside of litering the code with volatile qualifications is that 
it's an optimization stopper.  For example, if proc is declared 
volatile, the compiler couldn't merge multiple different proc->foo 
references into a single load into a register.

Note that all sorts of weirdnesses are possible when you have shared 
mutable state between multiple different threads.  For example, assume 
you have two threads, and two global ints x and y, initially both 0.  
Thread 1 do:   y = 1;   r1 = x;
(where r1 is a local variable in thread 1), while thread 2 does:   x = 1;   r2 = y;
(with r2 being a local variable in thread 2).

Here's the thing: both r1 and r2 can end up 0!  I've seen this in real 
code.  What happens is that the compiler notices that in both cases, the 
load and stores are independent, so it can reorder them.  And as loads 
tend to be expensive, and nothing can progress until the load completes, 
it moves the loads up before the stores, assuming the program won't 
notice.  Unfortunately, it does, as "the impossible" can then happen.

Brian

Brian Hurt <bhurt@janestcapital.com> writes:
> Note that all sorts of weirdnesses are possible when you have shared 
> mutable state between multiple different threads.

Yeah.  In the majority of places this isn't a big problem because
access to shared memory looks like
LWLockAcquire(some_lock);... mess with shared state ...LWLockRelease(some_lock);

and as long as the LWLock functions are extern and not inlineable
all is well.  But we do need to be more careful in places where
we're violating that coding rule, and the various stuff that looks
at or changes xid and xmin is violating it.

What I'm inclined to do for now is put "volatile" into those places in
procarray.c where there seems to be a risk.
        regards, tom lane