Re: PATCH: Reducing lock strength of trigger and foreign key DDL

Hi,

I have been thinking about why we need to grab an AccessExclusiveLock on
the table with the PK when we add a foreign key. Adding new tables with
foreign keys to old ones is common so it would be really nice if the
lock strength could be reduced.

A comment in the code claims that we need to lock so no rows are deleted
under us and that adding a trigger will lock in AccessExclusive anyway.
But with MVCC catalog access and the removal of SnapshotNow I do not see
why adding a trigger would require an exclusive lock. Just locking for
data changes should be enough.

Looking at the code the only see the duplicate name check use the fact
that we have grabbed an exclusive lock and that should work anyway due
to the unique constraint, but since I am pretty new to the code base I
could be missing something obvious. I have attached a proof of concept
patch which reduces the lock strength to ShareLock.

What do you say? Am I missing something?

My gut feel also says that if we know it is a RI trigger we are adding
then AccessShare should be enough for the PK table, since we could rely
on row locks to prevent rows from being deleted. It would be really nice
though if this was possible since this would make it possible to add a
new table with foreign keys and data without locking anything more than
the referred rows.

Andreas

Andreas Karlsson <andreas(at)proxel(dot)se> writes:
> I have attached a proof of concept
> patch which reduces the lock strength to ShareLock.

You're kidding, right? ShareLock isn't even self-exclusive.

regards, tom lane

On 10/22/2014 04:13 PM, Tom Lane wrote:
> Andreas Karlsson <andreas(at)proxel(dot)se> writes:
>> I have attached a proof of concept
>> patch which reduces the lock strength to ShareLock.
>
> You're kidding, right? ShareLock isn't even self-exclusive.

Why would it have to be self-exclusive? As far as I know we only need to
ensure that nobody changes the rows while we add the trigger. Adding
multiple triggers concurrently should not pose a problem unless I am
missing something (which I probably am).

Andreas

On Wed, Oct 22, 2014 at 3:06 AM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> I have been thinking about why we need to grab an AccessExclusiveLock on the
> table with the PK when we add a foreign key. Adding new tables with foreign
> keys to old ones is common so it would be really nice if the lock strength
> could be reduced.
>
> A comment in the code claims that we need to lock so no rows are deleted
> under us and that adding a trigger will lock in AccessExclusive anyway. But
> with MVCC catalog access and the removal of SnapshotNow I do not see why
> adding a trigger would require an exclusive lock. Just locking for data
> changes should be enough.

The use of MVCC catalog access doesn't necessarily mean that adding a
trigger doesn't require an AccessExclusive lock. Those changes - if I
dare to say so myself - solved a complex and important problem, but
only one of many problems in this area, and people seem prone to
thinking that they solved more problems than they in fact did.

I think instead of focusing on foreign keys, we should rewind a bit
and think about the locking level required to add a trigger. If we
figure something out there, then we can consider how it affects
foreign keys. I went looking for previous discussions of remaining
hazards and found these postings:

http://www.postgresql.org/message-id/CA+TgmoY4GLsXZk0tAO29-LJtcuj0SL1xWCwQ51xb-HFYsgi5RQ@mail.gmail.com
http://www.postgresql.org/message-id/20893.1393892127@sss.pgh.pa.us
http://www.postgresql.org/message-id/20140306224340.GA3551655@tornado.leadboat.com

As far as triggers are concerned, the issue of skew between the
transaction snapshot and what the ruleutils.c snapshots do seems to be
the principal issue. Commit e5550d5fec66aa74caad1f79b79826ec64898688
changed pg_get_constraintdef() to use an MVCC snapshot rather than a
current MVCC snapshot; if that change is safe, I am not aware of any
reason why we couldn't change pg_get_triggerdef() similarly. Barring
further hazards I haven't thought of, I would expect that we could add
a trigger to a relation with only ShareRowExclusiveLock. Anything
less than ShareRowExclusiveLock would open up strange timing races
around the firing of triggers by transactions writing the table: they
might or might not notice that a trigger had been added before
end-of-transaction, depending on the timing of cache flushes, which
certainly seems no good. But even RowExclusiveLock seems like a large
improvement over AccessExclusiveLock.

When a constraint trigger - which is used to implement a foreign key -
is added, there are actually TWO tables involved: the table upon which
the trigger will actually fire, and some other table which is
mentioned in passing in the trigger definition. It's possible that
the locking requirements for the secondary table are weaker since I
don't think the presence of the trigger actually affects runtime
behavior there. However, there's probably little point in try to
weaken the lock to less than the level required for the main table
because a foreign key involves adding referential integrity triggers
to both tables.

