From: | Florian Pflug <fgp(at)phlo(dot)org> |
---|---|
To: | Greg Jaskiewicz <gj(at)pointblue(dot)com(dot)pl> |
Cc: | Magnus Hagander <magnus(at)hagander(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Bug in walsender when calling out to do_pg_stop_backup (and others?) |
Date: | 2011-10-19 16:28:38 |
Message-ID: | EABFD036-F792-4BE6-B8AD-03B418CE4D23@phlo.org |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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!
From | Date | Subject | |
---|---|---|---|
Next Message | Robert Haas | 2011-10-19 16:34:12 | Re: [PATCH] Log crashed backend's query v3 |
Previous Message | Tom Lane | 2011-10-19 16:28:28 | Re: new compiler warnings |