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

On Tue, Nov 28, 2023 at 5:40 PM Peter Eisentraut <peter(at)eisentraut(dot)org>
wrote:

> On 23.11.23 15:13, Amul Sul wrote:
> > 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 samebehaviour.
>
> With your v5 patch, I see the following behavior:
>
> create table t1 (a int, b int generated always as (a + 1) stored);
> alter table t1 add column c int, alter column b set expression as (a + c);
> ERROR: 42703: column "c" does not exist
>
> I think intuitively, this ought to work. Maybe just moving the new pass
> after AT_PASS_ADD_COL would do it.
>
>
I think we can't support that (like alter type) since we need to place this
new
pass before AT_PASS_OLD_INDEX & AT_PASS_OLD_CONSTR to re-add indexes and
constraints for the validation.

While looking at this, I figured that converting the AT_PASS_* macros to
> an enum would make this code simpler and easier to follow. For patches
> like yours you wouldn't have to renumber the whole list. We could put
> this patch before yours if people agree with it.
>

Ok, 0001 patch does that.

>
> > 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.
>
> This looks reasonable after a first reading. (I have found that using
> git diff --patience makes the 0001 patch look more readable. Maybe
> that's helpful.)

Yeah, the attached version is generated with a better git-diff algorithm for
the readability.

Regards,
Amul