Virtual generated columns

On Mon, Apr 29, 2024 at 4:24 AM Peter Eisentraut <peter@eisentraut.org>
wrote:

Here is a patch set to implement virtual generated columns.

I'm very excited about this!

The main feature patch (0005 here) generally works but has a number of
open corner cases that need to be thought about and/or fixed, many of
which are marked in the code or the tests. I'll continue working on
that. But I wanted to see if I can get some feedback on the test
structure, so I don't have to keep changing it around later.

I'd be very interested to see virtual generated columns working, as one of
my past customers had a need to reclassify data in a partitioned table, and
the ability to detach a partition, alter the virtual generated columns, and
re-attach would have been great. In case you care, it was basically an
"expired" flag, but the rules for what data "expired" varied by country of
customer and level of service.

+ * Stored generated columns cannot work: They are computed after
+ * BEFORE triggers, but partition routing is done before all
+ * triggers.  Maybe virtual generated columns could be made to
+ * work, but then they would need to be handled as an expression
+ * below.

I'd say you nailed it with the test structure. The stored/virtual
copy/split is the ideal way to approach this, which makes the diff very
easy to understand.

+1 for not handling domain types yet.

 -- generation expression must be immutable
-CREATE TABLE gtest_err_4 (a int PRIMARY KEY, b double precision GENERATED
ALWAYS AS (random()) STORED);
+CREATE TABLE gtest_err_4 (a int PRIMARY KEY, b double precision GENERATED
ALWAYS AS (random()) VIRTUAL);

Does a VIRTUAL generated column have to be immutable? I can see where the
STORED one has to be, but consider the following:

CREATE TABLE foo (
created_at timestamptz DEFAULT CURRENT_TIMESTAMP,
row_age interval GENERATED ALWAYS AS CURRENT_TIMESTAMP - created_at
);

 -- can't have generated column that is a child of normal column
 CREATE TABLE gtest_normal (a int, b int);
-CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS (a * 2)
STORED) INHERITS (gtest_normal);  -- error
+CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS (a * 2)
VIRTUAL) INHERITS (gtest_normal);  -- error

This is the barrier to the partitioning reorganization scheme I described
above. Is there any hard rule why a child table couldn't have a generated
column matching the parent's regular column? I can see where it might
prevent indexing that column on the parent table, but is there some other
dealbreaker or is this just a "it doesn't work yet" situation?

One last thing to keep in mind is that there are two special case
expressions in the spec:

GENERATED ALWAYS AS ROW START
GENERATED ALWAYS AS ROW END

and we'll need to be able to fit those into the catalog. I'll start another
thread for that unless you prefer I keep it here.

On 29.04.24 10:23, Peter Eisentraut wrote:

Here is a patch set to implement virtual generated columns.

The main feature patch (0005 here) generally works but has a number of
open corner cases that need to be thought about and/or fixed, many of
which are marked in the code or the tests.  I'll continue working on
that.  But I wanted to see if I can get some feedback on the test
structure, so I don't have to keep changing it around later.

Here is an updated patch set. It needed some rebasing, especially
around the reverting of the catalogued not-null constraints. I have
also fixed up the various incomplete or "fixme" pieces of code mentioned
above. I have in most cases added "not supported yet" error messages
for now, with the idea that some of these things can be added in later,
as incremental features.

In particular, quoting from the commit message, the following are
currently not supported (but could possibly be added as incremental
features, some easier than others):

- index on virtual column
- expression index using a virtual column
- hence also no unique constraints on virtual columns
- not-null constraints on virtual columns
- (check constraints are supported)
- foreign key constraints on virtual columns
- extended statistics on virtual columns
- ALTER TABLE / SET EXPRESSION
- ALTER TABLE / DROP EXPRESSION
- virtual columns as trigger columns
- virtual column cannot have domain type

So, I think this basically works now, and the things that don't work
should be appropriately prevented. So if someone wants to test this and
tell me what in fact doesn't work correctly, that would be helpful.

On 29.04.24 20:54, Corey Huinker wrote:

 -- generation expression must be immutable
-CREATE TABLE gtest_err_4 (a int PRIMARY KEY, b double precision
GENERATED ALWAYS AS (random()) STORED);
+CREATE TABLE gtest_err_4 (a int PRIMARY KEY, b double precision
GENERATED ALWAYS AS (random()) VIRTUAL);

Does a VIRTUAL generated column have to be immutable? I can see where
the STORED one has to be, but consider the following:

CREATE TABLE foo (
created_at timestamptz DEFAULT CURRENT_TIMESTAMP,
row_age interval GENERATED ALWAYS AS CURRENT_TIMESTAMP - created_at
);

I have been hesitant about this, but I'm now leaning toward that we
could allow this.

 -- can't have generated column that is a child of normal column
 CREATE TABLE gtest_normal (a int, b int);
-CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS
(a * 2) STORED) INHERITS (gtest_normal);  -- error
+CREATE TABLE gtest_normal_child (a int, b int GENERATED ALWAYS AS
(a * 2) VIRTUAL) INHERITS (gtest_normal);  -- error

This is the barrier to the partitioning reorganization scheme I
described above. Is there any hard rule why a child table couldn't have
a generated column matching the parent's regular column? I can see where
it might prevent indexing that column on the parent table, but is there
some other dealbreaker or is this just a "it doesn't work yet" situation?

We had a quite a difficult time getting the inheritance business of
stored generated columns working correctly. I'm sticking to the
well-trodden path here. We can possibly expand this if someone wants to
work out the details.

One last thing to keep in mind is that there are two special case
expressions in the spec:

GENERATED ALWAYS AS ROW START
GENERATED ALWAYS AS ROW END

and we'll need to be able to fit those into the catalog. I'll start
another thread for that unless you prefer I keep it here.

I think this is a separate feature.

On Wed, 2024-05-22 at 19:22 +0200, Peter Eisentraut wrote:

On 29.04.24 10:23, Peter Eisentraut wrote:

Here is a patch set to implement virtual generated columns.

The main feature patch (0005 here) generally works but has a number
of
open corner cases that need to be thought about and/or fixed, many
of
which are marked in the code or the tests.  I'll continue working
on
that.  But I wanted to see if I can get some feedback on the test
structure, so I don't have to keep changing it around later.

Here is an updated patch set.  It needed some rebasing, especially
around the reverting of the catalogued not-null constraints.  I have
also fixed up the various incomplete or "fixme" pieces of code
mentioned
above.  I have in most cases added "not supported yet" error messages
for now, with the idea that some of these things can be added in
later,
as incremental features.

This is not (yet) full review.

Patches applied cleanly on 76618097a6c027ec603a3dd143f61098e3fb9794
from 2024-06-14.
I've run
./configure && make world-bin && make check && make check-world
on 0001, then 0001+0002, then 0001+0002+0003, up to applying
all 5 patches. All cases passed on Debian unstable on aarch64 (ARM64)
on gcc (Debian 13.2.0-25) 13.2.0.

v1-0001-Rename-regress-test-generated-to-generated_stored.patch:
no objections here, makes sense as preparation for future changes

v1-0002-Put-generated_stored-test-objects-in-a-schema.patch:
also no objections.
OTOH other tests (like publication.out, rowsecurity.out, stats_ext.out,
tablespace.out) are creating schemas and later dropping them - so here
it might also make sense to drop schema at the end of testing.

v1-0003-Remove-useless-initializations.patch:
All other cases (I checked directory src/backend/utils/cache)
calling MemoryContextAllocZero only initialize fields when they
are non-zero, so removing partial initialization with false brings
consistency to the code.

v1-0004-Remove-useless-code.patch:
Patch removes filling in of constraints from function
BuildDescForRelation. This function is only called from file
view.c and tablecmds.c (twice). In none of those cases
result->constr is used, so proposed change makes sense.
While I do not know code well, so might be wrong here,
I would apply this patch.

I haven't looked at the most important (and biggest) file yet,
v1-0005-Virtual-generated-columns.patch; hope to look at it
this week.
At the same I believe 0001-0004 can be applied - even backported
if it'll make maintenance of future changes easier. But that should
be commiter's decision.

Best regards

--
Tomasz Rybak, Debian Developer <serpent@debian.org>
GPG: A565 CE64 F866 A258 4DDC F9C7 ECB7 3E37 E887 AA8C

On Thu, May 23, 2024 at 1:23 AM Peter Eisentraut <peter@eisentraut.org> wrote:

On 29.04.24 10:23, Peter Eisentraut wrote:

Here is a patch set to implement virtual generated columns.

The main feature patch (0005 here) generally works but has a number of
open corner cases that need to be thought about and/or fixed, many of
which are marked in the code or the tests. I'll continue working on
that. But I wanted to see if I can get some feedback on the test
structure, so I don't have to keep changing it around later.

the test structure you made ( generated_stored.sql,
generated_virtual.sq) looks ok to me.
but do we need to reset the search_path at the end of
generated_stored.sql, generated_virtual.sql?

most of the test tables didn't use much storage,
maybe not necessary to clean up (drop the test table) at the end of sql files.

So, I think this basically works now, and the things that don't work
should be appropriately prevented. So if someone wants to test this and
tell me what in fact doesn't work correctly, that would be helpful.

in https://www.postgresql.org/docs/current/catalog-pg-attrdef.html

The catalog pg_attrdef stores column default values. The main
information about columns is stored in pg_attribute. Only columns for
which a default value has been explicitly set will have an entry here.

didn't mention generated columns related expressions.
Do we need to add something here? maybe a separate issue?

+ /*
+ * TODO: This could be done, but it would need a different implementation:
+ * no rewriting, but still need to recheck any constraints.
+ */
+ if (attTup->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ALTER TABLE / SET EXPRESSION is not supported for virtual
generated columns"),
+ errdetail("Column \"%s\" of relation \"%s\" is a virtual generated column.",
+   colName, RelationGetRelationName(rel))));

minor typo, should be
+ errmsg("ALTER TABLE SET EXPRESSION is not supported for virtual
generated columns"),

insert/update/delete/merge returning have problems:
CREATE TABLE t2 (
a int ,
b int GENERATED ALWAYS AS (a * 2),
d int default 22);
insert into t2(a) select g from generate_series(1,10) g;

insert into t2 select 100 returning *, (select t2.b), t2.b = t2.a * 2;
update t2 set a = 12 returning *, (select t2.b), t2.b = t2.a * 2;
update t2 set a = 12 returning *, (select (select t2.b)), t2.b = t2.a * 2;
delete from t2 where t2.b = t2.a * 2 returning *, 1,((select t2.b));

currently all these query, error message is "unexpected virtual
generated column reference"
we expect above these query work?

issue with merge:
CREATE TABLE t0 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 2) VIRTUAL);
insert into t0(a) select g from generate_series(1,10) g;
MERGE INTO t0 t USING t0 AS s ON 2 * t.a = s.b WHEN MATCHED THEN
DELETE returning *;

the above query returns zero rows, but for stored generated columns it
will return 10 rows.

in transformMergeStmt(ParseState *pstate, MergeStmt *stmt)
add
`qry->hasGeneratedVirtual = pstate->p_hasGeneratedVirtual;`
before
`assign_query_collations(pstate, qry);`
solve the problem.

On 28.06.24 02:00, jian he wrote:

inhttps://www.postgresql.org/docs/current/catalog-pg-attrdef.html
The catalog pg_attrdef stores column default values. The main
information about columns is stored in pg_attribute. Only columns for
which a default value has been explicitly set will have an entry here.
didn't mention generated columns related expressions.
Do we need to add something here? maybe a separate issue?

Yes and yes. I have committed a separate change to update the
documentation here.

On 17.06.24 21:31, Tomasz Rybak wrote:

v1-0001-Rename-regress-test-generated-to-generated_stored.patch:
no objections here, makes sense as preparation for future changes

v1-0002-Put-generated_stored-test-objects-in-a-schema.patch:
also no objections.
OTOH other tests (like publication.out, rowsecurity.out, stats_ext.out,
tablespace.out) are creating schemas and later dropping them - so here
it might also make sense to drop schema at the end of testing.

The existing tests for generated columns don't drop what they create at
the end, which can be useful for pg_upgrade testing for example. So
unless there are specific reasons to change it, I would leave that as is.

Other tests might have other reasons. For example, publications or row
security might interfere with many other tests.

v1-0003-Remove-useless-initializations.patch:
All other cases (I checked directory src/backend/utils/cache)
calling MemoryContextAllocZero only initialize fields when they
are non-zero, so removing partial initialization with false brings
consistency to the code.

v1-0004-Remove-useless-code.patch:
Patch removes filling in of constraints from function
BuildDescForRelation. This function is only called from file
view.c and tablecmds.c (twice). In none of those cases
result->constr is used, so proposed change makes sense.
While I do not know code well, so might be wrong here,
I would apply this patch.

I have committed these two now.

On 28.06.24 02:00, jian he wrote:

the test structure you made ( generated_stored.sql,
generated_virtual.sq) looks ok to me.
but do we need to reset the search_path at the end of
generated_stored.sql, generated_virtual.sql?

No, the session ends at the end of the test file, so we don't need to
reset session state.

+ /*
+ * TODO: This could be done, but it would need a different implementation:
+ * no rewriting, but still need to recheck any constraints.
+ */
+ if (attTup->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("ALTER TABLE / SET EXPRESSION is not supported for virtual
generated columns"),
+ errdetail("Column \"%s\" of relation \"%s\" is a virtual generated column.",
+   colName, RelationGetRelationName(rel))));

minor typo, should be
+ errmsg("ALTER TABLE SET EXPRESSION is not supported for virtual
generated columns"),

This style "ALTER TABLE / something else" is also used for other error
messages related to ALTER TABLE subcommands, so I am using the same here.

insert/update/delete/merge returning have problems:
CREATE TABLE t2 (
a int ,
b int GENERATED ALWAYS AS (a * 2),
d int default 22);
insert into t2(a) select g from generate_series(1,10) g;

insert into t2 select 100 returning *, (select t2.b), t2.b = t2.a * 2;
update t2 set a = 12 returning *, (select t2.b), t2.b = t2.a * 2;
update t2 set a = 12 returning *, (select (select t2.b)), t2.b = t2.a * 2;
delete from t2 where t2.b = t2.a * 2 returning *, 1,((select t2.b));

currently all these query, error message is "unexpected virtual
generated column reference"
we expect above these query work?

Yes, this is a bug. I'm looking into it.

issue with merge:
CREATE TABLE t0 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 2) VIRTUAL);
insert into t0(a) select g from generate_series(1,10) g;
MERGE INTO t0 t USING t0 AS s ON 2 * t.a = s.b WHEN MATCHED THEN
DELETE returning *;

