Обсуждение: ALTER COLUMN ... SET EXPRESSION to alter stored generated column's expression

On Wed, Aug 2, 2023 at 6:36 PM Amul Sul <sulamul@gmail.com> wrote:
>
> Hi,
>
> Currently, we have an option to drop the expression of stored generated columns
> as:
>
> ALTER [ COLUMN ] column_name DROP EXPRESSION [ IF EXISTS ]
>
> But don't have support to update that expression. The attached patch provides
> that as:
>
> ALTER [ COLUMN ] column_name SET EXPRESSION expression
>
> Note that this form of ALTER is meant to work for the column which is already
> generated. It then changes the generation expression in the catalog and rewrite
> the table, using the existing table rewrite facilities for ALTER TABLE.
> Otherwise, an error will be reported.
>
> To keep the code flow simple, I have renamed the existing function that was in
> use for DROP EXPRESSION so that it can be used for SET EXPRESSION as well,
> which is a similar design as SET/DROP DEFAULT. I kept this renaming code
> changes in a separate patch to minimize the diff in the main patch.
>
> Demo:
> -- Create table
> CREATE TABLE t1 (x int, y int GENERATED ALWAYS AS (x * 2) STORED);
> INSERT INTO t1 VALUES(generate_series(1,3));
>
> -- Check the generated data
> SELECT * FROM t1;
>  x | y
> ---+---
>  1 | 2
>  2 | 4
>  3 | 6
> (3 rows)
>
> -- Alter the expression
> ALTER TABLE t1 ALTER COLUMN y SET EXPRESSION (x * 4);
>
> -- Check the new data
> SELECT * FROM t1;
>  x | y
> ---+----
>  1 |  4
>  2 |  8
>  3 | 12
> (3 rows)
>
> Thank you.
> --
> Regards,
> Amul Sul
> EDB: http://www.enterprisedb.com
-------------------------
setup.

BEGIN;
set search_path = test;
DROP TABLE if exists gtest_parent, gtest_child;

CREATE TABLE gtest_parent (f1 date NOT NULL, f2 bigint, f3 bigint
GENERATED ALWAYS AS (f2 * 2) STORED) PARTITION BY RANGE (f1);

CREATE TABLE gtest_child PARTITION OF gtest_parent
  FOR VALUES FROM ('2016-07-01') TO ('2016-08-01');  -- inherits gen expr

CREATE TABLE gtest_child2 PARTITION OF gtest_parent (
    f3 WITH OPTIONS GENERATED ALWAYS AS (f2 * 22) STORED  -- overrides gen expr
) FOR VALUES FROM ('2016-08-01') TO ('2016-09-01');

CREATE TABLE gtest_child3 (f1 date NOT NULL, f2 bigint, f3 bigint
GENERATED ALWAYS AS (f2 * 33) STORED);
ALTER TABLE gtest_parent ATTACH PARTITION gtest_child3 FOR VALUES FROM
('2016-09-01') TO ('2016-10-01');

INSERT INTO gtest_parent (f1, f2) VALUES ('2016-07-15', 1);
INSERT INTO gtest_parent (f1, f2) VALUES ('2016-07-15', 2);
INSERT INTO gtest_parent (f1, f2) VALUES ('2016-08-15', 3);
UPDATE gtest_parent SET f1 = f1 + 60 WHERE f2 = 1;

ALTER TABLE ONLY gtest_parent ALTER COLUMN f3 SET EXPRESSION (f2 * 4);
ALTER TABLE gtest_child ALTER COLUMN f3 SET EXPRESSION (f2 * 10);
COMMIT;

set search_path = test;
SELECT table_name, column_name, is_generated, generation_expression
FROM information_schema.columns
WHERE table_name  in ('gtest_child','gtest_child1',
'gtest_child2','gtest_child3')
order by 1,2;
result:
  table_name  | column_name | is_generated | generation_expression
