Repository navigation
fix(alter): change and rename attributes on PostgreSQL - #1639
Draft
dimitri-yatsenko wants to merge 1 commit into
Draft
dimitri-yatsenko wants to merge 1 commit into
dimitri-yatsenko wants to merge 1 commit into
Conversation
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
requested review from
MilagrosMarin,
lum-agilyti and
ttngu207
October 18, 2026 00:40
This was referenced Oct 21, 2026
MilagrosMarin
left a comment
Contributor
There was a problem hiding this comment.
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:
- 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.
- Booleans and numbers:
<> 0and::integermatch what MySQL stores for a boolean, so both are fine. - Swapping names in one alter: acceptable, but the user gets PostgreSQL's "already exists". Detecting a swap in
_make_attribute_alterand 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 TYPEfor 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 opensself.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_infochange only strips the cast from a whole quoted literal (fullmatch), sonextval(...)and other expressions are left alone. That's the right scope.
dimitri-yatsenko
marked this pull request as draft
October 21, 2026 16:46
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On PostgreSQL,
Table.alter()could add and drop attributes but not change them. A change of type, nullability or default emitted MySQL'sMODIFY, 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 withMODIFY,CHANGE,ADDandDROP, and unit tests pin those statements to what master produced. PostgreSQL getsALTER COLUMN ... TYPE,SET/DROP NOT NULLandSET/DROP DEFAULT;RENAME COLUMN; andCOMMENT ON COLUMNfor each new or changed comment, carrying the core-type marker (:float64:) that describe() reads back. A changed table comment becomesCOMMENT ON TABLEin place of MySQL'sCOMMENT=, 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
USINGexpression 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 throughto_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'::realorlabel="'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
alter()fails with "already exists" and rolls back. Doing it in two alters works.Stacked on #1634: its base is
fix/alter-rename-keeps-datauntil that merges.Closes #1625.