Обсуждение: Align tests for stored and virtual generated columns

Hi,

I noticed that the tests for virtual and stored generated columns
contain the following comment;

 -- keep these tests aligned with generated_stored.sql (or generated_virtual.sql)

However, it seems that some additional tests for virtual generated columns
disrupted that alignment, as they were not added to generated_stored.sql.

I've attached a patch to restore the alignment.

In addition, I moved the test confirming that user-defined types are not allowed
in virtual generated columns to the generated_virtual.sql-specific section,
since this test is not necessary for stored generated columns.

Regards,
Yugo Nagata

-- 
Yugo Nagata <nagata@sraoss.co.jp>

On Thu, Aug 7, 2025 at 7:52 PM Yugo Nagata <nagata@sraoss.co.jp> wrote:
> I noticed that the tests for virtual and stored generated columns
> contain the following comment;
>
>  -- keep these tests aligned with generated_stored.sql (or generated_virtual.sql)
>
> However, it seems that some additional tests for virtual generated columns
> disrupted that alignment, as they were not added to generated_stored.sql.
>
> I've attached a patch to restore the alignment.

Hello,

Thanks for this patch! Mutaama Maasha, Surya Poondla, and I reviewed
it together. Here are our thoughts:

We agree we should try to keep these tests in sync, so if they are
diverging we should bring them back into line.

The patch still applies. Tests pass.

Going through the tests made me curious about trying to break virtual
columns. I couldn't come up with anything, although one scenario that
doesn't seem be tested is changing the collation of a column used by a
generated column. For instance:

```
-- English vs Turkish upper/lower i:
create table t2 ( x text COLLATE "en-x-icu", y text COLLATE "tr-x-icu" );
insert into t2 values ('i', 'i'), ('I', 'I');
select upper(x), ascii(upper(x)), lower(x), ascii(lower(x)), upper(y),
ascii(upper(y)), lower(y), ascii(lower(y)) from t2;

create table t3 (
  x text collate "en-x-icu",
  lx text collate "en-x-icu" generated always as (lower(x)),
  ux text collate "en-x-icu" generated always as (upper(x)),
  y text collate "tr-x-icu",
  ly text collate "tr-x-icu" generated always as (lower(y)),
  uy text collate "tr-x-icu" generated always as (upper(y))
);
insert into t3 (x, y) values ('i', 'i'), ('I', 'I');
alter table t3 add constraint x check (ascii(lx) < 128 and ascii(ux) < 128);
alter table t3 alter column x type text collate "tr-x-icu";
ERROR:  cannot alter type of a column used by a generated column
DETAIL:  Column "x" is used by generated column "lx".
```

Perhaps we could add a test like that? (We do have a test for changing
the *type* of a column used by a generated column though.)

