Re: Virtual generated columns

On 23.08.24 11:06, jian he wrote:
> drop table if exists gtest_err_1 cascade;
> CREATE TABLE gtest_err_1 (
> a int PRIMARY KEY generated by default as identity,
> b int GENERATED ALWAYS AS (22),
> d int default 22);
> create view gtest_err_1_v as select * from gtest_err_1;
> SELECT events & 4 != 0 AS can_upd, events & 8 != 0 AS can_ins,events &
> 16 != 0 AS can_del
> FROM pg_catalog.pg_relation_is_updatable('gtest_err_1_v'::regclass,
> false) t(events);
>
> insert into gtest_err_1_v(a,b, d) values ( 11, default,33) returning *;
> should the above query, b return 22?
> even b is "b int default" will return 22.

Confirmed. This is a bug in the rewriting that will hopefully be fixed
when I get to, uh, rewriting that using Dean's suggestions. Not done
here. (The problem, in the current implementation, is that
query->hasGeneratedVirtual does not get preserved in the view, and then
expand_generated_columns_in_query() does the wrong thing.)

> drop table if exists comment_test cascade;
> CREATE TABLE comment_test (
> id int,
> positive_col int GENERATED ALWAYS AS (22) CHECK (positive_col > 0),
> positive_col1 int GENERATED ALWAYS AS (22) stored CHECK (positive_col > 0) ,
> indexed_col int,
> CONSTRAINT comment_test_pk PRIMARY KEY (id));
> CREATE INDEX comment_test_index ON comment_test(indexed_col);
> ALTER TABLE comment_test ALTER COLUMN positive_col1 SET DATA TYPE text;
> ALTER TABLE comment_test ALTER COLUMN positive_col SET DATA TYPE text;
> the last query should work just fine?

I played with this and I don't see anything wrong with the current
behavior. I noticed that in your test case

> positive_col1 int GENERATED ALWAYS AS (22) stored CHECK
(positive_col > 0) ,

you have the wrong column name in the check constraint. I'm not sure if
that was intentional.

> drop table if exists def_test cascade;
> create table def_test (
> c0 int4 GENERATED ALWAYS AS (22) stored,
> c1 int4 GENERATED ALWAYS AS (22),
> c2 text default 'initial_default'
> );
> alter table def_test alter column c1 set default 10;
> ERROR: column "c1" of relation "def_test" is a generated column
> HINT: Use ALTER TABLE ... ALTER COLUMN ... SET EXPRESSION instead.
> alter table def_test alter column c1 drop default;
> ERROR: column "c1" of relation "def_test" is a generated column
>
> Is the first error message hint wrong?

Yes, somewhat. I looked into fixing that, but that got a bit messy. I
hope to be able to implement SET EXPRESSION before too long, so I'm
leaving it for now.

> also the second error message (column x is a generated column) is not helpful.
> here, we should just say that cannot set/drop default for virtual
> generated column?

Maybe, but that's not part of this patch.

> drop table if exists bar1, bar2;
> create table bar1(a integer, b integer GENERATED ALWAYS AS (22))
> partition by range (a);
> create table bar2(a integer);
> alter table bar2 add column b integer GENERATED ALWAYS AS (22) stored;
> alter table bar1 attach partition bar2 default;
> this works, which will make partitioned table and partition have
> different kinds of generated column,
> but this is not what we expected?

Fixed. (Needed code in MergeAttributesIntoExisting() similar to
MergeChildAttribute().)

> drop table if exists tp, tpp1, tpp2;
> CREATE TABLE tp (a int NOT NULL,b text GENERATED ALWAYS AS (22),c
> text) PARTITION BY LIST (a);
> CREATE TABLE tpp1(a int NOT NULL, b text GENERATED ALWAYS AS (c
> ||'1000' ), c text);
> ALTER TABLE tp ATTACH PARTITION tpp1 FOR VALUES IN (1);
> insert into tp(a,b,c) values (1,default, 'hello') returning a,b,c;
> insert into tpp1(a,b,c) values (1,default, 'hello') returning a,b,c;
>
> select tableoid::regclass, * from tpp1;
> select tableoid::regclass, * from tp;
> the above two queries return different results, slightly unintuitive, i guess.
> Do we need to mention it somewhere?

It is documented in ddl.sgml:

+ For virtual
+ generated columns, the generation expression of the table named in the
+ query applies when a table is read.

> CREATE TABLE atnotnull1 ();
> ALTER TABLE atnotnull1 ADD COLUMN c INT GENERATED ALWAYS AS (22), ADD
> PRIMARY KEY (c);
> ERROR: not-null constraints are not supported on virtual generated columns
> DETAIL: Column "c" of relation "atnotnull1" is a virtual generated column.
> I guess this error message is fine.

Yeah, maybe this will get improved when the catalogued not-null
constraints come back. Better wait for that.

> The last issue in the previous thread [1], ATPrepAlterColumnType
> seems not addressed.
>
> [1] https://postgr.es/m/CACJufxEGPYtFe79hbsMeOBOivfNnPRsw7Gjvk67m1x2MQggyiQ@mail.gmail.com

This is fixed now.

I also committed the two patches that renamed the existing test files,
so those are not included here anymore.

The new patch does some rebasing and contains various fixes to the
issues you presented. As I mentioned, I'll look into improving the
rewriting.