Date: Mon, 19 Nov 2018 15:54:31 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>,
	Michael Paquier <michael.paquier@gmail.com>,
	Jaime Casanova <jaime.casanova@2ndquadrant.com>
Subject: Re: [HACKERS] generated columns
Message-ID: <20181119185431.7wfp6rmxtqr2ds3p@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <79e4de23-52e3-2c45-9ba0-5f6932f590aa@2ndquadrant.com>
User-Agent: NeoMutt/20180716
Precedence: bulk

Disclaimer:  I had never seen this patch before.  I did not participate
in this feature design.  I did not discuss this review with the author
or anybody in 2ndQuadrant.  I do not have any particular affective bonds
with its author.  I did not receive payment nor goods in exchange for
this review.  I encourage others to review this patch, and all other
patches in the current commitfest and all future commitfests.


Now that the TupleTableSlot work has landed, the API of
ExecComputeStoredGenerated is clearly inadequate.  This should be
adjusted to work with the slot only, and not generate a heap tuple at
all -- if the calling code needs the heap tuple, have that code generate
that from the slot.  (Example problem: ExecConstraints runs using the
slot, not the heap tuple.)

The pg_dump tests verify a virtual generated column, but not a virtual
stored column.  It'd be good to have one for the latter.  The tables in
new test "generated" appear not to be dropped, which is good to test
pg_upgrade as well as pg_dump; I'd add a comment that this is on
purpose, lest someone else add DROP lines there later.  I think some
tests for logical replication would be good as well.

It's unclear why you made generated columns on partitions unsupported.
I'd fix the limitation if possible, but if not, at least document it.
(I particularly notice that partition key is marked as unsupported in
the regression test.  Consider partitioning on a SERIAL column, this is
clearly something that users will expect to work.)

About your catalog representation question:

On 2018-Oct-30, Peter Eisentraut wrote:

> 1. (current implementation) New column attgenerated contains 's' for
> STORED, 'v' for VIRTUAL, '\0' for nothing.  atthasdef means "there is
> something in pg_attrdef for this column".  So a generated column would
> have atthasdef = true, and attgenerated = s/v.  A traditional default
> would have atthasdef = true and attgenerated = '\0'.  The advantage is
> that this is easiest to implement and the internal representation is the
> most useful and straightforward.  The disadvantage is that old client
> code that wants to detect whether a column has a default would need to
> be changed (otherwise it would interpret a generated column as having a
> default value instead).

I think this is a reasonable implementation.  Client code that is
confused by this will obviously have to be updated, but if it isn't, the
bug is not serious.  I would clearly not go to the extreme trouble that
#2 poses just to avoid this problem.

That said, I think your "radical alternative" #3 is better.  Maybe we
want to avoid multiple compatibility breaks, so we'd go with 3 for pg12
instead of doing 1 now and changing it again later.

Like Robert, I would use something other than '\0' anyhow.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services