Here is a new version of the patch with added tests, improved comments,
some minor code cleanup and most importantly slightly changed logic for
when we should inline.
The current strategy is to always inline unless the CTE is recursive or
it has side effects, i.e. it is a DML query, it contains calls it a
volatile function, or it contains FOR SHARE/FOR UPDATE. I feel this is a
conservative approach which prevents behavioral changes (other than
performance).
Current open issues:
1. Currently the patch will inline CTEs even when there are multiple
references to them. If this is a win or not depends on the specific
query so I am not sure what we should do here. One thing worth noting is
that our current documentation talks a lot about how CTEs are only
evaluated once.
"A useful property of <literal>WITH</literal> queries is that they are
evaluated
only once per execution of the parent query, even if they are referred to
more than once by the parent query or sibling <literal>WITH</literal>
queries.
Thus, expensive calculations that are needed in multiple places can be
placed within a <literal>WITH</literal> query to avoid redundant work.
Another
possible application is to prevent unwanted multiple evaluations of
functions with side-effects."
What do you think?
2. Feedback on the new syntax. I am personally fine with the current
syntax, but it was just something I just quickly hacked together to move
the patch forward and which also solved my personal uses cases.
3. Are we happy with how I modified query_tree_walker()? I feel the code
would be clearer if we could change the tree walker to treat the RTE as
the parent node of the subquery instead of a sibling, but this seems
like potentially a quite invasive change.
4. I need to update the user documentation.
Andreas