Free indexed_tlist memory explicitly within set_plan_refs()

On Mon, May 25, 2015 at 10:17 AM, Peter Geoghegan <pg@heroku.com> wrote:

While trying to fix a largely unrelated bug, I noticed that the new
build_tlist_index() call for the "excluded" targetlist (used by ON
CONFLICT DO UPDATE queries) does not have its memory subsequently
freed by the caller. Since every other call to build_tlist_index()
does this, and comments above build_tlist_index() encourage it, I
think the new caller should do the same.

Attached patch adds such a pfree() call.

Yep. This looks correct to me.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, May 24, 2015 at 6:17 PM, Peter Geoghegan <pg@heroku.com> wrote:

Attached patch adds such a pfree() call.

Attached, revised version incorporates this small fix, while adding an
additional big fix, and a number of small style tweaks.

This is mainly concerned with fixing the bug I was trying to fix when
I spotted the minor pfree() issue:

postgres=# insert into upsert (key, val) values('Foo', 'Bar') on
conflict (key) do update set val = excluded.val where excluded.* is
not null;
ERROR: XX000: variable not found in subplan target lists
LOCATION: fix_join_expr_mutator, setrefs.c:2003
postgres=# insert into upsert (key, val) values(('Foo', 'Bar') on
conflict (key) do update set val = excluded.val where excluded.ctid is
not null;
ERROR: XX000: variable not found in subplan target lists
LOCATION: fix_join_expr_mutator, setrefs.c:2003

The first query shown should clearly finish processing by the
optimizer without raising this error message (execution should work
correctly too, of course). The second query shown should fail with a
user visible error message about referencing the excluded
pseudo-relation's ctid column not making sense.

The basic problem is that there wasn't much thought put into how the
excluded pseudo-relation's "reltargetlist" is generated -- it
currently comes from a call to expandRelAttrs() during parse analysis,
which, on its own, doesn't allow whole row Vars to work.

One approach to fixing this is to follow the example of RETURNING
lists with references to more than one relation:
preprocess_targetlist() handles this by calling pull_var_clause() and
making new TargetEntry entries for Vars found to not be referencing
the target (and not already in the targetlist for some other reason).
Another approach, preferred by Andres, is to have query_planner() do
more. I understand that the idea there is to make excluded.* closer to
a regular table, in that it can be expected to have a baserel, and
within the executor we have something closer to a bona-fide scan
reltargetlist, that we can expect to have all Vars appear in. This
should be enough to make the reltargetlist have the appropriate Vars
more or less in the regular course of planning, including excluded.*
whole row Vars. To make this work we could call
add_vars_to_targetlist(), and call add_base_rels_to_query() and then
build_base_rel_tlists() within query_planner() (while moving a few
other things around).

However, the ordering dependencies within query_planner() seemed quite
complicated to me, and I didn't want to modify such an important
routine just to fix this bug. RETURNING seemed like a perfectly good
precedent to follow, so that's what I did. Now, it might have been
that I misunderstood Andres when we discussed this problem on
Jabber/IM, but ISTM that the second idea doesn't have much advantage
over the first (I'm sure that Andres will be able to explain what he'd
like to do better here -- it was a quick conversation). I did
prototype the second approach, and the code footprint of what I have
here (the first approach) seems lower than it would have to be with
the second. Besides, I didn't see a convenient choke point to reject
system column references with the second approach. Attached patch
fixes the bug using the first approach. Tests were added demonstrating
that the cases above are fixed.

A second attached patch fixes another, largely independent bug. I
noticed array assignment with ON CONFLICT DO UPDATE was broken. See
commit message for full details.

Thoughts?
--
Peter Geoghegan

On Thu, May 28, 2015 at 2:37 PM, Peter Geoghegan <pg@heroku.com> wrote:

A second attached patch fixes another, largely independent bug. I
noticed array assignment with ON CONFLICT DO UPDATE was broken. See
commit message for full details.

Finally, here is a third patch, fixing the final bug that I discussed
with you privately. There are now fixes for all bugs that I'm
currently aware of.

This concerns a thinko in unique index inference. See the commit
message for full details.

--
Peter Geoghegan

On Thu, May 28, 2015 at 6:31 PM, Peter Geoghegan <pg@heroku.com> wrote:

This concerns a thinko in unique index inference. See the commit
message for full details.

It seems I missed a required defensive measure here. Attached patch
adds it, too.

--
Peter Geoghegan

On Thu, May 28, 2015 at 2:37 PM, Peter Geoghegan <pg@heroku.com> wrote:

Attached, revised version incorporates this small fix, while adding an
additional big fix, and a number of small style tweaks.

This is mainly concerned with fixing the bug I was trying to fix when
I spotted the minor pfree() issue:

postgres=# insert into upsert (key, val) values('Foo', 'Bar') on
conflict (key) do update set val = excluded.val where excluded.* is
not null;
ERROR: XX000: variable not found in subplan target lists
LOCATION: fix_join_expr_mutator, setrefs.c:2003

My fix for this issue
(0001-Fix-bug-with-whole-row-Vars-in-excluded-targetlist.patch) still
missed something. There needs to be additional handling in
ruleutils.c:

postgres=# explain insert into upsert as u
values (1, 'fooz') on conflict (key)
do update set val = excluded.val where excluded.* is not null;
ERROR: XX000: bogus varattno for INNER_VAR var: 0
LOCATION: get_variable, ruleutils.c:5904

I'll look for a fix for this additional issue tomorrow.
--
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

On Sat, May 30, 2015 at 1:07 AM, Peter Geoghegan <pg@heroku.com> wrote:

My fix for this issue
(0001-Fix-bug-with-whole-row-Vars-in-excluded-targetlist.patch) still
missed something. There needs to be additional handling in
ruleutils.c:

Debugging this allowed me to come up with a significantly simplified
approach. Attached is a new version of the original fix. Details are
in commit message -- there is no actual need to have
search_indexed_tlist_for_var() care about Vars being resjunk in a
special way, which is a good thing. There is also no need for further
ruleutils.c specialization, as I implied before.

Some deparsing tests are now included on top of what was already in
the first version.
--
Peter Geoghegan

On Sat, May 30, 2015 at 3:12 PM, Peter Geoghegan <pg@heroku.com> wrote:

Debugging this allowed me to come up with a significantly simplified
approach. Attached is a new version of the original fix. Details are
in commit message -- there is no actual need to have
search_indexed_tlist_for_var() care about Vars being resjunk in a
special way, which is a good thing.

It feels wrong to not have the additional, paranoid IsVar() check
within pull_var_targetlist_clause() check added in most recent
revision, even though it should not be necessary. Attached delta patch
adds this check.

I need to stop working on weekends...
--
Peter Geoghegan

On 2015-05-28 14:37:43 -0700, Peter Geoghegan wrote:

To fix, allow ParseState to reflect that an individual statement can be
both p_is_insert and p_is_update at the same time.

