Reduce lock levels others reloptions in ALTER TABLE

Started by Fabrízio de Royes Melloalmost 10 years ago4 messages
#1Fabrízio de Royes Mello
fabriziomello@gmail.com
1 attachment(s)

Hi all,

Some time ago we added [1]http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=47167b7907a802ed39b179c8780b76359468f076 the infrastructure to allow different lock
levels for relation options.

So per discussion [2]/messages/by-id/20150731022857.GC11473@alap3.anarazel.de the attached patch reduce lock levels down to
ShareUpdateExclusiveLock for:
- fastupdate
- fillfactor
- gin_pending_list_limit
- seq_page_cost
- random_page_cost
- n_distinct
- n_distinct_inherited
- buffering

Att,

[1]: http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=47167b7907a802ed39b179c8780b76359468f076
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=47167b7907a802ed39b179c8780b76359468f076
[2]: /messages/by-id/20150731022857.GC11473@alap3.anarazel.de
/messages/by-id/20150731022857.GC11473@alap3.anarazel.de

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

Show quoted text

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

Attachments:

reduce-locklevel-for-other-reloptions_v1.patchtext/x-diff; charset=US-ASCII; name=reduce-locklevel-for-other-reloptions_v1.patchDownload
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 86b9ae1..8128dd4 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -76,7 +76,7 @@ static relopt_bool boolRelOpts[] =
 			"fastupdate",
 			"Enables \"fast update\" feature for this GIN index",
 			RELOPT_KIND_GIN,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		true
 	},
@@ -100,7 +100,7 @@ static relopt_int intRelOpts[] =
 			"fillfactor",
 			"Packs table pages only to this percentage",
 			RELOPT_KIND_HEAP,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
 	},
@@ -109,7 +109,7 @@ static relopt_int intRelOpts[] =
 			"fillfactor",
 			"Packs btree index pages only to this percentage",
 			RELOPT_KIND_BTREE,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100
 	},
@@ -118,7 +118,7 @@ static relopt_int intRelOpts[] =
 			"fillfactor",
 			"Packs hash index pages only to this percentage",
 			RELOPT_KIND_HASH,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100
 	},
@@ -127,7 +127,7 @@ static relopt_int intRelOpts[] =
 			"fillfactor",
 			"Packs gist index pages only to this percentage",
 			RELOPT_KIND_GIST,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		GIST_DEFAULT_FILLFACTOR, GIST_MIN_FILLFACTOR, 100
 	},
@@ -136,7 +136,7 @@ static relopt_int intRelOpts[] =
 			"fillfactor",
 			"Packs spgist index pages only to this percentage",
 			RELOPT_KIND_SPGIST,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		SPGIST_DEFAULT_FILLFACTOR, SPGIST_MIN_FILLFACTOR, 100
 	},
@@ -250,7 +250,7 @@ static relopt_int intRelOpts[] =
 			"gin_pending_list_limit",
 			"Maximum size of the pending list for this GIN index, in kilobytes.",
 			RELOPT_KIND_GIN,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		-1, 64, MAX_KILOBYTES
 	},
@@ -297,7 +297,7 @@ static relopt_real realRelOpts[] =
 			"seq_page_cost",
 			"Sets the planner's estimate of the cost of a sequentially fetched disk page.",
 			RELOPT_KIND_TABLESPACE,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		-1, 0.0, DBL_MAX
 	},
@@ -306,7 +306,7 @@ static relopt_real realRelOpts[] =
 			"random_page_cost",
 			"Sets the planner's estimate of the cost of a nonsequentially fetched disk page.",
 			RELOPT_KIND_TABLESPACE,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		-1, 0.0, DBL_MAX
 	},
@@ -315,7 +315,7 @@ 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,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		0, -1.0, DBL_MAX
 	},
@@ -324,7 +324,7 @@ 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,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		0, -1.0, DBL_MAX
 	},
@@ -339,7 +339,7 @@ static relopt_string stringRelOpts[] =
 			"buffering",
 			"Enables buffering build for this GiST index",
 			RELOPT_KIND_GIST,
-			AccessExclusiveLock
+			ShareUpdateExclusiveLock
 		},
 		4,
 		false,
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 7c88ddc..3232cda 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -2065,19 +2065,19 @@ select * from my_locks order by 1;
 commit;
 begin; alter table alterlock set (fillfactor = 100);
 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 reset (fillfactor);
 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;
@@ -2110,10 +2110,10 @@ rollback;
 -- test that mixing options with different lock levels works as expected
 begin; alter table alterlock set (autovacuum_enabled = off, fillfactor = 80);
 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;
#2Fabrízio de Royes Mello
fabriziomello@gmail.com
In reply to: Fabrízio de Royes Mello (#1)
Re: Reduce lock levels others reloptions in ALTER TABLE

On Mon, Feb 29, 2016 at 2:58 PM, Fabrízio de Royes Mello <
fabriziomello@gmail.com> wrote:

Hi all,

Some time ago we added [1] the infrastructure to allow different lock

levels for relation options.

So per discussion [2] the attached patch reduce lock levels down to

ShareUpdateExclusiveLock for:

- fastupdate
- fillfactor
- gin_pending_list_limit
- seq_page_cost
- random_page_cost
- n_distinct
- n_distinct_inherited
- buffering

Att,

[1]

http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=47167b7907a802ed39b179c8780b76359468f076

[2]

/messages/by-id/20150731022857.GC11473@alap3.anarazel.de

Added to the next commitfest:

https://commitfest.postgresql.org/9/558/

Regards,

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

Show quoted text

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

#3Robert Haas
robertmhaas@gmail.com
In reply to: Fabrízio de Royes Mello (#1)
Re: Reduce lock levels others reloptions in ALTER TABLE

On Mon, Feb 29, 2016 at 12:58 PM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:

Some time ago we added [1] the infrastructure to allow different lock levels
for relation options.

So per discussion [2] the attached patch reduce lock levels down to
ShareUpdateExclusiveLock for:
- fastupdate
- fillfactor
- gin_pending_list_limit
- seq_page_cost
- random_page_cost
- n_distinct
- n_distinct_inherited
- buffering

I am of the opinion that there needs to be comments or a README or
some kind of documentation somewhere indicating the rationale for the
lock levels we choose here. Just changing it without analyzing why a
certain level is sufficient for safety and no higher than necessary
seems poor. And the reasoning should be documented for future
readers.

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

#4Simon Riggs
simon@2ndQuadrant.com
In reply to: Robert Haas (#3)
Re: Reduce lock levels others reloptions in ALTER TABLE

On 1 March 2016 at 19:00, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Feb 29, 2016 at 12:58 PM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:

Some time ago we added [1] the infrastructure to allow different lock

levels

for relation options.

So per discussion [2] the attached patch reduce lock levels down to
ShareUpdateExclusiveLock for:
- fastupdate
- fillfactor
- gin_pending_list_limit
- seq_page_cost
- random_page_cost
- n_distinct
- n_distinct_inherited
- buffering

I am of the opinion that there needs to be comments or a README or
some kind of documentation somewhere indicating the rationale for the
lock levels we choose here. Just changing it without analyzing why a
certain level is sufficient for safety and no higher than necessary
seems poor. And the reasoning should be documented for future
readers.

+1

The patch altered tests for fillfactor, so I extracted just that parameter
from the patch for commit, after adding comments and docs.

Fabrízio, you've been around long enough to know that patches need docs and
tests. And as Robert says, comments that show you've done the analysis to
show you know the patch is safe.

Some parts of this patch could be resubmitted in a later CF with some time
and attention spent on it, but it isn't in a good enough state for last CF.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services