Обсуждение: Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );

On 3/27/15 2:23 PM, Fabrízio de Royes Mello wrote:
> Hi all,
>
> I'm tweaking some autovacuum settings in a table with high write usage
> but with ALTER TABLE .. SET ( .. ) this task was impossible, so I did a
> catalog update  (pg_class) to change reloptions.
>
> Maybe it's a stupid doubt, but why we need to get an AccessExclusiveLock
> on relation to set reloptions if we just touch in pg_class tuples
> (RowExclusiveLock) ?

For a very long time catalog access was not MVCC safe. I think that's 
been changed, so at this point it may be OK to relax the lock, at least 
in the case of autovac settings. There may well be other settings in 
there where it would not be safe.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

<div dir="ltr"><br /><br />On Mon, Mar 30, 2015 at 7:41 PM, Jim Nasby <<a
href="mailto:Jim.Nasby@bluetreble.com">Jim.Nasby@bluetreble.com</a>>wrote:<br />><br />> On 3/27/15 2:23 PM,
Fabríziode Royes Mello wrote:<br />>><br />>> Hi all,<br />>><br />>> I'm tweaking some
autovacuumsettings in a table with high write usage<br />>> but with ALTER TABLE .. SET ( .. ) this task was
impossible,so I did a<br />>> catalog update  (pg_class) to change reloptions.<br />>><br />>> Maybe
it'sa stupid doubt, but why we need to get an AccessExclusiveLock<br />>> on relation to set reloptions if we
justtouch in pg_class tuples<br />>> (RowExclusiveLock) ?<br />><br />><br />> For a very long time
catalogaccess was not MVCC safe. I think that's been changed, so at this point it may be OK to relax the lock, at least
inthe case of autovac settings. There may well be other settings in there where it would not be safe.<br />><br
/><br/>Hummm.... There are a comment in AlterTableGetLockLevel:<br /><br /> 3017                 /*<br /> 3018        
        * Rel options are more complex than first appears. Options<br /> 3019                  * are set here for
tables,views and indexes; for historical<br /> 3020                  * reasons these can all be used with ALTER TABLE,
sowe can't<br /> 3021                  * decide between them using the basic grammar.<br /> 3022                  *<br
/> 3023                 * XXX Look in detail at each option to determine lock level,<br /> 3024                  * e.g.
cmd_lockmode= GetRelOptionsLockLevel((List *)<br /> 3025                  * cmd->def);<br /> 3026                
 */<br/> 3027             case AT_SetRelOptions:      /* Uses MVCC in getIndexes() and<br /> 3028                      
                  * getTables() */<br /> 3029             case AT_ResetRelOptions:    /* Uses MVCC in getIndexes()
and<br/> 3030                                          * getTables() */<br /> 3031                 cmd_lockmode =
AccessExclusiveLock;<br/> 3032                 break;<br /><br /><br />Maybe it's time to implement
"GetRelOptionsLockLevel"to relax the lock to autovac settings (AccessShareLock). To other settings we continue using
AccessExclusiveLock.<br/><br />There are some objection to implement in that way?<br /><br />Regards,<br /><br />--<br
/>Fabríziode Royes Mello<br />Consultoria/Coaching PostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/>>> Blog: <a
href="http://fabriziomello.github.io">http://fabriziomello.github.io</a><br/>>> Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a><br/>>> Github: <a
href="http://github.com/fabriziomello">http://github.com/fabriziomello</a></div>

On Tue, Mar 31, 2015 at 9:11 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
> Attached a very WIP patch to reduce lock level when setting autovacuum
> reloptions in "ALTER TABLE .. SET ( .. )" statement.

I think the first thing we need to here is analyze all of the options
and determine what the appropriate lock level is for each, and why.

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

On Tue, Mar 31, 2015 at 01:17:03PM -0400, Robert Haas wrote:
> On Tue, Mar 31, 2015 at 9:11 AM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
> > Attached a very WIP patch to reduce lock level when setting autovacuum
> > reloptions in "ALTER TABLE .. SET ( .. )" statement.
> 
> I think the first thing we need to here is analyze all of the options
> and determine what the appropriate lock level is for each, and why.

Agreed.  Fabrízio, see this message for the discussion that led to the code
comment you found (search for "relopt_gen"):
 http://www.postgresql.org/message-id/20140321034556.GA3927180@tornado.leadboat.com

Fabrízio de Royes Mello wrote:

> Ok guys. The attached patch refactor the reloptions adding a new field
> "lockmode" in "relopt_gen" struct and a new method to determine the
> required lock level from an option list.
> 
> We need determine the appropriate lock level for each reloption:

I don't think AccessShareLock is appropriate for any option change.  You
should be using a lock level that's self-conflicting, as a minimum
requirement, to avoid two processes changing the value concurrently.  (I
would probably go as far as ensuring that the lock level specified in
the table DoLockModesConflict() with itself in an Assert somewhere.)

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

