Re: ALTER COLUMN ... SET EXPRESSION to alter stored generated column's expression

On Mon, Nov 20, 2023 at 1:12 PM Peter Eisentraut <peter(at)eisentraut(dot)org>
wrote:

> 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.

Regards,
Amul