Repository navigation
fix(alter): keep an attribute's values when alter() renames it - #1634
Conversation
`alter()` renames an attribute whose new comment opens with `{old_name}`. It looked for that marker in the compiled column SQL, where it is not always found. A core type or codec writes its `:type:` marker in front of the comment (`COMMENT ":float32:{value} ..."`), and PostgreSQL keeps comments out of the column SQL altogether. In both cases the rename went unnoticed: `alter()` dropped the old attribute and added the new one, and every value was lost. On MySQL, renaming `value : float32` to `reading : float32 # {value}` left `reading` at 0.0 in every row; on PostgreSQL, renaming a nullable attribute left it NULL. A required attribute on PostgreSQL failed with a NOT NULL error instead.
The marker is now read from the comment as declared, after any `:type:` marker. On MySQL the rename becomes a CHANGE, which keeps the values. PostgreSQL has no CHANGE, so `alter()` now refuses a rename there with a DataJointError before running anything, instead of dropping the attribute.
The new tests rename a required and a nullable core-type attribute of a table holding rows. On MySQL they check that the values survive; on PostgreSQL that the rename is refused and the table keeps its values. All four fail before this change.
|
@MilagrosMarin @lum-agilyti: this, with its backport (#1635), is the last fix holding the v2.3.5 tag. It fixes data loss in 2.3.4, and everything else in v2.3.5 is merged. Could one of you review it? The backport is the same commit. |
MilagrosMarin
left a comment
There was a problem hiding this comment.
LGTM. Reading the rename marker from the declared comment, rather than the column SQL, fixes both cases: the :type: prefix on MySQL and the absent comment on PostgreSQL. Refusing the rename on PostgreSQL before any SQL runs is the right stopgap until #1625. The tests check that values survive on MySQL and that the table is untouched on PostgreSQL.
Two small, non-blocking notes:
- The description mentions codecs as well as core types, but the tests only rename a
float32. A case with a codec attribute (e.g.<blob>) would confirm the(?::[^:]*:)?prefix is skipped there too. - The comment above the PostgreSQL
raisedescribes the old behavior ("Without this check the rename went unnoticed…"). Something like "PostgreSQL has no CHANGE; refuse rather than DROP + ADD" would state the constraint itself.
|
Thanks, @MilagrosMarin. Merging, then #1635. Both notes go into #1639, which rewrites this code for 2.4: a rename test with a codec attribute such as |
alter()renames an attribute whose new comment opens with{old_name}. It looked for that marker in the compiled column SQL, where it is not always found. A core type or codec writes its:type:marker in front of the comment (COMMENT ":float32:{value} ..."), and PostgreSQL keeps comments out of the column SQL altogether. In both cases the rename went unnoticed:alter()dropped the old attribute and added the new one, and every value was lost. On MySQL, renamingvalue : float32toreading : float32 # {value}leftreadingat 0.0 in every row; on PostgreSQL, renaming a nullable attribute left it NULL. A required attribute on PostgreSQL failed with a NOT NULL error instead.The marker is now read from the comment as declared, after any
:type:marker. On MySQL the rename becomes a CHANGE, which keeps the values. PostgreSQL has no CHANGE, soalter()now refuses a rename there with a DataJointError before running anything, instead of dropping the attribute.The new tests rename a required and a nullable core-type attribute of a table holding rows. On MySQL they check that the values survive; on PostgreSQL that the rename is refused and the table keeps its values. All four fail before this change.
Renaming on PostgreSQL itself comes with #1625 in 2.4.
Closes #1631.