[8.4] Updated WITH clause patch (non-recursive)

Attached is an updated version of Greg Stark's patch to add support for
the non-recursive variant of the SQL99 WITH clause[1]. I haven't looked
at the actual functionality of the patch yet (which is quite trivial) --
I just fixed up bitrot and the like. I also removed support for
RECURSIVE and the search/cycle clause, along with their associated
keywords -- the current patch doesn't approach anything close to adding
support for the non-recursive case, so it seems like a net loss to add
additional keywords for no gain in functionality.

Remaining work is to review the guts of the patch (which shouldn't take
long), and write documentation and regression tests. I'm personally
hoping to see this get into the tree fairly early in the 8.4 cycle,
pending discussion of course.

-Neil

[1] http://archives.postgresql.org/pgsql-patches/2007-03/msg00139.php
http://archives.postgresql.org/pgsql-patches/2007-04/msg00055.php

"Neil Conway" <neilc(at)samurai(dot)com> writes:

> Remaining work is to review the guts of the patch (which shouldn't take
> long), and write documentation and regression tests. I'm personally
> hoping to see this get into the tree fairly early in the 8.4 cycle,
> pending discussion of course.

Note that as it stands it directly inlines the subquery into the query
everywhere you use it. So if the user was hoping to save database work by
avoiding duplicate subqueries he or she may be disappointed. On the other hand
inlining it can allow the planner to produce better plans.

Tom's feeling at the time was that even though it was providing something from
the standard, it wasn't actually allowing the user to do anything he couldn't
before. If it doesn't provide any additional expressive capabilities then I
think he didn't like taking "with" as a reserved word.

I still hope to do recursive queries for 8.4 so I don't have strong feelings
for this part either way.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's On-Demand Production Tuning

On Sun, 2008-01-27 at 09:17 +0000, Gregory Stark wrote:
> Tom's feeling at the time was that even though it was providing something from
> the standard, it wasn't actually allowing the user to do anything he couldn't
> before.

I think this feature has value:

(1) This is SQL-standard syntax (and not even wacko syntax, at that!),
and there is merit in implementing it on those grounds alone.

(2) It is supported by DB2, MS SQL and Oracle, so there is a further
compatibility argument to be made.

(3) It avoids the need to repeat subqueries multiple times in the main
query, which can make queries more concise. Defining subqueries outside
the main SELECT body can also have readability advantages.

> If it doesn't provide any additional expressive capabilities then I
> think he didn't like taking "with" as a reserved word.

Note that we can make WITH a type_func_name_keyword, rather than a
full-on reserved_keyword, which reduces the force of this argument
slightly.

If we're going to implement recursive queries eventually (which we
almost surely will, whether in 8.4 or a future release), we'll need to
make WITH more reserved at some point anyway, so I don't see much to be
gained in the long run by delaying it.

-Neil

Hello

On 27/01/2008, Neil Conway <neilc(at)samurai(dot)com> wrote:
> On Sun, 2008-01-27 at 09:17 +0000, Gregory Stark wrote:
> > Tom's feeling at the time was that even though it was providing something from
> > the standard, it wasn't actually allowing the user to do anything he couldn't
> > before.
>
> I think this feature has value:
>

+1

I thing so is better commit smaller pieces more often than one time
one big patch. Nine months long feature freeze time is enough.

Regards
Pavel Stehule

> (1) This is SQL-standard syntax (and not even wacko syntax, at that!),
> and there is merit in implementing it on those grounds alone.
>
> (2) It is supported by DB2, MS SQL and Oracle, so there is a further
> compatibility argument to be made.
>
> (3) It avoids the need to repeat subqueries multiple times in the main
> query, which can make queries more concise. Defining subqueries outside
> the main SELECT body can also have readability advantages.
>
> > If it doesn't provide any additional expressive capabilities then I
> > think he didn't like taking "with" as a reserved word.
>
> Note that we can make WITH a type_func_name_keyword, rather than a
> full-on reserved_keyword, which reduces the force of this argument
> slightly.
>
> If we're going to implement recursive queries eventually (which we
> almost surely will, whether in 8.4 or a future release), we'll need to
> make WITH more reserved at some point anyway, so I don't see much to be
> gained in the long run by delaying it.
>
> -Neil
>
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Have you searched our list archives?
>
> http://archives.postgresql.org
>

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> I still hope to do recursive queries for 8.4 so I don't have strong feelings
> for this part either way.

One question that hasn't been asked is whether this patch is likely to
help, or to get in the way, for a more full-fledged implementation.
I don't recall at the moment if Greg has a credible design sketch for
the remaining work, but it might be a good idea to review that before
deciding.

regards, tom lane

Neil Conway <neilc(at)samurai(dot)com> writes:
> (1) This is SQL-standard syntax (and not even wacko syntax, at that!),
> and there is merit in implementing it on those grounds alone.
> (2) It is supported by DB2, MS SQL and Oracle, so there is a further
> compatibility argument to be made.

