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

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

On Mon, Mar 30, 2015 at 7:41 PM, Jim Nasby <Jim.Nasby@bluetreble.com> wrote:

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.

Hummm.... There are a comment in AlterTableGetLockLevel:

3017 /*
3018 * Rel options are more complex than first appears.
Options
3019 * are set here for tables, views and indexes; for
historical
3020 * reasons these can all be used with ALTER TABLE, so
we can't
3021 * decide between them using the basic grammar.
3022 *
3023 * XXX Look in detail at each option to determine
lock level,
3024 * e.g. cmd_lockmode = GetRelOptionsLockLevel((List *)
3025 * cmd->def);
3026 */
3027 case AT_SetRelOptions: /* Uses MVCC in getIndexes()
and
3028 * getTables() */
3029 case AT_ResetRelOptions: /* Uses MVCC in getIndexes()
and
3030 * getTables() */
3031 cmd_lockmode = AccessExclusiveLock;
3032 break;

Maybe it's time to implement "GetRelOptionsLockLevel" to relax the lock to
autovac settings (AccessShareLock). To other settings we continue using
AccessExclusiveLock.

There are some objection to implement in that way?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

On Mon, Mar 30, 2015 at 8:14 PM, Fabrízio de Royes Mello <
fabriziomello@gmail.com> wrote:

On Mon, Mar 30, 2015 at 7:41 PM, Jim Nasby <Jim.Nasby@bluetreble.com>

wrote:

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.

Hummm.... There are a comment in AlterTableGetLockLevel:

3017 /*
3018 * Rel options are more complex than first appears.

Options

3019 * are set here for tables, views and indexes; for

historical

3020 * reasons these can all be used with ALTER TABLE,

so we can't

3021 * decide between them using the basic grammar.
3022 *
3023 * XXX Look in detail at each option to determine

lock level,

3024 * e.g. cmd_lockmode = GetRelOptionsLockLevel((List

*)

3025 * cmd->def);
3026 */
3027 case AT_SetRelOptions: /* Uses MVCC in

getIndexes() and

3028 * getTables() */
3029 case AT_ResetRelOptions: /* Uses MVCC in

getIndexes() and

3030 * getTables() */
3031 cmd_lockmode = AccessExclusiveLock;
3032 break;

Maybe it's time to implement "GetRelOptionsLockLevel" to relax the lock

to autovac settings (AccessShareLock). To other settings we continue using
AccessExclusiveLock.

There are some objection to implement in that way?

Attached a very WIP patch to reduce lock level when setting autovacuum
reloptions in "ALTER TABLE .. SET ( .. )" statement.

I confess the implementation is ugly, maybe we should add a new item to
reloptions constants in src/backend/access/common/reloptions.c and a proper
function to get lock level by reloption. Thoughts?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

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

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

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"):

/messages/by-id/20140321034556.GA3927180@tornado.leadboat.com

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

On Wed, Apr 1, 2015 at 1:45 AM, Noah Misch <noah@leadboat.com> wrote:

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"):

/messages/by-id/20140321034556.GA3927180@tornado.leadboat.com

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:

- boolRelopts:
* autovacuum_enabled (AccessShareLock)
* user_catalog_table (AccessExclusiveLock)
* fastupdate (AccessExclusiveLock)
* security_barrier (AccessExclusiveLock)

- intRelOpts:
* fillfactor (heap) (AccessExclusiveLock)
* fillfactor (btree) (AccessExclusiveLock)
* fillfactor (gist) (AccessExclusiveLock)
* fillfactor (spgist) (AccessExclusiveLock)
* autovacuum_vacuum_threshold (AccessShareLock)
* autovacuum_analyze_threshold (AccessShareLock)
* autovacuum_vacuum_cost_delay (AccessShareLock)
* autovacuum_vacuum_cost_limit (AccessShareLock)
* autovacuum_freeze_min_age (AccessShareLock)
* autovacuum_multixact_freeze_min_age (AccessShareLock)
* autovacuum_freeze_max_age (AccessShareLock)
* autovacuum_multixact_freeze_max_age (AccessShareLock)
* autovacuum_freeze_table_age (AccessShareLock)
* autovacuum_multixact_freeze_table_age (AccessShareLock)
* log_autovacuum_min_duration (AccessShareLock)
* pages_per_range (AccessExclusiveLock)
* gin_pending_list_limit (AccessExclusiveLock)

- realRelOpts:
* autovacuum_vacuum_scale_factor (AccessShareLock)
* autovacuum_analyze_scale_factor (AccessShareLock)
* seq_page_cost (AccessExclusiveLock)
* random_page_cost (AccessExclusiveLock)
* n_distinct (AccessExclusiveLock)
* n_distinct_inherited (AccessExclusiveLock)

- stringRelOpts:
* buffering (AccessExclusiveLock)
* check_option (AccessExclusiveLock)

In the above list I just change lock level from AccessExclusiveLock to
AccessShareLock to all "autovacuum" related reloptions because it was the
motivation of this patch.

I need some help to define the others.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

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

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

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?

(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?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

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 mean
Assert(DoLockModesConflict(relopt_gen->locklevel, relopt_gen->locklevel));

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

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

On Tue, Apr 7, 2015 at 10:15 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

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.

Ok.

(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 mean
Assert(DoLockModesConflict(relopt_gen->locklevel,

relopt_gen->locklevel));

Understood... IMHO the better place to perform this assert is in
"initialize_reloptions".

Thoughts?

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

On Tue, Apr 7, 2015 at 10:57 PM, Fabrízio de Royes Mello <
fabriziomello@gmail.com> wrote:

On Tue, Apr 7, 2015 at 10:15 PM, Alvaro Herrera <alvherre@2ndquadrant.com>

wrote:

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.

Ok.

(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 mean
Assert(DoLockModesConflict(relopt_gen->locklevel,

relopt_gen->locklevel));

Understood... IMHO the better place to perform this assert is in

"initialize_reloptions".

Thoughts?

The attached patch:
- introduce new field "lockmode" to "relopt_gen" struct
- initialize lockmode with ShareUpdateExclusiveLock to autovac settings
- add assert to ensuring that the lock level specified don't conflict with
itself
- add function GetRelOptionsLockLevel to determine the required lock mode
from an option list

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

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 broken by this.

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

Needs documentation

The new status of this patch is: Waiting on Author

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

On Thu, Jul 2, 2015 at 2:03 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

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 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?

I did some tests using ALTER TABLE on views and also ALTER VIEW and I
didn't identify any anomalies.

Needs documentation

Added.

Att,

*** This work is funded by Zenvia Mobile Results (http://www.zenvia.com.br)

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

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

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

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.

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.

Fixed.

+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".

Thanks for reviewing.

Regards,

*** This work is funded by Zenvia Mobile Results (http://www.zenvia.com.br)

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

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

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

@@ -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

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

On Thu, Jul 30, 2015 at 11:28 PM, 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.

I have no problem to do this change now instead of wait for 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.

+1

@@ -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.

+1

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.

What do you suggest then?

+/*
+ * 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.

And how is the better way to compare lock values to get the highest lock
level? Perhaps creating a function to compare lock levels?

Regards,

*** This work is funded by Zenvia Mobile Results (http://www.zenvia.com.br)

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL

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

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