--------------+-------------+--------------+-----------------------
 gtest_child  | f1          | NEVER        |
 gtest_child  | f1          | NEVER        |
 gtest_child  | f2          | NEVER        |
 gtest_child  | f2          | NEVER        |
 gtest_child  | f3          | ALWAYS       | (f2 * 2)
 gtest_child  | f3          | ALWAYS       | (f2 * 10)
 gtest_child2 | f1          | NEVER        |
 gtest_child2 | f1          | NEVER        |
 gtest_child2 | f2          | NEVER        |
 gtest_child2 | f2          | NEVER        |
 gtest_child2 | f3          | ALWAYS       | (f2 * 22)
 gtest_child2 | f3          | ALWAYS       | (f2 * 2)
 gtest_child3 | f1          | NEVER        |
 gtest_child3 | f1          | NEVER        |
 gtest_child3 | f2          | NEVER        |
 gtest_child3 | f2          | NEVER        |
 gtest_child3 | f3          | ALWAYS       | (f2 * 2)
 gtest_child3 | f3          | ALWAYS       | (f2 * 33)
(18 rows)

one partition, one column 2 generated expression. Is this the expected
behavior?

In the regress, you can replace  \d table_name to sql query (similar
to above) to get the generated expression meta data.
since here you want the meta data to be correct. then one select query
to valid generated expression behaviored sane or not.

On Thu, Aug 3, 2023 at 1:23 PM Amul Sul <sulamul@gmail.com> wrote:
>
>
> That is not expected & acceptable. But, somehow, I am not able to reproduce
> this behavior. Could you please retry this experiment by adding "table_schema"
> in your output query?
>
> Thank you.
>
> Regards,
> Amul
>

sorry. my mistake.
I created these partitions in a public schema and test schema. I
ignored table_schema when querying it.
Now, this patch works as expected.

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            tested, passed

Hi Amul,
Patch changes look fine.
Below are some of my observations as soon as we alter default expression on column

1. Materialized view becomes stale and starts giving incorrect results. We need to refresh the materialized view to get
correctresults.
 
2. Index on generated column need to be reindexed in order to use new expression.
3. Column Stats become stale and plan may be impacted due to outdated stats.

These things also happen as soon as we delete default expression or set default expression on column.

Thanks & Best Regards,
Ajit

On 8/2/23 12:35, Amul Sul wrote:
> Hi,
> 
> Currently, we have an option to drop the expression of stored generated
> columns
> as:
> 
> ALTER [ COLUMN ] column_name DROP EXPRESSION [ IF EXISTS ]
> 
> But don't have support to update that expression. The attached patch
> provides
> that as:
> 
> ALTER [ COLUMN ] column_name SET EXPRESSION expression

I am surprised this is not in the standard already.  I will go work on that.
-- 
Vik Fearing

On 8/2/23 12:35, Amul Sul wrote:
> Hi,
> 
> Currently, we have an option to drop the expression of stored generated
> columns
> as:
> 
> ALTER [ COLUMN ] column_name DROP EXPRESSION [ IF EXISTS ]
> 
> But don't have support to update that expression. The attached patch
> provides
> that as:
> 
> ALTER [ COLUMN ] column_name SET EXPRESSION expression

I love this idea.  It is something that the standard SQL language is 
lacking and I am submitting a paper to correct that based on this.  I 
will know in October what the committee thinks of it.  Thanks!

> Note that this form of ALTER is meant to work for the column which is
> already generated.

Why?  SQL does not have a way to convert a non-generated column into a 
generated column, and this seems like as good a way as any.

> To keep the code flow simple, I have renamed the existing function that was
> in use for DROP EXPRESSION so that it can be used for SET EXPRESSION as well,
> which is a similar design as SET/DROP DEFAULT. I kept this renaming code
> changes in a separate patch to minimize the diff in the main patch.

I don't like this part of the patch at all.  Not only is the 
documentation only half baked, but the entire concept of the two 
commands is different.  Especially since I believe the command should 
also create a generated column from a non-generated one.


Is is possible to compare the old and new expressions and no-op if they 
are the same?


psql (17devel)
Type "help" for help.

postgres=# create table t (c integer generated always as (null) stored);
CREATE TABLE
postgres=# select relfilenode from pg_class where oid = 't'::regclass;
  relfilenode
-------------
        16384
(1 row)

postgres=# alter table t alter column c set expression (null);
ALTER TABLE
postgres=# select relfilenode from pg_class where oid = 't'::regclass;
  relfilenode
