On Tue, 12 Feb 2019 at 10:33, Dean Rasheed <dean.a.rash...@gmail.com> wrote:
> Here's an updated patch ...
So I pushed that. However, ...
Playing around with it some more, I realised that whilst this appeared
to fix the reported problem, it exposes another issue which is down to
the interaction between rewriteTargetListIU() and rewriteValuesRTE()
--- for an INSERT with a VALUES RTE, rewriteTargetListIU() computes a
list of target attribute numbers (attrno_list) from the targetlist in
its original order, which rewriteValuesRTE() then relies on being the
same length (and in the same order) as each of the lists in the VALUES
RTE. That's OK for the initial invocation of those functions, but it
breaks down when they're recursively invoked for auto-updatable views.
For example, the following test-case, based on a slight variation of
the new regression tests:
create table foo (a int default 1, b int);
create view foo_v as select * from foo;
alter view foo_v alter column b set default 2;
insert into foo_v values (default), (default);
triggers the following Assert in rewriteValuesRTE():
/* Check list lengths (we can assume all the VALUES sublists are alike) */
Assert(list_length(attrnos) == list_length(linitial(rte->values_lists)));
What's happening is that the initial targetlist, which contains just 1
entry for the column a, gains another entry to set the default for
column b from the view. Then, when it recurses into
rewriteTargetListIU()/rewriteValuesRTE() for the base relation, the
size of the targetlist (now 2) no longer matches the sizes of the
VALUES RTE lists (1).
I think that that failed Assert was unreachable prior to 41531e42d3,
because the old version of rewriteValuesRTE() would always replace all
unresolved DEFAULT items with NULLs, so when it recursed into
rewriteValuesRTE() for the base relation, it would always bail out
early because there would be no DEFAULT items left, and so it would
fail to notice the list size mismatch.
My first thought was that this could be fixed by having
rewriteTargetListIU() compute attrno_list using only those targetlist
entries that refer to the VALUES RTE, and thus omit any new entries
added due to view defaults. That doesn't work though, because that's
not the only way that a list size mismatch can be triggered --- it's
also possible that rewriteTargetListIU() will merge together
targetlist entries, for example if they're array element assignments
referring to the same column, in which case the rewritten targetlist
could be shorter than the VALUES RTE lists, and attempting to compute
attrno_list from it correctly would be trickier.
So actually, I think the right way to fix this is to give up trying to
compute attrno_list in rewriteTargetListIU(), and have
rewriteValuesRTE() work out the attribute assignments itself for each
column of the VALUES RTE by examining the rewritten targetlist. That
looks to be quite straightforward, and results in a cleaner separation
of logic between the 2 functions, per the attached patch.
I think that rewriteValuesRTE() should only ever see DEFAULT items for
columns that are simple assignments to columns of the target relation,
so it only needs to work out the target attribute numbers for TLEs
that contain simple Vars referring to the VALUES RTE. Any DEFAULT seen
in a column not matching that would be an error, but actually I think
such a thing ought to be a "can't happen" error because of the prior
checks during parse analysis, so I've used elog() to report if this
does happen.
Incidentally, it looks like the part of rewriteValuesRTE()'s comment
about subscripted and field assignment has been wrong since
a3c7a993d5, so I've attempted to clarify that. That will need to look
different pre-9.6, I think.
Thoughts?
Regards,
Dean
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
new file mode 100644
index 7eb41ff..73fac75
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -67,14 +67,13 @@ static List *rewriteTargetListIU(List *t
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, boo
* 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, Range
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, Range
List *sublist = (List *) lfirst(lc);
List *newList = NIL;
ListCell *lc2;
- ListCell *lc3;
+ int i;
- forboth(lc2, sublist, lc3, attrnos)
+ Assert(list_length(sublist) == numattrs);
+
+ 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, Range
}
rte->values_lists = newValues;
+ pfree(attrnos);
+
return allReplaced;
}
@@ -3467,7 +3489,6 @@ RewriteQuery(Query *parsetree, List *rew
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 *rew
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 *rew
parsetree->commandType,
parsetree->override,
rt_entry_relation,
- parsetree->resultRelation, NULL);
+ parsetree->resultRelation);
}
if (parsetree->onConflict &&
@@ -3543,8 +3563,7 @@ RewriteQuery(Query *parsetree, List *rew
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 *rew
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 *rew
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
new file mode 100644
index 7ee6a54..6ce058f
--- 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
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
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
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
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
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
new file mode 100644
index 71c4c74..f5f88a1
--- 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
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
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
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
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;
