Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-20 Thread Jim Nasby
On 10/4/15 6:10 PM, Tom Lane wrote: Andrew Dunstan writes: >Sorry, I'm a bit late to this party. Does what you have committed mean >people are less likely to see "Out of Memory" coming from >pg_stat_statements? If not, what can be done about them short of a >restart? And what bad effects follow

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-09 Thread Jim Nasby
On 10/5/15 10:50 AM, Tom Lane wrote: Alvaro Herrera writes: Andrew Dunstan wrote: FWIW, (a) and (b) but not (c) is probably the right description for my client who has been seeing problems here. I think the fact that long IN lists are fingerprinted differently according to the number of ele

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-05 Thread Peter Geoghegan
On Mon, Oct 5, 2015 at 8:50 AM, Tom Lane wrote: > That's certainly something worth looking at, but I think it's probably > more complicated than that. If you just write "WHERE x IN (1,2,3,4)", > that gets folded to a ScalarArrayOp with a single array constant, which > the existing code would deal

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-05 Thread Tom Lane
Alvaro Herrera writes: > Andrew Dunstan wrote: >> FWIW, (a) and (b) but not (c) is probably the right description for my >> client who has been seeing problems here. > I think the fact that long IN lists are fingerprinted differently > according to the number of elements in the list makes the sce

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-05 Thread Alvaro Herrera
Andrew Dunstan wrote: > > On 10/05/2015 11:15 AM, Tom Lane wrote: > >Peter Geoghegan writes: > >>I'm annoyed and disappointed that the patch committed does not even > >>begin to address the underlying problem -- it just adds an escape > >>hatch, and fixes another theoretical issue that no one was

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-05 Thread Andrew Dunstan
On 10/05/2015 11:15 AM, Tom Lane wrote: Peter Geoghegan writes: I'm annoyed and disappointed that the patch committed does not even begin to address the underlying problem -- it just adds an escape hatch, and fixes another theoretical issue that no one was affected by. Honestly, next time I w

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-05 Thread Tom Lane
Peter Geoghegan writes: > I'm annoyed and disappointed that the patch committed does not even > begin to address the underlying problem -- it just adds an escape > hatch, and fixes another theoretical issue that no one was affected > by. Honestly, next time I won't bother. The problem as I see it

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-04 Thread Peter Geoghegan
On Wed, Sep 23, 2015 at 1:41 PM, Jim Nasby wrote: > max was set to 1. I don't know about average query text size, but the > command that was causing the error was a very large number of individual > INSERT ... VALUES statements all in one command. > > The machine had plenty of free memory and

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-04 Thread Peter Geoghegan
On Sun, Oct 4, 2015 at 3:16 PM, Tom Lane wrote: > Peter Geoghegan writes: >> To be clear: I wasn't sure why you though I falsely count entries with >> dropped texts within entry_dealloc(). > > In the existing^H^H^Hprevious code, dropped-text entries would essentially > act as length-zero summands

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-04 Thread Peter Geoghegan
On Sun, Oct 4, 2015 at 3:10 PM, Tom Lane wrote: >> That seems perfectly reasonable, yes. Should I leave that to you? > > After a closer look I decided that wasn't reasonable at all. Discounting > sticky texts would then mean that after a GC cycle, we might still think > the query texts file is bl

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-04 Thread Tom Lane
Andrew Dunstan writes: > Sorry, I'm a bit late to this party. Does what you have committed mean > people are less likely to see "Out of Memory" coming from > pg_stat_statements? If not, what can be done about them short of a > restart? And what bad effects follow from an event generating them?

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-04 Thread Andrew Dunstan
On 10/04/2015 06:16 PM, Tom Lane wrote: Peter Geoghegan writes: To be clear: I wasn't sure why you though I falsely count entries with dropped texts within entry_dealloc(). In the existing^H^H^Hprevious code, dropped-text entries would essentially act as length-zero summands in the average c

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-04 Thread Tom Lane
Peter Geoghegan writes: > To be clear: I wasn't sure why you though I falsely count entries with > dropped texts within entry_dealloc(). In the existing^H^H^Hprevious code, dropped-text entries would essentially act as length-zero summands in the average calculation, whereas I think we agree that

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-04 Thread Tom Lane
Peter Geoghegan writes: > On Sun, Oct 4, 2015 at 1:01 PM, Tom Lane wrote: >> Hm. The problem I've got with this is that then mean_query_len means >> something significantly different after entry_dealloc than it does >> after gc_texts. >> >> I'd be okay with changing *both* of those functions to

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-04 Thread Peter Geoghegan
On Sun, Oct 4, 2015 at 1:12 PM, Peter Geoghegan wrote: > On Sun, Oct 4, 2015 at 1:01 PM, Tom Lane wrote: >> Ah, right, sorry. I meant to make its result match what gc_texts would >> get, by not falsely counting entries with dropped texts. That's not >> what you have in your patch but it seems l

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-04 Thread Tom Lane
Peter Geoghegan writes: > On Sun, Oct 4, 2015 at 1:01 PM, Tom Lane wrote: >> Hm. The problem I've got with this is that then mean_query_len means >> something significantly different after entry_dealloc than it does >> after gc_texts. >> >> I'd be okay with changing *both* of those functions to

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-04 Thread Peter Geoghegan
On Sun, Oct 4, 2015 at 1:01 PM, Tom Lane wrote: > Ah, right, sorry. I meant to make its result match what gc_texts would > get, by not falsely counting entries with dropped texts. That's not > what you have in your patch but it seems like an easy enough fix. I'm trying to make mean_query_len re

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-04 Thread Tom Lane
Peter Geoghegan writes: > I'm not clear on what you actually propose to do to "make > entry_dealloc's recomputation of mean_query_len sane", but I think you > are talking about something distinct from what I've proposed Ah, right, sorry. I meant to make its result match what gc_texts would get,

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-04 Thread Peter Geoghegan
On Sun, Oct 4, 2015 at 9:27 AM, Tom Lane wrote: > Hm. I'm unconvinced by the aspects of this that involve using > mean_query_len as a filter on which texts will be accepted. While that's > not necessarily bad in the abstract, there are way too many implementation > artifacts here that will resul

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-04 Thread Tom Lane
Peter Geoghegan writes: > Attached, revised patch deals with the issues around removing the > query text file when garbage collection encounters trouble. There is > no reason to be optimistic about any error within gc_qtexts() not > recurring during a future garbage collection. OOM might be an > e

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-03 Thread Peter Geoghegan
On Fri, Oct 2, 2015 at 4:27 PM, Tom Lane wrote: > ... do you want to produce an updated patch? I'm not planning to look at > it until tomorrow anyway. Attached, revised patch deals with the issues around removing the query text file when garbage collection encounters trouble. There is no reason

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-02 Thread Peter Geoghegan
On Fri, Oct 2, 2015 at 4:27 PM, Tom Lane wrote: > ... do you want to produce an updated patch? I'm not planning to look at > it until tomorrow anyway. I'll post a new patch by about midday tomorrow west coast time. Hopefully that works for you. -- Peter Geoghegan -- Sent via pgsql-hackers m

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-02 Thread Tom Lane
... do you want to produce an updated patch? I'm not planning to look at it until tomorrow anyway. 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-hack

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-02 Thread Peter Geoghegan
On Fri, Oct 2, 2015 at 4:11 PM, Peter Geoghegan wrote: > Actually, isn't that another bug? The fact that we don't do the same > from within gc_qtexts() in normal cases, even with an exclusive lock > held? We do this: Ah, no. We check pgss->gc_count in any case, so it should be fine. That will als

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-02 Thread Peter Geoghegan
On Fri, Oct 2, 2015 at 3:57 PM, Peter Geoghegan wrote: > The spinlock acquisition above is actually necessary despite the > n_writers trick, because that's only used by qtext_store(). Actually, isn't that another bug? The fact that we don't do the same from within gc_qtexts() in normal cases, eve

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-02 Thread Peter Geoghegan
On Fri, Oct 2, 2015 at 3:57 PM, Peter Geoghegan wrote: > (void) AllocateFile(PGSS_TEXT_FILE, PG_BINARY_W); Naturally, a FreeFile() call will also be required for any successfully allocated file. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-02 Thread Peter Geoghegan
On Fri, Oct 2, 2015 at 2:42 PM, Tom Lane wrote: > Peter Geoghegan writes: >> I think that SIZE_MAX should be replaced by MaxAllocHugeSize before >> the patch is committed. That should be perfectly portable. > > Hmm ... only back to 9.4, but I guess that's far enough. I just realized that the exi

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-02 Thread Tom Lane
Peter Geoghegan writes: > I think that SIZE_MAX should be replaced by MaxAllocHugeSize before > the patch is committed. That should be perfectly portable. Hmm ... only back to 9.4, but I guess that's far enough. regards, tom lane -- Sent via pgsql-hackers mailing list

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-02 Thread Peter Geoghegan
On Fri, Oct 2, 2015 at 2:29 PM, Tom Lane wrote: > I'm not too impressed with this bit: > > /* Allocate buffer; beware that off_t might be wider than size_t */ > - if (stat.st_size <= MaxAllocSize) > + if (stat.st_size <= SIZE_MAX) > buf = (char *) malloc(stat.st

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-02 Thread Tom Lane
Peter Geoghegan writes: > It would be nice to get this committed before the next point releases > are tagged, since I've now heard a handful of complaints like this. I'm not too impressed with this bit: /* Allocate buffer; beware that off_t might be wider than size_t */ - if (stat

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-02 Thread Peter Geoghegan
On Fri, Oct 2, 2015 at 2:04 PM, Peter Geoghegan wrote: > It would be nice to get this committed before the next point releases > are tagged, since I've now heard a handful of complaints like this. I grep'd for SIZE_MAX to make sure that that was something that is available on all supported platfo

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-02 Thread Peter Geoghegan
On Tue, Sep 22, 2015 at 6:01 PM, Peter Geoghegan wrote: > On Tue, Sep 22, 2015 at 5:01 PM, Peter Geoghegan wrote: >> My guess is that this very large query involved a very large number of >> constants, possibly contained inside an " IN ( )". Slight variants of >> the same query, that a human woul

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-09-29 Thread Peter Geoghegan
On Fri, Sep 25, 2015 at 8:51 AM, Marti Raudsepp wrote: > I've also been seeing lots of log messages saying "LOG: out of > memory" on a server that's hosting development databases. I put off > debugging this until now because it didn't seem to have any adverse > effects on the system. That's unfo

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-09-25 Thread Peter Geoghegan
On Tue, Sep 22, 2015 at 6:01 PM, Peter Geoghegan wrote: > I'm doubtful that this had anything to do with MaxAllocSize. You'd > certainly need a lot of bloat to be affected by that in any way. I > wonder how high pg_stat_statements.max was set to on this system, and > how long each query text was o

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-09-25 Thread Peter Geoghegan
On Fri, Sep 25, 2015 at 11:37 AM, Peter Geoghegan wrote: >> So, as I understand it: if the system runs low on memory for an >> extended period, and/or the file grows beyond 1GB (MaxAlloc), garbage >> collection stops entirely, meaning it starts leaking disk space until >> a manual intervention. >

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-09-25 Thread Peter Geoghegan
On Fri, Sep 25, 2015 at 8:51 AM, Marti Raudsepp wrote: > I've also been seeing lots of log messages saying "LOG: out of > memory" on a server that's hosting development databases. I put off > debugging this until now because it didn't seem to have any adverse > effects on the system. > > The file

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-09-25 Thread Marti Raudsepp
On Wed, Sep 23, 2015 at 3:01 AM, Peter Geoghegan wrote: > I think that the real problem here is that garbage collection needs to > deal with OOM more appropriately. +1 I've also been seeing lots of log messages saying "LOG: out of memory" on a server that's hosting development databases. I put

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-09-23 Thread Jim Nasby
On 9/22/15 6:27 PM, Jim Nasby wrote: + ereport(LOG, + (errcode(ERRCODE_OUT_OF_MEMORY), + errmsg("out of memory attempting to pg_stat_statement file"), + errdetail("file \"%s\": size %lld", PGSS_TEXT_FILE, stat.st_size))); Uh, what? Oops. I'll fix that a

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-09-23 Thread Jim Nasby
On 9/22/15 8:01 PM, Peter Geoghegan wrote: I'm doubtful that this had anything to do with MaxAllocSize. You'd certainly need a lot of bloat to be affected by that in any way. I wonder how high pg_stat_statements.max was set to on this system, and how long each query text was on average. max was

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-09-22 Thread Peter Geoghegan
On Tue, Sep 22, 2015 at 6:55 PM, Alvaro Herrera wrote: > So if I have multiple queries like > > SELECT foo FROM bar WHERE baz IN (a, b) > SELECT foo FROM bar WHERE baz IN (a, b, c) > > they are not normalized down to the same? That seems odd. Yes, although in practice it's usually down to a vari

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-09-22 Thread Alvaro Herrera
Peter Geoghegan wrote: > My guess is that this very large query involved a very large number of > constants, possibly contained inside an " IN ( )". Slight variants of > the same query, that a human would probably consider to be equivalent > have caused artificial pressure on garbage collection.

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-09-22 Thread Peter Geoghegan
On Tue, Sep 22, 2015 at 5:01 PM, Peter Geoghegan wrote: > My guess is that this very large query involved a very large number of > constants, possibly contained inside an " IN ( )". Slight variants of > the same query, that a human would probably consider to be equivalent > have caused artificial

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-09-22 Thread Peter Geoghegan
On Tue, Sep 22, 2015 at 4:40 PM, Tom Lane wrote: > I wonder whether the real problem here is failure to truncate statement > texts to something sane. Do we really need to record the whole text of > multi-megabyte statements? Especially if doing so could render the entire > feature nonfunctional?

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-09-22 Thread Tom Lane
Peter Geoghegan writes: > I'm not opposed to this basic idea, but I think the message should be > reworded, and that the presence of two separate ereport() call sites > like the above is totally unnecessary. The existing MaxAllocSize check > is just defensive; no user-visible distinction needs to

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-09-22 Thread Tom Lane
David Rowley writes: > On 23 September 2015 at 10:16, Jim Nasby wrote: >>> Attached patch fixes, though I'm not sure if %lld is portable or not. It is not. > I think you could probably use INT64_FORMAT, Not in a message you expect to be translatable. There are ways around that, but TBH I do n

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-09-22 Thread Jim Nasby
On 9/22/15 5:58 PM, Peter Geoghegan wrote: On Tue, Sep 22, 2015 at 3:16 PM, Jim Nasby wrote: At first I thought the lack of context indicated a palloc had failed during ereport() (since we apparently just toss the previous error when that happens), but it turns out there's some error reporting

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-09-22 Thread Peter Geoghegan
On Tue, Sep 22, 2015 at 3:16 PM, Jim Nasby wrote: > At first I thought the lack of context indicated a palloc had failed during > ereport() (since we apparently just toss the previous error when that > happens), but it turns out there's some error reporting in > pg_stat_statements that's less than

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-09-22 Thread David Rowley
On 23 September 2015 at 10:16, Jim Nasby wrote: > > Attached patch fixes, though I'm not sure if %lld is portable or not. > > I think you could probably use INT64_FORMAT, and cast the stat.st_size to int64 too. There's an example in FileRead() in fd.c Regards David Rowley -- David Rowley

[HACKERS] Less than ideal error reporting in pg_stat_statements

2015-09-22 Thread Jim Nasby
A client was getting some hard to diagnose out of memory errors. What made this especially confusing was that there was no context reported at all, other than the (enormous) statement that triggered the error. At first I thought the lack of context indicated a palloc had failed during ereport(