Обсуждение: pgmonitor patch for query string
I would like to apply the following patch to the CVS tree. It allows pgmonitor to show query strings even if the backend is not compiled with debug symbols. It does this by creating a global variable 'debug_query_string' and assigning it when the query begins and clearing it when the query ends. It needs to be a global symbol so gdb can find it without debug symbols. Seems like a very safe patch, and it allows pgmonitor to be much more useful until we get a shared memory solution in 7.2. Is this OK with everyone? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 Index: src/backend/tcop/postgres.c =================================================================== RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/tcop/postgres.c,v retrieving revision 1.211 diff -c -r1.211 postgres.c *** src/backend/tcop/postgres.c 2001/03/14 15:14:35 1.211 --- src/backend/tcop/postgres.c 2001/03/14 16:29:26 *************** *** 74,79 **** --- 74,81 ---- extern int optind; extern char *optarg; + char *debug_query_string; /* used by pgmonitor */ + /* * for ps display */ *************** *** 621,626 **** --- 623,630 ---- List *parsetree_list, *parsetree_item; + debug_query_string = query_string; /* used by pgmonitor */ + /* * Start up a transaction command. All queries generated by the * query_string will be in this same command block, *unless* we find *************** *** 855,860 **** --- 859,866 ---- */ if (xact_started) finish_xact_command(); + + debug_query_string = NULL; /* used by pgmonitor */ } /* *************** *** 1718,1723 **** --- 1724,1731 ---- if (sigsetjmp(Warn_restart, 1) != 0) { + debug_query_string = NULL; /* used by pgmonitor */ + /* * NOTE: if you are tempted to add more code in this if-block, * consider the probability that it should be in AbortTransaction()
not with me it isn't ... it doesn't fix a bug, it doesn't go in ... save it for after v7.1 is released ... On Wed, 14 Mar 2001, Bruce Momjian wrote: > I would like to apply the following patch to the CVS tree. It allows > pgmonitor to show query strings even if the backend is not compiled with > debug symbols. > > It does this by creating a global variable 'debug_query_string' and > assigning it when the query begins and clearing it when the query ends. > It needs to be a global symbol so gdb can find it without debug symbols. > > Seems like a very safe patch, and it allows pgmonitor to be much more > useful until we get a shared memory solution in 7.2. > > Is this OK with everyone? > > -- > Bruce Momjian | http://candle.pha.pa.us > pgman@candle.pha.pa.us | (610) 853-3000 > + If your life is a hard drive, | 830 Blythe Avenue > + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 > Marc G. Fournier ICQ#7615664 IRC Nick: Scrappy Systems Administrator @ hub.org primary: scrappy@hub.org secondary: scrappy@{freebsd|postgresql}.org
> > not with me it isn't ... it doesn't fix a bug, it doesn't go in ... save > it for after v7.1 is released ... You are saying save it for 7.2, right? That will certainly be months away. Without this patch, pgmonitor's 'query' button will only work if the postgres binary was compiled with debug symbols. You are basically saying no, even thought it has no risks, right? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
On Wed, 14 Mar 2001, Bruce Momjian wrote: > > > > not with me it isn't ... it doesn't fix a bug, it doesn't go in ... save > > it for after v7.1 is released ... > > You are saying save it for 7.2, right? That will certainly be months > away. Without this patch, pgmonitor's 'query' button will only work if > the postgres binary was compiled with debug symbols. Put it up as a patch to v7.1, and include it as part of 7.1.1 ... > You are basically saying no, even thought it has no risks, right? I'm saying no because it doesn't fix any known bugs, it *adds* another feature ... we are *months* too late in the cycle for that ...
> On Wed, 14 Mar 2001, Bruce Momjian wrote: > > > > > > > not with me it isn't ... it doesn't fix a bug, it doesn't go in ... save > > > it for after v7.1 is released ... > > > > You are saying save it for 7.2, right? That will certainly be months > > away. Without this patch, pgmonitor's 'query' button will only work if > > the postgres binary was compiled with debug symbols. > > Put it up as a patch to v7.1, and include it as part of 7.1.1 ... Oh. It would just be easier to say that I support 7.1 rather than 7.1.1, though that is certainly much better than 7.2. :-) > > You are basically saying no, even thought it has no risks, right? > > I'm saying no because it doesn't fix any known bugs, it *adds* another > feature ... we are *months* too late in the cycle for that ... I agree we are many months back, and I am a little upset about it myself. I just don't see any delay or risk in applying the patch. However, 7.1.1 is fine, so I will just wait for that one. I am working on a lock-status display option now too, so I can put them both in 7.1.1 maybe with the pgmonitor script in CVS too. Sounds like a plan. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
> Bruce Momjian writes: > > > It does this by creating a global variable 'debug_query_string' and > > assigning it when the query begins and clearing it when the query ends. > > You can find out the current query for a given backend by configuring the > server with "debug_print_query on" and "log_pids on" and running > > sed -n "/[$pid]/"'s/^.*query: \(.*\)$/\1/p' $logfile > > This doesn't tell you whether the query is still running, but ps tells you > that. In fact, it might be an idea to add a logging option that prints > something like "query finished in xxx ms". We actually have something > similar hidden under show_query_stats, but the formatting needs to be made > more convenient and possibly less verbose. But at least this way you have > it for the record, and not only on the screen. Yes, I thought of that idea. I wasn't sure I would be able to find the log file in any installation-independent way, or even if a log file was even being kept. And I was afraid some people wouldn't want to log all queries. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Bruce Momjian writes: > It does this by creating a global variable 'debug_query_string' and > assigning it when the query begins and clearing it when the query ends. You can find out the current query for a given backend by configuring the server with "debug_print_query on" and "log_pids on" and running sed -n "/[$pid]/"'s/^.*query: \(.*\)$/\1/p' $logfile This doesn't tell you whether the query is still running, but ps tells you that. In fact, it might be an idea to add a logging option that prints something like "query finished in xxx ms". We actually have something similar hidden under show_query_stats, but the formatting needs to be made more convenient and possibly less verbose. But at least this way you have it for the record, and not only on the screen. -- Peter Eisentraut peter_e@gmx.net http://yi.org/peter-e/
The Hermit Hacker <scrappy@hub.org> writes:
> I'm saying no because it doesn't fix any known bugs, it *adds* another
> feature ... we are *months* too late in the cycle for that ...
I thought it was a pretty good idea even without any consideration for
Bruce's monitor program. The advantage is that one would be able to
extract the current query from a crashed backend's core dump, even if
the backend had been compiled without debug symbols. Right now you can
only find out the query if you had compiled with debug, because you have
to be able to look at local variables of functions. And there are an
awful lot of people who don't use debug-enabled builds...
Given that and the low-risk nature of the patch, I vote for it.
regards, tom lane
On Wed, 14 Mar 2001, Peter Eisentraut wrote: > Bruce Momjian writes: > > > It does this by creating a global variable 'debug_query_string' and > > assigning it when the query begins and clearing it when the query ends. > > You can find out the current query for a given backend by configuring the > server with "debug_print_query on" and "log_pids on" and running > > sed -n "/[$pid]/"'s/^.*query: \(.*\)$/\1/p' $logfile > > This doesn't tell you whether the query is still running, but ps tells you > that. In fact, it might be an idea to add a logging option that prints > something like "query finished in xxx ms". We actually have something > similar hidden under show_query_stats, but the formatting needs to be made > more convenient and possibly less verbose. But at least this way you have > it for the record, and not only on the screen. I *definitely* like this one ... I've been doing wrappers around my pg_exec() calls in PHP to do some stats generation to work on "slow queries", but having it in the backend would be more exact ... and easier to use then having to modify your apps ...
> > This doesn't tell you whether the query is still running, but ps tells you > > that. In fact, it might be an idea to add a logging option that prints > > something like "query finished in xxx ms". We actually have something > > similar hidden under show_query_stats, but the formatting needs to be made > > more convenient and possibly less verbose. But at least this way you have > > it for the record, and not only on the screen. > > I *definitely* like this one ... I've been doing wrappers around my > pg_exec() calls in PHP to do some stats generation to work on "slow > queries", but having it in the backend would be more exact ... and easier > to use then having to modify your apps ... > Added to TODO: * Allow logging of query durations -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
> > I don't understand the attraction of the UDP stuff. If we have the
> > stuff in shared memory, we can add a collector program that gathers info
> > from shared memory and allows others to access it, right?
>
> There are a couple of problems with shared memory. First you
> have to decide a size. That'll limit what you can put into
> and if you want to put things per table (#scans, #block-
> fetches, #cache-hits, ...), you might run out of mem either
> way with complicated, multy-thousand table schemas.
>
> And the above illustrates too that the data structs in the
> shmem wouldn't be just some simple arrays of counters. So we
> have to deal with locking for both, readers and writers of
> the statistics.
[ Jan, previous email was not sent to list, my mistake.]
OK, I understand the problem with pre-defined size. That is why I was
looking for a way to dump the information out to a flat file somehow.
I think no matter how we deal with this, we will need some way to turn
on/off such reporting. We can write into shared memory with little
penalty, but network or filesystem output is not going to be near-zero
cost.
OK, how about a shared buffer area that gets written in a loop so a
separate collection program can grab the info if it wants it, and if
not, it just gets overwritten later. It can even be per-backend:
loops start end (loop to start) ----- [-----------------------------] 5
statstat stat stat stat stat |^^^ current pointer
-- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610)
853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill,
Pennsylvania19026