Re: [HACKERS] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-28 Thread Peter Eisentraut 
On tor, 2010-07-15 at 10:24 +0100, Simon Riggs wrote:
 Patch to reduce lock levels for 
  ALTER TABLE
  CREATE TRIGGER
  CREATE RULE

Tried this out, but $subject is still the case.  The problem is that
ATRewriteCatalogs() calls AlterTableCreateToastTable() based on what it
thinks the subcommands are, and AlterTableCreateToastTable() takes an
AccessExclusiveLock.

This could possibly be addressed by moving AT_SetStatistics to
AT_PASS_MISC in order to avoid the TOAST table call.

In a related matter, assigning ShareUpdateExclusiveLock to AT_SetStorage
doesn't work either, because the TOAST table call needs to be done in
that case.

Perhaps this logic needs to be refactored a bit more so that there
aren't any inconsistencies between the lock modes and the passes,
which could lead to false expectations and deadlocks.


-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-28 Thread Simon Riggs 
On Wed, 2010-07-28 at 15:24 +0300, Peter Eisentraut wrote:
 On tor, 2010-07-15 at 10:24 +0100, Simon Riggs wrote:
  Patch to reduce lock levels for 
   ALTER TABLE
   CREATE TRIGGER
   CREATE RULE
 
 Tried this out, but $subject is still the case.  The problem is that
 ATRewriteCatalogs() calls AlterTableCreateToastTable() based on what it
 thinks the subcommands are, and AlterTableCreateToastTable() takes an
 AccessExclusiveLock.
 
 This could possibly be addressed by moving AT_SetStatistics to
 AT_PASS_MISC in order to avoid the TOAST table call.
 
 In a related matter, assigning ShareUpdateExclusiveLock to AT_SetStorage
 doesn't work either, because the TOAST table call needs to be done in
 that case.
 
 Perhaps this logic needs to be refactored a bit more so that there
 aren't any inconsistencies between the lock modes and the passes,
 which could lead to false expectations and deadlocks.

Thanks for your comments. Will reconsider and update.

-- 
 Simon Riggs   www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and 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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-28 Thread Simon Riggs 
On Wed, 2010-07-28 at 15:24 +0300, Peter Eisentraut wrote:
 On tor, 2010-07-15 at 10:24 +0100, Simon Riggs wrote:
  Patch to reduce lock levels for 
   ALTER TABLE
   CREATE TRIGGER
   CREATE RULE
 
 Tried this out, but $subject is still the case.  The problem is that
 ATRewriteCatalogs() calls AlterTableCreateToastTable() based on what it
 thinks the subcommands are, and AlterTableCreateToastTable() takes an
 AccessExclusiveLock.
 
 This could possibly be addressed by moving AT_SetStatistics to
 AT_PASS_MISC in order to avoid the TOAST table call.
 
 In a related matter, assigning ShareUpdateExclusiveLock to AT_SetStorage
 doesn't work either, because the TOAST table call needs to be done in
 that case.
 
 Perhaps this logic needs to be refactored a bit more so that there
 aren't any inconsistencies between the lock modes and the passes,
 which could lead to false expectations and deadlocks.

Changes as suggested, plus tests to confirm lock levels for
ShareUpdateExclusiveLock changes. Will commit soon, if no objection.

-- 
 Simon Riggs   www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and Services
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 516dbd2..77a3c57 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -376,7 +376,7 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose,
 
 	/* Check heap and index are valid to cluster on */
 	if (OidIsValid(indexOid))
-		check_index_is_clusterable(OldHeap, indexOid, recheck);
+		check_index_is_clusterable(OldHeap, indexOid, recheck, AccessExclusiveLock);
 
 	/* Log what we're doing (this could use more effort) */
 	if (OidIsValid(indexOid))
@@ -405,11 +405,11 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose,
  * definition can't change under us.
  */
 void
-check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck)
+check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMODE lockmode)
 {
 	Relation	OldIndex;
 
-	OldIndex = index_open(indexOid, AccessExclusiveLock);
+	OldIndex = index_open(indexOid, lockmode);
 
 	/*
 	 * Check that index is in fact an index on the given relation
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 9a7cd96..defb2e4 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2550,6 +2550,8 @@ AlterTableGetLockLevel(List *cmds)
 			case AT_DropCluster:
 			case AT_SetRelOptions:
 			case AT_ResetRelOptions:
+			case AT_SetOptions:
+			case AT_ResetOptions:
 			case AT_SetStorage:
 cmd_lockmode = ShareUpdateExclusiveLock;
 break;
@@ -2669,19 +2671,19 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
 			ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
 			/* Performs own permission checks */
 			ATPrepSetStatistics(rel, cmd-name, cmd-def, lockmode);
-			pass = AT_PASS_COL_ATTRS;
+			pass = AT_PASS_MISC;
 			break;
 		case AT_SetOptions:		/* ALTER COLUMN SET ( options ) */
 		case AT_ResetOptions:	/* ALTER COLUMN RESET ( options ) */
 			ATSimplePermissionsRelationOrIndex(rel);
 			/* This command never recurses */
-			pass = AT_PASS_COL_ATTRS;
+			pass = AT_PASS_MISC;
 			break;
 		case AT_SetStorage:		/* ALTER COLUMN SET STORAGE */
 			ATSimplePermissions(rel, false);
 			ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode);
 			/* No command-specific prep needed */
-			pass = AT_PASS_COL_ATTRS;
+			pass = AT_PASS_MISC;
 			break;
 		case AT_DropColumn:		/* DROP COLUMN */
 			ATSimplePermissions(rel, false);
@@ -6906,7 +6908,7 @@ ATExecClusterOn(Relation rel, const char *indexName, LOCKMODE lockmode)
 		indexName, RelationGetRelationName(rel;
 
 	/* Check index is valid to cluster on */
-	check_index_is_clusterable(rel, indexOid, false);
+	check_index_is_clusterable(rel, indexOid, false, lockmode);
 
 	/* And do the work */
 	mark_index_clustered(rel, indexOid);
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index 4f67930..74d4bd1 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -14,6 +14,7 @@
 #define CLUSTER_H
 
 #include nodes/parsenodes.h
+#include storage/lock.h
 #include utils/relcache.h
 
 
@@ -21,7 +22,7 @@ extern void cluster(ClusterStmt *stmt, bool isTopLevel);
 extern void cluster_rel(Oid tableOid, Oid indexOid, bool recheck,
 			bool verbose, int freeze_min_age, int freeze_table_age);
 extern void check_index_is_clusterable(Relation OldHeap, Oid indexOid,
-		   bool recheck);
+		   bool recheck, LOCKMODE lockmode);
 extern void mark_index_clustered(Relation rel, Oid indexOid);
 
 extern Oid	make_new_heap(Oid OIDOldHeap, Oid NewTableSpace);
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index 5aff44f..83e24fd 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -1473,6 +1473,127 @@

Re: [HACKERS] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-19 Thread Simon Riggs 
On Sun, 2010-07-18 at 22:47 -0400, Robert Haas wrote:
  But it seems
 that it's far from clear what to do about it, and it's not the job of
 this patch to fix it anyway.

Agreed.

 Regarding the actual patch, it looks mostly good.  Questions:
 
 1. Why in rewriteSupport.c are we adding a call to
 heap_inplace_update() in some situations?  Doesn't seem like this is
 something we should need or want to be monkeying with.

Hmm, yes, that looks like a hangover. Will change. No others similar.

 2. Instead of AlterTableGreatestLockLevel(), how about
 AlterTableGetLockLevel()?  Yeah, it's going to be the highest lock
 level required by any subcommand, but it seems mildly overspecified.
 I don't feel strongly about this one, though, if someone has a strong
 contrary opinion...

I felt it indicated the process it's using. Happy to change.

-- 
 Simon Riggs   www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and 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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-19 Thread Robert Haas 
On Mon, Jul 19, 2010 at 2:46 AM, Simon Riggs si...@2ndquadrant.com wrote:
 On Sun, 2010-07-18 at 22:47 -0400, Robert Haas wrote:
  But it seems
 that it's far from clear what to do about it, and it's not the job of
 this patch to fix it anyway.

 Agreed.

 Regarding the actual patch, it looks mostly good.  Questions:

 1. Why in rewriteSupport.c are we adding a call to
 heap_inplace_update() in some situations?  Doesn't seem like this is
 something we should need or want to be monkeying with.

 Hmm, yes, that looks like a hangover. Will change. No others similar.

 2. Instead of AlterTableGreatestLockLevel(), how about
 AlterTableGetLockLevel()?  Yeah, it's going to be the highest lock
 level required by any subcommand, but it seems mildly overspecified.
 I don't feel strongly about this one, though, if someone has a strong
 contrary opinion...

 I felt it indicated the process it's using. Happy to change.

