Skip to content

[FIX] lift_constraints: drop constraints individually, not in one statement - #462

Draft
quoc-pn wants to merge 1 commit into
OCA:masterfrom
komit-consulting:fix-lift_constraints-pg17-ordering
Draft

[FIX] lift_constraints: drop constraints individually, not in one statement#462
quoc-pn wants to merge 1 commit into
OCA:masterfrom
komit-consulting:fix-lift_constraints-pg17-ordering

Conversation

@quoc-pn

@quoc-pn quoc-pn commented Aug 5, 2026

Copy link
Copy Markdown
Member

Bug

lift_constraints() drops all constraints found on a column in a single ALTER TABLE ... DROP CONSTRAINT a, DROP CONSTRAINT b, ... statement.

On PostgreSQL 17+, NOT NULL is catalogued as a named constraint (<table>_<column>_not_null) instead of an implicit pg_attribute flag. PostgreSQL validates each DROP CONSTRAINT subcommand against the catalog state as it existed before the statement started, not against the pending effects of earlier subcommands within the same statement.

So when a column has both a not-null constraint and a primary key, and the not-null constraint happens to be listed before the primary key in the generated statement, PostgreSQL rejects the whole statement:

ERROR: column "id" is in a primary key

even though the very next clause in that same statement would have dropped the primary key.

This surfaced concretely in an OCA/OpenUpgrade migration script (hr_recruitment/19.0.1.1/pre-migration.py) calling lift_constraints(cr, legacy_table, "id", cascade=True) on a table whose id column has both a not-null constraint and a primary key, running against PostgreSQL 18.

Fix

Drop each constraint in its own separate ALTER TABLE ... DROP CONSTRAINT IF EXISTS ... statement instead of combining them into one multi-clause statement. This removes any cross-clause catalog ordering dependency, regardless of how many constraints are found or what order they come back in.

Test

The existing test_lift_constraints test (tests/test_openupgradelib.py) already exercises cascade=True on res_partner.id, which has both a primary key and (on PostgreSQL 17+) a not-null constraint — this test reproduces the failure before the fix and passes after it.

@legalsylvain
legalsylvain marked this pull request as draft August 5, 2026 09:33
@quoc-pn
quoc-pn marked this pull request as ready for review August 5, 2026 10:59
@quoc-pn
quoc-pn force-pushed the fix-lift_constraints-pg17-ordering branch from 28ec79d to 532b340 Compare August 5, 2026 11:11
@legalsylvain
legalsylvain marked this pull request as draft August 5, 2026 12:00

@hbrunn hbrunn left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the huge slab of generated bullshit text is just that. the only actual fix here is getting the order right, which should be better done by generating the correct order via pg_depend

…tement

On PostgreSQL 17+, NOT NULL is catalogued as a named constraint
(<table>_<column>_not_null) instead of an implicit pg_attribute flag.

lift_constraints() dropped all constraints on a column in a single
ALTER TABLE statement (`DROP CONSTRAINT a, DROP CONSTRAINT b, ...`).
PostgreSQL validates each DROP CONSTRAINT subcommand against the
catalog state as it existed before the statement started, not against
the pending effects of earlier subcommands in the same statement. So
when a column has both a not-null constraint and a primary key, and
the not-null constraint is listed before the primary key, PostgreSQL
rejects the statement with:

    ERROR: column "id" is in a primary key

even though the very next clause in the same statement would have
dropped that primary key.

Fix by dropping each constraint in its own ALTER TABLE statement
instead of combining them, so there is no cross-clause catalog
dependency for PostgreSQL to complain about.

This alone is not enough: even as separate statements, dropping the
not-null constraint before the primary key still fails with the same
error, since the primary key hasn't been dropped yet at that point.
Order the constraints so the primary key (if any) is dropped first;
dropping it with CASCADE takes the dependent not-null constraint (and
anything else referencing it) down with it, so the remaining
DROP CONSTRAINT IF EXISTS calls become no-ops.
@quoc-pn
quoc-pn force-pushed the fix-lift_constraints-pg17-ordering branch from 532b340 to 19ff019 Compare August 18, 2026 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants