Re: Virtual generated columns

On Thu, Aug 8, 2024 at 2:23 PM Peter Eisentraut <peter(at)eisentraut(dot)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;
---------------------------------------------------------------------------------