Is there a way we can make it easier to compare the two test scripts
for differences? Could we write a meta-test that compares them for
differences (in the spirit of `opr_sanity.sql`)? I experimented with
using psql variables to limit `STORED` vs `VIRTUAL` to only the top of
each SQL file. Then I could easily diff the two files and see how
diverged they were. Attached is a patch to do this and the results of
my diff (after applying the author's patch). It seems like there are
still a few trivial discrepancies that we could clean up.

To call out one less-trivial discrepancy:

```
--- sql/generated_stored.sql    2025-09-21 19:52:14.554930323 -0700
+++ sql/generated_virtual.sql   2025-09-21 19:52:21.447016340 -0700
...
-INSERT INTO gtest12 VALUES (3, 30), (4, 40);  -- currently not
allowed because of function permissions, should
 arguably be allowed
-SELECT a, c FROM gtest12;  -- allowed (does not actually invoke the function)
+--INSERT INTO gtest12 VALUES (3, 30), (4, 40);  -- allowed (does not
actually invoke the function)
+--SELECT a, c FROM gtest12;  -- currently not allowed because of
function permissions, should arguably be allowed
```

Why are the VIRTUAL tests commented out? The explanatory comments
suggest they should have opposite results from the STORED tests, which
makes sense, but shouldn't we be running them?

Similarly we noticed that the test for expansion of virtual generated
columns is not applied to stored columns. Is there a reason why not?

We found a couple places where this patch adds new test tables whose
numbering is out of sequence compared to the rest of the file. For
instance:

> @@ -806,6 +803,9 @@ SELECT attrelid, attname, attgenerated FROM pg_attribute WHERE attgenerated NOT
>  -- these tests are specific to generated_virtual.sql
>  --
>
> +-- using user-defined type not yet supported
> +CREATE TABLE gtest24xxx (a gtestdomain1, b gtestdomain1, c int GENERATED ALWAYS AS (greatest(a, b)) VIRTUAL);  --
error
> +
>  create table gtest32 (
>    a int primary key,
>    b int generated always as (a * 2),
> --
> 2.43.0

Why add gtest24xxx in between gtest28b and gtest32? Maybe it should be
gtest30 or 31?

Also here:

> +INSERT INTO gtest21b (a) VALUES (2), (0);  -- violates constraint
> +INSERT INTO gtest21b (a) VALUES (NULL);  -- error
>  ALTER TABLE gtest21b ALTER COLUMN b DROP NOT NULL;
>  INSERT INTO gtest21b (a) VALUES (0);  -- ok now
>
> +-- not-null constraint with partitioned table
> +CREATE TABLE gtestnn_parent (
> +    f1 int,
> +    f2 bigint,
> +    f3 bigint GENERATED ALWAYS AS (nullif(f1, 1) + nullif(f2, 10)) STORED NOT NULL
> +) PARTITION BY RANGE (f1);
> +CREATE TABLE gtestnn_child PARTITION OF gtestnn_parent FOR VALUES FROM (1) TO (5);
> +CREATE TABLE gtestnn_childdef PARTITION OF gtestnn_parent default;

Should gtestnn_parent have a number? It is between gtest21b and
gtest22a. Perhaps gtest21nn_parent? This is a tougher choice since 21
and 22 are taken.

Yours,

--
Paul              ~{:-)
pj@illuminatedcomputing.com

On 08.08.25 04:51, Yugo Nagata wrote:
> I noticed that the tests for virtual and stored generated columns
> contain the following comment;
> 
>   -- keep these tests aligned with generated_stored.sql (or generated_virtual.sql)
> 
> However, it seems that some additional tests for virtual generated columns
> disrupted that alignment, as they were not added to generated_stored.sql.
> 
> I've attached a patch to restore the alignment.
> 
> In addition, I moved the test confirming that user-defined types are not allowed
> in virtual generated columns to the generated_virtual.sql-specific section,
> since this test is not necessary for stored generated columns.

Committed, thanks.

On 30.09.25 20:01, Paul A Jungwirth wrote:
> Going through the tests made me curious about trying to break virtual
> columns. I couldn't come up with anything, although one scenario that
> doesn't seem be tested is changing the collation of a column used by a
> generated column. For instance:
> 
> ```
> -- English vs Turkish upper/lower i:
> create table t2 ( x text COLLATE "en-x-icu", y text COLLATE "tr-x-icu" );
> insert into t2 values ('i', 'i'), ('I', 'I');
> select upper(x), ascii(upper(x)), lower(x), ascii(lower(x)), upper(y),
> ascii(upper(y)), lower(y), ascii(lower(y)) from t2;
> 
> create table t3 (
>    x text collate "en-x-icu",
>    lx text collate "en-x-icu" generated always as (lower(x)),
>    ux text collate "en-x-icu" generated always as (upper(x)),
>    y text collate "tr-x-icu",
>    ly text collate "tr-x-icu" generated always as (lower(y)),
>    uy text collate "tr-x-icu" generated always as (upper(y))
> );
> insert into t3 (x, y) values ('i', 'i'), ('I', 'I');
> alter table t3 add constraint x check (ascii(lx) < 128 and ascii(ux) < 128);
> alter table t3 alter column x type text collate "tr-x-icu";
> ERROR:  cannot alter type of a column used by a generated column
> DETAIL:  Column "x" is used by generated column "lx".
> ```
> 
> Perhaps we could add a test like that? (We do have a test for changing
> the *type* of a column used by a generated column though.)

The example you show produces the error

ERROR:  cannot alter type of a column used by a generated column

which suggests that it is internally the same thing, so it seems another 
test wouldn't add any new coverage.

> Is there a way we can make it easier to compare the two test scripts
> for differences? Could we write a meta-test that compares them for
> differences (in the spirit of `opr_sanity.sql`)? I experimented with
> using psql variables to limit `STORED` vs `VIRTUAL` to only the top of
> each SQL file. Then I could easily diff the two files and see how
> diverged they were. Attached is a patch to do this and the results of
> my diff (after applying the author's patch). It seems like there are
> still a few trivial discrepancies that we could clean up.

I thought about something like that initially, too, but then decided 
against it because it would make each test individually harder to 
understand and manage.

> To call out one less-trivial discrepancy:
> 
> ```
> --- sql/generated_stored.sql    2025-09-21 19:52:14.554930323 -0700
> +++ sql/generated_virtual.sql   2025-09-21 19:52:21.447016340 -0700
> ...
> -INSERT INTO gtest12 VALUES (3, 30), (4, 40);  -- currently not
> allowed because of function permissions, should
>   arguably be allowed
> -SELECT a, c FROM gtest12;  -- allowed (does not actually invoke the function)
> +--INSERT INTO gtest12 VALUES (3, 30), (4, 40);  -- allowed (does not
> actually invoke the function)
> +--SELECT a, c FROM gtest12;  -- currently not allowed because of
> function permissions, should arguably be allowed
> ```
> 
> Why are the VIRTUAL tests commented out? The explanatory comments
> suggest they should have opposite results from the STORED tests, which
> makes sense, but shouldn't we be running them?

My recollection is that some of these tests are commented out because 
running them would produce some behavior that would affect subsequent 
tests (for example, adding or removing rows that they shouldn't), and so 
you would have to do some extra work to undo some of that to make this 
all work smoothly.  (Also, in some cases it would be a waste of time to 
run large blocks of tests for unsupported features, so all but the first 
few test statements are commented out.)

> Similarly we noticed that the test for expansion of virtual generated
> columns is not applied to stored columns. Is there a reason why not?

I guess this tests something that doesn't really apply to stored 
columns.  But maybe this could be double checked.

> We found a couple places where this patch adds new test tables whose
> numbering is out of sequence compared to the rest of the file.

Yeah, this whole test numbering turned into a disaster pretty early on. 
   At this point, I consider these test files to be a monument against 
doing that again.  In later feature work, I started to name test tables 
by random numbers. ;-)