New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to create/validate invalid foreign keys in Postgres #27756

Merged
merged 1 commit into from Dec 1, 2017

Conversation

Projects
None yet
8 participants
@travisofthenorth
Contributor

travisofthenorth commented Jan 20, 2017

Summary

In Postgres, adding foreign keys can cause significant downtime because the transaction needs to acquire some very heavy locks on the table being altered as well as the table being referenced. To illustrate, if I wanted to add a foreign key on my addresses table referencing my users table:

myapp=# BEGIN;
BEGIN
myapp=# ALTER TABLE addresses ADD CONSTRAINT "fk_rails_48c9e0c5a2" FOREIGN KEY ("user_id") REFERENCES "users" ("id");
ALTER TABLE
myapp=# SELECT locktype, relation::regclass, mode, transactionid AS tid, virtualtransaction AS vtid, pid, granted FROM pg_locks;
   locktype    |          relation          |        mode         |   tid   |  vtid   |  pid  | granted
---------------+----------------------------+---------------------+---------+---------+-------+---------
 relation      | pg_locks                   | AccessShareLock     |         | 2/80438 | 56984 | t
 relation      | users_pkey                 | AccessShareLock     |         | 2/80438 | 56984 | t
 relation      | index_addresses_on_user_id | AccessShareLock     |         | 2/80438 | 56984 | t
 relation      | addresses_pkey             | AccessShareLock     |         | 2/80438 | 56984 | t
 virtualxid    |                            | ExclusiveLock       |         | 2/80438 | 56984 | t
 relation      | addresses                  | AccessShareLock     |         | 2/80438 | 56984 | t
 relation      | addresses                  | AccessExclusiveLock |         | 2/80438 | 56984 | t
 transactionid |                            | ExclusiveLock       | 3919702 | 2/80438 | 56984 | t
 relation      | users                      | AccessShareLock     |         | 2/80438 | 56984 | t
 relation      | users                      | RowShareLock        |         | 2/80438 | 56984 | t
 relation      | users                      | AccessExclusiveLock |         | 2/80438 | 56984 | t
(11 rows)

...my transaction acquires an AccessExclusiveLock on users which is extremely detrimental on a high-traffic table, esp. when Postgres performs a potentially lengthy query to validate the check.

On the other hand, I can take a two-step approach which significantly reduces this burden; by introducing an invalid constraint in one transaction and validating it in another, the locks acquired are much less restrictive:

myapp=# BEGIN;
BEGIN
myapp=# ALTER TABLE addresses ADD CONSTRAINT "fk_rails_48c9e0c5a2" FOREIGN KEY ("user_id") REFERENCES "users" ("id") NOT VALID;
ALTER TABLE
myapp=# COMMIT;
COMMIT
myapp=# BEGIN;
BEGIN
myapp=# ALTER TABLE addresses VALIDATE CONSTRAINT "fk_rails_48c9e0c5a2";
ALTER TABLE
myapp=# SELECT locktype, relation::regclass, mode, transactionid AS tid, virtualtransaction AS vtid, pid, granted FROM pg_locks;
   locktype    |          relation          |           mode           |   tid   |  vtid   |  pid  | granted
---------------+----------------------------+--------------------------+---------+---------+-------+---------
 relation      | users_pkey                 | AccessShareLock          |         | 2/80443 | 56984 | t
 relation      | index_addresses_on_user_id | AccessShareLock          |         | 2/80443 | 56984 | t
 relation      | addresses_pkey             | AccessShareLock          |         | 2/80443 | 56984 | t
 relation      | addresses                  | AccessShareLock          |         | 2/80443 | 56984 | t
 relation      | users                      | AccessShareLock          |         | 2/80443 | 56984 | t
 relation      | users                      | RowShareLock             |         | 2/80443 | 56984 | t
 relation      | pg_locks                   | AccessShareLock          |         | 2/80443 | 56984 | t
 virtualxid    |                            | ExclusiveLock            |         | 2/80443 | 56984 | t
 relation      | addresses                  | ShareUpdateExclusiveLock |         | 2/80443 | 56984 | t
 transactionid |                            | ExclusiveLock            | 3919706 | 2/80443 | 56984 | t
(10 rows)

The first transaction acquires the same AccessExclusiveLock on the users table, but "the potentially-lengthy initial check to verify that all rows in the table satisfy the constraint is skipped" (source: postgres docs). Subsequently, the validation step does not block reads or writes on the users table. 馃挴

So, this PR introduces two things:

  • The ability to create invalid foreign keys by specifying the option valid: false
  • A validate_foreign_key method (which takes the same variety of params as the other foreign key methods) to validate a foreign key

I've heard rumors about this being on the roadmap for the Postgres team, i.e. skipping the check if the table being altered is empty and marking the constraint valid. In any case, perhaps someday this will be more easily achieved with built-in Postgres, but for now it's an issue.

@travisofthenorth

This comment has been minimized.

Show comment
Hide comment
@travisofthenorth

travisofthenorth Jan 20, 2017

Contributor

Please advise on any places to add test coverage. I was having a bit of difficulty identifying where to add tests for certain things.

Contributor

travisofthenorth commented Jan 20, 2017

Please advise on any places to add test coverage. I was having a bit of difficulty identifying where to add tests for certain things.

@travisofthenorth

This comment has been minimized.

Show comment
Hide comment
@travisofthenorth

travisofthenorth Jan 23, 2017

Contributor

@maclover7 is there a process I should be following for getting some 馃憖 on this PR? I believe I have some spec failures due to the bundler update issue, but nothing related to this code.

Contributor

travisofthenorth commented Jan 23, 2017

@maclover7 is there a process I should be following for getting some 馃憖 on this PR? I believe I have some spec failures due to the bundler update issue, but nothing related to this code.

@maclover7

This comment has been minimized.

Show comment
Hide comment
@maclover7

maclover7 Jan 23, 2017

Member

@travisofthenorth I added the needs feedback label, so hopefully someone will review shortly 馃槵

Member

maclover7 commented Jan 23, 2017

@travisofthenorth I added the needs feedback label, so hopefully someone will review shortly 馃槵

@travisofthenorth

This comment has been minimized.

Show comment
Hide comment
@travisofthenorth

travisofthenorth Feb 6, 2017

Contributor

@schneems @kaspth just pinging random members at this point...sorry about that. Just wondering if I can get any feedback on this?

Contributor

travisofthenorth commented Feb 6, 2017

@schneems @kaspth just pinging random members at this point...sorry about that. Just wondering if I can get any feedback on this?

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth Feb 11, 2017

Member

Sorry don't have the Postgres experience to vet this.

@kamipo you've been doing wonders with Active Record, are you interested in giving this PR a review? 馃槉

Member

kaspth commented Feb 11, 2017

Sorry don't have the Postgres experience to vet this.

@kamipo you've been doing wonders with Active Record, are you interested in giving this PR a review? 馃槉

@kamipo

This comment has been minimized.

Show comment
Hide comment
@kamipo

kamipo Feb 13, 2017

Member

VALIDATE CONSTRAINT works not only foreign key but also CHECK constraint.
And also Oracle have similar feature as NOVALIDATE (novalidate constraint to existing records).

So I prefer the naming here:

  • supports_invalid_foreign_keys? -> supports_validate_constraints?
    • what supports VALIDATE CONSTRAINT means that allowing NOVALIDATE constraint.
  • validate_foreign_key -> validate_constraint
  • not_valid? -> novalidate?
Member

kamipo commented Feb 13, 2017

VALIDATE CONSTRAINT works not only foreign key but also CHECK constraint.
And also Oracle have similar feature as NOVALIDATE (novalidate constraint to existing records).

So I prefer the naming here:

  • supports_invalid_foreign_keys? -> supports_validate_constraints?
    • what supports VALIDATE CONSTRAINT means that allowing NOVALIDATE constraint.
  • validate_foreign_key -> validate_constraint
  • not_valid? -> novalidate?
@travisofthenorth

This comment has been minimized.

Show comment
Hide comment
@travisofthenorth

travisofthenorth Feb 13, 2017

Contributor

@kamipo updated. A couple things to mention:

Contributor

travisofthenorth commented Feb 13, 2017

@kamipo updated. A couple things to mention:

@travisofthenorth

This comment has been minimized.

Show comment
Hide comment
@travisofthenorth

travisofthenorth Feb 22, 2017

Contributor

Ping @kamipo. Any other feedback?

Contributor

travisofthenorth commented Feb 22, 2017

Ping @kamipo. Any other feedback?