<div dir="ltr"><div class="gmail_extra">On Mon, Apr 6, 2015 at 12:53 AM, Alvaro Herrera <<a
href="mailto:alvherre@2ndquadrant.com"target="_blank">alvherre@2ndquadrant.com</a>> wrote:<br />><br />>
Fabríziode Royes Mello wrote:<br />><br />> > Ok guys. The attached patch refactor the reloptions adding a new
field<br/>> > "lockmode" in "relopt_gen" struct and a new method to determine the<br />> > required lock
levelfrom an option list.<br />> ><br />> > We need determine the appropriate lock level for each
reloption:<br/>><br />> I don't think AccessShareLock is appropriate for any option change.  You<br />> should
beusing a lock level that's self-conflicting, as a minimum<br />> requirement, to avoid two processes changing the
valueconcurrently.  <br /><br /></div><div class="gmail_extra">What locklevel do you suggest? Maybe ShareLock?<br
/></div><divclass="gmail_extra"><br /><br />> (I would probably go as far as ensuring that the lock level specified
in<br/>> the table DoLockModesConflict() with itself in an Assert somewhere.)<br />><br /><br /></div><div
class="gmail_extra">IfI understood this is to check if the locklevel of the reloption list don't conflict one each
other,is it?<br /></div><div class="gmail_extra"><br /></div><div class="gmail_extra">Regards,<br /><br /></div><div
class="gmail_extra">--<br/>Fabrízio de Royes Mello<br />Consultoria/Coaching PostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br"target="_blank">http://www.timbira.com.br</a><br />>> Blog: <a
href="http://fabriziomello.github.io"target="_blank">http://fabriziomello.github.io</a><br />>> Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello"target="_blank">http://br.linkedin.com/in/fabriziomello</a><br />>>
Twitter:<a href="http://twitter.com/fabriziomello" target="_blank">http://twitter.com/fabriziomello</a><br />>>
Github:<a href="http://github.com/fabriziomello" target="_blank">http://github.com/fabriziomello</a></div></div> 

Fabrízio de Royes Mello wrote:
> On Mon, Apr 6, 2015 at 12:53 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
> wrote:
> >
> > Fabrízio de Royes Mello wrote:
> >
> > > Ok guys. The attached patch refactor the reloptions adding a new field
> > > "lockmode" in "relopt_gen" struct and a new method to determine the
> > > required lock level from an option list.
> > >
> > > We need determine the appropriate lock level for each reloption:
> >
> > I don't think AccessShareLock is appropriate for any option change.  You
> > should be using a lock level that's self-conflicting, as a minimum
> > requirement, to avoid two processes changing the value concurrently.
> 
> What locklevel do you suggest? Maybe ShareLock?

ShareUpdateExclusive probably.  ShareLock doesn't conflict with itself.

> > (I would probably go as far as ensuring that the lock level specified in
> > the table DoLockModesConflict() with itself in an Assert somewhere.)
> 
> If I understood this is to check if the locklevel of the reloption list
> don't conflict one each other, is it?

I meanAssert(DoLockModesConflict(relopt_gen->locklevel, relopt_gen->locklevel));

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

<div dir="ltr"><div class="gmail_extra"><br />On Tue, Apr 7, 2015 at 10:15 PM, Alvaro Herrera <<a
href="mailto:alvherre@2ndquadrant.com">alvherre@2ndquadrant.com</a>>wrote:<br />><br />> Fabrízio de Royes
Mellowrote:<br />> > On Mon, Apr 6, 2015 at 12:53 AM, Alvaro Herrera <<a
href="mailto:alvherre@2ndquadrant.com">alvherre@2ndquadrant.com</a>><br/>> > wrote:<br />> > ><br
/>>> > Fabrízio de Royes Mello wrote:<br />> > ><br />> > > > Ok guys. The attached patch
refactorthe reloptions adding a new field<br />> > > > "lockmode" in "relopt_gen" struct and a new method
todetermine the<br />> > > > required lock level from an option list.<br />> > > ><br />>
>> > We need determine the appropriate lock level for each reloption:<br />> > ><br />> > >
Idon't think AccessShareLock is appropriate for any option change.  You<br />> > > should be using a lock
levelthat's self-conflicting, as a minimum<br />> > > requirement, to avoid two processes changing the value
concurrently.<br/>> ><br />> > What locklevel do you suggest? Maybe ShareLock?<br />><br />>
ShareUpdateExclusiveprobably.  ShareLock doesn't conflict with itself.<br />><br /><br /></div><div
class="gmail_extra">Ok.<br/><br /></div><div class="gmail_extra"><br />> > > (I would probably go as far as
ensuringthat the lock level specified in<br />> > > the table DoLockModesConflict() with itself in an Assert
somewhere.)<br/>> ><br />> > If I understood this is to check if the locklevel of the reloption list<br
/>>> don't conflict one each other, is it?<br />><br />> I mean<br />>        
Assert(DoLockModesConflict(relopt_gen->locklevel,relopt_gen->locklevel));<br />><br /><br /></div><div
class="gmail_extra">Understood...IMHO the better place to perform this assert is in "initialize_reloptions".<br
/></div><divclass="gmail_extra"><br /></div><div class="gmail_extra">Thoughts?<br /><br /></div><div
class="gmail_extra">--<br/>Fabrízio de Royes Mello<br />Consultoria/Coaching PostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/>>> Blog: <a
href="http://fabriziomello.github.io">http://fabriziomello.github.io</a><br/>>> Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a><br/>>> Github: <a
href="http://github.com/fabriziomello">http://github.com/fabriziomello</a></div></div>

The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:       not tested
Spec compliant:           not tested
Documentation:            not tested

Looks functionally complete

Need a test to show that ALTER TABLE works on views, as discussed on this thread. And confirmation that pg_dump is not
brokenby this.
 

Message-ID:    20140321205828.GB3969106@tornado.leadboat.com

Needs documentation

The new status of this patch is: Waiting on Author

On Fri, Jul 24, 2015 at 7:11 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
> On Thu, Jul 2, 2015 at 2:03 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> Looks functionally complete
>>
>> Need a test to show that ALTER TABLE works on views, as discussed on this
>> thread. And confirmation that pg_dump is not broken by this.
>>
>> Message-ID:     20140321205828.GB3969106@tornado.leadboat.com
>>
>
> Added more test cases to cover ALTER TABLE on views.
>
> I'm thinking about the isolation tests, what about add another 'alter-table'
> spec for isolation tests enabling and disabling 'autovacuum' options?

