From: Dean Rasheed Date: Sun, 3 Mar 2019 10:51:13 +0000 (+0000) Subject: Further fixing for multi-row VALUES lists for updatable views. X-Git-Tag: REL_12_BETA1~633 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=ed4653db8ca7a70ba7a4d329a44812893f8e59c2;p=postgresql Further fixing for multi-row VALUES lists for updatable views. Previously, rewriteTargetListIU() generated a list of attribute numbers from the targetlist, which were passed to rewriteValuesRTE(), which expected them to contain the same number of entries as there are columns in the VALUES RTE, and to be in the same order. That was fine when the target relation was a table, but for an updatable view it could be broken in at least three different ways --- rewriteTargetListIU() could insert additional targetlist entries for view columns with defaults, the view columns could be in a different order from the columns of the underlying base relation, and targetlist entries could be merged together when assigning to elements of an array or composite type. As a result, when recursing to the base relation, the list of attribute numbers generated from the rewritten targetlist could no longer be relied upon to match the columns of the VALUES RTE. We got away with that prior to 41531e42d3 because it used to always be the case that rewriteValuesRTE() did nothing for the underlying base relation, since all DEFAULTS had already been replaced when it was initially invoked for the view, but that was incorrect because it failed to apply defaults from the base relation. Fix this by examining the targetlist entries more carefully and picking out just those that are simple Vars referencing the VALUES RTE. That's sufficient for the purposes of rewriteValuesRTE(), which is only responsible for dealing with DEFAULT items in the VALUES RTE. Any DEFAULT item in the VALUES RTE that doesn't have a matching simple-Var-assignment in the targetlist is an error which we complain about, but in theory that ought to be impossible. Additionally, move this code into rewriteValuesRTE() to give a clearer separation of concerns between the 2 functions. There is no need for rewriteTargetListIU() to know about the details of the VALUES RTE. While at it, fix the comment for rewriteValuesRTE() which claimed that it doesn't support array element and field assignments --- that hasn't been true since a3c7a993d5 (9.6 and later). Back-patch to all supported versions, with minor differences for the pre-9.6 branches, which don't support array element and field assignments to the same column in multi-row VALUES lists. Reviewed by Amit Langote. Discussion: https://postgr.es/m/15623-5d67a46788ec8b7f@postgresql.org --- diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index 7eb41ff026..4fc50c89b9 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -67,14 +67,13 @@ static List *rewriteTargetListIU(List *targetList, CmdType commandType, OverridingKind override, Relation target_relation, - int result_rti, - List **attrno_list); + int result_rti); static TargetEntry *process_matched_tle(TargetEntry *src_tle, TargetEntry *prior_tle, const char *attrName); static Node *get_assignment_input(Node *node); -static bool rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, - Relation target_relation, List *attrnos, bool force_nulls); +static bool rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti, + Relation target_relation, bool force_nulls); static void markQueryForLocking(Query *qry, Node *jtnode, LockClauseStrength strength, LockWaitPolicy waitPolicy, bool pushedDown); @@ -705,11 +704,6 @@ adjustJoinTreeList(Query *parsetree, bool removert, int rt_index) * is not needed for rewriting, but will be needed by the planner, and we * can do it essentially for free while handling the other items. * - * If attrno_list isn't NULL, we return an additional output besides the - * rewritten targetlist: an integer list of the assigned-to attnums, in - * order of the original tlist's non-junk entries. This is needed for - * processing VALUES RTEs. - * * Note that for an inheritable UPDATE, this processing is only done once, * using the parent relation as reference. It must not do anything that * will not be correct when transposed to the child relation(s). (Step 4 @@ -722,8 +716,7 @@ rewriteTargetListIU(List *targetList, CmdType commandType, OverridingKind override, Relation target_relation, - int result_rti, - List **attrno_list) + int result_rti) { TargetEntry **new_tles; List *new_tlist = NIL; @@ -734,9 +727,6 @@ rewriteTargetListIU(List *targetList, numattrs; ListCell *temp; - if (attrno_list) /* initialize optional result list */ - *attrno_list = NIL; - /* * We process the normal (non-junk) attributes by scanning the input tlist * once and transferring TLEs into an array, then scanning the array to @@ -762,10 +752,6 @@ rewriteTargetListIU(List *targetList, elog(ERROR, "bogus resno %d in targetlist", attrno); att_tup = TupleDescAttr(target_relation->rd_att, attrno - 1); - /* put attrno into attrno_list even if it's dropped */ - if (attrno_list) - *attrno_list = lappend_int(*attrno_list, attrno); - /* We can (and must) ignore deleted attributes */ if (att_tup->attisdropped) continue; @@ -1240,22 +1226,26 @@ searchForDefault(RangeTblEntry *rte) * an insert into an auto-updatable view, and the product queries are inserts * into a rule-updatable view. * - * Note that we currently can't support subscripted or field assignment - * in the multi-VALUES case. The targetlist will contain simple Vars - * referencing the VALUES RTE, and therefore process_matched_tle() will - * reject any such attempt with "multiple assignments to same column". + * 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. * * Returns true if all DEFAULT items were replaced, and false if some were * left untouched. */ static bool -rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, - Relation target_relation, List *attrnos, bool force_nulls) +rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti, + Relation target_relation, bool force_nulls) { List *newValues; ListCell *lc; bool isAutoUpdatableView; bool allReplaced; + int numattrs; + int *attrnos; /* * Rebuilding all the lists is a pretty expensive proposition in a big @@ -1268,8 +1258,33 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, if (!force_nulls && !searchForDefault(rte)) return true; /* nothing to do */ - /* Check list lengths (we can assume all the VALUES sublists are alike) */ - Assert(list_length(attrnos) == list_length(linitial(rte->values_lists))); + /* + * 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. + */ + numattrs = list_length(linitial(rte->values_lists)); + attrnos = (int *) palloc0(numattrs * sizeof(int)); + + foreach(lc, parsetree->targetList) + { + TargetEntry *tle = (TargetEntry *) lfirst(lc); + + if (IsA(tle->expr, Var)) + { + Var *var = (Var *) tle->expr; + + if (var->varno == rti) + { + int attrno = var->varattno; + + Assert(attrno >= 1 && attrno <= numattrs); + attrnos[attrno - 1] = tle->resno; + } + } + } /* * Check if the target relation is an auto-updatable view, in which case @@ -1320,18 +1335,23 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, List *sublist = (List *) lfirst(lc); List *newList = NIL; ListCell *lc2; - ListCell *lc3; + int i; + + Assert(list_length(sublist) == numattrs); - forboth(lc2, sublist, lc3, attrnos) + i = 0; + foreach(lc2, sublist) { Node *col = (Node *) lfirst(lc2); - int attrno = lfirst_int(lc3); + int attrno = attrnos[i++]; if (IsA(col, SetToDefault)) { Form_pg_attribute att_tup; Node *new_expr; + if (attrno == 0) + elog(ERROR, "cannot set value in column %d to DEFAULT", i); att_tup = TupleDescAttr(target_relation->rd_att, attrno - 1); if (!force_nulls && !att_tup->attisdropped) @@ -1379,6 +1399,8 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, } rte->values_lists = newValues; + pfree(attrnos); + return allReplaced; } @@ -3467,7 +3489,6 @@ RewriteQuery(Query *parsetree, List *rewrite_events) List *locks; List *product_queries; bool hasUpdate = false; - List *attrnos = NIL; int values_rte_index = 0; bool defaults_remaining = false; @@ -3517,11 +3538,10 @@ RewriteQuery(Query *parsetree, List *rewrite_events) parsetree->commandType, parsetree->override, rt_entry_relation, - parsetree->resultRelation, - &attrnos); + parsetree->resultRelation); /* ... and the VALUES expression lists */ - if (!rewriteValuesRTE(parsetree, values_rte, - rt_entry_relation, attrnos, false)) + if (!rewriteValuesRTE(parsetree, values_rte, values_rte_index, + rt_entry_relation, false)) defaults_remaining = true; } else @@ -3532,7 +3552,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events) parsetree->commandType, parsetree->override, rt_entry_relation, - parsetree->resultRelation, NULL); + parsetree->resultRelation); } if (parsetree->onConflict && @@ -3543,8 +3563,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events) CMD_UPDATE, parsetree->override, rt_entry_relation, - parsetree->resultRelation, - NULL); + parsetree->resultRelation); } } else if (event == CMD_UPDATE) @@ -3554,7 +3573,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events) parsetree->commandType, parsetree->override, rt_entry_relation, - parsetree->resultRelation, NULL); + parsetree->resultRelation); } else if (event == CMD_DELETE) { @@ -3599,7 +3618,8 @@ RewriteQuery(Query *parsetree, List *rewrite_events) RangeTblEntry *values_rte = rt_fetch(values_rte_index, pt->rtable); - rewriteValuesRTE(pt, values_rte, rt_entry_relation, attrnos, + rewriteValuesRTE(pt, values_rte, values_rte_index, + rt_entry_relation, true); /* Force remaining defaults to NULL */ } } diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out index 7ee6a54102..6ce058f312 100644 --- a/src/test/regress/expected/updatable_views.out +++ b/src/test/regress/expected/updatable_views.out @@ -2791,6 +2791,7 @@ insert into base_tab_def_view values (12), (13); insert into base_tab_def_view values (14, default, default, default, default); insert into base_tab_def_view values (15, default, default, default, default), (16, default, default, default, default); +insert into base_tab_def_view values (17), (default); select * from base_tab_def order by a; a | b | c | d | e ----+---------------+---------------+--------------+--- @@ -2806,7 +2807,9 @@ select * from base_tab_def order by a; 14 | View default | Table default | View default | 15 | View default | Table default | View default | 16 | View default | Table default | View default | -(12 rows) + 17 | View default | Table default | View default | + | View default | Table default | View default | +(14 rows) -- Adding an INSTEAD OF trigger should cause NULLs to be inserted instead of -- table defaults, where there are no view defaults. @@ -2832,6 +2835,7 @@ insert into base_tab_def_view values (12), (13); insert into base_tab_def_view values (14, default, default, default, default); insert into base_tab_def_view values (15, default, default, default, default), (16, default, default, default, default); +insert into base_tab_def_view values (17), (default); select * from base_tab_def order by a; a | b | c | d | e ----+---------------+---------------+--------------+--- @@ -2847,7 +2851,9 @@ select * from base_tab_def order by a; 14 | View default | | View default | 15 | View default | | View default | 16 | View default | | View default | -(12 rows) + 17 | View default | | View default | + | View default | | View default | +(14 rows) -- Using an unconditional DO INSTEAD rule should also cause NULLs to be -- inserted where there are no view defaults. @@ -2866,6 +2872,7 @@ insert into base_tab_def_view values (12), (13); insert into base_tab_def_view values (14, default, default, default, default); insert into base_tab_def_view values (15, default, default, default, default), (16, default, default, default, default); +insert into base_tab_def_view values (17), (default); select * from base_tab_def order by a; a | b | c | d | e ----+---------------+---------------+--------------+--- @@ -2881,7 +2888,9 @@ select * from base_tab_def order by a; 14 | View default | | View default | 15 | View default | | View default | 16 | View default | | View default | -(12 rows) + 17 | View default | | View default | + | View default | | View default | +(14 rows) -- A DO ALSO rule should cause each row to be inserted twice. The first -- insert should behave the same as an auto-updatable view (using table @@ -2902,6 +2911,7 @@ insert into base_tab_def_view values (12), (13); insert into base_tab_def_view values (14, default, default, default, default); insert into base_tab_def_view values (15, default, default, default, default), (16, default, default, default, default); +insert into base_tab_def_view values (17), (default); select * from base_tab_def order by a, c NULLS LAST; a | b | c | d | e ----+---------------+---------------+--------------+--- @@ -2923,7 +2933,26 @@ select * from base_tab_def order by a, c NULLS LAST; 15 | View default | | View default | 16 | View default | Table default | View default | 16 | View default | | View default | -(18 rows) + 17 | View default | Table default | View default | + 17 | View default | | View default | + | View default | Table default | View default | + | View default | | View default | +(22 rows) drop view base_tab_def_view; drop table base_tab_def; +-- Test defaults with array assignments +create table base_tab (a serial, b int[], c text, d text default 'Table default'); +create view base_tab_view as select c, a, b from base_tab; +alter view base_tab_view alter column c set default 'View default'; +insert into base_tab_view (b[1], b[2], c, b[5], b[4], a, b[3]) +values (1, 2, default, 5, 4, default, 3), (10, 11, 'C value', 14, 13, 100, 12); +select * from base_tab order by a; + a | b | c | d +-----+------------------+--------------+--------------- + 1 | {1,2,3,4,5} | View default | Table default + 100 | {10,11,12,13,14} | C value | Table default +(2 rows) + +drop view base_tab_view; +drop table base_tab; diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql index 71c4c74336..f5f88a1116 100644 --- a/src/test/regress/sql/updatable_views.sql +++ b/src/test/regress/sql/updatable_views.sql @@ -1400,6 +1400,7 @@ insert into base_tab_def_view values (12), (13); insert into base_tab_def_view values (14, default, default, default, default); insert into base_tab_def_view values (15, default, default, default, default), (16, default, default, default, default); +insert into base_tab_def_view values (17), (default); select * from base_tab_def order by a; -- Adding an INSTEAD OF trigger should cause NULLs to be inserted instead of @@ -1426,6 +1427,7 @@ insert into base_tab_def_view values (12), (13); insert into base_tab_def_view values (14, default, default, default, default); insert into base_tab_def_view values (15, default, default, default, default), (16, default, default, default, default); +insert into base_tab_def_view values (17), (default); select * from base_tab_def order by a; -- Using an unconditional DO INSTEAD rule should also cause NULLs to be @@ -1445,6 +1447,7 @@ insert into base_tab_def_view values (12), (13); insert into base_tab_def_view values (14, default, default, default, default); insert into base_tab_def_view values (15, default, default, default, default), (16, default, default, default, default); +insert into base_tab_def_view values (17), (default); select * from base_tab_def order by a; -- A DO ALSO rule should cause each row to be inserted twice. The first @@ -1466,7 +1469,18 @@ insert into base_tab_def_view values (12), (13); insert into base_tab_def_view values (14, default, default, default, default); insert into base_tab_def_view values (15, default, default, default, default), (16, default, default, default, default); +insert into base_tab_def_view values (17), (default); select * from base_tab_def order by a, c NULLS LAST; drop view base_tab_def_view; drop table base_tab_def; + +-- Test defaults with array assignments +create table base_tab (a serial, b int[], c text, d text default 'Table default'); +create view base_tab_view as select c, a, b from base_tab; +alter view base_tab_view alter column c set default 'View default'; +insert into base_tab_view (b[1], b[2], c, b[5], b[4], a, b[3]) +values (1, 2, default, 5, 4, default, 3), (10, 11, 'C value', 14, 13, 100, 12); +select * from base_tab order by a; +drop view base_tab_view; +drop table base_tab;