Thread: BUG #17633: Define rule on views which do insert to another relation trigger cache lookup failed error.

The following bug has been logged on the website:

Bug reference:      17633
Logged by:          leafji's git home
Email address:      jiye_sw@126.com
PostgreSQL version: 14.5
Operating system:   Centos
Description:

Just execute as follow sql:

drop view v1;
drop table t1;
drop table t2;
create table t1(i int, j int);
create table t2(a int, b int, c int, d int);
create or replace rule t1_r as on insert to t1 do also insert into t2(c,d)
values (new.i, new.j);
insert into t1 values (1,default);
insert into t1 values (1,default),(2, default);
-- create rule on t1 directly no issue
drop rule t1_r on t1;
create view v1 as select * from t1;
— create rule on view query t1 will trigger this issue
create or replace rule v1_r as on insert to v1 do also insert into t2(c,d)
values (new.i, new.j);
insert into v1 values (1,default);
-- must multi values.
insert into v1 values (1,default),(2, default);
=> it will trigger cache lookup failed for type.

On Tue, 11 Oct 2022 at 20:09, Richard Guo <guofenglinux@gmail.com> wrote:
>
> I think the problem exists for auto-updatable view, as we leave the
> DEFAULT items untouched because we expect to apply the underlying base
> rel's default.
>
> In this case there is a rewrite rule on the view.  Applying the rule
> we'd get a product query whose target entries referring to the VALUES
> RTE have attribute 3 and 4 while the relation has only two attributes.
> Then we proceed to replacing the remaining DEFAULT items with NULLs.
> And when we try to access the relation's 3rd and 4th attributes, we are
> accessing illegal memory areas.
>

Yeah, I also notice this, attch a patch to fix it.

-- 
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index d02fd83c0a..2f77ff87dc 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -3857,13 +3857,20 @@ RewriteQuery(Query *parsetree, List *rewrite_events)
             foreach(n, product_queries)
             {
                 Query       *pt = (Query *) lfirst(n);
+                Relation    target_relation;
+                RangeTblEntry *target_entry;
                 RangeTblEntry *values_rte = rt_fetch(values_rte_index,
                                                      pt->rtable);
 
+                target_entry = rt_fetch(pt->resultRelation, pt->rtable);
+                target_relation = table_open(target_entry->relid, NoLock);
+
                 rewriteValuesRTE(pt, values_rte, values_rte_index,
-                                 rt_entry_relation,
+                                 target_relation,
                                  true,    /* Force remaining defaults to NULL */
                                  NULL);
+
+                table_close(target_relation, NoLock);
             }
         }

On Tue, 11 Oct 2022 at 21:16, Richard Guo <guofenglinux@gmail.com> wrote:
> On Tue, Oct 11, 2022 at 8:29 PM Japin Li <japinli@hotmail.com> wrote:
>
>>
>> On Tue, 11 Oct 2022 at 20:09, Richard Guo <guofenglinux@gmail.com> wrote:
>>
>> Yeah, I also notice this, attch a patch to fix it.
>
>
> +1 for the idea. We need to identify the right target relation for each
> product query and rt_entry_relation is not the right one.
>

After some more thinking, I find the previous cannot work correctly.
For example:

    CREATE OR REPLACE v1_r AS ON INSERT TO t1 DO ALSO SELECT * FROM t2;

> A minor comment is can we know the product query is not CMD_SELECT?
> If so I suggest we add an assertion before fetching the target relation,
> something like:
>
> Assert(pt->resultRelation != 0);
>

Oh, I think this might not be true.  The product query comes from rules,
which might be a SELECT query, IIUC.  See above example.


--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

Japin Li <japinli@hotmail.com> writes:
> On Tue, 11 Oct 2022 at 21:16, Richard Guo <guofenglinux@gmail.com> wrote:
>> +1 for the idea. We need to identify the right target relation for each
>> product query and rt_entry_relation is not the right one.

> After some more thinking, I find the previous cannot work correctly.

Yeah.  The product query might not be an INSERT at all, yet
rewriteValuesRTE is assuming it is --- that whole business of
scanning the targetlist for referencing Vars and then saving
their resnos doesn't make sense otherwise.

