From: Tom Lane <tgl@sss.pgh.pa.us>
To: Peter Geoghegan <pg@heroku.com>
Cc: Jim Nasby <Jim.Nasby@bluetreble.com>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: Less than ideal error reporting in pg_stat_statements
Date: Sun, 04 Oct 2015 12:27:15 -0400
Message-ID: <32385.1443976035@sss.pgh.pa.us> (raw)
In-Reply-To: <CAM3SWZTZfwJRBv85XHcbc-TRhM4xsOD6fYfU3anF9WVTNeRUFw@mail.gmail.com>
References: <5601D354.5000703@BlueTreble.com>
<CAM3SWZRo_5ZznTp=Mg5Kh2eq-LjWLVS-BXJwsHe_dS50g34Mfw@mail.gmail.com>
<15898.1442965225@sss.pgh.pa.us>
<CAM3SWZT41NeiK74J4PeKBA=rkML6-TR1pmj_ABf7Y6c8ihR+4A@mail.gmail.com>
<CAM3SWZTOjkP9EJsaUcvhpHxXfdCyXbCcFuTDNT4Qct90ZMU8Hw@mail.gmail.com>
<CAM3SWZTJEPY7A5HBJMB-s6rhZxk8LXiaxZPAMDytBpLs3P60Tw@mail.gmail.com>
<CAM3SWZQBf6nepv2PKm_PDqwxUzxgzp5664bmRpq4AhSTM2UX_w@mail.gmail.com>
<5913.1443822154@sss.pgh.pa.us>
<CAM3SWZQjp_W4bZC=BgEQrP3EXrEnOzaytkJZbntdmZe8f2EOBQ@mail.gmail.com>
<CAM3SWZTahp86i-5vDnfL5nzYCUjdmRu=sWRKXEZVd3E9dKGEFA@mail.gmail.com>
<CAM3SWZTwNKkeCN+n_ew2eLFrWT9+UcPRLUTyy_xWN12QzGORsA@mail.gmail.com>
<9935.1443828452@sss.pgh.pa.us>
<CAM3SWZTZfwJRBv85XHcbc-TRhM4xsOD6fYfU3anF9WVTNeRUFw@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
Peter Geoghegan <pg@heroku.com> 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
> exception, but probably not, since gc_qtexts() is reached when a new
> entry is created with a new query text, which in general makes OOM
> progressively more likely.
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 result in random-seeming decisions about whether
to normalize. For instance:
* mean_query_len only gets recomputed in entry_dealloc(), which is only
run if we exceed pgss_max, and gc_qtexts(), which is only run if we decide
the query texts file is more than 50% bloat. So there could be quite a
long startup transient before the value gets off its initialization
minimum, and I'm suspicious that there might be plausible use-cases where
it never does. So it's not so much "restrict to a multiple of the mean
query len" as "restrict to some number that might once upon a time have
had some relation to the mean query len, or maybe not".
* One could expect that after changing mean_query_len, the population of
query texts would change character as a result of the filter behavior
changing, so that convergence to stable behavior over the long haul is
not exactly self-evident.
* As you've got it here, entry_dealloc() and gc_qtexts() don't compute
mean_query_len the same way, because only one of them discriminates
against sticky entries. So the value would bounce about rather randomly
based on which one had run last.
* I'm not exactly convinced that sticky entries should be ignored for
this purpose anyway.
Taking a step back, ISTM the real issue you're fighting here is lots of
orphaned sticky entries, but the patch doesn't do anything directly to fix
that problem. I wonder if we should do something like making
entry_dealloc() and/or gc_qtexts() aggressively remove sticky entries,
or at least those with "large" texts.
I think the aspects of this patch that are reasonably uncontroversial are
increasing the allowed malloc attempt size in gc_qtexts, flushing the
query text file on malloc failure, fixing the missing cleanup steps after
a gc failure, and making entry_dealloc's recomputation of mean_query_len
sane (which I'll define for the moment as "the same as gc_qtexts would
get"). Since we're hard against a release deadline, I propose to commit
just those changes, and we can consider the idea of a query size filter
and/or redefining mean_query_len at leisure.
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, pg@heroku.com, Jim.Nasby@bluetreble.com
Subject: Re: Less than ideal error reporting in pg_stat_statements
In-Reply-To: <32385.1443976035@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox