>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
>> [ inlining-ctes-v5.patch ]
Tom> I took a little bit of a look through this. Some thoughts:
Tom> * I think it'd be a good idea if we made OFFSET/LIMIT in a CTE be
Tom> an alternate way of keeping it from being inlined. As the patch
Tom> stands, if that's the behavior you want, you have no way to
Tom> express it in a query that will also work in older servers. (I
Tom> will manfully resist suggesting that then we don't need the
Tom> nonstandard syntax at all ... oops, too late.)
I think this is the wrong approach, because you may want the
optimization-barrier effects of OFFSET/LIMIT _without_ the actual
materialization - there is no need to force a query like
with d as (select stuff from bigtable offset 1) select * from d;
to push all the data through an (on-disk) tuplestore.
Tom> * I have no faith in the idea that we can skip doing a copyObject
Tom> on the inlined subquery, except maybe in the case where we know
Tom> there's exactly one reference.
The code doesn't do a copyObject on the query if there are no level
changes because everywhere else just does another copyObject on it as
the first processing step (cf. set_subquery_pathlist and the various
functions called from pull_up_subqueries).
Tom> * In "here's where we do the actual substitution", if we're going
Tom> to scribble on the RTE rather than make a new one, we must take
Tom> pains to zero out the RTE_CTE-specific fields so that the RTE
Tom> looks the same as if it had been a RTE_SUBQUERY all along; cf
Tom> db1071d4e.
Yes, this needs fixing. (This code predates that commit.)
Tom> * Speaking of the comments, I'm not convinced that view rewrite is
Tom> a good comparison point; I think this is more like subquery
Tom> pullup.
It's not really like subquery pullup because we actually do go on to do
subquery pullup _after_ inlining CTEs.
Tom> * I wonder whether we should make range_table_walker more friendly
Tom> to the needs of this patch. The fact that it doesn't work for this
Tom> usage suggests that it might not work for others, too. I could see
Tom> replacing the QTW_EXAMINE_RTES flag with QTW_EXAMINE_RTES_BEFORE
Tom> and QTW_EXAMINE_RTES_AFTER so that callers could say which order
Tom> of operations they want (ie, visit RTE itself before or after its
Tom> substructure). Then we could get rid of the double traversal of
Tom> the RTE list.
Sure, why not.
--
Andrew.