-------------
        16393
(1 row)


I am not saying we should make every useless case avoid rewriting the 
table, but if there are simple wins, we should take them.  (I don't know 
how feasible this is.)

I think repeating the STORED keyword should be required here to 
future-proof virtual generated columns.

Consider this hypothetical example:

CREATE TABLE t (c INTEGER);
ALTER TABLE t ALTER COLUMN c SET EXPRESSION (42) STORED;
ALTER TABLE t ALTER COLUMN c SET EXPRESSION VIRTUAL;

If we don't require the STORED keyword on the second command, it becomes 
ambiguous.  If we then decide that VIRTUAL should be the default, we 
will break people's scripts.
-- 
Vik Fearing

Hi Amul,
I share others opinion that this feature is useful.

>> On Fri, 25 Aug 2023 at 03:06, Vik Fearing <vik@postgresfriends.org> wrote:
>>>
>>>
>>> I don't like this part of the patch at all.  Not only is the
>>> documentation only half baked, but the entire concept of the two
>>> commands is different.  Especially since I believe the command should
>>> also create a generated column from a non-generated one.
>>
>>
>> But I have to agree with Vik Fearing, we can make this patch better, should we?
>> I totally understand your intentions to keep the code flow simple and reuse existing code as much
>> as possible. But in terms of semantics of these commands, they are quite different from each other.
>> And in terms of reading of the code, this makes it even harder to understand what is going on here.
>> So, in my view, consider split these commands.
>
>
> Ok, probably, I would work in that direction. I did the same thing that
> SET/DROP DEFAULT does, despite semantic differences, and also, if I am not
> missing anything, the code complexity should be the same as that.

If we allow SET EXPRESSION to convert a non-generated column to a
generated one, the current way of handling ONLY would yield mismatch
between parent and child. That's not allowed as per the documentation
[1]. In that sense not allowing SET to change the GENERATED status is
better. I think that can be added as a V2 feature, if it overly
complicates the patch Or at least till a point that becomes part of
SQL standard.

I think V1 patch can focus on changing the expression of a column
which is already a generated column.

Regarding code, I think we should place it where it's reasonable -
following precedence is usually good. But I haven't reviewed the code
to comment on it.

[1] https://www.postgresql.org/docs/16/ddl-generated-columns.html

-- 
Best Wishes,
Ashutosh Bapat

On 28.08.23 11:54, Amul Sul wrote:
> Thanks for the review comments, I have fixed those in the attached 
> version. In
> addition to that, extended syntax to have the STORE keyword as suggested by
> Vik.

An additional comment: When you change the generation expression, you 
need to run ON UPDATE triggers on all rows, if there are any triggers 
defined.  That is because someone could have triggers defined on the 
column to either check for valid values or propagate values somewhere 
else, and if the expression changes, that is kind of like an UPDATE.

Similarly, I think we should consider how logical decoding should handle 
this operation.  I'd imagine it should generate UPDATE events on all 
rows.  A test case in test_decoding would be useful.

On Fri, Oct 6, 2023 at 6:06 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 28.08.23 11:54, Amul Sul wrote:
> > Thanks for the review comments, I have fixed those in the attached
> > version. In
> > addition to that, extended syntax to have the STORE keyword as suggested by
> > Vik.
>
> An additional comment: When you change the generation expression, you
> need to run ON UPDATE triggers on all rows, if there are any triggers
> defined.  That is because someone could have triggers defined on the
> column to either check for valid values or propagate values somewhere
> else, and if the expression changes, that is kind of like an UPDATE.
>
> Similarly, I think we should consider how logical decoding should handle
> this operation.  I'd imagine it should generate UPDATE events on all
> rows.  A test case in test_decoding would be useful.
>

Should we treat it the same fashion as ALTER COLUMN ... TYPE which
rewrites the column values? Of course that rewrites the whole table,
but logically they are comparable.

--
Best Wishes,
Ashutosh Bapat

