Обсуждение: Obsolete use of volatile in walsender.c, walreceiver.c, walreceiverfuncs.c?

Hi

In walsender.c, walreceiver.c, walreceiverfuncs.c there are several
places where volatile qualifiers are used apparently only to prevent
reordering around spinlock operations.  My understanding is that if
potential load/store reordering around spinlock operations is the only
reason for using volatile, 0709b7ee72e4bc71ad07b7120acd117265ab51d0
made it unnecessary.  For example see
6ba4ecbf477e0b25dd7bde1b0c4e07fc2da19348 which stripped some volatile
qualifiers out of xlog.c.

I did notice that sometimes walsnd->pid is read without acquiring the
spinlock.  Is that actually OK anyway (taking a stale and inconsistent
view of the contents of walsnd->pid WRT to the other members that are
later accessed while holding the spinlock)?

Would it be safe to remove all those volatile qualifiers, something
like in the attached, or am I missing something?

(There is also code in syncrep.c that is reading shmem without
acquiring spinlocks using volatile qualifiers, that is a different
situation, though I don't yet see how it is ordering sensitive or
reading the same object repeatedly, but I'm not talking about that
here).

--
Thomas Munro
http://www.enterprisedb.com

Thomas Munro wrote:

> In walsender.c, walreceiver.c, walreceiverfuncs.c there are several
> places where volatile qualifiers are used apparently only to prevent
> reordering around spinlock operations.

In replication/slot.c there are a number of places (12, I think) that
introduce a block specifically to contain a volatile cast on a variable
for spinlock-protected access.  We could remove the whole thing and save
at least 3 lines and one indentation level for each of them.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Sep 22, 2015 at 8:19 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Thomas Munro wrote:
>
>> In walsender.c, walreceiver.c, walreceiverfuncs.c there are several
>> places where volatile qualifiers are used apparently only to prevent
>> reordering around spinlock operations.
>
> In replication/slot.c there are a number of places (12, I think) that
> introduce a block specifically to contain a volatile cast on a variable
> for spinlock-protected access.  We could remove the whole thing and save
> at least 3 lines and one indentation level for each of them.

Right, see attached.

--
Thomas Munro
http://www.enterprisedb.com

Hi,

On 2015-09-17 16:32:17 +1200, Thomas Munro wrote:
> In walsender.c, walreceiver.c, walreceiverfuncs.c there are several
> places where volatile qualifiers are used apparently only to prevent
> reordering around spinlock operations.  My understanding is that if
> potential load/store reordering around spinlock operations is the only
> reason for using volatile, 0709b7ee72e4bc71ad07b7120acd117265ab51d0
> made it unnecessary.  For example see
> 6ba4ecbf477e0b25dd7bde1b0c4e07fc2da19348 which stripped some volatile
> qualifiers out of xlog.c.

Same in bufmgr.c et al. There it's actually rather annoying for new code
because volatile needs to be casted away in a bunch of places...

Greetings,

Andres Freund

On Tue, Sep 22, 2015 at 7:25 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Tue, Sep 22, 2015 at 8:19 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> Thomas Munro wrote:
>>
>>> In walsender.c, walreceiver.c, walreceiverfuncs.c there are several
>>> places where volatile qualifiers are used apparently only to prevent
>>> reordering around spinlock operations.
>>
>> In replication/slot.c there are a number of places (12, I think) that
>> introduce a block specifically to contain a volatile cast on a variable
>> for spinlock-protected access.  We could remove the whole thing and save
>> at least 3 lines and one indentation level for each of them.
>
> Right, see attached.

It seems to me that we could as well simplify checkpoint.c and
logical.c. In those files volatile casts are used as well to protect
from reordering for spinlock operations. See for example 0002 on top
of 0001 that is Thomas' patch.
--
Michael

On Thu, Oct 1, 2015 at 11:01 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
>> Right, see attached.
>
> It seems to me that we could as well simplify checkpoint.c and
> logical.c. In those files volatile casts are used as well to protect
> from reordering for spinlock operations. See for example 0002 on top
> of 0001 that is Thomas' patch.

These patches look good to me, so I have committed them.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Oct 7, 2015 at 8:52 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Oct 1, 2015 at 11:01 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>>> Right, see attached.
>>
>> It seems to me that we could as well simplify checkpoint.c and
>> logical.c. In those files volatile casts are used as well to protect
>> from reordering for spinlock operations. See for example 0002 on top
>> of 0001 that is Thomas' patch.
>
> These patches look good to me, so I have committed them.

Thanks.  Also, spin.h's comment contains an out of date warning about
this.  Here's a suggested fix for that, and a couple more volatrivia
patches.

--
Thomas Munro
http://www.enterprisedb.com

On Fri, Oct 16, 2015 at 9:07 AM, Thomas Munro
<thomas.munro@enterprisedb.com> wrote:
> On Wed, Oct 7, 2015 at 8:52 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Thu, Oct 1, 2015 at 11:01 PM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>>> Right, see attached.
>>>
>>> It seems to me that we could as well simplify checkpoint.c and
>>> logical.c. In those files volatile casts are used as well to protect
>>> from reordering for spinlock operations. See for example 0002 on top
>>> of 0001 that is Thomas' patch.
>>
>> These patches look good to me, so I have committed them.
>
> Thanks.  Also, spin.h's comment contains an out of date warning about
> this.  Here's a suggested fix for that, and a couple more volatrivia
> patches.

I have looked at the rest of the code, and it seems that we can get
rid of volatile in a couple of extra places like in the attached as
those are used with spin locks. This applies on top of Thomas' set.
--
Michael

On Thu, Oct 15, 2015 at 10:36 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Oct 16, 2015 at 9:07 AM, Thomas Munro
> <thomas.munro@enterprisedb.com> wrote:
>> On Wed, Oct 7, 2015 at 8:52 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Thu, Oct 1, 2015 at 11:01 PM, Michael Paquier
>>> <michael.paquier@gmail.com> wrote:
>>>>> Right, see attached.
>>>>
>>>> It seems to me that we could as well simplify checkpoint.c and
>>>> logical.c. In those files volatile casts are used as well to protect
>>>> from reordering for spinlock operations. See for example 0002 on top
>>>> of 0001 that is Thomas' patch.
>>>
>>> These patches look good to me, so I have committed them.
>>
>> Thanks.  Also, spin.h's comment contains an out of date warning about
>> this.  Here's a suggested fix for that, and a couple more volatrivia
>> patches.
>
> I have looked at the rest of the code, and it seems that we can get
> rid of volatile in a couple of extra places like in the attached as
> those are used with spin locks. This applies on top of Thomas' set.

OK, committed his, and yours.

I back-patched his spin.h comment fix to 9.5 since that's a factual
error, but the rest of this seems like optimization so I committed it
only to master.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Oct 17, 2015 at 3:21 AM, Robert Haas wrote:
> OK, committed his, and yours.
>
> I back-patched his spin.h comment fix to 9.5 since that's a factual
> error, but the rest of this seems like optimization so I committed it
> only to master.

That sounds right. Thanks!
-- 
Michael