[PATCH] Caching for stable expressions with constant arguments v3

All,

I'd love to see someone evaluate the impact of Marti's patch on JDBC applications which use named prepared statements. Anyone have a benchmark handy?

--Josh

On Sat, Sep 24, 2011 at 9:09 PM, Marti Raudsepp <marti@juffo.org> wrote:

Hi list!

This is the third version of my CacheExpr patch.

i wanted to try this, but it no longer applies in head... specially
because of the change of IF's for a switch in
src/backend/optimizer/util/clauses.c it also needs adjustment for the
rangetypes regress test

--
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación

On 25.09.2011 05:09, Marti Raudsepp wrote:

This is the third version of my CacheExpr patch.

This seems to have bitrotted, thanks to the recent refactoring in
eval_const_expressions().

For explanation about design decisions, please read these earlier messages:
http://archives.postgresql.org/pgsql-hackers/2011-09/msg00579.php
http://archives.postgresql.org/pgsql-hackers/2011-09/msg00812.php
http://archives.postgresql.org/pgsql-hackers/2011-09/msg00833.php

I wonder if it would be better to add the CacheExpr nodes to the tree as
a separate pass, instead of shoehorning it into eval_const_expressions?
I think would be more readable that way, even though a separate pass
would be more expensive. And there are callers of
eval_const_expressions() that have no use for the caching, like
process_implied_equality().

This comment in RelationGetExpressions() also worries me:

/*
* Run the expressions through eval_const_expressions. This is not just an
* optimization, but is necessary, because the planner will be comparing
* them to similarly-processed qual clauses, and may fail to detect valid
* matches without this. We don't bother with canonicalize_qual, however.
*/
result = (List *) eval_const_expressions(NULL, (Node *) result);

Do the injected CacheExprs screw up that equality? Or the constraint
exclusion logic in predtest.c?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

I wonder if it would be better to add the CacheExpr nodes to the tree as
a separate pass, instead of shoehorning it into eval_const_expressions?
I think would be more readable that way, even though a separate pass
would be more expensive.

A separate pass would be very considerably more expensive, because
(1) it would require making a whole new copy of each expression tree,
and (2) it would require looking up the volatility status of each
function and operator. eval_const_expressions already has to do the
latter, or has to do it in a lot of cases anyway, so I think it's
probably the best place to add this. If it weren't for (2) I would
suggest adding the work to setrefs.c instead, but as it is I think
we'd better suck it up and deal with any fallout in the later stages
of the planner.

regards, tom lane

On Sun, Dec 4, 2011 at 22:53, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

This seems to have bitrotted, thanks to the recent refactoring in
eval_const_expressions().

Luckily the conflicts are mostly whitespace changes, so shouldn't be
hard to fix. I'll try to come up with an updated patch today or
tomorrow.

I wonder if it would be better to add the CacheExpr nodes to the tree as a
separate pass, instead of shoehorning it into eval_const_expressions? I
think would be more readable that way, even though a separate pass would be
more expensive. And there are callers of eval_const_expressions() that have
no use for the caching, like process_implied_equality().

Per Tom's comment, I won't split out the cache insertion for now.

The context struct has a boolean 'cache' attribute that controls
whether caching is desired or not. I think this could be exposed to
the caller as an eval_const_expressions() argument.

This comment in RelationGetExpressions() also worries me:

[...]

Do the injected CacheExprs screw up that equality? Or the constraint
exclusion logic in predtest.c?

I suspect these cases are guaranteed not to produce any CacheExprs.
They're always immutable expressions. If they contain Var references
they're stored as is (not cachable); if not, they're folded to a
constant.

But I will have to double-check all the callers; it might be a good
idea to disable caching anyway in these cases.

Regards,
Marti

Marti Raudsepp <marti@juffo.org> writes:

On Sun, Dec 4, 2011 at 22:53, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

This comment in RelationGetExpressions() also worries me:

[...]

Do the injected CacheExprs screw up that equality? Or the constraint
exclusion logic in predtest.c?

I suspect these cases are guaranteed not to produce any CacheExprs.
They're always immutable expressions. If they contain Var references
they're stored as is (not cachable); if not, they're folded to a
constant.

But I will have to double-check all the callers; it might be a good
idea to disable caching anyway in these cases.

I think if you have some call sites inject CacheExprs and others not,
it will get more difficult to match up expressions that should be
considered equal. On the whole this seems like a bad idea. What is
the reason for having such a control boolean in the first place?

regards, tom lane

On Mon, Dec 5, 2011 at 19:31, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I think if you have some call sites inject CacheExprs and others not,
it will get more difficult to match up expressions that should be
considered equal.  On the whole this seems like a bad idea.  What is
the reason for having such a control boolean in the first place?

It's needed for correctness with PL/pgSQL simple expressions.

This seems a bit of a kludge, but I considered it the "least bad"
solution. Here's what I added to planner.c standard_planner():

/*
* glob->isSimple is a hint to eval_const_expressions() and PL/pgSQL that
* this statement is potentially a simple expression -- it contains no
* table references, no subqueries and no join clauses.
*
* We need this here because this prevents insertion of CacheExpr, which
* would break simple expressions in PL/pgSQL. Such queries wouldn't
* benefit from constant caching anyway.
*
* The actual definition of a simple statement is more strict, but we
* don't want to spend that checking overhead here.
*
* Caveat: Queries with set-returning functions in SELECT list could
* still potentially benefit from caching, but we don't check that now.
*/
glob->isSimple = (parse->commandType == CMD_SELECT &&
parse->jointree->fromlist == NIL &&
parse->hasSubLinks == FALSE &&
parse->cteList == NIL);

----

I considered stripping CacheExpr nodes later in PL/pgSQL, but I can't
remember right now why I rejected that approach (sorry, it's been 2
months).

Currently I'm also disabling CacheExpr nodes in
estimate_expression_value() since we know for a fact that the planner
only evaluates it once. But that probably doesn't make much of a
difference.

Regards,
Marti

On 05.12.2011 20:53, Marti Raudsepp wrote:

I considered stripping CacheExpr nodes later in PL/pgSQL, but I can't
remember right now why I rejected that approach (sorry, it's been 2
months).

Yet another idea would be to leave the CacheExprs there, but provide a
way to reset the caches. PL/pgSQL could then reset the caches between
every invocation. Or pass a flag to ExecInitExpr() to skip through the
CacheExprs.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

On 05.12.2011 20:53, Marti Raudsepp wrote:

I considered stripping CacheExpr nodes later in PL/pgSQL, but I can't
remember right now why I rejected that approach (sorry, it's been 2
months).

Yet another idea would be to leave the CacheExprs there, but provide a
way to reset the caches. PL/pgSQL could then reset the caches between
every invocation.

We're likely to need a way to reset these caches anyway, at some point...

Or pass a flag to ExecInitExpr() to skip through the CacheExprs.

Not sure what you mean by that --- are you imagining that the ExprState
tree would have structure not matching the Expr tree? That seems just
about guaranteed to break something somewhere.

regards, tom lane

On 05.12.2011 21:36, Tom Lane wrote:

Heikki Linnakangas<heikki.linnakangas@enterprisedb.com> writes:

Or pass a flag to ExecInitExpr() to skip through the CacheExprs.

Not sure what you mean by that --- are you imagining that the ExprState
tree would have structure not matching the Expr tree?

Yes. Or it could just set a flag in the CacheExprState nodes to not do
the caching.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Sun, Dec 4, 2011 at 22:53, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

This seems to have bitrotted, thanks to the recent refactoring in
eval_const_expressions().

So here's the rebased version of this. It probably took me longer to
rebase this than it took Andres to write his patch ;)