Show outdated Hide outdated ...rd/lib/active_record/connection_adapters/postgresql/schema_statements.rb Outdated
Show outdated Hide outdated ...ord/lib/active_record/connection_adapters/abstract/schema_definitions.rb Outdated
@travisofthenorth

This comment has been minimized.

Show comment
Hide comment
@travisofthenorth

travisofthenorth Feb 26, 2017

Contributor

@kamipo feedback addressed. I left valid? in there because it's helpful for querying existing constraints.

Contributor

travisofthenorth commented Feb 26, 2017

@kamipo feedback addressed. I left valid? in there because it's helpful for querying existing constraints.

Show outdated Hide outdated ...ord/lib/active_record/connection_adapters/abstract/schema_definitions.rb Outdated
Show outdated Hide outdated ...rd/lib/active_record/connection_adapters/postgresql/schema_statements.rb Outdated
@travisofthenorth

This comment has been minimized.

Show comment
Hide comment
@travisofthenorth

travisofthenorth Feb 26, 2017

Contributor

@kamipo feedback addressed. I'm a bit hesitant about the usage of validate? for existing FKs but I will defer to you.

Contributor

travisofthenorth commented Feb 26, 2017

@kamipo feedback addressed. I'm a bit hesitant about the usage of validate? for existing FKs but I will defer to you.

@travisofthenorth

This comment has been minimized.

Show comment
Hide comment
@travisofthenorth

travisofthenorth Feb 27, 2017

Contributor

Added validated? as an alias. @kamipo let me know if this looks good now.

Contributor

travisofthenorth commented Feb 27, 2017

Added validated? as an alias. @kamipo let me know if this looks good now.

@kamipo

This comment has been minimized.

Show comment
Hide comment
@kamipo

kamipo Feb 28, 2017

Member

Looks good to me 馃憤

Member

kamipo commented Feb 28, 2017

Looks good to me 馃憤

@travisofthenorth

This comment has been minimized.

Show comment
Hide comment
@travisofthenorth

travisofthenorth Feb 28, 2017

Contributor

Squashed and rebased. Thanks for the all the feedback @kamipo!

Contributor

travisofthenorth commented Feb 28, 2017

Squashed and rebased. Thanks for the all the feedback @kamipo!

@travisofthenorth

This comment has been minimized.

Show comment
Hide comment
@travisofthenorth

travisofthenorth May 16, 2017

Contributor

@kamipo now that Rails 5.1 is out there, is it easier to get this in? This feature is becoming increasingly important for one of the apps I contribute to.

Contributor

travisofthenorth commented May 16, 2017

@kamipo now that Rails 5.1 is out there, is it easier to get this in? This feature is becoming increasingly important for one of the apps I contribute to.

@kamipo

This comment has been minimized.

Show comment
Hide comment
@kamipo

kamipo May 16, 2017

Member

I can't merge PRs, so you'll have to wait for someone else from the Committers or Core teams to review this PR.

Member

kamipo commented May 16, 2017

I can't merge PRs, so you'll have to wait for someone else from the Committers or Core teams to review this PR.

@travisofthenorth

This comment has been minimized.

Show comment
Hide comment
@travisofthenorth

travisofthenorth May 16, 2017

Contributor

K, thanks for letting me know. Sorry for the noise!

Contributor

travisofthenorth commented May 16, 2017

K, thanks for letting me know. Sorry for the noise!

@travisofthenorth

This comment has been minimized.

Show comment
Hide comment
@travisofthenorth

travisofthenorth Jul 7, 2017

Contributor

Rebased on master and resolved a conflict.

Contributor

travisofthenorth commented Jul 7, 2017

Rebased on master and resolved a conflict.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Jul 8, 2017

Member

I'm a bit worried about how much stuff we're adding to the abstract layer, for a feature that only Postgres supports. Is any of that avoidable? 馃槙

Member

matthewd commented Jul 8, 2017

I'm a bit worried about how much stuff we're adding to the abstract layer, for a feature that only Postgres supports. Is any of that avoidable? 馃槙

@schneems

This comment has been minimized.

Show comment
Hide comment
@schneems

schneems Jul 8, 2017

Member

I would like to see Rails promote more best practices in regards to database use. Foreign key and other constraints are a huge part of this. On one hand it is catering to a specific database. On the other hand, should users of that DB get a worse experience just because ALL databases don't support that feature?

