Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oiHL8-0001Q1-9X for pgsql-bugs@arkaria.postgresql.org; Tue, 11 Oct 2022 15:38:42 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1oiHK7-0003i6-Sh for pgsql-bugs@arkaria.postgresql.org; Tue, 11 Oct 2022 15:37:39 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oiHK7-0003hx-JD for pgsql-bugs@lists.postgresql.org; Tue, 11 Oct 2022 15:37:39 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oiHK4-0002i5-DB for pgsql-bugs@lists.postgresql.org; Tue, 11 Oct 2022 15:37:39 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 29BFbC2I348880; Tue, 11 Oct 2022 11:37:12 -0400 From: Tom Lane To: Japin Li cc: Richard Guo , jiye_sw@126.com, pgsql-bugs@lists.postgresql.org Subject: Re: BUG #17633: Define rule on views which do insert to another relation trigger cache lookup failed error. In-reply-to: <312797.1665500865@sss.pgh.pa.us> References: <17633-98cc85e1fa91e905@postgresql.org> <312797.1665500865@sss.pgh.pa.us> Comments: In-reply-to Tom Lane message dated "Tue, 11 Oct 2022 11:07:45 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <348849.1665502606.0@sss.pgh.pa.us> Date: Tue, 11 Oct 2022 11:37:12 -0400 Message-ID: <348879.1665502632@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <348849.1665502606.1@sss.pgh.pa.us> 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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="fix-bug-17633.patch"; charset="us-ascii" Content-ID: <348849.1665502606.2@sss.pgh.pa.us> Content-Description: fix-bug-17633.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/re= writeHandler.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 rt= i, - 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 hav= e a * default, the value is explicitly set to NULL. * - * Additionally, if force_nulls is true, the target relation's defaults a= re - * 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-updatab= le - * view, for which we will have already called this function with force_n= ulls - * false. For these product queries, we must then force any remaining DE= FAULT - * 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 ins= erts - * into a rule-updatable view. - * - * Finally, if a DEFAULT item is found in a column mentioned in unused_co= ls, + * 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 =3D=3D 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, s= o - * skip this check in that case --- it isn't an auto-updatable view. + * NULL. */ isAutoUpdatableView =3D false; - if (!force_nulls && - target_relation->rd_rel->relkind =3D=3D RELKIND_VIEW && + if (target_relation->rd_rel->relkind =3D=3D RELKIND_VIEW && !view_has_instead_trigger(target_relation, CMD_INSERT)) { List *locks; @@ -1535,9 +1522,10 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *r= te, int rti, = if (attrno =3D=3D 0) elog(ERROR, "cannot set value in column %d to DEFAULT", i); + Assert(attrno > 0 && attrno <=3D target_relation->rd_att->natts); att_tup =3D TupleDescAttr(target_relation->rd_att, attrno - 1); = - if (!force_nulls && !att_tup->attisdropped) + if (!att_tup->attisdropped) new_expr =3D build_column_default(target_relation, attrno); else new_expr =3D NULL; /* force a NULL if dropped */ @@ -1587,6 +1575,50 @@ rewriteValuesRTE(Query *parsetree, RangeTblEntry *r= te, 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 attach= ed to + * an auto-updatable view. The action can't depend on the "target relati= on" + * 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-upda= table + * view. + */ +static void +rewriteValuesRTEToNulls(Query *parsetree, RangeTblEntry *rte) +{ + List *newValues; + ListCell *lc; + + Assert(rte->rtekind =3D=3D RTE_VALUES); + newValues =3D NIL; + foreach(lc, rte->values_lists) + { + List *sublist =3D (List *) lfirst(lc); + List *newList =3D NIL; + ListCell *lc2; + + foreach(lc2, sublist) + { + Node *col =3D (Node *) lfirst(lc2); + + if (IsA(col, SetToDefault)) + { + SetToDefault *def =3D (SetToDefault *) col; + + newList =3D lappend(newList, makeNullConst(def->typeId, + def->typeMod, + def->collation)); + } + else + newList =3D lappend(newList, col); + } + newValues =3D lappend(newValues, newList); + } + rte->values_lists =3D newValues; +} + = /* * Record in target_rte->extraUpdatedCols the indexes of any generated co= lumns @@ -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 =3D true; } @@ -3860,10 +3892,7 @@ RewriteQuery(Query *parsetree, List *rewrite_events= ) RangeTblEntry *values_rte =3D 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); } } = ------- =_aaaaaaaaaa0--