the above query returns zero rows, but for stored generated columns it
will return 10 rows.

in transformMergeStmt(ParseState *pstate, MergeStmt *stmt)
add
`qry->hasGeneratedVirtual = pstate->p_hasGeneratedVirtual;`
before
`assign_query_collations(pstate, qry);`
solve the problem.

Good catch. Will fix.

Thanks for this review. I will work on fixing the issues above and come
back with a new patch set.

statistic related bug.
borrow examples from
https://www.postgresql.org/docs/current/sql-createstatistics.html

CREATE TABLE t3 (a timestamp PRIMARY KEY, b timestamp GENERATED
ALWAYS AS (a) VIRTUAL);
CREATE STATISTICS s3 (ndistinct) ON b FROM t3;
INSERT INTO t3(a) SELECT i FROM generate_series('2020-01-01'::timestamp,
'2020-12-31'::timestamp,
'1 minute'::interval) s(i);
ANALYZE t3;
CREATE STATISTICS s3 (ndistinct) ON date_trunc('month', a),
date_trunc('day', b) FROM t3;
ANALYZE t3;
ERROR: unexpected virtual generated column reference

--this is allowed
CREATE STATISTICS s5 ON (b + interval '1 day') FROM t3;
--this is not allowed. seems inconsistent?
CREATE STATISTICS s6 ON (b ) FROM t3;

in CreateStatistics(CreateStatsStmt *stmt)
we have

if (selem->name)
{
if (attForm->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("statistics creation on virtual
generated columns is not supported")));
}
else if (IsA(selem->expr, Var)) /* column reference in parens */
{
if (get_attgenerated(relid, var->varattno) ==
ATTRIBUTE_GENERATED_VIRTUAL)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("statistics creation on virtual
generated columns is not supported")));
}
else /* expression */
{
...
}

you didn't make sure the last "else" branch is not related to virtual
generated columns

another bug?
drop table gtest12v;
CREATE TABLE gtest12v (a int PRIMARY KEY, b bigint, c int GENERATED
ALWAYS AS (b * 2) VIRTUAL);
insert into gtest12v (a,b) values (11, 22147483647);
table gtest12v;

insert ok, but select error:
ERROR: integer out of range

should insert fail?

CREATE TABLE gtest12v (a int PRIMARY KEY, b bigint, c int GENERATED
ALWAYS AS (b * 2) VIRTUAL);
CREATE SEQUENCE sequence_testx OWNED BY gtest12v.c;

seems to work. But I am not sure if there are any corner cases that
make it not work.
just want to raise this issue.

drop table t3;
CREATE TABLE t3( b bigint, c int GENERATED ALWAYS AS (b * 2) VIRTUAL);
insert into t3 (b) values (22147483647);
ANALYZE t3;

for ANALYZE
since column c has no actual storage, so it's not analyzable?
we need to change the function examine_attribute accordingly?

For the above example, for each insert row, we actually need to call
int84 to validate c value.
we probably need something similar to have ExecComputeStoredGenerated etc,
but we don't need to store it.

On 22.07.24 12:53, jian he wrote:

another bug?
drop table gtest12v;
CREATE TABLE gtest12v (a int PRIMARY KEY, b bigint, c int GENERATED
ALWAYS AS (b * 2) VIRTUAL);
insert into gtest12v (a,b) values (11, 22147483647);
table gtest12v;

insert ok, but select error:
ERROR: integer out of range

should insert fail?

I think this is the correct behavior.

There has been a previous discussion:
/messages/by-id/2e3d5147-16f8-af0f-00ab-4c72cafc896f@2ndquadrant.com

CREATE TABLE gtest12v (a int PRIMARY KEY, b bigint, c int GENERATED
ALWAYS AS (b * 2) VIRTUAL);
CREATE SEQUENCE sequence_testx OWNED BY gtest12v.c;

seems to work. But I am not sure if there are any corner cases that
make it not work.
just want to raise this issue.

I don't think this matters. You can make a sequence owned by any
column, even if that column doesn't have a default that invokes the
sequence. So nonsensical setups are possible, but they are harmless.

Thank you for your extensive testing. Here is a new patch set that has
fixed all the issues you have reported (MERGE, sublinks, statistics,
ANALYZE).

On Thu, 8 Aug 2024 at 07:23, Peter Eisentraut <peter@eisentraut.org> wrote:

Thank you for your extensive testing. Here is a new patch set that has
fixed all the issues you have reported (MERGE, sublinks, statistics,
ANALYZE).

I had a quick look at this and found one issue, which is that it
doesn't properly deal with virtual generated columns in wholerow
attributes:

CREATE TABLE foo(a int, a2 int GENERATED ALWAYS AS (a*2) VIRTUAL);
INSERT INTO foo VALUES (1);
SELECT foo FROM foo;

foo
------
(1,)
(1 row)

Looking at the rewriter changes, it occurred to me that it could
perhaps be done more simply using ReplaceVarsFromTargetList() for each
RTE with virtual generated columns. That function already has the
required wholerow handling code, so there'd be less code duplication.
I think it might be better to do this from within fireRIRrules(), just
after RLS policies are applied, so it wouldn't need to worry about
CTEs and sublink subqueries. That would also make the
hasGeneratedVirtual flags unnecessary, since we'd already only be
doing the extra work for tables with virtual generated columns. That
would eliminate possible bugs caused by failing to set those flags.

Regards,
Dean

On Thu, Aug 8, 2024 at 2:23 PM Peter Eisentraut <peter@eisentraut.org> wrote:

Thank you for your extensive testing. Here is a new patch set that has
fixed all the issues you have reported (MERGE, sublinks, statistics,
ANALYZE).

if (coldef->generated && restdef->generated &&
coldef->generated != restdef->generated)
ereport(ERROR,
(errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
errmsg("column \"%s\" inherits from
generated column of different kind",
restdef->colname)));
the error message is not informal. maybe add errhint that
"column \"%s\" should be same as parent table's generated column kind:
%s", "virtual"|"stored"

.../regress/expected/create_table_like.out | 23 +-
.../regress/expected/generated_stored.out | 27 +-
...rated_stored.out => generated_virtual.out} | 835 +++++++++---------
src/test/regress/parallel_schedule | 3 +
src/test/regress/sql/create_table_like.sql | 2 +-
src/test/regress/sql/generated_stored.sql | 10 +-
...rated_stored.sql => generated_virtual.sql} | 301 ++++---
src/test/subscription/t/011_generated.pl | 38 +-
55 files changed, 1280 insertions(+), 711 deletions(-)
copy src/test/regress/expected/{generated_stored.out
generated_virtual.out} (69%)
copy src/test/regress/sql/{generated_stored.sql => generated_virtual.sql} (72%)

I don't understand the "copy =>" part, I guess related to copy content
from stored to virtual.
anyway. some minor issue:

-- alter generation expression of parent and all its children altogether
ALTER TABLE gtest_parent ALTER COLUMN f3 SET EXPRESSION AS (f2 * 2);
\d gtest_parent
\d gtest_child
\d gtest_child2
\d gtest_child3
SELECT tableoid::regclass, * FROM gtest_parent ORDER BY 1, 2, 3;

The first line ALTER TABLE will fail for
src/test/regress/sql/generated_virtual.sql.
so no need
"""
\d gtest_parent
\d gtest_child
\d gtest_child2
\d gtest_child3
SELECT tableoid::regclass, * FROM gtest_parent ORDER BY 1, 2, 3;
"""

Similarly the following tests for gtest29 may aslo need change
-- ALTER TABLE ... ALTER COLUMN ... DROP EXPRESSION

since we cannot do ALTER TABLE SET EXPRESSION for virtual generated columns.

-- ALTER TABLE ... ALTER COLUMN
CREATE TABLE gtest27 (
a int,
b int,
x int GENERATED ALWAYS AS ((a + b) * 2) VIRTUAL
);
INSERT INTO gtest27 (a, b) VALUES (3, 7), (4, 11);
ALTER TABLE gtest27 ALTER COLUMN a TYPE text; -- error
ALTER TABLE gtest27 ALTER COLUMN x TYPE numeric;

will
ALTER TABLE gtest27 ALTER COLUMN a TYPE int4;
be a no-op?

do we need a document that virtual generated columns will use the
expression's collation.
see:
drop table if exists t5;
CREATE TABLE t5 (
a text collate "C",
b text collate "C" GENERATED ALWAYS AS (a collate case_insensitive) ,
d int DEFAULT 22
);
INSERT INTO t5(a,d) values ('d1',28), ('D2',27), ('D1',26);
select * from t5 order by b asc, d asc;

+ /*
+ * TODO: Prevent virtual generated columns from having a
+ * domain type.  We would have to enforce domain constraints
+ * when columns underlying the generated column change.  This
+ * could possibly be implemented, but it's not.
+ *
+ * XXX If column->typeName is not set, then this column
+ * definition is probably a partition definition and will
+ * presumably get its pre-vetted type from elsewhere.  If that
+ * doesn't hold, maybe this check needs to be moved elsewhere.
+ */
+ if (column->generated == ATTRIBUTE_GENERATED_VIRTUAL && column->typeName)
+ {
+ Type ctype;
+
+ ctype = typenameType(cxt->pstate, column->typeName, NULL);
+ if (((Form_pg_type) GETSTRUCT(ctype))->typtype == TYPTYPE_DOMAIN)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("virtual generated column \"%s\" cannot have a domain type",
+ column->colname),
+ parser_errposition(cxt->pstate,
+ column->location)));
+ ReleaseSysCache(ctype);
+ }

create domain mydomain as int4;
create type mydomainrange as range(subtype=mydomain);
CREATE TABLE t3( b bigint, c mydomain GENERATED ALWAYS AS ('11') VIRTUAL);
CREATE TABLE t3( b bigint, c mydomainrange GENERATED ALWAYS AS
('[4,50)') VIRTUAL);
domain will error out, domain over range is ok, is this fine?

+      When <literal>VIRTUAL</literal> is specified, the column will be
+      computed when it is read, and it will not occupy any storage.  When
+      <literal>STORED</literal> is specified, the column will be computed on
+      write and will be stored on disk.  <literal>VIRTUAL</literal> is the
+      default.
drop table if exists t5;
CREATE TABLE t5 (
    a int,
    b text storage extended collate "C"  GENERATED ALWAYS AS (a::text
collate case_insensitive) ,
    d int DEFAULT 22
);
select reltoastrelid <> 0 as has_toast_table from pg_class where oid =
't5'::regclass;

if really no storage, should table t5 have an associated toast table or not?
also check ALTER TABLE variant:
alter table t5 alter b set storage extended;

Do we need to do something in ATExecSetStatistics for cases like:
ALTER TABLE t5 ALTER b SET STATISTICS 2000;
(b is a generated virtual column).
because of
examine_attribute
if (attr->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
return NULL;
i guess, this won't have a big impact.

There are some issues with changing virtual generated column type.
like:
drop table if exists another;
create table another (f4 int, f2 text, f3 text, f1 int GENERATED
ALWAYS AS (f4));
insert into another values(1, 'one', 'uno'), (2, 'two', 'due'),(3,
'three', 'tre');
alter table another
alter f1 type text using f2 || ' and ' || f3 || ' more';
table another;

or
alter table another
alter f1 type text using f2 || ' and ' || f3 || ' more',
drop column f1;
ERROR: column "f1" of relation "another" does not exist

These two command outputs seem not right.
the stored generated column which works as expected.

in src/test/regress/sql/alter_table.sql
-- We disallow changing table's row type if it's used for storage
create table at_tab1 (a int, b text);
create table at_tab2 (x int, y at_tab1);
alter table at_tab1 alter column b type varchar; -- fails
drop table at_tab2;

I think the above restriction should apply to virtual generated columns too.
given in ATPrepAlterColumnType, not storage we still call
find_composite_type_dependencies

if (!RELKIND_HAS_STORAGE(tab->relkind))
{
/*
* For relations without storage, do this check now. Regular tables
* will check it later when the table is being rewritten.
*/
find_composite_type_dependencies(rel->rd_rel->reltype, rel, NULL);
}

so i think in ATPrepAlterColumnType, we should do:

if (attTup->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
{
find_composite_type_dependencies(rel->rd_rel->reltype, rel, NULL);
}
else if (tab->relkind == RELKIND_RELATION ||
tab->relkind == RELKIND_PARTITIONED_TABLE)
{
}
else if (transform)
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not a table",
RelationGetRelationName(rel))));

you may add following tests:
------------------------------------------------------------------------
create table at_tab1 (a int, b text GENERATED ALWAYS AS ('hello'), c text);
create table at_tab2 (x int, y at_tab1);
alter table at_tab1 alter column b type varchar; -- fails
drop table at_tab1, at_tab2;

-- Check it for a partitioned table, too
create table at_tab1 (a int, b text GENERATED ALWAYS AS ('hello'), c
text) partition by list(a);;
create table at_tab2 (x int, y at_tab1);
alter table at_tab1 alter column b type varchar; -- fails
drop table at_tab1, at_tab2;
---------------------------------------------------------------------------------

Thanks for the great testing again. Here is an updated patch that
addresses the issues you have pointed out.

On 14.08.24 02:00, jian he wrote:

On Thu, Aug 8, 2024 at 2:23 PM Peter Eisentraut <peter@eisentraut.org> wrote:

Thank you for your extensive testing. Here is a new patch set that has
fixed all the issues you have reported (MERGE, sublinks, statistics,
ANALYZE).

if (coldef->generated && restdef->generated &&
coldef->generated != restdef->generated)
ereport(ERROR,
(errcode(ERRCODE_INVALID_COLUMN_DEFINITION),
errmsg("column \"%s\" inherits from
generated column of different kind",
restdef->colname)));
the error message is not informal. maybe add errhint that
"column \"%s\" should be same as parent table's generated column kind:
%s", "virtual"|"stored"

Ok, I added an errdetail().

.../regress/expected/create_table_like.out | 23 +-
.../regress/expected/generated_stored.out | 27 +-
...rated_stored.out => generated_virtual.out} | 835 +++++++++---------
src/test/regress/parallel_schedule | 3 +
src/test/regress/sql/create_table_like.sql | 2 +-
src/test/regress/sql/generated_stored.sql | 10 +-
...rated_stored.sql => generated_virtual.sql} | 301 ++++---
src/test/subscription/t/011_generated.pl | 38 +-
55 files changed, 1280 insertions(+), 711 deletions(-)
copy src/test/regress/expected/{generated_stored.out
generated_virtual.out} (69%)
copy src/test/regress/sql/{generated_stored.sql => generated_virtual.sql} (72%)

I don't understand the "copy =>" part, I guess related to copy content
from stored to virtual.
anyway. some minor issue:

That's just what git format-patch produces. It shows that 72% of the
new file is a copy of the existing file.

-- alter generation expression of parent and all its children altogether
ALTER TABLE gtest_parent ALTER COLUMN f3 SET EXPRESSION AS (f2 * 2);
\d gtest_parent
\d gtest_child
\d gtest_child2
\d gtest_child3
SELECT tableoid::regclass, * FROM gtest_parent ORDER BY 1, 2, 3;

The first line ALTER TABLE will fail for
src/test/regress/sql/generated_virtual.sql.
so no need
"""
\d gtest_parent
\d gtest_child
\d gtest_child2
\d gtest_child3
SELECT tableoid::regclass, * FROM gtest_parent ORDER BY 1, 2, 3;
"""

Similarly the following tests for gtest29 may aslo need change
-- ALTER TABLE ... ALTER COLUMN ... DROP EXPRESSION

since we cannot do ALTER TABLE SET EXPRESSION for virtual generated columns.

I left all these tests in place from the equivalent STORED tests, in
case we want to add support for the VIRTUAL case as well. I expect that
we'll add support for some of these before too long.

-- ALTER TABLE ... ALTER COLUMN
CREATE TABLE gtest27 (
a int,
b int,
x int GENERATED ALWAYS AS ((a + b) * 2) VIRTUAL
);
INSERT INTO gtest27 (a, b) VALUES (3, 7), (4, 11);
ALTER TABLE gtest27 ALTER COLUMN a TYPE text; -- error
ALTER TABLE gtest27 ALTER COLUMN x TYPE numeric;

will
ALTER TABLE gtest27 ALTER COLUMN a TYPE int4;
be a no-op?

Changing the type of a column that is used by a generated column is
already prohibited. Are you proposing to change anything here?

do we need a document that virtual generated columns will use the
expression's collation.
see:
drop table if exists t5;
CREATE TABLE t5 (
a text collate "C",
b text collate "C" GENERATED ALWAYS AS (a collate case_insensitive) ,
d int DEFAULT 22
);
INSERT INTO t5(a,d) values ('d1',28), ('D2',27), ('D1',26);
select * from t5 order by b asc, d asc;

I have fixed this. It will now apply the collation of the column.

create domain mydomain as int4;
create type mydomainrange as range(subtype=mydomain);
CREATE TABLE t3( b bigint, c mydomain GENERATED ALWAYS AS ('11') VIRTUAL);
CREATE TABLE t3( b bigint, c mydomainrange GENERATED ALWAYS AS
('[4,50)') VIRTUAL);
domain will error out, domain over range is ok, is this fine?

Fixed. The check is now in CheckAttributeType() in heap.c, which has
the ability to recurse into composite data types.

+      When <literal>VIRTUAL</literal> is specified, the column will be
+      computed when it is read, and it will not occupy any storage.  When
+      <literal>STORED</literal> is specified, the column will be computed on
+      write and will be stored on disk.  <literal>VIRTUAL</literal> is the
+      default.
drop table if exists t5;
CREATE TABLE t5 (
a int,
b text storage extended collate "C"  GENERATED ALWAYS AS (a::text
collate case_insensitive) ,
d int DEFAULT 22
);
select reltoastrelid <> 0 as has_toast_table from pg_class where oid =
't5'::regclass;

if really no storage, should table t5 have an associated toast table or not?
also check ALTER TABLE variant:
alter table t5 alter b set storage extended;

Fixed. It does not trigger a toast table now.

Do we need to do something in ATExecSetStatistics for cases like:
ALTER TABLE t5 ALTER b SET STATISTICS 2000;
(b is a generated virtual column).
because of
examine_attribute
if (attr->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
return NULL;
i guess, this won't have a big impact.

This is also an error now.

There are some issues with changing virtual generated column type.
like:
drop table if exists another;
create table another (f4 int, f2 text, f3 text, f1 int GENERATED
ALWAYS AS (f4));
insert into another values(1, 'one', 'uno'), (2, 'two', 'due'),(3,
'three', 'tre');
alter table another
alter f1 type text using f2 || ' and ' || f3 || ' more';
table another;

or
alter table another
alter f1 type text using f2 || ' and ' || f3 || ' more',
drop column f1;
ERROR: column "f1" of relation "another" does not exist

These two command outputs seem not right.
the stored generated column which works as expected.

I noticed this is already buggy for stored generated columns. It should
prevent the use of the USING clause here. I'll propose a fix for that
in a separate thread. There might be further adjustments needed for
changing the types of virtual columns, but I'll come back to that after
the existing bug is fixed.

On 08.08.24 20:22, Dean Rasheed wrote:

Looking at the rewriter changes, it occurred to me that it could
perhaps be done more simply using ReplaceVarsFromTargetList() for each
RTE with virtual generated columns. That function already has the
required wholerow handling code, so there'd be less code duplication.

Hmm, I don't quite see how ReplaceVarsFromTargetList() could be used
here. It does have the wholerow logic that we need somehow, but other
than that it seems to target something different?

I think it might be better to do this from within fireRIRrules(), just
after RLS policies are applied, so it wouldn't need to worry about
CTEs and sublink subqueries. That would also make the
hasGeneratedVirtual flags unnecessary, since we'd already only be
doing the extra work for tables with virtual generated columns. That
would eliminate possible bugs caused by failing to set those flags.

Yes, ideally, we'd piggy-back this into fireRIRrules(). One thing I'm
missing is that if you're descending into subqueries, there is no link
to the upper levels' range tables, which we need to lookup the
pg_attribute entries of column referencing Vars. That's why there is
this whole custom walk with its own context data. Maybe there is a way
to do this already that I missed?

On Wed, 21 Aug 2024 at 08:00, Peter Eisentraut <peter@eisentraut.org> wrote:

On 08.08.24 20:22, Dean Rasheed wrote:

Looking at the rewriter changes, it occurred to me that it could
perhaps be done more simply using ReplaceVarsFromTargetList() for each
RTE with virtual generated columns. That function already has the
required wholerow handling code, so there'd be less code duplication.

Hmm, I don't quite see how ReplaceVarsFromTargetList() could be used
here. It does have the wholerow logic that we need somehow, but other
than that it seems to target something different?

Well what I was thinking was that (in fireRIRrules()'s final loop over
relations in the rtable), if the relation had any virtual generated
columns, you'd build a targetlist containing a TLE for each one,
containing the generated expression. Then you could just call
ReplaceVarsFromTargetList() to replace any Vars in the query with the
corresponding generated expressions. That takes care of descending
into subqueries, adjusting varlevelsup, and expanding wholerow Vars
that might refer to the generated expression.

I also have half an eye on how this patch will interact with my patch
to support RETURNING OLD/NEW values. If you use
ReplaceVarsFromTargetList(), it should just do the right thing for
RETURNING OLD/NEW generated expressions.

I think it might be better to do this from within fireRIRrules(), just
after RLS policies are applied, so it wouldn't need to worry about
CTEs and sublink subqueries. That would also make the
hasGeneratedVirtual flags unnecessary, since we'd already only be
doing the extra work for tables with virtual generated columns. That
would eliminate possible bugs caused by failing to set those flags.

Yes, ideally, we'd piggy-back this into fireRIRrules(). One thing I'm
missing is that if you're descending into subqueries, there is no link
to the upper levels' range tables, which we need to lookup the
pg_attribute entries of column referencing Vars. That's why there is
this whole custom walk with its own context data. Maybe there is a way
to do this already that I missed?

That link to the upper levels' range tables wouldn't be needed because
essentially using ReplaceVarsFromTargetList() flips the whole thing
round: instead of traversing the tree looking for Var nodes that need
to be replaced (possibly from upper query levels), you build a list of
replacement expressions to be applied and apply them from the top,
descending into subqueries as needed.

Another argument for doing it that way round is to not add too many
extra cycles to the processing of existing queries that don't
reference generated expressions. ISTM that this patch is potentially
adding quite a lot of additional overhead -- it looks like, for every
Var in the tree, it's calling get_attgenerated(), which involves a
syscache lookup to see if that column is a generated expression (which
most won't be). Ideally, we should be trying to do the minimum amount
of extra work in the common case where there are no generated
expressions.

Looking ahead, I can also imagine that one day we might want to
support subqueries in generated expressions. That would require
recursive processing of generated expressions in the generated
expression's subquery, as well as applying RLS policies to the new
relations pulled in, and checks to guard against infinite recursion.
fireRIRrules() already has the infrastructure to support all of that,
so that feels like a much more natural place to do this.

Regards,
Dean

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.

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?

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

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?

another variant:
CREATE TABLE list_parted (
a int NOT NULL,
b char(2) COLLATE "C",
c int GENERATED ALWAYS AS (22)
) PARTITION BY LIST (a);
CREATE TABLE parent (LIKE list_parted);
ALTER TABLE parent drop column c, add column c int GENERATED ALWAYS AS
(22) stored;
ALTER TABLE list_parted ATTACH PARTITION parent FOR VALUES IN (1);

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?

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.

The last issue in the previous thread [1]/messages/by-id/CACJufxEGPYtFe79hbsMeOBOivfNnPRsw7Gjvk67m1x2MQggyiQ@mail.gmail.com, ATPrepAlterColumnType
seems not addressed.

[1]: /messages/by-id/CACJufxEGPYtFe79hbsMeOBOivfNnPRsw7Gjvk67m1x2MQggyiQ@mail.gmail.com