On 12/13/17 22:39, Tom Lane wrote:
> Now that I look more closely at this patch, isn't it fixing things
> in the wrong place? Why is it that COPY needs to know about this,
> rather than build_column_default()? Aren't we going to have to
> fix every other caller of build_column_default()?
Yeah, that seems to be a problem. Attached is a patch that puts the
logic into build_column_default() and adds a few more tests covering
other omissions.
One problem is that this breaks the case ALTER TABLE ... ADD COLUMN ...
IDENTITY, because the sequence ownership isn't registered until after
the ALTER TABLE command. (This only worked for empty tables. For
pre-populated tables, it failed because of the above omission.) The
second patch tries to fix that. Needs more thought and documentation,
but it's an idea.
> For that matter, should build_column_default() know about it explicitly
> either? Why aren't we implementing IDENTITY by storing an appropriate
> default expression in the catalogs?
Initial versions were coded that way, but it was pretty messy and buggy
because you have to touch a lot of places to teach them the difference
between a real default expression and a synthetic one. Another problem
is that the NextValueExpr has special permission behavior that is not
really representable by a normal expression, thus introducing more
special cases, and possibly permission or security issues.
Obviously, it would have helped in the above case. But it was really messy.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