On 06.10.23 14:57, Ashutosh Bapat wrote:
> On Fri, Oct 6, 2023 at 6:06 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>>
>> On 28.08.23 11:54, Amul Sul wrote:
>>> Thanks for the review comments, I have fixed those in the attached
>>> version. In
>>> addition to that, extended syntax to have the STORE keyword as suggested by
>>> Vik.
>>
>> An additional comment: When you change the generation expression, you
>> need to run ON UPDATE triggers on all rows, if there are any triggers
>> defined.  That is because someone could have triggers defined on the
>> column to either check for valid values or propagate values somewhere
>> else, and if the expression changes, that is kind of like an UPDATE.
>>
>> Similarly, I think we should consider how logical decoding should handle
>> this operation.  I'd imagine it should generate UPDATE events on all
>> rows.  A test case in test_decoding would be useful.
> 
> Should we treat it the same fashion as ALTER COLUMN ... TYPE which
> rewrites the column values? Of course that rewrites the whole table,
> but logically they are comparable.

I don't know.  What are the semantics of that command with respect to 
triggers and logical decoding?

On Fri, Oct 6, 2023 at 9:14 AM Peter Eisentraut <peter@eisentraut.org> wrote:
> > Should we treat it the same fashion as ALTER COLUMN ... TYPE which
> > rewrites the column values? Of course that rewrites the whole table,
> > but logically they are comparable.
>
> I don't know.  What are the semantics of that command with respect to
> triggers and logical decoding?

ALTER COLUMN ... TYPE doesn't fire triggers, and I don't think logical
decoding will do anything with it, either. As Amul also suggested, I
tend to think that this command should behave like that command
instead of inventing some new behavior. Sure, this is kind of like an
UPDATE, but it's also not actually an UPDATE: it's DDL. Consider this
example:

rhaas=# create table foo (a int, b text);
CREATE TABLE
rhaas=# create function nozero () returns trigger as $$begin if (new.b
= '0') then raise 'zero is bad'; end if; return new; end$$ language
plpgsql;
CREATE FUNCTION
rhaas=# create trigger fnz before insert or update or delete on foo
for each row execute function nozero();
CREATE TRIGGER
rhaas=# insert into foo values (1, '0');
ERROR:  zero is bad
CONTEXT:  PL/pgSQL function nozero() line 1 at RAISE
rhaas=# insert into foo values (1, '00');
INSERT 0 1
rhaas=# alter table foo alter column b set data type integer using b::integer;
ALTER TABLE
rhaas=# select * from foo;
 a | b
---+---
 1 | 0
(1 row)

rhaas=# insert into foo values (2, '0');
ERROR:  type of parameter 14 (integer) does not match that when
preparing the plan (text)
CONTEXT:  PL/pgSQL function nozero() line 1 at IF
rhaas=# \c
You are now connected to database "rhaas" as user "rhaas".
rhaas=# insert into foo values (2, '0');
ERROR:  zero is bad
CONTEXT:  PL/pgSQL function nozero() line 1 at RAISE
rhaas=# insert into foo values (2, '00');
ERROR:  zero is bad
CONTEXT:  PL/pgSQL function nozero() line 1 at RAISE

The trigger here is supposed to prevent me from inserting 0 into
column b, but I've ended up with one anyway, because when the column
was of type text, I could insert 00, and when I changed the column to
type integer, the value got smashed down to just 0, and the trigger
wasn't fired to prevent that. You could certainly argue with that
behavior, but I think it's pretty reasonable, and it seems like if
this command behaved that way too, that would also be pretty
reasonable.  In fact, I'm inclined to think it would be preferable,
both for consistency and because it would be less work.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 25.10.23 08:12, Amul Sul wrote:
> Here is the rebase version for the latest master head(673a17e3120).
> 
> I haven't done any other changes related to the ON UPDATE trigger since that
> seems non-trivial; need a bit of work to add trigger support in 
> ATRewriteTable().
> Also, I am not sure yet, if we were doing these changes, and the correct 
> direction
> for that.

