Thread

[WIP] Caching for stable expressions with constant arguments v2

Marti Raudsepp <marti@juffo.org> — 2011-09-17T01:08:58Z

Hi!

Here's my work on this so far. The patch is still incomplete, but
works well and passes all regression tests (except plpgsql, which is
not a bug, but will be addressed in the future).

As Tom Lane suggested, I rewrote the whole thing and added a new
CacheExpr type. A "bool *cachable" argument is added to many
expression tree mutation functions to return information whether the
subtree is safe to cache. Hopefully this is more approachable than
"stableconst" :) The caller initializes this to true and callees set
it to false when proven otherwise.

If you build it with -DDEBUG_CACHEEXPR=1 then EXPLAIN VERBOSE will
show CACHE[...] around cached (sub)expressions.
----

In boolean expressions (lists of OR/AND expressions), the cahcable and
un-cachable elements are accumulated into separate lists. A new
boolean expression is then built for the cacheable part and prepended
-- so that it's checked first, in the hopes that the cachable check is
cheaper and short-circuits the rest. Not sure if there are significant
downsides to this.

For example:
Input: stable_expr() AND volatile_expr() AND stable_expr() AND volatile_expr()
Output: CACHE[(stable_expr() AND stable_expr())] AND volatile_expr()
AND volatile_expr()
----

To balance my feature karma, I also took the liberty to clean up some
duplicate code. Previously, all callers of simplify_function() did
their own mutation of argument lists prior to calling. Also, arguments
added later in reorder_function_arguments() and
add_function_defaults() were mutated separately in those functions.
Now all of that is done in one place, in simplify_function().
reorder_function_arguments/add_function_defaults functions don't need
a context argument anymore.
----

Work still to left do:
* Rename eval_const_expressions_mutator to const_expressions_mutator
since it doesn't just evaluate constant expressions anymore?
* Should CacheExpr.subexpr be renamed to "arg" for consistency with others?
* CaseExpr, CoalesceExpr, CoerceViaIO and some others aren't cachable yet
* Clean up some hairy code that I've marked with "XXX"
* Some comments are obsolete/incomplete, should be revised
* Remove the "enable_cacheexpr" GUC, it's useful for testing, but
probably not for real usage
* Currently a cachable expression is forbidden from being a "simple
expression". This probably causes the plpgsql regression. I think the
"real" solution is removing CacheExpr nodes afterwards if the
expression is indeed deemed simple; that gives us the best of both
worlds
---

PS: I still don't know how to get git to produce context diff, so
please excuse my unified diff.

This is also available on my GitHub "cache" branch:
https://github.com/intgr/postgres/commits/cache
But don't rely on that too much, I *will* rebase and alter history.

Regards,
Marti