Skip to content

Commit b484bff

Browse files
tglsfdctglsfdc
committed
Fix INSERT ON CONFLICT UPDATE through a view that isn't just SELECT *.
When expanding an updatable view that is an INSERT's target, the rewriter failed to rewrite Vars in the ON CONFLICT UPDATE clause. This accidentally worked if the view was just "SELECT * FROM ...", as the transformation would be a no-op in that case. With more complicated view targetlists, this omission would often lead to "attribute ... has the wrong type" errors or even crashes, as reported by Mario De Frutos Dieguez. Fix by adding code to rewriteTargetView to fix up the data structure correctly. The easiest way to update the exclRelTlist list is to rebuild it from scratch looking at the new target relation, so factor the code for that out of transformOnConflictClause to make it sharable. In passing, avoid duplicate permissions checks against the EXCLUDED pseudo-relation, and prevent useless view expansion of that relation's dummy RTE. The latter is only known to happen (after this patch) in cases where the query would fail later due to not having any INSTEAD OF triggers for the view. But by exactly that token, it would create an unintended and very poorly tested state of the query data structure, so it seems like a good idea to prevent it from happening at all. This has been broken since ON CONFLICT was introduced, so back-patch to 9.5. Dean Rasheed, based on an earlier patch by Amit Langote; comment-kibitzing and back-patching by me Discussion: https://postgr.es/m/CAFYwGJ0xfzy8jaK80hVN2eUWr6huce0RU8AgU04MGD00igqkTg@mail.gmail.com
1 parent f5b4bb8 commit b484bff

File tree

5 files changed

+514
-57
lines changed

5 files changed

+514
-57
lines changed

‎src/backend/parser/analyze.c

Lines changed: 81 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -963,9 +963,6 @@ transformOnConflictClause(ParseState *pstate,
963963
if (onConflictClause->action == ONCONFLICT_UPDATE)
964964
{
965965
Relation targetrel = pstate->p_target_relation;
966-
Var *var;
967-
TargetEntry *te;
968-
int attno;
969966

970967
/*
971968
* All INSERT expressions have been parsed, get ready for potentially
@@ -974,75 +971,36 @@ transformOnConflictClause(ParseState *pstate,
974971
pstate->p_is_insert = false;
975972

976973
/*
977-
* Add range table entry for the EXCLUDED pseudo relation; relkind is
974+
* Add range table entry for the EXCLUDED pseudo relation. relkind is
978975
* set to composite to signal that we're not dealing with an actual
979-
* relation.
976+
* relation, and no permission checks are required on it. (We'll
977+
* check the actual target relation, instead.)
980978
*/
981979
exclRte = addRangeTableEntryForRelation(pstate,
982980
targetrel,
983981
makeAlias("excluded", NIL),
984982
false, false);
985983
exclRte->relkind = RELKIND_COMPOSITE_TYPE;
986-
exclRelIndex = list_length(pstate->p_rtable);
987-
988-
/*
989-
* Build a targetlist representing the columns of the EXCLUDED pseudo
990-
* relation. Have to be careful to use resnos that correspond to
991-
* attnos of the underlying relation.
992-
*/
993-
for (attno = 0; attno < targetrel->rd_rel->relnatts; attno++)
994-
{
995-
Form_pg_attribute attr = targetrel->rd_att->attrs[attno];
996-
char *name;
997-
998-
if (attr->attisdropped)
999-
{
1000-
/*
1001-
* can't use atttypid here, but it doesn't really matter what
1002-
* type the Const claims to be.
1003-
*/
1004-
var = (Var *) makeNullConst(INT4OID, -1, InvalidOid);
1005-
name = "";
1006-
}
1007-
else
1008-
{
1009-
var = makeVar(exclRelIndex, attno + 1,
1010-
attr->atttypid, attr->atttypmod,
1011-
attr->attcollation,
1012-
0);
1013-
name = pstrdup(NameStr(attr->attname));
1014-
}
984+
exclRte->requiredPerms = 0;
985+
/* other permissions fields in exclRte are already empty */
1015986

1016-
te = makeTargetEntry((Expr *) var,
1017-
attno + 1,
1018-
name,
1019-
false);
1020-
1021-
/* don't require select access yet */
1022-
exclRelTlist = lappend(exclRelTlist, te);
1023-
}
987+
exclRelIndex = list_length(pstate->p_rtable);
1024988

1025-
/*
1026-
* Add a whole-row-Var entry to support references to "EXCLUDED.*".
1027-
* Like the other entries in exclRelTlist, its resno must match the
1028-
* Var's varattno, else the wrong things happen while resolving
1029-
* references in setrefs.c. This is against normal conventions for
1030-
* targetlists, but it's okay since we don't use this as a real tlist.
1031-
*/
1032-
var = makeVar(exclRelIndex, InvalidAttrNumber,
1033-
targetrel->rd_rel->reltype,
1034-
-1, InvalidOid, 0);
1035-
te = makeTargetEntry((Expr *) var, InvalidAttrNumber, NULL, true);
1036-
exclRelTlist = lappend(exclRelTlist, te);
989+
/* Create EXCLUDED rel's targetlist for use by EXPLAIN */
990+
exclRelTlist = BuildOnConflictExcludedTargetlist(targetrel,
991+
exclRelIndex);
1037992

1038993
/*
1039994
* Add EXCLUDED and the target RTE to the namespace, so that they can
1040-
* be used in the UPDATE statement.
995+
* be used in the UPDATE subexpressions.
1041996
*/
1042997
addRTEtoQuery(pstate, exclRte, false, true, true);
1043998
addRTEtoQuery(pstate, pstate->p_target_rangetblentry,
1044999
false, true, true);
10451000

1001+
/*
1002+
* Now transform the UPDATE subexpressions.
1003+
*/
10461004
onConflictSet =
10471005
transformUpdateTargetList(pstate, onConflictClause->targetList);
10481006

@@ -1067,6 +1025,74 @@ transformOnConflictClause(ParseState *pstate,
10671025
}
10681026

10691027

1028+
/*
1029+
* BuildOnConflictExcludedTargetlist
1030+
* Create target list for the EXCLUDED pseudo-relation of ON CONFLICT,
1031+
* representing the columns of targetrel with varno exclRelIndex.
1032+
*
1033+
* Note: Exported for use in the rewriter.
1034+
*/
1035+
List *
1036+
BuildOnConflictExcludedTargetlist(Relation targetrel,
1037+
Index exclRelIndex)
1038+
{
1039+
List *result = NIL;
1040+
int attno;
1041+
Var *var;
1042+
TargetEntry *te;
1043+
1044+
/*
1045+
* Note that resnos of the tlist must correspond to attnos of the
1046+
* underlying relation, hence we need entries for dropped columns too.
1047+
*/
1048+
for (attno = 0; attno < RelationGetNumberOfAttributes(targetrel); attno++)
1049+
{
1050+
Form_pg_attribute attr = TupleDescAttr(targetrel->rd_att, attno);
1051+
char *name;
1052+
1053+
if (attr->attisdropped)
1054+
{
1055+
/*
1056+
* can't use atttypid here, but it doesn't really matter what type
1057+
* the Const claims to be.
1058+
*/
1059+
var = (Var *) makeNullConst(INT4OID, -1, InvalidOid);
1060+
name = "";
1061+
}
1062+
else
1063+
{
1064+
var = makeVar(exclRelIndex, attno + 1,
1065+
attr->atttypid, attr->atttypmod,
1066+
attr->attcollation,
1067+
0);
1068+
name = pstrdup(NameStr(attr->attname));
1069+
}
1070+
1071+
te = makeTargetEntry((Expr *) var,
1072+
attno + 1,
1073+
name,
1074+
false);
1075+
1076+
result = lappend(result, te);
1077+
}
1078+
1079+
/*
1080+
* Add a whole-row-Var entry to support references to "EXCLUDED.*". Like
1081+
* the other entries in the EXCLUDED tlist, its resno must match the Var's
1082+
* varattno, else the wrong things happen while resolving references in
1083+
* setrefs.c. This is against normal conventions for targetlists, but
1084+
* it's okay since we don't use this as a real tlist.
1085+
*/
1086+
var = makeVar(exclRelIndex, InvalidAttrNumber,
1087+
targetrel->rd_rel->reltype,
1088+
-1, InvalidOid, 0);
1089+
te = makeTargetEntry((Expr *) var, InvalidAttrNumber, NULL, true);
1090+
result = lappend(result, te);
1091+
1092+
return result;
1093+
}
1094+
1095+
10701096
/*
10711097
* count_rowexpr_columns -
10721098
* get number of columns contained in a ROW() expression;

‎src/backend/rewrite/rewriteHandler.c

Lines changed: 101 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
#include "nodes/nodeFuncs.h"
2929
#include "parser/analyze.h"
3030
#include "parser/parse_coerce.h"
31+
#include "parser/parse_relation.h"
3132
#include "parser/parsetree.h"
3233
#include "rewrite/rewriteDefine.h"
3334
#include "rewrite/rewriteHandler.h"
@@ -1707,6 +1708,17 @@ fireRIRrules(Query *parsetree, List *activeRIRs, bool forUpdatePushedDown)
17071708
if (rte->relkind == RELKIND_MATVIEW)
17081709
continue;
17091710

1711+
/*
1712+
* In INSERT ... ON CONFLICT, ignore the EXCLUDED pseudo-relation;
1713+
* even if it points to a view, we needn't expand it, and should not
1714+
* because we want the RTE to remain of RTE_RELATION type. Otherwise,
1715+
* it would get changed to RTE_SUBQUERY type, which is an
1716+
* untested/unsupported situation.
1717+
*/
1718+
if (parsetree->onConflict &&
1719+
rt_index == parsetree->onConflict->exclRelIndex)
1720+
continue;
1721+
17101722
/*
17111723
* If the table is not referenced in the query, then we ignore it.
17121724
* This prevents infinite expansion loop due to new rtable entries
@@ -2812,8 +2824,6 @@ rewriteTargetView(Query *parsetree, Relation view)
28122824
*/
28132825
base_rte->relkind = base_rel->rd_rel->relkind;
28142826

2815-
heap_close(base_rel, NoLock);
2816-
28172827
/*
28182828
* If the view query contains any sublink subqueries then we need to also
28192829
* acquire locks on any relations they refer to. We know that there won't
@@ -2971,6 +2981,93 @@ rewriteTargetView(Query *parsetree, Relation view)
29712981
}
29722982
}
29732983

2984+
/*
2985+
* For INSERT .. ON CONFLICT .. DO UPDATE, we must also update assorted
2986+
* stuff in the onConflict data structure.
2987+
*/
2988+
if (parsetree->onConflict &&
2989+
parsetree->onConflict->action == ONCONFLICT_UPDATE)
2990+
{
2991+
Index old_exclRelIndex,
2992+
new_exclRelIndex;
2993+
RangeTblEntry *new_exclRte;
2994+
List *tmp_tlist;
2995+
2996+
/*
2997+
* Like the INSERT/UPDATE code above, update the resnos in the
2998+
* auxiliary UPDATE targetlist to refer to columns of the base
2999+
* relation.
3000+
*/
3001+
foreach(lc, parsetree->onConflict->onConflictSet)
3002+
{
3003+
TargetEntry *tle = (TargetEntry *) lfirst(lc);
3004+
TargetEntry *view_tle;
3005+
3006+
if (tle->resjunk)
3007+
continue;
3008+
3009+
view_tle = get_tle_by_resno(view_targetlist, tle->resno);
3010+
if (view_tle != NULL && !view_tle->resjunk && IsA(view_tle->expr, Var))
3011+
tle->resno = ((Var *) view_tle->expr)->varattno;
3012+
else
3013+
elog(ERROR, "attribute number %d not found in view targetlist",
3014+
tle->resno);
3015+
}
3016+
3017+
/*
3018+
* Also, create a new RTE for the EXCLUDED pseudo-relation, using the
3019+
* query's new base rel (which may well have a different column list
3020+
* from the view, hence we need a new column alias list). This should
3021+
* match transformOnConflictClause. In particular, note that the
3022+
* relkind is set to composite to signal that we're not dealing with
3023+
* an actual relation, and no permissions checks are wanted.
3024+
*/
3025+
old_exclRelIndex = parsetree->onConflict->exclRelIndex;
3026+
3027+
new_exclRte = addRangeTableEntryForRelation(make_parsestate(NULL),
3028+
base_rel,
3029+
makeAlias("excluded",
3030+
NIL),
3031+
false, false);
3032+
new_exclRte->relkind = RELKIND_COMPOSITE_TYPE;
3033+
new_exclRte->requiredPerms = 0;
3034+
/* other permissions fields in new_exclRte are already empty */
3035+
3036+
parsetree->rtable = lappend(parsetree->rtable, new_exclRte);
3037+
new_exclRelIndex = parsetree->onConflict->exclRelIndex =
3038+
list_length(parsetree->rtable);
3039+
3040+
/*
3041+
* Replace the targetlist for the EXCLUDED pseudo-relation with a new
3042+
* one, representing the columns from the new base relation.
3043+
*/
3044+
parsetree->onConflict->exclRelTlist =
3045+
BuildOnConflictExcludedTargetlist(base_rel, new_exclRelIndex);
3046+
3047+
/*
3048+
* Update all Vars in the ON CONFLICT clause that refer to the old
3049+
* EXCLUDED pseudo-relation. We want to use the column mappings
3050+
* defined in the view targetlist, but we need the outputs to refer to
3051+
* the new EXCLUDED pseudo-relation rather than the new target RTE.
3052+
* Also notice that "EXCLUDED.*" will be expanded using the view's
3053+
* rowtype, which seems correct.
3054+
*/
3055+
tmp_tlist = copyObject(view_targetlist);
3056+
3057+
ChangeVarNodes((Node *) tmp_tlist, new_rt_index,
3058+
new_exclRelIndex, 0);
3059+
3060+
parsetree->onConflict = (OnConflictExpr *)
3061+
ReplaceVarsFromTargetList((Node *) parsetree->onConflict,
3062+
old_exclRelIndex,
3063+
0,
3064+
view_rte,
3065+
tmp_tlist,
3066+
REPLACEVARS_REPORT_ERROR,
3067+
0,
3068+
&parsetree->hasSubLinks);
3069+
}
3070+
29743071
/*
29753072
* For UPDATE/DELETE, pull up any WHERE quals from the view. We know that
29763073
* any Vars in the quals must reference the one base relation, so we need
@@ -3089,6 +3186,8 @@ rewriteTargetView(Query *parsetree, Relation view)
30893186
}
30903187
}
30913188

3189+
heap_close(base_rel, NoLock);
3190+
30923191
return parsetree;
30933192
}
30943193

‎src/include/parser/analyze.h

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,4 +42,7 @@ extern void applyLockingClause(Query *qry, Index rtindex,
4242
LockClauseStrength strength,
4343
LockWaitPolicy waitPolicy, bool pushedDown);
4444

45+
extern List *BuildOnConflictExcludedTargetlist(Relation targetrel,
46+
Index exclRelIndex);
47+
4548
#endif /* ANALYZE_H */

0 commit comments

Comments
 (0)