Add support for RECURSIVE withs

[Raveline]

I'm going to need some recursive powered CTE, but hpqtypes-extra doesn't offer support for these neat things.
This PR propose adding support for them. It's a bit tricky, because the syntax demands that "RECURSIVE" be right after the "WITH" and cannot target a singular CTE.

[jonathanjouty]

I need to read up on how WITH RECURSIVE works, but in the meantime I have an excellent code review delay tactic: could you add a test that uses sqlWithRecursive?

(to be fair I do honestly think we should have a test)

[Raveline]

I must fix something first, but will gladly add tests.

[jonathanjouty]

BTW the code looks sensible, but I'm not too familiar with the inner workings of this library.
Who else can we get to take a look in @arybczak's absence 🤔

[Raveline]

There you go, I've added a simple test showcasing the recursive with ! I don't really know who else is familiar enough with the library, your name was on the suggested reviewers, that's why I bothered you in the first place !

[arybczak]

What happens if you do

sqlWithRecursive ...
sqlWith ...
...

?

Looks to me like the last sqlWith will overwrite the Recursive tag of the first sqlWithRecursive, right?

The simplest solution here would be to mark all WITH clauses recursive, right?

[arybczak]

Isn't this feature supported in pretty much all PostgreSQL versions? I checked and 9.6 looks to have it.

[Raveline]

They started in 8.4 according to Wikipedia, but I don't know what are the older versions supported by hpqtypes-extra. If we don't need to check for the feature, I'll be glad to remove these !

[arybczak]

We don't really care for anything older than 11 at this point ;)

[Raveline]

Removed then.

[Raveline]

@arybczak : Indeed, it would get overridden. I didn't think about it, shame on me.

As for marking ALL CTEs as recursive, apart from the obvious "adding code that is not always explicitly needed" issue (I guess we can live with that one), I just would like for @marco44 to confirm that there is no overhead / issue we didn't think about in putting "RECURSIVE" everywhere.

[marco44]

If you're building a list of CTE expressions, and one in the list is recursive, you need to add the recursive keyword to the whole CTE block, so if you have a bunch of sqlWith, sqlWithRecursive... the sqlWithRecursive "wins", you need to add the keyword. You're telling PostgreSQL that any of the subqueries may be recursive, so if it seens some UNION/UNION ALL constructs, it has to inspect them differently.

Making all CTEs recursive in the whole app (if I understood correctly the question) seems bad to me:

• Of course, there are a few things that will get more costly to parse and to plan, not by that much, but I don't think anyone benchmarked the impact of having a non-recursive query marked "recursive" (it would be quite complicated to benchmark, given the amount of cases that you'd need to test). The final plan should make no difference (so no execution penalty), as it's mostly allowing UNION/UNION ALL in the CTE subqueries after the WITH clause, and allowing those to iterate if they're referencing the initial term (the one before the UNION). So you're basically asking the planner to check for that all the time. And planning is still a big chunk of our total resource consumption (like 20-25% of the total CPU spent), so anything saved here is probably worth it.

• The second problem I see is that you're removing a protection that you get by not marking unnecessarily a query as recursive, as then the planner would notice you doing naugthy recursive things in a supposedly non recursive query and refuse it.

• And a last argument against… please think of your DBA. It's very useful for me to see a query marked as "with recursive" only if it is, because I know what it's doing without having to look into all blocks to guess if it's really doing some recursion or not.

[Raveline]

All right, lemme find a better way of handling this then.

[Raveline]

This version keeps RECURSIVE iff there is at least one withRecursiveWith around (this is tested), using a Semigroup instance that behaves more or less like Any.

[arybczak]

You can put a comment here that explains why it works like this 🙂

[Raveline]

Added !