Passes all regression tests and diffing clauses.c seems to have
introduced no functional differences.

But the good news is that I learned how to use git rebase and
pgindent, so it was worth it.

I will look into addressing some of the comments tomorrow, much thanks
for the feedback everyone!

Regards,
Marti

On Mon, Dec 5, 2011 at 21:04, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

Yet another idea would be to leave the CacheExprs there, but provide a way
to reset the caches. PL/pgSQL could then reset the caches between every
invocation. Or pass a flag to ExecInitExpr() to skip through the CacheExprs.

Great idea, I've ripped out all the conditional CacheExpr generation
logic from the planner and added a new boolean to CacheExprState
instead. This makes me happy as I'm now rid of most kludgy bits and
reduced the patch size somewhat. This also solves Tom's concern.

ExecInitExpr enables the cache when its 'PlanState *parent' attribute
isn't NULL. On the one hand this works out well since PlanState always
has a predictable life cycle thus caching is always safe.

On the other hand, a few places lose caching support this way since
they don't go through the planner:
* Column defaults in a COPY FROM operation. Common use case is
'timestamp default now()'
This might be a significant loss in some data-loading scenarios.
* ALTER TABLE t ALTER col TYPE x USING some_expr(); No big loss here.

Regards,
Marti

On Wed, Dec 7, 2011 at 00:29, Marti Raudsepp <marti@juffo.org> wrote:

ExecInitExpr enables the cache when its 'PlanState *parent' attribute
isn't NULL

[...]

On the other hand, a few places lose caching support this way since
they don't go through the planner:
* Column defaults in a COPY FROM operation. Common use case is
'timestamp default now()'
This might be a significant loss in some data-loading scenarios.
* ALTER TABLE t ALTER col TYPE x USING some_expr(); No big loss here.

Let me rephrase that as a question: Does it seem worthwhile to add a
new argument to ExecInitExpr to handle those two cases? Does relying
on the PlanState argument being NULL seem like a bad idea for any
reason?

PS: I forgot to mention that 2 test cases covering the two above query
types are deliberately left failing in the v4-wip patch.

Regards,
Marti

Marti Raudsepp <marti@juffo.org> writes:

Let me rephrase that as a question: Does it seem worthwhile to add a
new argument to ExecInitExpr to handle those two cases?

Possibly. Another way would be to keep its API as-is and introduce a
different function name for the other behavior. I would think that
we'd always know for any given caller which behavior we need, so a
flag as such isn't notationally helpful.

Does relying
on the PlanState argument being NULL seem like a bad idea for any
reason?

Yes, that seemed like a pretty horrid idea when I read your description,
but I hadn't got round to looking at just how awful it might be.

regards, tom lane

On 12/07/2011 04:58 PM, Marti Raudsepp wrote:

PS: I forgot to mention that 2 test cases covering the two above query
types are deliberately left failing in the v4-wip patch.

It's not completely clear what happens next with this. Are you hoping
code churn here has calmed down enough for Jaime or someone else to try
and look at this more already? Or should we wait for a new update based
on the feedback that Heikki and Tom have already provided first, perhaps
one that proposes fixes for these two test cases?

One general suggestion about the fight with upstream changes you've run
into here. Now that you're settling into the git workflow, you might
consider publishing updates to a site like Github in the future too.
That lets testing of the code at the point you wrote it always
possible. Given just the patch, reviewers normally must reconcile any
bit rot before they can even compile your code to try it. That gets
increasingly sketchy the longer your patch waits before the next
CommitFest considers it. With a published git working tree, reviewers
can pull that for some hands-on testing whenever, even if a merge
wouldn't actually work out at that point. You just need to be careful
not to push an update that isn't self-consistent to the world. I
normally attach the best formatted patch I can and publish to Github.
Then reviewers can use whichever they find easier, and always have the
option of postponing a look at merge issues if they just want to play
with the program.

