On Mon, Feb 25, 2019 at 09:46:35PM +0100, Peter Eisentraut wrote:
> The virtual generated column part is still a bit iffy. I'm still
> finding places here and there where virtual columns are not being
> expanded correctly. Maybe it needs more refactoring. One big unsolved
> issue is how the storage of such columns should work. Right now, they
> are stored as nulls. That works fine, but what I suppose we'd really
> want is to not store them at all. That, however, creates all kinds of
> complications in the planner if target lists have non-matching lengths
> or the resnos don't match up. I haven't figured out how to do this
> cleanly.
Hmm. Not storing virtual columns looks like the correct concept to
me instead of storing them as NULL.
> So I'm thinking if we can get agreement on the stored columns, I can cut
> out the virtual column stuff for PG12. That should be fairly easy.
The shape of what is used for stored columns looks fine to me.
+ if (attgenerated)
+ {
+ /*
+ * Generated column: Dropping anything that the generation expression
+ * refers to automatically drops the generated column.
+ */
+ recordDependencyOnSingleRelExpr(&colobject, expr, RelationGetRelid(rel),
+ DEPENDENCY_AUTO,
+ DEPENDENCY_AUTO, false);
+ }
A CCI is not necessary I think here, still the recent thread about
automatic dependencies with identity columns had a much similar
pattern...
+ else if (generated[0] == ATTRIBUTE_GENERATED_VIRTUAL)
+ default_str = psprintf("generated always as (%s)", PQgetvalue(res, i, attrdef_col));
Nit: I would add VIRTUAL instead of relying on the default option.
Another thing I was thinking about: could it be possible to add a
sanity check in sanity_check.sql so as a column more that one
field in attidentity, attgenerated and atthasdef set at the same time?
--
Michael