I did some detailed testing on Db2, Oracle, and MariaDB (the three 
existing implementations of this feature that I'm tracking), and none of 
them fire any row or statement triggers when the respective statement to 
alter the generation expression is run.  So I'm withdrawing my comment 
that this should fire triggers.  (I suppose event triggers are available 
if anyone really needs the functionality.)

On 09.11.23 13:00, Amul Sul wrote:
> On Tue, Nov 7, 2023 at 8:21 PM Peter Eisentraut <peter@eisentraut.org 
> <mailto:peter@eisentraut.org>> wrote:
> 
>     On 25.10.23 08:12, Amul Sul wrote:
>      > Here is the rebase version for the latest master head(673a17e3120).
>      >
>      > I haven't done any other changes related to the ON UPDATE trigger
>     since that
>      > seems non-trivial; need a bit of work to add trigger support in
>      > ATRewriteTable().
>      > Also, I am not sure yet, if we were doing these changes, and the
>     correct
>      > direction
>      > for that.
> 
>     I did some detailed testing on Db2, Oracle, and MariaDB (the three
>     existing implementations of this feature that I'm tracking), and
>     none of
>     them fire any row or statement triggers when the respective
>     statement to
>     alter the generation expression is run.  So I'm withdrawing my comment
>     that this should fire triggers.  (I suppose event triggers are
>     available
>     if anyone really needs the functionality.)
> 
> 
> Thank you for the confirmation.
> 
> Here is the updated version patch. Did minor changes to documents and tests.

I don't like the renaming in the 0001 patch.  I think it would be better 
to keep the two subcommands (DROP and SET) separate.  There is some 
overlap now, but for example I'm thinking about virtual generated 
columns, then there will be even more conditionals in there.  Let's keep 
it separate for clarity.

Also, it seems to me that the SET EXPRESSION variant should just do an 
update of the catalog table instead of a drop and re-insert.

The documentation needs some improvements:

+    ALTER [ COLUMN ] <replaceable 
class="parameter">column_name</replaceable> SET EXPRESSION <replaceable 
class="parameter">expression</replaceable> STORED

If we're going to follow the Db2 syntax, there should be an "AS" after 
EXPRESSION.  And the implemented syntax requires parentheses, so they 
should appear in the documentation.

Also, the keyword STORED shouldn't be there.  (The same command should 
be applicable to virtual generated columns in the future.)

There should be separate <varlistentry>s for SET and DROP in 
alter_table.sgml.

The functionality looks ok otherwise.

On 13.11.23 14:07, Amul Sul wrote:
>     Also, it seems to me that the SET EXPRESSION variant should just do an
>     update of the catalog table instead of a drop and re-insert.
> 
> I am not sure if that is sufficient; we need to get rid of the 
> dependencies of
> existing expressions on other columns and/or objects that need to be 
> removed.
> The drop and re-insert does that easily.

Ok, good point.

>     The documentation needs some improvements:
> 
>     +    ALTER [ COLUMN ] <replaceable
>     class="parameter">column_name</replaceable> SET EXPRESSION <replaceable
>     class="parameter">expression</replaceable> STORED
> 
>     If we're going to follow the Db2 syntax, there should be an "AS" after
>     EXPRESSION.  And the implemented syntax requires parentheses, so they
>     should appear in the documentation.
> 
>     Also, the keyword STORED shouldn't be there.  (The same command should
>     be applicable to virtual generated columns in the future.)
> 
> I have omitted "AS" intentionally, to keep syntax similar to our existing
> ALTERCOLUMN... SET DEFAULT <a_expr>.  Let me know if you want
> me to add that.

Well, my idea was to follow the Db2 syntax.  Otherwise, we are adding 
yet another slightly different syntax to the world.  Even if we think 
our idea is slightly better, it doesn't seem worth it.

> The STORED suggested by Vik[1].  I think we could skip that if there is 
> no need
> to differentiate between stored and virtual columns at ALTER.

I think that suggestion was based on the idea that this would convert 
non-generated columns to generated columns, but we have dropped that idea.

On 14.11.23 11:40, Amul Sul wrote:
> Please have a look at the attached version, updating the syntax to have "AS"
> after EXPRESSION and other changes suggested previously.

The code structure looks good to me now.

Question: Why are you using AT_PASS_ADD_OTHERCONSTR?  I don't know if 
it's right or wrong, but if you have a specific reason, it would be good 
to know.

I think ATExecSetExpression() needs to lock pg_attribute?  Did you lose 
that during the refactoring?