Both of the above arguments hold water only if we implement compatible
*semantics*, not merely syntax, so I find them unconvincing at this
stage.

> (3) It avoids the need to repeat subqueries multiple times in the main
> query, which can make queries more concise. Defining subqueries outside
> the main SELECT body can also have readability advantages.

Views fix that too.

>> If it doesn't provide any additional expressive capabilities then I
>> think he didn't like taking "with" as a reserved word.

> If we're going to implement recursive queries eventually (which we
> almost surely will, whether in 8.4 or a future release), we'll need to
> make WITH more reserved at some point anyway, so I don't see much to be
> gained in the long run by delaying it.

The point is that when you break people's apps you'll be able to point
to some real increment in functionality to justify it. With the patch
as it stands you'd essentially be saying "we're going to cause you pain
now for benefit later", which is a hard selling proposition.

I'm not opposed to applying this patch if it's an incremental step along
a clearly defined path to full WITH support in 8.4. I'm less eager to
put it in if there's not a plan and a commitment to make that happen.

regards, tom lane

On Sun, 2008-01-27 at 12:36 -0500, Tom Lane wrote:
> Both of the above arguments hold water only if we implement compatible
> *semantics*, not merely syntax, so I find them unconvincing at this
> stage.

How are the semantics of the proposed patch incompatible with the SQL
spec or the implementations in other systems? The proposed patch is a
*subset* of the functionality in the SQL spec, but it isn't incompatible
with it as far as I know (recursive and non-recursive WITH are distinct
features).

> > (3) It avoids the need to repeat subqueries multiple times in the main
> > query, which can make queries more concise. Defining subqueries outside
> > the main SELECT body can also have readability advantages.
>
> Views fix that too.

Sure, if you're willing to resort to DDL, and lose most of the
conciseness / readability gain.

> The point is that when you break people's apps you'll be able to point
> to some real increment in functionality to justify it.

If your application uses an identifier that is a reserved word in SQL-92
and in pretty much all major databases, I don't think you have much
cause for grievance when it becomes a reserved word in Postgres -- the
writing has been on the wall for a while. Do you have any reason to
think that "WITH" is a particularly common table or column name, by the
way?

Note also the keywords.c hack in 8.3 for the WITH keyword means that
pg_dump already treats WITH as a reserved word, so most dumps should
load without changes.

> With the patch as it stands you'd essentially be saying "we're going
> to cause you pain now for benefit later", which is a hard selling
> proposition.

Again, the readability + compatibility arguments are non-zero benefits,
IMHO. But your argument is essentially a public-relations one ("it will
look bad if..."), which I don't find very convincing. If we explain that
WITH is a reserved word per SQL spec and is part of the planned support
for recursive queries (whether in 8.4 or later), I can't see very many
users being annoyed.

(Compare that with the irritation we may well see from the removal of
implicit casts in 8.3, which will break *far* more applications, for a
benefit that many users will no doubt find rather hard to observe.)

-Neil

On Jan 27, 2008 8:13 PM, Neil Conway <neilc(at)samurai(dot)com> wrote:
> (Compare that with the irritation we may well see from the removal of
> implicit casts in 8.3, which will break *far* more applications, for a
> benefit that many users will no doubt find rather hard to observe.)

It's a bit off-topic but I was thinking the same *before* porting a
real application to 8.3. There are cases where it's just annoying to
not have the casts anymore but I find also a bunch of broken
behaviours (interval > int for example) which I'm quite happy to
detect and fix.
But I'm pretty sure we'll have a lot of feedback on this, probably
mostly negative at first.

--
Guillaume

Neil Conway wrote:
> On Sun, 2008-01-27 at 12:36 -0500, Tom Lane wrote:
>> Both of the above arguments hold water only if we implement
>> compatible *semantics*, not merely syntax, so I find them
>> unconvincing at this stage.
>
> How are the semantics of the proposed patch incompatible with the SQL
> spec or the implementations in other systems? The proposed patch is
> a *subset* of the functionality in the SQL spec, but it isn't
> incompatible with it as far as I know (recursive and non-recursive
> WITH are distinct features).

An implementation of WITH that inlines the subquery instead of executing
it only once (if appropriate) might not be incompatible with the SQL
spec, but it might very well turn out to be incompatible with other
major DBMSes from a practical point of view. If people use non-recursive
WITH as a replacement for constructs like
CREATE TEMPORARY TABLE temp AS SELECT ... ;
SELECT ... FROM temp, ... ;
, and not merely to increase readability, they won't gain anything from
an inlining WITH implementation.

This, BTW, is the reason that the C++ standard specifies the runtime
complexity (in big-O-notation) for things like vector/list/hash lookups,
instead of just specifying the interface.

regards, Florian Pflug

"Neil Conway" <neilc(at)samurai(dot)com> writes:

> Remaining work is to review the guts of the patch (which shouldn't take
> long), and write documentation and regression tests. I'm personally
> hoping to see this get into the tree fairly early in the 8.4 cycle,
> pending discussion of course.

Looking back at this I've realized (putting aside whether we want to apply the
patch as is which is another question) that to get the CTEs materialized so
they perform the way a user might expect them to would actually require the
same infrastructure that recursive queries will require.