Yes, please.

> I did some tests using ALTER TABLE on views and also ALTER VIEW and I didn't
> identify any anomalies.
>
>> Needs documentation
>>
>
> Added.
       for (i = 0; boolRelOpts[i].gen.name; i++)
+       {
+
Assert(DoLockModesConflict(boolRelOpts[i].gen.lockmode,
boolRelOpts[i].gen.lockmode));               j++;
+       }       for (i = 0; intRelOpts[i].gen.name; i++)
+       {
+               Assert(DoLockModesConflict(intRelOpts[i].gen.lockmode,
intRelOpts[i].gen.lockmode));               j++;
+       }       for (i = 0; realRelOpts[i].gen.name; i++)
+       {
+
Assert(DoLockModesConflict(realRelOpts[i].gen.lockmode,
realRelOpts[i].gen.lockmode));               j++;
+       }       for (i = 0; stringRelOpts[i].gen.name; i++)
+       {
+
Assert(DoLockModesConflict(stringRelOpts[i].gen.lockmode,
stringRelOpts[i].gen.lockmode));               j++;
+       }
Splitting those long lines into two will avoid some work for pgindent.

+GetRelOptionsLockLevel(List *defList)
+{
+       LOCKMODE    lockmode = NoLock;
Shouldn't this default to AccessExclusiveLock instead of NoLock?
--
Michael

On Fri, Jul 31, 2015 at 8:30 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
>
> On Fri, Jul 24, 2015 at 4:05 AM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>>
>> On Fri, Jul 24, 2015 at 7:11 AM, Fabrízio de Royes Mello
>> <fabriziomello@gmail.com> wrote:
>> > On Thu, Jul 2, 2015 at 2:03 PM, Simon Riggs <simon@2ndquadrant.com>
>> > wrote:
>> >> Looks functionally complete
>> >>
>> >> Need a test to show that ALTER TABLE works on views, as discussed on
>> >> this
>> >> thread. And confirmation that pg_dump is not broken by this.
>> >>
>> >> Message-ID:     20140321205828.GB3969106@tornado.leadboat.com
>> >>
>> >
>> > Added more test cases to cover ALTER TABLE on views.
>> >
>> > I'm thinking about the isolation tests, what about add another
>> > 'alter-table'
>> > spec for isolation tests enabling and disabling 'autovacuum' options?
>>
>> Yes, please.
>>
>
> Added. I really don't know if my isolation tests are completely correct, is
> my first time writing this kind of tests.

This patch size has increased from 16k to 157k because of the output
of the isolation tests you just added. This is definitely too large
and actually the test coverage is rather limited. Hence I think that
they should be changed as follows:
- Use only one table, the locks of tables a and b do not conflict, and
that's what we want to look at here.
- Check the locks of all the relation parameters, by that I mean as
well fillfactor and user_catalog_table which still take
AccessExclusiveLock on the relation
- Use custom permutations. I doubt that there is much sense to test
all the permutations in this context, and this will reduce the
expected output size drastically.

On further notice, I would recommend not to use the same string name
for the session and the query identifiers. And I think that inserting
only one tuple at initialization is just but fine.

Here is an example of such a spec:
setup
{CREATE TABLE a (id int PRIMARY KEY);INSERT INTO a SELECT generate_series(1,100);
}
teardown
{DROP TABLE a;
}
session "s1"
step "b1"       { BEGIN; }
# TODO add one query per parameter
step "at11"     { ALTER TABLE a SET (fillfactor=10); }
step "at12"     { ALTER TABLE a SET (autovacuum_vacuum_scale_factor=0.001); }
step "c1"       { COMMIT; }
session "s2"
step "b2"       { BEGIN; }
step "wx1"      { UPDATE a SET id = id + 10000; }
step "c2"       { COMMIT; }

And by testing permutations like for example that it is possible to
see which session is waiting for what. Input:
permutation "b1" "b2" "at11" "wx1" "c1" "c2"
Output where session 2 waits for lock taken after fillfactor update:
step b1: BEGIN;
step b2: BEGIN;
step at11: ALTER TABLE a SET (fillfactor=10);
step wx1: UPDATE a SET id = id + 10000; <waiting ...>
step c1: COMMIT;
step wx1: <... completed>
step c2: COMMIT;

Be careful as well to not include incorrect permutations in the
expected output file, the isolation tester is smart enough to ping you
about that.

>> +GetRelOptionsLockLevel(List *defList)
>> +{
>> +       LOCKMODE    lockmode = NoLock;
>> Shouldn't this default to AccessExclusiveLock instead of NoLock?
>>
>
> No, because it will break the logic bellow (get the highest locklevel
> according the defList), but I force return an AccessExclusiveLock if
> "defList == NIL".

Yep, OK with this change.

The rest of the patch looks good to me, so once the isolation test
coverage is done I think that it could be put in the hands of a
committer.
Regards
--
Michael

> @@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =

If we go through this list, I'd rather make informed decisions about
each reloption. Otherwise we're going to get patches for each of them
separately over the next versions.

> @@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
>          {
>              "fastupdate",
>              "Enables \"fast update\" feature for this GIN index",
> -            RELOPT_KIND_GIN
> +            RELOPT_KIND_GIN,
> +            AccessExclusiveLock
>          },
>          true
>      },


> @@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
>          {
>              "fillfactor",
>              "Packs table pages only to this percentage",
> -            RELOPT_KIND_HEAP
> +            RELOPT_KIND_HEAP,
> +            AccessExclusiveLock
>          },
>          HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100

> [some other fillfactor settings]

>      {
>          {
>              "gin_pending_list_limit",
>              "Maximum size of the pending list for this GIN index, in kilobytes.",
> -            RELOPT_KIND_GIN
> +            RELOPT_KIND_GIN,
> +            AccessExclusiveLock
>          },
>          -1, 64, MAX_KILOBYTES
>      },
> @@ -297,7 +325,8 @@ static relopt_string stringRelOpts[] =
>          {
>              "buffering",
>              "Enables buffering build for this GiST index",
> -            RELOPT_KIND_GIST
> +            RELOPT_KIND_GIST,
> +            AccessExclusiveLock
>          },
>          4,
>          false,

Why? These options just change things for the future and don't influence
past decisions. It seems unproblematic to change them.

> @@ -259,7 +283,8 @@ static relopt_real realRelOpts[] =
>          {
>              "seq_page_cost",
>              "Sets the planner's estimate of the cost of a sequentially fetched disk page.",
> -            RELOPT_KIND_TABLESPACE
> +            RELOPT_KIND_TABLESPACE,
> +            AccessExclusiveLock
>          },
>          -1, 0.0, DBL_MAX
>      },
> @@ -267,7 +292,8 @@ static relopt_real realRelOpts[] =
>          {
>              "random_page_cost",
>              "Sets the planner's estimate of the cost of a nonsequentially fetched disk page.",
> -            RELOPT_KIND_TABLESPACE
> +            RELOPT_KIND_TABLESPACE,
> +            AccessExclusiveLock
>          },
>          -1, 0.0, DBL_MAX
>      },
> @@ -275,7 +301,8 @@ static relopt_real realRelOpts[] =
>          {
>              "n_distinct",
>              "Sets the planner's estimate of the number of distinct values appearing in a column (excluding child
relations).",
> -            RELOPT_KIND_ATTRIBUTE
> +            RELOPT_KIND_ATTRIBUTE,
> +            AccessExclusiveLock
>          },
>          0, -1.0, DBL_MAX
>      },
> @@ -283,7 +310,8 @@ static relopt_real realRelOpts[] =
>          {
>              "n_distinct_inherited",
>              "Sets the planner's estimate of the number of distinct values appearing in a column (including child
relations).",
> -            RELOPT_KIND_ATTRIBUTE
> +            RELOPT_KIND_ATTRIBUTE,
> +            AccessExclusiveLock
>          },
>          0, -1.0, DBL_MAX
>      },

