Re: [HACKERS] Minor ON CONFLICT related fixes
On 2015-05-19 22:50:54 -0700, Peter Geoghegan wrote: On Tue, May 19, 2015 at 2:23 PM, Andres Freund and...@anarazel.de wrote: Pushed. I eyeballed the commit, and realized that I made a trivial error. New patch attached fixing that. Sorry for not getting this fix completely right first time around. Don't know how I missed it. Pushed, with expanded tests. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor ON CONFLICT related fixes
On Tue, May 19, 2015 at 2:23 PM, Andres Freund and...@anarazel.de wrote:
Pushed.
I eyeballed the commit, and realized that I made a trivial error. New
patch attached fixing that.
Sorry for not getting this fix completely right first time around.
Don't know how I missed it.
--
Peter Geoghegan
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 8cdef08..0585251 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -5500,7 +5500,7 @@ get_insert_query_def(Query *query, deparse_context *context)
get_rule_expr(confl-arbiterWhere, context, false);
}
}
- else
+ else if (confl-constraint != InvalidOid)
{
char *constraint = get_constraint_name(confl-constraint);
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor ON CONFLICT related fixes
On 2015-05-18 19:09:27 -0700, Peter Geoghegan wrote: On Mon, May 18, 2015 at 2:09 PM, Peter Geoghegan p...@heroku.com wrote: You pointed out that the reason for this trivial bug on Jabber, but here's the obvious fix, including an EXPLAIN regression test. Also, I attach a patch adding ruleutils.c deparsing support for INSERT statement target aliases. This was previously overlooked. Pushed. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor ON CONFLICT related fixes
On Tue, May 12, 2015 at 3:16 PM, Andres Freund and...@anarazel.de wrote:
Could you rebase and adjust your patch? I'd rather have the inference
structure refactoring separate.
I realized that I didn't split out the patch as requested.
Here is a cumulative version of what was previously posted.
Thanks
--
Peter Geoghegan
From b089633e7db8a778449ea609fad012bec317f02c Mon Sep 17 00:00:00 2001
From: Peter Geoghegan peter.geoghega...@gmail.com
Date: Mon, 18 May 2015 12:49:13 -0700
Subject: [PATCH 2/2] Further fixes to minor ON CONFLICT issues
Deparsing with an inference clause is now correctly supported.
Finally, fix a few typos, and rename a variable -- candidates seemed
like a misnomer for the return value of infer_arbiter_indexes().
---
src/backend/executor/nodeModifyTable.c| 2 +-
src/backend/optimizer/util/plancat.c | 14 ++---
src/backend/parser/parse_clause.c | 2 +-
src/backend/utils/adt/ruleutils.c | 75 ++-
src/test/regress/expected/insert_conflict.out | 2 +-
src/test/regress/expected/rules.out | 38 --
src/test/regress/sql/rules.sql| 7 ++-
7 files changed, 109 insertions(+), 31 deletions(-)
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 89f1f57..72bbd62 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -414,7 +414,7 @@ ExecInsert(ModifyTableState *mtstate,
estate, true, specConflict,
arbiterIndexes);
- /* adjust the tuple's state accordingly */
+ /* adjust the tuple's state */
if (!specConflict)
heap_finish_speculative(resultRelationDesc, tuple);
else
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index cbb4776..a857ba3 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -438,8 +438,8 @@ infer_arbiter_indexes(PlannerInfo *root)
Bitmapset *inferAttrs = NULL;
List *inferElems = NIL;
- /* Result */
- List *candidates = NIL;
+ /* Results */
+ List *results = NIL;
/*
* Quickly return NIL for ON CONFLICT DO NOTHING without an inference
@@ -565,11 +565,11 @@ infer_arbiter_indexes(PlannerInfo *root)
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg(ON CONFLICT DO UPDATE not supported with exclusion constraints)));
- candidates = lappend_oid(candidates, idxForm-indexrelid);
+ results = lappend_oid(results, idxForm-indexrelid);
list_free(indexList);
index_close(idxRel, NoLock);
heap_close(relation, NoLock);
- return candidates;
+ return results;
}
else if (indexOidFromConstraint != InvalidOid)
{
@@ -660,7 +660,7 @@ infer_arbiter_indexes(PlannerInfo *root)
if (!predicate_implied_by(predExprs, whereExplicit))
goto next;
- candidates = lappend_oid(candidates, idxForm-indexrelid);
+ results = lappend_oid(results, idxForm-indexrelid);
next:
index_close(idxRel, NoLock);
}
@@ -668,12 +668,12 @@ next:
list_free(indexList);
heap_close(relation, NoLock);
- if (candidates == NIL)
+ if (results == NIL)
ereport(ERROR,
(errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
errmsg(there is no unique or exclusion constraint matching the ON CONFLICT specification)));
- return candidates;
+ return results;
}
/*
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index c8af5ab..f8eebfe 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -2765,7 +2765,7 @@ transformOnConflictArbiter(ParseState *pstate,
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg(ON CONFLICT DO UPDATE requires inference specification or constraint name),
- errhint(For example, ON CONFLICT ON CONFLICT (column).),
+ errhint(For example, ON CONFLICT (column).),
parser_errposition(pstate,
exprLocation((Node *) onConflictClause;
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index 0a77400..b54d508 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -5479,13 +5479,38 @@ get_insert_query_def(Query *query, deparse_context *context)
{
OnConflictExpr *confl = query-onConflict;
+ appendStringInfo(buf, ON CONFLICT);
+
+ if (confl-arbiterElems)
+ {
+ /* Add the single-VALUES expression list */
+ appendStringInfoChar(buf, '(');
+ get_rule_expr((Node *) confl-arbiterElems, context, false);
+ appendStringInfoChar(buf, ')');
+
+ /* Add a WHERE clause (for partial indexes) if given */
+ if (confl-arbiterWhere != NULL)
+ {
+appendContextKeyword(context, WHERE ,
+ -PRETTYINDENT_STD, PRETTYINDENT_STD, 1);
+get_rule_expr(confl-arbiterWhere, context, false);
+ }
+ }
+ else
+ {
+ char *constraint = get_constraint_name(confl-constraint);
+
+ appendStringInfo(buf, ON CONSTRAINT %s,
+
Re: [HACKERS] Minor ON CONFLICT related fixes
On Mon, May 18, 2015 at 2:09 PM, Peter Geoghegan p...@heroku.com wrote: You pointed out that the reason for this trivial bug on Jabber, but here's the obvious fix, including an EXPLAIN regression test. Also, I attach a patch adding ruleutils.c deparsing support for INSERT statement target aliases. This was previously overlooked. -- Peter Geoghegan From baaa50e2c5316ec1b4b0f1d0a9defb941ad8dbe8 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan peter.geoghega...@gmail.com Date: Mon, 18 May 2015 19:01:20 -0700 Subject: [PATCH 3/3] Deparsing support for INSERT AS alias Now that ON CONFLICT DO UPDATE has established that INSERT targets may accept an alias, have ruleutils.c perform the appropriate deparsing. --- src/backend/utils/adt/ruleutils.c | 4 1 file changed, 4 insertions(+) diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index b54d508..a32a021 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -5392,6 +5392,10 @@ get_insert_query_def(Query *query, deparse_context *context) } appendStringInfo(buf, INSERT INTO %s , generate_relation_name(rte-relid, NIL)); + /* INSERT requires AS keyword for target alias */ + if (rte-alias != NULL) + appendStringInfo(buf, AS %s, + quote_identifier(rte-alias-aliasname)); /* * Add the insert-column-names list. To handle indirection properly, we -- 1.9.1 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor ON CONFLICT related fixes
On Tue, May 12, 2015 at 4:23 PM, Peter Geoghegan p...@heroku.com wrote:
Rebased version of patch is attached.
FYI, I found an unrelated bug within ruleutils.c (looks like the
targetlist kludge in set_deparse_planstate() isn't sufficiently
general):
postgres=# explain insert into upsert as u values('Bat', 'Bar') on
conflict (key) do update set val = excluded.val where exists (select 1
from upsert ii where ii.key = excluded.key);
ERROR: XX000: bogus varno: 65000
LOCATION: get_variable, ruleutils.c:5916
Obviously Vars of varno INNER_VAR are not expected here -- the subplan
makes set_deparse_planstate() not prepare get_variable() in the same
way that it prepares similar though simpler cases (e.g. no
subquery/subplan).
I don't have any bright ideas about how to fix this offhand. I believe
in principle that we ought to be able to fish through parent planstate
within set_deparse_planstate(), just in case it is called under these
circumstances, but that's pretty ugly, and is usually going to be
unnecessary. Doesn't look like there is a good way to delay the work
till get_variable() can see what looks like this case, either.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor ON CONFLICT related fixes
On 2015-05-11 20:16:00 -0700, Peter Geoghegan wrote:
On Mon, May 11, 2015 at 7:34 PM, Andres Freund and...@anarazel.de wrote:
You should try to understand why it's failing. Just prohibiting the
rules, without understanding what's actually going on, could very well
hide a real bug.
It's not as if I have no idea. ReplaceVarsFromTargetList() is probably
quite confused by all this, because the passed nomatch_varno argument
is often rt_index -- but what about EXCLUDED.*? adjustJoinTreeList()
does not know anything about EXCLUDED.* either. I see little practical
reason to make the rewriter do any better.
I don't think any of these are actually influenced by upsert?
When I debugged the problem of the optimizer raising that target
lists error with a rule with an action with EXCLUDED.* (within
setrefs.c's fix_join_expr_mutator()), it looked like an off-by-one
issue here:
/* If it's for acceptable_rel, adjust and return it */
if (var-varno == context-acceptable_rel)
{
var = copyVar(var);
var-varno += context-rtoffset;
if (var-varnoold 0)
var-varnoold += context-rtoffset;
return (Node *) var;
}
That's just a straight up bug. expression_tree_walker (called via
ChangeVarNodes) did not know about exclRelTlist, leading to
fix_join_expr() not matching the excluded vars to the excluded
relation/tlist.
I'm not immediately seing how that could bit us otherwise today, but
it'd definitely otherwise be a trap. That's why I think it's unwise to
ignore problems before having fully debugged them.
Additionally OffsetVarNodes() did not adjust exclRelIndex, which
breaks explain insofar it's not going to display the 'excluded.'
anymore. I don't think it could have other consequences.
Greetings,
Andres Freund
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor ON CONFLICT related fixes
On Tue, May 12, 2015 at 3:16 PM, Andres Freund and...@anarazel.de wrote: Could you rebase and adjust your patch? I'd rather have the inference structure refactoring separate. Sure. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor ON CONFLICT related fixes
On Tue, May 12, 2015 at 2:40 PM, Andres Freund and...@anarazel.de wrote:
It's not as if I have no idea. ReplaceVarsFromTargetList() is probably
quite confused by all this, because the passed nomatch_varno argument
is often rt_index -- but what about EXCLUDED.*? adjustJoinTreeList()
does not know anything about EXCLUDED.* either. I see little practical
reason to make the rewriter do any better.
I don't think any of these are actually influenced by upsert?
Evidently you're right. I'm not opposed to supporting ON CONFLICT
UPDATE with CREATE RULE, even if such rules are completely wonky. It
might be a useful way of testing the implementation.
When I debugged the problem of the optimizer raising that target
lists error with a rule with an action with EXCLUDED.* (within
setrefs.c's fix_join_expr_mutator()), it looked like an off-by-one
issue here:
/* If it's for acceptable_rel, adjust and return it */
if (var-varno == context-acceptable_rel)
{
var = copyVar(var);
var-varno += context-rtoffset;
if (var-varnoold 0)
var-varnoold += context-rtoffset;
return (Node *) var;
}
That's just a straight up bug. expression_tree_walker (called via
ChangeVarNodes) did not know about exclRelTlist, leading to
fix_join_expr() not matching the excluded vars to the excluded
relation/tlist.
The omissions you mention should be corrected on general principle, I think.
I'm not immediately seing how that could bit us otherwise today, but
it'd definitely otherwise be a trap. That's why I think it's unwise to
ignore problems before having fully debugged them.
Fair point.
Additionally OffsetVarNodes() did not adjust exclRelIndex, which
breaks explain insofar it's not going to display the 'excluded.'
anymore. I don't think it could have other consequences.
Okay.
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor ON CONFLICT related fixes
On Tue, May 12, 2015 at 3:18 PM, Peter Geoghegan p...@heroku.com wrote:
On Tue, May 12, 2015 at 3:16 PM, Andres Freund and...@anarazel.de wrote:
Could you rebase and adjust your patch? I'd rather have the inference
structure refactoring separate.
Sure.
Rebased version of patch is attached.
--
Peter Geoghegan
From cc2fcd8862e01880b1559f315aa3da23017edfe6 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan peter.geoghega...@gmail.com
Date: Mon, 11 May 2015 15:37:54 -0700
Subject: [PATCH] Further fixes to minor ON CONFLICT issues
Deparsing with an inference clause is now correctly supported. In
passing, re-factor InferenceElem representation of opclass, to defer
opfamily lookup for Relation index matching until index inference proper
(i.e., within the optimizer). This is done for the convenience of the
new ruleutils.c code, but independently make senses.
Finally, fix a few typos, and rename a variable -- candidates seemed
like a misnomer for the return value of infer_arbiter_indexes().
---
contrib/pg_stat_statements/pg_stat_statements.c | 3 +-
src/backend/executor/nodeModifyTable.c | 2 +-
src/backend/nodes/copyfuncs.c | 3 +-
src/backend/nodes/equalfuncs.c | 3 +-
src/backend/nodes/outfuncs.c| 3 +-
src/backend/nodes/readfuncs.c | 3 +-
src/backend/optimizer/util/plancat.c| 34 +++
src/backend/parser/parse_clause.c | 16 ++
src/backend/utils/adt/ruleutils.c | 75 -
src/include/nodes/primnodes.h | 3 +-
src/test/regress/expected/insert_conflict.out | 2 +-
src/test/regress/expected/rules.out | 38 +++--
src/test/regress/sql/rules.sql | 7 ++-
13 files changed, 133 insertions(+), 59 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 6abe3f0..f97cc2c 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -2637,8 +2637,7 @@ JumbleExpr(pgssJumbleState *jstate, Node *node)
InferenceElem *ie = (InferenceElem *) node;
APP_JUMB(ie-infercollid);
-APP_JUMB(ie-inferopfamily);
-APP_JUMB(ie-inferopcinputtype);
+APP_JUMB(ie-inferopclass);
JumbleExpr(jstate, ie-expr);
}
break;
diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index 89f1f57..72bbd62 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -414,7 +414,7 @@ ExecInsert(ModifyTableState *mtstate,
estate, true, specConflict,
arbiterIndexes);
- /* adjust the tuple's state accordingly */
+ /* adjust the tuple's state */
if (!specConflict)
heap_finish_speculative(resultRelationDesc, tuple);
else
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 76b63af..957c2bc 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -1803,8 +1803,7 @@ _copyInferenceElem(const InferenceElem *from)
COPY_NODE_FIELD(expr);
COPY_SCALAR_FIELD(infercollid);
- COPY_SCALAR_FIELD(inferopfamily);
- COPY_SCALAR_FIELD(inferopcinputtype);
+ COPY_SCALAR_FIELD(inferopclass);
return newnode;
}
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index e032142..f26626e 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -687,8 +687,7 @@ _equalInferenceElem(const InferenceElem *a, const InferenceElem *b)
{
COMPARE_NODE_FIELD(expr);
COMPARE_SCALAR_FIELD(infercollid);
- COMPARE_SCALAR_FIELD(inferopfamily);
- COMPARE_SCALAR_FIELD(inferopcinputtype);
+ COMPARE_SCALAR_FIELD(inferopclass);
return true;
}
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index fe868b8..454d9ec 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -1450,8 +1450,7 @@ _outInferenceElem(StringInfo str, const InferenceElem *node)
WRITE_NODE_FIELD(expr);
WRITE_OID_FIELD(infercollid);
- WRITE_OID_FIELD(inferopfamily);
- WRITE_OID_FIELD(inferopcinputtype);
+ WRITE_OID_FIELD(inferopclass);
}
static void
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 8136306..81b6243 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -1141,8 +1141,7 @@ _readInferenceElem(void)
READ_NODE_FIELD(expr);
READ_OID_FIELD(infercollid);
- READ_OID_FIELD(inferopfamily);
- READ_OID_FIELD(inferopcinputtype);
+ READ_OID_FIELD(inferopclass);
READ_DONE();
}
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index b425680..a857ba3 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -438,8 +438,8 @@ infer_arbiter_indexes(PlannerInfo *root)
Bitmapset *inferAttrs = NULL;
List
Re: [HACKERS] Minor ON CONFLICT related fixes
On 2015-05-12 23:40:47 +0200, Andres Freund wrote: That's just a straight up bug. expression_tree_walker (called via ChangeVarNodes) did not know about exclRelTlist, leading to fix_join_expr() not matching the excluded vars to the excluded relation/tlist. I've fixed these, and added a minimum amount of tests. Pleas echeck whether more are needed. Could you rebase and adjust your patch? I'd rather have the inference structure refactoring separate. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor ON CONFLICT related fixes
HI, Don't have time to go through this in depth. On 2015-05-11 18:53:22 -0700, Peter Geoghegan wrote: Note that the patch proposes to de-support CREATE RULE with an alternative action involving ON CONFLICT DO UPDATE. Such a rule seems particularly questionable to me, and I'm pretty sure it was understood that only ON CONFLICT DO NOTHING should be supported as an action for a rule (recall that INSERT statements with ON CONFLICT can, in general, never target relations with rules). At least, I believe Heikki said that. Deparsing with an inference clause is now correctly supported. However, user-defined rules with ON CONFLICT DO UPDATE are now formally disallowed/unsupported. It seemed there would be significant complexity involved in making this work correctly with the EXCLUDED.* pseudo-relation, which was not deemed worthwhile. Such a user-defined rule seems very questionable anyway. Do I understand correctly that you cut this out because there essentially was a ruleutils bug with with EXCLUDED? If so, I don't find that acceptable. I'm pretty strictly convincded that independent of rule support, we shouldn't add things to insert queries that get_query_def can't deparse. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor ON CONFLICT related fixes
On 2015-05-11 19:33:02 -0700, Peter Geoghegan wrote: On Mon, May 11, 2015 at 7:22 PM, Peter Geoghegan p...@heroku.com wrote: You just get an error within the optimizer following rewriting of a query involving the application of a rule that specifies an ON CONFLICT DO UPDATE alternative/DO INSTEAD action. I found it would say: variable not found in subplan target lists. BTW, that error was only raised when the EXCLUDED.* pseudo-alias was actually used, which is why our previous testing missed it. You should try to understand why it's failing. Just prohibiting the rules, without understanding what's actually going on, could very well hide a real bug. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor ON CONFLICT related fixes
On Mon, May 11, 2015 at 7:34 PM, Andres Freund and...@anarazel.de wrote:
You should try to understand why it's failing. Just prohibiting the
rules, without understanding what's actually going on, could very well
hide a real bug.
It's not as if I have no idea. ReplaceVarsFromTargetList() is probably
quite confused by all this, because the passed nomatch_varno argument
is often rt_index -- but what about EXCLUDED.*? adjustJoinTreeList()
does not know anything about EXCLUDED.* either. I see little practical
reason to make the rewriter do any better.
When I debugged the problem of the optimizer raising that target
lists error with a rule with an action with EXCLUDED.* (within
setrefs.c's fix_join_expr_mutator()), it looked like an off-by-one
issue here:
/* If it's for acceptable_rel, adjust and return it */
if (var-varno == context-acceptable_rel)
{
var = copyVar(var);
var-varno += context-rtoffset;
if (var-varnoold 0)
var-varnoold += context-rtoffset;
return (Node *) var;
}
--
Peter Geoghegan
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor ON CONFLICT related fixes
On Mon, May 11, 2015 at 6:53 PM, Peter Geoghegan p...@heroku.com wrote: Attached patch fixes some issues with ON CONFLICT DO UPDATE/DO NOTHING. There is a commit message which explains the changes at a high level. I just realized that there is a silly bug here. Attached is a fix that applies on top of the original. -- Peter Geoghegan From 1b32558d188762eb5c7214ea5ae042897e7d004f Mon Sep 17 00:00:00 2001 From: Peter Geoghegan peter.geoghega...@gmail.com Date: Mon, 11 May 2015 19:02:12 -0700 Subject: [PATCH 2/2] Add closing bracket --- src/backend/utils/adt/ruleutils.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c index 355acc9..aa21693 100644 --- a/src/backend/utils/adt/ruleutils.c +++ b/src/backend/utils/adt/ruleutils.c @@ -7768,6 +7768,9 @@ get_rule_expr(Node *node, deparse_context *context, get_rule_expr((Node *) iexpr-expr, context, showimplicit); +if (need_parens) + appendStringInfoChar(buf, ')'); + context-varprefix = varprefix; if (iexpr-infercollid) @@ -7782,8 +7785,6 @@ get_rule_expr(Node *node, deparse_context *context, get_opclass_name(inferopclass, inferopcinputtype, buf); } -if (need_parens) - appendStringInfoChar(buf, ')'); } break; -- 1.9.1 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor ON CONFLICT related fixes
On Mon, May 11, 2015 at 7:11 PM, Andres Freund and...@anarazel.de wrote: Do I understand correctly that you cut this out because there essentially was a ruleutils bug with with EXCLUDED? If so, I don't find that acceptable. I'm pretty strictly convincded that independent of rule support, we shouldn't add things to insert queries that get_query_def can't deparse. No, that wasn't the reason -- deparsing itself works fine. That code remains within ruleutils.c because I agree with this principle of yours. I tested the deparsing logic before making the case added fail as unsupported. If you remove the new ereport() call that relates to non-suppport of ON CONFLICT DO UPDATE as a rule action, then the CREATE RULE still succeeds, and you can deparse the query just fine (by quering pg_rules, typically). You just get an error within the optimizer following rewriting of a query involving the application of a rule that specifies an ON CONFLICT DO UPDATE alternative/DO INSTEAD action. I found it would say: variable not found in subplan target lists. I can't say that I explored the question very thoroughly, but it seems bothersome to have more than one relation involved in a Query involved in rewriting. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor ON CONFLICT related fixes
On Mon, May 11, 2015 at 7:22 PM, Peter Geoghegan p...@heroku.com wrote: You just get an error within the optimizer following rewriting of a query involving the application of a rule that specifies an ON CONFLICT DO UPDATE alternative/DO INSTEAD action. I found it would say: variable not found in subplan target lists. BTW, that error was only raised when the EXCLUDED.* pseudo-alias was actually used, which is why our previous testing missed it. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