Basically what I think we really want down the line is for something like:

WITH (select * from complex_view) AS x
SELECT *
FROM x
JOIN x as x2 ON (x.id=x2.id2)

to run the view once, materialize the results and then join the resulting data
with itself. At least that's what the user is likely expecting. Now it may be
that we have a better plan by inlining the two calls which in an ideal world
we would go ahead and try as well. But it's more likely that users would write
the WITH clause because they specifically want to avoid re-evaluating a
complex subquery.

To do this though we would need the same capability that recursive queries
would need. Namely the ability to have a single tuplestore with multiple
readers reading from different positions in the tuplestore.

So what I'm imagining doing is to add a flag to the RelOptInfo (Alternatively
we could create a new rtekind, RTE_CTE, but that would require most sites
which check for RTE_SUBQUERY to check for that as well).

Then (I think) in create_subqueryscan_plan we would have to check for this
flag and introduce the Memoize node I previously mentioned. That's basically a
Materialize node which keeps track of its position within the tuplestore in
its own state. It would also have to introuduce the one-time node with the
Materialize node which the Memoize would point to. I'm getting a bit vague
here as I haven't entirely absorbed how one-time plans work.

That would allow the query above to, for example, generate something like:

InitPlan
-> Memoize x (cost=0.00..34.00 rows=2400 width=4)
-> Seq scan on complex_view (cost=0.00..34.00 rows=2400 width=4)
Merge Join (cost=337.50..781.50 rows=28800 width=8)
Merge Cond: (x.id = x2.id)
-> Sort (cost=168.75..174.75 rows=2400 width=4)
Sort Key: x.id
-> MemoizeRead x (cost=0.00..34.00 rows=2400 width=4)
-> Sort (cost=168.75..174.75 rows=2400 width=4)
Sort Key: x2.id
-> MemoizeRead x x2 (cost=0.00..34.00 rows=2400 width=4)

Does this sound like the right track? Should I be doing this at the RelOptInfo
level or at some point higher up? Do I misunderstand anything about how
InitPlan is handled?

Other ideas: it might be interesting to note that we're sorting the same
Memoize node twice and push that down into the initial plan. Or somehow to
check whether it wouldn't be faster to just inline the memoized node directly
because perhaps there's a path available which would work for this read of it.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com
Ask me about EnterpriseDB's On-Demand Production Tuning

Hi,

From: Neil Conway <neilc(at)samurai(dot)com>
Subject: [PATCHES] [8.4] Updated WITH clause patch (non-recursive)
Date: Sat, 26 Jan 2008 23:58:40 -0800

> Attached is an updated version of Greg Stark's patch to add support for
> the non-recursive variant of the SQL99 WITH clause[1].

I found a bug with the following SQL.

postgres=# WITH x AS (SELECT 1), y AS (SELECT 2)
SELECT * FROM x UNION ALL SELECT * FROM y;
ERROR: relation "x" does not exist

Attached patch transforms WITH clause in transformSetOperationStmt().
It works correctly with the attached patch.

postgres=# WITH x AS (SELECT 1), y AS (SELECT 2)
SELECT * FROM x UNION ALL SELECT * FROM y;
?column?
----------
1
2
(2 rows)

Regards,
--
Yoshiyuki Asaba
y-asaba(at)sraoss(dot)co(dot)jp

There is little support for adding this patch without the recursive
part, so I added the URLs for this thread to the TODO list under
recursive queries.

---------------------------------------------------------------------------

Neil Conway wrote:
> Attached is an updated version of Greg Stark's patch to add support for
> the non-recursive variant of the SQL99 WITH clause[1]. I haven't looked
> at the actual functionality of the patch yet (which is quite trivial) --
> I just fixed up bitrot and the like. I also removed support for
> RECURSIVE and the search/cycle clause, along with their associated
> keywords -- the current patch doesn't approach anything close to adding
> support for the non-recursive case, so it seems like a net loss to add
> additional keywords for no gain in functionality.
>
> Remaining work is to review the guts of the patch (which shouldn't take
> long), and write documentation and regression tests. I'm personally
> hoping to see this get into the tree fairly early in the 8.4 cycle,
> pending discussion of course.
>
> -Neil
>
> [1] http://archives.postgresql.org/pgsql-patches/2007-03/msg00139.php
> http://archives.postgresql.org/pgsql-patches/2007-04/msg00055.php

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +