Re: [HACKERS] bug: json format and auto_explain
Tom Lane wrote: Euler Taveira de Oliveira eu...@timbira.com writes: While testing the EXPLAIN BUFFERS patch I found a bug. I'm too tired to provide a fix right now but if someone can take it I will appreciate. It seems ExplainJSONLineEnding() doesn't expect es-grouping_stack list as a null pointer. Looks like auto_explain is under the illusion that it need not call ExplainBeginOutput/ExplainEndOutput. Fortunately the author of auto_explain can now commit the fix by himself ... Kudos, BTW :-) -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug: json format and auto_explain
Alvaro Herrera escreveu: Fortunately the author of auto_explain can now commit the fix by himself ... Kudos, BTW :-) Congratulations! -- Euler Taveira de Oliveira http://www.timbira.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug: json format and auto_explain
Itagaki Takahiro itagaki.takah...@oss.ntt.co.jp writes:
Tom Lane t...@sss.pgh.pa.us wrote:
Looks like auto_explain is under the illusion that it need not call
ExplainBeginOutput/ExplainEndOutput.
Explain{Begin/End}Output are static functions, so we cannot call them
from an external contrib module. Instead, I'll suggest to call them
automatically from ExplainPrintPlan. The original codes in ExplainPrintPlan
was moved into ExplainOneResult, that name might be debatable.
This isn't an adequate solution I'm afraid --- what about the other
functions that are exported by explain.h?
I too am not totally thrilled with the idea of exporting
Explain{Begin/End}Output, but it might be the best solution.
We might also need to think about refactoring those functions:
there seem to be two different things going on there, one being
format-specific initialization which will certainly be necessary,
and one being output of a wrapper structure which might or might
not be appropriate for what auto_explain or other callers want.
In any case you need to expend more effort on the comments for the
functions. Inadequate specification of ExplainPrintPlan's call
requirements is what got us into this problem in the first place,
and the proposed patch makes that worse not better.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug: json format and auto_explain
On Mon, Dec 7, 2009 at 10:42 AM, Alvaro Herrera alvhe...@commandprompt.com wrote: Tom Lane wrote: Euler Taveira de Oliveira eu...@timbira.com writes: While testing the EXPLAIN BUFFERS patch I found a bug. I'm too tired to provide a fix right now but if someone can take it I will appreciate. It seems ExplainJSONLineEnding() doesn't expect es-grouping_stack list as a null pointer. Looks like auto_explain is under the illusion that it need not call ExplainBeginOutput/ExplainEndOutput. Fortunately the author of auto_explain can now commit the fix by himself ... Kudos, BTW :-) I just tested and this has been broken since it was committed (by Tom). However, I believe I did test it on the last version of the patch that I submitted, and I think it worked then. So we have lots of choices for who should fix this: - me, since it's my feature - Tom, since it appears that he broke it - Itagaki-san, since he's the auto_explain maintainer :-) ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug: json format and auto_explain
On Mon, Dec 7, 2009 at 11:07 AM, Tom Lane t...@sss.pgh.pa.us wrote:
Itagaki Takahiro itagaki.takah...@oss.ntt.co.jp writes:
Tom Lane t...@sss.pgh.pa.us wrote:
Looks like auto_explain is under the illusion that it need not call
ExplainBeginOutput/ExplainEndOutput.
Explain{Begin/End}Output are static functions, so we cannot call them
from an external contrib module. Instead, I'll suggest to call them
automatically from ExplainPrintPlan. The original codes in ExplainPrintPlan
was moved into ExplainOneResult, that name might be debatable.
This isn't an adequate solution I'm afraid --- what about the other
functions that are exported by explain.h?
I too am not totally thrilled with the idea of exporting
Explain{Begin/End}Output, but it might be the best solution.
We might also need to think about refactoring those functions:
there seem to be two different things going on there, one being
format-specific initialization which will certainly be necessary,
and one being output of a wrapper structure which might or might
not be appropriate for what auto_explain or other callers want.
In any case you need to expend more effort on the comments for the
functions. Â Inadequate specification of ExplainPrintPlan's call
requirements is what got us into this problem in the first place,
and the proposed patch makes that worse not better.
There's an awful lot of layers of function calls happening in
explain.c for no really discernable purpose that I can see.
ExplainOneQuery() calls either ExplainOneUtility() if it has a utility
command or ExplainOneQuery_hook() if that's defined or else it plans
the query and then calls ExplainOnePlan(). ExplainOneUtility() in
turn calls ExplainExecuteQuery for execute statements and handles
everything else internally. The placement of the hook seems pretty
dubious to me (does anyone actually use it?), and the whole structure
seems like it could probably be flattened a bit.
...Robert
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug: json format and auto_explain
Robert Haas robertmh...@gmail.com writes: There's an awful lot of layers of function calls happening in explain.c for no really discernable purpose that I can see. ExplainOneQuery() calls either ExplainOneUtility() if it has a utility command or ExplainOneQuery_hook() if that's defined or else it plans the query and then calls ExplainOnePlan(). ExplainOneUtility() in turn calls ExplainExecuteQuery for execute statements and handles everything else internally. The placement of the hook seems pretty dubious to me (does anyone actually use it?), and the whole structure seems like it could probably be flattened a bit. IIRC, a fair amount of the messiness has to do with handling prepared statements. Maybe refactoring the division of labor between prepare.c and explain.c would help. Whether it's worth the trouble is questionable though. The immediate issue seems to be insufficient thought about how to manage the initialization conditions for the various formatters, and I don't think that any flattening or refactoring of the rest of the logic would have made any difference there. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] bug: json format and auto_explain
Hi, While testing the EXPLAIN BUFFERS patch I found a bug. I'm too tired to provide a fix right now but if someone can take it I will appreciate. It seems ExplainJSONLineEnding() doesn't expect es-grouping_stack list as a null pointer. eu...@harman $ ./install/bin/psql psql (8.5devel) Type help for help. euler=# load 'auto_explain'; LOAD euler=# set auto_explain.log_min_duration to 0; SET euler=# set auto_explain.log_format to 'json'; SET euler=# explain (format json) select * from pgbench_branches inner join pgbench_history using (bid) where bid 100; server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. The connection to the server was lost. Attempting reset: Succeeded. euler=# \q eu...@harman /a/pgsql/dev $ gdb ./install/bin/postgres ./data/core GNU gdb 6.8 Copyright (C) 2008 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later http://gnu.org/licenses/gpl.html This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type show copying and show warranty for details. This GDB was configured as i686-pc-linux-gnu... warning: Can't read pathname for load map: Input/output error. Reading symbols from /usr/lib/libxml2.so.2...done. Loaded symbols for /usr/lib/libxml2.so.2 Reading symbols from /usr/lib/libssl.so.0.9.8...done. Loaded symbols for /usr/lib/libssl.so.0.9.8 Reading symbols from /usr/lib/libcrypto.so.0.9.8...done. Loaded symbols for /usr/lib/libcrypto.so.0.9.8 Reading symbols from /lib/libdl.so.2...done. Loaded symbols for /lib/libdl.so.2 Reading symbols from /lib/libm.so.6...done. Loaded symbols for /lib/libm.so.6 Reading symbols from /usr/lib/libldap-2.4.so.2...done. Loaded symbols for /usr/lib/libldap-2.4.so.2 Reading symbols from /lib/libc.so.6...done. Loaded symbols for /lib/libc.so.6 Reading symbols from /lib/libz.so.1...done. Loaded symbols for /lib/libz.so.1 Reading symbols from /usr/lib/libgssapi_krb5.so.2...done. Loaded symbols for /usr/lib/libgssapi_krb5.so.2 Reading symbols from /usr/lib/libkrb5.so.3...done. Loaded symbols for /usr/lib/libkrb5.so.3 Reading symbols from /lib/libcom_err.so.2...done. Loaded symbols for /lib/libcom_err.so.2 Reading symbols from /usr/lib/libk5crypto.so.3...done. Loaded symbols for /usr/lib/libk5crypto.so.3 Reading symbols from /lib/libresolv.so.2...done. Loaded symbols for /lib/libresolv.so.2 Reading symbols from /lib/ld-linux.so.2...done. Loaded symbols for /lib/ld-linux.so.2 Reading symbols from /usr/lib/liblber-2.4.so.2...done. Loaded symbols for /usr/lib/liblber-2.4.so.2 Reading symbols from /usr/lib/libsasl2.so.2...done. Loaded symbols for /usr/lib/libsasl2.so.2 Reading symbols from /usr/lib/libgnutls.so.26...done. Loaded symbols for /usr/lib/libgnutls.so.26 Reading symbols from /usr/lib/libkrb5support.so.0...done. Loaded symbols for /usr/lib/libkrb5support.so.0 Reading symbols from /lib/libpthread.so.0...done. Loaded symbols for /lib/libpthread.so.0 Reading symbols from /lib/libcrypt.so.1...done. Loaded symbols for /lib/libcrypt.so.1 Reading symbols from /usr/lib/libtasn1.so.3...done. Loaded symbols for /usr/lib/libtasn1.so.3 Reading symbols from /usr/lib/libgcrypt.so.11...done. Loaded symbols for /usr/lib/libgcrypt.so.11 Reading symbols from /usr/lib/libgpg-error.so.0...done. Loaded symbols for /usr/lib/libgpg-error.so.0 Reading symbols from /lib/libnss_files.so.2...done. Loaded symbols for /lib/libnss_files.so.2 Reading symbols from /a/pgsql/dev/install/lib/auto_explain.so...done. Loaded symbols for /a/pgsql/dev/install/lib/auto_explain.so Core was generated by `postgres: euler euler [local] EXPLAIN '. Program terminated with signal 11, Segmentation fault. [New process 2751] #0 0x081a2158 in ExplainJSONLineEnding (es=0xbfd117dc) at /a/pgsql/dev/postgresql/src/backend/commands/explain.c:1885 warning: Source file is more recent than executable. 1885if (linitial_int(es-grouping_stack) != 0) (gdb) print es-grouping_stack $1 = (List *) 0x0 (gdb) print es-str-data $2 = 0x865b09c (gdb) bt #0 0x081a2158 in ExplainJSONLineEnding (es=0xbfd117dc) at /a/pgsql/dev/postgresql/src/backend/commands/explain.c:1885 #1 0x081a1ada in ExplainOpenGroup (objtype=0x84fcb64 Plan, labelname=0x84fcb64 Plan, labeled=1 '\001', es=0xbfd117dc) at /a/pgsql/dev/postgresql/src/backend/commands/explain.c:1684 #2 0x0819faac in ExplainNode (plan=0x8652c98, planstate=0x8654868, outer_plan=0x0, relationship=0x0, plan_name=0x0, es=0xbfd117dc) at /a/pgsql/dev/postgresql/src/backend/commands/explain.c:739 #3 0x0819f393 in ExplainPrintPlan (es=0xbfd117dc, queryDesc=0x8653724) at /a/pgsql/dev/postgresql/src/backend/commands/explain.c:474 #4 0xb7ef0010 in explain_ExecutorEnd (queryDesc=0x8653724) at /a/pgsql/dev/postgresql/contrib/auto_explain/auto_explain.c:238 #5 0x081f1094 in ExecutorEnd (queryDesc=0x8653724) at /a/pgsql/dev/postgresql/src/backend/executor/execMain.c:314 #6
Re: [HACKERS] bug: json format and auto_explain
Euler Taveira de Oliveira eu...@timbira.com writes: While testing the EXPLAIN BUFFERS patch I found a bug. I'm too tired to provide a fix right now but if someone can take it I will appreciate. It seems ExplainJSONLineEnding() doesn't expect es-grouping_stack list as a null pointer. Looks like auto_explain is under the illusion that it need not call ExplainBeginOutput/ExplainEndOutput. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] bug: json format and auto_explain
Tom Lane t...@sss.pgh.pa.us wrote:
Looks like auto_explain is under the illusion that it need not call
ExplainBeginOutput/ExplainEndOutput.
They were added by XML formatter; I suppose it worked on 8.4.
Explain{Begin/End}Output are static functions, so we cannot call them
from an external contrib module. Instead, I'll suggest to call them
automatically from ExplainPrintPlan. The original codes in ExplainPrintPlan
was moved into ExplainOneResult, that name might be debatable.
Patch attached.
Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center
auto_explain_fix_20091207.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