I think the basic problem is that the two calls of rewriteValuesRTE
are really dealing with fundamentally different cases, and we should
probably not have tried to make the same function do both.  I'm going
to try splitting it into two functions, one for the force_nulls case
and one for the !force_nulls case.  The force_nulls case shouldn't
really need a target_relation parameter in the first place (since it
has no business assuming that the query is an INSERT).  I think it
should just replace each SetToDefault with a NULL of the same type
as the SetToDefault, and call it good.

            regards, tom lane

I wrote:
> I think the basic problem is that the two calls of rewriteValuesRTE
> are really dealing with fundamentally different cases, and we should
> probably not have tried to make the same function do both.  I'm going
> to try splitting it into two functions, one for the force_nulls case
> and one for the !force_nulls case.

As attached.  This seems *way* cleaner to me, even if it means we need
two copies of the loops-over-VALUES-list-entries.  I didn't write a
regression test yet, but this fixes the submitted bug and passes
check-world.

            regards, tom lane

diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index d02fd83c0a..a6dd99cc98 100644
--- a/src/backend/rewrite/rewriteHandler.c
+++ b/src/backend/rewrite/rewriteHandler.c
@@ -79,8 +79,9 @@ static TargetEntry *process_matched_tle(TargetEntry *src_tle,
 static Node *get_assignment_input(Node *node);
 static Bitmapset *findDefaultOnlyColumns(RangeTblEntry *rte);
 static bool rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
-                             Relation target_relation, bool force_nulls,
+                             Relation target_relation,
                              Bitmapset *unused_cols);
+static void rewriteValuesRTEToNulls(Query *parsetree, RangeTblEntry *rte);
 static void markQueryForLocking(Query *qry, Node *jtnode,
                                 LockClauseStrength strength, LockWaitPolicy waitPolicy,
                                 bool pushedDown);
@@ -1370,18 +1371,7 @@ findDefaultOnlyColumns(RangeTblEntry *rte)
  * 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.
- *
- * Finally, if a DEFAULT item is found in a column mentioned in unused_cols,
+ * Also, if a DEFAULT item is found in a column mentioned in unused_cols,
  * it is explicitly set to NULL.  This happens for columns in the VALUES RTE
  * whose corresponding targetlist entries have already been replaced with the
  * relation's default expressions, so that any values in those columns of the
@@ -1404,7 +1394,7 @@ findDefaultOnlyColumns(RangeTblEntry *rte)
  */
 static bool
 rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
-                 Relation target_relation, bool force_nulls,
+                 Relation target_relation,
                  Bitmapset *unused_cols)
 {
     List       *newValues;
@@ -1414,15 +1404,14 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
     int            numattrs;
     int           *attrnos;

+    Assert(rte->rtekind == RTE_VALUES);
+
     /*
      * 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 (!force_nulls && !searchForDefault(rte))
+    if (!searchForDefault(rte))
         return true;            /* nothing to do */

     /*
@@ -1456,12 +1445,10 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
     /*
      * 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.
+     * NULL.
      */
     isAutoUpdatableView = false;
-    if (!force_nulls &&
-        target_relation->rd_rel->relkind == RELKIND_VIEW &&
+    if (target_relation->rd_rel->relkind == RELKIND_VIEW &&
         !view_has_instead_trigger(target_relation, CMD_INSERT))
     {
         List       *locks;
@@ -1535,9 +1522,10 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,

                 if (attrno == 0)
                     elog(ERROR, "cannot set value in column %d to DEFAULT", i);
+                Assert(attrno > 0 && attrno <= target_relation->rd_att->natts);
                 att_tup = TupleDescAttr(target_relation->rd_att, attrno - 1);

-                if (!force_nulls && !att_tup->attisdropped)
+                if (!att_tup->attisdropped)
                     new_expr = build_column_default(target_relation, attrno);
                 else
                     new_expr = NULL;    /* force a NULL if dropped */
@@ -1587,6 +1575,50 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *rte, int rti,
     return allReplaced;
 }

+/*
+ * Mop up any remaining DEFAULT items in the given VALUES RTE by
+ * replacing them with NULL constants.
+ *
+ * This is used for the product queries generated by DO ALSO rules attached to
+ * an auto-updatable view.  The action can't depend on the "target relation"
+ * since the product query might not have one (it needn't be an INSERT).
+ * Essentially, such queries are treated as being attached to a rule-updatable
+ * view.
+ */
+static void
+rewriteValuesRTEToNulls(Query *parsetree, RangeTblEntry *rte)
+{
+    List       *newValues;
+    ListCell   *lc;
+
+    Assert(rte->rtekind == RTE_VALUES);
+    newValues = NIL;
+    foreach(lc, rte->values_lists)
+    {
+        List       *sublist = (List *) lfirst(lc);
+        List       *newList = NIL;
+        ListCell   *lc2;
+
+        foreach(lc2, sublist)
+        {
+            Node       *col = (Node *) lfirst(lc2);
+
+            if (IsA(col, SetToDefault))
+            {
+                SetToDefault *def = (SetToDefault *) col;
+
+                newList = lappend(newList, makeNullConst(def->typeId,
+                                                         def->typeMod,
+                                                         def->collation));
+            }
+            else
+                newList = lappend(newList, col);
+        }
+        newValues = lappend(newValues, newList);
+    }
+    rte->values_lists = newValues;
+}
+

 /*
  * Record in target_rte->extraUpdatedCols the indexes of any generated columns
@@ -3746,7 +3778,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events)
                                                             &unused_values_attrnos);
                 /* ... and the VALUES expression lists */
                 if (!rewriteValuesRTE(parsetree, values_rte, values_rte_index,
-                                      rt_entry_relation, false,
+                                      rt_entry_relation,
                                       unused_values_attrnos))
                     defaults_remaining = true;
             }
@@ -3860,10 +3892,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events)
                 RangeTblEntry *values_rte = rt_fetch(values_rte_index,
                                                      pt->rtable);

-                rewriteValuesRTE(pt, values_rte, values_rte_index,
-                                 rt_entry_relation,
-                                 true,    /* Force remaining defaults to NULL */
-                                 NULL);
+                rewriteValuesRTEToNulls(pt, values_rte);
             }
         }

On Tue, 11 Oct 2022 at 23:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> I think the basic problem is that the two calls of rewriteValuesRTE
>> are really dealing with fundamentally different cases, and we should
>> probably not have tried to make the same function do both.  I'm going
>> to try splitting it into two functions, one for the force_nulls case
>> and one for the !force_nulls case.
>
> As attached.  This seems *way* cleaner to me, even if it means we need
> two copies of the loops-over-VALUES-list-entries.  I didn't write a
> regression test yet, but this fixes the submitted bug and passes
> check-world.
>

Great.  LGTM.  Should we add the test-case for this?

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 9dd137415e..2fbc30aaad 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -3585,3 +3585,15 @@ DROP VIEW ruletest_v1;
 DROP TABLE ruletest_t2;
 DROP TABLE ruletest_t1;
 DROP USER regress_rule_user1;
+--
+-- Test case for BUG 17633
+--
+CREATE TABLE ruletest_t1 (x int);
+CREATE TABLE ruletest_t2 (a int, b int);
+CREATE VIEW ruletest_v1 AS SELECT * FROM ruletest_t1;
+CREATE RULE rule1 AS ON INSERT TO ruletest_v1
+    DO ALSO INSERT INTO ruletest_t2(b) VALUES (NEW.*);
+INSERT INTO ruletest_v1 VALUES (default), (default);
+DROP VIEW ruletest_v1;
+DROP TABLE ruletest_t1;
+DROP TABLE ruletest_t2;
diff --git a/src/test/regress/sql/rules.sql b/src/test/regress/sql/rules.sql
index bfb5f3b0bb..290ea2bd8a 100644
--- a/src/test/regress/sql/rules.sql
+++ b/src/test/regress/sql/rules.sql
@@ -1318,3 +1318,19 @@ DROP TABLE ruletest_t2;
 DROP TABLE ruletest_t1;
 
 DROP USER regress_rule_user1;
+
+--
+-- Test case for BUG 17633
+--
+CREATE TABLE ruletest_t1 (x int);
+CREATE TABLE ruletest_t2 (a int, b int);
+CREATE VIEW ruletest_v1 AS SELECT * FROM ruletest_t1;
+
+CREATE RULE rule1 AS ON INSERT TO ruletest_v1
+    DO ALSO INSERT INTO ruletest_t2(b) VALUES (NEW.*);
+
+INSERT INTO ruletest_v1 VALUES (default), (default);
+
+DROP VIEW ruletest_v1;
+DROP TABLE ruletest_t1;
+DROP TABLE ruletest_t2;