[bug+patch] Inserting DEFAULT into generated columns from VALUES RTE
Hello!
According to the docs[1]https://www.postgresql.org/docs/devel/ddl-generated-columns.html, one may use DEFAULT keyword while inserting
into generated columns (stored and identity). However, currently it
works only for a single VALUES sublist with DEFAULT for a generated column
but not for the case when VALUES RTE is used. This is not being tested
and it is broken.
I am attaching two patches. One for tests and another one with the
proposed changes based on ideas from Andrew on IRC. So if all good there
goes the credit where credit is due. If patch is no good, then it is
likely my misunderstanding how to put words into code :-)
This is my only second patch to PostgreSQL (the first one was rejected)
so don't be too harsh :-) It may not be perfect but I am open for a
feedback and this is just to get the ball rolling and to let the
community know about this issue.
Before you ask why would I want to insert DEFAULTs ... well, there are
ORMs[2]https://github.com/rails/rails/pull/39368#issuecomment-670351379 that still need to be patched and current situation contradicts
documentation[1]https://www.postgresql.org/docs/devel/ddl-generated-columns.html.
Footnotes:
[1]: https://www.postgresql.org/docs/devel/ddl-generated-columns.html
[2]: https://github.com/rails/rails/pull/39368#issuecomment-670351379
--
Mikhail
Attachments:
0001-Test-DEFAULT-in-VALUES-RTE-for-generated-columns.patchtext/x-patchDownload
From d606c4f952a6080dff6fb621ea034bfce2865f7b Mon Sep 17 00:00:00 2001
From: Mikhail Titov <mlt@gmx.us>
Date: Wed, 12 Aug 2020 22:42:37 -0500
Subject: [PATCH 1/2] Test DEFAULT in VALUES RTE for generated columns
---
src/test/regress/expected/generated.out | 9 +++++++++
src/test/regress/expected/identity.out | 13 +++++++++----
src/test/regress/sql/generated.sql | 4 ++++
src/test/regress/sql/identity.sql | 1 +
4 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
index 7ccc3c65ed..31525ef2a6 100644
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -90,6 +90,15 @@ CREATE TABLE gtest_err_8 (a int PRIMARY KEY, b int GENERATED BY DEFAULT AS (a *
ERROR: for a generated column, GENERATED ALWAYS must be specified
LINE 1: ...E gtest_err_8 (a int PRIMARY KEY, b int GENERATED BY DEFAULT...
^
+-- test VALUES RTE with defaults
+INSERT INTO gtest0 VALUES (1, DEFAULT), (2, DEFAULT);
+SELECT * FROM gtest0;
+ a | b
+---+----
+ 1 | 55
+ 2 | 55
+(2 rows)
+
INSERT INTO gtest1 VALUES (1);
INSERT INTO gtest1 VALUES (2, DEFAULT);
INSERT INTO gtest1 VALUES (3, 33); -- error
diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out
index 7ac9df767f..ca27b7ff73 100644
--- a/src/test/regress/expected/identity.out
+++ b/src/test/regress/expected/identity.out
@@ -76,6 +76,7 @@ INSERT INTO itest3 DEFAULT VALUES;
INSERT INTO itest3 DEFAULT VALUES;
INSERT INTO itest4 DEFAULT VALUES;
INSERT INTO itest4 DEFAULT VALUES;
+INSERT INTO itest4 (a, b) VALUES (DEFAULT, 'foo'), (DEFAULT, 'bar');
SELECT * FROM itest1;
a | b
---+---
@@ -99,10 +100,12 @@ SELECT * FROM itest3;
SELECT * FROM itest4;
a | b
----+---
+---+-----
1 |
2 |
-(2 rows)
+ 3 | foo
+ 4 | bar
+(4 rows)
-- VALUES RTEs
INSERT INTO itest3 VALUES (DEFAULT, 'a');
@@ -211,11 +214,13 @@ ALTER TABLE itest4 ALTER COLUMN a DROP NOT NULL;
INSERT INTO itest4 DEFAULT VALUES;
SELECT * FROM itest4;
a | b
----+---
+---+-----
1 |
2 |
+ 3 | foo
+ 4 | bar
|
-(3 rows)
+(5 rows)
-- check that sequence is removed
SELECT sequence_name FROM itest4_a_seq;
diff --git a/src/test/regress/sql/generated.sql b/src/test/regress/sql/generated.sql
index 4cff1279c7..18bb1d3c78 100644
--- a/src/test/regress/sql/generated.sql
+++ b/src/test/regress/sql/generated.sql
@@ -40,6 +40,10 @@ CREATE TABLE gtest_err_7d (a int PRIMARY KEY, b int GENERATED ALWAYS AS (generat
-- GENERATED BY DEFAULT not allowed
CREATE TABLE gtest_err_8 (a int PRIMARY KEY, b int GENERATED BY DEFAULT AS (a * 2) STORED);
+-- test VALUES RTE with defaults
+INSERT INTO gtest0 VALUES (1, DEFAULT), (2, DEFAULT);
+SELECT * FROM gtest0;
+
INSERT INTO gtest1 VALUES (1);
INSERT INTO gtest1 VALUES (2, DEFAULT);
INSERT INTO gtest1 VALUES (3, 33); -- error
diff --git a/src/test/regress/sql/identity.sql b/src/test/regress/sql/identity.sql
index 1bf2a976eb..b3d99583c2 100644
--- a/src/test/regress/sql/identity.sql
+++ b/src/test/regress/sql/identity.sql
@@ -47,6 +47,7 @@ INSERT INTO itest3 DEFAULT VALUES;
INSERT INTO itest3 DEFAULT VALUES;
INSERT INTO itest4 DEFAULT VALUES;
INSERT INTO itest4 DEFAULT VALUES;
+INSERT INTO itest4 (a, b) VALUES (DEFAULT, 'foo'), (DEFAULT, 'bar');
SELECT * FROM itest1;
SELECT * FROM itest2;
--
2.28.0.windows.1
0002-Allow-DEFAULT-in-VALUES-RTE-for-generated-columns.patchtext/x-patchDownload
From 7a187f698d31638c65da10a71e97060793a29c7f Mon Sep 17 00:00:00 2001
From: Mikhail Titov <mlt@gmx.us>
Date: Wed, 12 Aug 2020 21:07:22 -0500
Subject: [PATCH 2/2] Allow DEFAULT in VALUES RTE for generated columns
---
src/backend/parser/parse_relation.c | 50 ++++++++++++++++++++++------
src/backend/rewrite/rewriteHandler.c | 13 +++++++-
2 files changed, 51 insertions(+), 12 deletions(-)
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index b875a50646..da26863d22 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -2981,19 +2981,47 @@ expandNSItemVars(ParseNamespaceItem *nsitem,
if (colname[0])
{
Var *var;
+ ListCell *c;
+ Node *std = NULL;
+ bool all_default = true;
Assert(nscol->p_varno > 0);
- var = makeVar(nscol->p_varno,
- nscol->p_varattno,
- nscol->p_vartype,
- nscol->p_vartypmod,
- nscol->p_varcollid,
- sublevels_up);
- /* makeVar doesn't offer parameters for these, so set by hand: */
- var->varnosyn = nscol->p_varnosyn;
- var->varattnosyn = nscol->p_varattnosyn;
- var->location = location;
- result = lappend(result, var);
+ foreach(c, nsitem->p_rte->values_lists)
+ {
+ List *row = (List*) lfirst(c);
+ std = list_nth_node(Node, row, colindex);
+ if (!IsA(std, SetToDefault))
+ {
+ all_default = false;
+ break;
+ }
+ }
+
+ if (all_default && std)
+ {
+ /*
+ * If all nodes for the column are SetToDefault, keep the marker
+ * for later INSERT VALUES list rewriting where we remove
+ * TLE and force NULL constant values node.
+ * We cannot just remove values column
+ * as attribute numbering will be off.
+ */
+ result = lappend(result, std);
+ }
+ else
+ {
+ var = makeVar(nscol->p_varno,
+ nscol->p_varattno,
+ nscol->p_vartype,
+ nscol->p_vartypmod,
+ nscol->p_varcollid,
+ sublevels_up);
+ /* makeVar doesn't offer parameters for these, so set by hand: */
+ var->varnosyn = nscol->p_varnosyn;
+ var->varattnosyn = nscol->p_varattnosyn;
+ var->location = location;
+ result = lappend(result, var);
+ }
if (colnames)
*colnames = lappend(*colnames, colnameval);
}
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index fe777c3103..f885fe7405 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1373,8 +1373,19 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
Form_pg_attribute att_tup;
Node *new_expr;
+ /*
+ * TLE was likely already removed for this generated column.
+ * We do not have a good way to check it now,
+ * but attrno should not be 0 anyway.
+ */
if (attrno == 0)
- elog(ERROR, "cannot set value in column %d to DEFAULT", i);
+ {
+ SetToDefault *std = (SetToDefault*) col;
+ new_expr = (Node*) makeNullConst(std->typeId, std->typeMod, std->collation);
+ newList = lappend(newList, new_expr);
+ continue;
+ }
+
att_tup = TupleDescAttr(target_relation->rd_att, attrno - 1);
if (!force_nulls && !att_tup->attisdropped)
--
2.28.0.windows.1
Hi
čt 13. 8. 2020 v 6:31 odesílatel Mikhail Titov <mlt@gmx.us> napsal:
Hello!
According to the docs[1], one may use DEFAULT keyword while inserting
into generated columns (stored and identity). However, currently it
works only for a single VALUES sublist with DEFAULT for a generated column
but not for the case when VALUES RTE is used. This is not being tested
and it is broken.I am attaching two patches. One for tests and another one with the
proposed changes based on ideas from Andrew on IRC. So if all good there
goes the credit where credit is due. If patch is no good, then it is
likely my misunderstanding how to put words into code :-)This is my only second patch to PostgreSQL (the first one was rejected)
so don't be too harsh :-) It may not be perfect but I am open for a
feedback and this is just to get the ball rolling and to let the
community know about this issue.Before you ask why would I want to insert DEFAULTs ... well, there are
ORMs[2] that still need to be patched and current situation contradicts
documentation[1].
please, assign your patch to commitfest application
https://commitfest.postgresql.org/29/
Regards
Pavel
Show quoted text
Footnotes:
[1] https://www.postgresql.org/docs/devel/ddl-generated-columns.html[2] https://github.com/rails/rails/pull/39368#issuecomment-670351379
--
Mikhail
Previously submitted patch got somehow trailing spaces mangled on the
way out. This is an attempt to use application/octet-stream MIME instead
of text/x-patch to preserve those for regression tests.
On Thu, Aug 13, 2020 at 12:11 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
please, assign your patch to commitfest application
Here is the backlink https://commitfest.postgresql.org/29/2681/
--
Mikhail
Attachments:
v2-0001-Test-DEFAULT-in-VALUES-RTE-for-generated-columns.patchapplication/octet-streamDownload
From aa388025468b1c5ac7c2f3f5fc50ee09e63d5982 Mon Sep 17 00:00:00 2001
From: Mikhail Titov <mlt@gmx.us>
Date: Wed, 12 Aug 2020 22:42:37 -0500
Subject: [PATCH v2 1/2] Test DEFAULT in VALUES RTE for generated columns
---
src/test/regress/expected/generated.out | 9 +++++++++
src/test/regress/expected/identity.out | 3 +--
src/test/regress/sql/generated.sql | 4 ++++
src/test/regress/sql/identity.sql | 3 +--
4 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
index 7ccc3c65ed..31525ef2a6 100644
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -90,6 +90,15 @@ CREATE TABLE gtest_err_8 (a int PRIMARY KEY, b int GENERATED BY DEFAULT AS (a *
ERROR: for a generated column, GENERATED ALWAYS must be specified
LINE 1: ...E gtest_err_8 (a int PRIMARY KEY, b int GENERATED BY DEFAULT...
^
+-- test VALUES RTE with defaults
+INSERT INTO gtest0 VALUES (1, DEFAULT), (2, DEFAULT);
+SELECT * FROM gtest0;
+ a | b
+---+----
+ 1 | 55
+ 2 | 55
+(2 rows)
+
INSERT INTO gtest1 VALUES (1);
INSERT INTO gtest1 VALUES (2, DEFAULT);
INSERT INTO gtest1 VALUES (3, 33); -- error
diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out
index 7ac9df767f..b013a7c03a 100644
--- a/src/test/regress/expected/identity.out
+++ b/src/test/regress/expected/identity.out
@@ -74,8 +74,7 @@ INSERT INTO itest2 DEFAULT VALUES;
INSERT INTO itest2 DEFAULT VALUES;
INSERT INTO itest3 DEFAULT VALUES;
INSERT INTO itest3 DEFAULT VALUES;
-INSERT INTO itest4 DEFAULT VALUES;
-INSERT INTO itest4 DEFAULT VALUES;
+INSERT INTO itest4 VALUES (DEFAULT, DEFAULT), (DEFAULT, DEFAULT);
SELECT * FROM itest1;
a | b
---+---
diff --git a/src/test/regress/sql/generated.sql b/src/test/regress/sql/generated.sql
index 4cff1279c7..18bb1d3c78 100644
--- a/src/test/regress/sql/generated.sql
+++ b/src/test/regress/sql/generated.sql
@@ -40,6 +40,10 @@ CREATE TABLE gtest_err_7d (a int PRIMARY KEY, b int GENERATED ALWAYS AS (generat
-- GENERATED BY DEFAULT not allowed
CREATE TABLE gtest_err_8 (a int PRIMARY KEY, b int GENERATED BY DEFAULT AS (a * 2) STORED);
+-- test VALUES RTE with defaults
+INSERT INTO gtest0 VALUES (1, DEFAULT), (2, DEFAULT);
+SELECT * FROM gtest0;
+
INSERT INTO gtest1 VALUES (1);
INSERT INTO gtest1 VALUES (2, DEFAULT);
INSERT INTO gtest1 VALUES (3, 33); -- error
diff --git a/src/test/regress/sql/identity.sql b/src/test/regress/sql/identity.sql
index 1bf2a976eb..734014dc1c 100644
--- a/src/test/regress/sql/identity.sql
+++ b/src/test/regress/sql/identity.sql
@@ -45,8 +45,7 @@ INSERT INTO itest2 DEFAULT VALUES;
INSERT INTO itest2 DEFAULT VALUES;
INSERT INTO itest3 DEFAULT VALUES;
INSERT INTO itest3 DEFAULT VALUES;
-INSERT INTO itest4 DEFAULT VALUES;
-INSERT INTO itest4 DEFAULT VALUES;
+INSERT INTO itest4 VALUES (DEFAULT, DEFAULT), (DEFAULT, DEFAULT);
SELECT * FROM itest1;
SELECT * FROM itest2;
--
2.28.0.windows.1
v2-0002-Allow-DEFAULT-in-VALUES-RTE-for-generated-columns.patchapplication/octet-streamDownload
From a745c91edbb0b59fddb4e7efb135f997c7830a2b Mon Sep 17 00:00:00 2001
From: Mikhail Titov <mlt@gmx.us>
Date: Wed, 12 Aug 2020 21:07:22 -0500
Subject: [PATCH v2 2/2] Allow DEFAULT in VALUES RTE for generated columns
Use T_SetToDefault in TLE instead of T_Var during parse analysis
as a marker for all DEFAULTs column. Let existing code to get rid of
that SetToDefault from TLE in INSERT rewriter, then force NULLs
during RTE column rewriting when attribute number was not set
as it normally would with Var.
Original patch was sent as text/x-patch and suffered whitespace mangling.
https://www.postgresql.org/message-id/flat/9q0sgcr416t.fsf@gmx.us
---
src/backend/parser/parse_relation.c | 50 ++++++++++++++++++++++------
src/backend/rewrite/rewriteHandler.c | 13 +++++++-
2 files changed, 51 insertions(+), 12 deletions(-)
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index b875a50646..baf05e2185 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -2981,19 +2981,47 @@ expandNSItemVars(ParseNamespaceItem *nsitem,
if (colname[0])
{
Var *var;
+ ListCell *lc2;
+ Node *std = NULL;
+ bool all_default = true;
Assert(nscol->p_varno > 0);
- var = makeVar(nscol->p_varno,
- nscol->p_varattno,
- nscol->p_vartype,
- nscol->p_vartypmod,
- nscol->p_varcollid,
- sublevels_up);
- /* makeVar doesn't offer parameters for these, so set by hand: */
- var->varnosyn = nscol->p_varnosyn;
- var->varattnosyn = nscol->p_varattnosyn;
- var->location = location;
- result = lappend(result, var);
+ foreach(lc2, nsitem->p_rte->values_lists)
+ {
+ List *row = (List*) lfirst(lc2);
+ std = list_nth_node(Node, row, colindex);
+ if (!IsA(std, SetToDefault))
+ {
+ all_default = false;
+ break;
+ }
+ }
+
+ if (all_default && std)
+ {
+ /*
+ * If all nodes for the column are SetToDefault, keep the marker
+ * for later INSERT VALUES list rewriting where we remove
+ * TLE and force NULL constant values node while rewriting VALUES RTE.
+ * We cannot just remove values column
+ * as attribute numbering will be off.
+ */
+ result = lappend(result, std);
+ }
+ else
+ {
+ var = makeVar(nscol->p_varno,
+ nscol->p_varattno,
+ nscol->p_vartype,
+ nscol->p_vartypmod,
+ nscol->p_varcollid,
+ sublevels_up);
+ /* makeVar doesn't offer parameters for these, so set by hand: */
+ var->varnosyn = nscol->p_varnosyn;
+ var->varattnosyn = nscol->p_varattnosyn;
+ var->location = location;
+ result = lappend(result, var);
+ }
if (colnames)
*colnames = lappend(*colnames, colnameval);
}
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index fe777c3103..f885fe7405 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1373,8 +1373,19 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
Form_pg_attribute att_tup;
Node *new_expr;
+ /*
+ * TLE was likely already removed for this generated column.
+ * We do not have a good way to check it now,
+ * but attrno should not be 0 anyway.
+ */
if (attrno == 0)
- elog(ERROR, "cannot set value in column %d to DEFAULT", i);
+ {
+ SetToDefault *std = (SetToDefault*) col;
+ new_expr = (Node*) makeNullConst(std->typeId, std->typeMod, std->collation);
+ newList = lappend(newList, new_expr);
+ continue;
+ }
+
att_tup = TupleDescAttr(target_relation->rd_att, attrno - 1);
if (!force_nulls && !att_tup->attisdropped)
--
2.28.0.windows.1
Mikhail Titov <mlt@gmx.us> writes:
Previously submitted patch got somehow trailing spaces mangled on the
way out. This is an attempt to use application/octet-stream MIME instead
of text/x-patch to preserve those for regression tests.
I took a quick look through this. I agree with the general idea of
detecting cases where all of the entries in a VALUES column are DEFAULT,
but the implementation needs work.
The cfbot reports that it doesn't compile [1]https://travis-ci.org/github/postgresql-cfbot/postgresql/builds/724543451:
parse_relation.c: In function ‘expandNSItemVars’:
parse_relation.c:2992:34: error: ‘T_Node’ undeclared (first use in this function)
std = list_nth_node(Node, row, colindex);
^
I suspect this indicates that you did not use --enable-cassert in your own
testing, which is usually a bad idea; that enables a lot of checks that
you really want to have active for development purposes.
Hacking expandNSItemVars() for this purpose is an extremely bad idea.
The API spec for that is
* Produce a list of Vars, and optionally a list of column names,
* for the non-dropped columns of the nsitem.
This patch breaks that specification, and in turn breaks callers that
expect it to be adhered to. I see at least one caller that will suffer
assertion failures because of that, which reinforces my suspicion that
you did not test with assertions on.
I think you'd be better off to make transformInsertStmt(), specifically
its multi-VALUES-rows code path, check for all-DEFAULT columns and adjust
the tlist itself. Doing it there might be a good bit less inefficient
for very long VALUES lists, too, which is a case that we do worry about.
Since that's already iterating through the sub-lists, you could track
whether all rows seen so far contain SetToDefault in each column position,
and avoid extra scans of the sublists. (A BitmapSet might be a convenient
representation of that, though you could also use a bool array I suppose.)
I do not care for what you did in rewriteValuesRTE() either: just removing
a sanity check isn't OK, unless you've done something to make the sanity
check unnecessary which you surely have not. Perhaps you could extend
the initial scan of the tlist (lines 1294-1310) to notice SetToDefault
nodes as well as Var nodes and keep track of which columns have those.
Then you could cross-check that one or the other case applies whenever
you see a SetToDefault in the VALUES lists.
BTW, another thing that needs checking is whether a rule containing
an INSERT like this will reverse-list sanely. The whole idea of
replacing some of the Vars might not work so far as ruleutils.c is
concerned. In that case I think we might have to implement this
by having transformInsertStmt restructure the VALUES lists to
physically remove the all-DEFAULT column, and adjust the target column
list accordingly --- that is, make a parse-time transformation of
INSERT INTO gtest0 VALUES (1, DEFAULT), (2, DEFAULT);
into
INSERT INTO gtest0(a) VALUES (1), (2);
That'd have the advantage that you'd not have to hack up the
rewriter at all.
Also, in the case where the user has failed to ensure that all the
column entries are DEFAULT, I suppose that we'll still get the same
error as now:
regression=# INSERT INTO gtest0 VALUES (1, DEFAULT), (2, 42);
ERROR: cannot insert into column "b"
DETAIL: Column "b" is a generated column.
This seems fairly confusing and unhelpful. Perhaps it's not this
patch's job to improve it, but it'd be nice if we could do better.
One easy change would be to make the error message more specific:
ERROR: cannot insert a non-DEFAULT value into column "b"
(I think this wording is accurate, but I might be wrong.) It'd be
even better if we could emit an error cursor pointing at (one of)
the entries that are not DEFAULT, since in a command with a long
VALUES list it might not be that obvious where you screwed up.
FWIW, I would not bother splitting a patch like this into two parts.
That increases your effort level, and it increases the reviewer's
effort to apply it too, and this patch isn't big enough to justify it.
regards, tom lane
[1]: https://travis-ci.org/github/postgresql-cfbot/postgresql/builds/724543451
On Sun, 6 Sept 2020 at 22:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think you'd be better off to make transformInsertStmt(), specifically
its multi-VALUES-rows code path, check for all-DEFAULT columns and adjust
the tlist itself. Doing it there might be a good bit less inefficient
for very long VALUES lists, too, which is a case that we do worry about.
Since that's already iterating through the sub-lists, you could track
whether all rows seen so far contain SetToDefault in each column position,
and avoid extra scans of the sublists. (A BitmapSet might be a convenient
representation of that, though you could also use a bool array I suppose.)I do not care for what you did in rewriteValuesRTE() either: just removing
a sanity check isn't OK, unless you've done something to make the sanity
check unnecessary which you surely have not. Perhaps you could extend
the initial scan of the tlist (lines 1294-1310) to notice SetToDefault
nodes as well as Var nodes and keep track of which columns have those.
Then you could cross-check that one or the other case applies whenever
you see a SetToDefault in the VALUES lists.
That's not quite right because by the time rewriteValuesRTE() sees the
tlist, it will contain already-rewritten generated column expressions,
not SetToDefault nodes. If we're going to keep that sanity check (and
I think that we should), I think that the way to do it is to have
rewriteTargetListIU() record which columns it has expanded defaults
for, and pass that information to rewriteValuesRTE(). Those columns of
the VALUES RTE are no longer used in the query, so the sanity check
can be amended to ignore them while continuing to check the other
columns.
Incidentally, there is another way of causing that sanity check to
fail -- an INSERT ... OVERRIDING USER VALUE query with some DEFAULTS
in the VALUES RTE (but not necessarily all DEFAULTs) will trigger it.
So even if the parser completely removed any all-DEFAULT columns from
the VALUES RTE, some work in the rewriter would still be necessary.
BTW, another thing that needs checking is whether a rule containing
an INSERT like this will reverse-list sanely. The whole idea of
replacing some of the Vars might not work so far as ruleutils.c is
concerned. In that case I think we might have to implement this
by having transformInsertStmt restructure the VALUES lists to
physically remove the all-DEFAULT column, and adjust the target column
list accordingly --- that is, make a parse-time transformation of
INSERT INTO gtest0 VALUES (1, DEFAULT), (2, DEFAULT);
into
INSERT INTO gtest0(a) VALUES (1), (2);
That'd have the advantage that you'd not have to hack up the
rewriter at all.
I think it's actually easier to just do it all in the rewriter -- at
the point where we see that we're about to insert potentially illegal
values from a VALUES RTE into a generated column, scan it to see if
all the values in that column are DEFAULTs, and if so trigger the
existing code to replace the Var in the tlist with the generated
column expression. That way we only do extra work in the case for
which we're currently throwing an error, rather than for every query.
Also, I think that scanning the VALUES lists in this way is likely to
be cheaper than rebuilding them to remove a column.
Attached is a patch doing it that way, along with additional
regression tests that trigger both the original error and the
sanity-check error triggered by INSERT ... OVERRIDING USER VALUES. I
also added a few additional comments where I found the existing code a
little non-obvious.
I haven't touched the existing error messages. I think that's a
subject for a separate patch.
Regards,
Dean
Attachments:
fix-generated-cols-with-defaults-in-values-rte.patchtext/x-patch; charset=US-ASCII; name=fix-generated-cols-with-defaults-in-values-rte.patchDownload
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
new file mode 100644
index 41dd670..966cab5
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -69,13 +69,18 @@ static List *rewriteTargetListIU(List *t
CmdType commandType,
OverridingKind override,
Relation target_relation,
- int result_rti);
+ int result_rti,
+ RangeTblEntry *values_rte,
+ int values_rte_index,
+ Bitmapset **unused_values_attrnos);
static TargetEntry *process_matched_tle(TargetEntry *src_tle,
TargetEntry *prior_tle,
const char *attrName);
static Node *get_assignment_input(Node *node);
+static Bitmapset *findDefaultOnlyColumns(RangeTblEntry *rte);
static bool rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
- Relation target_relation, bool force_nulls);
+ Relation target_relation, bool force_nulls,
+ Bitmapset *unused_cols);
static void markQueryForLocking(Query *qry, Node *jtnode,
LockClauseStrength strength, LockWaitPolicy waitPolicy,
bool pushedDown);
@@ -708,13 +713,25 @@ adjustJoinTreeList(Query *parsetree, boo
* is incorrect by this light, since child relations might have different
* column ordering, but the planner will fix things by re-sorting the tlist
* for each child.)
+ *
+ * If values_rte is non-NULL (i.e., we are doing a multi-row INSERT using
+ * values from a VALUES RTE), we populate unused_values_attrnos with the
+ * attribute numbers of any unused columns from the VALUES RTE. This can
+ * happen for identity and generated columns whose targetlist entries are
+ * replaced with generated expressions (if INSERT ... OVERRIDING USER VALUE is
+ * used, or all the values to be inserted are DEFAULT). This information is
+ * required by rewriteValuesRTE() to handle any DEFAULT items in the unused
+ * columns.
*/
static List *
rewriteTargetListIU(List *targetList,
CmdType commandType,
OverridingKind override,
Relation target_relation,
- int result_rti)
+ int result_rti,
+ RangeTblEntry *values_rte,
+ int values_rte_index,
+ Bitmapset **unused_values_attrnos)
{
TargetEntry **new_tles;
List *new_tlist = NIL;
@@ -724,6 +741,7 @@ rewriteTargetListIU(List *targetList,
next_junk_attrno,
numattrs;
ListCell *temp;
+ Bitmapset *default_only_cols = NULL;
/*
* We process the normal (non-junk) attributes by scanning the input tlist
@@ -803,30 +821,102 @@ rewriteTargetListIU(List *targetList,
if (commandType == CMD_INSERT)
{
+ int values_attrno = 0;
+
+ /* Source attribute number for values that come from a VALUES RTE */
+ if (values_rte && new_tle && IsA(new_tle->expr, Var))
+ {
+ Var *var = (Var *) new_tle->expr;
+
+ if (var->varno == values_rte_index)
+ values_attrno = var->varattno;
+ }
+
+ /*
+ * Can only insert DEFAULT into GENERATED ALWAYS identity columns,
+ * unless either OVERRIDING USER VALUE or OVERRIDING SYSTEM VALUE
+ * is specified.
+ */
if (att_tup->attidentity == ATTRIBUTE_IDENTITY_ALWAYS && !apply_default)
{
if (override == OVERRIDING_USER_VALUE)
apply_default = true;
else if (override != OVERRIDING_SYSTEM_VALUE)
- ereport(ERROR,
- (errcode(ERRCODE_GENERATED_ALWAYS),
- errmsg("cannot insert into column \"%s\"", NameStr(att_tup->attname)),
- errdetail("Column \"%s\" is an identity column defined as GENERATED ALWAYS.",
- NameStr(att_tup->attname)),
- errhint("Use OVERRIDING SYSTEM VALUE to override.")));
+ {
+ /*
+ * If this column's values come from a VALUES RTE, test
+ * whether it contains only SetToDefault items. Since the
+ * VALUES list might be quite large, we arrange to only
+ * scan it once.
+ */
+ if (values_attrno != 0)
+ {
+ if (default_only_cols == NULL)
+ default_only_cols = findDefaultOnlyColumns(values_rte);
+
+ if (bms_is_member(values_attrno, default_only_cols))
+ apply_default = true;
+ }
+
+ if (!apply_default)
+ ereport(ERROR,
+ (errcode(ERRCODE_GENERATED_ALWAYS),
+ errmsg("cannot insert into column \"%s\"", NameStr(att_tup->attname)),
+ errdetail("Column \"%s\" is an identity column defined as GENERATED ALWAYS.",
+ NameStr(att_tup->attname)),
+ errhint("Use OVERRIDING SYSTEM VALUE to override.")));
+ }
}
+ /*
+ * Can insert any value into GENERATED BY DEFAULT identity
+ * columns, but we apply the default if OVERRIDING USER VALUE is
+ * specified.
+ */
if (att_tup->attidentity == ATTRIBUTE_IDENTITY_BY_DEFAULT && override == OVERRIDING_USER_VALUE)
apply_default = true;
+ /*
+ * Can only insert DEFAULT into generated columns, regardless of
+ * any OVERRIDING clauses.
+ */
if (att_tup->attgenerated && !apply_default)
- ereport(ERROR,
- (errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("cannot insert into column \"%s\"", NameStr(att_tup->attname)),
- errdetail("Column \"%s\" is a generated column.",
- NameStr(att_tup->attname))));
+ {
+ /*
+ * If this column's values come from a VALUES RTE, test
+ * whether it contains only SetToDefault items.
+ */
+ if (values_attrno != 0)
+ {
+ if (default_only_cols == NULL)
+ default_only_cols = findDefaultOnlyColumns(values_rte);
+
+ if (bms_is_member(values_attrno, default_only_cols))
+ apply_default = true;
+ }
+
+ if (!apply_default)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("cannot insert into column \"%s\"", NameStr(att_tup->attname)),
+ errdetail("Column \"%s\" is a generated column.",
+ NameStr(att_tup->attname))));
+ }
+
+ /*
+ * For an INSERT from a VALUES RTE, return the attribute numbers
+ * of any columns that will no longer be used (due to the
+ * targetlist entry being replaced by a default expression).
+ */
+ if (values_attrno != 0 && apply_default && unused_values_attrnos)
+ *unused_values_attrnos = bms_add_member(*unused_values_attrnos,
+ values_attrno);
}
+ /*
+ * Updates to identity and generated columns follow the same rules as
+ * above, except that UPDATE doesn't admit OVERRIDING clauses.
+ */
if (commandType == CMD_UPDATE)
{
if (att_tup->attidentity == ATTRIBUTE_IDENTITY_ALWAYS && new_tle && !apply_default)
@@ -1219,6 +1309,62 @@ searchForDefault(RangeTblEntry *rte)
return false;
}
+
+/*
+ * Search a VALUES RTE for columns that contain only SetToDefault items,
+ * returning a Bitmapset containing the attribute numbers of any such columns.
+ */
+static Bitmapset *
+findDefaultOnlyColumns(RangeTblEntry *rte)
+{
+ Bitmapset *default_only_cols = NULL;
+ ListCell *lc;
+
+ foreach(lc, rte->values_lists)
+ {
+ List *sublist = (List *) lfirst(lc);
+ ListCell *lc2;
+ int i;
+
+ if (default_only_cols == NULL)
+ {
+ /* Populate the initial result bitmap from the first row */
+ i = 0;
+ foreach(lc2, sublist)
+ {
+ Node *col = (Node *) lfirst(lc2);
+
+ i++;
+ if (IsA(col, SetToDefault))
+ default_only_cols = bms_add_member(default_only_cols, i);
+ }
+ }
+ else
+ {
+ /* Update the result bitmap from this next row */
+ i = 0;
+ foreach(lc2, sublist)
+ {
+ Node *col = (Node *) lfirst(lc2);
+
+ i++;
+ if (!IsA(col, SetToDefault))
+ default_only_cols = bms_del_member(default_only_cols, i);
+ }
+ }
+
+ /*
+ * If no column in the rows read so far contains only DEFAULT items,
+ * we are done.
+ */
+ if (default_only_cols == NULL)
+ break;
+ }
+
+ return default_only_cols;
+}
+
+
/*
* When processing INSERT ... VALUES with a VALUES RTE (ie, multiple VALUES
* lists), we have to replace any DEFAULT items in the VALUES lists with
@@ -1246,19 +1392,29 @@ searchForDefault(RangeTblEntry *rte)
* an insert into an auto-updatable view, and the product queries are inserts
* into a rule-updatable view.
*
+ * Finally, if a DEFAULT item is found in a column mentioned in unused_cols,
+ * it is explicitly set to NULL. This happens for columns in the VALUES RTE
+ * whose corresponding targetlist entries have already been replaced with the
+ * relation's default expressions, and any values in those columns of the
+ * VALUES RTE are no longer used. This can happen for identity and generated
+ * columns (if INSERT ... OVERRIDING USER VALUE is used, or all the values to
+ * be inserted are DEFAULT).
+ *
* Note that we may have subscripted or field assignment targetlist entries,
* as well as more complex expressions from already-replaced DEFAULT items if
* we have recursed to here for an auto-updatable view. However, it ought to
- * be impossible for such entries to have DEFAULTs assigned to them --- we
- * should only have to replace DEFAULT items for targetlist entries that
- * contain simple Vars referencing the VALUES RTE.
+ * be impossible for such entries to have DEFAULTs assigned to them, except
+ * for unused columns, as described above --- we should only have to replace
+ * DEFAULT items for targetlist entries that contain simple Vars referencing
+ * the VALUES RTE, or which are no longer referred to by the targetlist.
*
* Returns true if all DEFAULT items were replaced, and false if some were
* left untouched.
*/
static bool
rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
- Relation target_relation, bool force_nulls)
+ Relation target_relation, bool force_nulls,
+ Bitmapset *unused_cols)
{
List *newValues;
ListCell *lc;
@@ -1282,8 +1438,8 @@ rewriteValuesRTE(Query *parsetree, Range
* Scan the targetlist for entries referring to the VALUES RTE, and note
* the target attributes. As noted above, we should only need to do this
* for targetlist entries containing simple Vars --- nothing else in the
- * VALUES RTE should contain DEFAULT items, and we complain if such a
- * thing does occur.
+ * VALUES RTE should contain DEFAULT items (except possibly for unused
+ * columns), and we complain if such a thing does occur.
*/
numattrs = list_length(linitial(rte->values_lists));
attrnos = (int *) palloc0(numattrs * sizeof(int));
@@ -1370,6 +1526,22 @@ rewriteValuesRTE(Query *parsetree, Range
Form_pg_attribute att_tup;
Node *new_expr;
+ /*
+ * If this column isn't used, just replace the DEFAULT with
+ * NULL (attrno will be 0 in this case because the targetlist
+ * entry will have been replaced by the default expression).
+ */
+ if (bms_is_member(i, unused_cols))
+ {
+ SetToDefault *def = (SetToDefault *) col;
+
+ newList = lappend(newList,
+ makeNullConst(def->typeId,
+ def->typeMod,
+ def->collation));
+ continue;
+ }
+
if (attrno == 0)
elog(ERROR, "cannot set value in column %d to DEFAULT", i);
att_tup = TupleDescAttr(target_relation->rd_att, attrno - 1);
@@ -3614,15 +3786,21 @@ RewriteQuery(Query *parsetree, List *rew
if (values_rte)
{
+ Bitmapset *unused_values_attrnos = NULL;
+
/* Process the main targetlist ... */
parsetree->targetList = rewriteTargetListIU(parsetree->targetList,
parsetree->commandType,
parsetree->override,
rt_entry_relation,
- parsetree->resultRelation);
+ parsetree->resultRelation,
+ values_rte,
+ values_rte_index,
+ &unused_values_attrnos);
/* ... and the VALUES expression lists */
if (!rewriteValuesRTE(parsetree, values_rte, values_rte_index,
- rt_entry_relation, false))
+ rt_entry_relation, false,
+ unused_values_attrnos))
defaults_remaining = true;
}
else
@@ -3633,7 +3811,8 @@ RewriteQuery(Query *parsetree, List *rew
parsetree->commandType,
parsetree->override,
rt_entry_relation,
- parsetree->resultRelation);
+ parsetree->resultRelation,
+ NULL, 0, NULL);
}
if (parsetree->onConflict &&
@@ -3644,7 +3823,8 @@ RewriteQuery(Query *parsetree, List *rew
CMD_UPDATE,
parsetree->override,
rt_entry_relation,
- parsetree->resultRelation);
+ parsetree->resultRelation,
+ NULL, 0, NULL);
}
}
else if (event == CMD_UPDATE)
@@ -3654,7 +3834,8 @@ RewriteQuery(Query *parsetree, List *rew
parsetree->commandType,
parsetree->override,
rt_entry_relation,
- parsetree->resultRelation);
+ parsetree->resultRelation,
+ NULL, 0, NULL);
/* Also populate extraUpdatedCols (for generated columns) */
fill_extraUpdatedCols(rt_entry, rt_entry_relation);
@@ -3704,7 +3885,8 @@ RewriteQuery(Query *parsetree, List *rew
rewriteValuesRTE(pt, values_rte, values_rte_index,
rt_entry_relation,
- true); /* Force remaining defaults to NULL */
+ true, /* Force remaining defaults to NULL */
+ NULL);
}
}
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
new file mode 100644
index 4b06260..559fafa
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -91,17 +91,30 @@ ERROR: for a generated column, GENERATE
LINE 1: ...E gtest_err_8 (a int PRIMARY KEY, b int GENERATED BY DEFAULT...
^
INSERT INTO gtest1 VALUES (1);
-INSERT INTO gtest1 VALUES (2, DEFAULT);
+INSERT INTO gtest1 VALUES (2, DEFAULT); -- ok
INSERT INTO gtest1 VALUES (3, 33); -- error
ERROR: cannot insert into column "b"
DETAIL: Column "b" is a generated column.
+INSERT INTO gtest1 VALUES (3, 33), (4, 44); -- error
+ERROR: cannot insert into column "b"
+DETAIL: Column "b" is a generated column.
+INSERT INTO gtest1 VALUES (3, DEFAULT), (4, 44); -- error
+ERROR: cannot insert into column "b"
+DETAIL: Column "b" is a generated column.
+INSERT INTO gtest1 VALUES (3, 33), (4, DEFAULT); -- error
+ERROR: cannot insert into column "b"
+DETAIL: Column "b" is a generated column.
+INSERT INTO gtest1 VALUES (3, DEFAULT), (4, DEFAULT); -- ok
SELECT * FROM gtest1 ORDER BY a;
a | b
---+---
1 | 2
2 | 4
-(2 rows)
+ 3 | 6
+ 4 | 8
+(4 rows)
+DELETE FROM gtest1 WHERE a >= 3;
UPDATE gtest1 SET b = DEFAULT WHERE a = 1;
UPDATE gtest1 SET b = 11 WHERE a = 1; -- error
ERROR: column "b" can only be updated to DEFAULT
@@ -179,9 +192,37 @@ SELECT * FROM gtest1v;
3 | 6
(1 row)
-INSERT INTO gtest1v VALUES (4, 8); -- fails
+INSERT INTO gtest1v VALUES (4, 8); -- error
+ERROR: cannot insert into column "b"
+DETAIL: Column "b" is a generated column.
+INSERT INTO gtest1v VALUES (5, DEFAULT); -- ok
+INSERT INTO gtest1v VALUES (6, 66), (7, 77); -- error
ERROR: cannot insert into column "b"
DETAIL: Column "b" is a generated column.
+INSERT INTO gtest1v VALUES (6, DEFAULT), (7, 77); -- error
+ERROR: cannot insert into column "b"
+DETAIL: Column "b" is a generated column.
+INSERT INTO gtest1v VALUES (6, 66), (7, DEFAULT); -- error
+ERROR: cannot insert into column "b"
+DETAIL: Column "b" is a generated column.
+INSERT INTO gtest1v VALUES (6, DEFAULT), (7, DEFAULT); -- ok
+ALTER VIEW gtest1v ALTER COLUMN b SET DEFAULT 100;
+INSERT INTO gtest1v VALUES (8, DEFAULT); -- error
+ERROR: cannot insert into column "b"
+DETAIL: Column "b" is a generated column.
+INSERT INTO gtest1v VALUES (8, DEFAULT), (9, DEFAULT); -- error
+ERROR: cannot insert into column "b"
+DETAIL: Column "b" is a generated column.
+SELECT * FROM gtest1v;
+ a | b
+---+----
+ 3 | 6
+ 5 | 10
+ 6 | 12
+ 7 | 14
+(4 rows)
+
+DELETE FROM gtest1v WHERE a >= 5;
DROP VIEW gtest1v;
-- CTEs
WITH foo AS (SELECT * FROM gtest1) SELECT * FROM foo;
diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out
new file mode 100644
index 2238f89..7e36b36
--- a/src/test/regress/expected/identity.out
+++ b/src/test/regress/expected/identity.out
@@ -105,6 +105,62 @@ SELECT * FROM itest4;
(2 rows)
-- VALUES RTEs
+CREATE TABLE itest5 (a int generated always as identity, b text);
+INSERT INTO itest5 VALUES (1, 'a'); -- error
+ERROR: cannot insert into column "a"
+DETAIL: Column "a" is an identity column defined as GENERATED ALWAYS.
+HINT: Use OVERRIDING SYSTEM VALUE to override.
+INSERT INTO itest5 VALUES (DEFAULT, 'a'); -- ok
+INSERT INTO itest5 VALUES (2, 'b'), (3, 'c'); -- error
+ERROR: cannot insert into column "a"
+DETAIL: Column "a" is an identity column defined as GENERATED ALWAYS.
+HINT: Use OVERRIDING SYSTEM VALUE to override.
+INSERT INTO itest5 VALUES (DEFAULT, 'b'), (3, 'c'); -- error
+ERROR: cannot insert into column "a"
+DETAIL: Column "a" is an identity column defined as GENERATED ALWAYS.
+HINT: Use OVERRIDING SYSTEM VALUE to override.
+INSERT INTO itest5 VALUES (2, 'b'), (DEFAULT, 'c'); -- error
+ERROR: cannot insert into column "a"
+DETAIL: Column "a" is an identity column defined as GENERATED ALWAYS.
+HINT: Use OVERRIDING SYSTEM VALUE to override.
+INSERT INTO itest5 VALUES (DEFAULT, 'b'), (DEFAULT, 'c'); -- ok
+INSERT INTO itest5 OVERRIDING SYSTEM VALUE VALUES (-1, 'aa');
+INSERT INTO itest5 OVERRIDING SYSTEM VALUE VALUES (-2, 'bb'), (-3, 'cc');
+INSERT INTO itest5 OVERRIDING SYSTEM VALUE VALUES (DEFAULT, 'dd'), (-4, 'ee');
+INSERT INTO itest5 OVERRIDING SYSTEM VALUE VALUES (-5, 'ff'), (DEFAULT, 'gg');
+INSERT INTO itest5 OVERRIDING SYSTEM VALUE VALUES (DEFAULT, 'hh'), (DEFAULT, 'ii');
+INSERT INTO itest5 OVERRIDING USER VALUE VALUES (-1, 'aaa');
+INSERT INTO itest5 OVERRIDING USER VALUE VALUES (-2, 'bbb'), (-3, 'ccc');
+INSERT INTO itest5 OVERRIDING USER VALUE VALUES (DEFAULT, 'ddd'), (-4, 'eee');
+INSERT INTO itest5 OVERRIDING USER VALUE VALUES (-5, 'fff'), (DEFAULT, 'ggg');
+INSERT INTO itest5 OVERRIDING USER VALUE VALUES (DEFAULT, 'hhh'), (DEFAULT, 'iii');
+SELECT * FROM itest5;
+ a | b
+----+-----
+ 1 | a
+ 2 | b
+ 3 | c
+ -1 | aa
+ -2 | bb
+ -3 | cc
+ 4 | dd
+ -4 | ee
+ -5 | ff
+ 5 | gg
+ 6 | hh
+ 7 | ii
+ 8 | aaa
+ 9 | bbb
+ 10 | ccc
+ 11 | ddd
+ 12 | eee
+ 13 | fff
+ 14 | ggg
+ 15 | hhh
+ 16 | iii
+(21 rows)
+
+DROP TABLE itest5;
INSERT INTO itest3 VALUES (DEFAULT, 'a');
INSERT INTO itest3 VALUES (DEFAULT, 'b'), (DEFAULT, 'c');
SELECT * FROM itest3;
diff --git a/src/test/regress/sql/generated.sql b/src/test/regress/sql/generated.sql
new file mode 100644
index c86ad34..bd2b0bf
--- a/src/test/regress/sql/generated.sql
+++ b/src/test/regress/sql/generated.sql
@@ -41,10 +41,15 @@ CREATE TABLE gtest_err_7d (a int PRIMARY
CREATE TABLE gtest_err_8 (a int PRIMARY KEY, b int GENERATED BY DEFAULT AS (a * 2) STORED);
INSERT INTO gtest1 VALUES (1);
-INSERT INTO gtest1 VALUES (2, DEFAULT);
+INSERT INTO gtest1 VALUES (2, DEFAULT); -- ok
INSERT INTO gtest1 VALUES (3, 33); -- error
+INSERT INTO gtest1 VALUES (3, 33), (4, 44); -- error
+INSERT INTO gtest1 VALUES (3, DEFAULT), (4, 44); -- error
+INSERT INTO gtest1 VALUES (3, 33), (4, DEFAULT); -- error
+INSERT INTO gtest1 VALUES (3, DEFAULT), (4, DEFAULT); -- ok
SELECT * FROM gtest1 ORDER BY a;
+DELETE FROM gtest1 WHERE a >= 3;
UPDATE gtest1 SET b = DEFAULT WHERE a = 1;
UPDATE gtest1 SET b = 11 WHERE a = 1; -- error
@@ -75,7 +80,19 @@ SELECT * FROM gtest1 ORDER BY a;
-- views
CREATE VIEW gtest1v AS SELECT * FROM gtest1;
SELECT * FROM gtest1v;
-INSERT INTO gtest1v VALUES (4, 8); -- fails
+INSERT INTO gtest1v VALUES (4, 8); -- error
+INSERT INTO gtest1v VALUES (5, DEFAULT); -- ok
+INSERT INTO gtest1v VALUES (6, 66), (7, 77); -- error
+INSERT INTO gtest1v VALUES (6, DEFAULT), (7, 77); -- error
+INSERT INTO gtest1v VALUES (6, 66), (7, DEFAULT); -- error
+INSERT INTO gtest1v VALUES (6, DEFAULT), (7, DEFAULT); -- ok
+
+ALTER VIEW gtest1v ALTER COLUMN b SET DEFAULT 100;
+INSERT INTO gtest1v VALUES (8, DEFAULT); -- error
+INSERT INTO gtest1v VALUES (8, DEFAULT), (9, DEFAULT); -- error
+
+SELECT * FROM gtest1v;
+DELETE FROM gtest1v WHERE a >= 5;
DROP VIEW gtest1v;
-- CTEs
diff --git a/src/test/regress/sql/identity.sql b/src/test/regress/sql/identity.sql
new file mode 100644
index d4bc29a..cdda186
--- a/src/test/regress/sql/identity.sql
+++ b/src/test/regress/sql/identity.sql
@@ -56,6 +56,29 @@ SELECT * FROM itest4;
-- VALUES RTEs
+CREATE TABLE itest5 (a int generated always as identity, b text);
+INSERT INTO itest5 VALUES (1, 'a'); -- error
+INSERT INTO itest5 VALUES (DEFAULT, 'a'); -- ok
+INSERT INTO itest5 VALUES (2, 'b'), (3, 'c'); -- error
+INSERT INTO itest5 VALUES (DEFAULT, 'b'), (3, 'c'); -- error
+INSERT INTO itest5 VALUES (2, 'b'), (DEFAULT, 'c'); -- error
+INSERT INTO itest5 VALUES (DEFAULT, 'b'), (DEFAULT, 'c'); -- ok
+
+INSERT INTO itest5 OVERRIDING SYSTEM VALUE VALUES (-1, 'aa');
+INSERT INTO itest5 OVERRIDING SYSTEM VALUE VALUES (-2, 'bb'), (-3, 'cc');
+INSERT INTO itest5 OVERRIDING SYSTEM VALUE VALUES (DEFAULT, 'dd'), (-4, 'ee');
+INSERT INTO itest5 OVERRIDING SYSTEM VALUE VALUES (-5, 'ff'), (DEFAULT, 'gg');
+INSERT INTO itest5 OVERRIDING SYSTEM VALUE VALUES (DEFAULT, 'hh'), (DEFAULT, 'ii');
+
+INSERT INTO itest5 OVERRIDING USER VALUE VALUES (-1, 'aaa');
+INSERT INTO itest5 OVERRIDING USER VALUE VALUES (-2, 'bbb'), (-3, 'ccc');
+INSERT INTO itest5 OVERRIDING USER VALUE VALUES (DEFAULT, 'ddd'), (-4, 'eee');
+INSERT INTO itest5 OVERRIDING USER VALUE VALUES (-5, 'fff'), (DEFAULT, 'ggg');
+INSERT INTO itest5 OVERRIDING USER VALUE VALUES (DEFAULT, 'hhh'), (DEFAULT, 'iii');
+
+SELECT * FROM itest5;
+DROP TABLE itest5;
+
INSERT INTO itest3 VALUES (DEFAULT, 'a');
INSERT INTO itest3 VALUES (DEFAULT, 'b'), (DEFAULT, 'c');
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
I think it's actually easier to just do it all in the rewriter -- at
the point where we see that we're about to insert potentially illegal
values from a VALUES RTE into a generated column, scan it to see if
all the values in that column are DEFAULTs, and if so trigger the
existing code to replace the Var in the tlist with the generated
column expression. That way we only do extra work in the case for
which we're currently throwing an error, rather than for every query.
That makes sense, and it leads to a nicely small patch. I reviewed
this and pushed it. I found only one nitpicky bug: in
findDefaultOnlyColumns, the test must be bms_is_empty(default_only_cols)
not just default_only_cols == NULL, or it will fail to fall out early
as intended when the first row contains some DEFAULTs but later rows
don't. I did tweak some of the commentary, too.
I haven't touched the existing error messages. I think that's a
subject for a separate patch.
Fair. After looking around a bit, I think that getting an error
cursor as I'd speculated about is more trouble than it's worth.
For starters, we'd have to pass down the query string into this
code, and there might be some ticklish issues about whether a given
chunk of parsetree came from that string or from some rule or view.
However, I think that just adjusting the error string would be
helpful, as attached.
(I'm also wondering why the second case is generic ERRCODE_SYNTAX_ERROR
and not ERRCODE_GENERATED_ALWAYS. Didn't change it here, though.)
regards, tom lane
Attachments:
adjust-cannot-insert-message.patchtext/x-diff; charset=us-ascii; name=adjust-cannot-insert-message.patchDownload
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 839583f834..00a48686c4 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -861,7 +861,7 @@ rewriteTargetListIU(List *targetList,
if (!apply_default)
ereport(ERROR,
(errcode(ERRCODE_GENERATED_ALWAYS),
- errmsg("cannot insert into column \"%s\"",
+ errmsg("cannot insert a non-DEFAULT value into column \"%s\"",
NameStr(att_tup->attname)),
errdetail("Column \"%s\" is an identity column defined as GENERATED ALWAYS.",
NameStr(att_tup->attname)),
@@ -900,7 +900,7 @@ rewriteTargetListIU(List *targetList,
if (!apply_default)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
- errmsg("cannot insert into column \"%s\"",
+ errmsg("cannot insert a non-DEFAULT value into column \"%s\"",
NameStr(att_tup->attname)),
errdetail("Column \"%s\" is a generated column.",
NameStr(att_tup->attname))));
diff --git a/src/test/regress/expected/generated.out b/src/test/regress/expected/generated.out
index 559fafa09e..ca721d38bf 100644
--- a/src/test/regress/expected/generated.out
+++ b/src/test/regress/expected/generated.out
@@ -93,16 +93,16 @@ LINE 1: ...E gtest_err_8 (a int PRIMARY KEY, b int GENERATED BY DEFAULT...
INSERT INTO gtest1 VALUES (1);
INSERT INTO gtest1 VALUES (2, DEFAULT); -- ok
INSERT INTO gtest1 VALUES (3, 33); -- error
-ERROR: cannot insert into column "b"
+ERROR: cannot insert a non-DEFAULT value into column "b"
DETAIL: Column "b" is a generated column.
INSERT INTO gtest1 VALUES (3, 33), (4, 44); -- error
-ERROR: cannot insert into column "b"
+ERROR: cannot insert a non-DEFAULT value into column "b"
DETAIL: Column "b" is a generated column.
INSERT INTO gtest1 VALUES (3, DEFAULT), (4, 44); -- error
-ERROR: cannot insert into column "b"
+ERROR: cannot insert a non-DEFAULT value into column "b"
DETAIL: Column "b" is a generated column.
INSERT INTO gtest1 VALUES (3, 33), (4, DEFAULT); -- error
-ERROR: cannot insert into column "b"
+ERROR: cannot insert a non-DEFAULT value into column "b"
DETAIL: Column "b" is a generated column.
INSERT INTO gtest1 VALUES (3, DEFAULT), (4, DEFAULT); -- ok
SELECT * FROM gtest1 ORDER BY a;
@@ -193,25 +193,25 @@ SELECT * FROM gtest1v;
(1 row)
INSERT INTO gtest1v VALUES (4, 8); -- error
-ERROR: cannot insert into column "b"
+ERROR: cannot insert a non-DEFAULT value into column "b"
DETAIL: Column "b" is a generated column.
INSERT INTO gtest1v VALUES (5, DEFAULT); -- ok
INSERT INTO gtest1v VALUES (6, 66), (7, 77); -- error
-ERROR: cannot insert into column "b"
+ERROR: cannot insert a non-DEFAULT value into column "b"
DETAIL: Column "b" is a generated column.
INSERT INTO gtest1v VALUES (6, DEFAULT), (7, 77); -- error
-ERROR: cannot insert into column "b"
+ERROR: cannot insert a non-DEFAULT value into column "b"
DETAIL: Column "b" is a generated column.
INSERT INTO gtest1v VALUES (6, 66), (7, DEFAULT); -- error
-ERROR: cannot insert into column "b"
+ERROR: cannot insert a non-DEFAULT value into column "b"
DETAIL: Column "b" is a generated column.
INSERT INTO gtest1v VALUES (6, DEFAULT), (7, DEFAULT); -- ok
ALTER VIEW gtest1v ALTER COLUMN b SET DEFAULT 100;
INSERT INTO gtest1v VALUES (8, DEFAULT); -- error
-ERROR: cannot insert into column "b"
+ERROR: cannot insert a non-DEFAULT value into column "b"
DETAIL: Column "b" is a generated column.
INSERT INTO gtest1v VALUES (8, DEFAULT), (9, DEFAULT); -- error
-ERROR: cannot insert into column "b"
+ERROR: cannot insert a non-DEFAULT value into column "b"
DETAIL: Column "b" is a generated column.
SELECT * FROM gtest1v;
a | b
diff --git a/src/test/regress/expected/identity.out b/src/test/regress/expected/identity.out
index 7e36b36ecd..fbca0333a2 100644
--- a/src/test/regress/expected/identity.out
+++ b/src/test/regress/expected/identity.out
@@ -107,20 +107,20 @@ SELECT * FROM itest4;
-- VALUES RTEs
CREATE TABLE itest5 (a int generated always as identity, b text);
INSERT INTO itest5 VALUES (1, 'a'); -- error
-ERROR: cannot insert into column "a"
+ERROR: cannot insert a non-DEFAULT value into column "a"
DETAIL: Column "a" is an identity column defined as GENERATED ALWAYS.
HINT: Use OVERRIDING SYSTEM VALUE to override.
INSERT INTO itest5 VALUES (DEFAULT, 'a'); -- ok
INSERT INTO itest5 VALUES (2, 'b'), (3, 'c'); -- error
-ERROR: cannot insert into column "a"
+ERROR: cannot insert a non-DEFAULT value into column "a"
DETAIL: Column "a" is an identity column defined as GENERATED ALWAYS.
HINT: Use OVERRIDING SYSTEM VALUE to override.
INSERT INTO itest5 VALUES (DEFAULT, 'b'), (3, 'c'); -- error
-ERROR: cannot insert into column "a"
+ERROR: cannot insert a non-DEFAULT value into column "a"
DETAIL: Column "a" is an identity column defined as GENERATED ALWAYS.
HINT: Use OVERRIDING SYSTEM VALUE to override.
INSERT INTO itest5 VALUES (2, 'b'), (DEFAULT, 'c'); -- error
-ERROR: cannot insert into column "a"
+ERROR: cannot insert a non-DEFAULT value into column "a"
DETAIL: Column "a" is an identity column defined as GENERATED ALWAYS.
HINT: Use OVERRIDING SYSTEM VALUE to override.
INSERT INTO itest5 VALUES (DEFAULT, 'b'), (DEFAULT, 'c'); -- ok
@@ -197,7 +197,7 @@ SELECT * FROM itest1;
-- GENERATED ALWAYS
-- This is an error:
INSERT INTO itest2 VALUES (10, 'xyz');
-ERROR: cannot insert into column "a"
+ERROR: cannot insert a non-DEFAULT value into column "a"
DETAIL: Column "a" is an identity column defined as GENERATED ALWAYS.
HINT: Use OVERRIDING SYSTEM VALUE to override.
-- This inserts the row as presented:
@@ -313,7 +313,7 @@ SELECT * FROM itestv10;
(4 rows)
INSERT INTO itestv11 VALUES (10, 'xyz');
-ERROR: cannot insert into column "a"
+ERROR: cannot insert a non-DEFAULT value into column "a"
DETAIL: Column "a" is an identity column defined as GENERATED ALWAYS.
HINT: Use OVERRIDING SYSTEM VALUE to override.
INSERT INTO itestv11 OVERRIDING SYSTEM VALUE VALUES (11, 'xyz');
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
index 6a977006ef..24905332b1 100644
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -1475,10 +1475,10 @@ INSERT INTO rw_view1 (id) VALUES (2);
INSERT INTO base_tbl (id, idplus1) VALUES (3, DEFAULT);
INSERT INTO rw_view1 (id, idplus1) VALUES (4, DEFAULT);
INSERT INTO base_tbl (id, idplus1) VALUES (5, 6); -- error
-ERROR: cannot insert into column "idplus1"
+ERROR: cannot insert a non-DEFAULT value into column "idplus1"
DETAIL: Column "idplus1" is a generated column.
INSERT INTO rw_view1 (id, idplus1) VALUES (6, 7); -- error
-ERROR: cannot insert into column "idplus1"
+ERROR: cannot insert a non-DEFAULT value into column "idplus1"
DETAIL: Column "idplus1" is a generated column.
SELECT * FROM base_tbl;
id | idplus1
On Sun, 22 Nov 2020 at 20:58, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I found only one nitpicky bug: in
findDefaultOnlyColumns, the test must be bms_is_empty(default_only_cols)
not just default_only_cols == NULL, or it will fail to fall out early
as intended when the first row contains some DEFAULTs but later rows
don't.
Ah, good point. Thanks for fixing that.
I haven't touched the existing error messages. I think that's a
subject for a separate patch.Fair. After looking around a bit, I think that getting an error
cursor as I'd speculated about is more trouble than it's worth.
For starters, we'd have to pass down the query string into this
code, and there might be some ticklish issues about whether a given
chunk of parsetree came from that string or from some rule or view.
However, I think that just adjusting the error string would be
helpful, as attached.
+1
(I'm also wondering why the second case is generic ERRCODE_SYNTAX_ERROR
and not ERRCODE_GENERATED_ALWAYS. Didn't change it here, though.)
I can't see any reason for it to be different, and
ERRCODE_GENERATED_ALWAYS seems like the right code to use for both
cases.
Regards,
Dean
Dean Rasheed <dean.a.rasheed@gmail.com> writes:
On Sun, 22 Nov 2020 at 20:58, Tom Lane <tgl@sss.pgh.pa.us> wrote:
However, I think that just adjusting the error string would be
helpful, as attached.
+1
(I'm also wondering why the second case is generic ERRCODE_SYNTAX_ERROR
and not ERRCODE_GENERATED_ALWAYS. Didn't change it here, though.)
I can't see any reason for it to be different, and
ERRCODE_GENERATED_ALWAYS seems like the right code to use for both
cases.
Sounds good to me; pushed that way.
regards, tom lane