Skip to content

fix(alter): change and rename attributes on PostgreSQL - #1639

Draft
dimitri-yatsenko wants to merge 1 commit into
masterfrom
fix/alter-postgres-modify
Draft

dimitri-yatsenko wants to merge 1 commit into
masterfrom
fix/alter-postgres-modify

Conversation

@dimitri-yatsenko

Copy link
Copy Markdown
Member

On PostgreSQL, Table.alter() could add and drop attributes but not change them. A change of type, nullability or default emitted MySQL's MODIFY, a syntax error. A change of comment alone was ignored: PostgreSQL keeps comments out of the column declaration, and the declarations were what alter() compared.

alter() now compares the parts of each column (type, nullability, default and comment) instead of the compiled declarations, and a new adapter method, alter_table_ddl, turns the differences into statements. MySQL still gets one ALTER TABLE with MODIFY, CHANGE, ADD and DROP, and unit tests pin those statements to what master produced. PostgreSQL gets ALTER COLUMN ... TYPE, SET/DROP NOT NULL and SET/DROP DEFAULT; RENAME COLUMN; and COMMENT ON COLUMN for each new or changed comment, carrying the core-type marker (:float64:) that describe() reads back. A changed table comment becomes COMMENT ON TABLE in place of MySQL's COMMENT=, which was also a syntax error there. The statements run in one transaction, so a failure in any of them leaves the table as it was.

A type change adds a USING expression where PostgreSQL has no assignment conversion: strings and enum labels are parsed as the new type, numbers become booleans by comparison with zero, booleans become 0 or 1, and other values become JSON through to_jsonb. A conversion with no meaning, such as a date to a number, raises a DataJointError before anything runs. A default is dropped before the type changes and set again after it, because PostgreSQL would otherwise try to convert the default as well. A change to an enum creates the new type first, as adding an enum attribute already did.

