Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Wed, Aug 5, 2015 at 9:21 PM, Michael Paquier michael.paqu...@gmail.com
wrote:
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.
Fixed. Thanks for your review.
Regards,
*** This work is funded by Zenvia Mobile Results (http://www.zenvia.com.br)
--
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/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1c1c181..ad985cd 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -543,6 +543,10 @@ ALTER TABLE ALL IN TABLESPACE replaceable class=PARAMETERname/replaceable
of commandALTER TABLE/ that forces a table rewrite.
/para
+ para
+ Changing autovacuum storage parameters acquires a literalSHARE UPDATE EXCLUSIVE/literal lock.
+ /para
+
note
para
While commandCREATE TABLE/ allows literalOIDS/ to be specified
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 180f529..c39b878 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
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Tue, Aug 4, 2015 at 1:15 PM, Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Wed, Aug 5, 2015 at 9:31 AM, Robert Haas robertmh...@gmail.com wrote:
On Tue, Aug 4, 2015 at 1:15 PM, Alvaro Herrera alvhe...@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?
New version attached with suggested changes.
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/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1c1c181..ad985cd 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -543,6 +543,10 @@ ALTER TABLE ALL IN TABLESPACE replaceable class=PARAMETERname/replaceable
of commandALTER TABLE/ that forces a table rewrite.
/para
+ para
+ Changing autovacuum storage parameters acquires a literalSHARE UPDATE EXCLUSIVE/literal lock.
+ /para
+
note
para
While commandCREATE TABLE/ allows literalOIDS/ to be specified
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 180f529..c39b878 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
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
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? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Tue, Aug 4, 2015 at 5:55 AM, Andres Freund and...@anarazel.de wrote:
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?
Hi all,
IMHO is more simply we just fallback to AccessExclusiveLock if there are
different lockmodes in reloptions as Michael suggested before.
Look at the new version attached.
Regards,
*** This work is funded by Zenvia Mobile Results (http://www.zenvia.com.br)
--
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/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1c1c181..ad985cd 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -543,6 +543,10 @@ ALTER TABLE ALL IN TABLESPACE replaceable class=PARAMETERname/replaceable
of commandALTER TABLE/ that forces a table rewrite.
/para
+ para
+ Changing autovacuum storage parameters acquires a literalSHARE UPDATE EXCLUSIVE/literal lock.
+ /para
+
note
para
While commandCREATE TABLE/ allows literalOIDS/ to be specified
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 180f529..e0f9f09 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
+
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Wed, Aug 5, 2015 at 2:15 AM, Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
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 Herrerahttp://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
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Mon, Aug 3, 2015 at 2:15 AM, Michael Paquier michael.paqu...@gmail.com 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. 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(). Hey guys, Are you sure we need to do all this changes just to check the highest locklevel based on the reloptions? Or I misunderstood the whole thing? 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). Regards, *** This work is funded by Zenvia Mobile Results (http://www.zenvia.com.br) -- 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
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Tue, Aug 4, 2015 at 9:12 AM, Fabrízio de Royes Mello fabriziome...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Sat, Aug 1, 2015 at 5:00 AM, Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On August 1, 2015 2:17:24 PM GMT+02:00, Michael Paquier michael.paqu...@gmail.com wrote: On Sat, Aug 1, 2015 at 5:00 AM, Alvaro Herrera alvhe...@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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Fri, Jul 31, 2015 at 10:05 PM, Simon Riggs si...@2ndquadrant.com wrote: On 31 July 2015 at 02:46, Michael Paquier michael.paqu...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Fri, Jul 31, 2015 at 12:21 PM, Fabrízio de Royes Mello fabriziome...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On 31 July 2015 at 02:46, Michael Paquier michael.paqu...@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. 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. -- Simon Riggshttp://www.2ndQuadrant.com/ http://www.2ndquadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Fri, Jul 31, 2015 at 10:01 AM, Michael Paquier michael.paqu...@gmail.com
wrote:
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...
I did it in the attached version of the patch... But I don't know if the
names are good so fell free to suggest others if you dislike of my choice.
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.
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/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1c1c181..ad985cd 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -543,6 +543,10 @@ ALTER TABLE ALL IN TABLESPACE replaceable class=PARAMETERname/replaceable
of commandALTER TABLE/ that forces a table rewrite.
/para
+ para
+ Changing autovacuum storage parameters acquires a literalSHARE UPDATE EXCLUSIVE/literal lock.
+ /para
+
note
para
While commandCREATE TABLE/ allows literalOIDS/ to be specified
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 8176b6a..a62ada1 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
},
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
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 Herrerahttp://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
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Thu, Jul 30, 2015 at 11:28 PM, Andres Freund and...@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)
+{
+
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Fri, Jul 31, 2015 at 11:28 AM, Andres Freund and...@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
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Thu, Jul 30, 2015 at 10:46 PM, Michael Paquier michael.paqu...@gmail.com
wrote:
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.
Ok.
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 + 1; }
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 + 1; 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.
Changed the isolation tests according your suggestions.
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/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 1c1c181..ad985cd 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -543,6 +543,10 @@ ALTER TABLE ALL IN TABLESPACE replaceable class=PARAMETERname/replaceable
of commandALTER TABLE/ that forces a table rewrite.
/para
+ para
+ Changing autovacuum storage parameters acquires a literalSHARE UPDATE EXCLUSIVE/literal lock.
+ /para
+
note
para
While commandCREATE TABLE/ allows literalOIDS/ to be specified
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 8176b6a..b8d2a92 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
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
@@ -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)
+{
+ LOCKMODElockmode = NoLock;
+ ListCell*cell;
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Fri, Jul 31, 2015 at 8:30 AM, Fabrízio de Royes Mello
fabriziome...@gmail.com wrote:
On Fri, Jul 24, 2015 at 4:05 AM, Michael Paquier michael.paqu...@gmail.com
wrote:
On Fri, Jul 24, 2015 at 7:11 AM, Fabrízio de Royes Mello
fabriziome...@gmail.com wrote:
On Thu, Jul 2, 2015 at 2:03 PM, Simon Riggs si...@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.gb3969...@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 + 1; }
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 + 1; 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)
+{
+ LOCKMODElockmode = 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
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Fri, Jul 24, 2015 at 7:11 AM, Fabrízio de Royes Mello
fabriziome...@gmail.com wrote:
On Thu, Jul 2, 2015 at 2:03 PM, Simon Riggs si...@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.gb3969...@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)
+{
+ LOCKMODElockmode = 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
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Thu, Jul 2, 2015 at 2:03 PM, Simon Riggs si...@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.gb3969...@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
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/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
index 207fec1..b126855 100644
--- a/doc/src/sgml/ref/alter_table.sgml
+++ b/doc/src/sgml/ref/alter_table.sgml
@@ -541,6 +541,10 @@ ALTER TABLE ALL IN TABLESPACE replaceable class=PARAMETERname/replaceable
of commandALTER TABLE/ that forces a table rewrite.
/para
+ para
+ Changing autovacuum storage parameters acquires a literalSHARE UPDATE EXCLUSIVE/literal lock.
+ /para
+
note
para
While commandCREATE TABLE/ allows literalOIDS/ to be specified
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
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
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.gb3969...@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
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
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,
+
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
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? (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 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
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
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. (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 Herrerahttp://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
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
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? -- 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
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
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 Herrerahttp://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
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Wed, Apr 1, 2015 at 1:45 AM, Noah Misch n...@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
fabriziome...@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.ga3927...@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
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..f83ce2f 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,
+ AccessExclusiveLock
},
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
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Mon, Mar 30, 2015 at 8:14 PM, Fabrízio de Royes Mello
fabriziome...@gmail.com wrote:
On Mon, Mar 30, 2015 at 7:41 PM, Jim Nasby jim.na...@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
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/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 32e19c5..0be658f 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -402,6 +402,7 @@ static void ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmo
static void ATExecSetRelOptions(Relation rel, List *defList,
AlterTableType operation,
LOCKMODE lockmode);
+static LOCKMODE GetRelOptionsLockLevel(List *defList);
static void ATExecEnableDisableTrigger(Relation rel, char *trigname,
char fires_when, bool skip_system, LOCKMODE lockmode);
static void ATExecEnableDisableRule(Relation rel, char *rulename,
@@ -2783,6 +2784,39 @@ AlterTableInternal(Oid relid, List *cmds, bool recurse)
}
/*
+ * GetRelOptionsLockLevel
+ *
+ * Return AccessShareLock if all reloptions is related to autovacuum
+ * else return AccessExclusiveLock.
+ *
+ */
+LOCKMODE
+GetRelOptionsLockLevel(List *defList)
+{
+ LOCKMODE lockmode;
+ ListCell *cell;
+
+ if (defList == NIL)
+ return NoLock;
+
+ foreach(cell, defList)
+ {
+ DefElem *def = (DefElem *) lfirst(cell);
+
+ /* relax lock for autovacuum reloptions */
+ if (pg_strncasecmp(autovacuum_, def-defname, 11) == 0)
+ lockmode = AccessShareLock;
+ else
+ {
+ lockmode = AccessExclusiveLock;
+ break;
+ }
+ }
+
+ return lockmode;
+}
+
+/*
* AlterTableGetLockLevel
*
* Sets the overall lock level required for the supplied list of subcommands.
@@ -3028,7 +3062,7 @@ AlterTableGetLockLevel(List *cmds)
* getTables() */
case AT_ResetRelOptions: /* Uses MVCC in getIndexes() and
* getTables() */
-cmd_lockmode = AccessExclusiveLock;
+cmd_lockmode = GetRelOptionsLockLevel((List *) cmd-def);
break;
default: /* oops */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
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 fabriziome...@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.ga3927...@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
Re: [HACKERS] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Tue, Mar 31, 2015 at 9:11 AM, Fabrízio de Royes Mello fabriziome...@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
Re: [HACKERS] 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