/* Process DO UPDATE */
if (onConflictClause->action == ONCONFLICT_UPDATE)
{
+		/* p_is_update must be set here, after INSERT targetlist processing */
+		pstate->p_is_update = true;
+

It's not particularly pretty that you document in the commit message
that both is_insert and is_update can be set at the same time, and then
it has constraints like the above.

But that's more crummy API's fault than yours.

I'm right now not really coming up with a better idea how to fix
this. So I guess I'll apply something close to this tomorrow.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, Jul 12, 2015 at 1:45 PM, Andres Freund <andres@anarazel.de> wrote:

But that's more crummy API's fault than yours.

As you probably noticed, the only reason the p_is_update and
p_is_insert fields exist is for transformAssignedExpr() -- in fact, in
the master branch, nothing checks the value of p_is_update (although I
suppose a hook into a third-party module could see that module test
ParseState.p_is_update, so that isn't quite true).

I'm right now not really coming up with a better idea how to fix
this. So I guess I'll apply something close to this tomorrow.

Sounds good.

--
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

On 2015-07-12 22:45:18 +0200, Andres Freund wrote:

I'm right now not really coming up with a better idea how to fix
this. So I guess I'll apply something close to this tomorrow.

Took a bit longer than that :(

I've pushed a version of your patch. I just opted to remove p_is_update
instead of allowing both to be set at the same time. To me that seemed
simpler.

Thanks for the fix!

Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2015-05-28 18:31:56 -0700, Peter Geoghegan wrote:

Subject: [PATCH 3/3] Fix bug in unique index inference

ON CONFLICT unique index inference had a thinko that could affect cases
where the user-supplied inference clause required that an attribute
match a particular (user named) collation and/or opclass.

Firstly, infer_collation_opclass_match() matched on opclass and/or
collation. Secondly, the attribute had to be in the list of attributes
or expressions known to be in the definition of the index under
consideration. The second step wasn't correct though, because having
some match doesn't necessarily mean that the second step found the same
index attribute as the (collation/opclass wise) match from the first
step.

Yes, makes sense.

+		else
+		{
+			Node	   *nattExpr = list_nth(idxExprs, (natt - 1) - nplain);
+
+			/*
+			 * Note that unlike routines like match_index_to_operand(), we're
+			 * unconcerned about RelabelType.  An exact match is required.
+			 */
+			if (equal(elem->expr, nattExpr))
+				return true;

Why is that?

Regads,

Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Jul 24, 2015 at 2:58 AM, Andres Freund <andres@anarazel.de> wrote:

I've pushed a version of your patch. I just opted to remove p_is_update
instead of allowing both to be set at the same time. To me that seemed
simpler.

I would be hesitant to remove a struct field from a struct that
appears as a hook argument. Someone's extension (that uses parser
hooks) might have been relying on that. Perhaps not a big deal.

Thanks
--
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

On July 24, 2015 7:57:43 PM GMT+02:00, Peter Geoghegan <pg@heroku.com> wrote:

On Fri, Jul 24, 2015 at 2:58 AM, Andres Freund <andres@anarazel.de>
wrote:

I've pushed a version of your patch. I just opted to remove

p_is_update

instead of allowing both to be set at the same time. To me that

seemed

simpler.

I would be hesitant to remove a struct field from a struct that
appears as a hook argument. Someone's extension (that uses parser
hooks) might have been relying on that. Perhaps not a big deal.

They'd also be affected by the change in meaning...

--- 
Please excuse brevity and formatting - I am writing this on my mobile phone.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Jul 24, 2015 at 3:08 AM, Andres Freund <andres@anarazel.de> wrote:

+             else
+             {
+                     Node       *nattExpr = list_nth(idxExprs, (natt - 1) - nplain);
+
+                     /*
+                      * Note that unlike routines like match_index_to_operand(), we're
+                      * unconcerned about RelabelType.  An exact match is required.
+                      */
+                     if (equal(elem->expr, nattExpr))
+                             return true;

Why is that?

No very strong reason. RelabelType exists to represent a dummy
coercion between two binary-compatible types. I think that a unique
index inference specification (which is novel in some ways) does not
need to do anything special for this case.

Each inference specification attribute that is an expression should
match some attribute in some index's cataloged definition. The
inference specification looks very much like the CREATE UNIQUE INDEX
that created the unique index that is inferred (usually, they'll be
identical). No need to make it any more complicated than that.

In fact, I don't think it's possible to construct a case where it
could even be argued that it matters. I'm not very caffeinated at the
moment, so I'm not sure of that.

--
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

Hi,

To recap for other readers: There's a problem with ON CONFLICT when the
SET or ON CONFLICT ... WHERE clause references excluded.* (i.e. as a
whole row var). The problem is that setrefs.c in
fix_join_expr_mutator() currently won't find a matching entry in the
indexed tlist and thus error out with
elog(ERROR, "variable not found in subplan target lists");

The reason is that the targetlist we build the index list on just
contains the attributes in excluded.*.

Peter's patch upthread fixes this by pulling expressions from
onConflictSet/Where into the targetlist. I disliked this - much less
than initially - a bit because that seems a bit crufty given that we're
not actually getting data from a child node. This is different to
RETURNING where the targetlist massaging is actually important to get
the data up the tree.

An actually trivial, although not all that pretty, fix is to simply
accept wholerow references in fix_join_expr_mutator(), even if not in
the targetlist. As far as I can see the problem right now really can
only be hit for whole row references.

A variant of the second approach is to have a fix_onconflict_expr()
mutator that has such special handler.

Any opinions on either approach?

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

On Sat, Sep 19, 2015 at 5:11 PM, Andres Freund <andres@anarazel.de> wrote:

Peter's patch upthread fixes this by pulling expressions from
onConflictSet/Where into the targetlist. I disliked this - much less
than initially - a bit because that seems a bit crufty given that we're
not actually getting data from a child node. This is different to
RETURNING where the targetlist massaging is actually important to get
the data up the tree.

Maybe the massaging is somewhat justified by the fact that it's just
as good a place as any to reject system columns, and that needs to
happen somewhere. I know that you suggested that this be done during
parse analysis, but not sure how attached you are to that. Might also
be a good choke point for detecting when unexpected vars/expressions
appear in the targetlist due to unforeseen circumstances/bugs. I
actually cover a couple of "can't happen" cases at the same time,
IIRC.

Continuing to follow RETURNING may have some value, even if the
analogy is a bit more forced here.

An actually trivial, although not all that pretty, fix is to simply
accept wholerow references in fix_join_expr_mutator(), even if not in
the targetlist. As far as I can see the problem right now really can
only be hit for whole row references.

I am concerned about the risk of adding bugs to unrelated code paths
that this could create. I must admit that this concern is mostly
driven by paranoia, and not a seasoned appreciation of problems that
could arise from ordinary post-processing of join expressions.

A variant of the second approach is to have a fix_onconflict_expr()
mutator that has such special handler.

I suppose you could add a fix_join_expr_context field that had
fix_join_expr_mutator() avoid the special handler for post-processing
of join expressions. That might be a bit ugly too, but would involve
less code duplication.

Any opinions on either approach?

I think that I favor my original solution, although only by a tiny
margin. I will avoid offering either a -1 or a +1 to any proposal
here, although they all sound basically reasonable to me. A more
complete targetlist representation would have been something that I'd
probably vote against, since it seems complex and invasive, but that
doesn't matter now. In short, I defer to others here.

--
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

On 2015-09-19 18:40:14 -0700, Peter Geoghegan wrote:

An actually trivial, although not all that pretty, fix is to simply
accept wholerow references in fix_join_expr_mutator(), even if not in
the targetlist. As far as I can see the problem right now really can
only be hit for whole row references.

I am concerned about the risk of adding bugs to unrelated code paths
that this could create.

How? This is a must-not-reach code path currently?

Stuff I want to fix by tomorrow:
* Whole row var references to exclude
* wrong offsets for columns after dropped ones
* INSTEAD DO UPDATE for tables with oids

Do you know of anything else?

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

On Thu, Sep 24, 2015 at 8:25 AM, Andres Freund <andres@anarazel.de> wrote:

Stuff I want to fix by tomorrow:
* Whole row var references to exclude
* wrong offsets for columns after dropped ones
* INSTEAD DO UPDATE for tables with oids

Do you know of anything else?

You said something in Dallas about the test case developed by Amit
Langote touching on a different bug to the regression test I came up
with. If that is the case, then you didn't list that one separately.
Otherwise, no.

--
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

On 2015-09-24 17:25:21 +0200, Andres Freund wrote:

Stuff I want to fix by tomorrow:
* Whole row var references to exclude
* wrong offsets for columns after dropped ones
* INSTEAD DO UPDATE for tables with oids

Do you know of anything else?

So, took a bit longer than "tomorrow. I fought for a long while with a
mysterious issue, which turned out to be separate bug: The excluded
relation was affected by row level security policies, which doesn't make
sense.

My proposal in this WIP patch is to make it a bit clearer that
'EXCLUDED' isn't a real relation. I played around with adding a
different rtekind, but that's too heavy a hammer. What I instead did was
to set relkind to composite - which seems to signal pretty well that
we're not dealing with a real relation. That immediately fixes the RLS
issue as fireRIRrules has the following check:
if (rte->rtekind != RTE_RELATION ||
rte->relkind != RELKIND_RELATION)
continue;
It also makes it relatively straightforward to fix the system column
issue by adding an additional relkind check to scanRTEForColumn's system
column handling.

WRT to the wholerow issue: There's currently two reasons we need a
targetlist entry for excluded wholerow vars: 1) setrefs.c errors out
without - that can relativley easily be worked around 2) ruleutils.c
expects an entry in the child tlist. That could also be worked around,
but it's a bit more verbose. I'm inclined to not go the pullup route
but instead simply unconditionally add a wholerow var to the excluded
tlist.

Peter, what do you think?

Andres