From: Tom Lane <tgl@sss.pgh.pa.us>
To: Japin Li <japinli@hotmail.com>
Cc: Richard Guo <guofenglinux@gmail.com>
Cc: jiye_sw@126.com
Cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17633: Define rule on views which do insert to another relation trigger cache lookup failed error.
Date: Tue, 11 Oct 2022 11:37:12 -0400
Message-ID: <348879.1665502632@sss.pgh.pa.us> (raw)
In-Reply-To: <312797.1665500865@sss.pgh.pa.us>
References: <17633-98cc85e1fa91e905@postgresql.org>
<CAMbWs49K+LkBFTu3XqJgso=_BTJ81NJv9N5Bx0_4XikkmG-qdA@mail.gmail.com>
<CAMbWs4-Z8hSnHOTrCV4-Pp1+H9+i34O68pSkhEHLyDpYfPyqpQ@mail.gmail.com>
<MEYP282MB1669B7492DAFE6913C127ED8B6239@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
<CAMbWs491JOKxnc3f3XGj1oYAzi=OJP6=YjUZnQ=hAUxo3BF+hQ@mail.gmail.com>
<MEYP282MB1669B5146A97FCD8D18A75EFB6239@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
<312797.1665500865@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
Attachments:fix-bug-17633.patch (text/x-diff, 6.2K) downloaddiff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.cindex 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);
}
}
=
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, japinli@hotmail.com, guofenglinux@gmail.com, 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: <348879.1665502632@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox