Re: [PATCH] Allow multiple recursive self-references

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> writes:
> As I said earlier, I think semantically/mathematically, the changes
> proposed by this patch set are okay.

I took a quick look at this patch because I wondered how it would
affect the SEARCH/CYCLE bug discussed at [1]. Doesn't it break
rewriteSearchAndCycle() completely? That code assumes (without a
lot of checking) that a recursive query is a UNION [ALL] of exactly
two SELECTs.

Some other random thoughts while I'm looking at it (not a full review):

* The patch removes this comment in WorkTableScanNext:

- * Note: we are also assuming that this node is the only reader of the
- * worktable. Therefore, we don't need a private read pointer for the
- * tuplestore, nor do we need to tell tuplestore_gettupleslot to copy.

I see that it deals with the private-read-pointer question, but I do not
see any changes addressing the point about copying tuples fetched from the
tuplestore. Perhaps there's no issue, but if not, a comment explaining
why not would be appropriate.

* The long comment added to checkWellFormedRecursion will be completely
destroyed by pgindent, but that's the least of its problems: it does
not explain either why we need a tree rotation or why that doesn't
break SQL semantics. The shorter comment in front of it needs
rewritten, too. And I'm not really convinced that the new loop
is certain to terminate.

* The chunk added to checkWellFormedSelectStmt is undercommented,
because of which I'm not convinced that it's right at all. Since
that's really the meat of this patch, it needs more attention.
And the new variable names are still impossibly confusing.

regards, tom lane

[1] https://www.postgresql.org/message-id/flat/17320-70e37868182512ab%40postgresql.org