Hi,
while looking at fixing [1], I again came across the fact that we don't
initialize the projection etc during ExecInitWorkTableScan(), but do so during
the first call to ExecWorkTableScan().
This is explained with the following comment:
/*
* On the first call, find the ancestor RecursiveUnion's state via the
* Param slot reserved for it. (We can't do this during node init because
* there are corner cases where we'll get the init call before the
* RecursiveUnion does.)
*/
I remember being confused about this before. I think I dug out the relevant
commit back then 0a7abcd4c983 [2], but didn't end up finding the relevant
thread. This time I did: [3].
Basically the issue is that in queries with two CTEs we can, at least
currently, end up with a WorkTable scans on a CTE we've not yet initialized,
due to the WorkTable scan of one CTE appearing in the other. Thus
ExecInitRecursiveUnion() hasn't yet set up the param we use in
nodeWorktablescan.c to find the tuplestore and the type of the tuples it
contains.
I don't think this is a huge issue, but it surprised multiple times, so I'd
like to expand the comment. At least for me it's hard to get from "corner
cases" to one worktable scan appearing in another CTE and to mutally recursive
CTEs.
I did go down the rabbit hole of trying to avoid this issue because it "feels
wrong" to not know the return type of an executor node during initialization.
The easiest approach I could see was to add the "scan type" to WorkTableScan
(vs the target list, which includes the projection). Most of the other scan
nodes get the scan type from the underlying relation etc, and thus don't need
it in the plan node ([4]). That way the projection can be built normally
during ExecInitWorkTableScan(), but we still need to defer the lookup of
node->rustate. But that bothers me a lot less.
I'm not sure it's worth changing this. Or whether that'd be the right approach.
I'm also wondering if Tom's first instinct from back then making this an error
would have been the right call. But that ship has sailed.
To be clear, this "issue" is largely independent of [1] / not a blocker
whatsoever. Partially I wrote this to have an email to find the next time I
encounter this.
Greetings,
Andres Freund
[1] https://postgr.es/m/17737-55a063e3e6c41b4f%40postgresql.org
[2] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=0a7abcd4c983
[3] https://postgr.es/m/87zllbxngg.fsf%40oxford.xeocode.com
[4] I don't particularly like that, we spend a lot of time converting memory
inefficient target lists into tupledescs during executor initialization,
even though we rely on the tuple types not to be materially different
anyway. But that's a separate issue.