Re: [HACKERS] Calculage avg. width when operator = is missing

2015-09-24 Thread Shulgin, Oleksandr 
On Thu, Sep 24, 2015 at 12:30 AM, Tom Lane  wrote:

> Alvaro Herrera  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

Re: [HACKERS] Calculage avg. width when operator = is missing

2015-09-23 Thread Tom Lane 
Alvaro Herrera  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

Re: [HACKERS] Calculage avg. width when operator = is missing

2015-09-23 Thread Shulgin, Oleksandr 
On Wed, Sep 23, 2015 at 3:21 PM, Tom Lane  wrote:

> "Shulgin, Oleksandr"  writes:
> > On Tue, Sep 22, 2015 at 11:56 PM, Alvaro Herrera <
> alvhe...@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

Re: [HACKERS] Calculage avg. width when operator = is missing

2015-09-23 Thread Alvaro Herrera 
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 Herrerahttp://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

Re: [HACKERS] Calculage avg. width when operator = is missing

2015-09-23 Thread Tom Lane 
"Shulgin, Oleksandr"  writes:
> On Tue, Sep 22, 2015 at 11:56 PM, Alvaro Herrera 
> 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

Re: [HACKERS] Calculage avg. width when operator = is missing

2015-09-23 Thread Shulgin, Oleksandr 
On Tue, Sep 22, 2015 at 11:43 PM, Tom Lane  wrote:

> Alvaro Herrera  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

Re: [HACKERS] Calculage avg. width when operator = is missing

2015-09-22 Thread Shulgin, Oleksandr 
On Tue, Sep 22, 2015 at 11:56 PM, Alvaro Herrera 
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

Re: [HACKERS] Calculage avg. width when operator = is missing

2015-09-22 Thread Shulgin, Oleksandr 
On Tue, Sep 22, 2015 at 11:17 PM, Alvaro Herrera 
wrote:

> Shulgin, Oleksandr wrote:
> > On Sep 22, 2015 8:58 PM, "Andrew Dunstan"  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/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.
>
> 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

Re: [HACKERS] Calculage avg. width when operator = is missing

2015-09-22 Thread Alvaro Herrera 
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 Herrerahttp://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

Re: [HACKERS] Calculage avg. width when operator = is missing

2015-09-22 Thread Tom Lane 
Alvaro Herrera  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

Re: [HACKERS] Calculage avg. width when operator = is missing

2015-09-22 Thread Alvaro Herrera 
Shulgin, Oleksandr wrote:
> On Sep 22, 2015 8:58 PM, "Andrew Dunstan"  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/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.

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 Herrerahttp://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

Re: [HACKERS] Calculage avg. width when operator = is missing

2015-09-22 Thread Shulgin, Oleksandr 
On Sep 22, 2015 8:58 PM, "Andrew Dunstan"  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:
>>
>>
http://www.postgresql.org/message-id/flat/12480.1389370...@sss.pgh.pa.us#12480.1389370...@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

Re: [HACKERS] Calculage avg. width when operator = is missing

2015-09-22 Thread Andrew Dunstan 



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:


http://www.postgresql.org/message-id/flat/12480.1389370...@sss.pgh.pa.us#12480.1389370...@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

[HACKERS] Calculage avg. width when operator = is missing

2015-09-22 Thread Shulgin, Oleksandr 
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:

http://www.postgresql.org/message-id/flat/12480.1389370...@sss.pgh.pa.us#12480.1389370...@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
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 
  			 
    
    
14 matches