Re: [PATCHES] [8.4] Updated WITH clause patch (non-recursive)
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 [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] [8.4] Updated WITH clause patch (non-recursive)
Hi, From: Neil Conway [EMAIL PROTECTED] 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 [EMAIL PROTECTED] Index: src/backend/nodes/copyfuncs.c === RCS file: /projects/cvsroot/pgsql/src/backend/nodes/copyfuncs.c,v retrieving revision 1.390 diff -c -r1.390 copyfuncs.c *** src/backend/nodes/copyfuncs.c 21 Mar 2008 22:41:48 - 1.390 --- src/backend/nodes/copyfuncs.c 25 Mar 2008 04:18:06 - *** *** 1939,1944 --- 1939,1945 COPY_NODE_FIELD(limitOffset); COPY_NODE_FIELD(limitCount); COPY_NODE_FIELD(lockingClause); + COPY_NODE_FIELD(with_cte_list); COPY_SCALAR_FIELD(op); COPY_SCALAR_FIELD(all); COPY_NODE_FIELD(larg); Index: src/backend/nodes/equalfuncs.c === RCS file: /projects/cvsroot/pgsql/src/backend/nodes/equalfuncs.c,v retrieving revision 1.320 diff -c -r1.320 equalfuncs.c *** src/backend/nodes/equalfuncs.c 21 Mar 2008 22:41:48 - 1.320 --- src/backend/nodes/equalfuncs.c 25 Mar 2008 04:18:07 - *** *** 821,826 --- 821,827 COMPARE_NODE_FIELD(limitOffset); COMPARE_NODE_FIELD(limitCount); COMPARE_NODE_FIELD(lockingClause); + COMPARE_NODE_FIELD(with_cte_list); COMPARE_SCALAR_FIELD(op); COMPARE_SCALAR_FIELD(all); COMPARE_NODE_FIELD(larg); Index: src/backend/nodes/outfuncs.c === RCS file: /projects/cvsroot/pgsql/src/backend/nodes/outfuncs.c,v retrieving revision 1.324 diff -c -r1.324 outfuncs.c *** src/backend/nodes/outfuncs.c21 Mar 2008 22:41:48 - 1.324 --- src/backend/nodes/outfuncs.c25 Mar 2008 04:18:08 - *** *** 1599,1604 --- 1599,1605 WRITE_NODE_FIELD(limitOffset); WRITE_NODE_FIELD(limitCount); WRITE_NODE_FIELD(lockingClause); + WRITE_NODE_FIELD(with_cte_list); WRITE_ENUM_FIELD(op, SetOperation); WRITE_BOOL_FIELD(all); WRITE_NODE_FIELD(larg); Index: src/backend/parser/analyze.c === RCS file: /projects/cvsroot/pgsql/src/backend/parser/analyze.c,v retrieving revision 1.371 diff -c -r1.371 analyze.c *** src/backend/parser/analyze.c1 Jan 2008 19:45:50 - 1.371 --- src/backend/parser/analyze.c25 Mar 2008 04:18:09 - *** *** 688,693 --- 688,696 /* make FOR UPDATE/FOR SHARE info available to addRangeTableEntry */ pstate-p_locking_clause = stmt-lockingClause; + /* process the WITH clause (pull CTEs into the pstate's ctenamespace) */ + transformWithClause(pstate, stmt-with_cte_list); + /* process the FROM clause */ transformFromClause(pstate, stmt-fromClause); *** *** 1021,1026 --- 1024,1032 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg(SELECT FOR UPDATE/SHARE is not allowed with UNION/INTERSECT/EXCEPT))); + /* process the WITH clause (pull CTEs into the pstate's ctenamespace) */ + transformWithClause(pstate, stmt-with_cte_list); + /* * Recursively transform the components of the tree. */ Index: src/backend/parser/gram.y === RCS file: /projects/cvsroot/pgsql/src/backend/parser/gram.y,v retrieving revision 2.610 diff -c -r2.610 gram.y *** src/backend/parser/gram.y 21 Mar 2008 22:41:48 - 2.610 --- src/backend/parser/gram.y 25 Mar 2008 04:18:16 - *** *** 103,109 static SelectStmt *findLeftmostSelect(SelectStmt *node); static void insertSelectOptions(SelectStmt *stmt, List *sortClause, List *lockingClause, ! Node *limitOffset, Node *limitCount); static Node *makeSetOp(SetOperation op, bool all, Node *larg, Node *rarg); static Node *doNegate(Node *n, int location); static void doNegateFloat(Value *v); --- 103,110 static SelectStmt *findLeftmostSelect(SelectStmt *node); static
Re: [PATCHES] [8.4] Updated WITH clause patch (non-recursive)
Neil Conway [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] [8.4] Updated WITH clause patch (non-recursive)
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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [8.4] Updated WITH clause patch (non-recursive)
Neil Conway [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [8.4] Updated WITH clause patch (non-recursive)
On Sun, 2008-01-27 at 09:17 +, 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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [8.4] Updated WITH clause patch (non-recursive)
Hello On 27/01/2008, Neil Conway [EMAIL PROTECTED] wrote: On Sun, 2008-01-27 at 09:17 +, 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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] [8.4] Updated WITH clause patch (non-recursive)
Gregory Stark [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] [8.4] Updated WITH clause patch (non-recursive)
Neil Conway [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] [8.4] Updated WITH clause patch (non-recursive)
On Jan 27, 2008 8:13 PM, Neil Conway [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] [8.4] Updated WITH clause patch (non-recursive)
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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
