[Django] #31653: PostgreSQL add constraints via NOT VALID / VALIDATE CONSTRAINT
Django
-------------------------------------------------+------------------------
Reporter: Adam (Chainz) Johnson | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------------------+------------------------
If you read the ALTER TABLE PostgreSQL docs (
https://www.postgresql.org/docs/12/sql-altertable.html ) for `ADD
CONSTRAINT`, you'll see that it supports the `NOT VALID` option. This
prevents the constraint from being checked against all existing data,
although it does affect all inserted/updated rows. Adding a constraint
this way does not exclusively lock the whole table to check it. To promote
it to valid, the `VALIDATE CONSTRAINT` syntax can be used. This allows a
non-exclusive lock on the table to validate all existing rows. This two
step techinque allows constraints to be added to very active tables
without locking out concurrent access and thus inflicting downtime.
I suggest Django always use this form when adding foreign keys and check
constraints.
--
Ticket URL: <https://code.djangoproject.com/ticket/31653>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: | Status: new
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam (Chainz) Johnson):
It doesn't need to be deferred, as I understand it can be done within a
migration transaction. I was imagining `SchemaEditor.add_constraint` can
be overridden to add the `NOT VALID` clause then execute the second
`VALIDATE CONSTRAINT`.
--
Ticket URL: <https://code.djangoproject.com/ticket/31653#comment:2>
Django
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: | Status: new
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
Similar `NOVALIDATE` syntax is available on
[https://docs.oracle.com/en/database/oracle/oracle-
database/18/sqlrf/constraint.html#GUID-1055EA97-BA6F-4764-A15F-
1024FD5B6DFE Oracle]. However on both databases it has some caveats.
--
Ticket URL: <https://code.djangoproject.com/ticket/31653#comment:3>
Django
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: | Status: new
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
The documentation states
> After that, a VALIDATE CONSTRAINT command can be issued to verify that
existing rows satisfy the constraint. The validation step does not need to
lock out concurrent updates, since it knows that other transactions will
be enforcing the constraint for rows that they insert or update; only pre-
existing rows need to be checked.
My interpretation is that in order for other transactions to ''see'' that
the constraint was added in a `READ COMMITTED` scenario the `ADD
CONSTRAINT NOT VALID` must be committed first; performing a `ADD
CONSTRAINT NOT VALID` and `VALIDATE CONSTRAINT` in the same transaction
should result in the same operations as `ADD CONSTRAINT` otherwise
PostgreSQL would always do it by default?
I could be missing something as I didn't try it out locally but from my
understanding this would need the following set of migrations to be
generated to work properly.
{{{#!python
class Migration(migrations.Migration):
operations = [
AddConstraint(Constraint(name='foo'), validate=False),
]
class Migration(migrations.Migration):
operations = [
ValidateConstraint('foo'),
]
}}}
Running the first migration would commit the `ADD CONSTRAINT NOT VALID`
and allow other transactions to start enforcing it which is something the
`VALIDATE CONSTRAINT` could assume in the following transaction.
--
Ticket URL: <https://code.djangoproject.com/ticket/31653#comment:4>
Django
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: | Status: new
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam (Chainz) Johnson):
> My interpretation is that in order for other transactions to see that
the constraint was added in a READ COMMITTED scenario the ADD CONSTRAINT
NOT VALID must be committed first; performing a ADD CONSTRAINT NOT VALID
and VALIDATE CONSTRAINT in the same transaction should result in the same
operations as ADD CONSTRAINT otherwise PostgreSQL would always do it by
default?
Yes... the one time I've used `NOT VALID` was actually in an `atomic =
False` migration so I didn't test it with atomic. I was probably too
zealous reading the PostgreSQL docs which don't mention transactions.
I think custom `django.contrib.postgres` operations would be the way. They
can document to use non-atomic migrations. Rather than `AddConstraint(...,
validate=False)` I'd suggest `AddConstraintNotValid(...)` as a subclass of
`AddConstraint`, and `ValidateConstraint`.
--
Ticket URL: <https://code.djangoproject.com/ticket/31653#comment:5>
Django
CONSTRAINT
-------------------------------------+-------------------------------------
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: | Status: new
Cleanup/optimization |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Adam (Chainz) Johnson):
* component: Migrations => contrib.postgres
Old description:
> If you read the ALTER TABLE PostgreSQL docs (
> https://www.postgresql.org/docs/12/sql-altertable.html ) for `ADD
> CONSTRAINT`, you'll see that it supports the `NOT VALID` option. This
> prevents the constraint from being checked against all existing data,
> although it does affect all inserted/updated rows. Adding a constraint
> this way does not exclusively lock the whole table to check it. To
> promote it to valid, the `VALIDATE CONSTRAINT` syntax can be used. This
> allows a non-exclusive lock on the table to validate all existing rows.
> This two step techinque allows constraints to be added to very active
> tables without locking out concurrent access and thus inflicting
> downtime.
>
> I suggest Django always use this form when adding foreign keys and check
> constraints.
New description:
If you read the ALTER TABLE PostgreSQL docs (
https://www.postgresql.org/docs/12/sql-altertable.html ) for `ADD
CONSTRAINT`, you'll see that it supports the `NOT VALID` option. This
prevents the constraint from being checked against all existing data,
although it does affect all inserted/updated rows. Adding a constraint
this way does not exclusively lock the whole table to check it. To promote
it to valid, the `VALIDATE CONSTRAINT` syntax can be used. This allows a
non-exclusive lock on the table to validate all existing rows. This two
step techinque allows constraints to be added to very active tables
without locking out concurrent access and thus inflicting downtime.
I suggest `django.contrib.postgres` add custom operations for these
commands, with appropriate documentation around using them in non-atomic
migrations.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/31653#comment:6>
Django
CONSTRAINT
-------------------------------------+-------------------------------------
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: | Status: new
Cleanup/optimization |
Component: contrib.postgres | Version: master
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* stage: Unreviewed => Accepted
Comment:
> I think custom django.contrib.postgres operations would be the way. They
can document to use non-atomic migrations. Rather than AddConstraint(...,
validate=False) I'd suggest AddConstraintNotValid(...) as a subclass of
AddConstraint, and ValidateConstraint.
I think that's the way to go as well. I'd suggest we document breaking the
operations in two migrations instead of using `atomic=False` otherwise
your database will be left in a inconsistent state if the
`ValidateConstraint` operation fails.
--
Ticket URL: <https://code.djangoproject.com/ticket/31653#comment:7>
Django
CONSTRAINT
-------------------------------------+-------------------------------------
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: | Status: new
Cleanup/optimization |
Component: contrib.postgres | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam (Chainz) Johnson):
> I'd suggest we document breaking the operations in two migrations
instead of using atomic=False otherwise your database will be left in a
inconsistent state if the ValidateConstraint operation fails.
Yes. I think MySQL has made me reach for `atomic = False` too much 😂
--
Ticket URL: <https://code.djangoproject.com/ticket/31653#comment:8>
Django
CONSTRAINT
-------------------------------------+-------------------------------------
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: | Status: new
Cleanup/optimization |
Component: contrib.postgres | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Sanskar Jaiswal):
I'd like to work on this ticket. I was thinking of adding an
`add_constraint()` in
`db.backends.postgresql.schema.DatabaseSchemaEditor`, which could modify
the sql returned by calling `constraint.create_sql(model, self)` to have
the `NOT VALID` clause as well. This could be then used by the custom
operations, that'll be defined in `contrib.postgres.operations`. Does this
work or is my approach missing something here?
--
Ticket URL: <https://code.djangoproject.com/ticket/31653#comment:9>
Django
CONSTRAINT
-------------------------------------+-------------------------------------
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: | Status: new
Cleanup/optimization |
Component: contrib.postgres | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Adam (Chainz) Johnson):
I think it would be better to keep the SQL modification purely in the
operation class. It's only for PostgreSQL, so it doesn't need to be part
of the more generic backend class.
--
Ticket URL: <https://code.djangoproject.com/ticket/31653#comment:10>
Django
CONSTRAINT
-------------------------------------+-------------------------------------
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: | Status: new
Cleanup/optimization |
Component: contrib.postgres | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Sanskar Jaiswal):
But wouldn't modifying the SQL in a new `add_constraint()` method in
`db.backends.postgresql.schema.DatabaseSchemaEditor` avoid making it
backend agnostic and make it Postgres specific?
--
Ticket URL: <https://code.djangoproject.com/ticket/31653#comment:11>
Django
CONSTRAINT
-------------------------------------+-------------------------------------
Reporter: Adam (Chainz) | Owner: nobody
Johnson |
Type: | Status: new
Cleanup/optimization |
Component: contrib.postgres | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Sanskar Jaiswal):
* has_patch: 0 => 1
Comment:
Patch https://github.com/django/django/pull/13559
--
Ticket URL: <https://code.djangoproject.com/ticket/31653#comment:14>
Django
CONSTRAINT
-------------------------------------+-------------------------------------
Reporter: Adam (Chainz) | Owner: Sanskar
Johnson | Jaiswal
Type: New feature | Status: assigned
Component: contrib.postgres | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: nobody => Sanskar Jaiswal
* needs_better_patch: 0 => 1
* status: new => assigned
* type: Cleanup/optimization => New feature
--
Ticket URL: <https://code.djangoproject.com/ticket/31653#comment:15>
Django
CONSTRAINT
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Sanskar
| Jaiswal
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Sanskar Jaiswal):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31653#comment:16>
Django
CONSTRAINT
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Sanskar
| Jaiswal
Type: New feature | Status: assigned
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/31653#comment:17>
Django
CONSTRAINT
-------------------------------------+-------------------------------------
Reporter: Adam Johnson | Owner: Sanskar
| Jaiswal
Component: contrib.postgres | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"8c3bd0b708b488a1f6e8bd8cc6b96569904605be" 8c3bd0b7]:
{{{
#!CommitTicketReference repository=""
revision="8c3bd0b708b488a1f6e8bd8cc6b96569904605be"
Fixed #31653 -- Added AddConstraintNotValid()/ValidateConstraint()
operations for PostgreSQL.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31653#comment:18>