pg.ddx.io pgsql-hackers@postgresql.org mailing list archivehelp / color / mirror / Atom feed
[bug+patch] Inserting DEFAULT into generated columns from VALUES RTE 7+ messages / 4 participants [nested] [flat]
Commits from this thread: 2020-11-23 d36228a9fc Improve wording of two error messages related to generated columns. 2020-11-22 17958972fe Allow a multi-row INSERT to specify DEFAULTs for a generated column.
* [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE @ 2020-08-13 04:30 Mikhail Titov <mlt@gmx.us> 2020-08-13 05:11 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Pavel Stehule <pavel.stehule@gmail.com> 0 siblings, 1 reply; 7+ messages in thread From: Mikhail Titov @ 2020-08-13 04:30 UTC (permalink / raw) To: pgsql-hackers@lists.postgresql.org 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]. Footnotes: [1] https://www.postgresql.org/docs/devel/ddl-generated-columns.html [2] https://github.com/rails/rails/pull/39368#issuecomment-670351379 -- Mikhail Attachments: download all (2 files, .tar.gz)0001-Test-DEFAULT-in-VALUES-RTE-for-generated-columns.patch (text/x-patch, 3.4K) download
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^ permalink raw reply [nested|flat] 7+ messages in thread0002-Allow-DEFAULT-in-VALUES-RTE-for-generated-columns.patch (text/x-patch, 3.2K) download
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
* Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE 2020-08-13 04:30 [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Mikhail Titov <mlt@gmx.us> @ 2020-08-13 05:11 ` Pavel Stehule <pavel.stehule@gmail.com> 2020-08-14 19:57 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Mikhail Titov <mlt@gmx.us> 0 siblings, 1 reply; 7+ messages in thread From: Pavel Stehule @ 2020-08-13 05:11 UTC (permalink / raw) To: Mikhail Titov <mlt@gmx.us>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org> 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 > Footnotes: > [1] https://www.postgresql.org/docs/devel/ddl-generated-columns.html > > [2] https://github.com/rails/rails/pull/39368#issuecomment-670351379 > > -- > Mikhail > ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE 2020-08-13 04:30 [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Mikhail Titov <mlt@gmx.us> 2020-08-13 05:11 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Pavel Stehule <pavel.stehule@gmail.com> @ 2020-08-14 19:57 ` Mikhail Titov <mlt@gmx.us> 2020-11-20 14:30 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Dean Rasheed <dean.a.rasheed@gmail.com> 0 siblings, 1 reply; 7+ messages in thread From: Mikhail Titov @ 2020-08-14 19:57 UTC (permalink / raw) To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org> 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: download all (2 files, .tar.gz)v2-0001-Test-DEFAULT-in-VALUES-RTE-for-generated-columns.patch (application/octet-stream, 3.0K) download
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^ permalink raw reply [nested|flat] 7+ messages in threadv2-0002-Allow-DEFAULT-in-VALUES-RTE-for-generated-columns.patch (application/octet-stream, 3.6K) download
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
* Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE 2020-08-13 04:30 [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Mikhail Titov <mlt@gmx.us> 2020-08-13 05:11 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Pavel Stehule <pavel.stehule@gmail.com> 2020-08-14 19:57 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Mikhail Titov <mlt@gmx.us> @ 2020-11-20 14:30 ` Dean Rasheed <dean.a.rasheed@gmail.com> 2020-11-22 20:58 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 1 reply; 7+ messages in thread From: Dean Rasheed @ 2020-11-20 14:30 UTC (permalink / raw) To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Mikhail Titov <mlt@gmx.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org> 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:^ permalink raw reply [nested|flat] 7+ messages in threadfix-generated-cols-with-defaults-in-values-rte.patch (text/x-patch, 21.3K) download
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');
* Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE 2020-08-13 04:30 [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Mikhail Titov <mlt@gmx.us> 2020-08-13 05:11 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Pavel Stehule <pavel.stehule@gmail.com> 2020-08-14 19:57 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Mikhail Titov <mlt@gmx.us> 2020-11-20 14:30 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Dean Rasheed <dean.a.rasheed@gmail.com> @ 2020-11-22 20:58 ` Tom Lane <tgl@sss.pgh.pa.us> 2020-11-23 14:45 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Dean Rasheed <dean.a.rasheed@gmail.com> 0 siblings, 1 reply; 7+ messages in thread From: Tom Lane @ 2020-11-22 20:58 UTC (permalink / raw) To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: Mikhail Titov <mlt@gmx.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org> 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:^ permalink raw reply [nested|flat] 7+ messages in threadadjust-cannot-insert-message.patch (text/x-diff, 6.9K) download
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 =
* Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE 2020-08-13 04:30 [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Mikhail Titov <mlt@gmx.us> 2020-08-13 05:11 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Pavel Stehule <pavel.stehule@gmail.com> 2020-08-14 19:57 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Mikhail Titov <mlt@gmx.us> 2020-11-20 14:30 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Dean Rasheed <dean.a.rasheed@gmail.com> 2020-11-22 20:58 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Tom Lane <tgl@sss.pgh.pa.us> @ 2020-11-23 14:45 ` Dean Rasheed <dean.a.rasheed@gmail.com> 2020-11-23 16:16 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 1 reply; 7+ messages in thread From: Dean Rasheed @ 2020-11-23 14:45 UTC (permalink / raw) To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Mikhail Titov <mlt@gmx.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org> 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE 2020-08-13 04:30 [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Mikhail Titov <mlt@gmx.us> 2020-08-13 05:11 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Pavel Stehule <pavel.stehule@gmail.com> 2020-08-14 19:57 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Mikhail Titov <mlt@gmx.us> 2020-11-20 14:30 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Dean Rasheed <dean.a.rasheed@gmail.com> 2020-11-22 20:58 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Tom Lane <tgl@sss.pgh.pa.us> 2020-11-23 14:45 ` Re: [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Dean Rasheed <dean.a.rasheed@gmail.com> @ 2020-11-23 16:16 ` Tom Lane <tgl@sss.pgh.pa.us> 0 siblings, 0 replies; 7+ messages in thread From: Tom Lane @ 2020-11-23 16:16 UTC (permalink / raw) To: Dean Rasheed <dean.a.rasheed@gmail.com>; +Cc: Mikhail Titov <mlt@gmx.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org> 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 ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2020-11-23 16:16 UTC | newest] Thread overview: 7+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-08-13 04:30 [bug+patch] Inserting DEFAULT into generated columns from VALUES RTE Mikhail Titov <mlt@gmx.us> 2020-08-13 05:11 ` Pavel Stehule <pavel.stehule@gmail.com> 2020-08-14 19:57 ` Mikhail Titov <mlt@gmx.us> 2020-11-20 14:30 ` Dean Rasheed <dean.a.rasheed@gmail.com> 2020-11-22 20:58 ` Tom Lane <tgl@sss.pgh.pa.us> 2020-11-23 14:45 ` Dean Rasheed <dean.a.rasheed@gmail.com> 2020-11-23 16:16 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions for how to clone and mirror all data and code used for this inbox