I know it's not covered in this PR but validation race conditions are a huge issue. Recently I had a validation cause over 80% of ALL load on my postgres database, and I had no idea. Here's the PR explaining the issue and fixing it codetriage/codetriage#573.

It would like to see Rails continue to play better with postgres in the future.

@matthewd RE: size. Half of the PR is tests. I agree that we want to keep the abstract layer to not get too bloated. Any alternative implementation ideas?

Member

schneems commented Jul 8, 2017

I would like to see Rails promote more best practices in regards to database use. Foreign key and other constraints are a huge part of this. On one hand it is catering to a specific database. On the other hand, should users of that DB get a worse experience just because ALL databases don't support that feature?

I know it's not covered in this PR but validation race conditions are a huge issue. Recently I had a validation cause over 80% of ALL load on my postgres database, and I had no idea. Here's the PR explaining the issue and fixing it codetriage/codetriage#573.

It would like to see Rails continue to play better with postgres in the future.

@matthewd RE: size. Half of the PR is tests. I agree that we want to keep the abstract layer to not get too bloated. Any alternative implementation ideas?

@travisofthenorth

This comment has been minimized.

Show comment
Hide comment
@travisofthenorth

travisofthenorth Jul 10, 2017

Contributor

@matthewd it seemed unavoidable but it was my first time digging into the adapter code so I could be wrong. I'm happy to make changes so let me know if you have any ideas.

Contributor

travisofthenorth commented Jul 10, 2017

@matthewd it seemed unavoidable but it was my first time digging into the adapter code so I could be wrong. I'm happy to make changes so let me know if you have any ideas.

@travisofthenorth

This comment has been minimized.

Show comment
Hide comment
@travisofthenorth

travisofthenorth Jul 10, 2017

Contributor

@matthewd I took another look at it. The latest commit moves most of the implementation into postgres-specific classes. I left some code in the ForeignKeyDefinition struct in abstract because it felt weird re-implementing that. Let me know how it looks now.

Contributor

travisofthenorth commented Jul 10, 2017

@matthewd I took another look at it. The latest commit moves most of the implementation into postgres-specific classes. I left some code in the ForeignKeyDefinition struct in abstract because it felt weird re-implementing that. Let me know how it looks now.

@travisofthenorth

This comment has been minimized.

Show comment
Hide comment
@travisofthenorth

travisofthenorth Sep 3, 2017

Contributor

Rebased on master and squashed to 1 commit. @matthewd please advise if you want to see any more changes in this PR, otherwise I think it should be good to go.

Contributor

travisofthenorth commented Sep 3, 2017

Rebased on master and squashed to 1 commit. @matthewd please advise if you want to see any more changes in this PR, otherwise I think it should be good to go.

Add support for invalid foreign keys in Postgres
Add validate_constraint and update naming
@travisofthenorth

This comment has been minimized.

Show comment
Hide comment
@travisofthenorth

travisofthenorth Dec 1, 2017

Contributor

@kamipo @matthewd just rebased on master. Any chance this could make it into 5.2?

Contributor

travisofthenorth commented Dec 1, 2017

@kamipo @matthewd just rebased on master. Any chance this could make it into 5.2?

@kamipo

kamipo approved these changes Dec 1, 2017

@matthewd matthewd merged commit 9f33a8f into rails:master Dec 1, 2017

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Dec 1, 2017

Member

Sorry I didn't come back to this sooner, especially after you did a great job of addressing my concern about the split between abstract vs postgres adapters. 馃憤馃徎

Member

matthewd commented Dec 1, 2017

Sorry I didn't come back to this sooner, especially after you did a great job of addressing my concern about the split between abstract vs postgres adapters. 馃憤馃徎

@travisofthenorth travisofthenorth deleted the travisofthenorth:validate-foreign-keys branch Dec 1, 2017

#
# Validates the constraint named +constraint_name+ on +accounts+.
#
# validate_foreign_key :accounts, :constraint_name

This comment has been minimized.

@thizzle

thizzle Mar 7, 2018

This comment doesn't match the method it's documenting.

@thizzle

thizzle Mar 7, 2018

This comment doesn't match the method it's documenting.

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Mar 8, 2018

Contributor

It was fixed by 70c96b4

@bogdanvlviv

bogdanvlviv Mar 8, 2018

Contributor

It was fixed by 70c96b4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment