BUG #15623: Inconsistent use of default for updatable view
The following bug has been logged on the website:
Bug reference: 15623
Logged by: Roger Curley
Email address: rocurley@gmail.com
PostgreSQL version: 11.1
Operating system: Ubuntu 11.1
Description:
Steps to reproduce (run in psql shell):
```
DROP TABLE IF EXISTS test CASCADE;
CREATE TABLE test (
id int PRIMARY KEY,
value int DEFAULT 0
);
CREATE VIEW test_view AS (SELECT * FROM test);
INSERT INTO test_view VALUES (1, DEFAULT), (2, DEFAULT);
INSERT INTO test VALUES (3, DEFAULT), (4, DEFAULT);
INSERT INTO test_view VALUES (5, DEFAULT);
SELECT * FROM test;
```
Result:
```
id | value
----+-------
1 |
2 |
3 | 0
4 | 0
5 | 0
```
Expected Result:
```
id | value
----+-------
1 | 0
2 | 0
3 | 0
4 | 0
5 | 0
```
In particular, it's surprising that inserting 1 row into an updatable view
uses the table default, while inserting 2 uses null.
Hi,
On 2019/02/08 6:42, PG Bug reporting form wrote:
The following bug has been logged on the website:
Bug reference: 15623
Logged by: Roger Curley
Email address: rocurley@gmail.com
PostgreSQL version: 11.1
Operating system: Ubuntu 11.1
Description:Steps to reproduce (run in psql shell):
```
DROP TABLE IF EXISTS test CASCADE;
CREATE TABLE test (
id int PRIMARY KEY,
value int DEFAULT 0
);
CREATE VIEW test_view AS (SELECT * FROM test);INSERT INTO test_view VALUES (1, DEFAULT), (2, DEFAULT);
INSERT INTO test VALUES (3, DEFAULT), (4, DEFAULT);
INSERT INTO test_view VALUES (5, DEFAULT);
SELECT * FROM test;
```Result:
```
id | value
----+-------
1 |
2 |
3 | 0
4 | 0
5 | 0
```Expected Result:
```
id | value
----+-------
1 | 0
2 | 0
3 | 0
4 | 0
5 | 0
```
In particular, it's surprising that inserting 1 row into an updatable view
uses the table default, while inserting 2 uses null.
Thanks for the report. Seems odd indeed.
Looking into this, the reason it works when inserting just one row vs.
more than one row is that those two cases are handled by nearby but
different pieces of code. The code that handles multiple rows seems buggy
as seen in the above example. Specifically, I think the bug is in
rewriteValuesRTE() which is a function to replace the default placeholders
in the input rows by the default values as defined for the target
relation. It is called twice when inserting via the view -- first for the
view relation and then again for the underlying table. This arrangement
seems to work correctly if the view specifies its own defaults for columns
(assuming that it's okay for the view's defaults to override the
underlying base table's). If there are no view-specified defaults, then
rewriteValuesRTE replaces the default placeholders in the input row by
NULL constants when called for the first time with the view as target
relation and the next invocation for the underlying table finds that it
has no work to do, so its defaults are not filled.
Attached find a patch that adjusts rewriteValuesRTE to not replace the
default placeholder if the view has no default value for a given column.
Also, adds a test in updatable_views.sql.
Thanks,
Amit
Attachments:
view-insert-null-default-fix.patchtext/plain; charset=UTF-8; name=view-insert-null-default-fix.patchDownload
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 0338e4e1ad..4643257982 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1223,6 +1223,11 @@ searchForDefault(RangeTblEntry *rte)
* 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".
+ *
+ * If target relation is a view and the default value for a given column
+ * is NULL, then we defer replacing the corresponding DEFAULT item in the
+ * VALUES list to when the list is rewritten with the underlying table as
+ * the target relation.
*/
static void
rewriteValuesRTE(RangeTblEntry *rte, Relation target_relation, List *attrnos)
@@ -1268,8 +1273,17 @@ rewriteValuesRTE(RangeTblEntry *rte, Relation target_relation, List *attrnos)
/*
* If there is no default (ie, default is effectively NULL),
- * we've got to explicitly set the column to NULL.
+ * we've got to explicitly set the column to NULL. But if
+ * the target relation is a view, we should not finalize the
+ * value just yet.
*/
+ if (!new_expr &&
+ target_relation->rd_rel->relkind == RELKIND_VIEW)
+ {
+ newList = lappend(newList, col);
+ continue;
+ }
+
if (!new_expr)
{
new_expr = (Node *) makeConst(att_tup->atttypid,
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
index 420c5a54f2..be608e891e 100644
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -2772,3 +2772,45 @@ drop view rw_view1;
drop table base_tbl;
drop user regress_view_user1;
drop user regress_view_user2;
+-- test that default values of underlying table are properly applied
+-- when inserting via a view over the table
+create table base_tab_def (a int, b int default 0);
+create view base_tab_def_view as select * from base_tab_def;
+-- will insert 0 for b
+insert into base_tab_def_view values (1);
+insert into base_tab_def_view values (2), (3);
+insert into base_tab_def_view values (4, default), (5, default);
+select * from base_tab_def;
+ a | b
+---+---
+ 1 | 0
+ 2 | 0
+ 3 | 0
+ 4 | 0
+ 5 | 0
+(5 rows)
+
+alter view base_tab_def_view alter b set default 1000;
+-- will insert 1000 for b
+insert into base_tab_def_view values (6), (7);
+insert into base_tab_def_view values (8, default), (9, default);
+-- will insert 0 for b
+insert into base_tab_def values (8), (9);
+select * from base_tab_def;
+ a | b
+---+------
+ 1 | 0
+ 2 | 0
+ 3 | 0
+ 4 | 0
+ 5 | 0
+ 6 | 1000
+ 7 | 1000
+ 8 | 1000
+ 9 | 1000
+ 8 | 0
+ 9 | 0
+(11 rows)
+
+drop table base_tab_def cascade;
+NOTICE: drop cascades to view base_tab_def_view
diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql
index dc6d5cbe35..7e559ffe57 100644
--- a/src/test/regress/sql/updatable_views.sql
+++ b/src/test/regress/sql/updatable_views.sql
@@ -1379,3 +1379,21 @@ drop view rw_view1;
drop table base_tbl;
drop user regress_view_user1;
drop user regress_view_user2;
+
+-- test that default values of underlying table are properly applied
+-- when inserting via a view over the table
+create table base_tab_def (a int, b int default 0);
+create view base_tab_def_view as select * from base_tab_def;
+-- will insert 0 for b
+insert into base_tab_def_view values (1);
+insert into base_tab_def_view values (2), (3);
+insert into base_tab_def_view values (4, default), (5, default);
+select * from base_tab_def;
+alter view base_tab_def_view alter b set default 1000;
+-- will insert 1000 for b
+insert into base_tab_def_view values (6), (7);
+insert into base_tab_def_view values (8, default), (9, default);
+-- will insert 0 for b
+insert into base_tab_def values (8), (9);
+select * from base_tab_def;
+drop table base_tab_def cascade;
On Fri, 8 Feb 2019 at 05:07, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
Thanks for the report. Seems odd indeed.
Hmm, indeed. That seems to have been broken ever since updatable views
were added.
Looking into this, the reason it works when inserting just one row vs.
more than one row is that those two cases are handled by nearby but
different pieces of code. The code that handles multiple rows seems buggy
as seen in the above example. Specifically, I think the bug is in
rewriteValuesRTE() which is a function to replace the default placeholders
in the input rows by the default values as defined for the target
relation. It is called twice when inserting via the view -- first for the
view relation and then again for the underlying table.
Right, except when the view is trigger-updatable. In that case, we do
have to explicitly set the column value to NULL when
rewriteValuesRTE() is called for the view, because it won't be called
again for the underlying table -- it is the trigger's responsibility
to work how (or indeed if) to update the underlying table. IOW, you
need to also use view_has_instead_trigger() to check the view,
otherwise your patch breaks this case:
DROP TABLE IF EXISTS test CASCADE;
CREATE TABLE test (
id int PRIMARY KEY,
value int DEFAULT 0
);
CREATE VIEW test_view AS (SELECT * FROM test);
CREATE OR REPLACE FUNCTION test_view_ins() RETURNS trigger
AS
$$
BEGIN
INSERT INTO test VALUES (NEW.id, NEW.value);
RETURN NEW;
END;
$$
LANGUAGE plpgsql;
CREATE TRIGGER test_view_trig INSTEAD OF INSERT ON test_view
FOR EACH ROW EXECUTE FUNCTION test_view_ins();
INSERT INTO test_view VALUES (1, DEFAULT), (2, DEFAULT);
ERROR: unrecognized node type: 142
While playing around with this, I noticed a related bug affecting the
new identity columns feature. I've not investigated it fully, but It
looks almost the same -- if the column is an identity column, and
we're inserting a multi-row VALUES set containing DEFAULTS, they will
get rewritten to NULLs which will then lead to an error if overriding
the generated value isn't allowed:
DROP TABLE IF EXISTS foo CASCADE;
CREATE TABLE foo
(
a int,
b int GENERATED ALWAYS AS IDENTITY
);
INSERT INTO foo VALUES (1,DEFAULT); -- OK
INSERT INTO foo VALUES (2,DEFAULT),(3,DEFAULT); -- Fails
I think fixing that should be tackled separately, because it may turn
out to be subtly different, but it definitely looks like another bug.
Regards,
Dean
Thanks for looking at this.
On Fri, Feb 8, 2019 at 8:01 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On Fri, 8 Feb 2019 at 05:07, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
Thanks for the report. Seems odd indeed.
Hmm, indeed. That seems to have been broken ever since updatable views
were added.Looking into this, the reason it works when inserting just one row vs.
more than one row is that those two cases are handled by nearby but
different pieces of code. The code that handles multiple rows seems buggy
as seen in the above example. Specifically, I think the bug is in
rewriteValuesRTE() which is a function to replace the default placeholders
in the input rows by the default values as defined for the target
relation. It is called twice when inserting via the view -- first for the
view relation and then again for the underlying table.Right, except when the view is trigger-updatable. In that case, we do
have to explicitly set the column value to NULL when
rewriteValuesRTE() is called for the view, because it won't be called
again for the underlying table -- it is the trigger's responsibility
to work how (or indeed if) to update the underlying table. IOW, you
need to also use view_has_instead_trigger() to check the view,
otherwise your patch breaks this case:DROP TABLE IF EXISTS test CASCADE;
CREATE TABLE test (
id int PRIMARY KEY,
value int DEFAULT 0
);
CREATE VIEW test_view AS (SELECT * FROM test);CREATE OR REPLACE FUNCTION test_view_ins() RETURNS trigger
AS
$$
BEGIN
INSERT INTO test VALUES (NEW.id, NEW.value);
RETURN NEW;
END;
$$
LANGUAGE plpgsql;CREATE TRIGGER test_view_trig INSTEAD OF INSERT ON test_view
FOR EACH ROW EXECUTE FUNCTION test_view_ins();INSERT INTO test_view VALUES (1, DEFAULT), (2, DEFAULT);
ERROR: unrecognized node type: 142
Oops, I missed this bit. Updated the patch per your suggestion and
expanded the test case to exercise this.
While playing around with this, I noticed a related bug affecting the
new identity columns feature. I've not investigated it fully, but It
looks almost the same -- if the column is an identity column, and
we're inserting a multi-row VALUES set containing DEFAULTS, they will
get rewritten to NULLs which will then lead to an error if overriding
the generated value isn't allowed:DROP TABLE IF EXISTS foo CASCADE;
CREATE TABLE foo
(
a int,
b int GENERATED ALWAYS AS IDENTITY
);INSERT INTO foo VALUES (1,DEFAULT); -- OK
INSERT INTO foo VALUES (2,DEFAULT),(3,DEFAULT); -- FailsI think fixing that should be tackled separately, because it may turn
out to be subtly different, but it definitely looks like another bug.
I haven't looked into the details of this, but agree about raising a
thread on -hackers about it.
Thanks,
Amit
Attachments:
view-insert-null-default-fix-v2.patchapplication/octet-stream; name=view-insert-null-default-fix-v2.patchDownload
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 0338e4e1ad..7abaeda000 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -1223,6 +1223,11 @@ searchForDefault(RangeTblEntry *rte)
* 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".
+ *
+ * If target relation is a view and the default value for a given column
+ * is NULL, then we defer replacing the corresponding DEFAULT item in the
+ * VALUES list to when the list is rewritten with the underlying table as
+ * the target relation.
*/
static void
rewriteValuesRTE(RangeTblEntry *rte, Relation target_relation, List *attrnos)
@@ -1268,8 +1273,19 @@ rewriteValuesRTE(RangeTblEntry *rte, Relation target_relation, List *attrnos)
/*
* If there is no default (ie, default is effectively NULL),
- * we've got to explicitly set the column to NULL.
+ * we've got to explicitly set the column to NULL. But if
+ * the target relation is a view and the query will be
+ * rewritten to be executed on the base table, we should
+ * not finalize the value just yet.
*/
+ if (!new_expr &&
+ target_relation->rd_rel->relkind == RELKIND_VIEW &&
+ !view_has_instead_trigger(target_relation, CMD_INSERT))
+ {
+ newList = lappend(newList, col);
+ continue;
+ }
+
if (!new_expr)
{
new_expr = (Node *) makeConst(att_tup->atttypid,
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
index 420c5a54f2..f5ad5630d9 100644
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -2772,3 +2772,78 @@ drop view rw_view1;
drop table base_tbl;
drop user regress_view_user1;
drop user regress_view_user2;
+-- test that default values of underlying table are properly applied
+-- when inserting via a view over the table
+create table base_tab_def (a int, b int default 0);
+create view base_tab_def_view as select * from base_tab_def;
+-- will insert 0 for b
+insert into base_tab_def_view values (1);
+insert into base_tab_def_view values (2), (3);
+insert into base_tab_def_view values (4, default), (5, default);
+select * from base_tab_def order by 1;
+ a | b
+---+---
+ 1 | 0
+ 2 | 0
+ 3 | 0
+ 4 | 0
+ 5 | 0
+(5 rows)
+
+alter view base_tab_def_view alter b set default 1000;
+-- will insert 1000 for b
+insert into base_tab_def_view values (6), (7);
+insert into base_tab_def_view values (8, default), (9, default);
+-- will insert 0 for b
+insert into base_tab_def values (10), (11);
+select * from base_tab_def order by 1;
+ a | b
+----+------
+ 1 | 0
+ 2 | 0
+ 3 | 0
+ 4 | 0
+ 5 | 0
+ 6 | 1000
+ 7 | 1000
+ 8 | 1000
+ 9 | 1000
+ 10 | 0
+ 11 | 0
+(11 rows)
+
+alter view base_tab_def_view alter b drop default;
+create function base_tab_def_view_instrig_func() returns trigger
+as
+$$
+begin
+ insert into base_tab_def values (new.a, new.b);
+ return new;
+end;
+$$
+language plpgsql;
+create trigger base_tab_def_view_instrig instead of insert on base_tab_def_view
+ for each row execute function base_tab_def_view_instrig_func();
+-- will insert null, the view's default value, for b
+insert into base_tab_def_view values (12, default), (13, default);
+select * from base_tab_def order by 1;
+ a | b
+----+------
+ 1 | 0
+ 2 | 0
+ 3 | 0
+ 4 | 0
+ 5 | 0
+ 6 | 1000
+ 7 | 1000
+ 8 | 1000
+ 9 | 1000
+ 10 | 0
+ 11 | 0
+ 12 |
+ 13 |
+(13 rows)
+
+drop table base_tab_def cascade;
+NOTICE: drop cascades to view base_tab_def_view
+drop function base_tab_def_view_instrig_func;
diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql
index dc6d5cbe35..7cfab2de38 100644
--- a/src/test/regress/sql/updatable_views.sql
+++ b/src/test/regress/sql/updatable_views.sql
@@ -1379,3 +1379,37 @@ drop view rw_view1;
drop table base_tbl;
drop user regress_view_user1;
drop user regress_view_user2;
+
+-- test that default values of underlying table are properly applied
+-- when inserting via a view over the table
+create table base_tab_def (a int, b int default 0);
+create view base_tab_def_view as select * from base_tab_def;
+-- will insert 0 for b
+insert into base_tab_def_view values (1);
+insert into base_tab_def_view values (2), (3);
+insert into base_tab_def_view values (4, default), (5, default);
+select * from base_tab_def order by 1;
+alter view base_tab_def_view alter b set default 1000;
+-- will insert 1000 for b
+insert into base_tab_def_view values (6), (7);
+insert into base_tab_def_view values (8, default), (9, default);
+-- will insert 0 for b
+insert into base_tab_def values (10), (11);
+select * from base_tab_def order by 1;
+alter view base_tab_def_view alter b drop default;
+create function base_tab_def_view_instrig_func() returns trigger
+as
+$$
+begin
+ insert into base_tab_def values (new.a, new.b);
+ return new;
+end;
+$$
+language plpgsql;
+create trigger base_tab_def_view_instrig instead of insert on base_tab_def_view
+ for each row execute function base_tab_def_view_instrig_func();
+-- will insert null, the view's default value, for b
+insert into base_tab_def_view values (12, default), (13, default);
+select * from base_tab_def order by 1;
+drop table base_tab_def cascade;
+drop function base_tab_def_view_instrig_func;
On Sat, 9 Feb 2019 at 16:57, Amit Langote <amitlangote09@gmail.com> wrote:
On Fri, Feb 8, 2019 at 8:01 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On Fri, 8 Feb 2019 at 05:07, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
Looking into this, the reason it works when inserting just one row vs.
more than one row is that those two cases are handled by nearby but
different pieces of code. The code that handles multiple rows seems buggy
as seen in the above example. Specifically, I think the bug is in
rewriteValuesRTE() which is a function to replace the default placeholders
in the input rows by the default values as defined for the target
relation. It is called twice when inserting via the view -- first for the
view relation and then again for the underlying table.Right, except when the view is trigger-updatable...
Oops, I missed this bit. Updated the patch per your suggestion and
expanded the test case to exercise this.
Unfortunately, that's still not quite right because it makes the
behaviour of single- and multi-row inserts inconsistent for
rule-updatable views. Attached is an updated patch that fixes that. I
adjusted the tests a bit to try to make it clearer which defaults get
applied, and test all possibilities.
However, this is still not the end of the story, because it doesn't
fix the fact that, if the view has a DO ALSO rule on it, single-row
inserts behave differently from multi-row inserts. In that case, each
insert becomes 2 inserts, and defaults need to be treated differently
in each of the 2 queries. That's going to need a little more thought.
Regards,
Dean
Attachments:
view-insert-null-default-fix-v3.patchapplication/octet-stream; name=view-insert-null-default-fix-v3.patchDownload
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
new file mode 100644
index 0338e4e..eedf003
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -73,8 +73,8 @@ static TargetEntry *process_matched_tle(
TargetEntry *prior_tle,
const char *attrName);
static Node *get_assignment_input(Node *node);
-static void rewriteValuesRTE(RangeTblEntry *rte, Relation target_relation,
- List *attrnos);
+static void rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte,
+ Relation target_relation, List *attrnos);
static void markQueryForLocking(Query *qry, Node *jtnode,
LockClauseStrength strength, LockWaitPolicy waitPolicy,
bool pushedDown);
@@ -1225,10 +1225,12 @@ searchForDefault(RangeTblEntry *rte)
* reject any such attempt with "multiple assignments to same column".
*/
static void
-rewriteValuesRTE(RangeTblEntry *rte, Relation target_relation, List *attrnos)
+rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte,
+ Relation target_relation, List *attrnos)
{
List *newValues;
ListCell *lc;
+ bool autoUpdatableView;
/*
* Rebuilding all the lists is a pretty expensive proposition in a big
@@ -1241,6 +1243,53 @@ rewriteValuesRTE(RangeTblEntry *rte, Rel
/* Check list lengths (we can assume all the VALUES sublists are alike) */
Assert(list_length(attrnos) == list_length(linitial(rte->values_lists)));
+ /*
+ * If we see a DEFAULT placeholder in the VALUES list for a column that
+ * has no default, we explicitly set the column to NULL except if the
+ * target relation is an auto-updatable view, in which case we leave the
+ * value untouched so that any default on the underlying base relation
+ * will be applied instead.
+ *
+ * This exception only applies to auto-updatable views --- for rule- and
+ * trigger-updatable views, it is the rule/trigger's responsibility to
+ * work out whether to insert default values into the base relation, and
+ * we must explicitly set the column to NULL, as for tables.
+ */
+ autoUpdatableView = false;
+ if (target_relation->rd_rel->relkind == RELKIND_VIEW &&
+ !view_has_instead_trigger(target_relation, CMD_INSERT))
+ {
+ List *locks;
+ bool hasUpdate;
+ bool found;
+ ListCell *l;
+
+ /* Look for an unconditional DO INSTEAD rule */
+ locks = matchLocks(CMD_INSERT, target_relation->rd_rules,
+ parsetree->resultRelation, parsetree, &hasUpdate);
+
+ found = false;
+ foreach(l, locks)
+ {
+ RewriteRule *rule_lock = (RewriteRule *) lfirst(l);
+
+ if (rule_lock->isInstead &&
+ rule_lock->qual == NULL)
+ {
+ found = true;
+ break;
+ }
+ }
+
+ /*
+ * If we didn't find an unconditional DO INSTEAD rule, assume that the
+ * view is auto-updatable. If it isn't, rewriteTargetView() will
+ * throw an error.
+ */
+ if (!found)
+ autoUpdatableView = true;
+ }
+
newValues = NIL;
foreach(lc, rte->values_lists)
{
@@ -1268,10 +1317,18 @@ rewriteValuesRTE(RangeTblEntry *rte, Rel
/*
* If there is no default (ie, default is effectively NULL),
- * we've got to explicitly set the column to NULL.
+ * we've got to explicitly set the column to NULL, unless the
+ * target relation is an auto-updatable view.
*/
if (!new_expr)
{
+ if (autoUpdatableView)
+ {
+ /* Leave the value untouched */
+ newList = lappend(newList, col);
+ continue;
+ }
+
new_expr = (Node *) makeConst(att_tup->atttypid,
-1,
att_tup->attcollation,
@@ -3432,7 +3489,8 @@ RewriteQuery(Query *parsetree, List *rew
parsetree->resultRelation,
&attrnos);
/* ... and the VALUES expression lists */
- rewriteValuesRTE(values_rte, rt_entry_relation, attrnos);
+ rewriteValuesRTE(parsetree, values_rte, rt_entry_relation,
+ attrnos);
}
else
{
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
new file mode 100644
index 420c5a5..bc22033
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -2772,3 +2772,111 @@ drop view rw_view1;
drop table base_tbl;
drop user regress_view_user1;
drop user regress_view_user2;
+-- Test single- and multi-row inserts with table and view defaults
+create table base_tab_def (a int, b text default 'Table default',
+ c text default 'Table default', d text, e text);
+create view base_tab_def_view as select * from base_tab_def;
+alter view base_tab_def_view alter b set default 'View default';
+alter view base_tab_def_view alter d set default 'View default';
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by 1;
+ a | b | c | d | e
+----+---------------+---------------+--------------+---
+ 1 | Table default | Table default | |
+ 2 | Table default | Table default | |
+ 3 | Table default | Table default | |
+ 4 | Table default | Table default | |
+ 5 | Table default | Table default | |
+ 6 | Table default | Table default | |
+ 11 | View default | Table default | View default |
+ 12 | View default | Table default | View default |
+ 13 | View default | Table default | View default |
+ 14 | View default | Table default | View default |
+ 15 | View default | Table default | View default |
+ 16 | View default | Table default | View default |
+(12 rows)
+
+-- INSTEAD OF trigger inserts NULLs rather than the table defaults
+create function base_tab_def_view_instrig_func() returns trigger
+as
+$$
+begin
+ insert into base_tab_def values (new.a, new.b, new.c, new.d, new.e);
+ return new;
+end;
+$$
+language plpgsql;
+create trigger base_tab_def_view_instrig instead of insert on base_tab_def_view
+ for each row execute function base_tab_def_view_instrig_func();
+truncate base_tab_def;
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by 1;
+ a | b | c | d | e
+----+---------------+---------------+--------------+---
+ 1 | Table default | Table default | |
+ 2 | Table default | Table default | |
+ 3 | Table default | Table default | |
+ 4 | Table default | Table default | |
+ 5 | Table default | Table default | |
+ 6 | Table default | Table default | |
+ 11 | View default | | View default |
+ 12 | View default | | View default |
+ 13 | View default | | View default |
+ 14 | View default | | View default |
+ 15 | View default | | View default |
+ 16 | View default | | View default |
+(12 rows)
+
+-- Unconditional DO INSTEAD rule also inserts NULLs
+drop trigger base_tab_def_view_instrig on base_tab_def_view;
+drop function base_tab_def_view_instrig_func;
+create rule base_tab_def_view_ins_rule as on insert to base_tab_def_view
+ do instead insert into base_tab_def values (new.a, new.b, new.c, new.d, new.e);
+truncate base_tab_def;
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by 1;
+ a | b | c | d | e
+----+---------------+---------------+--------------+---
+ 1 | Table default | Table default | |
+ 2 | Table default | Table default | |
+ 3 | Table default | Table default | |
+ 4 | Table default | Table default | |
+ 5 | Table default | Table default | |
+ 6 | Table default | Table default | |
+ 11 | View default | | View default |
+ 12 | View default | | View default |
+ 13 | View default | | View default |
+ 14 | View default | | View default |
+ 15 | View default | | View default |
+ 16 | View default | | View default |
+(12 rows)
+
+drop view base_tab_def_view;
+drop table base_tab_def;
diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql
new file mode 100644
index dc6d5cb..0276286
--- a/src/test/regress/sql/updatable_views.sql
+++ b/src/test/regress/sql/updatable_views.sql
@@ -1379,3 +1379,66 @@ drop view rw_view1;
drop table base_tbl;
drop user regress_view_user1;
drop user regress_view_user2;
+
+-- Test single- and multi-row inserts with table and view defaults
+create table base_tab_def (a int, b text default 'Table default',
+ c text default 'Table default', d text, e text);
+create view base_tab_def_view as select * from base_tab_def;
+alter view base_tab_def_view alter b set default 'View default';
+alter view base_tab_def_view alter d set default 'View default';
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by 1;
+
+-- INSTEAD OF trigger inserts NULLs rather than the table defaults
+create function base_tab_def_view_instrig_func() returns trigger
+as
+$$
+begin
+ insert into base_tab_def values (new.a, new.b, new.c, new.d, new.e);
+ return new;
+end;
+$$
+language plpgsql;
+create trigger base_tab_def_view_instrig instead of insert on base_tab_def_view
+ for each row execute function base_tab_def_view_instrig_func();
+truncate base_tab_def;
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by 1;
+
+-- Unconditional DO INSTEAD rule also inserts NULLs
+drop trigger base_tab_def_view_instrig on base_tab_def_view;
+drop function base_tab_def_view_instrig_func;
+create rule base_tab_def_view_ins_rule as on insert to base_tab_def_view
+ do instead insert into base_tab_def values (new.a, new.b, new.c, new.d, new.e);
+truncate base_tab_def;
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by 1;
+drop view base_tab_def_view;
+drop table base_tab_def;
On Sun, 10 Feb 2019 at 00:48, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
However, this is still not the end of the story, because it doesn't
fix the fact that, if the view has a DO ALSO rule on it, single-row
inserts behave differently from multi-row inserts. In that case, each
insert becomes 2 inserts, and defaults need to be treated differently
in each of the 2 queries. That's going to need a little more thought.
Here's an updated patch to handle that case.
In case it's not obvious, I'm not intending to try to get this into
next week's updates -- more time is needed to be sure of this fix, and
more pairs of eyes would definitely be helpful, once those updates
have been shipped.
Regards,
Dean
Attachments:
view-insert-null-default-fix-v4.patchapplication/octet-stream; name=view-insert-null-default-fix-v4.patchDownload
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
new file mode 100644
index 0338e4e..f61fd25
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -73,8 +73,8 @@ static TargetEntry *process_matched_tle(
TargetEntry *prior_tle,
const char *attrName);
static Node *get_assignment_input(Node *node);
-static void rewriteValuesRTE(RangeTblEntry *rte, Relation target_relation,
- List *attrnos);
+static bool rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte,
+ Relation target_relation, List *attrnos);
static void markQueryForLocking(Query *qry, Node *jtnode,
LockClauseStrength strength, LockWaitPolicy waitPolicy,
bool pushedDown);
@@ -1223,12 +1223,19 @@ searchForDefault(RangeTblEntry *rte)
* 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".
+ *
+ * Returns true if all DEFAULT items were replaced, and false if some were
+ * left untouched (can happen for an auto-updatable view, allowing the
+ * defaults of the underlying base relation to be applied).
*/
-static void
-rewriteValuesRTE(RangeTblEntry *rte, Relation target_relation, List *attrnos)
+static bool
+rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte,
+ Relation target_relation, List *attrnos)
{
List *newValues;
ListCell *lc;
+ bool isAutoUpdatableView;
+ bool allReplaced;
/*
* Rebuilding all the lists is a pretty expensive proposition in a big
@@ -1236,12 +1243,60 @@ rewriteValuesRTE(RangeTblEntry *rte, Rel
* placeholders. So first scan to see if there are any.
*/
if (!searchForDefault(rte))
- return; /* nothing to do */
+ 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)));
+ /*
+ * If we see a DEFAULT placeholder in the VALUES list for a column that
+ * has no default, we explicitly set the column to NULL except if the
+ * target relation is an auto-updatable view, in which case we leave the
+ * value untouched so that any default on the underlying base relation
+ * will be applied instead.
+ *
+ * This exception only applies to auto-updatable views --- for rule- and
+ * trigger-updatable views, it is the rule/trigger's responsibility to
+ * work out whether to insert default values into the base relation, and
+ * we must explicitly set the column to NULL, as for tables.
+ */
+ isAutoUpdatableView = false;
+ if (target_relation->rd_rel->relkind == RELKIND_VIEW &&
+ !view_has_instead_trigger(target_relation, CMD_INSERT))
+ {
+ List *locks;
+ bool hasUpdate;
+ bool found;
+ ListCell *l;
+
+ /* Look for an unconditional DO INSTEAD rule */
+ locks = matchLocks(CMD_INSERT, target_relation->rd_rules,
+ parsetree->resultRelation, parsetree, &hasUpdate);
+
+ found = false;
+ foreach(l, locks)
+ {
+ RewriteRule *rule_lock = (RewriteRule *) lfirst(l);
+
+ if (rule_lock->isInstead &&
+ rule_lock->qual == NULL)
+ {
+ found = true;
+ break;
+ }
+ }
+
+ /*
+ * If we didn't find an unconditional DO INSTEAD rule, assume that the
+ * view is auto-updatable. If it isn't, rewriteTargetView() will
+ * throw an error.
+ */
+ if (!found)
+ isAutoUpdatableView = true;
+ }
+
newValues = NIL;
+ allReplaced = true;
foreach(lc, rte->values_lists)
{
List *sublist = (List *) lfirst(lc);
@@ -1268,10 +1323,19 @@ rewriteValuesRTE(RangeTblEntry *rte, Rel
/*
* If there is no default (ie, default is effectively NULL),
- * we've got to explicitly set the column to NULL.
+ * we've got to explicitly set the column to NULL, unless the
+ * target relation is an auto-updatable view.
*/
if (!new_expr)
{
+ if (isAutoUpdatableView)
+ {
+ /* Leave the value untouched */
+ newList = lappend(newList, col);
+ allReplaced = false;
+ continue;
+ }
+
new_expr = (Node *) makeConst(att_tup->atttypid,
-1,
att_tup->attcollation,
@@ -1296,6 +1360,76 @@ rewriteValuesRTE(RangeTblEntry *rte, Rel
newValues = lappend(newValues, newList);
}
rte->values_lists = newValues;
+
+ return allReplaced;
+}
+
+
+/*
+ * For an INSERT ... VALUES with a VALUES RTE (ie, multiple VALUES lists),
+ * replace any remaining DEFAULT items in the VALUES lists with NULLs.
+ *
+ * This is used after rewriteValuesRTE(), if it left any DEFAULT items
+ * untouched (because the target relation was an auto-updatable view) and the
+ * target relation is no longer an auto-updatable view. This applies to the
+ * product queries when an automatically updatable view has DO ALSO rules
+ * attached -- the original query is turned into an insert on the base
+ * relation, using that relation to replace any remaining defaults, but any
+ * product queries refer to the rule action, so any remaining defaults there
+ * must be set to NULL.
+ */
+static void
+rewriteValuesRTEFinal(RangeTblEntry *rte, Relation target_relation,
+ List *attrnos)
+{
+ List *newValues;
+ ListCell *lc;
+
+ /* Check list lengths (we can assume all the VALUES sublists are alike) */
+ Assert(list_length(attrnos) == list_length(linitial(rte->values_lists)));
+
+ newValues = NIL;
+ foreach(lc, rte->values_lists)
+ {
+ List *sublist = (List *) lfirst(lc);
+ List *newList = NIL;
+ ListCell *lc2;
+ ListCell *lc3;
+
+ forboth(lc2, sublist, lc3, attrnos)
+ {
+ Node *col = (Node *) lfirst(lc2);
+ int attrno = lfirst_int(lc3);
+
+ if (IsA(col, SetToDefault))
+ {
+ Form_pg_attribute att_tup;
+ Node *new_expr;
+
+ att_tup = TupleDescAttr(target_relation->rd_att, attrno - 1);
+ new_expr = (Node *) makeConst(att_tup->atttypid,
+ -1,
+ att_tup->attcollation,
+ att_tup->attlen,
+ (Datum) 0,
+ true, /* isnull */
+ att_tup->attbyval);
+ /* this is to catch a NOT NULL domain constraint */
+ new_expr = coerce_to_domain(new_expr,
+ InvalidOid, -1,
+ att_tup->atttypid,
+ COERCION_IMPLICIT,
+ COERCE_IMPLICIT_CAST,
+ -1,
+ false);
+ newList = lappend(newList, new_expr);
+ }
+ else
+ newList = lappend(newList, col);
+ }
+ newValues = lappend(newValues, newList);
+ }
+ rte->values_lists = newValues;
}
@@ -3383,6 +3517,9 @@ RewriteQuery(Query *parsetree, List *rew
List *locks;
List *product_queries;
bool hasUpdate = false;
+ List *attrnos = NIL;
+ int values_rte_index = 0;
+ bool finalize_values_rte = false;
result_relation = parsetree->resultRelation;
Assert(result_relation != 0);
@@ -3416,14 +3553,15 @@ RewriteQuery(Query *parsetree, List *rew
parsetree->rtable);
if (rte->rtekind == RTE_VALUES)
+ {
values_rte = rte;
+ values_rte_index = rtr->rtindex;
+ }
}
}
if (values_rte)
{
- List *attrnos;
-
/* Process the main targetlist ... */
parsetree->targetList = rewriteTargetListIU(parsetree->targetList,
parsetree->commandType,
@@ -3432,7 +3570,9 @@ RewriteQuery(Query *parsetree, List *rew
parsetree->resultRelation,
&attrnos);
/* ... and the VALUES expression lists */
- rewriteValuesRTE(values_rte, rt_entry_relation, attrnos);
+ if (!rewriteValuesRTE(parsetree, values_rte,
+ rt_entry_relation, attrnos))
+ finalize_values_rte = true;
}
else
{
@@ -3488,6 +3628,32 @@ RewriteQuery(Query *parsetree, List *rew
&qual_product);
/*
+ * If we have a VALUES RTE with any remaining untouched DEFAULT items,
+ * and we got any product queries, finalize the VALUES RTE for each
+ * product query (replacing the remaining DEFAULT items with NULLs).
+ * We don't do this for the original query, because we know that it
+ * must be an auto-insert on a view, and so should use the base
+ * relation's defaults for any remaining DEFAULT items.
+ */
+ if (finalize_values_rte && product_queries != NIL)
+ {
+ ListCell *n;
+
+ /*
+ * Each product query has its own copy of the VALUES RTE at the
+ * same index in the rangetable, so we must finalize each one.
+ */
+ foreach(n, product_queries)
+ {
+ Query *pt = (Query *) lfirst(n);
+ RangeTblEntry *values_rte = rt_fetch(values_rte_index,
+ pt->rtable);
+
+ rewriteValuesRTEFinal(values_rte, rt_entry_relation, attrnos);
+ }
+ }
+
+ /*
* If there were no INSTEAD rules, and the target relation is a view
* without any INSTEAD OF triggers, see if the view can be
* automatically updated. If so, we perform the necessary query
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
new file mode 100644
index 420c5a5..ed07c1a
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -2772,3 +2772,156 @@ drop view rw_view1;
drop table base_tbl;
drop user regress_view_user1;
drop user regress_view_user2;
+-- Test single- and multi-row inserts with table and view defaults.
+-- Table defaults should be used, unless overridden by view defaults.
+create table base_tab_def (a int, b text default 'Table default',
+ c text default 'Table default', d text, e text);
+create view base_tab_def_view as select * from base_tab_def;
+alter view base_tab_def_view alter b set default 'View default';
+alter view base_tab_def_view alter d set default 'View default';
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by a;
+ a | b | c | d | e
+----+---------------+---------------+--------------+---
+ 1 | Table default | Table default | |
+ 2 | Table default | Table default | |
+ 3 | Table default | Table default | |
+ 4 | Table default | Table default | |
+ 5 | Table default | Table default | |
+ 6 | Table default | Table default | |
+ 11 | View default | Table default | View default |
+ 12 | View default | Table default | View default |
+ 13 | View default | Table default | View default |
+ 14 | View default | Table default | View default |
+ 15 | View default | Table default | View default |
+ 16 | View default | Table default | View default |
+(12 rows)
+
+-- Adding an INSTEAD OF trigger should cause NULLs to be inserted instead of
+-- table defaults, where there are no view defaults.
+create function base_tab_def_view_instrig_func() returns trigger
+as
+$$
+begin
+ insert into base_tab_def values (new.a, new.b, new.c, new.d, new.e);
+ return new;
+end;
+$$
+language plpgsql;
+create trigger base_tab_def_view_instrig instead of insert on base_tab_def_view
+ for each row execute function base_tab_def_view_instrig_func();
+truncate base_tab_def;
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by a;
+ a | b | c | d | e
+----+---------------+---------------+--------------+---
+ 1 | Table default | Table default | |
+ 2 | Table default | Table default | |
+ 3 | Table default | Table default | |
+ 4 | Table default | Table default | |
+ 5 | Table default | Table default | |
+ 6 | Table default | Table default | |
+ 11 | View default | | View default |
+ 12 | View default | | View default |
+ 13 | View default | | View default |
+ 14 | View default | | View default |
+ 15 | View default | | View default |
+ 16 | View default | | View default |
+(12 rows)
+
+-- Using an unconditional DO INSTEAD rule should also cause NULLs to be
+-- inserted where there are no view defaults.
+drop trigger base_tab_def_view_instrig on base_tab_def_view;
+drop function base_tab_def_view_instrig_func;
+create rule base_tab_def_view_ins_rule as on insert to base_tab_def_view
+ do instead insert into base_tab_def values (new.a, new.b, new.c, new.d, new.e);
+truncate base_tab_def;
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by a;
+ a | b | c | d | e
+----+---------------+---------------+--------------+---
+ 1 | Table default | Table default | |
+ 2 | Table default | Table default | |
+ 3 | Table default | Table default | |
+ 4 | Table default | Table default | |
+ 5 | Table default | Table default | |
+ 6 | Table default | Table default | |
+ 11 | View default | | View default |
+ 12 | View default | | View default |
+ 13 | View default | | View default |
+ 14 | View default | | View default |
+ 15 | View default | | View default |
+ 16 | View default | | View default |
+(12 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
+-- defaults, unless overridden by view defaults). The second insert should
+-- behave the same as a rule-updatable view (inserting NULLs where there are
+-- no view defaults).
+drop rule base_tab_def_view_ins_rule on base_tab_def_view;
+create rule base_tab_def_view_ins_rule as on insert to base_tab_def_view
+ do also insert into base_tab_def values (new.a, new.b, new.c, new.d, new.e);
+truncate base_tab_def;
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by a, c NULLS LAST;
+ a | b | c | d | e
+----+---------------+---------------+--------------+---
+ 1 | Table default | Table default | |
+ 2 | Table default | Table default | |
+ 3 | Table default | Table default | |
+ 4 | Table default | Table default | |
+ 5 | Table default | Table default | |
+ 6 | Table default | Table default | |
+ 11 | View default | Table default | View default |
+ 11 | View default | | View default |
+ 12 | View default | Table default | View default |
+ 12 | View default | | View default |
+ 13 | View default | Table default | View default |
+ 13 | View default | | View default |
+ 14 | View default | Table default | View default |
+ 14 | View default | | View default |
+ 15 | View default | Table default | View default |
+ 15 | View default | | View default |
+ 16 | View default | Table default | View default |
+ 16 | View default | | View default |
+(18 rows)
+
+drop view base_tab_def_view;
+drop table base_tab_def;
diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql
new file mode 100644
index dc6d5cb..56d4c19
--- a/src/test/regress/sql/updatable_views.sql
+++ b/src/test/regress/sql/updatable_views.sql
@@ -1379,3 +1379,91 @@ drop view rw_view1;
drop table base_tbl;
drop user regress_view_user1;
drop user regress_view_user2;
+
+-- Test single- and multi-row inserts with table and view defaults.
+-- Table defaults should be used, unless overridden by view defaults.
+create table base_tab_def (a int, b text default 'Table default',
+ c text default 'Table default', d text, e text);
+create view base_tab_def_view as select * from base_tab_def;
+alter view base_tab_def_view alter b set default 'View default';
+alter view base_tab_def_view alter d set default 'View default';
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by a;
+
+-- Adding an INSTEAD OF trigger should cause NULLs to be inserted instead of
+-- table defaults, where there are no view defaults.
+create function base_tab_def_view_instrig_func() returns trigger
+as
+$$
+begin
+ insert into base_tab_def values (new.a, new.b, new.c, new.d, new.e);
+ return new;
+end;
+$$
+language plpgsql;
+create trigger base_tab_def_view_instrig instead of insert on base_tab_def_view
+ for each row execute function base_tab_def_view_instrig_func();
+truncate base_tab_def;
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by a;
+
+-- Using an unconditional DO INSTEAD rule should also cause NULLs to be
+-- inserted where there are no view defaults.
+drop trigger base_tab_def_view_instrig on base_tab_def_view;
+drop function base_tab_def_view_instrig_func;
+create rule base_tab_def_view_ins_rule as on insert to base_tab_def_view
+ do instead insert into base_tab_def values (new.a, new.b, new.c, new.d, new.e);
+truncate base_tab_def;
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by a;
+
+-- 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
+-- defaults, unless overridden by view defaults). The second insert should
+-- behave the same as a rule-updatable view (inserting NULLs where there are
+-- no view defaults).
+drop rule base_tab_def_view_ins_rule on base_tab_def_view;
+create rule base_tab_def_view_ins_rule as on insert to base_tab_def_view
+ do also insert into base_tab_def values (new.a, new.b, new.c, new.d, new.e);
+truncate base_tab_def;
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by a, c NULLS LAST;
+
+drop view base_tab_def_view;
+drop table base_tab_def;
On Sun, 10 Feb 2019 at 11:18, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On Sun, 10 Feb 2019 at 00:48, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
However, this is still not the end of the story, because it doesn't
fix the fact that, if the view has a DO ALSO rule on it, single-row
inserts behave differently from multi-row inserts. In that case, each
insert becomes 2 inserts, and defaults need to be treated differently
in each of the 2 queries. That's going to need a little more thought.Here's an updated patch to handle that case.
In case it's not obvious, I'm not intending to try to get this into
next week's updates -- more time is needed to be sure of this fix.
So I did some more testing of this and I'm reasonably happy that this
now fixes the originally reported issue of inconsistent handling of
DEFAULTS in multi-row VALUES lists vs single-row ones. I tested
various other scenarios involving conditional/unconditional
also/instead rules, and I didn't find any other surprises. Attached is
an updated patch with improved comments, and a little less code
duplication.
Regards,
Dean
Attachments:
view-insert-null-default-fix-v5.patchtext/x-patch; charset=US-ASCII; name=view-insert-null-default-fix-v5.patchDownload
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
new file mode 100644
index 0338e4e..65dd317
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -73,8 +73,8 @@ static TargetEntry *process_matched_tle(
TargetEntry *prior_tle,
const char *attrName);
static Node *get_assignment_input(Node *node);
-static void rewriteValuesRTE(RangeTblEntry *rte, Relation target_relation,
- List *attrnos);
+static bool rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte,
+ Relation target_relation, List *attrnos, bool force_nulls);
static void markQueryForLocking(Query *qry, Node *jtnode,
LockClauseStrength strength, LockWaitPolicy waitPolicy,
bool pushedDown);
@@ -1219,29 +1219,102 @@ searchForDefault(RangeTblEntry *rte)
* the appropriate default expressions. The other aspects of targetlist
* rewriting need be applied only to the query's targetlist proper.
*
+ * For an auto-updatable view, each DEFAULT item in the VALUES list is
+ * replaced with the default from the view, if it has one. Otherwise it is
+ * left untouched so that the underlying base relation's default can be
+ * applied instead (when we later recurse to here after rewriting the query
+ * to refer to the base relation instead of the view).
+ *
+ * For other types of relation, including rule- and trigger-updatable views,
+ * all DEFAULT items are replaced, and if the target relation doesn't have a
+ * default, the value is explicitly set to NULL.
+ *
+ * Additionally, if force_nulls is true, the target relation's defaults are
+ * ignored and all DEFAULT items in the VALUES list are explicitly set to
+ * NULL, regardless of the target relation's type. This is used for the
+ * product queries generated by DO ALSO rules attached to an auto-updatable
+ * view, for which we will have already called this function with force_nulls
+ * false. For these product queries, we must then force any remaining DEFAULT
+ * items to NULL to provide concrete values for the rule actions.
+ * Essentially, this is a mix of the 2 cases above --- the original query is
+ * 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".
+ *
+ * Returns true if all DEFAULT items were replaced, and false if some were
+ * left untouched.
*/
-static void
-rewriteValuesRTE(RangeTblEntry *rte, Relation target_relation, List *attrnos)
+static bool
+rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte,
+ Relation target_relation, List *attrnos, bool force_nulls)
{
List *newValues;
ListCell *lc;
+ bool isAutoUpdatableView;
+ bool allReplaced;
/*
* Rebuilding all the lists is a pretty expensive proposition in a big
* VALUES list, and it's a waste of time if there aren't any DEFAULT
* placeholders. So first scan to see if there are any.
+ *
+ * We skip this check if force_nulls is true, because we know that there
+ * are DEFAULT items present in that case.
*/
- if (!searchForDefault(rte))
- return; /* nothing to do */
+ 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)));
+ /*
+ * Check if the target relation is an auto-updatable view, in which case
+ * unresolved defaults will be left untouched rather than being set to
+ * NULL. If force_nulls is true, we always set DEFAULT items to NULL, so
+ * skip this check in that case --- it isn't an auto-updatable view.
+ */
+ isAutoUpdatableView = false;
+ if (!force_nulls &&
+ target_relation->rd_rel->relkind == RELKIND_VIEW &&
+ !view_has_instead_trigger(target_relation, CMD_INSERT))
+ {
+ List *locks;
+ bool hasUpdate;
+ bool found;
+ ListCell *l;
+
+ /* Look for an unconditional DO INSTEAD rule */
+ locks = matchLocks(CMD_INSERT, target_relation->rd_rules,
+ parsetree->resultRelation, parsetree, &hasUpdate);
+
+ found = false;
+ foreach(l, locks)
+ {
+ RewriteRule *rule_lock = (RewriteRule *) lfirst(l);
+
+ if (rule_lock->isInstead &&
+ rule_lock->qual == NULL)
+ {
+ found = true;
+ break;
+ }
+ }
+
+ /*
+ * If we didn't find an unconditional DO INSTEAD rule, assume that the
+ * view is auto-updatable. If it isn't, rewriteTargetView() will
+ * throw an error.
+ */
+ if (!found)
+ isAutoUpdatableView = true;
+ }
+
newValues = NIL;
+ allReplaced = true;
foreach(lc, rte->values_lists)
{
List *sublist = (List *) lfirst(lc);
@@ -1261,17 +1334,26 @@ rewriteValuesRTE(RangeTblEntry *rte, Rel
att_tup = TupleDescAttr(target_relation->rd_att, attrno - 1);
- if (!att_tup->attisdropped)
+ if (!force_nulls && !att_tup->attisdropped)
new_expr = build_column_default(target_relation, attrno);
else
new_expr = NULL; /* force a NULL if dropped */
/*
* If there is no default (ie, default is effectively NULL),
- * we've got to explicitly set the column to NULL.
+ * we've got to explicitly set the column to NULL, unless the
+ * target relation is an auto-updatable view.
*/
if (!new_expr)
{
+ if (isAutoUpdatableView)
+ {
+ /* Leave the value untouched */
+ newList = lappend(newList, col);
+ allReplaced = false;
+ continue;
+ }
+
new_expr = (Node *) makeConst(att_tup->atttypid,
-1,
att_tup->attcollation,
@@ -1296,6 +1378,8 @@ rewriteValuesRTE(RangeTblEntry *rte, Rel
newValues = lappend(newValues, newList);
}
rte->values_lists = newValues;
+
+ return allReplaced;
}
@@ -3383,6 +3467,9 @@ 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;
result_relation = parsetree->resultRelation;
Assert(result_relation != 0);
@@ -3416,14 +3503,15 @@ RewriteQuery(Query *parsetree, List *rew
parsetree->rtable);
if (rte->rtekind == RTE_VALUES)
+ {
values_rte = rte;
+ values_rte_index = rtr->rtindex;
+ }
}
}
if (values_rte)
{
- List *attrnos;
-
/* Process the main targetlist ... */
parsetree->targetList = rewriteTargetListIU(parsetree->targetList,
parsetree->commandType,
@@ -3432,7 +3520,9 @@ RewriteQuery(Query *parsetree, List *rew
parsetree->resultRelation,
&attrnos);
/* ... and the VALUES expression lists */
- rewriteValuesRTE(values_rte, rt_entry_relation, attrnos);
+ if (!rewriteValuesRTE(parsetree, values_rte,
+ rt_entry_relation, attrnos, false))
+ defaults_remaining = true;
}
else
{
@@ -3488,6 +3578,33 @@ RewriteQuery(Query *parsetree, List *rew
&qual_product);
/*
+ * If we have a VALUES RTE with any remaining untouched DEFAULT items,
+ * and we got any product queries, finalize the VALUES RTE for each
+ * product query (replacing the remaining DEFAULT items with NULLs).
+ * We don't do this for the original query, because we know that it
+ * must be an auto-insert on a view, and so should use the base
+ * relation's defaults for any remaining DEFAULT items.
+ */
+ if (defaults_remaining && product_queries != NIL)
+ {
+ ListCell *n;
+
+ /*
+ * Each product query has its own copy of the VALUES RTE at the
+ * same index in the rangetable, so we must finalize each one.
+ */
+ foreach(n, product_queries)
+ {
+ Query *pt = (Query *) lfirst(n);
+ RangeTblEntry *values_rte = rt_fetch(values_rte_index,
+ pt->rtable);
+
+ rewriteValuesRTE(pt, values_rte, rt_entry_relation, attrnos,
+ true); /* Force defaults to NULL */
+ }
+ }
+
+ /*
* If there were no INSTEAD rules, and the target relation is a view
* without any INSTEAD OF triggers, see if the view can be
* automatically updated. If so, we perform the necessary query
diff --git a/src/test/regress/expected/updatable_views.out b/src/test/regress/expected/updatable_views.out
new file mode 100644
index 420c5a5..ed07c1a
--- a/src/test/regress/expected/updatable_views.out
+++ b/src/test/regress/expected/updatable_views.out
@@ -2772,3 +2772,156 @@ drop view rw_view1;
drop table base_tbl;
drop user regress_view_user1;
drop user regress_view_user2;
+-- Test single- and multi-row inserts with table and view defaults.
+-- Table defaults should be used, unless overridden by view defaults.
+create table base_tab_def (a int, b text default 'Table default',
+ c text default 'Table default', d text, e text);
+create view base_tab_def_view as select * from base_tab_def;
+alter view base_tab_def_view alter b set default 'View default';
+alter view base_tab_def_view alter d set default 'View default';
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by a;
+ a | b | c | d | e
+----+---------------+---------------+--------------+---
+ 1 | Table default | Table default | |
+ 2 | Table default | Table default | |
+ 3 | Table default | Table default | |
+ 4 | Table default | Table default | |
+ 5 | Table default | Table default | |
+ 6 | Table default | Table default | |
+ 11 | View default | Table default | View default |
+ 12 | View default | Table default | View default |
+ 13 | View default | Table default | View default |
+ 14 | View default | Table default | View default |
+ 15 | View default | Table default | View default |
+ 16 | View default | Table default | View default |
+(12 rows)
+
+-- Adding an INSTEAD OF trigger should cause NULLs to be inserted instead of
+-- table defaults, where there are no view defaults.
+create function base_tab_def_view_instrig_func() returns trigger
+as
+$$
+begin
+ insert into base_tab_def values (new.a, new.b, new.c, new.d, new.e);
+ return new;
+end;
+$$
+language plpgsql;
+create trigger base_tab_def_view_instrig instead of insert on base_tab_def_view
+ for each row execute function base_tab_def_view_instrig_func();
+truncate base_tab_def;
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by a;
+ a | b | c | d | e
+----+---------------+---------------+--------------+---
+ 1 | Table default | Table default | |
+ 2 | Table default | Table default | |
+ 3 | Table default | Table default | |
+ 4 | Table default | Table default | |
+ 5 | Table default | Table default | |
+ 6 | Table default | Table default | |
+ 11 | View default | | View default |
+ 12 | View default | | View default |
+ 13 | View default | | View default |
+ 14 | View default | | View default |
+ 15 | View default | | View default |
+ 16 | View default | | View default |
+(12 rows)
+
+-- Using an unconditional DO INSTEAD rule should also cause NULLs to be
+-- inserted where there are no view defaults.
+drop trigger base_tab_def_view_instrig on base_tab_def_view;
+drop function base_tab_def_view_instrig_func;
+create rule base_tab_def_view_ins_rule as on insert to base_tab_def_view
+ do instead insert into base_tab_def values (new.a, new.b, new.c, new.d, new.e);
+truncate base_tab_def;
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by a;
+ a | b | c | d | e
+----+---------------+---------------+--------------+---
+ 1 | Table default | Table default | |
+ 2 | Table default | Table default | |
+ 3 | Table default | Table default | |
+ 4 | Table default | Table default | |
+ 5 | Table default | Table default | |
+ 6 | Table default | Table default | |
+ 11 | View default | | View default |
+ 12 | View default | | View default |
+ 13 | View default | | View default |
+ 14 | View default | | View default |
+ 15 | View default | | View default |
+ 16 | View default | | View default |
+(12 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
+-- defaults, unless overridden by view defaults). The second insert should
+-- behave the same as a rule-updatable view (inserting NULLs where there are
+-- no view defaults).
+drop rule base_tab_def_view_ins_rule on base_tab_def_view;
+create rule base_tab_def_view_ins_rule as on insert to base_tab_def_view
+ do also insert into base_tab_def values (new.a, new.b, new.c, new.d, new.e);
+truncate base_tab_def;
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by a, c NULLS LAST;
+ a | b | c | d | e
+----+---------------+---------------+--------------+---
+ 1 | Table default | Table default | |
+ 2 | Table default | Table default | |
+ 3 | Table default | Table default | |
+ 4 | Table default | Table default | |
+ 5 | Table default | Table default | |
+ 6 | Table default | Table default | |
+ 11 | View default | Table default | View default |
+ 11 | View default | | View default |
+ 12 | View default | Table default | View default |
+ 12 | View default | | View default |
+ 13 | View default | Table default | View default |
+ 13 | View default | | View default |
+ 14 | View default | Table default | View default |
+ 14 | View default | | View default |
+ 15 | View default | Table default | View default |
+ 15 | View default | | View default |
+ 16 | View default | Table default | View default |
+ 16 | View default | | View default |
+(18 rows)
+
+drop view base_tab_def_view;
+drop table base_tab_def;
diff --git a/src/test/regress/sql/updatable_views.sql b/src/test/regress/sql/updatable_views.sql
new file mode 100644
index dc6d5cb..56d4c19
--- a/src/test/regress/sql/updatable_views.sql
+++ b/src/test/regress/sql/updatable_views.sql
@@ -1379,3 +1379,91 @@ drop view rw_view1;
drop table base_tbl;
drop user regress_view_user1;
drop user regress_view_user2;
+
+-- Test single- and multi-row inserts with table and view defaults.
+-- Table defaults should be used, unless overridden by view defaults.
+create table base_tab_def (a int, b text default 'Table default',
+ c text default 'Table default', d text, e text);
+create view base_tab_def_view as select * from base_tab_def;
+alter view base_tab_def_view alter b set default 'View default';
+alter view base_tab_def_view alter d set default 'View default';
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by a;
+
+-- Adding an INSTEAD OF trigger should cause NULLs to be inserted instead of
+-- table defaults, where there are no view defaults.
+create function base_tab_def_view_instrig_func() returns trigger
+as
+$$
+begin
+ insert into base_tab_def values (new.a, new.b, new.c, new.d, new.e);
+ return new;
+end;
+$$
+language plpgsql;
+create trigger base_tab_def_view_instrig instead of insert on base_tab_def_view
+ for each row execute function base_tab_def_view_instrig_func();
+truncate base_tab_def;
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by a;
+
+-- Using an unconditional DO INSTEAD rule should also cause NULLs to be
+-- inserted where there are no view defaults.
+drop trigger base_tab_def_view_instrig on base_tab_def_view;
+drop function base_tab_def_view_instrig_func;
+create rule base_tab_def_view_ins_rule as on insert to base_tab_def_view
+ do instead insert into base_tab_def values (new.a, new.b, new.c, new.d, new.e);
+truncate base_tab_def;
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by a;
+
+-- 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
+-- defaults, unless overridden by view defaults). The second insert should
+-- behave the same as a rule-updatable view (inserting NULLs where there are
+-- no view defaults).
+drop rule base_tab_def_view_ins_rule on base_tab_def_view;
+create rule base_tab_def_view_ins_rule as on insert to base_tab_def_view
+ do also insert into base_tab_def values (new.a, new.b, new.c, new.d, new.e);
+truncate base_tab_def;
+insert into base_tab_def values (1);
+insert into base_tab_def values (2), (3);
+insert into base_tab_def values (4, default, default, default, default);
+insert into base_tab_def values (5, default, default, default, default),
+ (6, default, default, default, default);
+insert into base_tab_def_view values (11);
+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);
+select * from base_tab_def order by a, c NULLS LAST;
+
+drop view base_tab_def_view;
+drop table base_tab_def;
Hi Dean,
On 2019/02/12 19:33, Dean Rasheed wrote:
On Sun, 10 Feb 2019 at 11:18, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On Sun, 10 Feb 2019 at 00:48, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
However, this is still not the end of the story, because it doesn't
fix the fact that, if the view has a DO ALSO rule on it, single-row
inserts behave differently from multi-row inserts. In that case, each
insert becomes 2 inserts, and defaults need to be treated differently
in each of the 2 queries. That's going to need a little more thought.Here's an updated patch to handle that case.
In case it's not obvious, I'm not intending to try to get this into
next week's updates -- more time is needed to be sure of this fix.So I did some more testing of this and I'm reasonably happy that this
now fixes the originally reported issue of inconsistent handling of
DEFAULTS in multi-row VALUES lists vs single-row ones. I tested
various other scenarios involving conditional/unconditional
also/instead rules, and I didn't find any other surprises. Attached is
an updated patch with improved comments, and a little less code
duplication.
Thanks for updating the patch.
I can't really comment on all of the changes that that you made
considering various cases, but became curious if the single-row and
multi-row inserts cases could share the code that determines if the
DEFAULT item be replaced by the target-relation-specified default or NULL?
IOW, is there some reason why we can't avoid the special handling for the
multi-row (RTE_VALUES) case?
Thanks,
Amit
On Wed, 13 Feb 2019 at 03:02, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
I can't really comment on all of the changes that that you made
considering various cases, but became curious if the single-row and
multi-row inserts cases could share the code that determines if the
DEFAULT item be replaced by the target-relation-specified default or NULL?
IOW, is there some reason why we can't avoid the special handling for the
multi-row (RTE_VALUES) case?
No, not as far as I can see.
The tricky part for a multi-row insert is working out what to do when
it sees a DEFAULT, and there is no column default on the target
relation. For an auto-updatable view, it needs to leave the DEFAULT
untouched, so that it can later apply the base relation's column
default when it recurses. For all other kinds of relation, it needs to
turn the DEFAULT into a NULL.
For a single-row insert, that's all much easier. If it sees a DEFAULT,
and there is no column default, it simply omits that entry from the
targetlist. If it then recurses to the base relation, it will put the
targetlist entry back in, if the base relation has a column default.
So it doesn't need to know locally whether it's an auto-updatable
view, and the logic is much simpler. The multi-row case can't easily
do that (add and remove columns) because it's working with a
fixed-width table structure.
Actually, that's not quite the end of it. So far, this has only been
considering INSERT's. I think there are more issues with UPDATE's, but
that's a whole other can of worms. I think I'll commit this first, and
start a thread on -hackers to discuss that.
Regards,
Dean
On 2019/02/13 18:04, Dean Rasheed wrote:
On Wed, 13 Feb 2019 at 03:02, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:I can't really comment on all of the changes that that you made
considering various cases, but became curious if the single-row and
multi-row inserts cases could share the code that determines if the
DEFAULT item be replaced by the target-relation-specified default or NULL?
IOW, is there some reason why we can't avoid the special handling for the
multi-row (RTE_VALUES) case?No, not as far as I can see.
The tricky part for a multi-row insert is working out what to do when
it sees a DEFAULT, and there is no column default on the target
relation. For an auto-updatable view, it needs to leave the DEFAULT
untouched, so that it can later apply the base relation's column
default when it recurses. For all other kinds of relation, it needs to
turn the DEFAULT into a NULL.For a single-row insert, that's all much easier. If it sees a DEFAULT,
and there is no column default, it simply omits that entry from the
targetlist. If it then recurses to the base relation, it will put the
targetlist entry back in, if the base relation has a column default.
So it doesn't need to know locally whether it's an auto-updatable
view, and the logic is much simpler. The multi-row case can't easily
do that (add and remove columns) because it's working with a
fixed-width table structure.
Hmm yeah, column sets must be the same in in all value-lists.
Actually, that's not quite the end of it. So far, this has only been
considering INSERT's. I think there are more issues with UPDATE's, but
that's a whole other can of worms. I think I'll commit this first, and
start a thread on -hackers to discuss that.
Sure, thanks for the explanation.
Regards,
Amit
On Tue, 12 Feb 2019 at 10:33, Dean Rasheed <dean.a.rasheed@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
Attachments:
fix-rewrite-values-rte.patchtext/x-patch; charset=US-ASCII; name=fix-rewrite-values-rte.patchDownload
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;
On 2019/02/27 18:37, Dean Rasheed wrote:
On Tue, 12 Feb 2019 at 10:33, Dean Rasheed <dean.a.rasheed@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.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.
+1. Only rewriteValuesRTE needs to use that information, so it's better
to teach it to figure it by itself.
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.
+ if (attrno == 0)
+ elog(ERROR, "Cannot set value in column %d to
DEFAULT", i);Maybe: s/Cannot/cannot/g
+ Assert(list_length(sublist) == numattrs);
Isn't this Assert useless, because we're setting numattrs to
list_length(<first-sublist>) and transformInsertStmt ensures that all
sublists are same length?
Thanks,
Amit
On Thu, 28 Feb 2019 at 07:47, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
+ if (attrno == 0) + elog(ERROR, "Cannot set value in column %d to DEFAULT", i);Maybe: s/Cannot/cannot/g
Ah yes, you're right. That is the convention.
+ Assert(list_length(sublist) == numattrs);
Isn't this Assert useless, because we're setting numattrs to
list_length(<first-sublist>) and transformInsertStmt ensures that all
sublists are same length?
Well possibly I'm being over-paranoid, but given that it may have come
via a previous invocation of rewriteValuesRTE() that may have
completely rebuilt the lists, it seemed best to be sure that it hadn't
done something unexpected. It's about to use that to read from the
attrnos array, so it might read beyond the array bounds if any of the
prior logic was faulty.
Thanks for looking.
Regards,
Dean