On Oct19, 2011, at 18:05 , Greg Jaskiewicz wrote:
> On 19 Oct 2011, at 17:54, Florian Pflug wrote:
>> On Oct19, 2011, at 17:47 , Greg Jaskiewicz wrote:
>>> On 15 Oct 2011, at 11:31, Florian Pflug wrote:
>>>> Ok, here's a first cut.
>>> So I looked at the patch, and first thing that pops out, 
>>> is lack of the volatile keyword before the ClientConnectionLostPending 
>>> variable is defined. Is that done on purpose ? Is that on purpose ?
>> That's on purpose. volatile is only necessary for variables which are either 
>> accessed from within signal handlers or which live in shared memory. Neither 
>> is true for ClientConnectionLostPending, so non-volatile should be fine.
> Ok, cool.
> I'm aware of the reasons behind volatile, just noticed that some other flags 
> used in similar way are marked as such. At the end of the day, this is just a 
> hint to the compiler anyway. 

All the other flags which indicate cancellation reasons are set from signal 
handers, I believe. We could of course mark as ClientConnectionLostPending as 
volatile just to be consistent. Not sure whether that's a good idea, or not. It 
might prevent a mistake should we ever add code to detect lost connections 
asynchronously (i.e., from somewhere else than pq_flush). And the cost is 
probably negligible, because CHECK_FOR_INTERRUPTS tests for InterruptPending 
before calling ProcessInterrupts(), so we only pay the cost of volatile if 
there's actually an interrupt pending. But I still think it's better to add 
qualifies such a volatile only when really necessary. A comment about why it 
*isn't* volatile is probably in order, though, so I'll add that in the next 
version of the patch.

best regards,
Florian Pflug

PS: Thanks for the review. It's very much appreciated!

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

Reply via email to