Tiny comment: The error message in ATExecSetExpression() does not need 
to mention "stored", since it would be also applicable to virtual 
generated columns in the future.

Documentation additions in alter_table.sgml should use one-space indent 
consistently.  Also, "This form replaces expression" is missing a "the"?

On 15.11.23 13:26, Amul Sul wrote:
>     Question: Why are you using AT_PASS_ADD_OTHERCONSTR?  I don't know if
>     it's right or wrong, but if you have a specific reason, it would be
>     good
>     to know.
> 
> I referred to ALTER COLUMN DEFAULT and used that.

Hmm, I'm not sure if that is a good comparison.  For ALTER TABLE, SET 
DEFAULT is just a catalog manipulation, it doesn't change any data, so 
it's pretty easy.  SET EXPRESSION changes data, which other phases might 
want to inspect?  For example, if you do SET EXPRESSION and add a 
constraint in the same ALTER TABLE statement, do those run in the 
correct order?

>     I think ATExecSetExpression() needs to lock pg_attribute?  Did you lose
>     that during the refactoring?
> 
> I have removed that intentionally since we were not updating anything in
> pg_attribute like ALTER DROP EXPRESSION.

ok

>     Tiny comment: The error message in ATExecSetExpression() does not need
>     to mention "stored", since it would be also applicable to virtual
>     generated columns in the future.
> 
> I had to have the same thought, but later decided when we do that
> virtual column thing, we could simply change that. I am fine to do that 
> change
> now as well, let me know your thought.

Not a big deal, but I would change it now.

Another small thing I found:  In ATExecColumnDefault(), there is an 
errhint() that suggests DROP EXPRESSION instead of DROP DEFAULT.  You 
could now add another hint that suggests SET EXPRESSION instead of SET 
DEFAULT.

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?

(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?)

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

On 11.12.23 13:22, Amul Sul wrote:
> 
>     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.

Could we have AT_PASS_ADD_COL before AT_PASS_OLD_*?  So overall it would be

...
AT_PASS_ALTER_TYPE,
AT_PASS_ADD_COL,         // moved
AT_PASS_SET_EXPRESSION,  // new
AT_PASS_OLD_INDEX,
AT_PASS_OLD_CONSTR,
AT_PASS_ADD_CONSTR,
...

This appears to not break any existing tests.

On 25.12.23 13:10, Amul Sul wrote:
>      > 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.
> 
>     Could we have AT_PASS_ADD_COL before AT_PASS_OLD_*?  So overall it
>     would be
> 
>     ...
>     AT_PASS_ALTER_TYPE,
>     AT_PASS_ADD_COL,         // moved
>     AT_PASS_SET_EXPRESSION,  // new
>     AT_PASS_OLD_INDEX,
>     AT_PASS_OLD_CONSTR,
>     AT_PASS_ADD_CONSTR,
>     ...
> 
>     This appears to not break any existing tests.
> 
> 
> (Sorry, for the delay)
> 
> Agree. I did that change in 0001 patch.

I have committed this patch set.

I couple of notes:

You had included the moving of the AT_PASS_ADD_COL enum in the first 
patch.  This is not a good style.  Refactoring patches should not 
include semantic changes.  I have moved that change the final patch that 
introduced the new feature.

I did not commit the 0002 patch that renamed some functions.  I think 
names like AlterColumn are too general, which makes this renaming 
possibly counterproductive.  I don't know a better name, maybe 
AlterTypeOrSimilar, but that's obviously silly.  I think leaving it at 
AlterType isn't too bad, since most of the code is indeed for ALTER TYPE 
support.  We can reconsider this if we have a better idea.

In RememberAllDependentForRebuilding(), I dropped some of the additional 
errors that you introduced for the AT_SetExpression cases.  These didn't 
seem useful.  For example, it is not possible for a generated column to 
depend on another generated column, so there is no point checking for 
it.  Also, there were no test cases that covered any of these 
situations.  If we do want any of these, we should have tests and 
documentation for them.

For the tests that examine the EXPLAIN plans, I had to add an ANALYZE 
after the SET EXPRESSION.  Otherwise, I got unstable test results, 
presumably because in some cases the analyze happened in the background.