On 17.11.23 13:25, Amul Sul wrote: > To fix this we should be doing something like ALTER COLUMN TYPE and the pass > should be AT_PASS_ALTER_TYPE (rename it or invent a new one near to that) so > that in ATRewriteCatalogs(), we would execute ATPostAlterTypeCleanup(). > > I simply tried that by doing blind copy of code from > ATExecAlterColumnType() in > 0002 patch. We don't really need to do all the stuff such as re-adding > indexes, constraints etc, but I am out of time for today to figure out the > optimum code and I will be away from work in the first half of the coming > week and the week after that. Therefore, I thought of sharing an approach to > get comments/thoughts on the direction, thanks.
The exact sequencing of this seems to be tricky. It's clear that we need to do it earlier than at the end. I also think it should be strictly after AT_PASS_ALTER_TYPE so that the new expression can refer to the new type of a column. It should also be after AT_PASS_ADD_COL, so that the new expression can refer to any newly added column. But then it's after AT_PASS_OLD_INDEX and AT_PASS_OLD_CONSTR, is that a problem?
AT_PASS_ALTER_TYPE and AT_PASS_ADD_COL cannot be together, the ALTER TYPE cannot see that column, I think we can adopt the same behaviour.
But, we need to have ALTER SET EXPRESSION after the ALTER TYPE since if we add the new generated expression for the current type (e.g. int) and we would alter the type (e.g. text or numeric) then that will be problematic in the ATRewriteTable() where a new generation expression will generate value for the old type but the actual type is something else. Therefore I have added
AT_PASS_SET_EXPRESSION to execute after AT_PASS_ALTER_TYPE.
(It might be an option for the first version of this feature to not support altering columns that have constraints on them. But we do need to support columns with indexes on them. Does that work ok? Does that depend on the relative order of AT_PASS_OLD_INDEX?)
I tried to reuse the code by borrowing code from ALTER TYPE, see if that looks good to you.
But I have concerns, with that code reuse where we drop and re-add the indexes and constraints which seems unnecessary for SET EXPRESSION where column attributes will stay the same. I don't know why ATLER TYPE does that for index since finish_heap_swap() anyway does reindexing. We could skip re-adding index for SET EXPRESSION which would be fine but we could not skip the re-addition of constraints, since rebuilding constraints for checking might need a good amount of code copy especially for foreign key constraints.
Please have a look at the attached version, 0001 patch does the code refactoring, and 0002 is the implementation, using the newly refactored code to re-add indexes and constraints for the validation. Added tests for the same.