These probably are the settings that are most interesting to change
without access exlusive locks.

>      j = 0;
>      for (i = 0; boolRelOpts[i].gen.name; i++)
> +    {
> +        Assert(DoLockModesConflict(boolRelOpts[i].gen.lockmode,
> +                                   boolRelOpts[i].gen.lockmode));
>          j++;
> +    }
>      for (i = 0; intRelOpts[i].gen.name; i++)
> +    {
> +        Assert(DoLockModesConflict(intRelOpts[i].gen.lockmode,
> +                                   intRelOpts[i].gen.lockmode));
>          j++;
> +    }
>      for (i = 0; realRelOpts[i].gen.name; i++)
> +    {
> +        Assert(DoLockModesConflict(realRelOpts[i].gen.lockmode,
> +                                   realRelOpts[i].gen.lockmode));
>          j++;
> +    }
>      for (i = 0; stringRelOpts[i].gen.name; i++)
> +    {
> +        Assert(DoLockModesConflict(stringRelOpts[i].gen.lockmode,
> +                                   stringRelOpts[i].gen.lockmode));
>          j++;
> +    }
>      j += num_custom_options;

Doesn't really seem worth it to assert individually in each case here to
me.

> +/*
> + * Determine the required LOCKMODE from an option list
> + */
> +LOCKMODE
> +GetRelOptionsLockLevel(List *defList)
> +{
> +    LOCKMODE    lockmode = NoLock;
> +    ListCell    *cell;
> +
> +    if (defList == NIL)
> +        return AccessExclusiveLock;
> +
> +    if (need_initialization)
> +        initialize_reloptions();
> +
> +    foreach(cell, defList)
> +    {
> +        DefElem *def = (DefElem *) lfirst(cell);
> +        int        i;
> +
> +        for (i = 0; relOpts[i]; i++)
> +        {
> +            if (pg_strncasecmp(relOpts[i]->name, def->defname, relOpts[i]->namelen + 1) == 0)
> +            {
> +                if (lockmode < relOpts[i]->lockmode)
> +                    lockmode = relOpts[i]->lockmode;
> +            }
> +        }
> +    }
> +
> +    return lockmode;
> +}

We usually don't compare lock values that way, i.e. there's not
guaranteed to be a strict monotonicity between lock levels. I don't
really agree with that policy, but it's nonetheless there.

Greetings,

Andres Freund

<div dir="ltr"><br />On Thu, Jul 30, 2015 at 11:28 PM, Andres Freund <<a
href="mailto:andres@anarazel.de">andres@anarazel.de</a>>wrote:<br />><br />> > @@ -57,7 +57,8 @@ static
relopt_boolboolRelOpts[] =<br />><br />> If we go through this list, I'd rather make informed decisions about<br
/>>each reloption. Otherwise we're going to get patches for each of them<br />> separately over the next
versions.<br/>><br /><br />I have no problem to do this change now instead of wait for next versions...<br /><br
/><br/>> > @@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =<br />> >               {<br />> >    
                 "fastupdate",<br />> >                       "Enables \"fast update\" feature for this GIN
index",<br/>> > -                     RELOPT_KIND_GIN<br />> > +                     RELOPT_KIND_GIN,<br
/>>> +                     AccessExclusiveLock<br />> >               },<br />> >              
true<br/>> >       },<br />><br />><br />> > @@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =<br
/>>>               {<br />> >                       "fillfactor",<br />> >                      
"Packstable pages only to this percentage",<br />> > -                     RELOPT_KIND_HEAP<br />> > +    
               RELOPT_KIND_HEAP,<br />> > +                     AccessExclusiveLock<br />> >              
},<br/>> >               HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100<br />><br />> > [some other
fillfactorsettings]<br />><br />> >       {<br />> >               {<br />> >                    
 "gin_pending_list_limit",<br />> >                       "Maximum size of the pending list for this GIN index,
inkilobytes.",<br />> > -                     RELOPT_KIND_GIN<br />> > +                    
RELOPT_KIND_GIN,<br/>> > +                     AccessExclusiveLock<br />> >               },<br />> >
             -1, 64, MAX_KILOBYTES<br />> >       },<br />> > @@ -297,7 +325,8 @@ static relopt_string
