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