Thread: Scrollable cursors and Sort performance
I'm interested in the behaviour of ExecSort, which *for all queries* prepares the sort result for randomAccess, even when part of a plan that will *never* go backwards/rewind etc. A recent performance test shows this output from mid-way through a heap sort with trace_sort=on (the query itself is not relevant here) > LOG: 00000: finished writing final run 65 to tape 64: CPU 57.50s/484.51u sec elapsed 597.46 sec > LOG: 00000: finished merge step: CPU 107.90s/653.53u sec elapsed 941.83 sec Which shows that the *unnecessary* final merge takes 344 secs, adding approximately 60% to the elapsed time of the query and nearly doubling the CPU requirement. [Aside: you'll notice the above test was performed with my recent sort improvement patch applied, but the behaviour of ExecSort is identical in both cases. However in the current cvstip case, you simply don't notice the extra expense of the request for randomAccess because of the additional time taken by the sort] So, why does the planner think random access is required? Well, only for use in queries; CREATE INDEX for example does not force this. To allow support for CURSORs, of course. >From the code, we never call ExecSort with a direction other than Forward unless we are issuing a FETCH with a direction other than one identified internally as FETCH_FORWARD. According to the SQL standard, that can only happen when a scrollable cursor has been declared using DECLARE ... SCROLL. The current PostgreSQL manual says the following: FETCH: "The cursor should be declared with the SCROLL option if one intends to use any variants of FETCH other than FETCH NEXT or FETCH FORWARD with a positive count. For simple queries PostgreSQL will allow backwards fetch from cursors not declared with SCROLL, but this behavior is best not relied on. If the cursor is declared with NO SCROLL, no backward fetches are allowed." DECLARE: "The SCROLL option should be specified when defining a cursor that will be used to fetch backwards. This is required by the SQL standard. However, for compatibility with earlier versions, PostgreSQL will allow backward fetches without SCROLL, if the cursor's query plan is simple enough that no extra overhead is needed to support it. However, application developers are advised not to rely on using backward fetches from a cursor that has not been created with SCROLL. If NO SCROLL is specified, then backward fetches are disallowed in any case." The current behaviour is to plan every query as if it would allow backwards scans, then immediately disallow backwards scans *if* it fails the "no extra overhead" test later on in the Declare Cursor processing. [portalcmds.c:PerformCursorOpen()]. (i.e. you pay, but get no benefit) My suggestion is that the backwards-compatible behaviour of allowing backwards/absolute FETCHes *without* a specific SCROLL command be deprecated in the next release, so that the default is *disallow*. We've warned people and now its time to turn it off by default. (This would be re-enabled using default_cursor_scroll = on). If that is not acceptable, then we should re-evaluate the idea that sorts *always* allow backward scans [execAmi.c:ExecSupportsBackwardScan()], replacing this either with *never* or some kind of query cost test (but that seems much less preferable). Materialize already provides the infrastructure required to do this. This will then allow us to use the firm knowledge that a plan will only ever be scanned in a Forwards direction at plan time. ...and that will allow us to optimize out the rather large step taken during Sort to freeze the result unnecessarily for randomAccess. This will then give a good perfomance gain for larger joins and aggregations, neither of which would ever allow backwards scans using them current method anyway. I intend to add a short patch to pass down the cursor state during planning, so that when it is explicitly specified the sort node is able to recognise this and avoid work. Also, to add a GUC to force the not-explicitly-specified case to be the same as the NO SCROLL case, as the standard requires. Comments? Best Regards, Simon Riggs
Simon Riggs <simon@2ndquadrant.com> writes:
> Which shows that the *unnecessary* final merge takes 344 secs, adding
> approximately 60% to the elapsed time of the query and nearly doubling
> the CPU requirement.
The merge step would certainly have to happen anyway, so this claim is
not justified. You have not presented any evidence about how much of
the reported time is actually I/O related.
regards, tom lane
On Fri, 2006-02-10 at 10:13 -0500, Tom Lane wrote: > Simon Riggs <simon@2ndquadrant.com> writes: > > Which shows that the *unnecessary* final merge takes 344 secs, adding > > approximately 60% to the elapsed time of the query and nearly doubling > > the CPU requirement. > > The merge step would certainly have to happen anyway, so this claim is > not justified. You have not presented any evidence about how much of > the reported time is actually I/O related. You are right that the last read off disk and merge steps would still be required, if the full result set were to be read. However, I am pointing out a task in addition to that. Reading a large file from disk and then writing it back down *when there is no benefit* seems like a task we would want to avoid, whatever the size and however (sequential/random) the I/Os are spent. We need not debate the CPU time differences. The cited test sorted 1561238 data blocks, or 12.7 GB. Which at 120 MB/s would take 100 seconds one-way on a stand-alone system. I think a very large chunk of the elapsed time could reasonably be accounted for from I/O costs, since it would need to read then write all of that data. In the test, the post-sort retrieval of rows took 150 secs, indicating a sequential retrieval rate of 85 MB/sec, so there is no reason to believe that a slow disk over-emphasised performance loss for this case. Best Regards, Simon Riggs
On Fri, Feb 10, 2006 at 01:32:44PM +0000, Simon Riggs wrote: > I intend to add a short patch to pass down the cursor state during > planning, so that when it is explicitly specified the sort node is able > to recognise this and avoid work. Also, to add a GUC to force the > not-explicitly-specified case to be the same as the NO SCROLL case, as > the standard requires. So is this only an issue if you're using a cursor, or does this affect plain SELECT ... ORDER BY as well? Reason I'm asking is that users should be able to explicitly be able to turn the extra step off somehow. I'm not clear if NO SCROLL is sufficient to do that or not. -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
On Fri, 2006-02-10 at 10:22 -0600, Jim C. Nasby wrote: > On Fri, Feb 10, 2006 at 01:32:44PM +0000, Simon Riggs wrote: > > I intend to add a short patch to pass down the cursor state during > > planning, so that when it is explicitly specified the sort node is able > > to recognise this and avoid work. Also, to add a GUC to force the > > not-explicitly-specified case to be the same as the NO SCROLL case, as > > the standard requires. > > So is this only an issue if you're using a cursor, or does this affect > plain SELECT ... ORDER BY as well? > > Reason I'm asking is that users should be able to explicitly be able to > turn the extra step off somehow. I'm not clear if NO SCROLL is > sufficient to do that or not. It effects all sorts, whether or not they are even cursors. If a cursor is defined NO SCROLL, which is the SQL Standard implicit default, then we are safe to assume there will be no rewinds or backward scans. The PostgreSQL current implicit default is SCROLL, which means that no part of the executor can currently make useful assumptions about scan direction, so this is a wider issue than just sorts. Best Regards, Simon Riggs
Simon Riggs <simon@2ndquadrant.com> writes:
> On Fri, 2006-02-10 at 10:13 -0500, Tom Lane wrote:
>> The merge step would certainly have to happen anyway, so this claim is
>> not justified. You have not presented any evidence about how much of
>> the reported time is actually I/O related.
> You are right that the last read off disk and merge steps would still be
> required, if the full result set were to be read. However, I am pointing
> out a task in addition to that. Reading a large file from disk and then
> writing it back down *when there is no benefit* seems like a task we
> would want to avoid, whatever the size and however (sequential/random)
> the I/Os are spent. We need not debate the CPU time differences.
If the cost of avoiding it were zero, then sure. But propagating the
needed information down to the sort step is not a zero-effort thing,
and therefore I'd like to see an argument for it that's not got obvious
logical holes.
Your analysis of when randomAccess is required needs work, in any case,
since you forgot about ExecReScan. Not to mention mark/restore.
I also don't care for the proposal to replace Sort with Sort/Materialize
in cases where random access is needed: that will certainly be *slower*
than what we do now. When you are talking about penalizing some cases
to make other ones faster, you definitely need an argument without holes
in it.
I suspect that the right thing is not to see this as a planner issue at
all, but to try to drive the choice off the context in which the plan
gets invoked. Possibly we could pass a "need random access" boolean
down through the ExecInitNode calls (I seem to recall some prior
discussion of doing something like that, in the context of telling
Materialize that it could be a no-op in some cases).
Lastly, there isn't any obvious reason that I can see for having to
change the default assumption about cursors. Most queries don't go
through cursors. For those that do, we already document that specifying
NO SCROLL can be a performance win, so any app that's not saying that
when it could has only itself to blame.
regards, tom lane
On Fri, 2006-02-10 at 11:58 -0500, Tom Lane wrote: > Simon Riggs <simon@2ndquadrant.com> writes: > > On Fri, 2006-02-10 at 10:13 -0500, Tom Lane wrote: > >> The merge step would certainly have to happen anyway, so this claim is > >> not justified. You have not presented any evidence about how much of > >> the reported time is actually I/O related. > > > You are right that the last read off disk and merge steps would still be > > required, if the full result set were to be read. However, I am pointing > > out a task in addition to that. Reading a large file from disk and then > > writing it back down *when there is no benefit* seems like a task we > > would want to avoid, whatever the size and however (sequential/random) > > the I/Os are spent. We need not debate the CPU time differences. > > If the cost of avoiding it were zero, then sure. But propagating the > needed information down to the sort step is not a zero-effort thing, > and therefore I'd like to see an argument for it that's not got obvious > logical holes. > > I also don't care for the proposal to replace Sort with Sort/Materialize > in cases where random access is needed: that will certainly be *slower* > than what we do now. When you are talking about penalizing some cases > to make other ones faster, you definitely need an argument without holes > in it. That wasn't the initial proposal... > Your analysis of when randomAccess is required needs work, in any case, > since you forgot about ExecReScan. Not to mention mark/restore. OK - other thoughts there for later also. > I suspect that the right thing is not to see this as a planner issue at > all, but to try to drive the choice off the context in which the plan > gets invoked. Possibly we could pass a "need random access" boolean > down through the ExecInitNode calls (I seem to recall some prior > discussion of doing something like that, in the context of telling > Materialize that it could be a no-op in some cases). Yeh, that was me just being a little vague on implementation, but handing off from planner to executor via the Plan node is what I was hacking at now. I'll follow your recommendation and do it for the general case. Propagating it down should allow a few similar optimizations. Any others please shout 'em in everybody. > Lastly, there isn't any obvious reason that I can see for having to > change the default assumption about cursors. Most queries don't go > through cursors. For those that do, we already document that specifying > NO SCROLL can be a performance win, so any app that's not saying that > when it could has only itself to blame. The obvious reason is why force people to go out of their way for a performance win? This is the same as OIDs, AFAICS. Some people used them in their programs - well fine, they can keep 'em. Most people didn't and don't and will appreciate having their programs speed up. Not everybody gets the chance to change the SQL in an application program, however much they might want to and know they should. Third party software is most software. The only way to please both is to have a GUC, whatever it is set to by default. Best Regards, Simon Riggs
On Fri, Feb 10, 2006 at 04:48:42PM +0000, Simon Riggs wrote: > If a cursor is defined NO SCROLL, which is the SQL Standard implicit > default, then we are safe to assume there will be no rewinds or backward > scans. The PostgreSQL current implicit default is SCROLL, which means > that no part of the executor can currently make useful assumptions about > scan direction, so this is a wider issue than just sorts. Umm, the documentation says: PostgreSQL will allow backward fetches without SCROLL, if the cursor's query plan is simple enough that no extra overhead is needed to support it. So if the default is SCROLL someone needs to fix the docs because that's not what it says. It says that *some plans* can be fetched backwards even if you don't say scroll. The documentation clearly says we don't need to support backwards searches without scroll if it causes problems. Have a nice day, -- Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/ > Patent. n. Genius is 5% inspiration and 95% perspiration. A patent is a > tool for doing 5% of the work and then sitting around waiting for someone > else to do the other 95% so you can sue them.
Simon Riggs <simon@2ndquadrant.com> writes:
> Not everybody gets the chance to change the SQL in an application
> program, however much they might want to and know they should. Third
> party software is most software.
Right. You are proposing to *break* some applications in order to
make other ones faster. How popular do you think you will be?
regards, tom lane
On Fri, 2006-02-10 at 19:14 +0100, Martijn van Oosterhout wrote: > On Fri, Feb 10, 2006 at 04:48:42PM +0000, Simon Riggs wrote: > > If a cursor is defined NO SCROLL, which is the SQL Standard implicit > > default, then we are safe to assume there will be no rewinds or backward > > scans. The PostgreSQL current implicit default is SCROLL, which means > > that no part of the executor can currently make useful assumptions about > > scan direction, so this is a wider issue than just sorts. > > Umm, the documentation says: PostgreSQL will allow backward fetches > without SCROLL, if the cursor's query plan is simple enough that no > extra overhead is needed to support it. > > So if the default is SCROLL someone needs to fix the docs because > that's not what it says. It says that *some plans* can be fetched > backwards even if you don't say scroll. The documentation clearly says > we don't need to support backwards searches without scroll if it > causes problems. Changing the docs is not the problem here. I don't understand the point you are making and how it effects the issue. The problem is knowing before the sort is executed whether the sort result will ever be used in the future by a backward scan. We can only do this by definition, restricting the future use of a FETCH. Best Regards, Simon Riggs
On Fri, 2006-02-10 at 14:04 -0500, Tom Lane wrote: > Simon Riggs <simon@2ndquadrant.com> writes: > > Not everybody gets the chance to change the SQL in an application > > program, however much they might want to and know they should. Third > > party software is most software. > > Right. You are proposing to *break* some applications in order to > make other ones faster. How popular do you think you will be? How does this differ from OIDs? I'm suggesting that people have a choice. Best Regards, Simon Riggs
On Fri, Feb 10, 2006 at 07:16:41PM +0000, Simon Riggs wrote: > On Fri, 2006-02-10 at 19:14 +0100, Martijn van Oosterhout wrote: > > On Fri, Feb 10, 2006 at 04:48:42PM +0000, Simon Riggs wrote: > > > If a cursor is defined NO SCROLL, which is the SQL Standard implicit > > > default, then we are safe to assume there will be no rewinds or backward > > > scans. The PostgreSQL current implicit default is SCROLL, which means > > > that no part of the executor can currently make useful assumptions about > > > scan direction, so this is a wider issue than just sorts. > > > > Umm, the documentation says: PostgreSQL will allow backward fetches > > without SCROLL, if the cursor's query plan is simple enough that no > > extra overhead is needed to support it. > > > > So if the default is SCROLL someone needs to fix the docs because > > that's not what it says. It says that *some plans* can be fetched > > backwards even if you don't say scroll. The documentation clearly says > > we don't need to support backwards searches without scroll if it > > causes problems. > > Changing the docs is not the problem here. I don't understand the point > you are making and how it effects the issue. > > The problem is knowing before the sort is executed whether the sort > result will ever be used in the future by a backward scan. We can only > do this by definition, restricting the future use of a FETCH. I think the point that Martijn was trying to make was that per our docs it would be perfectly acceptable for us to make any cursor NO SCROLL implicitly if it means less work for the optimizer. Whether that's a good thing is a different story... ISTM doing so would be very confusing for users. Since this affects all queries with sorts I would definately be in favor of exposing an option to change this ASAP, even if the default was to maintain the current behavior for compatability. Disk sorts are hugely expensive, and anything to reduce that expense would be very welcome. -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
On Sat, Feb 11, 2006 at 11:32:02AM -0600, Jim C. Nasby wrote: > I think the point that Martijn was trying to make was that per our docs > it would be perfectly acceptable for us to make any cursor NO SCROLL > implicitly if it means less work for the optimizer. Ok, I take that back. The actual quote[1] is: "Depending upon the complexity of the query's execution plan, specifying SCROLL may impose a performance penalty on the query's execution time." Clearly that says it can affect execution time, not that we're free to alter the default behavior at will. But speaking of documentation, it doesn't actually say what the default is. Care update that, or should I formally submit a patch? [1] http://www.postgresql.org/docs/8.1/interactive/sql-declare.html -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
Jim C. Nasby wrote: > On Sat, Feb 11, 2006 at 11:32:02AM -0600, Jim C. Nasby wrote: > > I think the point that Martijn was trying to make was that per our docs > > it would be perfectly acceptable for us to make any cursor NO SCROLL > > implicitly if it means less work for the optimizer. > > Ok, I take that back. The actual quote[1] is: > > "Depending upon the complexity of the query's execution plan, specifying > SCROLL may impose a performance penalty on the query's execution time." > > Clearly that says it can affect execution time, not that we're free to > alter the default behavior at will. > > But speaking of documentation, it doesn't actually say what the default > is. Care update that, or should I formally submit a patch? Patch please. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
On Sat, 2006-02-11 at 11:44 -0600, Jim C. Nasby wrote: > On Sat, Feb 11, 2006 at 11:32:02AM -0600, Jim C. Nasby wrote: > > I think the point that Martijn was trying to make was that per our docs > > it would be perfectly acceptable for us to make any cursor NO SCROLL > > implicitly if it means less work for the optimizer. > > Ok, I take that back. The actual quote[1] is: > > "Depending upon the complexity of the query's execution plan, specifying > SCROLL may impose a performance penalty on the query's execution time." > > Clearly that says it can affect execution time, not that we're free to > alter the default behavior at will. > > But speaking of documentation, it doesn't actually say what the default > is. Care update that, or should I formally submit a patch? > > [1] http://www.postgresql.org/docs/8.1/interactive/sql-declare.html Why do that ahead of me making the suggested changes? Best Regards, Simon Riggs
On Sat, Feb 11, 2006 at 07:47:32PM +0000, Simon Riggs wrote: > On Sat, 2006-02-11 at 11:44 -0600, Jim C. Nasby wrote: > > On Sat, Feb 11, 2006 at 11:32:02AM -0600, Jim C. Nasby wrote: > > > I think the point that Martijn was trying to make was that per our docs > > > it would be perfectly acceptable for us to make any cursor NO SCROLL > > > implicitly if it means less work for the optimizer. > > > > Ok, I take that back. The actual quote[1] is: > > > > "Depending upon the complexity of the query's execution plan, specifying > > SCROLL may impose a performance penalty on the query's execution time." > > > > Clearly that says it can affect execution time, not that we're free to > > alter the default behavior at will. > > > > But speaking of documentation, it doesn't actually say what the default > > is. Care update that, or should I formally submit a patch? > > > > [1] http://www.postgresql.org/docs/8.1/interactive/sql-declare.html > > Why do that ahead of me making the suggested changes? Because right now it doesn't say what the actual default is. -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
> On Sat, 2006-02-11 at 11:44 -0600, Jim C. Nasby wrote: >> But speaking of documentation, it doesn't actually say what the default >> is. Care update that, or should I formally submit a patch? >> >> [1] http://www.postgresql.org/docs/8.1/interactive/sql-declare.html Actually, if you submit a patch that says either "SCROLL is the default" or "NO SCROLL is the default", it will be rejected as incorrect. The reason is that the default behavior is different from either of these, as is explained in the NOTES section. regards, tom lane
On Sat, Feb 11, 2006 at 07:50:22PM -0500, Tom Lane wrote: > > On Sat, 2006-02-11 at 11:44 -0600, Jim C. Nasby wrote: > >> But speaking of documentation, it doesn't actually say what the default > >> is. Care update that, or should I formally submit a patch? > >> > >> [1] http://www.postgresql.org/docs/8.1/interactive/sql-declare.html > > Actually, if you submit a patch that says either "SCROLL is the default" > or "NO SCROLL is the default", it will be rejected as incorrect. The > reason is that the default behavior is different from either of these, > as is explained in the NOTES section. Ok, so *that's* where the bit about the query plan being simple enough. Based on that, ISTM that it should be premissable for us to decide that a cursor requiring a sort isn't "simple enough" to support SCROLL. In any case, here's a patch that makes the non-standard behavior easier for people to find. -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
Attachment
On Fri, 2006-02-10 at 17:46 +0000, Simon Riggs wrote: > > Lastly, there isn't any obvious reason that I can see for having to > > change the default assumption about cursors. Most queries don't go > > through cursors. For those that do, we already document that specifying > > NO SCROLL can be a performance win, so any app that's not saying that > > when it could has only itself to blame. > > The obvious reason is why force people to go out of their way for a > performance win? This is the same as OIDs, AFAICS. Some people used them > in their programs - well fine, they can keep 'em. Most people didn't and > don't and will appreciate having their programs speed up. > > Not everybody gets the chance to change the SQL in an application > program, however much they might want to and know they should. Third > party software is most software. > > The only way to please both is to have a GUC, whatever it is set to by > default. Further consideration: Since not all queries are cursors, as you say, the issue is less important, once we patch the executor as discussed. Maybe we *can* dispense with the GUC? Currently, ExecSupportsBackwardScan() makes the decision based upon the node types present in the plan. For an in-memory sort, allowing backward scans is effectively zero cost. It is only when the sort goes to disk that it incurs extra overhead that you might wish to avoid. It would be possible to update the state of the cursor AFTER the sort has been performed. The cursor's query is not executed until the first fetch, at which point all meaningful cursor requests can be accomplished with a forward-only cursor. So it would be possible to make the cursor NO SCROLL at that point. My thinking is that non-deterministic behaviour like that is probably not helpful and could still result in some existing programs breaking. So, although having a GUC is not the only way, it does seem like the right thing to do....but secondary to the main executor change. Best Regards, Simon Riggs
Simon Riggs <simon@2ndquadrant.com> writes:
> On Fri, 2006-02-10 at 11:58 -0500, Tom Lane wrote:
>> I suspect that the right thing is not to see this as a planner issue at
>> all, but to try to drive the choice off the context in which the plan
>> gets invoked. Possibly we could pass a "need random access" boolean
>> down through the ExecInitNode calls (I seem to recall some prior
>> discussion of doing something like that, in the context of telling
>> Materialize that it could be a no-op in some cases).
> Yeh, that was me just being a little vague on implementation, but
> handing off from planner to executor via the Plan node is what I was
> hacking at now. I'll follow your recommendation and do it for the
> general case. Propagating it down should allow a few similar
> optimizations.
Have you done anything with this idea yet? I was just thinking of
attacking it myself. After looking at my old notes about Materialize,
I am thinking that we should add a "int flags" parameter to the InitNode
calls along with ExecutorStart and probably PortalStart. This would
contain a bitwise OR of at least the following flag bits:
need-ReScanneed-backwards-scanneed-mark-restoreno-execute (so flags can replace ExecutorStart's explainOnly param)
We'd have lots of room for expansion, but these are the ones that seem
to have immediate use. And most callers of ExecutorStart/PortalStart
know they don't need these things, so could just pass zero always.
Interesting point: how should EXPLAIN ANALYZE set these bits? For its
own purposes it need not request random access, but it might be
interesting to make it possible to examine both the random and nonrandom
behaviors, now that these will be significantly different performancewise.
Possibly we could make EXPLAIN ANALYZE EXECUTE set the random-access
bits.
regards, tom lane
On Sun, 2006-02-26 at 19:26 -0500, Tom Lane wrote: > Simon Riggs <simon@2ndquadrant.com> writes: > > On Fri, 2006-02-10 at 11:58 -0500, Tom Lane wrote: > >> I suspect that the right thing is not to see this as a planner issue at > >> all, but to try to drive the choice off the context in which the plan > >> gets invoked. Possibly we could pass a "need random access" boolean > >> down through the ExecInitNode calls (I seem to recall some prior > >> discussion of doing something like that, in the context of telling > >> Materialize that it could be a no-op in some cases). > > > Yeh, that was me just being a little vague on implementation, but > > handing off from planner to executor via the Plan node is what I was > > hacking at now. I'll follow your recommendation and do it for the > > general case. Propagating it down should allow a few similar > > optimizations. > > Have you done anything with this idea yet? I was just thinking of > attacking it myself. Started, but have been side-tracked by other urgent matters. Happy to complete this today with you? This mini-project has made me realise I don't understand the executor as well as I should, so I'm keen to see it through, even if I'm a bit slower at it. > After looking at my old notes about Materialize, > I am thinking that we should add a "int flags" parameter to the InitNode > calls along with ExecutorStart and probably PortalStart. This would > contain a bitwise OR of at least the following flag bits: > > need-ReScan > need-backwards-scan > need-mark-restore > no-execute (so flags can replace ExecutorStart's explainOnly param) > > We'd have lots of room for expansion, but these are the ones that seem > to have immediate use. And most callers of ExecutorStart/PortalStart > know they don't need these things, so could just pass zero always. Design-wise I was looking at putting a named struc in there, so it would be easily expandible in the future to carry anything else that needs to be passed down through the nodes like this. I guess thats the same line-of-thought you're on too. > Interesting point: how should EXPLAIN ANALYZE set these bits? For its > own purposes it need not request random access, but it might be > interesting to make it possible to examine both the random and nonrandom > behaviors, now that these will be significantly different performancewise. > Possibly we could make EXPLAIN ANALYZE EXECUTE set the random-access > bits. Good point. Whichever we do will be wrong in some cases.... I've no real opinion on this other than a vague preference for it to be quick. Best Regards, Simon Riggs
Simon Riggs <simon@2ndquadrant.com> writes:
> On Sun, 2006-02-26 at 19:26 -0500, Tom Lane wrote:
>> After looking at my old notes about Materialize,
>> I am thinking that we should add a "int flags" parameter to the InitNode
>> calls along with ExecutorStart and probably PortalStart.
> Design-wise I was looking at putting a named struc in there, so it would
> be easily expandible in the future to carry anything else that needs to
> be passed down through the nodes like this.
That would be the hard way, primarily because it would require copying
and modifying the struct at each level of recursion --- which'd turn
what should be a nearly zero-cost patch into something with possibly
nontrivial cost.
Copy and modify is needed because as one descends through the plan tree
the requirements change. For instance, MergeJoin requires mark/restore
capability of its right input, but this will never be a requirement
propagated from the top (or anyplace else). Materialize on the other
hand should turn off some of the bits, since it won't pass backwards
scan or mark/restore calls down to its child. These are trivial changes
to implement with a flag-word representation, not so with a struct.
If I saw a need for non-boolean parameters in this structure then maybe
I'd agree, but there's no evidence of a need for them. What the child
plan nodes need to know is "will I get any mark/restore calls" and such
like, and those are certainly boolean conditions.
I'm envisioning coding like
ExecInitMergeJoin(MergeJoin *node, EState *estate, int flags)
.../* reject unsupported cases */Assert(!(flags & (EXEC_FLAG_BACKWARD | EXEC_FLAG_MARK)));
...innerPlanState(mergestate) = ExecInitNode(innerPlan(node), estate,
flags | EXEC_FLAG_MARK);
nodeSort.c would have a test like
node->random = (flags & (EXEC_FLAG_BACKWARD | EXEC_FLAG_MARK)) != 0;
etc etc.
regards, tom lane
On Mon, 2006-02-27 at 10:07 -0500, Tom Lane wrote: > Simon Riggs <simon@2ndquadrant.com> writes: > > On Sun, 2006-02-26 at 19:26 -0500, Tom Lane wrote: > >> After looking at my old notes about Materialize, > >> I am thinking that we should add a "int flags" parameter to the InitNode > >> calls along with ExecutorStart and probably PortalStart. > > > Design-wise I was looking at putting a named struc in there, so it would > > be easily expandible in the future to carry anything else that needs to > > be passed down through the nodes like this. > > That would be the hard way, primarily because it would require copying > and modifying the struct at each level of recursion --- which'd turn > what should be a nearly zero-cost patch into something with possibly > nontrivial cost. Yeh, didn't take me long to see the costs; I just gave that idea up prior to reading your post. I won't go into *why* I was trying that, especially since I've stopped... Following your recipe pretty close as of now. Best Regards, Simon Riggs
On Mon, Feb 27, 2006 at 02:17:23PM +0000, Simon Riggs wrote: > > Interesting point: how should EXPLAIN ANALYZE set these bits? For its > > own purposes it need not request random access, but it might be > > interesting to make it possible to examine both the random and nonrandom > > behaviors, now that these will be significantly different performancewise. > > Possibly we could make EXPLAIN ANALYZE EXECUTE set the random-access > > bits. > > Good point. Whichever we do will be wrong in some cases.... I've no real > opinion on this other than a vague preference for it to be quick. Wouldn't an EXPLAIN ANALYZE DECLARE ... have the right information to know if backward scan, etc was needed? -- Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com Pervasive Software http://pervasive.com work: 512-231-6117 vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
"Jim C. Nasby" <jnasby@pervasive.com> writes:
> On Mon, Feb 27, 2006 at 02:17:23PM +0000, Simon Riggs wrote:
>>> Possibly we could make EXPLAIN ANALYZE EXECUTE set the random-access
>>> bits.
>>
>> Good point. Whichever we do will be wrong in some cases.... I've no real
>> opinion on this other than a vague preference for it to be quick.
> Wouldn't an EXPLAIN ANALYZE DECLARE ... have the right information to
> know if backward scan, etc was needed?
There is no EXPLAIN ANALYZE DECLARE, and AFAICS it would be a
contradiction in terms to have one, since DECLARE doesn't run the query.
Perhaps the correct addition would be EXPLAIN ANALYZE FETCH. (EXECUTE
is unrelated, now that I think harder about it.)
regards, tom lane
On Mon, Feb 27, 2006 at 06:01:21PM -0500, Tom Lane wrote:
> "Jim C. Nasby" <jnasby@pervasive.com> writes:
> > On Mon, Feb 27, 2006 at 02:17:23PM +0000, Simon Riggs wrote:
> >>> Possibly we could make EXPLAIN ANALYZE EXECUTE set the random-access
> >>> bits.
> >>
> >> Good point. Whichever we do will be wrong in some cases.... I've no real
> >> opinion on this other than a vague preference for it to be quick.
>
> > Wouldn't an EXPLAIN ANALYZE DECLARE ... have the right information to
> > know if backward scan, etc was needed?
>
> There is no EXPLAIN ANALYZE DECLARE, and AFAICS it would be a
> contradiction in terms to have one, since DECLARE doesn't run the query.
> Perhaps the correct addition would be EXPLAIN ANALYZE FETCH. (EXECUTE
> is unrelated, now that I think harder about it.)
You have no idea how glad I am that I'm not the only one who doesn't know about
'new' features (this first appeared in the docs in 7.4)... :)
decibel=# explain analyze declare test cursor for select * from pg_users; QUERY PLAN
----------------------------------------------------------Seq Scan on pg_authid (cost=0.00..1.01 rows=1 width=79)
Filter:rolcanlogin
(2 rows)
So, since presumably that accepts a full cursor declaration, would that suffice
for controlling EXPLAIN ANALYZE?
BTW, ISTM that it would also be useful to have EXPLAIN FETCH, since you
could have already defined a cursor. But I suspect a more common case
would be cut & paste of the declare from application code into psql,
which would make EXPLAIN DECLARE easier to use. Though, I never really
use cursors, so...
--
Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com
Pervasive Software http://pervasive.com work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461
"Jim C. Nasby" <jnasby@pervasive.com> writes:
> On Mon, Feb 27, 2006 at 06:01:21PM -0500, Tom Lane wrote:
>> There is no EXPLAIN ANALYZE DECLARE, and AFAICS it would be a
>> contradiction in terms to have one, since DECLARE doesn't run the query.
> You have no idea how glad I am that I'm not the only one who doesn't know about
> 'new' features (this first appeared in the docs in 7.4)... :)
> decibel=# explain analyze declare test cursor for select * from pg_users;
> QUERY PLAN
> ----------------------------------------------------------
> Seq Scan on pg_authid (cost=0.00..1.01 rows=1 width=79)
> Filter: rolcanlogin
> (2 rows)
Please notice that it didn't run the query (no actual-time data).
Perhaps it would be better if the code raised an error instead of
silently ignoring the ANALYZE keyword. I think this behavior
was chosen on the grounds that since DECLARE doesn't run the query,
it's consistent for EXPLAIN ANALYZE DECLARE to be a no-op as well.
But it's confusing now that I look at it again. In any case,
one should clearly need to say FETCH to get a cursor to execute.
regards, tom lane