stringRelOpts[]=<br />> >               {<br />> >                       "buffering",<br />> >      
               "Enables buffering build for this GiST index",<br />> > -                     RELOPT_KIND_GIST<br
/>>> +                     RELOPT_KIND_GIST,<br />> > +                     AccessExclusiveLock<br />>
>              },<br />> >               4,<br />> >               false,<br />><br />> Why? These
optionsjust change things for the future and don't influence<br />> past decisions. It seems unproblematic to change
them.<br/>><br /><br />+1<br /><br /><br />> > @@ -259,7 +283,8 @@ static relopt_real realRelOpts[] =<br
/>>>               {<br />> >                       "seq_page_cost",<br />> >                      
"Setsthe planner's estimate of the cost of a sequentially fetched disk page.",<br />> > -                    
RELOPT_KIND_TABLESPACE<br/>> > +                     RELOPT_KIND_TABLESPACE,<br />> > +                    
AccessExclusiveLock<br/>> >               },<br />> >               -1, 0.0, DBL_MAX<br />> >      
},<br/>> > @@ -267,7 +292,8 @@ static relopt_real realRelOpts[] =<br />> >               {<br />> >  
                   "random_page_cost",<br />> >                       "Sets the planner's estimate of the cost of
anonsequentially fetched disk page.",<br />> > -                     RELOPT_KIND_TABLESPACE<br />> > +    
               RELOPT_KIND_TABLESPACE,<br />> > +                     AccessExclusiveLock<br />> >        
     },<br />> >               -1, 0.0, DBL_MAX<br />> >       },<br />> > @@ -275,7 +301,8 @@ static
relopt_realrealRelOpts[] =<br />> >               {<br />> >                       "n_distinct",<br />>
>                      "Sets the planner's estimate of the number of distinct values appearing in a column
(excludingchild relations).",<br />> > -                     RELOPT_KIND_ATTRIBUTE<br />> > +              
     RELOPT_KIND_ATTRIBUTE,<br />> > +                     AccessExclusiveLock<br />> >               },<br
/>>>               0, -1.0, DBL_MAX<br />> >       },<br />> > @@ -283,7 +310,8 @@ static relopt_real
realRelOpts[]=<br />> >               {<br />> >                       "n_distinct_inherited",<br />>
>                      "Sets the planner's estimate of the number of distinct values appearing in a column
(includingchild relations).",<br />> > -                     RELOPT_KIND_ATTRIBUTE<br />> > +              
     RELOPT_KIND_ATTRIBUTE,<br />> > +                     AccessExclusiveLock<br />> >               },<br
/>>>               0, -1.0, DBL_MAX<br />> >       },<br />><br />> These probably are the settings
thatare most interesting to change<br />> without access exlusive locks.<br />><br /><br />+1<br /><br /><br
/>>>       j = 0;<br />> >       for (i = 0; boolRelOpts[i].<a href="http://gen.name">gen.name</a>; i++)<br
/>>> +     {<br />> > +             Assert(DoLockModesConflict(boolRelOpts[i].gen.lockmode,<br />> >
+                                                               boolRelOpts[i].gen.lockmode));<br />> >          
   j++;<br />> > +     }<br />> >       for (i = 0; intRelOpts[i].<a href="http://gen.name">gen.name</a>;
i++)<br/>> > +     {<br />> > +             Assert(DoLockModesConflict(intRelOpts[i].gen.lockmode,<br
/>>> +                                                                intRelOpts[i].gen.lockmode));<br />>
>              j++;<br />> > +     }<br />> >       for (i = 0; realRelOpts[i].<a
href="http://gen.name">gen.name</a>;i++)<br />> > +     {<br />> > +            
Assert(DoLockModesConflict(realRelOpts[i].gen.lockmode,<br/>> > +                                                
              realRelOpts[i].gen.lockmode));<br />> >               j++;<br />> > +     }<br />> >  
   for (i = 0; stringRelOpts[i].<a href="http://gen.name">gen.name</a>; i++)<br />> > +     {<br />> > +  
         Assert(DoLockModesConflict(stringRelOpts[i].gen.lockmode,<br />> > +                                    
                          stringRelOpts[i].gen.lockmode));<br />> >               j++;<br />> > +     }<br
