Calculage avg. width when operator = is missing
Hi Hackers,
I've recently stumbled upon a problem with table bloat estimation in case
there are columns of type JSON.
The quick bloat estimation queries use sum over pg_statistic.stawidth of
table's columns, but in case of JSON the corresponding entry is never
created by the ANALYZE command due to equality comparison operator
missing. I understand why there is no such operator defined for this
particular type, but shouldn't we still try to produce meaningful average
width estimation?
In my case the actual bloat is around 40% as verified with pgstattuple,
while the bloat reported by quick estimate can be between 75% and 95%(!) in
three instances of this problem. We're talking about some hundreds of GB
of miscalculation.
Attached patch against master makes the std_typanalyze still try to compute
the minimal stats even if there is no "=" operator. Makes sense?
I could also find this report in archives that talks about similar problem,
but due to all values being over the analyze threshold:
/messages/by-id/12480.1389370514@sss.pgh.pa.us
I think we could try harder, otherwise any estimate relying on average
width can be way off in such cases.
--
Alex
Attachments:
still-analyze-when-no-eqopr.patchtext/x-patch; charset=US-ASCII; name=still-analyze-when-no-eqopr.patchDownload
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
new file mode 100644
index 861048f..903681e
*** a/src/backend/commands/analyze.c
--- b/src/backend/commands/analyze.c
*************** std_typanalyze(VacAttrStats *stats)
*** 1723,1732 ****
<opr, &eqopr, NULL,
NULL);
- /* If column has no "=" operator, we can't do much of anything */
- if (!OidIsValid(eqopr))
- return false;
-
/* Save the operator info for compute_stats routines */
mystats = (StdAnalyzeData *) palloc(sizeof(StdAnalyzeData));
mystats->eqopr = eqopr;
--- 1723,1728 ----
*************** std_typanalyze(VacAttrStats *stats)
*** 1737,1743 ****
/*
* Determine which standard statistics algorithm to use
*/
! if (OidIsValid(ltopr))
{
/* Seems to be a scalar datatype */
stats->compute_stats = compute_scalar_stats;
--- 1733,1739 ----
/*
* Determine which standard statistics algorithm to use
*/
! if (OidIsValid(eqopr) && OidIsValid(ltopr))
{
/* Seems to be a scalar datatype */
stats->compute_stats = compute_scalar_stats;
*************** std_typanalyze(VacAttrStats *stats)
*** 1776,1786 ****
/*
* compute_minimal_stats() -- compute minimal column statistics
*
! * We use this when we can find only an "=" operator for the datatype.
*
* We determine the fraction of non-null rows, the average width, the
* most common values, and the (estimated) number of distinct values.
*
* The most common values are determined by brute force: we keep a list
* of previously seen values, ordered by number of times seen, as we scan
* the samples. A newly seen value is inserted just after the last
--- 1772,1784 ----
/*
* compute_minimal_stats() -- compute minimal column statistics
*
! * We use this when we cannot find "=" and "<" operators for the datatype.
*
* We determine the fraction of non-null rows, the average width, the
* most common values, and the (estimated) number of distinct values.
*
+ * Only if there is a "=" operator, we try to find the most common values.
+ *
* The most common values are determined by brute force: we keep a list
* of previously seen values, ordered by number of times seen, as we scan
* the samples. A newly seen value is inserted just after the last
*************** compute_minimal_stats(VacAttrStatsP stat
*** 1809,1830 ****
Datum value;
int count;
} TrackItem;
! TrackItem *track;
! int track_cnt,
! track_max;
int num_mcv = stats->attr->attstattarget;
StdAnalyzeData *mystats = (StdAnalyzeData *) stats->extra_data;
! /*
! * We track up to 2*n values for an n-element MCV list; but at least 10
! */
! track_max = 2 * num_mcv;
! if (track_max < 10)
! track_max = 10;
! track = (TrackItem *) palloc(track_max * sizeof(TrackItem));
! track_cnt = 0;
! fmgr_info(mystats->eqfunc, &f_cmpeq);
for (i = 0; i < samplerows; i++)
{
--- 1807,1830 ----
Datum value;
int count;
} TrackItem;
! TrackItem *track = NULL;
! int track_cnt = 0,
! track_max = 0;
int num_mcv = stats->attr->attstattarget;
StdAnalyzeData *mystats = (StdAnalyzeData *) stats->extra_data;
! if (OidIsValid(mystats->eqfunc))
! {
! /*
! * We track up to 2*n values for an n-element MCV list; but at least 10
! */
! track_max = 2 * num_mcv;
! if (track_max < 10)
! track_max = 10;
! track = (TrackItem *) palloc(track_max * sizeof(TrackItem));
! fmgr_info(mystats->eqfunc, &f_cmpeq);
! }
for (i = 0; i < samplerows; i++)
{
*************** compute_minimal_stats(VacAttrStatsP stat
*** 1876,1926 ****
total_width += strlen(DatumGetCString(value)) + 1;
}
! /*
! * See if the value matches anything we're already tracking.
! */
! match = false;
! firstcount1 = track_cnt;
! for (j = 0; j < track_cnt; j++)
{
! /* We always use the default collation for statistics */
! if (DatumGetBool(FunctionCall2Coll(&f_cmpeq,
! DEFAULT_COLLATION_OID,
! value, track[j].value)))
{
! match = true;
! break;
}
- if (j < firstcount1 && track[j].count == 1)
- firstcount1 = j;
- }
! if (match)
! {
! /* Found a match */
! track[j].count++;
! /* This value may now need to "bubble up" in the track list */
! while (j > 0 && track[j].count > track[j - 1].count)
! {
! swapDatum(track[j].value, track[j - 1].value);
! swapInt(track[j].count, track[j - 1].count);
! j--;
! }
! }
! else
! {
! /* No match. Insert at head of count-1 list */
! if (track_cnt < track_max)
! track_cnt++;
! for (j = track_cnt - 1; j > firstcount1; j--)
{
! track[j].value = track[j - 1].value;
! track[j].count = track[j - 1].count;
}
! if (firstcount1 < track_cnt)
{
! track[firstcount1].value = value;
! track[firstcount1].count = 1;
}
}
}
--- 1876,1929 ----
total_width += strlen(DatumGetCString(value)) + 1;
}
! if (track)
{
! /*
! * See if the value matches anything we're already tracking.
! */
! match = false;
! firstcount1 = track_cnt;
! for (j = 0; j < track_cnt; j++)
{
! /* We always use the default collation for statistics */
! if (DatumGetBool(FunctionCall2Coll(&f_cmpeq,
! DEFAULT_COLLATION_OID,
! value, track[j].value)))
! {
! match = true;
! break;
! }
! if (j < firstcount1 && track[j].count == 1)
! firstcount1 = j;
}
! if (match)
{
! /* Found a match */
! track[j].count++;
! /* This value may now need to "bubble up" in the track list */
! while (j > 0 && track[j].count > track[j - 1].count)
! {
! swapDatum(track[j].value, track[j - 1].value);
! swapInt(track[j].count, track[j - 1].count);
! j--;
! }
}
! else
{
! /* No match. Insert at head of count-1 list */
! if (track_cnt < track_max)
! track_cnt++;
! for (j = track_cnt - 1; j > firstcount1; j--)
! {
! track[j].value = track[j - 1].value;
! track[j].count = track[j - 1].count;
! }
! if (firstcount1 < track_cnt)
! {
! track[firstcount1].value = value;
! track[firstcount1].count = 1;
! }
}
}
}
On 09/22/2015 12:16 PM, Shulgin, Oleksandr wrote:
Hi Hackers,
I've recently stumbled upon a problem with table bloat estimation in
case there are columns of type JSON.The quick bloat estimation queries use sum over pg_statistic.stawidth
of table's columns, but in case of JSON the corresponding entry is
never created by the ANALYZE command due to equality comparison
operator missing. I understand why there is no such operator defined
for this particular type, but shouldn't we still try to produce
meaningful average width estimation?In my case the actual bloat is around 40% as verified with
pgstattuple, while the bloat reported by quick estimate can be between
75% and 95%(!) in three instances of this problem. We're talking
about some hundreds of GB of miscalculation.Attached patch against master makes the std_typanalyze still try to
compute the minimal stats even if there is no "=" operator. Makes sense?I could also find this report in archives that talks about similar
problem, but due to all values being over the analyze threshold:/messages/by-id/12480.1389370514@sss.pgh.pa.us
I think we could try harder, otherwise any estimate relying on average
width can be way off in such cases.
Yes, "/revenons/ à /nos moutons/." You can set up text based comparison
ops fairly easily for json - you just need to be aware of the
limitations. See https://gist.github.com/adunstan/32ad224d7499d2603708
But I agree we should be able to do some analysis of types without
comparison ops.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sep 22, 2015 8:58 PM, "Andrew Dunstan" <andrew@dunslane.net> wrote:
On 09/22/2015 12:16 PM, Shulgin, Oleksandr wrote:
Hi Hackers,
I've recently stumbled upon a problem with table bloat estimation in
case there are columns of type JSON.
The quick bloat estimation queries use sum over pg_statistic.stawidth of
table's columns, but in case of JSON the corresponding entry is never
created by the ANALYZE command due to equality comparison operator
missing. I understand why there is no such operator defined for this
particular type, but shouldn't we still try to produce meaningful average
width estimation?
In my case the actual bloat is around 40% as verified with pgstattuple,
while the bloat reported by quick estimate can be between 75% and 95%(!) in
three instances of this problem. We're talking about some hundreds of GB
of miscalculation.
Attached patch against master makes the std_typanalyze still try to
compute the minimal stats even if there is no "=" operator. Makes sense?
I could also find this report in archives that talks about similar
problem, but due to all values being over the analyze threshold:
/messages/by-id/12480.1389370514@sss.pgh.pa.us
I think we could try harder, otherwise any estimate relying on average
width can be way off in such cases.
Yes, "/revenons/ à /nos moutons/." You can set up text based comparison
ops fairly easily for json - you just need to be aware of the limitations.
See https://gist.github.com/adunstan/32ad224d7499d2603708
Yes, I've already tried this approach and have found that analyze
performance degrades an order of magnitude due to sql-level function
overhead and casts to text. In my tests, from 200ms to 2000ms with btree
ops on a default sample of 30,000 rows.
Should have mentioned that.
There is a very hacky way to substitute bttextcmp for the sort support
function after defining the opclass by updating pg_amproc, buy I would
rather avoid that. :-)
--
Alex
Shulgin, Oleksandr wrote:
On Sep 22, 2015 8:58 PM, "Andrew Dunstan" <andrew@dunslane.net> wrote:
Yes, "/revenons/ � /nos moutons/." You can set up text based comparison
ops fairly easily for json - you just need to be aware of the limitations.
See https://gist.github.com/adunstan/32ad224d7499d2603708Yes, I've already tried this approach and have found that analyze
performance degrades an order of magnitude due to sql-level function
overhead and casts to text. In my tests, from 200ms to 2000ms with btree
ops on a default sample of 30,000 rows.
You should be able to create a C function json_cmp() that simply calls
bttextcmp() internally, and C functions for each operator using that
one, in the same way.
In any case I think your patch is a good starting point.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
In any case I think your patch is a good starting point.
The comments seemed to need some wordsmithing, but I think this is
probably basically a good idea; we've had similar complaints before
about some other equality-less datatypes, such as point.
Should we consider this HEAD-only, or a back-patchable bug fix?
Or perhaps compromise on HEAD + 9.5?
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
Tom Lane wrote:
Should we consider this HEAD-only, or a back-patchable bug fix?
Or perhaps compromise on HEAD + 9.5?
It looks like a bug to me, but I think it might destabilize approved
execution plans(*), so it may not be such a great idea to back patch
branches that are already released. I think HEAD + 9.5 is good.
(*) I hear there are even applications where queries and their approved
execution plans are kept in a manifest, and plans that deviate from that
raise all kinds of alarms. I have never seen such a thing ...
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 22, 2015 at 11:17 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:
Shulgin, Oleksandr wrote:
On Sep 22, 2015 8:58 PM, "Andrew Dunstan" <andrew@dunslane.net> wrote:
Yes, "/revenons/ à /nos moutons/." You can set up text based comparison
ops fairly easily for json - you just need to be aware of thelimitations.
Yes, I've already tried this approach and have found that analyze
performance degrades an order of magnitude due to sql-level function
overhead and casts to text. In my tests, from 200ms to 2000ms with btree
ops on a default sample of 30,000 rows.You should be able to create a C function json_cmp() that simply calls
bttextcmp() internally, and C functions for each operator using that
one, in the same way.
Yes, but I didn't try this because of the requirement to
compile/install/maintain the externally loadable module. If I could just
use CREATE FUNCTION on a postgres' internal function such as texteq or
bttextcmp (with obj_file of NULL, for example) I would definitely do that.
:-)
--
Alex
On Tue, Sep 22, 2015 at 11:56 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:
Tom Lane wrote:
Should we consider this HEAD-only, or a back-patchable bug fix?
Or perhaps compromise on HEAD + 9.5?It looks like a bug to me, but I think it might destabilize approved
execution plans(*), so it may not be such a great idea to back patch
branches that are already released. I think HEAD + 9.5 is good.(*) I hear there are even applications where queries and their approved
execution plans are kept in a manifest, and plans that deviate from that
raise all kinds of alarms. I have never seen such a thing ...
Ugh. Anyway, do you expect any plans to change only due to avg. width
estimation being different? Why would that be so?
--
Alex
On Tue, Sep 22, 2015 at 11:43 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
In any case I think your patch is a good starting point.
The comments seemed to need some wordsmithing, but I think this is
probably basically a good idea; we've had similar complaints before
about some other equality-less datatypes, such as point.Should we consider this HEAD-only, or a back-patchable bug fix?
Or perhaps compromise on HEAD + 9.5?
I failed to realize that the complaint I've referred to regarding all too
wide samples was addressed back then by this
commit: 6286526207d53e5b31968103adb89b4c9cd21499
For what it's worth, that time the decision was "This has been like this
since roughly neolithic times, so back-patch to all supported branches."
Does the same logic not apply here?
--
Alex
"Shulgin, Oleksandr" <oleksandr.shulgin@zalando.de> writes:
On Tue, Sep 22, 2015 at 11:56 PM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:It looks like a bug to me, but I think it might destabilize approved
execution plans(*), so it may not be such a great idea to back patch
branches that are already released. I think HEAD + 9.5 is good.(*) I hear there are even applications where queries and their approved
execution plans are kept in a manifest, and plans that deviate from that
raise all kinds of alarms. I have never seen such a thing ...
Ugh. Anyway, do you expect any plans to change only due to avg. width
estimation being different? Why would that be so?
Certainly, eg it could affect a decision about whether to use a hash join
or hash aggregation through changing the planner's estimate of the
required hashtable size. We wouldn't be bothering to track that data if
it didn't affect plans.
Personally I think Alvaro's position is unduly conservative: to the extent
that plans change it'd likely be for the better. But I'm not excited
enough to fight hard about it.
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
Tom Lane wrote:
Personally I think Alvaro's position is unduly conservative: to the extent
that plans change it'd likely be for the better. But I'm not excited
enough to fight hard about it.
I don't really care enough. We have received some complaints about
keeping plans stable, but maybe it's okay.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Sep 23, 2015 at 3:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
"Shulgin, Oleksandr" <oleksandr.shulgin@zalando.de> writes:
On Tue, Sep 22, 2015 at 11:56 PM, Alvaro Herrera <
alvherre@2ndquadrant.com>
wrote:
It looks like a bug to me, but I think it might destabilize approved
execution plans(*), so it may not be such a great idea to back patch
branches that are already released. I think HEAD + 9.5 is good.(*) I hear there are even applications where queries and their approved
execution plans are kept in a manifest, and plans that deviate from that
raise all kinds of alarms. I have never seen such a thing ...Ugh. Anyway, do you expect any plans to change only due to avg. width
estimation being different? Why would that be so?Certainly, eg it could affect a decision about whether to use a hash join
or hash aggregation through changing the planner's estimate of the
required hashtable size. We wouldn't be bothering to track that data if
it didn't affect plans.Personally I think Alvaro's position is unduly conservative: to the extent
that plans change it'd likely be for the better. But I'm not excited
enough to fight hard about it.
Yeah, I can see now, as I was studying the hash node code today intensively
for an unrelated reason.
I also believe that given that we are going to have more accurate stats,
the plan changes in this case hopefully are a good thing.
--
Alex
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Tom Lane wrote:
Personally I think Alvaro's position is unduly conservative: to the extent
that plans change it'd likely be for the better. But I'm not excited
enough to fight hard about it.
I don't really care enough. We have received some complaints about
keeping plans stable, but maybe it's okay.
The other side of the coin is that there haven't been so many requests for
changing this; more than just this one, but not a groundswell. So 9.5
only seems like a good compromise unless we get more votes for back-patch.
I reviewed the patch and concluded that it would be better to split
compute_minimal_stats into two functions instead of sprinkling it so
liberally with if's. So I did that and pushed it.
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
On Thu, Sep 24, 2015 at 12:30 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Tom Lane wrote:
Personally I think Alvaro's position is unduly conservative: to the
extent
that plans change it'd likely be for the better. But I'm not excited
enough to fight hard about it.I don't really care enough. We have received some complaints about
keeping plans stable, but maybe it's okay.The other side of the coin is that there haven't been so many requests for
changing this; more than just this one, but not a groundswell. So 9.5
only seems like a good compromise unless we get more votes for back-patch.I reviewed the patch and concluded that it would be better to split
compute_minimal_stats into two functions instead of sprinkling it so
liberally with if's. So I did that and pushed it.
Thanks, I was not really happy about all the checks because some of them
were rather implicit (e.g. num_mcv being 0 due to track being NULL, etc.).
Adding this as a separate function makes me feel safer.
--
Alex