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

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

On Wed, Aug 2, 2023 at 6:36 PM Amul Sul <sulamul(at)gmail(dot)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 Wed, Aug 2, 2023 at 9:16 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:

> On Wed, Aug 2, 2023 at 6:36 PM Amul Sul <sulamul(at)gmail(dot)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?

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

On Thu, Aug 3, 2023 at 1:23 PM Amul Sul <sulamul(at)gmail(dot)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 correct results.
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

Hi Amul,

On Wed, Aug 2, 2023 at 4:06 PM Amul Sul <sulamul(at)gmail(dot)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
>
> +1 to the idea.

Here are few review comments:.
*0001 patch*
1. Alignment changed for below comment:

> AT_ColumnExpression, /* alter column drop expression */

*00002 patch*
1. EXPRESSION should be added after DEFAULT per alphabetic order?

> + COMPLETE_WITH("(", "COMPRESSION", "EXPRESSION", "DEFAULT", "GENERATED",
> "NOT NULL", "STATISTICS", "STORAGE",
>

2. The variable *isdrop* can be aligned better:

> + bool isdrop = (cmd->def == NULL);
>

3. The AlteredTableInfo structure has member Relation, So need to pass
parameter Relation separately?

> static ObjectAddress ATExecColumnExpression(AlteredTableInfo *tab,
> Relation rel,
> const char *colName, Node *newDefault,
> bool missing_ok, LOCKMODE lockmode);
>

4. Exceeded 80 char limit:

> /*
> * Mark the column as no longer generated. (The atthasdef flag needs to
>

5. Update the comment. Use 'set' along with 'drop':

> AT_ColumnExpression, /* alter column drop expression */

Thanks,
Vaibhav Dalvi

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

On Thu, Aug 24, 2023 at 9:36 AM Vaibhav Dalvi <
vaibhav(dot)dalvi(at)enterprisedb(dot)com> wrote:

> Hi Amul,
>
>
> On Wed, Aug 2, 2023 at 4:06 PM Amul Sul <sulamul(at)gmail(dot)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
>>
>> +1 to the idea.
>

Thank you.

> 3. The AlteredTableInfo structure has member Relation, So need to pass
> parameter Relation separately?
>
>> static ObjectAddress ATExecColumnExpression(AlteredTableInfo *tab,
>> Relation rel,
>> const char *colName, Node *newDefault,
>> bool missing_ok, LOCKMODE lockmode);
>
>
Yeah, but I think, let it be since other AT routines have the same.

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.

Regards,
Amul

On Fri, Aug 25, 2023 at 5:35 AM Vik Fearing <vik(at)postgresfriends(dot)org> wrote:

> 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!
>
>
Great, thank you so much.

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

Well, I had to have the same thought but Peter Eisentraut thinks that we
should
have that in a separate patch & I am fine with that.

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

I am not sure I understood this, why would that break the documentation
even if
we allow non-generated columns to be generated. This makes the code flow
simple
and doesn't have any issue for the future extension to allow non-generated
columns too.

>
> 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 that is feasible, but I am not sure if we want to do that & add an
extra
code for the case, which is not really breaking anything except making the
system do extra work for the user's thoughtless action.

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

Agree, added in the v2 version posted a few minutes ago.

Regards,
Amul

Hi!

I'm pretty much like the idea of the patch. Looks like an overlook in SQL
standard for me.
Anyway, patch apply with no conflicts and implements described
functionality.

On Fri, 25 Aug 2023 at 03:06, Vik Fearing <vik(at)postgresfriends(dot)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.

Hope, that helps. Again, I'm +1 for this patch.

--
Best regards,
Maxim Orlov.

On Wed, Sep 13, 2023 at 2:28 PM Maxim Orlov <orlovmg(at)gmail(dot)com> wrote:

> Hi!
>
> I'm pretty much like the idea of the patch. Looks like an overlook in SQL
> standard for me.
> Anyway, patch apply with no conflicts and implements described
> functionality.
>
>
Thank you for looking at this.

> On Fri, 25 Aug 2023 at 03:06, Vik Fearing <vik(at)postgresfriends(dot)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.

Regards,
Amul

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

>> On Fri, 25 Aug 2023 at 03:06, Vik Fearing <vik(at)postgresfriends(dot)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 Thu, Sep 14, 2023 at 7:23 PM Ashutosh Bapat <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com>
wrote:

> Hi Amul,
> I share others opinion that this feature is useful.
>
> >> On Fri, 25 Aug 2023 at 03:06, Vik Fearing <vik(at)postgresfriends(dot)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.
>

Yes, that going to be a bit complicated including the case trying to convert
the non-generated column of a child table where need to find all the
ancestors
and siblings and make the same changes.

Regards,
Amul

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(at)eisentraut(dot)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(at)eisentraut(dot)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 6:03 PM Peter Eisentraut <peter(at)eisentraut(dot)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.
>

If I am not mistaken, the existing table rewrite facilities for ALTER TABLE
don't have support to run triggers or generate an event for each row,
right?

Do you expect to write a new code to handle this rewriting?

Regards,
Amul

On Fri, Oct 6, 2023 at 9:14 AM Peter Eisentraut <peter(at)eisentraut(dot)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

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.

Regards,
Amul

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 Tue, Nov 7, 2023 at 8:21 PM Peter Eisentraut <peter(at)eisentraut(dot)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.

Regards,
Amul

On Thu, 9 Nov 2023 at 15:01, Amul Sul <sulamul(at)gmail(dot)com> wrote:

>
> Here is the updated version patch. Did minor changes to documents and
> tests.
>
Overall patch looks good to me. Since Peter did withdraw his comment on
triggers and no open problems
are present, we can make this patch RfC, shall we? It would be nice to
correct this in the next release.

--
Best regards,
Maxim Orlov.

On 09.11.23 13:00, Amul Sul wrote:
> On Tue, Nov 7, 2023 at 8:21 PM Peter Eisentraut <peter(at)eisentraut(dot)org
> <mailto:peter(at)eisentraut(dot)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 Mon, Nov 13, 2023 at 1:40 PM Peter Eisentraut <peter(at)eisentraut(dot)org>
wrote:

> On 09.11.23 13:00, Amul Sul wrote:
> > On Tue, Nov 7, 2023 at 8:21 PM Peter Eisentraut <peter(at)eisentraut(dot)org
> > <mailto:peter(at)eisentraut(dot)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.
>

Understood. Will do the same.

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

> 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
ALTER COLUMN ... SET DEFAULT <a_expr>. Let me know if you want
me to add that.

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.

Regards,
Amul

1] postgr.es/m/[email protected]

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 Mon, Nov 13, 2023 at 9:09 PM Peter Eisentraut <peter(at)eisentraut(dot)org>
wrote:

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