Renames now work on PostgreSQL as RENAME COLUMN. The rename marker is read from the comment as written (#1634, which this builds on); where #1634 refused a rename on PostgreSQL, this carries it out. The MySQL statements are unchanged.

describe() on PostgreSQL wrote defaults with the cast the server reports, as value='1.5'::real or label="'abc'::character varying". alter() parses describe() for the current definition, so the first made it fail on any table with such a default, and the second reads as a different default. The adapter now reports the value alone, as MySQL does.

New integration tests run on both backends. Each alters a table that holds data and checks that the table then describes itself exactly as a table declared from the new definition does, that alter() finds nothing left to change, and that the values survived. They cover a change of type (including to and from strings, booleans and enums), nullability, default, attribute comment and table comment; several changes in one alter; a rename alone and with other changes; a failed change, which leaves the table unchanged; and a refused conversion.

For reviewers to confirm

  • Conversions refused on PostgreSQL: date or timestamp, uuid, and bytea, to or from numbers, booleans and each other. They raise before anything runs.
  • Booleans and numbers: a boolean becomes 0 or 1, and a number becomes a boolean by comparison with zero.
  • Renames run one after another on PostgreSQL, so swapping two names in one alter() fails with "already exists" and rolls back. Doing it in two alters works.

Stacked on #1634: its base is fix/alter-rename-keeps-data until that merges.

Closes #1625.

On PostgreSQL, `Table.alter()` could add and drop attributes but not change them. A change of type, nullability or default emitted MySQL's `MODIFY`, a syntax error. A change of comment alone was ignored: PostgreSQL keeps comments out of the column declaration, and the declarations were what alter() compared.

alter() now compares the parts of each column (type, nullability, default and comment) instead of the compiled declarations, and a new adapter method, `alter_table_ddl`, turns the differences into statements. MySQL still gets one ALTER TABLE with `MODIFY`, `CHANGE`, `ADD` and `DROP`, and unit tests pin those statements to what master produced. PostgreSQL gets `ALTER COLUMN ... TYPE`, `SET`/`DROP NOT NULL` and `SET`/`DROP DEFAULT`; `RENAME COLUMN`; and `COMMENT ON COLUMN` for each new or changed comment, carrying the core-type marker (`:float64:`) that describe() reads back. A changed table comment becomes `COMMENT ON TABLE` in place of MySQL's `COMMENT=`, which was also a syntax error there. The statements run in one transaction, so a failure in any of them leaves the table as it was.

A type change adds a `USING` expression where PostgreSQL has no assignment conversion: strings and enum labels are parsed as the new type, numbers become booleans by comparison with zero, booleans become 0 or 1, and other values become JSON through `to_jsonb`. A conversion with no meaning, such as a date to a number, raises a DataJointError before anything runs. A default is dropped before the type changes and set again after it, because PostgreSQL would otherwise try to convert the default as well. A change to an enum creates the new type first, as adding an enum attribute already did.

Renames now work on PostgreSQL as `RENAME COLUMN`. The rename marker is read from the comment as written (#1634, which this builds on); where #1634 refused a rename on PostgreSQL, this carries it out. The MySQL statements are unchanged.

describe() on PostgreSQL wrote defaults with the cast the server reports, as `value='1.5'::real` or `label="'abc'::character varying"`. alter() parses describe() for the current definition, so the first made it fail on any table with such a default, and the second reads as a different default. The adapter now reports the value alone, as MySQL does.

New integration tests run on both backends. Each alters a table that holds data and checks that the table then describes itself exactly as a table declared from the new definition does, that alter() finds nothing left to change, and that the values survived. They cover a change of type (including to and from strings, booleans and enums), nullability, default, attribute comment and table comment; several changes in one alter; a rename alone and with other changes; a failed change, which leaves the table unchanged; and a refused conversion.

Closes #1625.
@dimitri-yatsenko dimitri-yatsenko added this to the v2.4 milestone Oct 18, 2026
@dimitri-yatsenko dimitri-yatsenko added the bug Indicates an unexpected problem or unintended behavior label Oct 18, 2026
Base automatically changed from fix/alter-rename-keeps-data to master October 21, 2026 15:51

@MilagrosMarin MilagrosMarin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This reads well. Comparing columns by their parts, and letting the adapter produce the statements, makes the PostgreSQL path straightforward, and pinning the MySQL output to what master produced is reassuring. It needs a rebase now that #1634 has merged (it is CONFLICTING, and the "Stacked on #1634" line is out of date); I'll approve once it is rebased.

On the three points for reviewers:

  1. Refused conversions: agreed. None of date/timestamp, uuid, or bytea to or from numbers, booleans, or each other has an obvious meaning, and passing unrecognized type pairs to PostgreSQL, with the change rolling back on failure, is a sensible default.
  2. Booleans and numbers: <> 0 and ::integer match what MySQL stores for a boolean, so both are fine.
  3. Swapping names in one alter: acceptable, but the user gets PostgreSQL's "already exists". Detecting a swap in _make_attribute_alter and raising a DataJointError that says to rename in two alters would be friendlier, and it would fail before anything runs.

Smaller notes, none blocking:

  • pre_ddl (CREATE TYPE for a changed enum) runs before the transaction, so a failure later in the alter rolls back the columns but leaves the new enum type, unused. Harmless, but worth a comment, or moving it inside the transaction.
  • On PostgreSQL, alter() now opens self.connection.transaction. What happens when the caller is already inside a transaction (an error on nesting, or joining the outer one)? A test or a line in the docstring would settle it.
  • The parse_column_info change only strips the cast from a whole quoted literal (fullmatch), so nextval(...) and other expressions are left alone. That's the right scope.

@dimitri-yatsenko
dimitri-yatsenko marked this pull request as draft October 21, 2026 16:46

This branch has not been deployed

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

Labels

bug Indicates an unexpected problem or unintended behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants