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 Wed, Aug 2, 2023 at 9:16 PM jian he <jian.universality@gmail.com> wrote:

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?

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

+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.dalvi@enterprisedb.com> wrote:

Hi Amul,

On Wed, Aug 2, 2023 at 4:06 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

+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@postgresfriends.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@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.

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

Regards,
Amul

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]: https://www.postgresql.org/docs/16/ddl-generated-columns.html
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.bapat.oss@gmail.com>
wrote:

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.

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@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 6:03 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.

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

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