Ok.

Please have a look at the attached version, updating the syntax to have "AS"
after EXPRESSION and other changes suggested previously.

Regards,
Amul

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 Wed, Nov 15, 2023 at 5:09 PM Peter Eisentraut <peter(at)eisentraut(dot)org>
wrote:

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

Thank you for your review.

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

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

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

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

Ok, will fix that.

Regards,
Amul

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 Thu, Nov 16, 2023 at 2:50 AM Peter Eisentraut <peter(at)eisentraut(dot)org>
wrote:

> 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, you are correct, but currently AT_PASS_ADD_OTHERCONSTR is for
AT_CookedColumnDefault, AT_ColumnDefault, and AT_AddIdentity.
AT_CookedColumnDefault is only supported for CREATE TABLE. AT_ColumnDefault
and AT_AddIdentity will be having errors while operating on the generated
column. So
that anomaly does not exist, but could be in future addition. I think it is
better to
use AT_PASS_MISC to keep this operation at last.

While testing this, I found a serious problem with the patch that CHECK and
FOREIGN KEY constraint check does not happens at rewrite, see this:

create table a (y int primary key);
insert into a values(1),(2);
create table b (x int, y int generated always as(x) stored, foreign key(y)
references a(y));
insert into b values(1),(2);
insert into b values(3); <------ an error, expected one

alter table b alter column y set expression as (x*100); <------ no error,
NOT expected

select * from b;
x | y
---+-----
1 | 100
2 | 200
(2 rows)

Also,

delete from a; <------ no error, NOT expected.
select * from a;
y
---
(0 rows)

Shouldn't that have been handled by the ATRewriteTables() facility
implicitly
like NOT NULL constraints? Or should we prepare a list of CHECK and FK
constraints and pass it through tab->constraints?

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

Ok.

Regards,
Amul Sul

On Thu, Nov 16, 2023 at 7:05 PM Amul Sul <sulamul(at)gmail(dot)com> wrote:

> On Thu, Nov 16, 2023 at 2:50 AM Peter Eisentraut <peter(at)eisentraut(dot)org>
> wrote:
>
>> 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, you are correct, but currently AT_PASS_ADD_OTHERCONSTR is for
> AT_CookedColumnDefault, AT_ColumnDefault, and AT_AddIdentity.
> AT_CookedColumnDefault is only supported for CREATE TABLE.
> AT_ColumnDefault
> and AT_AddIdentity will be having errors while operating on the generated
> column. So
> that anomaly does not exist, but could be in future addition. I think it
> is better to
> use AT_PASS_MISC to keep this operation at last.
>
> While testing this, I found a serious problem with the patch that CHECK and
> FOREIGN KEY constraint check does not happens at rewrite, see this:
>
> create table a (y int primary key);
> insert into a values(1),(2);
> create table b (x int, y int generated always as(x) stored, foreign
> key(y) references a(y));
> insert into b values(1),(2);
> insert into b values(3); <------ an error, expected one
>
> alter table b alter column y set expression as (x*100); <------ no
> error, NOT expected
>
> select * from b;
> x | y
> ---+-----
> 1 | 100
> 2 | 200
> (2 rows)
>
> Also,
>
> delete from a; <------ no error, NOT expected.
> select * from a;
> y
> ---
> (0 rows)
>
> Shouldn't that have been handled by the ATRewriteTables() facility
> implicitly
> like NOT NULL constraints? Or should we prepare a list of CHECK and FK
> constraints and pass it through tab->constraints?
>

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.

Regards,
Amul

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

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

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 Mon, Dec 18, 2023 at 3:01 PM Peter Eisentraut <peter(at)eisentraut(dot)org>
wrote:

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

(Sorry, for the delay)

Agree. I did that change in 0001 patch.

Regards,
Amul

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.

On Fri, Jan 5, 2024 at 12:28 AM Peter Eisentraut <peter(at)eisentraut(dot)org>
wrote:

> On 25.12.23 13:10, Amul Sul wrote:
> >
> 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.
>
>
Understood.

Thank you for your guidance and the commit.

Regards,
Amul