--
Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us

On Sat, Dec 10, 2011 at 16:50, Greg Smith <greg@2ndquadrant.com> wrote:

Or should we wait for a new update based on the
feedback that Heikki and Tom have already provided first, perhaps one that
proposes fixes for these two test cases?

Yes, I will post and updated and rebased version tomorrow.

Now that you're settling into the git workflow, you might consider
publishing updates to a site like Github in the future too.

It's in the 'cache' branch on my Github:
https://github.com/intgr/postgres/commits/cache
(I linked to it in the v3 posting, but forgot to mention it later)

Regards,
Marti

On Sat, Dec 10, 2011 at 21:05, Marti Raudsepp <marti@juffo.org> wrote:

Yes, I will post and updated and rebased version tomorrow.

(Sorry, I'm 1 day late :)

Here's v5 of the patch. Changes from v4:
* Rebased over Thomas Munro's constifying patch
* ExecInitExpr is now split in two:
* ExecInitExprMutator, which now takes a context struct argument, to
prevent having to pass around 2 arguments through recursion every
time.
* ExecInitExpr, which is the public function, now has a new boolean
argument to enable/disable cache. All callers have been converted.
* A new test case for a VALUES() expression.

I don't know of anything lacking in the current patch so I'm not
planning to make changes until further feedback. I'm also not very
confident about the refactoring I made with ExecInitExpr, so
bikeshedding is welcome. :)

Patch attached. Also available in the 'cache' branch on my Github:
https://github.com/intgr/postgres/commits/cache

Regards,
Marti

On Sun, Dec 4, 2011 at 22:53, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

I wonder if it would be better to add the CacheExpr nodes to the tree as a
separate pass, instead of shoehorning it into eval_const_expressions? I
think would be more readable that way, even though a separate pass would be
more expensive. And there are callers of eval_const_expressions() that have
no use for the caching, like process_implied_equality().

I had an idea how to implement this approach with minimal performance
impact. It might well be a dumb idea, but I wanted to get it out
there.

The 'struct Expr' type could have another attribute that stores
whether its sub-expressions contain stable/volatile functions, and
whether it only contains of constant arguments. This attribute would
be filled in for every Expr by eval_const_expressions.

Based on this information, ExecInitExpr (which is pretty simple for
now) could decide where to inject CacheExprState nodes. It's easier to
make that decision there, since by that stage the cachability of the
parent node with each argument is already known. (Currently I have to
stick all cachable arguments in a temporary list until I've determined
whether all arguments are cachable, or just some of them are)

This would decouple expression caching from the planner entirely;
CacheExpr wouldn't exist anymore. AFAICT this would also let us get
rid of contain_mutable_functions_walker() and possibly others.

Regards,
Marti

Marti Raudsepp <marti@juffo.org> writes:

The 'struct Expr' type could have another attribute that stores
whether its sub-expressions contain stable/volatile functions, and
whether it only contains of constant arguments. This attribute would
be filled in for every Expr by eval_const_expressions.

Hmm. I'm a bit hesitant to burn the extra space that would be required
for that. On the other hand it is a relatively clean solution to
postponing the whether-to-actually-cache decisions until runtime.
But on the third hand, I don't know whether this is really much cleaner
than always injecting CacheExpr and having some runtime flag that
prevents it from caching. The argument that CacheExpr could confuse
equal() checks applies just as much to this, unless you make some ad-hoc
decision that equal() should ignore these flag bits.

This would decouple expression caching from the planner entirely;
CacheExpr wouldn't exist anymore. AFAICT this would also let us get
rid of contain_mutable_functions_walker() and possibly others.

Yeah, you could imagine getting rid of tree walks for both mutability
and returns-set tests, and maybe other things too --- once you've paid
the price of an expression attributes flag word, there's room for quite
a few bits in there. Don't know offhand if that's attractive enough to
tip the scales.

regards, tom lane