Re: [HACKERS] generated columns - Mailing list pgsql-hackers

On Sun, Jan 13, 2019 at 03:31:23PM +0100, Pavel Stehule wrote:
> ne 13. 1. 2019 v 10:43 odesílatel Peter Eisentraut <
> peter.eisentraut@2ndquadrant.com> napsal:
>> See here:
>>
>> https://www.postgresql.org/message-id/b5c27634-1d44-feba-7494-ce5a31f914ca@2ndquadrant.com
>
> I understand - it is logical. But it is sad, so this feature is not
> complete. The benefit is not too big - against functional indexes or views.
> But it can be first step.

I wouldn't say that.  Volatibility restrictions based on immutable
functions apply to many concepts similar like expression pushdowns to
make for deterministic results.  The SQL spec takes things on the safe
side.

>> Ah, the volatility checking needs some improvements.  I'll address that
>> in the next patch version.
>
> ok

The same problem happens for stored and virtual columns.

The latest patch has a small header conflict at the top of
rewriteHandler.c which is simple enough to fix.

It would be nice to add a test with composite types, say something
like:
=# create type double_int as (a int, b int);
CREATE TYPE
=# create table double_tab (a int,
  b double_int GENERATED ALWAYS AS ((a * 2, a * 3)) stored,
  c double_int GENERATED ALWAYS AS ((a * 4, a * 5)) virtual);
CREATE TABLE
=# insert into double_tab values (1), (6);
INSERT 0 2
=# select * from double_tab ;
 a |    b    |    c
---+---------+---------
 1 | (2,3)   | (4,5)
 6 | (12,18) | (24,30)
(2 rows)

Glad to see that typed tables are handled and forbidden, and the
trigger definition looks sane to me.

+-- partitioned table
+CREATE TABLE gtest_parent (f1 date NOT NULL, f2 bigint, f3 bigint
GENERATED ALWAYS AS (f2 * 2)) PARTITION BY RANGE (f1);
+CREATE TABLE gtest_child PARTITION OF gtest_parent FOR VALUES FROM
('2016-07-01') TO ('2016-08-01');
+INSERT INTO gtest_parent (f1, f2) VALUES ('2016-07-15', 1);
In my experience, having tests which handle multiple layers of
partitions is never a bad thing.

+    /*
+     * Changing the type of a column that is used by a
+     * generated column is not allowed by SQL standard.
+     * It might be doable with some thinking and effort.
Just mentioning the part about the SQL standard seems fine to me.

+   bool        has_generated_stored;
+   bool        has_generated_virtual;
 } TupleConstr;
Could have been more simple to use a char as representation here.

Using NULL as generation expression results in a crash when selecting
the relation created:
=# CREATE TABLE gtest_err_1 (a int PRIMARY KEY, b int GENERATED ALWAYS
AS (NULL));
CREATE TABLE
=# select * from gtest_err_1;
server closed the connection unexpectedly

=# CREATE TABLE gtest_err_2 (a int PRIMARY KEY, b int NOT NULL
GENERATED ALWAYS AS (NULL));
CREATE TABLE
A NOT NULL column can use NULL as generated result :)

+   The view <literal>column_column_usage</literal> identifies all
generated
"column_column_usage" is redundant.  Could it be possible to come up
with a better name?

When testing a bulk INSERT into a table which has a stored generated
column, memory keeps growing in size linearly, which does not seem
normal to me.  If inserting more tuples than what I tested (I stopped
at 10M because of lack of time), it seems to me that this could result
in OOMs.  I would have expected the memory usage to be steady.

+    /* ignore virtual generated columns; they are always null here */
+    if (att->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)
+        continue;
Looking for an assertion or a sanity check of some kind?

+       if (pstate->p_expr_kind == EXPR_KIND_GENERATED_COLUMN &&
+           attnum < InvalidAttrNumber && attnum != TableOidAttributeNumber)
+           ereport(ERROR,
+                   (errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
+                    errmsg("cannot use system column \"%s\" in column generation expression",
+                           colname),
+                    parser_errposition(pstate, location)));
There is a test using xmon, you may want one with tableoid.

+/*
+ * Thin wrapper around libpq to obtain server version.
+ */
+static int
+libpqrcv_server_version(WalReceiverConn *conn)
This should be introduced in separate patch in my opinion (needed
afterwards for logirep).

I have not gone through the PL part of the changes yet, except
plpgsql.

What about the catalog representation of attgenerated?  Would it merge
with attidentity & co?  Or not?
--
Michael