Re: ALTER COLUMN ... SET EXPRESSION to alter stored generated column's expression - Mailing list pgsql-hackers

From Peter Eisentraut
Subject Re: ALTER COLUMN ... SET EXPRESSION to alter stored generated column's expression
Date
Msg-id dff9d4bc-2c0a-48bf-9e3b-f43d0a69fd22@eisentraut.org
Whole thread Raw
In response to Re: ALTER COLUMN ... SET EXPRESSION to alter stored generated column's expression  (Amul Sul <sulamul@gmail.com>)
Responses Re: ALTER COLUMN ... SET EXPRESSION to alter stored generated column's expression
List pgsql-hackers
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.)
Attachment

pgsql-hackers by date:

Previous
From: "Drouvot, Bertrand"
Date:
Subject: Re: Synchronizing slots from primary to standby
Next
From: Thomas Munro
Date:
Subject: Re: Streaming I/O, vectored I/O (WIP)