Обсуждение: Ticket 117: Explain Buffers
Hi, This patch handles two new parameters for EXPLAIN: COSTS and BUFFERS. I'm not sure that we need to add support for format. I would like to have your opinion on this? -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
Вложения
2010/1/23 Guillaume Lelarge <guillaume@lelarge.info>: > Hi, > > This patch handles two new parameters for EXPLAIN: COSTS and BUFFERS. > I'm not sure that we need to add support for format. I would like to > have your opinion on this? Not really. The whole idea of using EXPLAIN on the menu is to get the graphical output, so I don't see where the user would care aobut the format of the text behind it. That said, we might want to consider switching to using one of the machine readable formats for newer versions, and then just implement parsing of these options there. In the end, that should lead to less complicated code.. (mind you, I didn't even look at your code so I don't know how complex it is :D) While you're whacking around the explain code, one thing that has always nagged me, is that ANALYZE is an option for EXPLAIN. IMO we should have "Explain" and "Explain Analyze" as separate commands, because they actually do different things. And then options for things like verbose, costs, buffers etc. If we're going to change that ever, now is probably the best time :-) Thoughts? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Le 23/01/2010 15:14, Magnus Hagander a écrit : > 2010/1/23 Guillaume Lelarge <guillaume@lelarge.info>: >> Hi, >> >> This patch handles two new parameters for EXPLAIN: COSTS and BUFFERS. >> I'm not sure that we need to add support for format. I would like to >> have your opinion on this? > > Not really. The whole idea of using EXPLAIN on the menu is to get the > graphical output, so I don't see where the user would care aobut the > format of the text behind it. > That was also what I thought... but, if so, why do we propose VERBOSE option? in this case, we don't get a graphical EXPLAIN, but a textual EXPLAIN VERBOSE. > That said, we might want to consider switching to using one of the > machine readable formats for newer versions, and then just implement > parsing of these options there. In the end, that should lead to less > complicated code.. (mind you, I didn't even look at your code so I > don't know how complex it is :D) > We can't switch completely if we want to still have compatibility with the older releases. Anyways, this is something we'll have to do to ease the process. > While you're whacking around the explain code, one thing that has > always nagged me, is that ANALYZE is an option for EXPLAIN. IMO we > should have "Explain" and "Explain Analyze" as separate commands, > because they actually do different things. And then options for things > like verbose, costs, buffers etc. If we're going to change that ever, > now is probably the best time :-) > > Thoughts? > I don't quite get why we should do that. You can already have an EXPLAIN or an EXPLAIN ANALYZE. Or is it simply to really differentiate them? because one won't launch the query and the other one will? -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
2010/1/23 Guillaume Lelarge <guillaume@lelarge.info>: > Le 23/01/2010 15:14, Magnus Hagander a écrit : >> 2010/1/23 Guillaume Lelarge <guillaume@lelarge.info>: >>> Hi, >>> >>> This patch handles two new parameters for EXPLAIN: COSTS and BUFFERS. >>> I'm not sure that we need to add support for format. I would like to >>> have your opinion on this? >> >> Not really. The whole idea of using EXPLAIN on the menu is to get the >> graphical output, so I don't see where the user would care aobut the >> format of the text behind it. >> > > That was also what I thought... but, if so, why do we propose VERBOSE > option? in this case, we don't get a graphical EXPLAIN, but a textual > EXPLAIN VERBOSE. Shows how often I use it. I thought we put that stuff in the main explain output, not flick to text mode. Couldn't we put the verbose stuff in the text output and still show the graphical one? Hmm. That might at least be easier to do if we use a machine format explain? >> That said, we might want to consider switching to using one of the >> machine readable formats for newer versions, and then just implement >> parsing of these options there. In the end, that should lead to less >> complicated code.. (mind you, I didn't even look at your code so I >> don't know how complex it is :D) >> > > We can't switch completely if we want to still have compatibility with > the older releases. Anyways, this is something we'll have to do to ease > the process. Right. What I'd envision us doing is a simple if (version < 9.0) run_old_explain_code(); else run_new_explain_code(); and only the codepath for run_new_explain_code() - which would use a machine format output - would have the implementation of any of these new parameters. It would be slightly more work to get started since the new parser needs to be done (hopefully there are classes that'll do 95% of that for us already though), but not having to maintain manual parsing of these new options should be a win in the long run. And eventually, some 5 years from now or so, we can retire the old explain code because we no longer support pre-9.0 versions :D >> While you're whacking around the explain code, one thing that has >> always nagged me, is that ANALYZE is an option for EXPLAIN. IMO we >> should have "Explain" and "Explain Analyze" as separate commands, >> because they actually do different things. And then options for things >> like verbose, costs, buffers etc. If we're going to change that ever, >> now is probably the best time :-) >> >> Thoughts? >> > > I don't quite get why we should do that. You can already have an EXPLAIN > or an EXPLAIN ANALYZE. Or is it simply to really differentiate them? > because one won't launch the query and the other one will? Now we have: explain Explain Options -> [verbose, analyze, newstuff] I'd prefer Explain Explain Analyze Explain Options -> [verbose, newstuff] My usecase is that most of the time, I want to do explain. So I d ot. Then to do explain analyze today i have to do: Explain Options -> Analyze Explain Explain Options -> Analyze (so I don't accidentally run with analyze the next time again) I think most people will set what's under Explain Options to "this is how I like it". But that's just not relevant for analyze - it's not a preference, it's a different operation. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Magnus Hagander escreveu: > While you're whacking around the explain code, one thing that has > always nagged me, is that ANALYZE is an option for EXPLAIN. IMO we > should have "Explain" and "Explain Analyze" as separate commands, > because they actually do different things. And then options for things > like verbose, costs, buffers etc. If we're going to change that ever, > now is probably the best time :-) > +1. It always annoys me too. There is another improvement that we could change: EXPLAIN VERBOSE could generate graphical output too. I understand the reason why we don't generate it (that's because of the long text output) but in 8.4 or later that noisy output was wiped out. It' so trivial that it is attached. Another must-fix is to have new machine-readable formats in multiple lines on 'Data Output'. Sorry, don't have time to figure out now... -- Euler Taveira de Oliveira http://www.timbira.com/ Index: frm/frmQuery.cpp =================================================================== --- frm/frmQuery.cpp (revisão 8167) +++ frm/frmQuery.cpp (cópia de trabalho) @@ -2431,7 +2431,7 @@ // If this was an EXPLAIN query, process the results if (done && explain) { - if (!verbose) + if (!verbose || conn->BackendMinimumVersion(8, 4)) { int i; wxString str;
Le 23/01/2010 16:40, Magnus Hagander a écrit : > 2010/1/23 Guillaume Lelarge <guillaume@lelarge.info>: >> Le 23/01/2010 15:14, Magnus Hagander a écrit : >>> 2010/1/23 Guillaume Lelarge <guillaume@lelarge.info>: >>>> Hi, >>>> >>>> This patch handles two new parameters for EXPLAIN: COSTS and BUFFERS. >>>> I'm not sure that we need to add support for format. I would like to >>>> have your opinion on this? >>> >>> Not really. The whole idea of using EXPLAIN on the menu is to get the >>> graphical output, so I don't see where the user would care aobut the >>> format of the text behind it. >>> >> >> That was also what I thought... but, if so, why do we propose VERBOSE >> option? in this case, we don't get a graphical EXPLAIN, but a textual >> EXPLAIN VERBOSE. > > Shows how often I use it. I thought we put that stuff in the main > explain output, not flick to text mode. > > Couldn't we put the verbose stuff in the text output and still show > the graphical one? > Done with Euler's patch. > Hmm. That might at least be easier to do if we use a machine format explain? > >>> That said, we might want to consider switching to using one of the >>> machine readable formats for newer versions, and then just implement >>> parsing of these options there. In the end, that should lead to less >>> complicated code.. (mind you, I didn't even look at your code so I >>> don't know how complex it is :D) >>> >> >> We can't switch completely if we want to still have compatibility with >> the older releases. Anyways, this is something we'll have to do to ease >> the process. > > Right. What I'd envision us doing is a simple > if (version < 9.0) > run_old_explain_code(); > else > run_new_explain_code(); > > and only the codepath for run_new_explain_code() - which would use a > machine format output - would have the implementation of any of these > new parameters. > > It would be slightly more work to get started since the new parser > needs to be done (hopefully there are classes that'll do 95% of that > for us already though), but not having to maintain manual parsing of > these new options should be a win in the long run. > > And eventually, some 5 years from now or so, we can retire the old > explain code because we no longer support pre-9.0 versions :D > OK. I'll add a ticket so that we can take care of this in another patch. >>> While you're whacking around the explain code, one thing that has >>> always nagged me, is that ANALYZE is an option for EXPLAIN. IMO we >>> should have "Explain" and "Explain Analyze" as separate commands, >>> because they actually do different things. And then options for things >>> like verbose, costs, buffers etc. If we're going to change that ever, >>> now is probably the best time :-) >>> >>> Thoughts? >>> >> >> I don't quite get why we should do that. You can already have an EXPLAIN >> or an EXPLAIN ANALYZE. Or is it simply to really differentiate them? >> because one won't launch the query and the other one will? > > Now we have: > explain > Explain Options -> [verbose, analyze, newstuff] > > I'd prefer > Explain > Explain Analyze > Explain Options -> [verbose, newstuff] > Done. F7 launches an explain, Shift-F7 launches an explain analyze. New patch attached. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
Вложения
Le 23/01/2010 19:20, Euler Taveira de Oliveira a écrit : > Magnus Hagander escreveu: >> While you're whacking around the explain code, one thing that has >> always nagged me, is that ANALYZE is an option for EXPLAIN. IMO we >> should have "Explain" and "Explain Analyze" as separate commands, >> because they actually do different things. And then options for things >> like verbose, costs, buffers etc. If we're going to change that ever, >> now is probably the best time :-) >> > +1. It always annoys me too. > > There is another improvement that we could change: EXPLAIN VERBOSE could > generate graphical output too. I understand the reason why we don't generate > it (that's because of the long text output) but in 8.4 or later that noisy > output was wiped out. It' so trivial that it is attached. > I added your patch in mine. See my answer to Magnus. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
Guillaume Lelarge escreveu: > New patch attached. > Nice job. But there are just a few gripes about your patch: (i) it doesn't disable the EXPLAIN button when the BUFFERS option is enabled, (ii) it doesn't disable the EXPLAIN ANALYZE menu when the query is running, and (iii) there is a bug when the BUFFERS option is enabled and you close and open the query tool again (the EXPLAIN menu turns enabled instead of disabled). The attached patch (on the top of your patch) addresses all of these problems. -- Euler Taveira de Oliveira http://www.timbira.com/ *** pgadmin/frm/frmQuery.cpp.orig 2010-01-26 00:52:01.000000000 -0200 --- pgadmin/frm/frmQuery.cpp 2010-01-26 00:54:01.000000000 -0200 *************** *** 1892,1897 **** --- 1892,1900 ---- void frmQuery::OnBuffers(wxCommandEvent& event) { queryMenu->Enable(MNU_EXPLAIN, !queryMenu->IsChecked(MNU_BUFFERS)); + toolBar->EnableTool(MNU_EXPLAIN, !queryMenu->IsChecked(MNU_BUFFERS)); + + settings->SetExplainBuffers(queryMenu->IsChecked(MNU_BUFFERS)); } // Update the main SQL query from the GQB if desired *************** *** 2111,2122 **** toolBar->EnableTool(MNU_EXECUTE, !running); toolBar->EnableTool(MNU_EXECPGS, !running); toolBar->EnableTool(MNU_EXECFILE, !running); ! toolBar->EnableTool(MNU_EXPLAIN, !running); toolBar->EnableTool(MNU_CANCEL, running); queryMenu->Enable(MNU_EXECUTE, !running); queryMenu->Enable(MNU_EXECPGS, !running); queryMenu->Enable(MNU_EXECFILE, !running); ! queryMenu->Enable(MNU_EXPLAIN, !running); queryMenu->Enable(MNU_CANCEL, running); fileMenu->Enable(MNU_EXPORT, sqlResult->CanExport()); fileMenu->Enable(MNU_QUICKREPORT, sqlResult->CanExport()); --- 2114,2126 ---- toolBar->EnableTool(MNU_EXECUTE, !running); toolBar->EnableTool(MNU_EXECPGS, !running); toolBar->EnableTool(MNU_EXECFILE, !running); ! toolBar->EnableTool(MNU_EXPLAIN, (!running && !settings->GetExplainBuffers())); toolBar->EnableTool(MNU_CANCEL, running); queryMenu->Enable(MNU_EXECUTE, !running); queryMenu->Enable(MNU_EXECPGS, !running); queryMenu->Enable(MNU_EXECFILE, !running); ! queryMenu->Enable(MNU_EXPLAIN, (!running && !settings->GetExplainBuffers())); ! queryMenu->Enable(MNU_EXPLAINANALYZE, !running); queryMenu->Enable(MNU_CANCEL, running); fileMenu->Enable(MNU_EXPORT, sqlResult->CanExport()); fileMenu->Enable(MNU_QUICKREPORT, sqlResult->CanExport());
Le 26/01/2010 04:13, Euler Taveira de Oliveira a écrit : > Guillaume Lelarge escreveu: >> New patch attached. >> > Nice job. But there are just a few gripes about your patch: (i) it doesn't > disable the EXPLAIN button when the BUFFERS option is enabled, (ii) it doesn't > disable the EXPLAIN ANALYZE menu when the query is running, and (iii) there is > a bug when the BUFFERS option is enabled and you close and open the query tool > again (the EXPLAIN menu turns enabled instead of disabled). > > The attached patch (on the top of your patch) addresses all of these problems. > Thanks a lot for your help. I added your patch, here is the new complete patch. Seems good to me. Any more comments before I apply it? -- Guillaume. http://www.postgresqlfr.org http://dalibo.com
Вложения
Le 26/01/2010 10:18, Guillaume Lelarge a écrit : > Le 26/01/2010 04:13, Euler Taveira de Oliveira a écrit : >> Guillaume Lelarge escreveu: >>> New patch attached. >>> >> Nice job. But there are just a few gripes about your patch: (i) it doesn't >> disable the EXPLAIN button when the BUFFERS option is enabled, (ii) it doesn't >> disable the EXPLAIN ANALYZE menu when the query is running, and (iii) there is >> a bug when the BUFFERS option is enabled and you close and open the query tool >> again (the EXPLAIN menu turns enabled instead of disabled). >> >> The attached patch (on the top of your patch) addresses all of these problems. >> > > Thanks a lot for your help. I added your patch, here is the new complete > patch. > > Seems good to me. Any more comments before I apply it? > Commited. -- Guillaume. http://www.postgresqlfr.org http://dalibo.com