/>>>       j += num_custom_options;<br />><br />> Doesn't really seem worth it to assert individually in
eachcase here to<br />> me.<br />><br /><br />What do you suggest then?<br /><br /><br /><br />> > +/*<br
/>>> + * Determine the required LOCKMODE from an option list<br />> > + */<br />> > +LOCKMODE<br
/>>> +GetRelOptionsLockLevel(List *defList)<br />> > +{<br />> > +     LOCKMODE    lockmode =
NoLock;<br/>> > +     ListCell    *cell;<br />> > +<br />> > +     if (defList == NIL)<br />> >
+            return AccessExclusiveLock;<br />> > +<br />> > +     if (need_initialization)<br />> >
+            initialize_reloptions();<br />> > +<br />> > +     foreach(cell, defList)<br />> > +    
{<br/>> > +             DefElem *def = (DefElem *) lfirst(cell);<br />> > +             int            
i;<br/>> > +<br />> > +             for (i = 0; relOpts[i]; i++)<br />> > +             {<br />>
>+                     if (pg_strncasecmp(relOpts[i]->name, def->defname, relOpts[i]->namelen + 1) == 0)<br
/>>> +                     {<br />> > +                             if (lockmode <
relOpts[i]->lockmode)<br/>> > +                                     lockmode = relOpts[i]->lockmode;<br
/>>> +                     }<br />> > +             }<br />> > +     }<br />> > +<br />>
>+     return lockmode;<br />> > +}<br />><br />> We usually don't compare lock values that way, i.e.
there'snot<br />> guaranteed to be a strict monotonicity between lock levels. I don't<br />> really agree with
thatpolicy, but it's nonetheless there.<br />><br /><br />And how is the better way to compare lock values to get
thehighest lock level? Perhaps creating a function to compare lock levels?<br /><br />Regards,<br /><br />*** This work
isfunded by Zenvia Mobile Results (<a href="http://www.zenvia.com.br">http://www.zenvia.com.br</a>)<br /><br />--<br
/>Fabríziode Royes Mello<br />Consultoria/Coaching PostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/>>> Blog: <a
href="http://fabriziomello.github.io">http://fabriziomello.github.io</a><br/>>> Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a><br/>>> Github: <a
href="http://github.com/fabriziomello">http://github.com/fabriziomello</a></div>

On Fri, Jul 31, 2015 at 11:28 AM, Andres Freund <andres@anarazel.de> wrote:
>
>> @@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
>
> If we go through this list, I'd rather make informed decisions about
> each reloption. Otherwise we're going to get patches for each of them
> separately over the next versions.

Just dropping quickly a reply: I meant table relopts only, excluding
the index stuff for now regarding the isolation tests.

>> +                     AccessExclusiveLock
>> +     foreach(cell, defList)
>> +     {
>> +             DefElem *def = (DefElem *) lfirst(cell);
>> +             int             i;
>> +
>> +             for (i = 0; relOpts[i]; i++)
>> +             {
>> +                     if (pg_strncasecmp(relOpts[i]->name, def->defname, relOpts[i]->namelen + 1) == 0)
>> +                     {
>> +                             if (lockmode < relOpts[i]->lockmode)
>> +                                     lockmode = relOpts[i]->lockmode;
>> +                     }
>> +             }
>> +     }
>> +
>> +     return lockmode;
>> +}
>
> We usually don't compare lock values that way, i.e. there's not
> guaranteed to be a strict monotonicity between lock levels. I don't
> really agree with that policy, but it's nonetheless there.

Yeah, there are some in lock.c but that's rather localized.
-- 
Michael

On Fri, Jul 31, 2015 at 11:41 AM, Fabrízio de Royes Mello wrote:
>> We usually don't compare lock values that way, i.e. there's not
>> guaranteed to be a strict monotonicity between lock levels. I don't
>> really agree with that policy, but it's nonetheless there.
>
> And how is the better way to compare lock values to get the highest lock
> level? Perhaps creating a function to compare lock levels?