So I tentatively propose (and with due regard for the possibility
others may see dangers that I've missed) that a reasonable goal would
be to lower the lock strength required for both CREATE TRIGGER and ADD
FOREIGN KEY from AccessExclusiveLock to ShareRowExclusiveLock,
allowing concurrent SELECT and SELECT FOR SHARE against the tables,
but not any actual write operations.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Oct 24, 2014 at 12:07:42PM -0400, Robert Haas wrote:
> I think instead of focusing on foreign keys, we should rewind a bit
> and think about the locking level required to add a trigger.

Agreed.

> http://www.postgresql.org/message-id/CA+TgmoY4GLsXZk0tAO29-LJtcuj0SL1xWCwQ51xb-HFYsgi5RQ@mail.gmail.com
> http://www.postgresql.org/message-id/20893.1393892127@sss.pgh.pa.us
> http://www.postgresql.org/message-id/20140306224340.GA3551655@tornado.leadboat.com
>
> As far as triggers are concerned, the issue of skew between the
> transaction snapshot and what the ruleutils.c snapshots do seems to be
> the principal issue. Commit e5550d5fec66aa74caad1f79b79826ec64898688
> changed pg_get_constraintdef() to use an MVCC snapshot rather than a
> current MVCC snapshot; if that change is safe, I am not aware of any
> reason why we couldn't change pg_get_triggerdef() similarly.

pg_get_triggerdef() is fine as-is with concurrent CREATE TRIGGER. The
pg_get_constraintdef() change arose to ensure a consistent result when
concurrent ALTER TABLE VALIDATE CONSTRAINT mutates a constraint definition.
(Reducing the lock level of DROP TRIGGER or ALTER TRIGGER, however, would
create the analogous problem for pg_get_triggerdef().)

> So I tentatively propose (and with due regard for the possibility
> others may see dangers that I've missed) that a reasonable goal would
> be to lower the lock strength required for both CREATE TRIGGER and ADD
> FOREIGN KEY from AccessExclusiveLock to ShareRowExclusiveLock,
> allowing concurrent SELECT and SELECT FOR SHARE against the tables,
> but not any actual write operations.

+1

On 10/24/2014 06:07 PM, Robert Haas wrote:
> I think instead of focusing on foreign keys, we should rewind a bit
> and think about the locking level required to add a trigger.

Agreed.

> As far as triggers are concerned, the issue of skew between the
> transaction snapshot and what the ruleutils.c snapshots do seems to be
> the principal issue. Commit e5550d5fec66aa74caad1f79b79826ec64898688
> changed pg_get_constraintdef() to use an MVCC snapshot rather than a
> current MVCC snapshot; if that change is safe, I am not aware of any
> reason why we couldn't change pg_get_triggerdef() similarly. Barring
> further hazards I haven't thought of, I would expect that we could add
> a trigger to a relation with only ShareRowExclusiveLock.

Thanks for the info. This is just the kind of issues I was worrying about.

> Anything
> less than ShareRowExclusiveLock would open up strange timing races
> around the firing of triggers by transactions writing the table: they
> might or might not notice that a trigger had been added before
> end-of-transaction, depending on the timing of cache flushes, which
> certainly seems no good. But even RowExclusiveLock seems like a large
> improvement over AccessExclusiveLock.

Would not ShareLock give the same result, except for also allowing
concurrent CREATE INDEX and concurrent other CREATE TRIGGER which does
not look dangerous to me?

From a user point of view ShareRowExclusiveLock should be as useful as
ShareLock.

> When a constraint trigger - which is used to implement a foreign key -
> is added, there are actually TWO tables involved: the table upon which
> the trigger will actually fire, and some other table which is
> mentioned in passing in the trigger definition. It's possible that
> the locking requirements for the secondary table are weaker since I
> don't think the presence of the trigger actually affects runtime
> behavior there. However, there's probably little point in try to
> weaken the lock to less than the level required for the main table
> because a foreign key involves adding referential integrity triggers
> to both tables.
>
> So I tentatively propose (and with due regard for the possibility
> others may see dangers that I've missed) that a reasonable goal would
> be to lower the lock strength required for both CREATE TRIGGER and ADD
> FOREIGN KEY from AccessExclusiveLock to ShareRowExclusiveLock,
> allowing concurrent SELECT and SELECT FOR SHARE against the tables,
> but not any actual write operations.

Agreed.. But I think reducing the lock level of the secondary table is
much more important than doing the same for the primary table due to the
case where the secondary table is an existing table which is hit by a
workload of long running queries and DML while the primary is a new
table which is added now. In my dream world I could add the new table
without any disruption at all of queries using the secondary table, no
matter the duration of the transaction adding the table (barring
insertion of actual data into the primary table, which would take row
locks).

This is just a dream scenario though, and focusing on triggers is indeed
the reasonable goal for 9.5.

--
Andreas Karlsson

There are actually TWO tables involved: the table upon which
the trigger will actually fire, and some other table which is
mentioned in passing in the trigger definition. It's possible that
the locking requirements for the secondary table are weaker since I
don't think the presence of the trigger actually affects runtime
behavior there. However, there's probably little point in try to
weaken the lock to less than the level required for the main table
because a foreign key involves adding referential integrity triggers
to both tables.

-----
GUL
--
View this message in context: http://postgresql.1045698.n5.nabble.com/Reducing-lock-strength-of-adding-foreign-keys-tp5823894p5824376.html
Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.

Thanks for weighing in, Noah.

On Sat, Oct 25, 2014 at 2:00 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
>> http://www.postgresql.org/message-id/CA+TgmoY4GLsXZk0tAO29-LJtcuj0SL1xWCwQ51xb-HFYsgi5RQ@mail.gmail.com
>> http://www.postgresql.org/message-id/20893.1393892127@sss.pgh.pa.us
>> http://www.postgresql.org/message-id/20140306224340.GA3551655@tornado.leadboat.com
>>
>> As far as triggers are concerned, the issue of skew between the
>> transaction snapshot and what the ruleutils.c snapshots do seems to be
>> the principal issue. Commit e5550d5fec66aa74caad1f79b79826ec64898688
>> changed pg_get_constraintdef() to use an MVCC snapshot rather than a
>> current MVCC snapshot; if that change is safe, I am not aware of any
>> reason why we couldn't change pg_get_triggerdef() similarly.
>
> pg_get_triggerdef() is fine as-is with concurrent CREATE TRIGGER. The
> pg_get_constraintdef() change arose to ensure a consistent result when
> concurrent ALTER TABLE VALIDATE CONSTRAINT mutates a constraint definition.
> (Reducing the lock level of DROP TRIGGER or ALTER TRIGGER, however, would
> create the analogous problem for pg_get_triggerdef().)

Maybe so, but I'd favor changing it anyway and getting it over with.
The current situation seems to have little to recommend it; moreover,
it would be nice, if it's possible and safe, to weaken the lock levels
for all three of those commands at the same time. Do you see any
hazards for ALTER or DROP that do not exist for CREATE?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sun, Oct 26, 2014 at 9:48 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> Agreed.. But I think reducing the lock level of the secondary table is much
> more important than doing the same for the primary table due to the case
> where the secondary table is an existing table which is hit by a workload of
> long running queries and DML while the primary is a new table which is added
> now. In my dream world I could add the new table without any disruption at
> all of queries using the secondary table, no matter the duration of the
> transaction adding the table (barring insertion of actual data into the
> primary table, which would take row locks).

That would indeed be nice, but it doesn't seem very practical, because
the parent needs triggers, too: if you try to delete a row from the
parent, or update the key, it's got to go look at the child and see
whether there are rows against the old value. Then it's got to either
update those rows, or null out the value, or throw an error.
Regardless of which of those things it does (which depends on the ON
DELETE and ON UPDATE settings you choose), it's hard to imagine that
it would be a good idea for any of those things to start happening in
the middle of a transaction or statement.

So, let's take what we can get. :-)

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Oct 27, 2014 at 08:24:15AM -0400, Robert Haas wrote:
> On Sat, Oct 25, 2014 at 2:00 PM, Noah Misch <noah(at)leadboat(dot)com> wrote:
> >> http://www.postgresql.org/message-id/CA+TgmoY4GLsXZk0tAO29-LJtcuj0SL1xWCwQ51xb-HFYsgi5RQ@mail.gmail.com
> >> http://www.postgresql.org/message-id/20893.1393892127@sss.pgh.pa.us
> >> http://www.postgresql.org/message-id/20140306224340.GA3551655@tornado.leadboat.com
> >>
> >> As far as triggers are concerned, the issue of skew between the
> >> transaction snapshot and what the ruleutils.c snapshots do seems to be
> >> the principal issue. Commit e5550d5fec66aa74caad1f79b79826ec64898688
> >> changed pg_get_constraintdef() to use an MVCC snapshot rather than a
> >> current MVCC snapshot; if that change is safe, I am not aware of any
> >> reason why we couldn't change pg_get_triggerdef() similarly.
> >
> > pg_get_triggerdef() is fine as-is with concurrent CREATE TRIGGER. The
> > pg_get_constraintdef() change arose to ensure a consistent result when
> > concurrent ALTER TABLE VALIDATE CONSTRAINT mutates a constraint definition.
> > (Reducing the lock level of DROP TRIGGER or ALTER TRIGGER, however, would
> > create the analogous problem for pg_get_triggerdef().)
>
> Maybe so, but I'd favor changing it anyway and getting it over with.
> The current situation seems to have little to recommend it; moreover,
> it would be nice, if it's possible and safe, to weaken the lock levels
> for all three of those commands at the same time. Do you see any
> hazards for ALTER or DROP that do not exist for CREATE?

ALTER TRIGGER is not bad; like you say, change pg_get_triggerdef_worker() the
way commit e5550d5 changed pg_get_constraintdef_worker(). DROP TRIGGER is
more difficult. pg_constraint.tgqual of a dropped trigger may reference other
dropped objects, which calls for equipping get_rule_expr() to use the
transaction snapshot. That implicates quite a bit of ruleutils.c code.

On 10/28/2014 01:33 AM, Noah Misch wrote:
> ALTER TRIGGER is not bad; like you say, change pg_get_triggerdef_worker() the
> way commit e5550d5 changed pg_get_constraintdef_worker(). DROP TRIGGER is
> more difficult. pg_constraint.tgqual of a dropped trigger may reference other
> dropped objects, which calls for equipping get_rule_expr() to use the
> transaction snapshot. That implicates quite a bit of ruleutils.c code.

I started looking into this again and fixed
pg_get_constraintdef_worker() as suggested.

But I have no idea how to fix get_rule_expr() since it relies on doing
lookups in the catcache. Replacing these with uncached lookups sounds
like it could cause quite some slowdown. Any ideas?

Indexes should suffer from the same problems since they too have emay
contain expressions but they seem to solve this by having a higher lock
level on DROP INDEX, but I do wonder about the CONCURRENTLY case.

By the way, unless I am mistaken there is currently no protection
against having a concurrent ALTER FUNCTION ... RENAME mess up what is
dumped in by pg_get_triggerdef().

--
Andreas Karlsson

Hi,

I have attached a patch with the current status of my work on reducing
the lock level of trigger and foreign key related DDL.

This commit reduces the lock level of the following commands from ACCESS
EXCLUSIVE to SHARE ROW EXCLUSIVE, plus that it does the same for the
referred table of the foreign key.

ADD TRIGGER
ALTER TRIGGER
ALTER TABLE ... ADD FOREIGN KEY
ALTER TABLE ... DISABLE TRIGGER
ALTER TABLE ... ENABLE TRIGGER

The patch does currently not reducing the lock level of the following
two commands, but I think it would be really nice to fix those too and
here I would like some feedback and ideas.

DROP TRIGGER
ALTER TABLE ... DROP CONSTRAINT -- For foreign keys

Foreign keys and triggers are fixed at the same time because foreign
keys are implemented with two triggers, one at each of the involved tables.

The idea behind the patch is that since we started using MVCC snapshots
we no longer need to hold an exclusive lock on the table when adding a
trigger. Theoretically we only need to lock out writers of the rows of
the table (SHARE ROW EXCLUSIVE/SHARE), but as noted by Robert and Noah
just reducing the lock level will break pg_dump since
pg_get_triggerdef() does not use the current snapshot when reading the
catalog.

I have fixed pg_get_triggerdef() to use the snapshot like how
e5550d5fec66aa74caad1f79b79826ec64898688 fixed pg_get_constraintdef(),
and this fixes the code for the ALTER TRIGGER case (ADD TRIGGER was
already safe). But to fix it for the DROP TRIGGER case we need to also
make the dumping of the WHEN clause (which is dumped by get_rule_expr())
use the current snapshot.

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()? Is this patch
worthwhile even without reducing the lock levels of the drop commands?

Andreas

I'm not sure about the rest of this but...

On Sat, Dec 13, 2014 at 3:45 PM, Andreas Karlsson <andreas(at)proxel(dot)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

On 25 October 2014 19:00, Noah Misch <noah(at)leadboat(dot)com> wrote:

>> So I tentatively propose (and with due regard for the possibility
>> others may see dangers that I've missed) that a reasonable goal would
>> be to lower the lock strength required for both CREATE TRIGGER and ADD
>> FOREIGN KEY from AccessExclusiveLock to ShareRowExclusiveLock,
>> allowing concurrent SELECT and SELECT FOR SHARE against the tables,
>> but not any actual write operations.
>
> +1

All of this is just a replay of the earlier conversations about
reducing lock levels.

My original patch did reduce lock levels for CREATE TRIGGER, but we
stepped back from doing that in 9.4 until we had feedback as to
whether the whole idea of reducing lock levels was safe, which Robert,
Tom and others were slightly uncertain about.

Is there anything different here from work in my original patch series?

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Dec 17, 2014 at 02:41:39PM +0000, Simon Riggs wrote:
> Is there anything different here from work in my original patch series?

Not to my knowledge.

On 12/17/2014 03:41 PM, Simon Riggs wrote:
> All of this is just a replay of the earlier conversations about
> reducing lock levels.
>
> My original patch did reduce lock levels for CREATE TRIGGER, but we
> stepped back from doing that in 9.4 until we had feedback as to
> whether the whole idea of reducing lock levels was safe, which Robert,
> Tom and others were slightly uncertain about.
>
> Is there anything different here from work in my original patch series?

As far as I know, no.

--
Andreas Karlsson

On Sun, Dec 14, 2014 at 5:45 AM, Andreas Karlsson <andreas(at)proxel(dot)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

Thanks for the review.

Here is a new version of the patch with updated isolation tests and
documentation.

--
Andreas Karlsson

On Tue, Jan 13, 2015 at 5:21 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> Here is a new version of the patch with updated isolation tests and
> documentation.
Thanks.

Some more comments:
- It would be good to have as well tests for ENABLE/DISABLE TRIGGER,
the locks of those commands being reduced as well.
- Patch has some typos, like "an share".
- Documentation can be simplified. Listing the flavors of ALTER TABLE
taking ROW SHARE EXCLUSIVE in mvcc/sgml is clumsy IMO, it is better to
mention that in alter_table.sgml and have a link pointing to the page
of ALTER TABLE in mvcc.sgml.
- In ATAddForeignKeyConstraint, I think that it would be better to not
remove the comment, adding instead that ShareRowExclusiveLock is
actually safe because CREATE TRIGGER uses this level of lock.
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.
Regards,
--
Michael

I wrote:
> I think that we could pass it to a committer.
Marked as such.
--
Michael

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

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;
> - Relation tgrel;
> + Snapshot snapshot = RegisterSnapshot(GetTransactionSnapshot());
> + Relation tgrel = 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

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

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

On Fri, Jan 16, 2015 at 10:59 AM, Andres Freund <andres(at)2ndquadrant(dot)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.GA387814@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

On 01/19/2015 06:14 PM, Robert Haas wrote:
> On Fri, Jan 16, 2015 at 10:59 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> 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.GA387814@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.

The function name in the trigger should already be broken in master
since changing the name of a function does not lock the table. Would be
really neat to fix though.

A possible way forward I see here is to undo my changes to the lock
level of ALTER TRIGGER and of pg_get_triggerdef_worker(). This should
work if Noah is correct about his analysis in where he came to the
conclusion: "pg_get_triggerdef() is fine as-is with concurrent CREATE
TRIGGER.". Is this still true with the new information?

--
Andreas Karlsson

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.

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

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.

--
Andreas Karlsson

On Fri, Jan 23, 2015 at 8:55 AM, Andreas Karlsson <andreas(at)proxel(dot)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(at)tablecmds(dot)c we ought to put AT_ReAddConstraint,
AT_AddConstraintRecurse and AT_ProcessedConstraint under the same
banner as AT_AddConstraint. Thoughts?
--
Michael

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(at)tablecmds(dot)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

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(at)tablecmds(dot)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

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(at)tablecmds(dot)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

On 01/30/2015 07:48 AM, Michael Paquier wrote:
> Looking at the latest patch, it seems that in
> AlterTableGetLockLevel(at)tablecmds(dot)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.

--
Andreas Karlsson

On Sun, Feb 8, 2015 at 10:05 AM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> On 01/30/2015 07:48 AM, Michael Paquier wrote:
>>
>> Looking at the latest patch, it seems that in
>> AlterTableGetLockLevel(at)tablecmds(dot)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

On 7 February 2015 at 20:05, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> On 01/30/2015 07:48 AM, Michael Paquier wrote:
>>
>> Looking at the latest patch, it seems that in
>> AlterTableGetLockLevel(at)tablecmds(dot)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

On Mon, Apr 6, 2015 at 12:56 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On 7 February 2015 at 20:05, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
>> On 01/30/2015 07:48 AM, Michael Paquier wrote:
>>>
>>> Looking at the latest patch, it seems that in
>>> AlterTableGetLockLevel(at)tablecmds(dot)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

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

On 5 April 2015 at 19:19, Michael Paquier <michael(dot)paquier(at)gmail(dot)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