Reducing lock strength of adding foreign keys

Andreas Karlsson <andreas@proxel.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

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

On 10/22/2014 04:13 PM, Tom Lane wrote:

Andreas Karlsson <andreas@proxel.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

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

On Wed, Oct 22, 2014 at 3:06 AM, Andreas Karlsson <andreas@proxel.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:

/messages/by-id/CA+TgmoY4GLsXZk0tAO29-LJtcuj0SL1xWCwQ51xb-HFYsgi5RQ@mail.gmail.com
/messages/by-id/20893.1393892127@sss.pgh.pa.us
/messages/by-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

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

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.

/messages/by-id/CA+TgmoY4GLsXZk0tAO29-LJtcuj0SL1xWCwQ51xb-HFYsgi5RQ@mail.gmail.com
/messages/by-id/20893.1393892127@sss.pgh.pa.us
/messages/by-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

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

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

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

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.

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

Thanks for weighing in, Noah.

On Sat, Oct 25, 2014 at 2:00 PM, Noah Misch <noah@leadboat.com> wrote:

/messages/by-id/CA+TgmoY4GLsXZk0tAO29-LJtcuj0SL1xWCwQ51xb-HFYsgi5RQ@mail.gmail.com
/messages/by-id/20893.1393892127@sss.pgh.pa.us
/messages/by-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

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

On Sun, Oct 26, 2014 at 9:48 PM, Andreas Karlsson <andreas@proxel.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

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

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@leadboat.com> wrote:

/messages/by-id/CA+TgmoY4GLsXZk0tAO29-LJtcuj0SL1xWCwQ51xb-HFYsgi5RQ@mail.gmail.com
/messages/by-id/20893.1393892127@sss.pgh.pa.us
/messages/by-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.

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

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

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

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

On 25 October 2014 19:00, Noah Misch <noah@leadboat.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

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

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.

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

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

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

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

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@proxel.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

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