I guess that this is exactly what Andres has in mind, aka something
like LockModeCompare(lockmode, lockmode) that returns {-1,0,1}
depending on which lock is higher on the hierarchy. This would do
exactly what your patch is doing though, except that this will
localize the comparison operators in lock.c. Though I am seeing at
quick glance a couple of places already do such comparisons:
backend/commands/tablecmds.c:        if (cmd_lockmode > lockmode)
backend/storage/lmgr/lock.c:        lockmode > RowExclusiveLock)
backend/storage/lmgr/lock.c:    if (lockmode >= AccessExclusiveLock &&
backend/access/heap/heapam.c:    Assert(lockmode >= NoLock && lockmode
< MAX_LOCKMODES);
backend/access/heap/heapam.c:    Assert(lockmode >= NoLock && lockmode
< MAX_LOCKMODES);
backend/access/heap/heapam.c:    Assert(lockmode >= NoLock && lockmode
< MAX_LOCKMODES);
backend/access/index/indexam.c:    Assert(lockmode >= NoLock &&
lockmode < MAX_LOCKMODES);
All of them are just sanity checks, except the one in tablecmds.c is
not (2dbbda0). Hence I am thinking that this is not really a problem
this patch should tackle by itself...
Regards,
--
Michael

On Fri, Jul 31, 2015 at 12:21 PM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
>
> On Thu, Jul 30, 2015 at 10:46 PM, Michael Paquier wrote:
>
>
>> On further notice, I would recommend not to use the same string name
>> for the session and the query identifiers. And I think that inserting
>> only one tuple at initialization is just but fine.
>>[...]
>> Be careful as well to not include incorrect permutations in the
>> expected output file, the isolation tester is smart enough to ping you
>> about that.
>>
>
> Changed the isolation tests according your suggestions.

Thanks, this looks good, and it reduces the patch size back to 26k. I
am switching that as ready for committer, we could argue more about
the lock levels that could be used for other parameters but ISTM that
this patch is taking the safest approach and we could always revisit
some lock levels afterwards.
--
Michael

On Fri, Jul 31, 2015 at 10:05 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 31 July 2015 at 02:46, Michael Paquier <michael.paquier@gmail.com> wrote:
>
>>
>> > Added. I really don't know if my isolation tests are completely correct,
>> > is
>> > my first time writing this kind of tests.
>>
>> This patch size has increased from 16k to 157k because of the output
>> of the isolation tests you just added.
>
>
> That's too much.

Yes, same opinion as mentioned upthread.

> Why do we need more isolation tests? There isn't anything critical here, its
> just different lock levels for ALTER TABLE. A few normal regression tests
> are fine for this.

Fabrizio went down to 26k with his latest patch by using only a subset
of permutations. To put it shortly, those things are worth testing. We
have the infrastructure to do it, and we lack of coverage in this
area. Hence this patch is a good occasion to do it IMO.
-- 
Michael

Fabrízio de Royes Mello wrote:

> In this patch I didn't change all lockmode comparison places previous
> pointed by you, but I can change it maybe adding other method called
> LockModeIsValid(lockmode) to do the comparison "lockmode >= NoLock &&
> lockmode < MAX_LOCKMODES" used in many places.

I don't like this.  Is it possible to write these comparisons in terms
of what they conflict with?  I think there are two main cases in the
existing code:

1. "is this lock mode valid" (sounds reasonable)
2. "can this be acquired in hot standby" (not so much, but makes  sense.)

and now we have your third thing, "what is the strongest of these two
locks".  For instance, if you told me to choose between ShareLock and
ShareUpdateExclusiveLock I wouldn't know which one is strongest.  I
don't it's sensible to have the "lock mode compare" primitive honestly.
I don't have any great ideas to offer ATM sadly.

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

On Sat, Aug 1, 2015 at 5:00 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Fabrízio de Royes Mello wrote:
>
>> In this patch I didn't change all lockmode comparison places previous
>> pointed by you, but I can change it maybe adding other method called
>> LockModeIsValid(lockmode) to do the comparison "lockmode >= NoLock &&
>> lockmode < MAX_LOCKMODES" used in many places.
>
> I don't like this.  Is it possible to write these comparisons in terms
> of what they conflict with?  I think there are two main cases in the
> existing code:
>
> 1. "is this lock mode valid" (sounds reasonable)
> 2. "can this be acquired in hot standby" (not so much, but makes
>    sense.)
>
> and now we have your third thing, "what is the strongest of these two
> locks".

The third method already exists in tablecmds.c:       /*        * Take the greatest lockmode from any subcommand
*/      if (cmd_lockmode > lockmode)           lockmode = cmd_lockmode; 

> For instance, if you told me to choose between ShareLock and
> ShareUpdateExclusiveLock I wouldn't know which one is strongest.  I
> don't it's sensible to have the "lock mode compare" primitive honestly.
> I don't have any great ideas to offer ATM sadly.

Yes, the thing is that lowering the lock levels is good for
concurrency, but the non-monotony of the lock levels makes it
impossible to choose an intermediate state correctly. Hence I think
that the one and only safe answer to this problem is that we check if
the locks taken for each subcommand are all the same, and use this
lock. If there is just one lock different, like for example one
subcommand uses ShareLock, and a second one ShareUpdateExclusiveLock
(good example of yours) we simply fallback to AccessExclusiveLock,
based on the fact that we are sure that this lock conflicts with all
the other ones.

At the same time I think that we should as well patch the existing
code path of tablecmds.c that already does those comparisons.Regards,
--
Michael

On August 1, 2015 2:17:24 PM GMT+02:00, Michael Paquier <michael.paquier@gmail.com> wrote:
>On Sat, Aug 1, 2015 at 5:00 AM, Alvaro Herrera
><alvherre@2ndquadrant.com> wrote:
>> Fabrízio de Royes Mello wrote:
>>
>>> In this patch I didn't change all lockmode comparison places
>previous
>>> pointed by you, but I can change it maybe adding other method called
>>> LockModeIsValid(lockmode) to do the comparison "lockmode >= NoLock
>&&
>>> lockmode < MAX_LOCKMODES" used in many places.
>>
>> I don't like this.  Is it possible to write these comparisons in
>terms
>> of what they conflict with?  I think there are two main cases in the
>> existing code:
>>
>> 1. "is this lock mode valid" (sounds reasonable)
>> 2. "can this be acquired in hot standby" (not so much, but makes
>>    sense.)
>>
>> and now we have your third thing, "what is the strongest of these two
>> locks".
>
>The third method already exists in tablecmds.c:
>        /*
>         * Take the greatest lockmode from any subcommand
>         */
>        if (cmd_lockmode > lockmode)
>            lockmode = cmd_lockmode;
>
>> For instance, if you told me to choose between ShareLock and
>> ShareUpdateExclusiveLock I wouldn't know which one is strongest.  I
>> don't it's sensible to have the "lock mode compare" primitive
>honestly.
>> I don't have any great ideas to offer ATM sadly.
>
>Yes, the thing is that lowering the lock levels is good for
>concurrency, but the non-monotony of the lock levels makes it
>impossible to choose an intermediate state correctly. 

How about simply acquiring all the locks individually of they're different types? These few acquisitions won't matter.

--- 
Please excuse brevity and formatting - I am writing this on my mobile phone.

On Sat, Aug 1, 2015 at 9:20 PM, Andres Freund wrote:
> On August 1, 2015 2:17:24 PM GMT+02:00, Michael Paquier wrote:
>>> For instance, if you told me to choose between ShareLock and
>>> ShareUpdateExclusiveLock I wouldn't know which one is strongest.  I
>>> don't it's sensible to have the "lock mode compare" primitive
>>honestly.
>>> I don't have any great ideas to offer ATM sadly.
>>
>>Yes, the thing is that lowering the lock levels is good for
>>concurrency, but the non-monotony of the lock levels makes it
>>impossible to choose an intermediate state correctly.
>
> How about simply acquiring all the locks individually of they're different types? These few acquisitions won't
matter.

As long as this only applies on master, this may be fine... We could
basically pass a LOCKMASK to the multiple layers of tablecmds.c
instead of LOCKMODE to track all the locks that need to be taken, and
all the relations open during operations. Would it be worth having
some routines like relation_multi_[open|close]() as well? Like that:
Relation[] relation_multi_open(relation Oid, LOCKMASK):
void relation_multi_close(Relation[]);

If we do something, we may consider patching as well 9.5, it seems to
me that tablecmds.c is broken by assuming that lock hierarchy is
monotone in AlterTableGetLockLevel().
-- 
Michael

On Tue, Aug 4, 2015 at 9:12 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
> Are you sure we need to do all this changes just to check the highest
> locklevel based on the reloptions?

Well, by looking at the code that's what it looks as. That's a large
change not that straight-forward.

> Or I misunderstood the whole thing?

No I think we are on the same page.

> Maybe we just check the DoLockModeConflict in the new method
> GetReloptionsLockLevel and if true then fallback to AccessExclusiveLock (as
> Michael suggested in a previous email).

Honestly that's what I would suggest for this patch, and also fix the
existing problem of tablecmds.c that does the same assumption. It
seems saner to me for now than adding a whole new level of routines
and wrappers, and your patch has IMO great value when each ALTER
COMMAND is kicked individually.
--
Michael

On 2015-08-03 14:15:27 +0900, Michael Paquier wrote:
> On Sat, Aug 1, 2015 at 9:20 PM, Andres Freund wrote:
> > On August 1, 2015 2:17:24 PM GMT+02:00, Michael Paquier wrote:
> >>> For instance, if you told me to choose between ShareLock and
> >>> ShareUpdateExclusiveLock I wouldn't know which one is strongest.  I
> >>> don't it's sensible to have the "lock mode compare" primitive
> >>honestly.
> >>> I don't have any great ideas to offer ATM sadly.
> >>
> >>Yes, the thing is that lowering the lock levels is good for
> >>concurrency, but the non-monotony of the lock levels makes it
> >>impossible to choose an intermediate state correctly.
> >
> > How about simply acquiring all the locks individually of they're different types? These few acquisitions won't
matter.
> 
> As long as this only applies on master, this may be fine... We could
> basically pass a LOCKMASK to the multiple layers of tablecmds.c
> instead of LOCKMODE to track all the locks that need to be taken, and
> all the relations open during operations.

This sounds far too complicated to me. Just LockRelationOid() the
relation with the appropriate level everytime you pass through the
function?

Andres Freund wrote:
> On 2015-08-03 14:15:27 +0900, Michael Paquier wrote:

> > As long as this only applies on master, this may be fine... We could
> > basically pass a LOCKMASK to the multiple layers of tablecmds.c
> > instead of LOCKMODE to track all the locks that need to be taken, and
> > all the relations open during operations.
> 
> This sounds far too complicated to me. Just LockRelationOid() the
> relation with the appropriate level everytime you pass through the
> function?

That opens up for lock escalation and deadlocks, doesn't it?  You are
probably thinking that it's okay to ignore those but I don't necessarily
agree with that.

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

On Wed, Aug 5, 2015 at 2:15 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Andres Freund wrote:
>> On 2015-08-03 14:15:27 +0900, Michael Paquier wrote:
>
>> > As long as this only applies on master, this may be fine... We could
>> > basically pass a LOCKMASK to the multiple layers of tablecmds.c
>> > instead of LOCKMODE to track all the locks that need to be taken, and
>> > all the relations open during operations.
>>
>> This sounds far too complicated to me. Just LockRelationOid() the
>> relation with the appropriate level everytime you pass through the
>> function?
>
> That opens up for lock escalation and deadlocks, doesn't it?  You are
> probably thinking that it's okay to ignore those but I don't necessarily
> agree with that.

Yes, I think so as long as we would try to take multiple locks... And
we go back to the lock hierarchy then, because there is no way to
define in some cases which lock is strictly stronger than the other.
Honestly I think that we should go with the version Fabrizio doing the
lock comparison, with a assert safeguard in AlterTableGetLockLevel.
The logic would get broken if ALTER TABLE begins to use a lock level
that cannot be ordered according to the others, but that's not the
case now.
-- 
Michael

On Tue, Aug 4, 2015 at 1:15 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> That opens up for lock escalation and deadlocks, doesn't it?  You are
> probably thinking that it's okay to ignore those but I don't necessarily
> agree with that.

Agreed.  I think we're making a mountain out of a molehill here.  As
long as the locks that are actually used are monotonic, just use > and
stick a comment in there explaining that it could need adjustment if
we use other lock levels in the future.  I presume all the lock-levels
used for DDL are, and will always be, self-exclusive, so why all this
hand-wringing?

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

On Thu, Aug 6, 2015 at 3:06 AM, Fabrízio de Royes Mello wrote:
> On Wed, Aug 5, 2015 at 9:31 AM, Robert Haas wrote:
>> Agreed.  I think we're making a mountain out of a molehill here.  As
>> long as the locks that are actually used are monotonic, just use > and
>> stick a comment in there explaining that it could need adjustment if
>> we use other lock levels in the future.  I presume all the lock-levels
>> used for DDL are, and will always be, self-exclusive, so why all this
>> hand-wringing?
>>
>
> New version attached with suggested changes.

Thanks!

+# SET autovacuum_* options needs a ShareUpdateExclusiveLock
+# so we mix reads with it to see what works or waits
s/needs/need/ and I think you mean mixing "writes", not "reads".

Those are minor things though, and from my point of view a committer
can look at it.
Regards,
--
Michael