Cool.  I think with those two changes it's time to commit this.  It's
possible there's some case we've overlooked, but I think that we've
been over this fairly thoroughly, so hopefully not.  Anyway, we're
doing this at the beginning of the 9.1 cycle rather than the end, so
there's hopefully time for any lingering bugs to be found...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres 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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-18 Thread Andres Freund 
On Saturday 17 July 2010 09:55:37 Simon Riggs wrote:
 On Fri, 2010-07-16 at 23:03 +0200, Andres Freund wrote:
  Sure its not that bad, but at least it needs to get documented imho.
  Likely others should chime in here ;-)
 
 Don't understand you. This is a clear bug in join removal, test case
 attached, a minor rework of your original test case.
As shown below the same issue exists in other codepaths that we cant easily fix 
in a stable release :-( - so I think documenting it is the only viable action 
for the back-branches.

  What could the join removal path (and similar places) *possibly* do
  against such a case? Without stopping to use SnapshotNow I dont see
  any way :-(
 The bug is caused by allowing join removal to work in serializable
 transactions. The fix for 9.0 is easy and clear: disallow join removal
 when planning a query as the second or subsequent query in a
 serializable transaction.
 
 A wider fix might be worth doing for 9.1, not sure.

Unfortunately the same issue exists with constraint exclusion - and we can 
hardly disable that for serializable transactions...


CREATE TABLE testconstr(data int);
INSERT INTO testconstr VALUES(1),(10);

T1:
test=# explain analyze SELECT * FROM testconstr WHERE data  5;
  QUERY PLAN
   
---
 Seq Scan on testconstr  (cost=0.00..40.00 rows=800 width=4) (actual 
time=0.029..0.032 rows=1 loops=1)
   Filter: (data  5)
 Total runtime: 0.097 ms
(3 rows)

test=# BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
BEGIN

--make sure we do have a snapshot
test=# SELECT * FROM pg_class WHERE 0 = 1

T2:
DELETE FROM testconstr WHERE data = 5;
ALTER TABLE testconstr ADD CONSTRAINT t CHECK(data  5);

T1:
test=# explain analyze SELECT * FROM testconstr WHERE data  5;
 QUERY PLAN 


 Result  (cost=0.00..0.01 rows=1 width=0) (actual time=0.003..0.003 rows=0 
loops=1)
   One-Time Filter: false
 Total runtime: 0.045 ms
(3 rows)

test=# SET constraint_exclusion = false;
SET
test=# explain analyze SELECT * FROM testconstr WHERE data  5;
  QUERY PLAN
   
---
 Seq Scan on testconstr  (cost=0.00..40.00 rows=800 width=4) (actual 
time=0.030..0.033 rows=1 loops=1)
   Filter: (data  5)
 Total runtime: 0.099 ms
(3 rows)


Thats seems to be an issue that you realistically can hit in production...

I think the same problem exists with inheritance planning - i.e. a child table 
added to a relation in T1 while T2 already holds a snapshot but hasnt used 
that specific table was created will see the new child. Thats less severe but 
still annoying.

Beside using an actual Snapshot in portions of the planner (i.e. stats should 
continue using SnapshotNow) I dont really see a fix here.


Andres

Andres

-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-18 Thread Simon Riggs 
On Sun, 2010-07-18 at 17:28 +0200, Andres Freund wrote:

 Unfortunately the same issue exists with constraint exclusion - and we
 can hardly disable that for serializable transactions...

Then I think the fix is to look at the xmin values on all of the tables
used during planning and ensure that we only use constraint-based
optimisations in a serializable transaction when our top xmin is later
than the last DDL change (via its xmin).

-- 
 Simon Riggs   www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and 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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-18 Thread Andres Freund 
On Sunday 18 July 2010 18:02:26 Simon Riggs wrote:
 On Sun, 2010-07-18 at 17:28 +0200, Andres Freund wrote:
  Unfortunately the same issue exists with constraint exclusion - and we
  can hardly disable that for serializable transactions...
 
 Then I think the fix is to look at the xmin values on all of the tables
 used during planning and ensure that we only use constraint-based
 optimisations in a serializable transaction when our top xmin is later
 than the last DDL change (via its xmin).
Why not just use a the normal snapshot at that point? Any older constraints 
should be just as valid for the tuples visible for the to-be-planned query.
I also think that would lay groundwork for reducing lock-levels further in the 
future.

Andres

-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-18 Thread Tom Lane 
Andres Freund and...@anarazel.de writes:
 On Sunday 18 July 2010 18:02:26 Simon Riggs wrote:
 Then I think the fix is to look at the xmin values on all of the tables
 used during planning and ensure that we only use constraint-based
 optimisations in a serializable transaction when our top xmin is later
 than the last DDL change (via its xmin).

 Why not just use a the normal snapshot at that point?

There isn't a normal snapshot that the planner should be relying on.
It doesn't know what snap the resulting plan will be used with.

I'm unconvinced that this is a problem worth worrying about, but if it
is then Simon's probably got the right idea: check the xmin of a
pg_constraint row before depending on it for planning.  Compare the
handling of indexes made with CREATE INDEX CONCURRENTLY.

regards, tom lane

-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-18 Thread Andres Freund 
On Sunday 18 July 2010 19:20:25 Tom Lane wrote:
 Andres Freund and...@anarazel.de writes:
  On Sunday 18 July 2010 18:02:26 Simon Riggs wrote:
  Then I think the fix is to look at the xmin values on all of the tables
  used during planning and ensure that we only use constraint-based
  optimisations in a serializable transaction when our top xmin is later
  than the last DDL change (via its xmin).
  
  Why not just use a the normal snapshot at that point?
 There isn't a normal snapshot that the planner should be relying on.
 It doesn't know what snap the resulting plan will be used with.
Ok, I will write more stupid stuff in the next paragraph. Feel free to ignore.

What I meant was to use
* the transactions snapshot if we are in a transaction while planning
* SnapshotNow otherwise (not sure if thats a situation really existing - I yet 
have no idea how such utitlity statements are handled snapshot-wise)

The errors I described shouldn't matter for an already existing plan. Also the 
problem with a stale plan is already existing (only slightly aggravated due to 
the change) and should be handled via plan invalidation. Right?

Phantasizing:
If you continued with that you even could allow read only access to tables 
during ALTER TABLE et al. if the actual unlinking of the old filerelnode would 
get moved to the checkpoint or similar...

 I'm unconvinced that this is a problem worth worrying about, but if it
 is then Simon's probably got the right idea: check the xmin of a
 pg_constraint row before depending on it for planning.  Compare the
 handling of indexes made with CREATE INDEX CONCURRENTLY.
I am happy enough to write a docpatch for those issues and leave it there.

Andres

-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-18 Thread Robert Haas 
On Sun, Jul 18, 2010 at 1:20 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Andres Freund and...@anarazel.de writes:
 On Sunday 18 July 2010 18:02:26 Simon Riggs wrote:
 Then I think the fix is to look at the xmin values on all of the tables
 used during planning and ensure that we only use constraint-based
 optimisations in a serializable transaction when our top xmin is later
 than the last DDL change (via its xmin).

 Why not just use a the normal snapshot at that point?

 There isn't a normal snapshot that the planner should be relying on.
 It doesn't know what snap the resulting plan will be used with.

 I'm unconvinced that this is a problem worth worrying about, but if it
 is then Simon's probably got the right idea: check the xmin of a
 pg_constraint row before depending on it for planning.  Compare the
 handling of indexes made with CREATE INDEX CONCURRENTLY.

It generally seems like a Bad Thing to use one snapshot for planning
and another snapshot for execution.  For example, if one transaction
(ostensibly serializable) runs a query twice in a row and in the mean
time some other transaction redefines a function used by that query,
the two runs will return different results, which is inconsistent with
any serial order of execution of those transactions.  But it seems
that it's far from clear what to do about it, and it's not the job of
this patch to fix it anyway.

Regarding the actual patch, it looks mostly good.  Questions:

1. Why in rewriteSupport.c are we adding a call to
heap_inplace_update() in some situations?  Doesn't seem like this is
something we should need or want to be monkeying with.

2. Instead of AlterTableGreatestLockLevel(), how about
AlterTableGetLockLevel()?  Yeah, it's going to be the highest lock
level required by any subcommand, but it seems mildly overspecified.
I don't feel strongly about this one, though, if someone has a strong
contrary opinion...

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres 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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-17 Thread Simon Riggs 
On Fri, 2010-07-16 at 23:03 +0200, Andres Freund wrote:

 Sure its not that bad, but at least it needs to get documented imho.
 Likely others should chime in here ;-)

Don't understand you. This is a clear bug in join removal, test case
attached, a minor rework of your original test case.

 What could the join removal path (and similar places) *possibly* do
 against such a case? Without stopping to use SnapshotNow I dont see
 any way :-(

The bug is caused by allowing join removal to work in serializable
transactions. The fix for 9.0 is easy and clear: disallow join removal
when planning a query as the second or subsequent query in a
serializable transaction.

A wider fix might be worth doing for 9.1, not sure.

-- 
 Simon Riggs   www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and Services
CREATE TABLE testsnap(t int);
CREATE TABLE testsnap2 ();
INSERT INTO testsnap VALUES(1),(1);


T1: 
BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
SELECT count(*) FROM testsnap2; -- serializable snapshot is frozen

T2:
DELETE FROM testsnap;
ALTER TABLE testsnap ADD CONSTRAINT t unique(t);

T1:
SELECT count(*) FROM testsnap;
SELECT t1.* FROM testsnap t1 LEFT JOIN testsnap t2 USING(t);
explain analyze SELECT t1.* FROM testsnap t1 LEFT JOIN testsnap t2 USING(t);


-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-17 Thread Simon Riggs 
On Fri, 2010-07-16 at 20:45 -0400, Tom Lane wrote:
 Andres Freund and...@anarazel.de writes:
  Just to help me: The primary reasons for using SnapshotNow is speed and in 
  some cases correctness (referential integrity). Right? Any other reasons?
 
 Well, the main point for system catalog accesses is that you *must* have
 an up-to-date view of the table schemas.  As an example, if someone just
 added an index to an existing table, it would not do for an INSERT to
 fail to update that index --- no matter whether it's from a serializable
 transaction or not.  So the DDL-executing transaction must hold a lock
 that would block any operation that had better be able to see what it
 did, and once another transaction has acquired the lock that lets it go
 ahead with another operation, it had better see the results of the DDL
 transaction.
 
 However that argument mostly applies to what the executor does.  A plan
 could still be usable despite having been made against a now-obsolete
 version of the table schema.
 
 In the case at hand, I think most constraint-adding situations would
 require at least ShareLock, because they had better block execution of
 INSERT/UPDATE/DELETE operations that could fail to honor the constraint
 if they didn't see it in the catalogs.  But AFAICS, addition of a
 constraint need not block SELECT, and it need not invalidate existing
 plans.
 
 CREATE INDEX uses ShareLock because it's okay to run multiple CREATE
 INDEXes in parallel (thanks to some rather dodgy coding of the catalog
 updates).  For other cases of constraint additions, it might not be
 practical to run two constraint additions in parallel.  In that case we
 could use ShareRowExclusive instead, which is self-exclusive but is not
 any stronger than Share from the perspective of DML commands.  Since
 it's not, I'm unconvinced that it's worth taking any great pains to try
 to make constraint additions run in parallel.

The patch follows all of the above exactly.

-- 
 Simon Riggs   www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and 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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-16 Thread Andres Freund 
Hi Simon,

Your patch implements part of a feature I desire greatly - thanks!

Some comments:

On Thursday 15 July 2010 11:24:27 Simon Riggs wrote:
 ! LOCKMODE
 ! AlterTableGreatestLockLevel(List *cmds)
 ! {
 !ListCell   *lcmd;
 !LOCKMODE lockmode = ShareUpdateExclusiveLock;  /* default for compiler 
 */
Actually its not only for the compiler, its necessary for correctness
as you omit the default at least in the AT_AddConstraint case.

 !
 !foreach(lcmd, cmds)
 !{
 !AlterTableCmd *cmd = (AlterTableCmd *) lfirst(lcmd);
 !LOCKMODE cmd_lockmode  = AccessExclusiveLock; /* default for 
 compiler */
 !
 !switch (cmd-subtype)
 !{
 !/*
 ! * Need AccessExclusiveLock for these subcommands 
 because they
 ! * affect or potentially affect both read and write 
 operations.
 ! *
 ! * New subcommand types should be added here by default.
 ! */
 !case AT_AddColumn:  /* may rewrite 
 heap, in some cases and visible to SELECT */
 !case AT_DropColumn: /* change 
 visible to SELECT */
 !case AT_AddColumnToView:/* CREATE VIEW */
 !case AT_AlterColumnType:/* must rewrite heap */
 !case AT_DropConstraint: /* as DROP INDEX */
 !case AT_AddOids:
 !case AT_DropOids:   /* calls 
 AT_DropColumn */
 !case AT_EnableAlwaysRule:   /* as DROP INDEX */
 !case AT_EnableReplicaRule:  /* as DROP INDEX */
 !case AT_EnableRule: /* as DROP 
 INDEX */
 !case AT_DisableRule:/* as DROP INDEX */
 !case AT_ChangeOwner:/* change visible to 
 SELECT */
 !case AT_SetTableSpace:  /* must rewrite heap */
 !case AT_DropNotNull:/* may change some SQL 
 plans */
 !case AT_SetNotNull:
 !cmd_lockmode = AccessExclusiveLock;
 !break;
 !
 !/*
 ! * These subcommands affect write operations only.
 ! */
 !case AT_ColumnDefault:
 !case AT_ProcessedConstraint:/* becomes 
 AT_AddConstraint */
 !case AT_AddConstraintRecurse:   /* becomes 
 AT_AddConstraint */
 !case AT_EnableTrig:
 !case AT_EnableAlwaysTrig:
 !case AT_EnableReplicaTrig:
 !case AT_EnableTrigAll:
 !case AT_EnableTrigUser:
 !case AT_DisableTrig:
 !case AT_DisableTrigAll:
 !case AT_DisableTrigUser:
 !case AT_AddIndex:   /* from 
 ADD CONSTRAINT */
 !cmd_lockmode = ShareRowExclusiveLock;
 !break;
 !
 !case AT_AddConstraint:
 !if (IsA(cmd-def, Constraint))
 !{
 !Constraint *con = (Constraint *) 
 cmd-def;
 !
 !switch (con-contype)
 !{
 !case CONSTR_EXCLUSION:
 !case CONSTR_PRIMARY:
 !case CONSTR_UNIQUE:
 !/*
 ! * Cases essentially 
 the same as CREATE INDEX. We
 ! * could reduce the 
 lock strength to ShareLock if we
 ! * can work out how to 
 allow concurrent catalog updates.
 ! */
 !cmd_lockmode = 
 ShareRowExclusiveLock;
 !break;
 !case CONSTR_FOREIGN:
 !/*
 ! * We add triggers to 
 both tables when we add a
 ! * Foreign Key, so the 
 lock level must be at least
 ! * as strong as CREATE 
 TRIGGER.
 ! */
 !cmd_lockmode = 
 ShareRowExclusiveLock;
 !break;
 !
 !

Re: [HACKERS] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-16 Thread Andres Freund 
On Friday 16 July 2010 20:41:44 Andres Freund wrote:
  ! */
  !case AT_AddColumn:  /* may
  rewrite heap, in some cases and visible to SELECT */ !
 case AT_DropColumn: /* change
  visible to SELECT */ !case
  AT_AddColumnToView:/* CREATE VIEW */ !case
  AT_AlterColumnType:/* must rewrite heap */ !
 case AT_DropConstraint: /* as DROP INDEX */
  !case AT_AddOids:
  !case AT_DropOids:   /* calls
  AT_DropColumn */ !case
  AT_EnableAlwaysRule:   /* as DROP INDEX */ !
 case AT_EnableReplicaRule:  /* as DROP INDEX */
  !case AT_EnableRule: /* as DROP
  INDEX */
Another remark:

Imho it would be usefull to keep that list in same order as in the enum - 
currently its hard to make sure no case is missing.

Andres

-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-16 Thread Simon Riggs 
On Fri, 2010-07-16 at 20:41 +0200, Andres Freund wrote:

 You argue above that you cant change SET [NOT] NULL to be less
 restrictive because it might change plans - isnt that true for some of the 
 above cases as well?
 
 For example UNIQUE/PRIMARY might make join removal possible - which could
 only be valid after invalid tuples where deleted earlier in that
 transaction. Another case which it influences are grouping plans...

This is only for adding a constraint, not removing it. Join removal
would be possible after the ALTER finishes, but won't change plans
already in progress. The idea is to minimise the impact, not maximise
the benefit of the newly added constraint; I don't think we should block
all queries just because a few might benefit.

 So I think the only case where its actually possible to lower the
 level is CONSTR_EXCLUSION and _FOREIGN.
 The latter might get impossible soon by planner improvements (Peter's
 functional dependencies patch for example).

Same, I don't see why it would block queries.


-- 
 Simon Riggs   www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and 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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-16 Thread Simon Riggs 
On Fri, 2010-07-16 at 21:10 +0200, Andres Freund wrote:
 On Friday 16 July 2010 20:41:44 Andres Freund wrote:
   ! */
   !case AT_AddColumn:  /* may
   rewrite heap, in some cases and visible to SELECT */ !
  case AT_DropColumn: /* change
   visible to SELECT */ !case
   AT_AddColumnToView:/* CREATE VIEW */ !case
   AT_AlterColumnType:/* must rewrite heap */ !
  case AT_DropConstraint: /* as DROP INDEX */
   !case AT_AddOids:
   !case AT_DropOids:   /* calls
   AT_DropColumn */ !case
   AT_EnableAlwaysRule:   /* as DROP INDEX */ !
  case AT_EnableReplicaRule:  /* as DROP INDEX */
   !case AT_EnableRule: /* as DROP
   INDEX */
 Another remark:
 
 Imho it would be usefull to keep that list in same order as in the enum - 
 currently its hard to make sure no case is missing.

Not really; the default case is to reject, so any full test suite will
pick that up.

The cases are ordered by resulting lock type, which seemed the best way
to check we didn't accidentally assign an incorrect lock type.

-- 
 Simon Riggs   www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and 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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-16 Thread Andres Freund 
On Friday 16 July 2010 21:12:33 Simon Riggs wrote:
 On Fri, 2010-07-16 at 20:41 +0200, Andres Freund wrote:
  You argue above that you cant change SET [NOT] NULL to be less
  restrictive because it might change plans - isnt that true for some of
  the above cases as well?
  
  For example UNIQUE/PRIMARY might make join removal possible - which could
  only be valid after invalid tuples where deleted earlier in that
  transaction. Another case which it influences are grouping plans...
 
 This is only for adding a constraint, not removing it. Join removal
 would be possible after the ALTER finishes, but won't change plans
 already in progress. The idea is to minimise the impact, not maximise
 the benefit of the newly added constraint; I don't think we should block
 all queries just because a few might benefit.
Its not about benefit, its about correctness:

CREATE TABLE testsnap(t int);
INSERT INTO testsnap VALUES(1),(1);


T1:
test=# BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
BEGIN
Time: 0.853 ms
test=# explain analyze SELECT t1.* FROM testsnap t1 LEFT JOIN testsnap t2 
USING(t);   
   QUERY PLAN
 
-
  Merge Left Join  (cost=337.49..781.49 rows=28800 width=4) (actual 
time=0.090..0.118 rows=4 loops=1)
Merge Cond: (t1.t = t2.t)
-  Sort  (cost=168.75..174.75 rows=2400 width=4) (actual time=0.049..0.051 
rows=2 loops=1)
  Sort Key: t1.t
  Sort Method:  quicksort  Memory: 25kB
  -  Seq Scan on testsnap t1  (cost=0.00..34.00 rows=2400 width=4) 
(actual time=0.018..0.023 rows=2 loops=1)
-  Sort  (cost=168.75..174.75 rows=2400 width=4) (actual time=0.026..0.033 
rows=3 loops=1)
  Sort Key: t2.t
  Sort Method:  quicksort  Memory: 25kB
  -  Seq Scan on testsnap t2  (cost=0.00..34.00 rows=2400 width=4) 
(actual time=0.005..0.009 rows=2 loops=1)
  Total runtime: 0.279 ms
 (11 rows)


T2:
test=# DELETE FROM testsnap;
DELETE 2
Time: 1.184 ms
test=# ALTER TABLE testsnap ADD CONSTRAINT t unique(t);
NOTICE:  0: ALTER TABLE / ADD UNIQUE will create implicit index t for 
table testsnap
LOCATION:  DefineIndex, indexcmds.c:471
ALTER TABLE
Time: 45.639 ms

T1:
Time: 1.948 ms
test=# explain analyze SELECT t1.* FROM testsnap t1 LEFT JOIN testsnap t2 
USING(t);
  QUERY PLAN
 
-
  Seq Scan on testsnap t1  (cost=0.00..1.02 rows=2 width=4) (actual 
time=0.013..0.016 rows=2 loops=1)
  Total runtime: 0.081 ms
 (2 rows)

Time: 2.004 ms
test=#

boom.



Andres

-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-16 Thread Andres Freund 
On Friday 16 July 2010 21:15:44 Simon Riggs wrote:
 On Fri, 2010-07-16 at 21:10 +0200, Andres Freund wrote:
  On Friday 16 July 2010 20:41:44 Andres Freund wrote:
! */
!case AT_AddColumn:  /* may
rewrite heap, in some cases and visible to SELECT */ !

   case AT_DropColumn: /* change

visible to SELECT */ !case
AT_AddColumnToView:/* CREATE VIEW */ !   
case AT_AlterColumnType:/* must rewrite heap */ !

   case AT_DropConstraint: /* as DROP INDEX
   */

!case AT_AddOids:
!case AT_DropOids:   /*
calls AT_DropColumn */ !case
AT_EnableAlwaysRule:   /* as DROP INDEX */ !

   case AT_EnableReplicaRule:  /* as DROP INDEX
   */

!case AT_EnableRule: /* as
DROP INDEX */
  
  Another remark:
  
  Imho it would be usefull to keep that list in same order as in the enum -
  currently its hard to make sure no case is missing.
 
 Not really; the default case is to reject, so any full test suite will
 pick that up.
 
 The cases are ordered by resulting lock type, which seemed the best way
 to check we didn't accidentally assign an incorrect lock type.
Well, I meant ordering it correctly inside the locktypes, sorry for the 
inprecision.

Andres

-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-16 Thread Simon Riggs 
On Fri, 2010-07-16 at 21:38 +0200, Andres Freund wrote:
 boom

Your test case would still occur in the case where the first query had
not been executed against the same table. So the test case illustrates a
failing of join removal, not of this patch.

-- 
 Simon Riggs   www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and 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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-16 Thread Andres Freund 
On Friday 16 July 2010 22:24:32 Simon Riggs wrote:
 On Fri, 2010-07-16 at 21:38 +0200, Andres Freund wrote:
  boom
 
 Your test case would still occur in the case where the first query had
 not been executed against the same table. So the test case illustrates a
 failing of join removal, not of this patch.
Well, yes. Thats a well known (and documented) issue of pg's serialized 
transactions - which you can protect against quite easily (see the trunctate 
docs for example).
The difference is that I know of no sensible way you sensibly could protect 
against such issues with the patch applied while its easy before(LOCK TABLE 
... IN  SHARE MODE for all used tables).
I know of several sites which have *long* running serialized transactions open 
for analysis and I know there have been other cases of it.

Sure its not that bad, but at least it needs to get documented imho. Likely 
others should chime in here ;-)

What could the join removal path (and similar places) *possibly* do against 
such a case? Without stopping to use SnapshotNow I dont see any way :-(


Andres

-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-16 Thread Tom Lane 
Andres Freund and...@anarazel.de writes:
 What could the join removal path (and similar places) *possibly* do against 
 such a case? Without stopping to use SnapshotNow I dont see any way :-(

But the planner, along with most of the rest of the backend, *does* use
SnapshotNow when examining the system catalogs.

I share your feeling of discomfort but so far I don't see a hole in
Simon's argument.  Adding a constraint should never make a
previously-correct plan incorrect.  Removing one is a very different
story, but he says he's not changing that case.  (Disclaimer: I have
not read the patch.)

regards, tom lane

-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-16 Thread Robert Haas 
On Jul 16, 2010, at 6:01 PM, Tom Lane t...@sss.pgh.pa.us wrote:
 Andres Freund and...@anarazel.de writes:
 What could the join removal path (and similar places) *possibly* do against 
 such a case? Without stopping to use SnapshotNow I dont see any way :-(
 
 But the planner, along with most of the rest of the backend, *does* use
 SnapshotNow when examining the system catalogs.
 
 I share your feeling of discomfort but so far I don't see a hole in
 Simon's argument.  Adding a constraint should never make a
 previously-correct plan incorrect.  Removing one is a very different
 story, but he says he's not changing that case.  (Disclaimer: I have
 not read the patch.)

Perhaps we should start by deciding whether Andres' case is a bug in the first 
place, and then we can argue about whether it's a join-removal bug, a 
lock-weakening bug, or a preexisting bug.

...Robert
-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-16 Thread Andres Freund 
On Saturday 17 July 2010 01:53:24 Robert Haas wrote:
 On Jul 16, 2010, at 6:01 PM, Tom Lane t...@sss.pgh.pa.us wrote:
  Andres Freund and...@anarazel.de writes:
  What could the join removal path (and similar places) *possibly* do
  against such a case? Without stopping to use SnapshotNow I dont see any
  way :-(
  
  But the planner, along with most of the rest of the backend, *does* use
  SnapshotNow when examining the system catalogs.
  
  I share your feeling of discomfort but so far I don't see a hole in
  Simon's argument. 
Neither do I.

  Adding a constraint should never make a
  previously-correct plan incorrect.  Removing one is a very different
  story, but he says he's not changing that case.  (Disclaimer: I have
  not read the patch.)
 
 Perhaps we should start by deciding whether Andres' case is a bug in the
 first place, and then we can argue about whether it's a join-removal bug,
 a lock-weakening bug, or a preexisting bug.
The case where its possible to produce such a case *after* having used/locked 
all participating relations is new I think. 
Being able to create invalid results by doing DDL in another connection on 
not-yet-used tables is at least as old as constraint exclusion. Its a bit 
easier to work around, but thats it.

So I personally would not consider this patch as having a bug anymore 
(thinking helps...).

Whether the general issue is a bug or a to-be-more-exhausitive-documented-
gotcha I have no idea. I know two people having hit it in production - I dont 
think its a that common issue though. Starting with the fact that not that 
many people use serializable.

Just to help me: The primary reasons for using SnapshotNow is speed and in 
some cases correctness (referential integrity). Right? Any other reasons?

Andres


-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-16 Thread Tom Lane 
Andres Freund and...@anarazel.de writes:
 Just to help me: The primary reasons for using SnapshotNow is speed and in 
 some cases correctness (referential integrity). Right? Any other reasons?

Well, the main point for system catalog accesses is that you *must* have
an up-to-date view of the table schemas.  As an example, if someone just
added an index to an existing table, it would not do for an INSERT to
fail to update that index --- no matter whether it's from a serializable
transaction or not.  So the DDL-executing transaction must hold a lock
that would block any operation that had better be able to see what it
did, and once another transaction has acquired the lock that lets it go
ahead with another operation, it had better see the results of the DDL
transaction.

However that argument mostly applies to what the executor does.  A plan
could still be usable despite having been made against a now-obsolete
version of the table schema.

In the case at hand, I think most constraint-adding situations would
require at least ShareLock, because they had better block execution of
INSERT/UPDATE/DELETE operations that could fail to honor the constraint
if they didn't see it in the catalogs.  But AFAICS, addition of a
constraint need not block SELECT, and it need not invalidate existing
plans.

CREATE INDEX uses ShareLock because it's okay to run multiple CREATE
INDEXes in parallel (thanks to some rather dodgy coding of the catalog
updates).  For other cases of constraint additions, it might not be
practical to run two constraint additions in parallel.  In that case we
could use ShareRowExclusive instead, which is self-exclusive but is not
any stronger than Share from the perspective of DML commands.  Since
it's not, I'm unconvinced that it's worth taking any great pains to try
to make constraint additions run in parallel.

regards, tom lane

-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-15 Thread Simon Riggs 
On Thu, 2010-07-08 at 07:16 +0100, Simon Riggs wrote:

 I'll take my previous patch through to completion now

Patch to reduce lock levels for 
 ALTER TABLE
 CREATE TRIGGER
 CREATE RULE

I've completely re-analyzed the required lock levels for sub-commands,
so lock levels can now also be these, if appropriate.
 ShareUpdateExclusiveLock - allows db reads and writes
 ShareRowExclusiveLock - allows db reads only

When ALTER TABLE is specified with multiple subcommands the highest lock
level required by any subcommand is applied to the whole combined
command.

The lock levels are in many ways different from both my own earlier
patch and much of the discussion on this thread, which I have taken to
be general comments rather than considered thought.

Nothing much speculative here, so will commit in a few days barring
objections.

-- 
 Simon Riggs   www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and Services
*** a/doc/src/sgml/mvcc.sgml
--- b/doc/src/sgml/mvcc.sgml
***
*** 532,538  SELECT SUM(value) FROM mytab WHERE class = 2;
  most productnamePostgreSQL/productname commands automatically
  acquire locks of appropriate modes to ensure that referenced
  tables are not dropped or modified in incompatible ways while the
! command executes.  (For example, commandALTER TABLE/ cannot safely be
  executed concurrently with other operations on the same table, so it
  obtains an exclusive lock on the table to enforce that.)
 /para
--- 532,538 
  most productnamePostgreSQL/productname commands automatically
  acquire locks of appropriate modes to ensure that referenced
  tables are not dropped or modified in incompatible ways while the
! command executes.  (For example, commandTRUNCATE/ cannot safely be
  executed concurrently with other operations on the same table, so it
  obtains an exclusive lock on the table to enforce that.)
 /para
***
*** 695,702  SELECT SUM(value) FROM mytab WHERE class = 2;
  /para
  
  para
!  This lock mode is not automatically acquired by any
!  productnamePostgreSQL/productname command.
  /para
 /listitem
/varlistentry
--- 695,703 
  /para
  
  para
!  Acquired by commandCREATE TRIGGER/command,
!  commandCREATE RULE/command (except for literalON SELECT/
!  rules) and in some cases commandALTER TABLE/command.
  /para
 /listitem
/varlistentry
***
*** 742,752  SELECT SUM(value) FROM mytab WHERE class = 2;
  /para
  
  para
!  Acquired by the commandALTER TABLE/command, commandDROP
!  TABLE/command, commandTRUNCATE/command, commandREINDEX/command,
   commandCLUSTER/command, and commandVACUUM FULL/command
!  commands.  This is also the default lock mode for commandLOCK
!  TABLE/command statements that do not specify a mode explicitly.
  /para
 /listitem
/varlistentry
--- 743,754 
  /para
  
  para
!  Acquired by the commandDROP TABLE/command,
!  commandTRUNCATE/command, commandREINDEX/command,
   commandCLUSTER/command, and commandVACUUM FULL/command
!  commands, as well as most variants of commandALTER TABLE/.
!  This is also the default lock mode for commandLOCK TABLE/command
!  statements that do not specify a mode explicitly.
  /para
 /listitem
/varlistentry
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 64,69 
--- 64,70 
  #include rewrite/rewriteHandler.h
  #include storage/bufmgr.h
  #include storage/lmgr.h
+ #include storage/lock.h
  #include storage/smgr.h
  #include utils/acl.h
  #include utils/builtins.h
***
*** 253,273  static void validateForeignKeyConstraint(Constraint *fkconstraint,
  			 Oid pkindOid, Oid constraintOid);
  static void createForeignKeyTriggers(Relation rel, Constraint *fkconstraint,
  		 Oid constraintOid, Oid indexOid);
! static void ATController(Relation rel, List *cmds, bool recurse);
  static void ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd,
! 		  bool recurse, bool recursing);
! static void ATRewriteCatalogs(List **wqueue);
  static void ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel,
! 		  AlterTableCmd *cmd);
  static void ATRewriteTables(List **wqueue);
  static void ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap);
  static AlteredTableInfo *ATGetQueueEntry(List **wqueue, Relation rel);
  static void ATSimplePermissions(Relation rel, bool allowView);
  static void ATSimplePermissionsRelationOrIndex(Relation rel);
  static void ATSimpleRecursion(List **wqueue, Relation rel,
!   AlterTableCmd *cmd, bool recurse);
  static void ATOneLevelRecursion(List **wqueue, Relation rel,
! 	AlterTableCmd *cmd);
  static void

Re: [HACKERS] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-15 Thread Josh Berkus 
On 7/7/10 6:04 PM, Cédric Villemain wrote:
 I just faced production issue where it is impossible to alter table to
 adjust autovacuum settings in a pg8.4. (5K tps, 260M rows table, lock
 too much)

We could try to resolve the COMMENT ON issue with the same mechanism.
What we need is a table lock which blocks other DDL statements, but not DML.

-- 
  -- Josh Berkus
 PostgreSQL Experts Inc.
 http://www.pgexperts.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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-11 Thread Simon Riggs 
On Fri, 2010-07-09 at 15:03 -0400, Robert Haas wrote:
 On Fri, Jul 9, 2010 at 1:18 PM, Simon Riggs si...@2ndquadrant.com wrote:
  On Fri, 2010-07-09 at 13:04 -0400, Robert Haas wrote:
  Tom asked what happens when two transactions attempt to do concurrent
  actions on the same table.  Your response was that we should handle it
  like CREATE INDEX, and handle the update of the pg_class row
  non-transactionally.  But of course, if you use a self-conflicting
  lock at the relation level, then the relation locks conflict and you
  never have to worry about how to update the pg_class entry in the face
  of concurrent updates.
 
  From memory, Tom was also worried about the prospect of people updating
  pg_class directly using SQL. That seems a rare, yet valid concern.
 
 Yes, and it's another another reason why we shouldn't use
 non-transactional updates.
 
 http://archives.postgresql.org/pgsql-hackers/2008-11/msg00744.php
 
  I've already agreed with your point that we should use SHARE UPDATE
  EXCLUSIVE.
 
 The point you seem to be missing is that once we make that decision,
 we can throw all the heap_inplace_update() stuff out the window, and
 the whole problem becomes much simpler.

That is a point I missed. 

Considering this further, it seems we have two conflicting requirements

1. ALTER TABLE ... ADD FOREIGN KEY needs a SHARE mode lock if we want to
run that concurrently with itself and CREATE INDEX operations during a
pg_restore. This was my original goal.

2. In most other cases, SHARE UPDATE EXCLUSIVE is the most useful lock,
especially during heavy operational use.

Since adding an FK requires adding triggers also that puts both of the
above in direct conflict.

ISTM that we should follow (2) and let (1) be added to the TODO for
later work, as an option. I'll followu up on (2).

-- 
 Simon Riggs   www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and 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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-10 Thread Bruce Momjian 
Robert Haas wrote:
 On Wed, Jul 7, 2010 at 9:04 PM, C?dric Villemain
 cedric.villemain.deb...@gmail.com wrote:
   I assume this did not get done for 9.0. ?Do we want a TODO item?
 
  Yes.
 
  Added:
 
  ? ? ? ?Reduce locking required for ALTER commands
 
  I just faced production issue where it is impossible to alter table to
  adjust autovacuum settings in a pg8.4. (5K tps, 260M rows table, lock
  too much)
 
  Can we add some mechanism to prevent that situation also in the TODO item ?
 
  (alternative is actualy to alter other tables and adjust the
  postgresql.conf for biggest tables, but not an ideal solution anyway)
 
 
  ? ? ? ? ? ?* 
  http://archives.postgresql.org/pgsql-hackers/2009-08/msg00533.php
  ? ? ? ? ? ?* 
  http://archives.postgresql.org/pgsql-hackers/2009-10/msg01083.php
  ? ? ? ? ? ?* 
  http://archives.postgresql.org/pgsql-hackers/2010-01/msg02349.php
 
 Bruce, that last link is about something else completely.  Here are
 some better ones:
 
 http://archives.postgresql.org/pgsql-hackers/2008-10/msg01248.php
 http://archives.postgresql.org/pgsql-hackers/2008-10/msg00242.php

Thanks, TODO updated.

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  + None of us is going to be here forever. +

-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-09 Thread Robert Haas 
On Thu, Jul 8, 2010 at 5:09 PM, Simon Riggs si...@2ndquadrant.com wrote:
 On Thu, 2010-07-08 at 06:08 -0400, Robert Haas wrote:
 On Thu, Jul 8, 2010 at 2:16 AM, Simon Riggs si...@2ndquadrant.com wrote:
  On Wed, 2010-07-07 at 22:26 -0400, Robert Haas wrote:
  Rereading the thread, I'm a bit confused by why we're proposing to use
  a SHARE lock; it seems to me that a self-conflicting lock type would
  simplify things.  There's a bunch of discussion on the thread about
  how to handle pg_class updates atomically, but doesn't using a
  self-conflicting lock type eliminate that problem?
 
  The use of the SHARE lock had nothing to do with the pg_class update
  requirement, so that suggestion won't help there.

 Forgive me if I press on this just a bit further, but ISTM that an
 atomic pg_class update functionality isn't intrinsically required,
 because if it were the current code would need it.  So what is
 changing in this patch that makes it necessary when it isn't now?
 ISTM it must be that the lock is weaker.  What am I missing?

 Not sure I follow that logic. Discussion on the requirement is in the
 archives. I don't wish to question that aspect myself.

The relevant link from the archives is here:

http://archives.postgresql.org/pgsql-hackers/2008-10/msg00242.php

Tom asked what happens when two transactions attempt to do concurrent
actions on the same table.  Your response was that we should handle it
like CREATE INDEX, and handle the update of the pg_class row
non-transactionally.  But of course, if you use a self-conflicting
lock at the relation level, then the relation locks conflict and you
never have to worry about how to update the pg_class entry in the face
of concurrent updates.

Which, come to think of it, is probably a good thing, because on
further reflection I'm pretty sure the proposed approach will fall
down completely for some of these operations.  heap_inplace_update()
can only be used when (a) the new tuple is identical in size to the
old tuple, and (b) no action is required on rollback.  That's fine for
updating things like relpages and reltuples (which are just hints
anyway) but it ain't gonna work for changing, say, reloptions, which
is variable-length.  It's also not going to work for changing things
like attstorage, even though a change there can't affect the tuple
size, because modifying the tuple in place won't handle rollbacks
correctly.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres 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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-09 Thread Simon Riggs 
On Fri, 2010-07-09 at 13:04 -0400, Robert Haas wrote:

 Tom asked what happens when two transactions attempt to do concurrent
 actions on the same table.  Your response was that we should handle it
 like CREATE INDEX, and handle the update of the pg_class row
 non-transactionally.  But of course, if you use a self-conflicting
 lock at the relation level, then the relation locks conflict and you
 never have to worry about how to update the pg_class entry in the face
 of concurrent updates. 

From memory, Tom was also worried about the prospect of people updating
pg_class directly using SQL. That seems a rare, yet valid concern.

I've already agreed with your point that we should use SHARE UPDATE
EXCLUSIVE.

-- 
 Simon Riggs   www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and 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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-09 Thread Robert Haas 
On Fri, Jul 9, 2010 at 1:18 PM, Simon Riggs si...@2ndquadrant.com wrote:
 On Fri, 2010-07-09 at 13:04 -0400, Robert Haas wrote:
 Tom asked what happens when two transactions attempt to do concurrent
 actions on the same table.  Your response was that we should handle it
 like CREATE INDEX, and handle the update of the pg_class row
 non-transactionally.  But of course, if you use a self-conflicting
 lock at the relation level, then the relation locks conflict and you
 never have to worry about how to update the pg_class entry in the face
 of concurrent updates.

 From memory, Tom was also worried about the prospect of people updating
 pg_class directly using SQL. That seems a rare, yet valid concern.

Yes, and it's another another reason why we shouldn't use
non-transactional updates.

http://archives.postgresql.org/pgsql-hackers/2008-11/msg00744.php

 I've already agreed with your point that we should use SHARE UPDATE
 EXCLUSIVE.

The point you seem to be missing is that once we make that decision,
we can throw all the heap_inplace_update() stuff out the window, and
the whole problem becomes much simpler.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres 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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-08 Thread Simon Riggs 
On Wed, 2010-07-07 at 22:26 -0400, Robert Haas wrote:

 Rereading the thread, I'm a bit confused by why we're proposing to use
 a SHARE lock; it seems to me that a self-conflicting lock type would
 simplify things.  There's a bunch of discussion on the thread about
 how to handle pg_class updates atomically, but doesn't using a
 self-conflicting lock type eliminate that problem?

The use of the SHARE lock had nothing to do with the pg_class update
requirement, so that suggestion won't help there.

 It strikes me that for the following operations, which don't affect
 queries at all, we could use a SHARE UPDATE EXCLUSIVE, which is likely
 superior to SHARE for this purpose because it wouldn't lock out
 concurrent DML write operations: 

Yes, we can also use SHARE UPDATE EXCLUSIVE for some of them. The use of
SHARE lock was specifically targeted at ADD FOREIGN KEY, to allow
multiple concurrent FKs. Not much use for production however, so SUE
looks better to me.

Not sure I agree with the don't affect queries at all bit

I'll take my previous patch through to completion now, I'm funded to do
that work now. Sept commitfest though.

-- 
 Simon Riggs   www.2ndQuadrant.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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-08 Thread Robert Haas 
On Thu, Jul 8, 2010 at 2:16 AM, Simon Riggs si...@2ndquadrant.com wrote:
 On Wed, 2010-07-07 at 22:26 -0400, Robert Haas wrote:
 Rereading the thread, I'm a bit confused by why we're proposing to use
 a SHARE lock; it seems to me that a self-conflicting lock type would
 simplify things.  There's a bunch of discussion on the thread about
 how to handle pg_class updates atomically, but doesn't using a
 self-conflicting lock type eliminate that problem?

 The use of the SHARE lock had nothing to do with the pg_class update
 requirement, so that suggestion won't help there.

Forgive me if I press on this just a bit further, but ISTM that an
atomic pg_class update functionality isn't intrinsically required,
because if it were the current code would need it.  So what is
changing in this patch that makes it necessary when it isn't now?
ISTM it must be that the lock is weaker.  What am I missing?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres 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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-08 Thread Simon Riggs 
On Thu, 2010-07-08 at 06:08 -0400, Robert Haas wrote:
 On Thu, Jul 8, 2010 at 2:16 AM, Simon Riggs si...@2ndquadrant.com wrote:
  On Wed, 2010-07-07 at 22:26 -0400, Robert Haas wrote:
  Rereading the thread, I'm a bit confused by why we're proposing to use
  a SHARE lock; it seems to me that a self-conflicting lock type would
  simplify things.  There's a bunch of discussion on the thread about
  how to handle pg_class updates atomically, but doesn't using a
  self-conflicting lock type eliminate that problem?
 
  The use of the SHARE lock had nothing to do with the pg_class update
  requirement, so that suggestion won't help there.
 
 Forgive me if I press on this just a bit further, but ISTM that an
 atomic pg_class update functionality isn't intrinsically required,
 because if it were the current code would need it.  So what is
 changing in this patch that makes it necessary when it isn't now?
 ISTM it must be that the lock is weaker.  What am I missing?

Not sure I follow that logic. Discussion on the requirement is in the
archives. I don't wish to question that aspect myself.

-- 
 Simon Riggs   www.2ndQuadrant.com
 PostgreSQL Development, 24x7 Support, Training and 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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-07 Thread Cédric Villemain 
2010/3/3 Bruce Momjian br...@momjian.us:
 Peter Eisentraut wrote:
 On m?n, 2010-02-22 at 10:32 -0500, Bruce Momjian wrote:
  Simon Riggs wrote:
   On Mon, 2009-10-19 at 12:56 -0300, Alvaro Herrera wrote:
Simon Riggs wrote:

 On Fri, 2009-08-07 at 15:58 -0400, Alvaro Herrera wrote:
  Tom Lane wrote:
   Peter Eisentraut pete...@gmx.net writes:
Is there a good reason for $subject, other than that the code 
is entangled
with other ALTER TABLE code?
  
   I think it could be lower, but it would take nontrivial 
   restructuring of
   the ALTER TABLE support.  In particular, consider what happens 
   when you
   have a list of subcommands that don't all require the same lock 
   level.
   I think you'd need to scan the list and find the highest 
   required lock
   level before starting ...
 
  IIRC there was a patch from Simon to address this issue, but it 
  had some
  holes which he didn't have time to close, so it sank.  Maybe this 
  can be
  resurrected and fixed.

 I was intending to finish that patch in this release cycle.
   
Since you're busy with Hot Standby, any chance you could pass it on?
  
   If you'd like. It's mostly finished, just one last thing to finish:
   atomic changes to pg_class via an already agreed API.
 
  I assume this did not get done for 9.0.  Do we want a TODO item?

 Yes.

 Added:

        Reduce locking required for ALTER commands

I just faced production issue where it is impossible to alter table to
adjust autovacuum settings in a pg8.4. (5K tps, 260M rows table, lock
too much)

Can we add some mechanism to prevent that situation also in the TODO item ?

(alternative is actualy to alter other tables and adjust the
postgresql.conf for biggest tables, but not an ideal solution anyway)


            * http://archives.postgresql.org/pgsql-hackers/2009-08/msg00533.php
            * http://archives.postgresql.org/pgsql-hackers/2009-10/msg01083.php
            * http://archives.postgresql.org/pgsql-hackers/2010-01/msg02349.php

 --
  Bruce Momjian  br...@momjian.us        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do

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




-- 
Cédric Villemain   2ndQuadrant
http://2ndQuadrant.fr/ PostgreSQL : Expertise, Formation et Support

-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-07-07 Thread Robert Haas 
On Wed, Jul 7, 2010 at 9:04 PM, Cédric Villemain
cedric.villemain.deb...@gmail.com wrote:
  I assume this did not get done for 9.0.  Do we want a TODO item?

 Yes.

 Added:

        Reduce locking required for ALTER commands

 I just faced production issue where it is impossible to alter table to
 adjust autovacuum settings in a pg8.4. (5K tps, 260M rows table, lock
 too much)

 Can we add some mechanism to prevent that situation also in the TODO item ?

 (alternative is actualy to alter other tables and adjust the
 postgresql.conf for biggest tables, but not an ideal solution anyway)


            * 
 http://archives.postgresql.org/pgsql-hackers/2009-08/msg00533.php
            * 
 http://archives.postgresql.org/pgsql-hackers/2009-10/msg01083.php
            * 
 http://archives.postgresql.org/pgsql-hackers/2010-01/msg02349.php

Bruce, that last link is about something else completely.  Here are
some better ones:

http://archives.postgresql.org/pgsql-hackers/2008-10/msg01248.php
http://archives.postgresql.org/pgsql-hackers/2008-10/msg00242.php

All,

Rereading the thread, I'm a bit confused by why we're proposing to use
a SHARE lock; it seems to me that a self-conflicting lock type would
simplify things.  There's a bunch of discussion on the thread about
how to handle pg_class updates atomically, but doesn't using a
self-conflicting lock type eliminate that problem?

It strikes me that for the following operations, which don't affect
queries at all, we could use a SHARE UPDATE EXCLUSIVE, which is likely
superior to SHARE for this purpose because it wouldn't lock out
concurrent DML write operations:

ALTER [ COLUMN ] column SET STATISTICS integer
ALTER [ COLUMN ] column SET STORAGE { PLAIN | EXTERNAL | EXTENDED | MAIN }
ALTER [ COLUMN ] column SET ( attribute_option = value [, ... ] )
ALTER [ COLUMN ] column RESET ( attribute_option [, ... ] )
CLUSTER ON index_name
SET WITHOUT CLUSTER
SET ( storage_parameter = value [, ... ] )
RESET ( storage_parameter [, ... ] )

(Of the above list, arguably SET STORAGE and [RE]SET (fillfactor) do
in fact affect DML writes, but it seems like changing them on the fly
should still be safe.)

The remaining commands which Simon proposed to downgrade to share-locks were:

ALTER [ COLUMN ] column SET DEFAULT expression
CREATE RULE (only non-ON SELECT rules)
CREATE TRIGGER
ALTER [ COLUMN ] column SET NOT NULL (but not DROP NOT NULL)
ADD table_constraint (but not DROP CONSTRAINT)
DISABLE TRIGGER [ trigger_name | ALL | USER ]
ENABLE TRIGGER [ trigger_name | ALL | USER ]
ENABLE REPLICA TRIGGER trigger_name
ENABLE ALWAYS TRIGGER trigger_name

Setting a column default, creating a non-select RULE, and
creating/disabling a trigger shouldn't affect SELECT statements, so as
long as we lock out all updates we should be OK.  For these it seems
we could use SHARE ROW EXCLUSIVE, which will conflict with any other
DML command and with any data change, but not with SELECTs.

I am somewhat fuzzy on what the correct locking is for SET NOT NULL
and ADD table_constraint.  I believe that the idea here is that a
query plan might rely on the existence of a constraint for
correctness, so we must lock out all queries when dropping one; but a
query plan can't rely on the absence of a constraint for correctness
(since the constraint could be true anyway), so it's safe to allow one
to be added even when there are queries in flight.  If that's correct
then it seems like we could use SHARE ROW EXCLUSIVE for these command
types as well.  However, these two particular commands have another
distinguishing characteristic also: they might run for a while, so it
would be useful to be able to do more than one at once.  So maybe it's
worth thinking a little harder about how to weaken those two in
particular to some non-self-conflicting lock type.  Then again, even
SHARE ROW EXCLUSIVE is a big improvement over ACCESS EXCLUSIVE, so
maybe that would be enough for a first go at the problem.

Thoughts?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres 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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-03-02 Thread Bruce Momjian 
Peter Eisentraut wrote:
 On m?n, 2010-02-22 at 10:32 -0500, Bruce Momjian wrote:
  Simon Riggs wrote:
   On Mon, 2009-10-19 at 12:56 -0300, Alvaro Herrera wrote:
Simon Riggs wrote:
 
 On Fri, 2009-08-07 at 15:58 -0400, Alvaro Herrera wrote:
  Tom Lane wrote:
   Peter Eisentraut pete...@gmx.net writes:
Is there a good reason for $subject, other than that the code 
is entangled 
with other ALTER TABLE code?
   
   I think it could be lower, but it would take nontrivial 
   restructuring of
   the ALTER TABLE support.  In particular, consider what happens 
   when you
   have a list of subcommands that don't all require the same lock 
   level.
   I think you'd need to scan the list and find the highest required 
   lock
   level before starting ...
  
  IIRC there was a patch from Simon to address this issue, but it had 
  some
  holes which he didn't have time to close, so it sank.  Maybe this 
  can be
  resurrected and fixed.
 
 I was intending to finish that patch in this release cycle.

Since you're busy with Hot Standby, any chance you could pass it on?
   
   If you'd like. It's mostly finished, just one last thing to finish:
   atomic changes to pg_class via an already agreed API.
  
  I assume this did not get done for 9.0.  Do we want a TODO item?
 
 Yes.

Added:

Reduce locking required for ALTER commands

* http://archives.postgresql.org/pgsql-hackers/2009-08/msg00533.php
* http://archives.postgresql.org/pgsql-hackers/2009-10/msg01083.php
* http://archives.postgresql.org/pgsql-hackers/2010-01/msg02349.php 

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com

  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do

-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-03-01 Thread Peter Eisentraut 
On mån, 2010-02-22 at 10:32 -0500, Bruce Momjian wrote:
 Simon Riggs wrote:
  On Mon, 2009-10-19 at 12:56 -0300, Alvaro Herrera wrote:
   Simon Riggs wrote:

On Fri, 2009-08-07 at 15:58 -0400, Alvaro Herrera wrote:
 Tom Lane wrote:
  Peter Eisentraut pete...@gmx.net writes:
   Is there a good reason for $subject, other than that the code is 
   entangled 
   with other ALTER TABLE code?
  
  I think it could be lower, but it would take nontrivial 
  restructuring of
  the ALTER TABLE support.  In particular, consider what happens when 
  you
  have a list of subcommands that don't all require the same lock 
  level.
  I think you'd need to scan the list and find the highest required 
  lock
  level before starting ...
 
 IIRC there was a patch from Simon to address this issue, but it had 
 some
 holes which he didn't have time to close, so it sank.  Maybe this can 
 be
 resurrected and fixed.

I was intending to finish that patch in this release cycle.
   
   Since you're busy with Hot Standby, any chance you could pass it on?
  
  If you'd like. It's mostly finished, just one last thing to finish:
  atomic changes to pg_class via an already agreed API.
 
 I assume this did not get done for 9.0.  Do we want a TODO item?

Yes.



-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2010-02-22 Thread Bruce Momjian 
Simon Riggs wrote:
 On Mon, 2009-10-19 at 12:56 -0300, Alvaro Herrera wrote:
  Simon Riggs wrote:
   
   On Fri, 2009-08-07 at 15:58 -0400, Alvaro Herrera wrote:
Tom Lane wrote:
 Peter Eisentraut pete...@gmx.net writes:
  Is there a good reason for $subject, other than that the code is 
  entangled 
  with other ALTER TABLE code?
 
 I think it could be lower, but it would take nontrivial restructuring 
 of
 the ALTER TABLE support.  In particular, consider what happens when 
 you
 have a list of subcommands that don't all require the same lock level.
 I think you'd need to scan the list and find the highest required lock
 level before starting ...

IIRC there was a patch from Simon to address this issue, but it had some
holes which he didn't have time to close, so it sank.  Maybe this can be
resurrected and fixed.
   
   I was intending to finish that patch in this release cycle.
  
  Since you're busy with Hot Standby, any chance you could pass it on?
 
 If you'd like. It's mostly finished, just one last thing to finish:
 atomic changes to pg_class via an already agreed API.

I assume this did not get done for 9.0.  Do we want a TODO item?

-- 
  Bruce Momjian  br...@momjian.ushttp://momjian.us
  EnterpriseDB http://enterprisedb.com
  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do
  + If your life is a hard drive, Christ can be your backup. +

-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2009-10-19 Thread Alvaro Herrera 
Simon Riggs wrote:
 
 On Fri, 2009-08-07 at 15:58 -0400, Alvaro Herrera wrote:
  Tom Lane wrote:
   Peter Eisentraut pete...@gmx.net writes:
Is there a good reason for $subject, other than that the code is 
entangled 
with other ALTER TABLE code?
   
   I think it could be lower, but it would take nontrivial restructuring of
   the ALTER TABLE support.  In particular, consider what happens when you
   have a list of subcommands that don't all require the same lock level.
   I think you'd need to scan the list and find the highest required lock
   level before starting ...
  
  IIRC there was a patch from Simon to address this issue, but it had some
  holes which he didn't have time to close, so it sank.  Maybe this can be
  resurrected and fixed.
 
 I was intending to finish that patch in this release cycle.

Since you're busy with Hot Standby, any chance you could pass it on?


-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2009-10-19 Thread Simon Riggs 
On Mon, 2009-10-19 at 12:56 -0300, Alvaro Herrera wrote:
 Simon Riggs wrote:
  
  On Fri, 2009-08-07 at 15:58 -0400, Alvaro Herrera wrote:
   Tom Lane wrote:
Peter Eisentraut pete...@gmx.net writes:
 Is there a good reason for $subject, other than that the code is 
 entangled 
 with other ALTER TABLE code?

I think it could be lower, but it would take nontrivial restructuring of
the ALTER TABLE support.  In particular, consider what happens when you
have a list of subcommands that don't all require the same lock level.
I think you'd need to scan the list and find the highest required lock
level before starting ...
   
   IIRC there was a patch from Simon to address this issue, but it had some
   holes which he didn't have time to close, so it sank.  Maybe this can be
   resurrected and fixed.
  
  I was intending to finish that patch in this release cycle.
 
 Since you're busy with Hot Standby, any chance you could pass it on?

If you'd like. It's mostly finished, just one last thing to finish:
atomic changes to pg_class via an already agreed API.

-- 
 Simon Riggs   www.2ndQuadrant.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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2009-08-08 Thread Simon Riggs 

On Fri, 2009-08-07 at 15:58 -0400, Alvaro Herrera wrote:
 Tom Lane wrote:
  Peter Eisentraut pete...@gmx.net writes:
   Is there a good reason for $subject, other than that the code is 
   entangled 
   with other ALTER TABLE code?
  
  I think it could be lower, but it would take nontrivial restructuring of
  the ALTER TABLE support.  In particular, consider what happens when you
  have a list of subcommands that don't all require the same lock level.
  I think you'd need to scan the list and find the highest required lock
  level before starting ...
 
 IIRC there was a patch from Simon to address this issue, but it had some
 holes which he didn't have time to close, so it sank.  Maybe this can be
 resurrected and fixed.

I was intending to finish that patch in this release cycle.

MERGE needs further work if you are looking for a project. It isn't
immediately obvious but MERGE logic is a requirement for maintaining
materialized views, which is why I was working on that.

-- 
 Simon Riggs   www.2ndQuadrant.com


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

[HACKERS] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2009-08-07 Thread Peter Eisentraut 
Is there a good reason for $subject, other than that the code is entangled 
with other ALTER TABLE code?

The new SET DISTINCT might be equally affected.

-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2009-08-07 Thread Tom Lane 
Peter Eisentraut pete...@gmx.net writes:
 Is there a good reason for $subject, other than that the code is entangled 
 with other ALTER TABLE code?

I think it could be lower, but it would take nontrivial restructuring of
the ALTER TABLE support.  In particular, consider what happens when you
have a list of subcommands that don't all require the same lock level.
I think you'd need to scan the list and find the highest required lock
level before starting ...

regards, tom lane

-- 
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] ALTER TABLE SET STATISTICS requires AccessExclusiveLock

2009-08-07 Thread Alvaro Herrera 
Tom Lane wrote:
 Peter Eisentraut pete...@gmx.net writes:
  Is there a good reason for $subject, other than that the code is entangled 
  with other ALTER TABLE code?
 
 I think it could be lower, but it would take nontrivial restructuring of
 the ALTER TABLE support.  In particular, consider what happens when you
 have a list of subcommands that don't all require the same lock level.
 I think you'd need to scan the list and find the highest required lock
 level before starting ...

IIRC there was a patch from Simon to address this issue, but it had some
holes which he didn't have time to close, so it sank.  Maybe this can be
resurrected and fixed.

-- 
Alvaro Herrerahttp://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

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