Re: [HACKERS] PATCH: Reducing lock strength of trigger and foreign key DDL
On 04/05/2015 05:56 PM, Simon Riggs wrote: Committed. We move forwards, slowly but surely. Thanks for the patch. Thanks to you and the reviewers for helping me out with this patch! -- Andreas Karlsson -- 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] PATCH: Reducing lock strength of trigger and foreign key DDL
On 5 April 2015 at 19:19, Michael Paquier michael.paqu...@gmail.com wrote: Cool! Thanks for showing up. Visibility Activity. How is REINDEX CONCURRENTLY doing? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, RemoteDBA, 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] PATCH: Reducing lock strength of trigger and foreign key DDL
On Mon, Apr 6, 2015 at 12:56 AM, Simon Riggs si...@2ndquadrant.com wrote: On 7 February 2015 at 20:05, Andreas Karlsson andr...@proxel.se wrote: On 01/30/2015 07:48 AM, Michael Paquier wrote: Looking at the latest patch, it seems that in AlterTableGetLockLevel@tablecmds.c we ought to put AT_ReAddConstraint, AT_AddConstraintRecurse and AT_ProcessedConstraint under the same banner as AT_AddConstraint. Thoughts? A new version of the patch is attached which treats them as the same for locking. I think it is correct and improves readability to do so. Committed. We move forwards, slowly but surely. Thanks for the patch. Cool! Thanks for showing up. -- 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] PATCH: Reducing lock strength of trigger and foreign key DDL
On 7 February 2015 at 20:05, Andreas Karlsson andr...@proxel.se wrote: On 01/30/2015 07:48 AM, Michael Paquier wrote: Looking at the latest patch, it seems that in AlterTableGetLockLevel@tablecmds.c we ought to put AT_ReAddConstraint, AT_AddConstraintRecurse and AT_ProcessedConstraint under the same banner as AT_AddConstraint. Thoughts? A new version of the patch is attached which treats them as the same for locking. I think it is correct and improves readability to do so. Committed. We move forwards, slowly but surely. Thanks for the patch. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, RemoteDBA, 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] PATCH: Reducing lock strength of trigger and foreign key DDL
On Sun, Feb 8, 2015 at 10:05 AM, Andreas Karlsson andr...@proxel.se wrote: On 01/30/2015 07:48 AM, Michael Paquier wrote: Looking at the latest patch, it seems that in AlterTableGetLockLevel@tablecmds.c we ought to put AT_ReAddConstraint, AT_AddConstraintRecurse and AT_ProcessedConstraint under the same banner as AT_AddConstraint. Thoughts? A new version of the patch is attached which treats them as the same for locking. I think it is correct and improves readability to do so. Well then, let's switch it to Ready for committer. I am moving as well this entry to the next CF with the same status. -- 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] PATCH: Reducing lock strength of trigger and foreign key DDL
On 02/06/2015 08:16 AM, Michael Paquier wrote: On Fri, Jan 30, 2015 at 10:26 PM, Andreas Karlsson wrote: On 01/30/2015 07:48 AM, Michael Paquier wrote: Looking at the latest patch, it seems that in AlterTableGetLockLevel@tablecmds.c we ought to put AT_ReAddConstraint, AT_AddConstraintRecurse and AT_ProcessedConstraint under the same banner as AT_AddConstraint. Thoughts? Good point. I think moving those would be a good thing even though it is technically not necessary for AT_AddConstraintRecurse, since that one should only be related to check constraints. Andreas, are you planning to send an updated patch? Yes, I will hopefully send it later today or tomorrow. Andreas -- 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] PATCH: Reducing lock strength of trigger and foreign key DDL
On Fri, Jan 30, 2015 at 10:26 PM, Andreas Karlsson wrote: On 01/30/2015 07:48 AM, Michael Paquier wrote: Looking at the latest patch, it seems that in AlterTableGetLockLevel@tablecmds.c we ought to put AT_ReAddConstraint, AT_AddConstraintRecurse and AT_ProcessedConstraint under the same banner as AT_AddConstraint. Thoughts? Good point. I think moving those would be a good thing even though it is technically not necessary for AT_AddConstraintRecurse, since that one should only be related to check constraints. Andreas, are you planning to send an updated patch? -- 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] PATCH: Reducing lock strength of trigger and foreign key DDL
On 01/30/2015 07:48 AM, Michael Paquier wrote: Ok, so the deal is to finally reduce the locks to ShareRowExclusiveLock for the following commands : - CREATE TRIGGER - ALTER TABLE ENABLE/DISABLE - ALTER TABLE ADD CONSTRAINT Correct. I personally still find this useful enough to justify a patch. Looking at the latest patch, it seems that in AlterTableGetLockLevel@tablecmds.c we ought to put AT_ReAddConstraint, AT_AddConstraintRecurse and AT_ProcessedConstraint under the same banner as AT_AddConstraint. Thoughts? Good point. I think moving those would be a good thing even though it is technically not necessary for AT_AddConstraintRecurse, since that one should only be related to check constraints. -- Andreas -- 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] PATCH: Reducing lock strength of trigger and foreign key DDL
On Fri, Jan 23, 2015 at 8:55 AM, Andreas Karlsson andr...@proxel.se wrote: On 01/22/2015 10:31 PM, Andreas Karlsson wrote: I agree with this view, and am not sure myself that it is worth lowering the lock level of ALTER TRIGGER RENAME. I have attached a patch without the changes to ALTER TRIGGER and ruleutils.c and also fixes the comment issues noted by Andres. Whops, forgot to include the isolation tests. Ok, so the deal is to finally reduce the locks to ShareRowExclusiveLock for the following commands : - CREATE TRIGGER - ALTER TABLE ENABLE/DISABLE - ALTER TABLE ADD CONSTRAINT Looking at the latest patch, it seems that in AlterTableGetLockLevel@tablecmds.c we ought to put AT_ReAddConstraint, AT_AddConstraintRecurse and AT_ProcessedConstraint under the same banner as AT_AddConstraint. Thoughts? -- 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] PATCH: Reducing lock strength of trigger and foreign key DDL
On 01/20/2015 10:08 AM, Noah Misch wrote: Fair enough. It did reinforce pg_get_constraintdef() as a subroutine of pg_dump rather than an independent, rigorous interface. It perhaps made the function worse for non-pg_dump callers. In that vein, each one of these hacks has a cost. One could make a reasonable argument that ALTER TRIGGER RENAME locking is not important enough to justify spreading the hack from pg_get_constraintdef() to pg_get_triggerdef(). Lowering the CREATE TRIGGER lock level does not require any ruleutils.c change for the benefit of pg_dump, because pg_dump won't see the pg_trigger row of a too-recent trigger. I agree with this view, and am not sure myself that it is worth lowering the lock level of ALTER TRIGGER RENAME. I have attached a patch without the changes to ALTER TRIGGER and ruleutils.c and also fixes the comment issues noted by Andres. -- Andreas Karlsson diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml index a0d6867..fc86224 100644 --- a/doc/src/sgml/mvcc.sgml +++ b/doc/src/sgml/mvcc.sgml @@ -908,9 +908,9 @@ ERROR: could not serialize access due to read/write dependencies among transact /para para - This lock mode is not automatically acquired by any - productnamePostgreSQL/productname command. -/para + Acquired by commandCREATE TRIGGER/command and many forms of + commandALTER TABLE/command (see xref linkend=SQL-ALTERTABLE). +/para /listitem /varlistentry @@ -957,9 +957,9 @@ ERROR: could not serialize access due to read/write dependencies among transact commandTRUNCATE/command, commandREINDEX/command, commandCLUSTER/command, and commandVACUUM FULL/command commands. Many forms of commandALTER TABLE/ also acquire - a lock at this level (see xref linkend=SQL-ALTERTABLE). - This is also the default lock mode for commandLOCK TABLE/command - statements that do not specify a mode explicitly. + a lock at this level. This is also the default lock mode for + commandLOCK TABLE/command statements that do not specify + a mode explicitly. /para /listitem /varlistentry diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index b3a4970..f5bbfcd 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -406,6 +406,9 @@ ALTER TABLE ALL IN TABLESPACE replaceable class=PARAMETERname/replaceable mode, and triggers configured as literalENABLE ALWAYS/literal will fire regardless of the current replication mode. /para + para + This command acquires a literalSHARE ROW EXCLUSIVE/literal lock. + /para /listitem /varlistentry diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 66d5083..08aa71b 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -2858,13 +2858,8 @@ AlterTableGetLockLevel(List *cmds) break; /* - * These subcommands affect write operations only. XXX - * Theoretically, these could be ShareRowExclusiveLock. + * These subcommands affect write operations only. */ - case AT_ColumnDefault: - case AT_ProcessedConstraint: /* becomes AT_AddConstraint */ - case AT_AddConstraintRecurse: /* becomes AT_AddConstraint */ - case AT_ReAddConstraint: /* becomes AT_AddConstraint */ case AT_EnableTrig: case AT_EnableAlwaysTrig: case AT_EnableReplicaTrig: @@ -2873,6 +2868,17 @@ AlterTableGetLockLevel(List *cmds) case AT_DisableTrig: case AT_DisableTrigAll: case AT_DisableTrigUser: +cmd_lockmode = ShareRowExclusiveLock; +break; + +/* + * These subcommands affect write operations only. XXX + * Theoretically, these could be ShareRowExclusiveLock. + */ + case AT_ColumnDefault: + case AT_ProcessedConstraint: /* becomes AT_AddConstraint */ + case AT_AddConstraintRecurse: /* becomes AT_AddConstraint */ + case AT_ReAddConstraint: /* becomes AT_AddConstraint */ case AT_AlterConstraint: case AT_AddIndex: /* from ADD CONSTRAINT */ case AT_AddIndexConstraint: @@ -2909,11 +2915,9 @@ AlterTableGetLockLevel(List *cmds) /* * 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. XXX Might be set - * down to ShareRowExclusiveLock though trigger - * info is accessed by pg_get_triggerdef + * as strong as CREATE TRIGGER. */ - cmd_lockmode = AccessExclusiveLock; + cmd_lockmode = ShareRowExclusiveLock; break; default: @@ -6030,16 +6034,13 @@ ATAddForeignKeyConstraint(AlteredTableInfo *tab, Relation rel, ListCell *old_pfeqop_item = list_head(fkconstraint-old_conpfeqop); /* - * Grab an exclusive lock on the pk table, so that someone doesn't delete - * rows out from under
Re: [HACKERS] PATCH: Reducing lock strength of trigger and foreign key DDL
On Fri, Jan 16, 2015 at 04:59:56PM +0100, Andres Freund wrote: On 2015-01-16 15:16:20 +0100, Andreas Karlsson wrote: For this reason I opted to only lower the lock levels of ADD and ALTER TRIGGER, and not DROP TRIGGER. Neither of those require MVCC of then WHEN clause. I'm unconvinced that this is true. Using a snapshot for part of getting a definition certainly opens the door for getting strange results. Acquiring a lock that prevents schema changes on the table and then getting the definition using the syscaches guarantees that that definition is at least self consistent because no further schema changes are possible and the catalog caches will be up2date. What you're doing though is doing part of the scan using the transaction's snapshot (as used by pg_dump that will usually be a repeatable read snapshot and possibly quite old) and the other using a fresh catalog snapshot. This can result in rather odd things. Just consider: S1: CREATE TABLE flubber(id serial primary key, data text); S1: CREATE FUNCTION blarg() RETURNS TRIGGER LANGUAGE plpgsql AS $$BEGIN RETURN NEW; END;$$; S1: CREATE TRIGGER flubber_blarg BEFORE INSERT ON flubber FOR EACH ROW WHEN (NEW.data IS NOT NULL) EXECUTE PROCEDURE blarg(); S2: BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE; S2: SELECT 'dosomethingelse'; S1: ALTER TABLE flubber RENAME TO wasflubber; S1: ALTER TABLE wasflubber RENAME COLUMN data TO wasdata; S1: ALTER TRIGGER flubber_blarg ON wasflubber RENAME TO wasflubber_blarg; S1: ALTER FUNCTION blarg() RENAME TO wasblarg; S2: SELECT pg_get_triggerdef(oid) FROM pg_trigger; This will give you: The old trigger name. The new table name. The new column name. The new function name. I don't think using a snapshot for tiny parts of these functions actually buys anything. Now, this isn't a pattern you introduced. But I think we should think about this for a second before expanding it further. Fair enough. It did reinforce pg_get_constraintdef() as a subroutine of pg_dump rather than an independent, rigorous interface. It perhaps made the function worse for non-pg_dump callers. In that vein, each one of these hacks has a cost. One could make a reasonable argument that ALTER TRIGGER RENAME locking is not important enough to justify spreading the hack from pg_get_constraintdef() to pg_get_triggerdef(). Lowering the CREATE TRIGGER lock level does not require any ruleutils.c change for the benefit of pg_dump, because pg_dump won't see the pg_trigger row of a too-recent trigger. Before you argue that this isn't relevant for pg_dump: It is. Precisely the above can happen - just replace the 'dosomethingelse' with several LOCK TABLEs as pg_dump does. The first blocks after a snapshot has been acquired. While waiting, all the ALTERs happen. We wish pg_dump would take a snapshot of the database; that is, we wish its output always matched some serial execution of transactions. pg_dump has, since ancient times, failed to achieve that if non-table DDL commits during the dump or if table DDL commits between acquiring the dump transaction snapshot and acquiring the last table lock. My reviews have defended the standard that table DDL issued after pg_dump has acquired locks does not change the dump. That's what we bought with pg_get_constraintdef()'s use of the transaction snapshot and would buy with the same in pg_get_triggerdef(). My reviews have deliberately ignored effects on scenarios where pg_dump already fails to guarantee snapshot-like output. Arguably the benefit here is that the window for this issue is becoming smaller as pg_dump (and hopefully other possible callers) acquire exclusive locks on the table. I.e. that the lowering of the lock level doesn't introduce new races. But on the other side of the coin, this makes the result of pg_get_triggerdef() even *more* inaccurate in many cases. What is this about pg_dump acquiring exclusive locks? To summarize, the problem you raise has been out of scope, because it affects pg_dump only at times when pg_dump is already wrong. -- 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] PATCH: Reducing lock strength of trigger and foreign key DDL
On Fri, Jan 16, 2015 at 10:59 AM, Andres Freund and...@2ndquadrant.com wrote: Just consider: S1: CREATE TABLE flubber(id serial primary key, data text); S1: CREATE FUNCTION blarg() RETURNS TRIGGER LANGUAGE plpgsql AS $$BEGIN RETURN NEW; END;$$; S1: CREATE TRIGGER flubber_blarg BEFORE INSERT ON flubber FOR EACH ROW WHEN (NEW.data IS NOT NULL) EXECUTE PROCEDURE blarg(); S2: BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE; S2: SELECT 'dosomethingelse'; S1: ALTER TABLE flubber RENAME TO wasflubber; S1: ALTER TABLE wasflubber RENAME COLUMN data TO wasdata; S1: ALTER TRIGGER flubber_blarg ON wasflubber RENAME TO wasflubber_blarg; S1: ALTER FUNCTION blarg() RENAME TO wasblarg; S2: SELECT pg_get_triggerdef(oid) FROM pg_trigger; This will give you: The old trigger name. The new table name. The new column name. The new function name. Ouch. That's clearly no good. I'm struggling to understand whether this is a problem with our previous analysis, or a problem with this patch: http://www.postgresql.org/message-id/20141028003356.ga387...@tornado.leadboat.com pg_get_triggerdef_worker() relies on generate_function_name(), which uses the system caches, and on get_rule_expr(), for deparsing the WHEN clause. If we allowed only ADDING triggers with a lesser lock and never modifying or dropping them with a lesser lock, then changing the initial scan of pg_trigger at the top of pg_get_triggerdef_worker() to use the transaction snapshot might be OK; if we can see the trigger with the transaction snapshot at all, we know it can't have subsequently changed. But allowing alterations of any kind isn't going to work, so I think our previous analysis on that point was incorrect. I *think* we could fix that if generate_function_name() and get_rule_expr() had an option to use the active snapshot instead of a fresh snapshot. The former doesn't look too hard to arrange, but the latter is a tougher nut to crack. -- 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] PATCH: Reducing lock strength of trigger and foreign key DDL
Hi, /* - * Grab an exclusive lock on the pk table, so that someone doesn't delete - * rows out from under us. (Although a lesser lock would do for that - * purpose, we'll need exclusive lock anyway to add triggers to the pk - * table; trying to start with a lesser lock will just create a risk of - * deadlock.) + * Grab ShareRowExclusiveLock on the pk table, so that someone doesn't + * delete rows out from under us. Note that this does not create risks + * of deadlocks as triggers add added to the pk table using the same + * lock. */ add added doesn't look intended. The rest of the sentence doesn't look entirely right either. /* * Triggers must be on tables or views, and there are additional @@ -526,8 +526,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, * can skip this for internally generated triggers, since the name * modification above should be sufficient. * - * NOTE that this is cool only because we have AccessExclusiveLock on the - * relation, so the trigger set won't be changing underneath us. + * NOTE that this is cool only because of the unique contraint. I fail to see what the unique constraint has to do with this? The previous comment refers to the fact that the AccessExclusiveLock is what prevents a race where another transaction adds a trigger with the same name already exists. Yes, the unique index would, as noted earlier in the comment, catch the error. But that's not the point of the check. Unless I miss something the comment is just as true if you replace the access exclusive with share row exlusive as it's also self conflicting. @@ -1272,8 +1271,7 @@ renametrig(RenameStmt *stmt) * on tgrelid/tgname would complain anyway) and to ensure a trigger does * exist with oldname. * - * NOTE that this is cool only because we have AccessExclusiveLock on the - * relation, so the trigger set won't be changing underneath us. + * NOTE that this is cool only because there is a unique constraint. */ Same as above. tgrel = heap_open(TriggerRelationId, RowExclusiveLock); diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index dd748ac..8eeccf2 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -699,7 +699,8 @@ pg_get_triggerdef_worker(Oid trigid, bool pretty) HeapTuple ht_trig; Form_pg_trigger trigrec; StringInfoData buf; - Relationtgrel; + Snapshotsnapshot = RegisterSnapshot(GetTransactionSnapshot()); + Relationtgrel = heap_open(TriggerRelationId, AccessShareLock); ScanKeyData skey[1]; SysScanDesc tgscan; int findx = 0; @@ -710,18 +711,18 @@ pg_get_triggerdef_worker(Oid trigid, bool pretty) /* * Fetch the pg_trigger tuple by the Oid of the trigger */ - tgrel = heap_open(TriggerRelationId, AccessShareLock); - ScanKeyInit(skey[0], ObjectIdAttributeNumber, BTEqualStrategyNumber, F_OIDEQ, ObjectIdGetDatum(trigid)); tgscan = systable_beginscan(tgrel, TriggerOidIndexId, true, - NULL, 1, skey); + snapshot, 1, skey); ht_trig = systable_getnext(tgscan); + UnregisterSnapshot(snapshot); + if (!HeapTupleIsValid(ht_trig)) elog(ERROR, could not find tuple for trigger %u, trigid); Hm. Pushing the snapshot is supposed to make this fully mvcc? Idon't think that's actually sufficient because of the deparsing of the WHEN clause and of the function name. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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] PATCH: Reducing lock strength of trigger and foreign key DDL
On 01/16/2015 03:01 PM, Andres Freund wrote: Hi, /* -* Grab an exclusive lock on the pk table, so that someone doesn't delete -* rows out from under us. (Although a lesser lock would do for that -* purpose, we'll need exclusive lock anyway to add triggers to the pk -* table; trying to start with a lesser lock will just create a risk of -* deadlock.) +* Grab ShareRowExclusiveLock on the pk table, so that someone doesn't +* delete rows out from under us. Note that this does not create risks +* of deadlocks as triggers add added to the pk table using the same +* lock. */ add added doesn't look intended. The rest of the sentence doesn't look entirely right either. It was intended to be are added, but the sentence is pretty awful anyway. I am not sure the sentence is really necessary anyway. /* * Triggers must be on tables or views, and there are additional @@ -526,8 +526,7 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, * can skip this for internally generated triggers, since the name * modification above should be sufficient. * -* NOTE that this is cool only because we have AccessExclusiveLock on the -* relation, so the trigger set won't be changing underneath us. +* NOTE that this is cool only because of the unique contraint. I fail to see what the unique constraint has to do with this? The previous comment refers to the fact that the AccessExclusiveLock is what prevents a race where another transaction adds a trigger with the same name already exists. Yes, the unique index would, as noted earlier in the comment, catch the error. But that's not the point of the check. Unless I miss something the comment is just as true if you replace the access exclusive with share row exlusive as it's also self conflicting. Yeah, this must have been a remainder from the version where I only grabbed a ShareLock. The comment should be restored. Hm. Pushing the snapshot is supposed to make this fully mvcc? Idon't think that's actually sufficient because of the deparsing of the WHEN clause and of the function name. Indeed. As Noah and I discussed previously in this thread we would need to do quite a bit of refactoring of ruleutils.c to make it fully MVCC. For this reason I opted to only lower the lock levels of ADD and ALTER TRIGGER, and not DROP TRIGGER. Neither of those require MVCC of then WHEN clause. Andreas -- 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] PATCH: Reducing lock strength of trigger and foreign key DDL
On 2015-01-16 15:16:20 +0100, Andreas Karlsson wrote: Indeed. As Noah and I discussed previously in this thread we would need to do quite a bit of refactoring of ruleutils.c to make it fully MVCC. Right. For this reason I opted to only lower the lock levels of ADD and ALTER TRIGGER, and not DROP TRIGGER. Neither of those require MVCC of then WHEN clause. I'm unconvinced that this is true. Using a snapshot for part of getting a definition certainly opens the door for getting strange results. Acquiring a lock that prevents schema changes on the table and then getting the definition using the syscaches guarantees that that definition is at least self consistent because no further schema changes are possible and the catalog caches will be up2date. What you're doing though is doing part of the scan using the transaction's snapshot (as used by pg_dump that will usually be a repeatable read snapshot and possibly quite old) and the other using a fresh catalog snapshot. This can result in rather odd things. Just consider: S1: CREATE TABLE flubber(id serial primary key, data text); S1: CREATE FUNCTION blarg() RETURNS TRIGGER LANGUAGE plpgsql AS $$BEGIN RETURN NEW; END;$$; S1: CREATE TRIGGER flubber_blarg BEFORE INSERT ON flubber FOR EACH ROW WHEN (NEW.data IS NOT NULL) EXECUTE PROCEDURE blarg(); S2: BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE; S2: SELECT 'dosomethingelse'; S1: ALTER TABLE flubber RENAME TO wasflubber; S1: ALTER TABLE wasflubber RENAME COLUMN data TO wasdata; S1: ALTER TRIGGER flubber_blarg ON wasflubber RENAME TO wasflubber_blarg; S1: ALTER FUNCTION blarg() RENAME TO wasblarg; S2: SELECT pg_get_triggerdef(oid) FROM pg_trigger; This will give you: The old trigger name. The new table name. The new column name. The new function name. I don't think using a snapshot for tiny parts of these functions actually buys anything. Now, this isn't a pattern you introduced. But I think we should think about this for a second before expanding it further. Before you argue that this isn't relevant for pg_dump: It is. Precisely the above can happen - just replace the 'dosomethingelse' with several LOCK TABLEs as pg_dump does. The first blocks after a snapshot has been acquired. While waiting, all the ALTERs happen. Arguably the benefit here is that the window for this issue is becoming smaller as pg_dump (and hopefully other possible callers) acquire exclusive locks on the table. I.e. that the lowering of the lock level doesn't introduce new races. But on the other side of the coin, this makes the result of pg_get_triggerdef() even *more* inaccurate in many cases. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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] PATCH: Reducing lock strength of trigger and foreign key DDL
On 01/14/2015 08:48 AM, Michael Paquier wrote: All those things gathered give the patch attached. Andreas, if you are fine with it I think that we could pass it to a committer. Excellent changes. Thanks for the patch and the reviews. -- Andreas Karlsson -- 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] PATCH: Reducing lock strength of trigger and foreign key DDL
I wrote: I think that we could pass it to a committer. Marked as such. -- 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] PATCH: Reducing lock strength of trigger and foreign key DDL
On Sun, Dec 14, 2014 at 5:45 AM, Andreas Karlsson andr...@proxel.se wrote: get_rule_expr() relies heavily on the catcache which to me does not look like it could easily be (and probably not even should be) made to use the current snapshot. Refactoring ruleutils.c to rely less no the catcache seems like a reasonable thing to do if we want to reduce weirdness of how it ignores MVCC but it is quite a bit of work and I fear it could give us performance regressions. Do you have any ideas for how to fix get_rule_expr()? Agreed. There are 20 calls to SearchSysCache in ruleutils.c, let's not focus on that for now though and get things right for this patch. Is this patch worthwhile even without reducing the lock levels of the drop commands? Yes. IMV, it is better to do this work slowly and incrementally. Here are some comments about this patch: 1) There is no documentation. Could you update mvcc.sgml for SHARE ROW EXCLUSIVE? 2) Some isolation tests would be welcome, the point of the feature is to test if SELECT and SELECT FOR SHARE/UPDATE are allowed while running one of the command mentioned above. 3) This patch breaks the isolation test alter-table-1. 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] PATCH: Reducing lock strength of trigger and foreign key DDL
I'm not sure about the rest of this but... On Sat, Dec 13, 2014 at 3:45 PM, Andreas Karlsson andr...@proxel.se wrote: Is this patch worthwhile even without reducing the lock levels of the drop commands? Yes! It certainly makes more sense to reduce the lock levels where we can do that relatively easily, and postpone work on related projects that are harder, rather than waiting until it all seems to work before doing anything at all. -- 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
