On Tue, Apr 7, 2015 at 10:57 PM, Fabrízio de Royes Mello <
fabriziome...@gmail.com> wrote:
>
>
> On Tue, Apr 7, 2015 at 10:15 PM, Alvaro Herrera <alvhe...@2ndquadrant.com>
wrote:
> >
> > Fabrízio de Royes Mello wrote:
> > > On Mon, Apr 6, 2015 at 12:53 AM, Alvaro Herrera <
alvhe...@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
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 8176b6a..f273944 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
{
"autovacuum_enabled",
"Enables autovacuum in this relation",
- RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+ RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+ ShareUpdateExclusiveLock
},
true
},
@@ -65,7 +66,8 @@ static relopt_bool boolRelOpts[] =
{
"user_catalog_table",
"Declare a table as an additional catalog table, e.g. for the purpose of logical replication",
- RELOPT_KIND_HEAP
+ RELOPT_KIND_HEAP,
+ AccessExclusiveLock
},
false
},
@@ -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
},
@@ -81,7 +84,8 @@ static relopt_bool boolRelOpts[] =
{
"security_barrier",
"View acts as a row security barrier",
- RELOPT_KIND_VIEW
+ RELOPT_KIND_VIEW,
+ AccessExclusiveLock
},
false
},
@@ -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
},
@@ -103,7 +108,8 @@ static relopt_int intRelOpts[] =
{
"fillfactor",
"Packs btree index pages only to this percentage",
- RELOPT_KIND_BTREE
+ RELOPT_KIND_BTREE,
+ AccessExclusiveLock
},
BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100
},
@@ -111,7 +117,8 @@ static relopt_int intRelOpts[] =
{
"fillfactor",
"Packs hash index pages only to this percentage",
- RELOPT_KIND_HASH
+ RELOPT_KIND_HASH,
+ AccessExclusiveLock
},
HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100
},
@@ -119,7 +126,8 @@ static relopt_int intRelOpts[] =
{
"fillfactor",
"Packs gist index pages only to this percentage",
- RELOPT_KIND_GIST
+ RELOPT_KIND_GIST,
+ AccessExclusiveLock
},
GIST_DEFAULT_FILLFACTOR, GIST_MIN_FILLFACTOR, 100
},
@@ -127,7 +135,8 @@ static relopt_int intRelOpts[] =
{
"fillfactor",
"Packs spgist index pages only to this percentage",
- RELOPT_KIND_SPGIST
+ RELOPT_KIND_SPGIST,
+ AccessExclusiveLock
},
SPGIST_DEFAULT_FILLFACTOR, SPGIST_MIN_FILLFACTOR, 100
},
@@ -135,7 +144,8 @@ static relopt_int intRelOpts[] =
{
"autovacuum_vacuum_threshold",
"Minimum number of tuple updates or deletes prior to vacuum",
- RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+ RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+ ShareUpdateExclusiveLock
},
-1, 0, INT_MAX
},
@@ -143,7 +153,8 @@ static relopt_int intRelOpts[] =
{
"autovacuum_analyze_threshold",
"Minimum number of tuple inserts, updates or deletes prior to analyze",
- RELOPT_KIND_HEAP
+ RELOPT_KIND_HEAP,
+ ShareUpdateExclusiveLock
},
-1, 0, INT_MAX
},
@@ -151,7 +162,8 @@ static relopt_int intRelOpts[] =
{
"autovacuum_vacuum_cost_delay",
"Vacuum cost delay in milliseconds, for autovacuum",
- RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+ RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+ ShareUpdateExclusiveLock
},
-1, 0, 100
},
@@ -159,7 +171,8 @@ static relopt_int intRelOpts[] =
{
"autovacuum_vacuum_cost_limit",
"Vacuum cost amount available before napping, for autovacuum",
- RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+ RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+ ShareUpdateExclusiveLock
},
-1, 1, 10000
},
@@ -167,7 +180,8 @@ static relopt_int intRelOpts[] =
{
"autovacuum_freeze_min_age",
"Minimum age at which VACUUM should freeze a table row, for autovacuum",
- RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+ RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+ ShareUpdateExclusiveLock
},
-1, 0, 1000000000
},
@@ -175,7 +189,8 @@ static relopt_int intRelOpts[] =
{
"autovacuum_multixact_freeze_min_age",
"Minimum multixact age at which VACUUM should freeze a row multixact's, for autovacuum",
- RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+ RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+ ShareUpdateExclusiveLock
},
-1, 0, 1000000000
},
@@ -183,7 +198,8 @@ static relopt_int intRelOpts[] =
{
"autovacuum_freeze_max_age",
"Age at which to autovacuum a table to prevent transaction ID wraparound",
- RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+ RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+ ShareUpdateExclusiveLock
},
-1, 100000000, 2000000000
},
@@ -191,7 +207,8 @@ static relopt_int intRelOpts[] =
{
"autovacuum_multixact_freeze_max_age",
"Multixact age at which to autovacuum a table to prevent multixact wraparound",
- RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+ RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+ ShareUpdateExclusiveLock
},
-1, 100000000, 2000000000
},
@@ -199,21 +216,24 @@ static relopt_int intRelOpts[] =
{
"autovacuum_freeze_table_age",
"Age at which VACUUM should perform a full table sweep to freeze row versions",
- RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+ RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+ ShareUpdateExclusiveLock
}, -1, 0, 2000000000
},
{
{
"autovacuum_multixact_freeze_table_age",
"Age of multixact at which VACUUM should perform a full table sweep to freeze row versions",
- RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+ RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+ ShareUpdateExclusiveLock
}, -1, 0, 2000000000
},
{
{
"log_autovacuum_min_duration",
"Sets the minimum execution time above which autovacuum actions will be logged",
- RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+ RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+ ShareUpdateExclusiveLock
},
-1, -1, INT_MAX
},
@@ -221,14 +241,16 @@ static relopt_int intRelOpts[] =
{
"pages_per_range",
"Number of pages that each page range covers in a BRIN index",
- RELOPT_KIND_BRIN
+ RELOPT_KIND_BRIN,
+ AccessExclusiveLock
}, 128, 1, 131072
},
{
{
"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
},
@@ -243,7 +265,8 @@ static relopt_real realRelOpts[] =
{
"autovacuum_vacuum_scale_factor",
"Number of tuple updates or deletes prior to vacuum as a fraction of reltuples",
- RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+ RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+ ShareUpdateExclusiveLock
},
-1, 0.0, 100.0
},
@@ -251,7 +274,8 @@ static relopt_real realRelOpts[] =
{
"autovacuum_analyze_scale_factor",
"Number of tuple inserts, updates or deletes prior to analyze as a fraction of reltuples",
- RELOPT_KIND_HEAP
+ RELOPT_KIND_HEAP,
+ ShareUpdateExclusiveLock
},
-1, 0.0, 100.0
},
@@ -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
},
@@ -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,
@@ -308,7 +337,8 @@ static relopt_string stringRelOpts[] =
{
"check_option",
"View has WITH CHECK OPTION defined (local or cascaded).",
- RELOPT_KIND_VIEW
+ RELOPT_KIND_VIEW,
+ AccessExclusiveLock
},
0,
true,
@@ -344,13 +374,25 @@ initialize_reloptions(void)
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;
if (relOpts)
@@ -1406,3 +1448,36 @@ tablespace_reloptions(Datum reloptions, bool validate)
return (bytea *) tsopts;
}
+
+/*
+ * Determine the required LOCKMODE from an option list
+ */
+LOCKMODE
+GetRelOptionsLockLevel(List *defList)
+{
+ LOCKMODE lockmode = NoLock;
+ ListCell *cell;
+
+ if (defList == NIL)
+ return lockmode;
+
+ 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;
+}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 06e4332..41462af 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3025,16 +3025,12 @@ AlterTableGetLockLevel(List *cmds)
* are set here for tables, views and indexes; for historical
* reasons these can all be used with ALTER TABLE, so we can't
* decide between them using the basic grammar.
- *
- * XXX Look in detail at each option to determine lock level,
- * e.g. cmd_lockmode = GetRelOptionsLockLevel((List *)
- * cmd->def);
*/
case AT_SetRelOptions: /* Uses MVCC in getIndexes() and
* getTables() */
case AT_ResetRelOptions: /* Uses MVCC in getIndexes() and
* getTables() */
- cmd_lockmode = AccessExclusiveLock;
+ cmd_lockmode = GetRelOptionsLockLevel((List *) cmd->def);
break;
default: /* oops */
diff --git a/src/include/access/reloptions.h b/src/include/access/reloptions.h
index e7b6bb5..c444c71 100644
--- a/src/include/access/reloptions.h
+++ b/src/include/access/reloptions.h
@@ -22,6 +22,7 @@
#include "access/htup.h"
#include "access/tupdesc.h"
#include "nodes/pg_list.h"
+#include "storage/lock.h"
/* types supported by reloptions */
typedef enum relopt_type
@@ -62,6 +63,7 @@ typedef struct relopt_gen
* marker) */
const char *desc;
bits32 kinds;
+ LOCKMODE lockmode;
int namelen;
relopt_type type;
} relopt_gen;
@@ -274,5 +276,6 @@ extern bytea *index_reloptions(RegProcedure amoptions, Datum reloptions,
bool validate);
extern bytea *attribute_reloptions(Datum reloptions, bool validate);
extern bytea *tablespace_reloptions(Datum reloptions, bool validate);
+extern LOCKMODE GetRelOptionsLockLevel(List *defList);
#endif /* RELOPTIONS_H */
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 65274bc..b88243f 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1910,19 +1910,19 @@ select * from my_locks order by 1;
commit;
begin; alter table alterlock set (toast.autovacuum_enabled = off);
select * from my_locks order by 1;
- relname | max_lockmode
------------+---------------------
- alterlock | AccessExclusiveLock
- pg_toast | AccessExclusiveLock
+ relname | max_lockmode
+-----------+--------------------------
+ alterlock | ShareUpdateExclusiveLock
+ pg_toast | ShareUpdateExclusiveLock
(2 rows)
commit;
begin; alter table alterlock set (autovacuum_enabled = off);
select * from my_locks order by 1;
- relname | max_lockmode
------------+---------------------
- alterlock | AccessExclusiveLock
- pg_toast | AccessExclusiveLock
+ relname | max_lockmode
+-----------+--------------------------
+ alterlock | ShareUpdateExclusiveLock
+ pg_toast | ShareUpdateExclusiveLock
(2 rows)
commit;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
