Обсуждение: Abbreviated keys for Numeric (was: Re: B-Tree support function number 3 (strxfrm() optimization))
Another spinoff from the abbreviation discussion. Peter Geoghegan suggested on IRC that numeric would benefit from abbreviation, and indeed it does (in some cases by a factor of about 6-7x or more, because numeric comparison is no speed demon). This patch abbreviates numerics to a weight+initial digits representation (the details differ slightly between 32bit and 64bit builds to make the best use of the available bits). On 32bit, numeric values that are between about 10^-44 and 10^83, and which differ either in order of magnitude or in the leading 7 significant decimal digits (not base-10000 digits, single decimals) will get distinct abbreviations. On 64bit the range is 10^-176 to 10^332 and the first 4 base-10000 digits are kept, thus comparing 13 to 16 decimal digits. This is expected to be ample for applications using numeric to store numbers; applications that store things in numeric that aren't actually numbers might not see the benefit, but I have not found any detectable slowdown from the patch even on constructed pathological data. -- Andrew (irc:RhodiumToad)
Вложения
On Mon, Jan 26, 2015 at 8:43 AM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: > Another spinoff from the abbreviation discussion. Peter Geoghegan > suggested on IRC that numeric would benefit from abbreviation, and > indeed it does (in some cases by a factor of about 6-7x or more, because > numeric comparison is no speed demon). Cool. What I find particularly interesting about this patch is that it makes sorting numerics significantly faster than even sorting float8 values, at least some of the time, even though the latter has generic SortSupport (for fmgr elision). Example: postgres=# create table foo as select x::float8 x, x::numeric y from (select random() * 10000000 x from generate_series(1,1000000) a) b; SELECT 1000000 This query takes about 525ms after repeated executions: select * from (select * from foo order by x offset 1000000000) i; However, this query takes about 412ms: select * from (select * from foo order by y offset 1000000000) i; There is probably a good case to be made for float8 abbreviation support....just as well that your datum abbreviation patch doesn't imply that pass-by-value types cannot be abbreviated across the board (it only implies that abbreviation of pass-by-value types is not supported in the datum sort case). :-) Anyway, the second query above (the one with the numeric ORDER BY column) is enormously faster than the same query executed against master's tip. That takes about 1720ms following repeated executions. So at least that case is over 4x faster, suggesting that abbreviation support for numeric is well worthwhile. So I'm signed up to review this one too. -- Peter Geoghegan
>>>>> "Peter" == Peter Geoghegan <pg@heroku.com> writes: Peter> What I find particularly interesting about this patch is that itPeter> makes sorting numerics significantly fasterthan even sortingPeter> float8 values, I get a much smaller difference there than you do. Obvious overheads in float8 comparison include having to check for NaN, and the fact that DatumGetFloat8 on 64bit doesn't get inlined and forces a store/load to memory rather than just using a register. Looking at those might be more beneficial than messing with abbreviations. -- Andrew (irc:RhodiumToad)
On Mon, Jan 26, 2015 at 3:12 PM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: > Obvious overheads in float8 comparison include having to check for NaN, > and the fact that DatumGetFloat8 on 64bit doesn't get inlined and forces > a store/load to memory rather than just using a register. Looking at > those might be more beneficial than messing with abbreviations. Aren't there issues with the alignment of double precision floating point numbers on x86, too? Maybe my information there is at least partially obsolete. But it seems we'd have to control for this to be sure. I am not seriously suggesting pursuing abbreviation for float8 in the near term - numeric is clearly what we should concentrate on. It's interesting that abbreviation of float8 could potentially make sense, though. -- Peter Geoghegan
On 2015-01-26 15:35:44 -0800, Peter Geoghegan wrote: > On Mon, Jan 26, 2015 at 3:12 PM, Andrew Gierth > <andrew@tao11.riddles.org.uk> wrote: > > Obvious overheads in float8 comparison include having to check for NaN, > > and the fact that DatumGetFloat8 on 64bit doesn't get inlined and forces > > a store/load to memory rather than just using a register. Looking at > > those might be more beneficial than messing with abbreviations. > > Aren't there issues with the alignment of double precision floating > point numbers on x86, too? Maybe my information there is at least > partially obsolete. But it seems we'd have to control for this to be > sure. I think getting rid of the function call for DatumGetFloat8() would be quite the win. On x86-64 the conversion then should amount to mov %rd?,-0x8(%rsp);movsd -0x8(%rsp),%xmm0 - that's pretty cheap. Both instructions have a cycle count of 1 + L1 access latency (4) + 2 because they use the same exection port. So it's about 12 fully pipelineable cycles. 2 if the pipeline can kept busy otherwise. I doubt that'd be noticeable if the conversion were inlined. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
On 27/01/15 00:51, Andres Freund wrote: > On 2015-01-26 15:35:44 -0800, Peter Geoghegan wrote: >> On Mon, Jan 26, 2015 at 3:12 PM, Andrew Gierth >> <andrew@tao11.riddles.org.uk> wrote: >>> Obvious overheads in float8 comparison include having to check for NaN, >>> and the fact that DatumGetFloat8 on 64bit doesn't get inlined and forces >>> a store/load to memory rather than just using a register. Looking at >>> those might be more beneficial than messing with abbreviations. >> >> Aren't there issues with the alignment of double precision floating >> point numbers on x86, too? Maybe my information there is at least >> partially obsolete. But it seems we'd have to control for this to be >> sure. > > I think getting rid of the function call for DatumGetFloat8() would be > quite the win. On x86-64 the conversion then should amount to mov > %rd?,-0x8(%rsp);movsd -0x8(%rsp),%xmm0 - that's pretty cheap. Both > instructions have a cycle count of 1 + L1 access latency (4) + 2 because > they use the same exection port. So it's about 12 fully pipelineable > cycles. 2 if the pipeline can kept busy otherwise. I doubt that'd be > noticeable if the conversion were inlined. > IIRC the DatumGetFloat8 was quite visible in the perf when I was writing the array version of width_bucket. It was one of the motivations for making special float8 version since not having to call it had significant effect. Sadly I don't remember if it was the function call itself or the conversion anymore. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
>>>>> "Peter" == Peter Geoghegan <pg@heroku.com> writes: Peter> What I find particularly interesting about this patch is that itPeter> makes sorting numerics significantly fasterthan even sortingPeter> float8 values, Played some more with this. Testing on some different gcc versions showed that the results were not consistent between versions; the latest I tried (4.9) showed float8 as somewhat faster, while 4.7 showed float8 as slightly slower; the difference was all in the time of the float8 case, the time for numeric was virtually the same. For one specific test query, taking the best time of multiple runs, float8: gcc4.7 = 980ms, gcc4.9 = 833ms numeric: gcc4.7 = 940ms, gcc4.9 = 920ms (vs. 650ms for bigint on either version) So honestly I think abbreviation for float8 is a complete red herring. Also, I couldn't get any detectable benefit from inlining DatumGetFloat8, though I may have to play more with that to be certain (above tests did not have any float8-related modifications at all, just the datum and numeric abbrevs patches). -- Andrew (irc:RhodiumToad)
On 28/01/15 06:29, Andrew Gierth wrote: >>>>>> "Peter" == Peter Geoghegan <pg@heroku.com> writes: > Peter> What I find particularly interesting about this patch is that it > Peter> makes sorting numerics significantly faster than even sorting > Peter> float8 values, > > Played some more with this. Testing on some different gcc versions > showed that the results were not consistent between versions; the latest > I tried (4.9) showed float8 as somewhat faster, while 4.7 showed float8 > as slightly slower; the difference was all in the time of the float8 > case, the time for numeric was virtually the same. > > For one specific test query, taking the best time of multiple runs, > > float8: gcc4.7 = 980ms, gcc4.9 = 833ms > numeric: gcc4.7 = 940ms, gcc4.9 = 920ms > > (vs. 650ms for bigint on either version) > > So honestly I think abbreviation for float8 is a complete red herring. > > Also, I couldn't get any detectable benefit from inlining > DatumGetFloat8, though I may have to play more with that to be certain > (above tests did not have any float8-related modifications at all, just > the datum and numeric abbrevs patches). > Since gcc5.0 is due to be released in less than 3 months, it might be worth testing with that. Cheers, Gavin
On Mon, Jan 26, 2015 at 3:35 PM, Peter Geoghegan <pg@heroku.com> wrote: > I am not seriously suggesting pursuing abbreviation for float8 in the > near term - numeric is clearly what we should concentrate on. It's > interesting that abbreviation of float8 could potentially make sense, > though. Note that in the IEEE 754 standard, the exponent does not have a sign. Rather, an exponent bias is subtracted from it (127 for single precision floats, and 1023 for double precision floats). This, and the bit sequence of the mantissa allows floats to be compared and sorted correctly even when interpreting them as integers. The exception is NaN, but then we have an exception to that exception. This is a really old idea, actually. I first saw it in a paper written in the 1960s, long before math coprocessors became standard. Haven't really thrashed this out enough, but I offhand I guess it would work. The other problem is that positive IEEE floating-point numbers sort like integers with the same bits, and negative IEEE floating-point numbers sort in the reverse order of integers with the same bits. So we'd probably end up with an encoding scheme that accounted for that, and forget about tie-breakers (or have a NOOP "return 0" tie-breaker). An example of the problem: postgres=# create table foo (a float8); CREATE TABLE postgres=# insert into foo values (1), (2), (3), (-1), (-2), (-3); INSERT 0 6 postgres=# select * from foo order by a;a -----1-2-3 1 2 3 (6 rows) The reason that this conversion usually doesn't occur in library sorting routines is because it only helps significantly on x86, has additional memory overhead, and ordinarily requires that we convert back when we're done sorting. The costs/benefit analysis for tuplesort would be much more favorable than a generic float sorting case, given that we pretty much have datum1 storage as a sunk costs anyway, and given that we don't need to convert back the datum1 representation, and given that the encoding process would be dirt cheap and occur at a time when we were likely totally bottlenecked on memory bandwidth. I don't want to get bogged down on this - the numeric abbreviation patch *is* still much more compelling - but maybe abbreviation of float8 isn't a red herring after all. -- Peter Geoghegan
On Sat, Jan 31, 2015 at 7:07 PM, Peter Geoghegan <pg@heroku.com> wrote: > I don't want to get bogged down on this - the numeric abbreviation > patch *is* still much more compelling - but maybe abbreviation of > float8 isn't a red herring after all. I'm completely on-board with doing something about numeric. I think it might be pretty foolish to try to do anything about any data type the CPU has hard-wired knowledge of. We're basically betting that we can do better in software than they did in hardware, and even if that happens to be true on some systems under some circumstances, it leaves us in a poor position to leverage future improvements to the silicon. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi, On 26.1.2015 17:43, Andrew Gierth wrote: > Another spinoff from the abbreviation discussion. Peter Geoghegan > suggested on IRC that numeric would benefit from abbreviation, and > indeed it does (in some cases by a factor of about 6-7x or more, because > numeric comparison is no speed demon). > > This patch abbreviates numerics to a weight+initial digits > representation (the details differ slightly between 32bit and 64bit > builds to make the best use of the available bits). > > On 32bit, numeric values that are between about 10^-44 and 10^83, and > which differ either in order of magnitude or in the leading 7 > significant decimal digits (not base-10000 digits, single decimals) will > get distinct abbreviations. On 64bit the range is 10^-176 to 10^332 and > the first 4 base-10000 digits are kept, thus comparing 13 to 16 decimal > digits. This is expected to be ample for applications using numeric to > store numbers; applications that store things in numeric that aren't > actually numbers might not see the benefit, but I have not found any > detectable slowdown from the patch even on constructed pathological > data. I've done some testing on this (along with the other patch doing the same with Datum values), but I'm yet to see a query that actually benefits from this. For example with the same percentile_disc() test as in the other thread: create table stuff as select random()::numeric as randnum from generate_series(1,1000000); analyze stuff; select percentile_disc(0) within group (order by randnum) from stuff; I get pretty much no difference in runtimes (not even for the smallest dataset, where the Datum patch speedup was significant). What am I doing wrong? -- Tomas Vondra http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Feb 20, 2015 at 1:33 PM, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: > For example with the same percentile_disc() test as in the other thread: > > create table stuff as select random()::numeric as randnum > from generate_series(1,1000000); > > analyze stuff; > > select percentile_disc(0) within group (order by randnum) from stuff; > > > I get pretty much no difference in runtimes (not even for the smallest > dataset, where the Datum patch speedup was significant). > > What am I doing wrong? So you're testing both the patches (numeric + datum tuplesort) at the same time? I can't think why this would make any difference. Did you forget to initdb, so that the numeric sortsupport routine was used? -- Peter Geoghegan
On 21.2.2015 00:14, Peter Geoghegan wrote:
> On Fri, Feb 20, 2015 at 1:33 PM, Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
>> For example with the same percentile_disc() test as in the other
>> thread:
>>
>> create table stuff as select random()::numeric as randnum from
>> generate_series(1,1000000);
>>
>> analyze stuff;
>>
>> select percentile_disc(0) within group (order by randnum) from
>> stuff;
>>
>>
>> I get pretty much no difference in runtimes (not even for the
>> smallest dataset, where the Datum patch speedup was significant).
>>
>> What am I doing wrong?
>
> So you're testing both the patches (numeric + datum tuplesort) at the
> same time?
No, I was just testing two similar patches separately. I.e. master vs.
each patch separately.
> I can't think why this would make any difference. Did you forget to
> initdb, so that the numeric sortsupport routine was used?
No, but just to be sure I repeated the benchmarks and I still get the
same results. Each test run does this:
1) remove data directory
2) initdb
3) copy postgresql.conf (with minor tweaks - work_mem/shared_buffers)
4) start
5) create database
6) create test table
7) run a query 5x
I repeated this, just to be sure, but nope - still no speedup :-(
For master vs. patch, I do get these results:
master patched speedup
---------------------------------------------------------
generate_series(1,1000000) 1.20 1.25 0.96
generate_series(1,2000000) 2.75 2.75 1.00
generate_series(1,3000000) 4.40 4.40 1.00
So, no difference :(
Scripts attached, but it's really trivial test - hopefully I haven't
done anything dumb.
--
Tomas Vondra http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On Fri, Feb 20, 2015 at 4:11 PM, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: >> So you're testing both the patches (numeric + datum tuplesort) at the >> same time? > > No, I was just testing two similar patches separately. I.e. master vs. > each patch separately. Well, you're sorting numeric here, no? Why should it matter that a datum sort has abbreviation support, if the underlying type (numeric) does not support abbreviation? OTOH, why should having oplcass abbreviation support (for numeric) matter if the class of tuple sorted (datum "tuples") does not support abbreviation? You need both to meaningfully benchmark either (as long as you're looking at a case involving both). I suggest looking at datum sorts with text for the datum sort patch, and non-datum tuplesort cases for the numeric patch, at least until such time as one or the other is committed. -- Peter Geoghegan
On 21.2.2015 01:17, Peter Geoghegan wrote: > On Fri, Feb 20, 2015 at 4:11 PM, Tomas Vondra > <tomas.vondra@2ndquadrant.com> wrote: >>> So you're testing both the patches (numeric + datum tuplesort) at the >>> same time? >> >> No, I was just testing two similar patches separately. I.e. master vs. >> each patch separately. > > Well, you're sorting numeric here, no? Why should it matter that a > datum sort has abbreviation support, if the underlying type (numeric) > does not support abbreviation? OTOH, why should having oplcass > abbreviation support (for numeric) matter if the class of tuple sorted > (datum "tuples") does not support abbreviation? You need both to > meaningfully benchmark either (as long as you're looking at a case > involving both). > > I suggest looking at datum sorts with text for the datum sort patch, > and non-datum tuplesort cases for the numeric patch, at least until > such time as one or the other is committed. Isn't this patch about adding abbreviated keys for Numeric data type? That's how I understood it, and looking into numeric_sortsup.patch seems to confirm that. There's another patch for Datum, but that's a different thread. -- Tomas Vondra http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Feb 20, 2015 at 4:42 PM, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: > Isn't this patch about adding abbreviated keys for Numeric data type? > That's how I understood it, and looking into numeric_sortsup.patch seems > to confirm that. > > There's another patch for Datum, but that's a different thread. Right...so don't test a datum sort case, since that isn't supported at all in the master branch. Your test case is invalid for that reason. -- Peter Geoghegan
On 21.2.2015 01:45, Peter Geoghegan wrote: > On Fri, Feb 20, 2015 at 4:42 PM, Tomas Vondra > <tomas.vondra@2ndquadrant.com> wrote: >> Isn't this patch about adding abbreviated keys for Numeric data type? >> That's how I understood it, and looking into numeric_sortsup.patch seems >> to confirm that. >> >> There's another patch for Datum, but that's a different thread. > > Right...so don't test a datum sort case, since that isn't supported at > all in the master branch. Your test case is invalid for that reason. What do you mean by 'Datum sort case'? The test I was using is this: create table stuff as select (random())::numeric as randnum from generate_series(1,1000000); select percentile_disc(0) within group (order by randnum) from stuff; That's a table with a Numeric column, and a sort on that Numeric, no? -- Tomas Vondra http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>>>>> "Tomas" == Tomas Vondra <tomas.vondra@2ndquadrant.com> writes: >> Right...so don't test a datum sort case, since that isn't supported>> at all in the master branch. Your test case is invalidfor that>> reason. Tomas> What do you mean by 'Datum sort case'? A case where the code path goes via tuplesort_begin_datum rather than tuplesort_begin_heap. Tomas> The test I was using is this: Tomas> select percentile_disc(0) within group (order by randnum) from stuff; Sorting single columns in aggregate calls uses the Datum sort path (in fact I think it's currently the only place that does). Do that test with _both_ the Datum and Numeric sort patches in place, and you will see the effect. With only the Numeric patch, the numeric abbrev code is not called. If you want a test that works without the Datum patch, try: select count(*) from (select randnum from stuff order by randnum) s; -- Andrew (irc:RhodiumToad)
On 21.2.2015 02:00, Andrew Gierth wrote: >>>>>> "Tomas" == Tomas Vondra <tomas.vondra@2ndquadrant.com> writes: > > >> Right...so don't test a datum sort case, since that isn't supported > >> at all in the master branch. Your test case is invalid for that > >> reason. > > Tomas> What do you mean by 'Datum sort case'? > > A case where the code path goes via tuplesort_begin_datum rather than > tuplesort_begin_heap. > > Tomas> The test I was using is this: > > Tomas> select percentile_disc(0) within group (order by randnum) from stuff; > > Sorting single columns in aggregate calls uses the Datum sort path (in > fact I think it's currently the only place that does). > > Do that test with _both_ the Datum and Numeric sort patches in place, > and you will see the effect. With only the Numeric patch, the numeric > abbrev code is not called. D'oh! Thanks for the explanation. -- Tomas Vondra http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi,
On 21.2.2015 02:06, Tomas Vondra wrote:
> On 21.2.2015 02:00, Andrew Gierth wrote:
>>>>>>> "Tomas" == Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
>>
>> >> Right...so don't test a datum sort case, since that isn't supported
>> >> at all in the master branch. Your test case is invalid for that
>> >> reason.
>>
>> Tomas> What do you mean by 'Datum sort case'?
>>
>> A case where the code path goes via tuplesort_begin_datum rather than
>> tuplesort_begin_heap.
>>
>> Tomas> The test I was using is this:
>>
>> Tomas> select percentile_disc(0) within group (order by randnum) from stuff;
>>
>> Sorting single columns in aggregate calls uses the Datum sort path (in
>> fact I think it's currently the only place that does).
>>
>> Do that test with _both_ the Datum and Numeric sort patches in place,
>> and you will see the effect. With only the Numeric patch, the numeric
>> abbrev code is not called.
>
> D'oh! Thanks for the explanation.
OK, so I've repeated the benchmarks with both patches applied, and I
think the results are interesting. I extended the benchmark a bit - see
the SQL script attached.
1) multiple queries
select percentile_disc(0) within group (order by val) from stuff
select count(distinct val) from stuff
select * from
(select * from stuff order by val offset 100000000000) foo
2) multiple data types - int, float, text and numeric
3) multiple scales - 1M, 2M, 3M, 4M and 5M rows
Each query was executed 10x, the timings were averaged. I do know some
of the data types don't benefit from the patches, but I included them to
get a sense of how noisy the results are.
I did the measurements for
1) master
2) master + datum_sort_abbrev.patch
3) master + datum_sort_abbrev.patch + numeric_sortsup.patch
and then computed the speedup for each type/scale combination (the
impact on all the queries is almost exactly the same).
Complete results are available here: http://bit.ly/1EA4mR9
I'll post all the summary here, although some of the numbers are about
the other abbreviated keys patch.
1) datum_sort_abbrev.patch vs. master
scale float int numeric text
---------------------------------------------
1 101% 99% 105% 404%
2 101% 98% 96% 98%
3 101% 101% 99% 97%
4 100% 101% 98% 95%
5 99% 98% 93% 95%
2) numeric_sortsup.patch vs. master
scale float int numeric text
---------------------------------------------
1 97% 98% 374% 396%
2 100% 101% 407% 96%
3 99% 102% 407% 95%
4 99% 101% 423% 92%
5 95% 99% 411% 92%
I think the gains are pretty awesome - I mean, 400% speedup for Numeric
accross the board? Yes please!
The gains for text are also very nice, although in this case that only
happens for the smallest scale (1M rows), and for larger scales it's
actually slower than current master :-(
It's not just rainbows and unicorns, though. With both patches applied,
text sorts get even slower (up to ~8% slower than master), It also seems
to impact float (which gets ~5% slower, for some reason), but I don't
see how that could happen ... but I suspect this might be noise.
I'll repeat the tests on another machine after the weekend, and post an
update whether the results are the same or significantly different.
regards
--
Tomas Vondra http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 21/02/15 18:18, Tomas Vondra wrote: > Hi, > > On 21.2.2015 02:06, Tomas Vondra wrote: >> On 21.2.2015 02:00, Andrew Gierth wrote: >>>>>>>> "Tomas" == Tomas Vondra <tomas.vondra@2ndquadrant.com> writes: >>> >> Right...so don't test a datum sort case, since that isn't supported >>> >> at all in the master branch. Your test case is invalid for that >>> >> reason. >>> >>> Tomas> What do you mean by 'Datum sort case'? >>> >>> A case where the code path goes via tuplesort_begin_datum rather than >>> tuplesort_begin_heap. >>> >>> Tomas> The test I was using is this: >>> >>> Tomas> select percentile_disc(0) within group (order by randnum) from stuff; >>> >>> Sorting single columns in aggregate calls uses the Datum sort path (in >>> fact I think it's currently the only place that does). >>> >>> Do that test with _both_ the Datum and Numeric sort patches in place, >>> and you will see the effect. With only the Numeric patch, the numeric >>> abbrev code is not called. >> D'oh! Thanks for the explanation. > OK, so I've repeated the benchmarks with both patches applied, and I > think the results are interesting. I extended the benchmark a bit - see > the SQL script attached. > > 1) multiple queries > > select percentile_disc(0) within group (order by val) from stuff > > select count(distinct val) from stuff > > select * from > (select * from stuff order by val offset 100000000000) foo > > 2) multiple data types - int, float, text and numeric > > 3) multiple scales - 1M, 2M, 3M, 4M and 5M rows > > Each query was executed 10x, the timings were averaged. I do know some > of the data types don't benefit from the patches, but I included them to > get a sense of how noisy the results are. > > I did the measurements for > > 1) master > 2) master + datum_sort_abbrev.patch > 3) master + datum_sort_abbrev.patch + numeric_sortsup.patch > > and then computed the speedup for each type/scale combination (the > impact on all the queries is almost exactly the same). > > Complete results are available here: http://bit.ly/1EA4mR9 > > I'll post all the summary here, although some of the numbers are about > the other abbreviated keys patch. > > > 1) datum_sort_abbrev.patch vs. master > > scale float int numeric text > --------------------------------------------- > 1 101% 99% 105% 404% > 2 101% 98% 96% 98% > 3 101% 101% 99% 97% > 4 100% 101% 98% 95% > 5 99% 98% 93% 95% > > 2) numeric_sortsup.patch vs. master > > scale float int numeric text > --------------------------------------------- > 1 97% 98% 374% 396% > 2 100% 101% 407% 96% > 3 99% 102% 407% 95% > 4 99% 101% 423% 92% > 5 95% 99% 411% 92% > > > I think the gains are pretty awesome - I mean, 400% speedup for Numeric > accross the board? Yes please! > > The gains for text are also very nice, although in this case that only > happens for the smallest scale (1M rows), and for larger scales it's > actually slower than current master :-( > > It's not just rainbows and unicorns, though. With both patches applied, > text sorts get even slower (up to ~8% slower than master), It also seems > to impact float (which gets ~5% slower, for some reason), but I don't > see how that could happen ... but I suspect this might be noise. > > I'll repeat the tests on another machine after the weekend, and post an > update whether the results are the same or significantly different. > > regards > > > What are the standard deviations? Do the arithmetic means change much if you exclude the 2 fastest & 2 slowest? How do the arithmetic means compare to their respective medians? Essentially, how consistent are the results, or how great is the noise? There may be better indicators than the ones I've suggested above. Cheers, Gavin
Hi Gavin, On 21.2.2015 06:35, Gavin Flower wrote: > On 21/02/15 18:18, Tomas Vondra wrote: >> >> OK, so I've repeated the benchmarks with both patches applied, and I >> think the results are interesting. I extended the benchmark a bit - see >> the SQL script attached. >> >> 1) multiple queries >> >> select percentile_disc(0) within group (order by val) from stuff >> >> select count(distinct val) from stuff >> >> select * from >> (select * from stuff order by val offset 100000000000) foo >> >> 2) multiple data types - int, float, text and numeric >> >> 3) multiple scales - 1M, 2M, 3M, 4M and 5M rows >> >> Each query was executed 10x, the timings were averaged. I do know some >> of the data types don't benefit from the patches, but I included them to >> get a sense of how noisy the results are. >> >> I did the measurements for >> >> 1) master >> 2) master + datum_sort_abbrev.patch >> 3) master + datum_sort_abbrev.patch + numeric_sortsup.patch >> >> and then computed the speedup for each type/scale combination (the >> impact on all the queries is almost exactly the same). >> >> Complete results are available here: http://bit.ly/1EA4mR9 >> >> I'll post all the summary here, although some of the numbers are about >> the other abbreviated keys patch. >> >> >> 1) datum_sort_abbrev.patch vs. master >> >> scale float int numeric text >> --------------------------------------------- >> 1 101% 99% 105% 404% >> 2 101% 98% 96% 98% >> 3 101% 101% 99% 97% >> 4 100% 101% 98% 95% >> 5 99% 98% 93% 95% >> >> 2) numeric_sortsup.patch vs. master >> >> scale float int numeric text >> --------------------------------------------- >> 1 97% 98% 374% 396% >> 2 100% 101% 407% 96% >> 3 99% 102% 407% 95% >> 4 99% 101% 423% 92% >> 5 95% 99% 411% 92% >> >> >> I think the gains are pretty awesome - I mean, 400% speedup for Numeric >> accross the board? Yes please! >> >> The gains for text are also very nice, although in this case that only >> happens for the smallest scale (1M rows), and for larger scales it's >> actually slower than current master :-( >> >> It's not just rainbows and unicorns, though. With both patches applied, >> text sorts get even slower (up to ~8% slower than master), It also seems >> to impact float (which gets ~5% slower, for some reason), but I don't >> see how that could happen ... but I suspect this might be noise. >> >> I'll repeat the tests on another machine after the weekend, and post an >> update whether the results are the same or significantly different. >> >> regards >> > > What are the standard deviations? I have checked that the results are consistent before sending the results to the list, but I didn't want tu dump a huge pile of stddev/min/max/... numbers into that e-mail. So I just mentioned that the results are available in a spreadsheet (http://bit.ly/1EA4mR9). See for example the "details" sheet with all the numbers aggregated. That's where the numbers for the answers below come from. I'll provide references to the columns. For all three cases (master, datum, datum+numeric) STDDEV numbers are ~1-2% of the average. See columns T-V in the table. > > Do the arithmetic means change much if you exclude the 2 fastest & 2 > slowest? No. The change (compared to plain average) is ~1-2% of the value. See columns W-Y in the table. > > How do the arithmetic means compare to their respective medians? I have not included medians into the current table (will do for the next run), but all the other metrics seem quite consistent. > Essentially, how consistent are the results, or how great is the > noise? There may be better indicators than the ones I've suggested > above. I believe the results are very consistent (you may check the raw data in the spreadsheet), but let me check after repeating this. I'll also run the same test suite on another machine (I wonder how a different CPU will perform). regards -- Tomas Vondra http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Feb 20, 2015 at 9:18 PM, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: > The gains for text are also very nice, although in this case that only > happens for the smallest scale (1M rows), and for larger scales it's > actually slower than current master :-( That's odd. I have a hard time thinking of why the datum sort patch could be at fault, though. I bet the cost model of the text sortsupport routine is somehow hitting a snag on those larger sized sets. They should be just as accelerated, and probably more so, than your 1M sized set that was sped up 4x here. Can you see what is output with debugging of text abbreviation turned on? Put "#define DEBUG_ABBREV_KEYS" at the top of varlena.c and rebuild. Report on the debug1 output, and see if and when abbreviation is aborted. I suspected that the cost model was too conservative (or, more lightly, just too simplistic). I ought to revisit my patch to give the ad-hoc cost model a sense of proportion about how far along we are, which was previously deferred [1]. When there is a strong physical/logical correlation, that can be essential. Did you first index the text field, and then run CLUSTER for the larger sized sets on that index (to test abbreviation)? That would cause there to be a lot of abbreviated keys that seemed to poorly capture the entropy of their underlying values, when in fact that was entirely down to our only considering the first 10 tuples in a 100 million tuple set. Having some patience is important there, and a hint at how far in we are gives the ad-hoc cost model a much better sense of proportion...it then has a sense of how patient it should be. [1] http://www.postgresql.org/message-id/CA+TgmoaSXpD73cOj-vSFRfk0nmxjAN6WOQ_Hd9SkmZbOTi+6CQ@mail.gmail.com -- Peter Geoghegan
On Sat, Feb 21, 2015 at 10:57 AM, Peter Geoghegan <pg@heroku.com> wrote: > On Fri, Feb 20, 2015 at 9:18 PM, Tomas Vondra > <tomas.vondra@2ndquadrant.com> wrote: >> The gains for text are also very nice, although in this case that only >> happens for the smallest scale (1M rows), and for larger scales it's >> actually slower than current master :-( > > That's odd. I have a hard time thinking of why the datum sort patch > could be at fault, though. Oh, wait. For queries like this, which I now see in your spreadsheet: select * from (select * from stuff_text order by randtxt offset 100000000000) foo There is no reason to think that either patch will improve things over master branch tip's performance. This doesn't use a datum tuplesort. So that explains it, I think. Although I cannot easily explain the disparity in performance between 1M and 5M sized sets for this query: select count(distinct randtxt) from stuff_text You did make sure that the queries didn't spill to disk, right? Or that they did so consistently, at least. There is also no reason to think that integer or float datum sorts will be accelerated, because they could always use sortsupport - the datum sort patch is only about making it also possible to also use abbreviation for opclasses that support it, like text (and, eventually, numeric). -- Peter Geoghegan
On 21.2.2015 19:57, Peter Geoghegan wrote: > On Fri, Feb 20, 2015 at 9:18 PM, Tomas Vondra > <tomas.vondra@2ndquadrant.com> wrote: >> The gains for text are also very nice, although in this case that only >> happens for the smallest scale (1M rows), and for larger scales it's >> actually slower than current master :-( > > That's odd. I have a hard time thinking of why the datum sort patch > could be at fault, though. I bet the cost model of the text > sortsupport routine is somehow hitting a snag on those larger sized > sets. They should be just as accelerated, and probably more so, than > your 1M sized set that was sped up 4x here. > > Can you see what is output with debugging of text abbreviation turned > on? Put "#define DEBUG_ABBREV_KEYS" at the top of varlena.c and > rebuild. Report on the debug1 output, and see if and when abbreviation > is aborted. > > I suspected that the cost model was too conservative (or, more > lightly, just too simplistic). I ought to revisit my patch to give the > ad-hoc cost model a sense of proportion about how far along we are, > which was previously deferred [1]. When there is a strong > physical/logical correlation, that can be essential. I don't think there's a correlation - at least not in the usual sense that the data are stored 'sorted' by the column. The tables were generated as random. Hmmm, maybe we should add such correlated cases into the test script, to see how that behaves with those patches ... > > Did you first index the text field, and then run CLUSTER for the > larger sized sets on that index (to test abbreviation)? That would > cause there to be a lot of abbreviated keys that seemed to poorly > capture the entropy of their underlying values, when in fact that was > entirely down to our only considering the first 10 tuples in a 100 > million tuple set. Having some patience is important there, and a hint > at how far in we are gives the ad-hoc cost model a much better sense > of proportion...it then has a sense of how patient it should be. There are no indexes in the test script. -- Tomas Vondra http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 21.2.2015 20:33, Peter Geoghegan wrote: > On Sat, Feb 21, 2015 at 10:57 AM, Peter Geoghegan <pg@heroku.com> > >> That's odd. I have a hard time thinking of why the datum sort >> patch could be at fault, though. > > Oh, wait. For queries like this, which I now see in your > spreadsheet: > > select * from (select * from stuff_text order by randtxt offset > 100000000000) foo > > There is no reason to think that either patch will improve things > over master branch tip's performance. This doesn't use a datum > tuplesort. So that explains it, I think. Really? Because those are the queries that you posted on 26/1 to demonstrate that this patch makes sorting Numeric even faster than sorting float8. And for the Numeric data type this actually gets significant speedup with the numeric_sortsupp.patch (~4x). But maybe for text that works differently? > Although I cannot easily explain the disparity in performance between > 1M and 5M sized sets for this query: > > select count(distinct randtxt) from stuff_text > > You did make sure that the queries didn't spill to disk, right? Or > that they did so consistently, at least. All the queries were running with work_mem=1GB, and I don't think they were spilling to disk. Actually, I don't have the 'merge' patch applied, so that would probably crash because of SIGSEGV. > There is also no reason to think that integer or float datum sorts > will be accelerated, because they could always use sortsupport - the > datum sort patch is only about making it also possible to also use > abbreviation for opclasses that support it, like text (and, > eventually, numeric). Yes, I'm aware of that. I used that as a control group, to get and idea of how noisy the results are, and maybe check if the patches don't affect it for some unexpected reason. -- Tomas Vondra http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Feb 21, 2015 at 12:15 PM, Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote: >> Although I cannot easily explain the disparity in performance between >> 1M and 5M sized sets for this query: >> >> select count(distinct randtxt) from stuff_text >> >> You did make sure that the queries didn't spill to disk, right? Or >> that they did so consistently, at least. > > All the queries were running with work_mem=1GB, and I don't think they > were spilling to disk. Actually, I don't have the 'merge' patch applied, > so that would probably crash because of SIGSEGV. Actually, that isn't so -- Andrew's datum sort patch incidentally fixes tuplesort_begin_datum() in the same way as the patch I posted. You shouldn't actually need my patch, which was just an immediate fix. I can recreate the problem you see with text sort regressions. Abbreviation is aborted for the case in question, unsurprisingly, and fairly far in. With that many tuples, the idea of taking abbreviated cardinality as a proxy for full cardinality becomes less important, because either way you have to do at least 10 comparisons per item on average. Originally, I had as a heuristic that once you get to a million items without aborting, stop considering the possibility of aborting - that much should probably be added back, at a minimum. Andrew is already doing that in his numeric patch (at 100,000 tuples), and that may be the only reason why numeric is not regressed too. For a query like this, if I "#define DEBUG_ABBREV_KEYS", so that we don't actually abort, and run this query of yours until it stabilizes: select * from (select * from stuff_text order by randtxt offset 100000000000) foo; It takes about 5.3 seconds. If I don't, however - if it is allowed to run its course - it takes a whopping 35 seconds. So the ad-hoc cost model of the text opclass definitely needs more work, as I suspected. -- Peter Geoghegan
On Sat, Feb 21, 2015 at 2:09 PM, Peter Geoghegan <pg@heroku.com> wrote: > I can recreate the problem you see with text sort regressions. > Abbreviation is aborted for the case in question, unsurprisingly, and > fairly far in. With that many tuples, the idea of taking abbreviated > cardinality as a proxy for full cardinality becomes less important, > because either way you have to do at least 10 comparisons per item on > average. Actually, it's closer to 20 comparisons past 1 million, on average. See my earlier 0002-* patch comments [1]. [1] http://www.postgresql.org/message-id/attachment/35861/0002-Estimate-total-number-of-rows-to-be-sorted.patch (search for "20") -- Peter Geoghegan
On 21.2.2015 23:09, Peter Geoghegan wrote: > On Sat, Feb 21, 2015 at 12:15 PM, Tomas Vondra > <tomas.vondra@2ndquadrant.com> wrote: >>> Although I cannot easily explain the disparity in performance between >>> 1M and 5M sized sets for this query: >>> >>> select count(distinct randtxt) from stuff_text >>> >>> You did make sure that the queries didn't spill to disk, right? Or >>> that they did so consistently, at least. >> >> All the queries were running with work_mem=1GB, and I don't think they >> were spilling to disk. Actually, I don't have the 'merge' patch applied, >> so that would probably crash because of SIGSEGV. > > Actually, that isn't so -- Andrew's datum sort patch incidentally > fixes tuplesort_begin_datum() in the same way as the patch I posted. > You shouldn't actually need my patch, which was just an immediate fix. Oh, I see - it was failing only because I applied the numeric patch without the datum one. Anyway, I'm logging using log_temp_files=0 and I see no temp files in the log file, so it's all in-memory sorts. > I can recreate the problem you see with text sort regressions. > Abbreviation is aborted for the case in question, unsurprisingly, > and fairly far in. With that many tuples, the idea of taking > abbreviated cardinality as a proxy for full cardinality becomes less > important, because either way you have to do at least 10 comparisons > per item on average. Originally, I had as a heuristic that once you > get to a million items without aborting, stop considering the > possibility of aborting - that much should probably be added back, at > a minimum. Andrew is already doing that in his numeric patch (at > 100,000 tuples), and that may be the only reason why numeric is not > regressed too. OK. I'm not going to pretend I fully understand what's going on here. I haven't paid that close attention to the original strxfrm() patch, but if I got it right there's some heuristics that needs tuning. > For a query like this, if I "#define DEBUG_ABBREV_KEYS", so that we > don't actually abort, and run this query of yours until it stabilizes: > > select * from (select * from stuff_text order by randtxt offset > 100000000000) foo; > > It takes about 5.3 seconds. If I don't, however - if it is allowed > to run its course - it takes a whopping 35 seconds. So the ad-hoc > cost model of the text opclass definitely needs more work, as I > suspected. That tuning should probably happen before this patch gets in, right? I'm not very comfortable with first committing this, and then fixing the regression as that might miss the 9.5. -- Tomas Vondra http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>>>>> "Gavin" == Gavin Flower <GavinFlower@archidevsys.co.nz> writes: Gavin> What are the standard deviations? Gavin> Do the arithmetic means change much if you exclude the 2 fastestGavin> & 2 slowest? Gavin> How do the arithmetic means compare to their respective medians? Gavin> Essentially, how consistent are the results, or how great is theGavin> noise? There may be better indicators thanthe ones I'veGavin> suggested above. This is all rather missing the point. The relevant metric is not how much noise is introduced between runs of the same code, but rather how much noise is introduced as a result of non-consequential changes to the code. I can get variations of several percent - easily more than three sigmas of the timing of repeated runs of unchanged code - in the time taken to sort a float8 column simply from introducing varying amounts of padding into the body of a function which is never called in the test. Clearly, the only possible effect here is that the changed memory addresses of functions must be resulting in different patterns of cache misses / cache replacements, or TLB misses, or similar low-level effects which have nothing to do with the code as such. (That this is a low-level alignment effect is supported by the fact that the performance changes are not monotonic in the size of the padding; adding more padding may cause either speedups or slowdowns.) -- Andrew (irc:RhodiumToad)
On 22/02/15 22:59, Andrew Gierth wrote: >>>>>> "Gavin" == Gavin Flower <GavinFlower@archidevsys.co.nz> writes: > Gavin> What are the standard deviations? > > Gavin> Do the arithmetic means change much if you exclude the 2 fastest > Gavin> & 2 slowest? > > Gavin> How do the arithmetic means compare to their respective medians? > > Gavin> Essentially, how consistent are the results, or how great is the > Gavin> noise? There may be better indicators than the ones I've > Gavin> suggested above. > > This is all rather missing the point. > > The relevant metric is not how much noise is introduced between runs of > the same code, but rather how much noise is introduced as a result of > non-consequential changes to the code. > > I can get variations of several percent - easily more than three sigmas > of the timing of repeated runs of unchanged code - in the time taken to > sort a float8 column simply from introducing varying amounts of padding > into the body of a function which is never called in the test. Clearly, > the only possible effect here is that the changed memory addresses of > functions must be resulting in different patterns of cache misses / > cache replacements, or TLB misses, or similar low-level effects which > have nothing to do with the code as such. > > (That this is a low-level alignment effect is supported by the fact that > the performance changes are not monotonic in the size of the padding; > adding more padding may cause either speedups or slowdowns.) > What is you have pointed out is 'obvious', in retrospect - but still, far more extreme than I would have ever expected! There well may be analogues of the story where a DP manager was convinced to get faster hard drives, 50% faster than the old ones, to speed up an overnight batch run. The program then took about 50% longer to run. Turns out that previously the mainframe processed one block in a little less time than the old hard drive took to rotate once. So the new hard drives ended up doing two rotations per block read, so took longer, despite being faster! (A modern low end Smart Phone, probably has at least a thousand times more processing power than that poor old Mainframe!) Cheers, Gavin
On 22.2.2015 10:59, Andrew Gierth wrote: >>>>>> "Gavin" == Gavin Flower <GavinFlower@archidevsys.co.nz> writes: > > Gavin> Essentially, how consistent are the results, or how great is the > Gavin> noise? There may be better indicators than the ones I've > Gavin> suggested above. > > This is all rather missing the point. > > The relevant metric is not how much noise is introduced between runs > of the same code, but rather how much noise is introduced as a result > of non-consequential changes to the code. > > I can get variations of several percent - easily more than three > sigmas of the timing of repeated runs of unchanged code - in the time > taken to sort a float8 column simply from introducing varying amounts > of padding into the body of a function which is never called in the > test. Clearly, the only possible effect here is that the changed > memory addresses of functions must be resulting in different patterns > of cache misses / cache replacements, or TLB misses, or similar > low-level effects which have nothing to do with the code as such. > > (That this is a low-level alignment effect is supported by the fact > that the performance changes are not monotonic in the size of the > padding; adding more padding may cause either speedups or slowdowns.) Interesting, but I think Gavin was asking about how much variation was there for each tested case (e.g. query executed on the same code / dataset). And in those cases the padding / alignment won't change, and thus the effects you describe won't influence the results, no? -- Tomas Vondra http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>>>>> "Tomas" == Tomas Vondra <tomas.vondra@2ndquadrant.com> writes: Tomas> Interesting, but I think Gavin was asking about how muchTomas> variation was there for each tested case (e.g. queryexecuted onTomas> the same code / dataset). And in those cases the padding /Tomas> alignment won't change, and thusthe effects you describe won'tTomas> influence the results, no? My point is exactly the fact that since the result is not affected, this variation between runs of the same code is of no real relevance to the question of whether a given change in performance can properly be attributed to a patch. Put it this way: suppose I have a test that when run repeatedly with no code changes takes 6.10s (s=0.025s), and I apply a patch that changes that to 6.26s (s=0.025s). Did the patch have an impact on performance? Now suppose that instead of applying the patch I insert random amounts of padding in an unused function and find that my same test now takes a mean of 6.20s (s=0.058s) when I take the best timing for each padding size and calculate stats across sizes. Now it looks obvious that the actual code of the patch probably wasn't responsible for any change... The numbers used here aren't theoretical; they are obtained by testing a single query - "select * from d_flt order by v offset 10000000" where d_flt contains 5 million float8 values - over 990 times with 33 different random padding sizes (uniform in 0-32767). Here's a scatter plot, with 3 runs of each padding size so you can see the repeatability: http://tinyurl.com/op9qg8a -- Andrew.
Hi, On 23.2.2015 11:59, Andrew Gierth wrote: >>>>>> "Tomas" == Tomas Vondra <tomas.vondra@2ndquadrant.com> writes: > > Tomas> Interesting, but I think Gavin was asking about how much > Tomas> variation was there for each tested case (e.g. query executed on > Tomas> the same code / dataset). And in those cases the padding / > Tomas> alignment won't change, and thus the effects you describe won't > Tomas> influence the results, no? > > My point is exactly the fact that since the result is not affected, > this variation between runs of the same code is of no real relevance > to the question of whether a given change in performance can properly > be attributed to a patch. > > Put it this way: suppose I have a test that when run repeatedly with no > code changes takes 6.10s (s=0.025s), and I apply a patch that changes > that to 6.26s (s=0.025s). Did the patch have an impact on performance? > > Now suppose that instead of applying the patch I insert random amounts > of padding in an unused function and find that my same test now takes a > mean of 6.20s (s=0.058s) when I take the best timing for each padding > size and calculate stats across sizes. Now it looks obvious that the > actual code of the patch probably wasn't responsible for any change... > > The numbers used here aren't theoretical; they are obtained by testing a > single query - "select * from d_flt order by v offset 10000000" where > d_flt contains 5 million float8 values - over 990 times with 33 > different random padding sizes (uniform in 0-32767). Here's a scatter > plot, with 3 runs of each padding size so you can see the repeatability: > http://tinyurl.com/op9qg8a I think we're talking about slightly different things, then. I believe Gavin was asking about variability for executions with a particular code (i.e. with fixed amount of padding), to decide whether it even makes sense to compare results for different patches or whether the differences are just random noise. Interpreting those differences - whether they are due to changes in the algorithm or a result of some padding somewhere else in the code, that's of course important too. I believe the small regressions (1-10%) for small data sets, might be caused by this 'random padding' effect, because that's probably where L1/L2 cache is most important. For large datasets the caches are probably not as efficient anyway, so the random padding makes no difference, and the speedup is just as good as for the other queries. See for example this: http://www.postgresql.org/message-id/54EB580C.2000904@2ndquadrant.com But I'm speculating here ... time for a profiler, I guess. -- Tomas Vondra http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>>>>> "Tomas" == Tomas Vondra <tomas.vondra@2ndquadrant.com> writes: Tomas> I believe the small regressions (1-10%) for small data sets,Tomas> might be caused by this 'random padding' effect,because that'sTomas> probably where L1/L2 cache is most important. For large datasetsTomas> the caches are probablynot as efficient anyway, so the randomTomas> padding makes no difference, Except that _your own results_ show that this is not the case. In your first set of results you claimed a reduction in performance to 91% for a query which is _in no way whatsoever_ affected by any of the code changes. How is this not noise? I refer specifically to this case from your spreadsheet: query select * from (select * from stuff_text order by randtxt offset 100000000000) foo type text scale 5 master 26.949 datum 28.051 numeric 29.734 datum 96% numeric 91% This query does not invoke any code path touched by either the datum or numeric patches! The observed slowdown is therefore just noise (assuming here that your timings are correct). Whether that case can be improved by tweaking the _text_ abbreviation code is another question, one which is not relevant to either of the patches currently in play. -- Andrew (irc:RhodiumToad)
On 24.2.2015 05:09, Andrew Gierth wrote: >>>>>> "Tomas" == Tomas Vondra <tomas.vondra@2ndquadrant.com> writes: > > Tomas> I believe the small regressions (1-10%) for small data sets, > Tomas> might be caused by this 'random padding' effect, because that's > Tomas> probably where L1/L2 cache is most important. For large datasets > Tomas> the caches are probably not as efficient anyway, so the random > Tomas> padding makes no difference, > > Except that _your own results_ show that this is not the case. > > In your first set of results you claimed a reduction in performance > to 91% for a query which is _in no way whatsoever_ affected by any > of the code changes. How is this not noise? > > I refer specifically to this case from your spreadsheet: > > query select * from (select * from stuff_text order by randtxt > offset 100000000000) foo > type text > scale 5 > master 26.949 > datum 28.051 > numeric 29.734 > datum 96% > numeric 91% > > This query does not invoke any code path touched by either the datum > or numeric patches! The observed slowdown is therefore just noise > (assuming here that your timings are correct). I don't see how that contradicts what I said? Weren't you claiming that changes / random padding in unrelated functions may impact other places by introducing different patterns of cache misses etc? But yes, I do agree the first set of results is rather noisy, at least partially due to running on CPU (i7-4770R) with some power-management features etc. That's why I did the follow-up tests on a different system with CPUs that don't not do that - at least the i5 does not, and the results are much better IMHO. > Whether that case can be improved by tweaking the _text_ > abbreviation code is another question, one which is not relevant to > either of the patches currently in play. I don't think I suggested that. -- Tomas Vondra http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Mon, Feb 23, 2015 at 5:59 AM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: >>>>>> "Tomas" == Tomas Vondra <tomas.vondra@2ndquadrant.com> writes: > Tomas> Interesting, but I think Gavin was asking about how much > Tomas> variation was there for each tested case (e.g. query executed on > Tomas> the same code / dataset). And in those cases the padding / > Tomas> alignment won't change, and thus the effects you describe won't > Tomas> influence the results, no? > > My point is exactly the fact that since the result is not affected, this > variation between runs of the same code is of no real relevance to the > question of whether a given change in performance can properly be > attributed to a patch. > > Put it this way: suppose I have a test that when run repeatedly with no > code changes takes 6.10s (s=0.025s), and I apply a patch that changes > that to 6.26s (s=0.025s). Did the patch have an impact on performance? > > Now suppose that instead of applying the patch I insert random amounts > of padding in an unused function and find that my same test now takes a > mean of 6.20s (s=0.058s) when I take the best timing for each padding > size and calculate stats across sizes. Now it looks obvious that the > actual code of the patch probably wasn't responsible for any change... > > The numbers used here aren't theoretical; they are obtained by testing a > single query - "select * from d_flt order by v offset 10000000" where > d_flt contains 5 million float8 values - over 990 times with 33 > different random padding sizes (uniform in 0-32767). Here's a scatter > plot, with 3 runs of each padding size so you can see the repeatability: > http://tinyurl.com/op9qg8a Neat chart. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attached is a revision of this patch, that I'm calling v2. What do you think, Andrew? I've cut the int32 representation/alternative !USE_FLOAT8_BYVAL encoding scheme entirely, which basically means that 32-bit systems don't get to have this optimization. 64-bit systems have been commonplace now for about a decade. This year, new phones came out with 64-bit architectures, so increasingly even people that work with embedded systems don't care about 32-bit. I'm not suggesting that legacy doesn't matter - far from it - but I care much less about having the latest performance improvements on what are largely legacy systems. Experience suggests that this is a good time of the cycle to cut scope. The last commitfest has a way of clarifying what is actually important. It seems unwise to include what is actually a fairly distinct encoding scheme, which the int32/ !USE_FLOAT8_BYVAL variant really was (the same can't really be said for text abbreviation, since that can basically work the same way on 32-bit systems, with very little extra code). This isn't necessarily the right decision in general, but I feel it's the right decision in the present climate of everyone frantically closing things out, and feeling burnt out. I'm sorry that I threw some of your work away, but since we both have other pressing concerns, perhaps this is understandable. It may be revisited, or I may lose the argument on this point, but going this way cuts the code by about 30%, and makes me feel a lot better about the risk of regressing marginal cases, since I know we always have 8 bytes to work with. There might otherwise be a danger of regressing under tested 32-bit platforms, or indeed missing other bugs, and frankly I don't have time to think about that right now. Other than that, I've tried to keep things closer to the text opclass. For example, the cost model now has a few debugging traces (disabled by default). I have altered the ad-hoc cost model so that it no longer concerns itself with NULL inputs, which seemed questionable (not least since the abbreviation conversion function isn't actually called for NULL inputs. Any attempt to track the full count within numeric code therefore cannot work.). I also now allocate a buffer of scratch memory separately from the main sortsupport object - doing one allocation for all sortsupport state, bunched together as a buffer seemed like a questionable micro-optimization. For similar reasons, I avoid playing tricks in the VARATT_IS_SHORT() case -- my preferred approach to avoiding palloc()/pfree() cycles is to simply re-use the same buffer across calls to numeric_abbrev_convert(), and maybe risk having to enlarge the relatively tiny buffer once or twice. In other words, it works more or less the same way as it does with text abbreviation. It seemed unwise to silently disable abbreviation when someone happened to build with DEC_DIGITS != 4. A static assertion now gives these unusual cases the opportunity to make an informed decision about either disabling abbreviation or not changing DEC_DIGITS in light of the performance penalty, in a self-documenting way. The encoding scheme is unchanged. I think that your conclusions on those details were sound. Numeric abbreviation has a more compelling cost/benefit ratio than even that of text. I easily managed to get the same 6x - 7x improvement that you reported when sorting 10 million random numeric rows. Thanks -- Peter Geoghegan
Вложения
>>>>> "Peter" == Peter Geoghegan <pg@heroku.com> writes: Peter> Attached is a revision of this patch, that I'm calling v2. WhatPeter> do you think, Andrew? "No." is I think the best summary of my response. I strongly suggest whichever committer ends up looking at this consider my original version unchanged in preference to this. The cost/benefit decision of supporting abbreviation on 32bit platforms is a point that can be debated (I strongly support retaining the 32bit code, obviously), but the substantive changes here are actively wrong. Peter> Other than that, I've tried to keep things closer to the textPeter> opclass. For example, the cost model now hasa few debuggingPeter> traces (disabled by default). I have altered the ad-hoc costPeter> model so that it no longer concernsitself with NULL inputs,Peter> which seemed questionable (not least since the abbreviationPeter> conversion functionisn't actually called for NULL inputs. AnyPeter> attempt to track the full count within numeric code thereforePeter>cannot work.). This is simply wrong. The reason why the cost model (in my version) tracks non-null values by having its own counter is precisely BECAUSE the passed-in memtupcount includes nulls, and therefore the code will UNDERESTIMATE the fraction of successfully abbreviated values if the comparison is based on memtupcount. In your version, if there are 9999 null values at the start of the input, then on the first non-null value after that, memtupcount will be 10000 and there will be only 1 distinct abbreviated value, causing abbreviation to spuriously abort. The test to clamp the estimate to 1.0 is just nonsensical and serves no purpose whatever, and the comment for it is wrong. You should fix the text abbreviation code, not propagate your mistakes further. (BTW, there's an outright typo in your code, ';;' for ';' at the end of a line. Sloppy.) Peter> I also now allocate a buffer of scratch memory separately fromPeter> the main sortsupport object - doing one allocationfor allPeter> sortsupport state, bunched together as a buffer seemed like aPeter> questionable micro-optimization. It's yet another cache line... I admit I did not benchmark that choice, but then neither did you. Peter> It seemed unwise to silently disable abbreviation when someonePeter> happened to build with DEC_DIGITS != 4. A staticassertion nowPeter> gives these unusual cases the opportunity to make an informedPeter> decision about either disablingabbreviation or not changingPeter> DEC_DIGITS in light of the performance penalty, in aPeter> self-documenting way. A) Nobody in their right minds is ever going to do that anyway B) Anybody who does that is either not concerned about performance or is concerned only about performance of the low-level numeric ops, and abbreviation is the last thing they're going to be worried about in either case. -- Andrew (irc:RhodiumToad)
On Fri, Mar 20, 2015 at 7:10 PM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: > Peter> Other than that, I've tried to keep things closer to the text > Peter> opclass. For example, the cost model now has a few debugging > Peter> traces (disabled by default). I have altered the ad-hoc cost > Peter> model so that it no longer concerns itself with NULL inputs, > Peter> which seemed questionable (not least since the abbreviation > Peter> conversion function isn't actually called for NULL inputs. Any > Peter> attempt to track the full count within numeric code therefore > Peter> cannot work.). > This is simply wrong. The reason why the cost model (in my version) > tracks non-null values by having its own counter is precisely BECAUSE > the passed-in memtupcount includes nulls, and therefore the code will > UNDERESTIMATE the fraction of successfully abbreviated values if the > comparison is based on memtupcount. Oh, right. It's the other way around in your original. I don't really buy it, either way. In what sense is a NULL value ever abbreviated? It isn't. Whatever about the cost model, that's the truth of the matter. There is always going to be a sort of tension in any cost model, between whether or not it's worth making it more sophisticated, and the extent to which tweaking the model is chasing diminishing returns. > In your version, if there are 9999 > null values at the start of the input, then on the first non-null value > after that, memtupcount will be 10000 and there will be only 1 distinct > abbreviated value, causing abbreviation to spuriously abort. By what objective standard is that spurious? As things stand, I hesitate to get these ad-hoc cost models into the business of worrying about sorts with many NULL values, because of the additional complexity. Sorts with many NULL values, such as your example, have always been very fast, but also very rare in the real world. Sure, things might change in the event of many NULLs, and so you might consider that it's worth hanging on at no extra cost. But these cost models are really about preventing the very worst case. It seems worthwhile to not over-complicate them. They're based on the assumption that a sample of the first n values are representative of the whole, which, in general, could certainly be false. We do what we can. Your example has one abbreviated key in it, which is exactly worthless among 9999 NULL values. If it is representative of the next 10K rows, or the next 100K, then we probably should abort. Maybe that isn't exactly the right thing here, but if so that's only because numeric abbreviation is relatively cheap. in general, amortizing the cost of comparisons through encoding is a lousy strategy when there will be so few real comparisons. I'd like to hear what other people think. We could certainly consider adding that back, since it isn't especially complicated. Perhaps I was hasty there. > The test to clamp the estimate to 1.0 is just nonsensical and serves no > purpose whatever, and the comment for it is wrong. > > You should fix the text abbreviation code, not propagate your mistakes > further. > > (BTW, there's an outright typo in your code, ';;' for ';' at the end of > a line. Sloppy.) We're really going to call out minor typos like that as sloppy? If so, let me name a few of yours: * Wrong ordering of header includes * Trailing whitespace * "...but this time is it he original weight in digit..." (not "it is"?) I also think that your explanation of the encoding schemes was perfunctory. And, the VARATT_IS_SHORT() hack that you added seemed wholly unnecessary. You better remind the committer that's going to "consider my [Andrew's] original version unchanged in preference to this" to go over these points again. Or you could try and work it out with me, the reviewer, rather than behaving so petulantly. > Peter> I also now allocate a buffer of scratch memory separately from > Peter> the main sortsupport object - doing one allocation for all > Peter> sortsupport state, bunched together as a buffer seemed like a > Peter> questionable micro-optimization. > > It's yet another cache line... I admit I did not benchmark that choice, > but then neither did you. You're right, I didn't. There are two reasons why: 1) It doesn't work that way. I am not required to make sure that a patch I'm reviewing doesn't take advantage of every possible micro-optimization. Clarity is a more pressing concern. If I changed existing code in the master branch, that would be another story. You're the patch author here, remember? If it's such a loss, then prove it. 2) This patch is extremely effective in general. Well done! It seemed silly to worry about a micro-optimization like that, especially given the current time pressures for *both* of us. It can always be revisited. > Peter> It seemed unwise to silently disable abbreviation when someone > Peter> happened to build with DEC_DIGITS != 4. A static assertion now > Peter> gives these unusual cases the opportunity to make an informed > Peter> decision about either disabling abbreviation or not changing > Peter> DEC_DIGITS in light of the performance penalty, in a > Peter> self-documenting way. > > A) Nobody in their right minds is ever going to do that anyway > > B) Anybody who does that is either not concerned about performance or is > concerned only about performance of the low-level numeric ops, and > abbreviation is the last thing they're going to be worried about in > either case. You're probably right about that, but then if we do things my way, we don't have to plaster #ifdef rubbish all over the place. That seems like another distinct advantage to me (it's mostly what I was thinking of, actually) - it makes sense to reduce the places that have defenses against this to one if we can, because, as you point out, it'll probably never come up anyway. Get off your high horse. I was perfectly respectful in my comments to you. I expect the same in return. If you insist in being so abrasive at the slightest perceived provocation, no one will want to work with you. This process is consensus driven. You can make objections without being a jerk. -- Peter Geoghegan
>>>>> "Peter" == Peter Geoghegan <pg@heroku.com> writes: >> This is simply wrong. The reason why the cost model (in my version)>> tracks non-null values by having its own counteris precisely>> BECAUSE the passed-in memtupcount includes nulls, and therefore the>> code will UNDERESTIMATE the fractionof successfully abbreviated>> values if the comparison is based on memtupcount. Peter> Oh, right. It's the other way around in your original. Peter> I don't really buy it, either way. In what sense is a NULL valuePeter> ever abbreviated? It isn't. Whatever aboutthe cost model,Peter> that's the truth of the matter. There is always going to be aPeter> sort of tension in any costmodel, between whether or not it'sPeter> worth making it more sophisticated, and the extent to whichPeter> tweaking themodel is chasing diminishing returns. Comparisons between nulls and nulls, or between nulls and non-nulls, are cheap; only comparisons between non-nulls and non-nulls can be expensive. The purpose of abbreviation is to replace expensive comparisons by cheap ones where possible, and therefore the cost model used for abbreviation should ignore nulls entirely; all that matters is the number of non-null values and the probability of saving time by abbreviating them. So if you're sorting a million rows of which 900,000 are null and 100,000 contain 50 different non-null values, then the absolute time saved (not the proportion) by doing abbreviation should be on the same order as the absolute time saved by abbreviation when sorting just the 100,000 non-null rows. But if the cost model does 1,000,000/50 and gets 20,000, and decides "that's worse than my 1 in 10,000 target, I'll abort abbreviations", then you have sacrificed the time gain for no reason at all. This is what I mean by "spurious". This is why the cost model must compute the fraction as 100,000/50, ignoring the null inputs, if it's going to perform anything like optimally in the presence of nulls. Peter> By what objective standard is that spurious? The objective standard of my wallclock. Doing a simple count of values abbreviated is not anything that could be considered "complex", and there is nothing at all "ad-hoc" about it. Your method is simply incorrect on both logical and performance grounds. Peter> Your example has one abbreviated key in it, which is exactlyPeter> worthless among 9999 NULL values. My example wasn't intended to imply that there would be only one non-null value in the whole sort, merely to illustrate why nulls need to be ignored so as not to incorrectly bias the model. You do not know whether abbreviation will be useful until you have seen a sufficient number of NON-NULL values. The problem of the initial subset of data not necessarily being representative is not relevant to this. Peter> I also think that your explanation of the encoding schemes wasPeter> perfunctory. I'm interested in other opinions on that, because I find your replacement for it both confusingly organized and a bit misleading (for example saying the top bit is "wasted" is wrong, it's reserved because we need it free for the sign). (It is true that mine assumes that the reader knows what "excess-N" means, or can find out.) Here's mine, which is given as a single block comment: + * Two different representations are used for the abbreviated form, one in + * int32 and one in int64, with the int64 one used if USE_FLOAT8_BYVAL is set + * (which despite the name is also the flag that says whether int64 is + * passed-by-value). In both cases the representation is negated relative to + * the original value, because we use the largest negative value for NaN, which + * sorts higher than other values. We convert the absolute value of the numeric + * to a 31-bit or 63-bit positive value, and then negate it if the original + * number was positive. + * + * The 31-bit value is constructed as: + * + * 0 + 7bits digit weight + 24 bits digit value + * + * where the digit weight is in single decimal digits, not digit words, and + * stored in excess-44 representation. The 24-bit digit value is the 7 most + * significant decimal digits of the value converted to binary. Values whose + * weights would fall outside the representable range are rounded off to zero + * (which is also used to represent actual zeros) or to 0x7FFFFFFF (which + * otherwise cannot occur). Abbreviation therefore fails to gain any advantage + * where values are outside the range 10^-44 to 10^83, which is not considered + * to be a serious limitation, or when values are of the same magnitude and + * equal in the first 7 decimal digits, which is considered to be an + * unavoidable limitation given the available bits. (Stealing three more bits + * to compare another digit would narrow the range of representable weights by + * a factor of 8, which starts to look like a real limiting factor.) + * + * (The value 44 for the excess is essentially arbitrary) + * + * The 63-bit value is constructed as: + * + * 0 + 7bits weight + 4 x 14-bit packed digit words + * + * The weight in this case is again stored in excess-44, but this time is it + * the original weight in digit words (i.e. powers of 10000). The first four + * digit words of the value (if present; trailing zeros are assumed as needed) + * are packed into 14 bits each to form the rest of the value. Again, + * out-of-range values are rounded off to 0 or 0x7FFFFFFFFFFFFFFF. The + * representable range in this case is 10^-176 to 10^332, which is considered + * to be good enough for all practical purposes, and comparison of 4 words + * means that at least 13 decimal digits are compared, which is considered to + * be a reasonable compromise between effectiveness and efficiency in computing + * the abbreviation. + * + * (The value 44 for the excess is even more arbitrary here, it was chosen just + * to match the value used in the 31-bit case) Peter's, inline with the code (omitted here): + * First, store "weight" in first 7 binary digits (7 binary digits is + * the smallest number of digits that allows us to represent weights + * -44 to 83 inclusive). + * + * The weight is stored in excess-44 -- the original weight in digit + * words (i.e. powers of NBASE/10000). The "excess-K"/offset binary + * technique is also used with IEEE-754 floating point numbers. As + * with IEEE-754, we use an exponent without a sign (a 7-bit exponent + * without a sign). And as with IEEE-754, the lack of a sign does not + * imply that the exponent must be *logically* positive. + * + * Adding 44 to "weight" makes the smallest possible "exponent" 7 + * binary digit number (where "res" hasn't already saturated to + * NUMERIC_ABBREV_NEG_INFINITY) have the value zero in this + * representation (since the smallest possible "weight" is -44). If + * the "weight" is 83, the largest possible, then "exponent" will have + * 0b1111111 as its first 7 binary digits. The 7-digit exponent is + * always positive (forgetting for a moment that it's really + * excess-44), and thus the bits could be equivalently interpreted as + * either signed or unsigned. + * + * We left shift 56 bits in order to have this 7-bit representation + * stored at the beginning of "exponent", a two's complement/signed + * 64-bit integer. In doing so, an eighth bit is "wasted". [...] + * Append 4 x 14-bit packed digit words into remaining 56 bits. + * 14-bits of storage should be enough to represent the largest + * possible base-NBASE digit, despite the fact that those are stored as + * int16. [...] + * Flip the abbreviated int64 representation's sign, for positive numerics + * only, since the 63-bit value is currently the absolute value. With + * this, the last detail of encoding things such that int64 comparisons can + * be interpreted backwards (within the abbreviated comparator, as a proxy + * for actual numeric comparisons) is complete. -- Andrew (irc:RhodiumToad)
Was there some reason why you added #include "utils/memutils.h"? Because I don't see anything in your patch that actually needs it. -- Andrew (irc:RhodiumToad)
So here's a version 3 patch: 1. #ifdefism is reduced to a minimum by (a) desupporting values of NBASE other than 10000, and (b) keeping the 32bit code, so that there is now no longer any question of abbreviation not being used (it always is). So the only #ifs in the code body (rather than declarations) are to select which implementation of numeric_abbrev_convert_var to use. 2. Some (but not all) stylistic changes have been made following Peter's version. 3. Buffer management has been simplified by simply allocating the maximum needed size upfront (since it's only needed for short varlenas). The truncation behavior has been removed. 4. Updated oid choice etc. The substance of the code is unchanged from my original patch. I didn't add diagnostic output to numeric_abbrev_abort, see my separate post about the suggestion of a GUC for that. -- Andrew (irc:RhodiumToad)
Вложения
On Sat, Mar 21, 2015 at 3:33 PM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: > Was there some reason why you added #include "utils/memutils.h"? > Because I don't see anything in your patch that actually needs it. MaxAllocSize is defiined there. -- Peter Geoghegan
On Fri, Mar 20, 2015 at 11:58 PM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: > Comparisons between nulls and nulls, or between nulls and non-nulls, are > cheap; only comparisons between non-nulls and non-nulls can be > expensive. > > The purpose of abbreviation is to replace expensive comparisons by cheap > ones where possible, and therefore the cost model used for abbreviation > should ignore nulls entirely; all that matters is the number of non-null > values and the probability of saving time by abbreviating them. I don't think it's that simple. The actual point of abbreviation is to amortize the total cost of performing O(n log n) comparisons (the average case, which is usually assumed by infrastructure like sortsupport), by performing an encoding process n times. That encoding process may be more expensive than each of the comparisons. Sometimes significantly more expensive. Forget about abbreviation entirely for a minute - consider plain old sorting of strxfrm() blobs, which for example the glibc documentation recommends (with some caveats) as the preferred approach to sorting strings while respecting collation. Suppose that your applications happens to frequently encounter fully sorted arrays of strings when passed an array of strings to sort. If you were using our qsort() implementation, which, rather questionably, has a pre-check for fully sorted input (that throws away work when the pre-check fails), then you'll only have n comparisons. Even if an encoding is only as expensive as an actual comparison, the potential to lose with encoding clearly exists. Similarly, we don't abbreviate for top-n heap sorts, even though all of those abbreviated keys may be used for at least one comparison. Now, I hear what you're saying about weighing the costs and the benefits -- you point out that abbreviation of NULL values is not a cost, which is certainly true. But if the total number of NULL values is so high that they dominate, then abbreviation might well be the wrong thing for that reason alone. In general, string transformation is not recommended for sorting very small lists of strings. Where I think your argument is stronger is around the cost of actually aborting in particular (even though not much work is thrown away). Scanning through the memtuples array once more certainly isn't free. And you could take the view that it's always worth the risk, since it's at most a marginal cost saved if the first ~10k tuples are representative, but a large cost incurred when they are not. So on reflection, I am inclined to put the check for non-null values back in. I also concede that the cost of abbreviation is lower with your numeric encoding scheme than it is for that of the text opclass, which favors this approach. -- Peter Geoghegan
>>>>> "Peter" == Peter Geoghegan <pg@heroku.com> writes: Peter> I don't think it's that simple. The actual point of abbreviationPeter> is to amortize the total cost of performingO(n log n)Peter> comparisons (the average case, which is usually assumed byPeter> infrastructure like sortsupport),by performing an encodingPeter> process n times. That encoding process may be more expensivePeter> than eachof the comparisons. Sometimes significantly morePeter> expensive. But the cost of the encoding depends only on the number of non-null values. Again, the nulls turn out to be irrelevant. Peter> Forget about abbreviation entirely for a minute - consider plainPeter> old sorting of strxfrm() blobs, which for examplethe glibcPeter> documentation recommends (with some caveats) as the preferredPeter> approach to sorting strings whilerespecting collation. SupposePeter> that your applications happens to frequently encounter fullyPeter> sorted arraysof strings when passed an array of strings toPeter> sort. If you were using our qsort() implementation, which,Peter>rather questionably, has a pre-check for fully sorted inputPeter> (that throws away work when the pre-check fails),then you'llPeter> only have n comparisons. Even if an encoding is only asPeter> expensive as an actual comparison,the potential to lose withPeter> encoding clearly exists. Similarly, we don't abbreviate forPeter> top-n heapsorts, even though all of those abbreviated keys mayPeter> be used for at least one comparison. Nothing in the above paragraph has the slightest relevance to even the text abbreviation code, much less the numeric one. Peter> Now, I hear what you're saying about weighing the costs and thePeter> benefits -- you point out that abbreviationof NULL values isPeter> not a cost, which is certainly true. It's not a cost because it DOESN'T HAPPEN. The abbreviation function is not called for null inputs. Peter> But if the total number of NULL values is so high that theyPeter> dominate, then abbreviation might well be the wrongthing forPeter> that reason alone. No. This is the point that you're getting wrong. The amount of time saved by the abbreviation code depends ONLY on the number and cardinality of the NON-NULL values. Also, the time _lost_ by the abbreviation code if we fail to abort in pathological cases STILL depends only on the number of non-null values, so there's no benefit in aborting even in a case when we have 1000 equal non-null values mixed in with tens of millions of nulls. Let me give you an actual example: create table numtest2 as select floor(random() * 200)::numeric as a from generate_series(1,1000000); create table numtest3 as select a from (select floor(random() * 200)::numeric as a from generate_series(1,1000000) union all select null from generate_series(1,4000000)) s order byrandom(); So one table has a million non-null rows with 200 distinct values, and the other has the same plus 4 million null rows. Using my code, which ignores nulls in the cost model: select * from numtest2 order by a offset 100000000; -- 1.5 seconds select * from numtest3 order by a offset 100000000; -- 3.6 seconds With abbreviation disabled entirely: select * from numtest2 order by a offset 100000000; -- 4.5 seconds select * from numtest3 order by a offset 100000000; -- 6.8 seconds Notice that while proportionally the speedup is only <2x rather than 3x for the version with nulls, the absolute difference in time is the same in both cases - abbreviation saved us ~3 seconds in both cases. (This relation remains true for larger values too: make it 19 million nulls rather than 4 million and the timings are 11.5 sec / 14.5 sec, still a 3 sec difference.) Your version would have aborted abbrevation on that second query, thus losing a 3 second speedup. How on earth is that supposed to be justified? It's not like there's any realistically possible case where your version performs faster than mine by more than a tiny margin. Peter> Where I think your argument is stronger is around the cost ofPeter> actually aborting in particular (even though notmuch work isPeter> thrown away). Scanning through the memtuples array once morePeter> certainly isn't free. Yes, the cost of actually aborting abbreviation goes up with the number of nulls. But your version of the code makes that WORSE, by making it more likely that we will abort unnecessarily. If we use the raw value of memtuplecount for anything, it should be to make it LESS likely that we abort abbreviations (since we'd be paying a higher cost), not more. But benchmarking doesn't suggest that this is necessary, at least not for numerics. Peter> And you could take the view that it's always worth the risk,Peter> since it's at most a marginal cost saved if thefirst ~10kPeter> tuples are representative, but a large cost incurred when theyPeter> are not. So on reflection, I aminclined to put the check forPeter> non-null values back in. Right result but the wrong reasoning. Peter> I also concede that the cost of abbreviation is lower with yourPeter> numeric encoding scheme than it is for thatof the text opclass,Peter> which favors this approach. If you do some benchmarks on text columns, you'll find that this is a win there too. -- Andrew (irc:RhodiumToad)
>>>>> "Peter" == Peter Geoghegan <pg@heroku.com> writes: >> Was there some reason why you added #include "utils/memutils.h"?>> Because I don't see anything in your patch that actuallyneeds it. Peter> MaxAllocSize is defiined there. So it is. (That seems to me to be another mistake along the same lines as putting the context callbacks in memutils; it's a definition that callers of palloc need to use, even if they're not creating contexts themselves.) However, given that this is a buffer for aligning VARATT_IS_SHORT datums, allowing it to expand to MaxAllocSize seems like overkill; the updated patch I just posted preallocates it at the max size of a short varlena (plus long varlena header). -- Andrew (irc:RhodiumToad)
On Sun, Mar 22, 2015 at 6:48 PM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: > Your version would have aborted abbrevation on that second query, thus > losing a 3 second speedup. How on earth is that supposed to be > justified? It's not like there's any realistically possible case where > your version performs faster than mine by more than a tiny margin. As I said, that's really why you won the argument on this particular point. Why are you still going on about it? > Peter> Where I think your argument is stronger is around the cost of > Peter> actually aborting in particular (even though not much work is > Peter> thrown away). Scanning through the memtuples array once more > Peter> certainly isn't free. > > Yes, the cost of actually aborting abbreviation goes up with the number > of nulls. But your version of the code makes that WORSE, by making it > more likely that we will abort unnecessarily. > > If we use the raw value of memtuplecount for anything, it should be to > make it LESS likely that we abort abbreviations (since we'd be paying a > higher cost), not more. But benchmarking doesn't suggest that this is > necessary, at least not for numerics. > > Peter> And you could take the view that it's always worth the risk, > Peter> since it's at most a marginal cost saved if the first ~10k > Peter> tuples are representative, but a large cost incurred when they > Peter> are not. So on reflection, I am inclined to put the check for > Peter> non-null values back in. > > Right result but the wrong reasoning. I think that there is an argument to be made against abbreviation when we simply have a small list of strings to begin with (e.g. 50), as per the glibc strxfrm() docs (which, as I said, may not apply with numeric to the same extent). It didn't end up actually happening that way for the text opclass for various reasons, mostly because the cost model was already complicated enough. Ideally, the number of comparisons per key is at least 10 when we abbreviate with text, which works out at about 1,000 tuples (as costed by cost_sort()). For that reason, leaving aside the risk of aborting when the sampled tuples are not representative (which is a big issue, that does clearly swing things in favor of always disregarding NULLs), having few actual values to sort is in theory a reason to not encode/abbreviate. -- Peter Geoghegan
On Sun, Mar 22, 2015 at 1:01 PM, Andrew Gierth
<andrew@tao11.riddles.org.uk> wrote:
> The substance of the code is unchanged from my original patch. I didn't
> add diagnostic output to numeric_abbrev_abort, see my separate post
> about the suggestion of a GUC for that.
I don't think that V2 really changed the substance, which seems to be
the implication of your remarks here. You disagreed with my decision
on NULL values - causing me to reconsider my position (so that's now
irrelevant) - and you disagreed with not including support for 32-bit
platforms. Those were the only non-stylistic changes, though. I
certainly didn't change any details of the algorithm that you
proposed, which, FWIW, I think is rather clever. I added a few
defensive assertions to the encoding/conversion routine (which I see
you've removed in V3, a long with a couple of other helpful
assertions), and restructured and expanded upon the comments, but
that's all.
You haven't really taken into my account my V2 feedback with this V3
revision. Even after you yourself specifically called out your
non-explanation of excess-44 as a possible point of confusion for
readers of your patch, you didn't change or expand upon your remarks
on that one iota.
I see that code like this (from your V1) appears in your V3, as if V2
never happened:
+/*
+ * non-fmgr interface to the comparison routine to allow sortsupport to elide
+ * the fmgr call. The saving here is small given how slow numeric
+ * comparisons are, but there's no reason not to implement it.
+ */
This comment is totally redundant at best, and misleading at worst. Of
course we have a non-fmgr interface - it's needed to make abbreviation
work. And of course *any* opclass that performs abbreviation won't get
any great saving from cases where only the fmgr interface is elided
(e.g. numeric is not the leading sorttuple attribute). Text is unusual
in that there is a small additional saving over fmgr-elision for
obscure reasons.
Ditto for comments like this, which re-appear in V3:
+/*
+ * Ordinary (non-sortsupport) comparisons follow.
+ */
Your V3 has obsolete comments here:
+ nss = palloc(sizeof(NumericSortSupport));
+
+ /*
+ * palloc a buffer for handling unaligned packed values in addition to
+ * the support struct
+ */
+ nss->buf = palloc(VARATT_SHORT_MAX + VARHDRSZ + 1);
I still don't think you should be referencing the text opclass behavior here:
+ * number. We make no attempt to estimate the cardinality of the real values,
+ * since it plays no part in the cost model here (if the abbreviation is equal,
+ * the cost of comparing equal and unequal underlying values is comparable).
+ * We discontinue even checking for abort (saving us the hashing overhead) if
+ * the estimated cardinality gets to 100k; that would be enough to support many
+ * billions of rows while doing no worse than breaking even.
This is dubious:
+#if DEC_DIGITS != 4
+#error "Numeric bases other than 10000 are no longer supported"
+#endif
Because there is a bunch of code within numeric.c that deals with the
DEC_DIGITS != 4 case. For example, this code has been within numeric.c
forever:
#if DEC_DIGITS == 4 || DEC_DIGITS == 2
static NumericDigit const_ten_data[1] = {10};
static NumericVar const_ten =
{1, 0, NUMERIC_POS, 0, NULL, const_ten_data};
#elif DEC_DIGITS == 1
static NumericDigit const_ten_data[1] = {1};
static NumericVar const_ten =
{1, 1, NUMERIC_POS, 0, NULL, const_ten_data};
#endif
As has this:
while (ndigits-- > 0)
{
#if DEC_DIGITS == 4
*digits++ = ((decdigits[i] * 10 + decdigits[i + 1]) * 10 +
decdigits[i + 2]) * 10 + decdigits[i + 3];
#elif DEC_DIGITS == 2
*digits++ = decdigits[i] * 10 + decdigits[i + 1];
#elif DEC_DIGITS == 1
*digits++ = decdigits[i];
#else
#error unsupported NBASE
#endif
i += DEC_DIGITS;
}
I tend to think that when Tom wrote this code back in 2003, he thought
it might be useful to change DEC_DIGITS on certain builds. And so, we
ought to continue to support it to the extent that we already do,
allowing these cases to opt out of abbreviation in an informed manner
(since it seems hard to make abbreviation work with DEC_DIGITS != 4
builds). In any case, you should have deleted all this code (which
there is rather a lot of) in proposing to not support DEC_DIGITS != 4
builds generally.
Attached revision, V4, incorporates some of your V3 changes. As I
mentioned, I changed my mind on the counting of non-NULL values, which
V4 reflects...but there are also comments that now make it clear why
that might be useful.
I've retained your new allocate once approach to buffer sizing, which
seems sound if a little obscure.
This abort function code (from your V1 + V3) seems misleading:
+ if (memtupcount < 10000 || nss->input_count < 10000 || !nss->estimating)
+ return false;
It's impossible for "memtupcount < 10000" while "nss->input_count <
10000" as well, since the latter counts a subset of what the former
counts. So I only check the latter now.
I have not added back 32-bit support, which, IMV isn't worth it at
this time - it is, after all, almost a fully independent abbreviation
scheme to the 64-bit scheme. Right now we ought to be cutting scope,
or 9.5 will never reach feature freeze. I have already spent
considerably more time on this patch than I had intended to. I need to
catch a flight to New York at the crack of dawn tomorrow, to get to
pgConf US, and I have much work to do on my UPSERT patch, so I've
simply run out of time. If the committer that picks this up wants to
look at 32-bit support, that may make sense. I think that given the
current lack of cycles from everyone, we'll be doing well to even get
64-bit numeric abbreviation support in 9.5. I ask that you have a some
perspective on cutting 32-bit support, Andrew. In any case, I've
personally run out of time for this for 9.5.
We may add a patch to change things so that a GUC controls
abbreviation debug output, and we may add a patch that standardizes
that INT64_MIN and INT64_MAX are available everywhere. But unless and
until those other patches are committed, I see no reason to assume
that they will be, and so those aspects remain unchanged from my V2.
Unless there is strong opposition, or bugs come to light, or the
GUC/macros are added by independent commits to the master branch, I
expect to mark this revision "Ready for Committer" shortly.
Thanks
--
Peter Geoghegan
Вложения
On Sat, Mar 21, 2015 at 2:58 AM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: > Peter> I don't really buy it, either way. In what sense is a NULL value > Peter> ever abbreviated? It isn't. Whatever about the cost model, > Peter> that's the truth of the matter. There is always going to be a > Peter> sort of tension in any cost model, between whether or not it's > Peter> worth making it more sophisticated, and the extent to which > Peter> tweaking the model is chasing diminishing returns. > > Comparisons between nulls and nulls, or between nulls and non-nulls, are > cheap; only comparisons between non-nulls and non-nulls can be > expensive. > > The purpose of abbreviation is to replace expensive comparisons by cheap > ones where possible, and therefore the cost model used for abbreviation > should ignore nulls entirely; all that matters is the number of non-null > values and the probability of saving time by abbreviating them. > > So if you're sorting a million rows of which 900,000 are null and > 100,000 contain 50 different non-null values, then the absolute time > saved (not the proportion) by doing abbreviation should be on the same > order as the absolute time saved by abbreviation when sorting just the > 100,000 non-null rows. > > But if the cost model does 1,000,000/50 and gets 20,000, and decides > "that's worse than my 1 in 10,000 target, I'll abort abbreviations", > then you have sacrificed the time gain for no reason at all. This is > what I mean by "spurious". This is why the cost model must compute the > fraction as 100,000/50, ignoring the null inputs, if it's going to > perform anything like optimally in the presence of nulls. I think Andrew is right. > Peter> I also think that your explanation of the encoding schemes was > Peter> perfunctory. > > I'm interested in other opinions on that, because I find your > replacement for it both confusingly organized and a bit misleading (for > example saying the top bit is "wasted" is wrong, it's reserved because > we need it free for the sign). > > (It is true that mine assumes that the reader knows what "excess-N" > means, or can find out.) > > Here's mine, which is given as a single block comment: > > [ long explanatory comment ] > > Peter's, inline with the code (omitted here): > > [ long explanatory comment ] In my opinion, Andrew's version is far clearer. Peter's version is full of jargon that I can't understand. I could probably figure it out with a few hours and a search engine, but that really shouldn't be necessary. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Mar 23, 2015 at 12:04 PM, Robert Haas <robertmhaas@gmail.com> wrote: > In my opinion, Andrew's version is far clearer. Peter's version is > full of jargon that I can't understand. I could probably figure it > out with a few hours and a search engine, but that really shouldn't be > necessary. Really? Andrew's version doesn't even explain what excess-K is. Surely that's obscure jargon that requires an explanation. -- Peter Geoghegan
On Mon, Mar 23, 2015 at 1:01 PM, Peter Geoghegan <pg@heroku.com> wrote: > On Sun, Mar 22, 2015 at 1:01 PM, Andrew Gierth > <andrew@tao11.riddles.org.uk> wrote: >> The substance of the code is unchanged from my original patch. I didn't >> add diagnostic output to numeric_abbrev_abort, see my separate post >> about the suggestion of a GUC for that. > > I don't think that V2 really changed the substance, which seems to be > the implication of your remarks here. You disagreed with my decision > on NULL values - causing me to reconsider my position (so that's now > irrelevant) - and you disagreed with not including support for 32-bit > platforms. Those were the only non-stylistic changes, though. I > certainly didn't change any details of the algorithm that you > proposed, which, FWIW, I think is rather clever. I added a few > defensive assertions to the encoding/conversion routine (which I see > you've removed in V3, a long with a couple of other helpful > assertions), and restructured and expanded upon the comments, but > that's all. > > You haven't really taken into my account my V2 feedback with this V3 > revision. Even after you yourself specifically called out your > non-explanation of excess-44 as a possible point of confusion for > readers of your patch, you didn't change or expand upon your remarks > on that one iota. Guys, can we please knock it off with the dueling patches? Peter, it's really not all that helpful to take somebody else's patch, rewrite it in a form that they may or may not agree with (even if it's just the comments), and post that as "v2". And when the person then posts "v3" that reverts most of your changes, don't go put them all back and call that "v4". Instead, you should take the hint: these are not "versions" of the same patch - they are two different approaches to the same problem. In this type of situation, I generally post my patch with a name like "topicofthepatch-rmh-v1.patch" or "topicofthepatch-rmh-20150323.patch", putting my initials in there to show that this is my version of the patch, not the original author's and that it may or may not be endorsed by the original author. Having 26 versions of this patch where all of the odd-numbered versions looks like Andrew's original version and all of the even-numbered versions look like Peter's "v2" is not going to make anybody happy - not either of you, not me, and not anybody else here. The typical style of review here, which I endorse, is to tell the other person what you think they should change. There is a place for directly posting a new version yourself, when the amount of cleanup required is too great to be articulated in an email, and you really want to get the thing committed; or when the original author has disappeared and you want to take up the work. But you should try to do that only in cases where you are fairly sure your work will be welcome because, quite aside from whether it ruffles any feathers, it's easy to waste a lot of time rewriting something only to find out that others don't agree with your rewrites. Discussion helps to avoid that. Furthermore, when there *is* a disagreement about something, the thing to do is to ask for (or simply wait for) the opinions of others, not dig in your heals. Andrew doesn't have a *right* to have his version committed, and you don't have a *right* to change it. What we all have a right to do is discuss, and hopefully agree on, what is best. Thanks, -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Mar 23, 2015 at 3:07 PM, Peter Geoghegan <pg@heroku.com> wrote: > On Mon, Mar 23, 2015 at 12:04 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> In my opinion, Andrew's version is far clearer. Peter's version is >> full of jargon that I can't understand. I could probably figure it >> out with a few hours and a search engine, but that really shouldn't be >> necessary. > > Really? Since I'm not in the habit of posting things to the list that I don't really believe, it shouldn't so often be necessary to ask me if I really meant it. If I posted it, and it wasn't April 1st, I meant it. > Andrew's version doesn't even explain what excess-K is. Surely > that's obscure jargon that requires an explanation. Well, it's possible to infer it from what he wrote afterwards, and if you don't, you can still pretty much understand the main thrust of what it's doing and why. I bet that could be rewritten to avoid using the term altogether, but even if not it's pretty clear. I don't really want to get into a nitpicking session here, but if you're wondering what I don't like as well about your version, it's things like this: + (7 binary digits is + * the smallest number of digits that allows us to represent weights + * -44 to 83 inclusive). This isn't a stupid comment, but it also isn't really commenting on the right thing. I mean, -44 to 83 is a range of 128 possible integers, so obviously it's going to need 7 bits to represent it, because 2^7=128. So, whatever information this is trying to convey, it's not quite succeeding. It's also a forward reference, because you haven't yet given any clue what's interesting about why we are trying to represent a value between -44 and 83. A bit further down: As + * with IEEE-754, we use an exponent without a sign (a 7-bit exponent + * without a sign). As to the beginning of this sentence, bringing IEEE-754 into this discussion doesn't clarify anything in my mind. I don't think most people reading these comments are likely to be familiar with IEEE-754, or want to go look it up. As to the end of the sentence, writing "an exponent without a sign" and then describing that as "a 7-bit exponent without a sign" is extremely redundant. Perhaps you were trying to say that we are similar to IEEE-754 in that we use an exponent without a sign (whatever that means) but different in that we ours is 7-bits, but it's not really clear. I offer these not in the spirit of asking you to correct these specific things but just of explaining generally the sort of thing that causes me to prefer Andrew's version. Hope that's helpful. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Mar 23, 2015 at 12:20 PM, Robert Haas <robertmhaas@gmail.com> wrote: > Guys, can we please knock it off with the dueling patches? > > Peter, it's really not all that helpful to take somebody else's patch, > rewrite it in a form that they may or may not agree with (even if it's > just the comments), and post that as "v2". And when the person then > posts "v3" that reverts most of your changes, don't go put them all > back and call that "v4". Instead, you should take the hint: these are > not "versions" of the same patch - they are two different approaches > to the same problem. In this type of situation, I generally post my > patch with a name like "topicofthepatch-rmh-v1.patch" or > "topicofthepatch-rmh-20150323.patch", putting my initials in there to > show that this is my version of the patch, not the original author's > and that it may or may not be endorsed by the original author. Having > 26 versions of this patch where all of the odd-numbered versions looks > like Andrew's original version and all of the even-numbered versions > look like Peter's "v2" is not going to make anybody happy - not either > of you, not me, and not anybody else here. As I said, I don't really consider that my patch is a rewrite, especially V4, which changes nothing substantive except removing 32-bit support. I do take your point, though - Andrew's objections should have been reason enough to name my patches another way. I don't want to take credit for Andrew's work, though, since very little of substance has actually been changed. I can understand why his remarks would give the impression that this is some kind of rewrite, but they mostly applied to my removal of numeric tracking of non-NULL values. He won that argument, so that's now irrelevant. I must also admit that I am somewhat annoyed here, since Andrew has questioned essentially ever revision I've proposed to both of the sort support patches he wrote, and in a rather bellicose way. They were mostly very modest revisions. -- Peter Geoghegan
On Mon, Mar 23, 2015 at 12:52 PM, Robert Haas <robertmhaas@gmail.com> wrote: > As > + * with IEEE-754, we use an exponent without a sign (a 7-bit exponent > + * without a sign). > > As to the beginning of this sentence, bringing IEEE-754 into this > discussion doesn't clarify anything in my mind. I don't think most > people reading these comments are likely to be familiar with IEEE-754, > or want to go look it up. As to the end of the sentence, writing "an > exponent without a sign" and then describing that as "a 7-bit exponent > without a sign" is extremely redundant. Perhaps you were trying to > say that we are similar to IEEE-754 in that we use an exponent without > a sign (whatever that means) but different in that we ours is 7-bits, > but it's not really clear. The IEEE-754 exponent does not have a signedness bit, but can still be "logically" negative (while still being manipulated like an unsigned integer for addition and subtraction type purposes). That's why it uses what they call an exponent bias - excess-127, for single precision floats, and excess-1023 for double precision floats. Once you do go and Google that, you can find many illustrative diagrams and so on. Andrew's term "excess-44", on the other hand, shows no relevant pages on the first page of Google results (you have to search for "excess-k" to get any hint of what that even is). Regardless of anything else, I really don't think that Andrew's non-explanation of the much more obscure "excess-44" is okay. > I offer these not in the spirit of asking you to correct these > specific things but just of explaining generally the sort of thing > that causes me to prefer Andrew's version. Hope that's helpful. I think that you make some valid points. Thank you. -- Peter Geoghegan
>>>>> "Peter" == Peter Geoghegan <pg@heroku.com> writes: Peter> As I said, I don't really consider that my patch is a rewrite,Peter> especially V4, which changes nothing substantiveexcept removingPeter> 32-bit support. Well, that's a hell of an "except". Here's my main arguments for why 32bit support should be kept: 1. It exists and works well (and yes, I have tested it). 2. This optimization is a huge win even on very small data sets. On sorts of as few as 100 items it gives detectable (on the order of +50%) improvements. On 1000 items the speedup can easily be 3 times. So it's not just people with big data who want this; even small databases will benefit. 3. Keeping the 32bit support (and desupporting DEC_DIGITS != 4) makes it unnecessary to have #ifdefs that disable the numeric abbreviation entirely. (You don't even need those for comparative performance testing; easier to do that by tweaking the catalogs.) As against that, you have the fact that it's ~70 lines of code in one self-contained function which is 32bit-specific. So what do other people think? -- Andrew (irc:RhodiumToad)
On Mon, Mar 23, 2015 at 2:41 PM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: > Peter> As I said, I don't really consider that my patch is a rewrite, > Peter> especially V4, which changes nothing substantive except removing > Peter> 32-bit support. > > Well, that's a hell of an "except". I guess you're right. I'm willing to go with whatever the consensus is on that question. -- Peter Geoghegan
>>>>> "Peter" == Peter Geoghegan <pg@heroku.com> writes: Peter> Your V3 has obsolete comments here: Peter> + nss = palloc(sizeof(NumericSortSupport));Peter> +Peter> + /*Peter> + * palloc a buffer for handling unaligned packedvalues in addition toPeter> + * the support structPeter> + */Peter> + nss->buf = palloc(VARATT_SHORT_MAX + VARHDRSZ+ 1); I don't see why it's obsolete; it still describes what the code is doing, even though the buffer is no longer contiguous with the support struct. The code makes it clear that the buffer is separate. Peter> I still don't think you should be referencing the text opclassPeter> behavior here: Consider it a fence against people trying to change the code for "consistency". Peter> This is dubious: Peter> +#if DEC_DIGITS != 4Peter> +#error "Numeric bases other than 10000 are no longer supported"Peter> +#endif Peter> Because there is a bunch of code within numeric.c that dealsPeter> with the DEC_DIGITS != 4 case. For example, thiscode has beenPeter> within numeric.c forever: The earlier comment should make it clear that all the DEC_DIGITS != 4 support is "historical". I didn't consider it appropriate to actually rip out all the #ifs; I simply tried to make it clear where the landmines were if anyone wanted to try experimenting with other DEC_DIGITS values. Peter> I tend to think that when Tom wrote this code back in 2003, hePeter> thought it might be useful to change DEC_DIGITSon certainPeter> builds. And so, we ought to continue to support it to the extentPeter> that we already do, Right, but we really don't support it in any meaningful sense. The base-10000 version was added for 7.4, which was also the first version with protocol-3 support. Since the V3 protocol has usable support for binary mode, people since then have been writing interfaces on the assumption that the binary representation for numerics is base-10000. Since there's no good way to query the server for the value of DEC_DIGITS (and so clients don't try), this means that changing it breaks compatibility. -- Andrew (irc:RhodiumToad)
On Mon, Mar 23, 2015 at 4:08 PM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: > The earlier comment should make it clear that all the DEC_DIGITS != 4 > support is "historical". I didn't consider it appropriate to actually > rip out all the #ifs; I simply tried to make it clear where the > landmines were if anyone wanted to try experimenting with other > DEC_DIGITS values. There is exactly one landmine: this patch. I really don't understand why you're so resistant to linking the DEC_DIGITS != 4 to it failing. After all, your V1 linked it to abbreviation silently not being enabled when numeric.c was built. -- Peter Geoghegan
On Mon, Mar 23, 2015 at 5:41 PM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: >>>>>> "Peter" == Peter Geoghegan <pg@heroku.com> writes: > > Peter> As I said, I don't really consider that my patch is a rewrite, > Peter> especially V4, which changes nothing substantive except removing > Peter> 32-bit support. > > Well, that's a hell of an "except". > > Here's my main arguments for why 32bit support should be kept: > > 1. It exists and works well (and yes, I have tested it). > > 2. This optimization is a huge win even on very small data sets. On > sorts of as few as 100 items it gives detectable (on the order of +50%) > improvements. On 1000 items the speedup can easily be 3 times. So it's > not just people with big data who want this; even small databases will > benefit. > > 3. Keeping the 32bit support (and desupporting DEC_DIGITS != 4) makes it > unnecessary to have #ifdefs that disable the numeric abbreviation > entirely. (You don't even need those for comparative performance > testing; easier to do that by tweaking the catalogs.) > > As against that, you have the fact that it's ~70 lines of code in one > self-contained function which is 32bit-specific. > > So what do other people think? I agree with you. Fewer and fewer people are running 32-bit systems these days, but there must surely be more people running 32-bit systems than there are running with DEC_DIGITS != 4. I think it's a stretch to say that DEC_DIGITS != 4 is "supported" in any meaningful sense, so I don't think de-supporting it is an issue. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Mar 23, 2015 at 5:47 PM, Robert Haas <robertmhaas@gmail.com> wrote: > I agree with you. Fewer and fewer people are running 32-bit systems > these days, but there must surely be more people running 32-bit > systems than there are running with DEC_DIGITS != 4. I think it's a > stretch to say that DEC_DIGITS != 4 is "supported" in any meaningful > sense, so I don't think de-supporting it is an issue. Of course Andrew's analysis is correct...very few people are building with DEC_DIGITS != 4. Maybe zero. That's beside the point, IMV, which is that it's less invasive to just keep the code the way it is. Desupporting DEC_DIGITS != 4, by making the code break in a general way, without reference to this patch, seems misguided. I would like the build to break in a way that makes the customizer of numeric.c realize that they can disable abbreviation manually too, and still build with DEC_DIGITS != 4. Otherwise, you better remove all the existing specialized DEC_DIGITS != 4 code, of which there is a fair bit. I don't think it makes sense to call that code "historical". -- Peter Geoghegan
On Mon, Mar 23, 2015 at 4:03 PM, Peter Geoghegan <pg@heroku.com> wrote: > I must also admit that I am somewhat annoyed here, since Andrew has > questioned essentially ever revision I've proposed to both of the sort > support patches he wrote, and in a rather bellicose way. They were > mostly very modest revisions. Yep, both of you are questioning each other's work, and both of you are expressing your opposition to the other person's ideas in ways that are fairly forceful, and I think it's fair to say that you are both annoyed. Believe me, I've been there. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Mar 23, 2015 at 8:54 PM, Peter Geoghegan <pg@heroku.com> wrote: > On Mon, Mar 23, 2015 at 5:47 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> I agree with you. Fewer and fewer people are running 32-bit systems >> these days, but there must surely be more people running 32-bit >> systems than there are running with DEC_DIGITS != 4. I think it's a >> stretch to say that DEC_DIGITS != 4 is "supported" in any meaningful >> sense, so I don't think de-supporting it is an issue. > > Of course Andrew's analysis is correct...very few people are building > with DEC_DIGITS != 4. Maybe zero. That's beside the point, IMV, which > is that it's less invasive to just keep the code the way it is. Well, not committing the patch at all would be even less invasive. But that's true of any patch, so I don't think being less invasive can be the prime goal. Of course it's usually better to be less invasive and get the same benefits, but when being less invasive means getting fewer benefits, the additional invasiveness has to be weighed against what you get out of it. > Desupporting DEC_DIGITS != 4, by making the code break in a general > way, without reference to this patch, seems misguided. I would like > the build to break in a way that makes the customizer of numeric.c > realize that they can disable abbreviation manually too, and still > build with DEC_DIGITS != 4. Otherwise, you better remove all the > existing specialized DEC_DIGITS != 4 code, of which there is a fair > bit. I don't think it makes sense to call that code "historical". That's a false dichotomy. We have a bunch of code lying around with #ifdef NOT_USED around it, and that's not intended to imply that you can build with -DNOT_USED. I admit to having not looked at the patch yet, so I may have a clearer position on exactly what to do about this once I've done that. But, as a general principle, I don't accept that we must either keep the DEC_DIGITS != 4 case working in its entirety or remove it completely. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Mon, Mar 23, 2015 at 6:02 PM, Robert Haas <robertmhaas@gmail.com> wrote: > Well, not committing the patch at all would be even less invasive. > But that's true of any patch, so I don't think being less invasive can > be the prime goal. Of course it's usually better to be less invasive > and get the same benefits, but when being less invasive means getting > fewer benefits, the additional invasiveness has to be weighed against > what you get out of it. I agree with that principle. But desupporting DEC_DIGITS != 4 as Andrew proposed gives no clue to how it can be worked around should someone want DEC_DIGITS != 4, as was once anticipated. Whereas a simple static assertion gives us that flexibility, with two lines of code, and without either removing or rendering entirely dead considerable swathes of numeric.c. You can argue that the code was dead anyway, but Tom didn't seem to feel that way when he wrote it. Why mess with that? There is no benefit to doing so. -- Peter Geoghegan
On Mon, Mar 23, 2015 at 9:12 PM, Peter Geoghegan <pg@heroku.com> wrote: > On Mon, Mar 23, 2015 at 6:02 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> Well, not committing the patch at all would be even less invasive. >> But that's true of any patch, so I don't think being less invasive can >> be the prime goal. Of course it's usually better to be less invasive >> and get the same benefits, but when being less invasive means getting >> fewer benefits, the additional invasiveness has to be weighed against >> what you get out of it. > > I agree with that principle. But desupporting DEC_DIGITS != 4 as > Andrew proposed gives no clue to how it can be worked around should > someone want DEC_DIGITS != 4, as was once anticipated. Whereas a > simple static assertion gives us that flexibility, with two lines of > code, and without either removing or rendering entirely dead > considerable swathes of numeric.c. You can argue that the code was > dead anyway, but Tom didn't seem to feel that way when he wrote it. > Why mess with that? There is no benefit to doing so. I'll wait to comment on this until I have a few minutes to read TFP. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sun, Mar 22, 2015 at 1:01 PM, Andrew Gierth
<andrew@tao11.riddles.org.uk> wrote:
> So here's a version 3 patch:
By the way, there was another bug in this that I forgot to point out,
but removed, here:
+ if (nss->estimating)
+ {
+ uint32 tmp = (uint32)result;
+ addHyperLogLog(&nss->abbr_card, hash_uint32(tmp));
+ }
And here:
+ if (nss->estimating)
+ {
+ uint32 tmp = ((uint32)result) ^ (uint32)((uint64) result >> 32);
+ addHyperLogLog(&nss->abbr_card, hash_uint32(tmp));
+ }
(I simply operate on the raw Datum when hashing for hyperLogLog, in a
similar manner to the text opclass, which is safe with 8 byte datums +
pass by value int64).
The problem here is that you're not calling hash_uint32() through
DatumGetUInt32() - hash_uint32() returns Datum. And yes, that matters
- see commit 2c22afaa.
You also need to indent this code correctly.
--
Peter Geoghegan
>>>>> "Peter" == Peter Geoghegan <pg@heroku.com> writes:
Peter> By the way, there was another bug in this that I forgot to pointPeter> out, but removed, here:
"removed"? looks just the same in either of your patches...
Peter> + if (nss->estimating)Peter> + {Peter> + uint32 tmp = (uint32)result;Peter> + addHyperLogLog(&nss->abbr_card,
hash_uint32(tmp));Peter>+ }
Yes, that should have DatumGetUInt32() around the hash_uint32, thanks
Peter> (I simply operate on the raw Datum when hashing for hyperLogLog,Peter> in a similar manner to the text opclass,
whichis safe with 8Peter> byte datums + pass by value int64).
But this paragraph makes no sense, and the code currently in varlena.c
is just as wrong in its usage of hash_uint32 as I was.
--
Andrew (irc:RhodiumToad)
On Mon, Mar 23, 2015 at 10:36 PM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: > Peter> (I simply operate on the raw Datum when hashing for hyperLogLog, > Peter> in a similar manner to the text opclass, which is safe with 8 > Peter> byte datums + pass by value int64). > > But this paragraph makes no sense, and the code currently in varlena.c > is just as wrong in its usage of hash_uint32 as I was. Ugh, yes, not all varlena.c calls to hash_uint32() do the right thing. But then, a number of existing ones don't. I'll need to make a pass over this soon. -- Peter Geoghegan
So here's the latest (and, hopefully, last) version: - adds diagnostic output from numeric_abbrev_abort using the trace_sort GUC - fixed Datum cs. uint32 issues in hash_uint32 - added a short comment about excess-k representation - tweaked the indenting and comments a bit I'm not particularly committed to any specific way of handling the DEC_DIGITS issue. (I moved away from the "transparently skip abbreviations" approach of the original because it seemed that reducing #ifdefism in the code was a desirable feature.) The INT64_MIN/MAX changes should be committed fairly soon. (I haven't posted a patch for TRACE_SORT) -- Andrew (irc:RhodiumToad)
Вложения
On Mon, Mar 23, 2015 at 09:41:40PM +0000, Andrew Gierth wrote: > >>>>> "Peter" == Peter Geoghegan <pg@heroku.com> writes: > > Peter> As I said, I don't really consider that my patch is a rewrite, > Peter> especially V4, which changes nothing substantive except removing > Peter> 32-bit support. > > Well, that's a hell of an "except". > > Here's my main arguments for why 32bit support should be kept: > > 1. It exists and works well (and yes, I have tested it). > > 2. This optimization is a huge win even on very small data sets. On > sorts of as few as 100 items it gives detectable (on the order of +50%) > improvements. On 1000 items the speedup can easily be 3 times. So it's > not just people with big data who want this; even small databases will > benefit. > > 3. Keeping the 32bit support (and desupporting DEC_DIGITS != 4) makes it > unnecessary to have #ifdefs that disable the numeric abbreviation > entirely. (You don't even need those for comparative performance > testing; easier to do that by tweaking the catalogs.) > > As against that, you have the fact that it's ~70 lines of code in one > self-contained function which is 32bit-specific. > > So what do other people think? > +1 for including 32-bit support as well. This is a tremendous performance increase and users of older systems will benefit, and should benefit. Regards, Ken
On Tue, Mar 24, 2015 at 12:03 AM, Andrew Gierth
<andrew@tao11.riddles.org.uk> wrote:
> So here's the latest (and, hopefully, last) version:
>
> - adds diagnostic output from numeric_abbrev_abort using the trace_sort
> GUC
>
> - fixed Datum cs. uint32 issues in hash_uint32
>
> - added a short comment about excess-k representation
>
> - tweaked the indenting and comments a bit
You still pointlessly check memtupcount here:
+ if (memtupcount < 10000 || nss->input_count < 10000 || !nss->estimating)
+ return false;
I still don't like this comment:
+ nss = palloc(sizeof(NumericSortSupport));
+
+ /*
+ * palloc a buffer for handling unaligned packed values in addition to
+ * the support struct
+ */
+ nss->buf = palloc(VARATT_SHORT_MAX + VARHDRSZ + 1);
This cast to void is unnecessary:
+numeric_fast_cmp(Datum x, Datum y, SortSupport ssup)
+{
+ Numeric nx = DatumGetNumeric(x);
+ Numeric ny = DatumGetNumeric(y);
+ int result;
+
+ (void) ssup;
Please try and at least consider my feedback. I don't expect you to do
exactly what I ask, but I also don't expect you to blithely ignore it.
> I'm not particularly committed to any specific way of handling the
> DEC_DIGITS issue. (I moved away from the "transparently skip
> abbreviations" approach of the original because it seemed that reducing
> #ifdefism in the code was a desirable feature.)
Good.
> The INT64_MIN/MAX changes should be committed fairly soon. (I haven't
> posted a patch for TRACE_SORT)
I wouldn't assume that.
--
Peter Geoghegan
>>>>> "Peter" == Peter Geoghegan <pg@heroku.com> writes: Peter> You still pointlessly check memtupcount here: Peter> + if (memtupcount < 10000 || nss->input_count < 10000 || !nss->estimating)Peter> + return false; It's in a register; the test is free. Peter> This cast to void is unnecessary: Peter> + (void) ssup; It's an explicit statement that the parameter is otherwise unused. Maybe that compiler warning isn't usually on by default, but I personally regard it as good style to be explicit about it. Peter> Please try and at least consider my feedback. I don't expect youPeter> to do exactly what I ask, but I also don'texpect you toPeter> blithely ignore it. You should really stop digging this hole deeper. >> The INT64_MIN/MAX changes should be committed fairly soon. (I>> haven't posted a patch for TRACE_SORT) Peter> I wouldn't assume that. Oh ye of little faith. I would not have said that had I not already been informed of it by a committer, and indeed it is now committed. -- Andrew (irc:RhodiumToad)
On Wed, Mar 25, 2015 at 6:26 PM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: >>>>>> "Peter" == Peter Geoghegan <pg@heroku.com> writes: > > Peter> You still pointlessly check memtupcount here: > > Peter> + if (memtupcount < 10000 || nss->input_count < 10000 || !nss->estimating) > Peter> + return false; > > It's in a register; the test is free. Unbelievable! The test is "free", and you wrote it that way in the first place, so we should just leave it that way rather than changing it to suit you? As things stand, the test is totally misleading. In fact, I think it was the source of my initial confusion about the way you track non-null tuple count separately. I certainly didn't have any clue from comments. Apart from fixing this, I added some comments explaining that in my version, but you didn't make any effort to follow that either. > Peter> This cast to void is unnecessary: > > Peter> + (void) ssup; > > It's an explicit statement that the parameter is otherwise unused. > Maybe that compiler warning isn't usually on by default, but I > personally regard it as good style to be explicit about it. Your personal preferences don't enter into it, since every sort support routine since 2011 has followed that style (most still don't touch the sortsupport object). That's the way things work around here - consistency matters. If the existing convention is wrong, we fix the existing convention. Why do you get to ignore well established practice like this? Are you special? > Peter> Please try and at least consider my feedback. I don't expect you > Peter> to do exactly what I ask, but I also don't expect you to > Peter> blithely ignore it. > > You should really stop digging this hole deeper. You have that backwards. > >> The INT64_MIN/MAX changes should be committed fairly soon. (I > >> haven't posted a patch for TRACE_SORT) > > Peter> I wouldn't assume that. > > Oh ye of little faith. I would not have said that had I not already been > informed of it by a committer, and indeed it is now committed. You could have said so. -- Peter Geoghegan
On Mon, Mar 23, 2015 at 9:46 PM, Peter Geoghegan <pg@heroku.com> wrote: > On Mon, Mar 23, 2015 at 2:41 PM, Andrew Gierth > <andrew@tao11.riddles.org.uk> wrote: >> Peter> As I said, I don't really consider that my patch is a rewrite, >> Peter> especially V4, which changes nothing substantive except removing >> Peter> 32-bit support. >> >> Well, that's a hell of an "except". > > > I guess you're right. I'm willing to go with whatever the consensus is > on that question. So I changed my mind - I now think we should support 32-bit abbreviation. You might wonder why I've changed my mind so suddenly. It's not because I started caring about 32-bit systems overnight. For the record, I still think that they are almost irrelevant. But I've seen a new angle. One of the likely future uses for abbreviated keys is to guide B-Tree index scans. One technique that looks really interesting is storing an abbreviated key in internal pages. I always knew that abbreviation is as useful for index scans as it is for sorting - maybe more so. Reading "Modern B-Tree techniques" [1] again today, I realized that we can store fixed sized abbreviated keys in line items directly. If we stored a 4 byte abbreviated key, we'd pay no storage overhead on 64-bit systems that already lose that to ItemId alignment. We'd only generate abbreviated keys in internal B-Tree pages, during relatively infrequent page splits (internal pages naturally have a very wide spread of values anyway), so that would be a very low cost. Even when abbreviation doesn't work out, we have to visit the line item anyway, and an integer comparison on the same cacheline is effectively free. It would probably work out all the time anyway, owing to the natural spread of items within internal pages. Best of all, most index scans don't even need to look past the itemId array (for internal pages, which are the large majority visited by any given index scan, but a tiny minority of those actually stored), hugely cutting down on the number of cache misses. I could see this making index scans on numeric IndexTuples noticeably faster than even those on int8 IndexTuples. Of course, text is the type that this is really compelling for (perhaps int4 too - perhaps we could automatically get this for types that fit in 4 bytes naturally on 64-bit platforms). I'm not sure that we could do this with text without adopting ICU, which makes firm guarantees about binary sort key stability, so I thought that numeric could be considered a proof of concept for that. [1] http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.219.7269&rep=rep1&type=pdf -- Peter Geoghegan
On Tue, Mar 24, 2015 at 3:03 AM, Andrew Gierth
<andrew@tao11.riddles.org.uk> wrote:
> So here's the latest (and, hopefully, last) version:
>
> - adds diagnostic output from numeric_abbrev_abort using the trace_sort
> GUC
>
> - fixed Datum cs. uint32 issues in hash_uint32
>
> - added a short comment about excess-k representation
>
> - tweaked the indenting and comments a bit
>
> I'm not particularly committed to any specific way of handling the
> DEC_DIGITS issue. (I moved away from the "transparently skip
> abbreviations" approach of the original because it seemed that reducing
> #ifdefism in the code was a desirable feature.)
>
> The INT64_MIN/MAX changes should be committed fairly soon. (I haven't
> posted a patch for TRACE_SORT)
I think this is really nice work, so I have committed this version. I
made a few fairly minor changes, hopefully without breaking anything
in the process:
- I adjusted things for recent commits around INT{32,63}_{MIN_MAX}.
- I removed (void) ssup; which I do not think is normal PostgreSQL style.
- Removed the #if DEC_DIGITS != 4 check. The comment is great, but I
think we don't need protect against #if 0 code get re-enabled.
- I removed the too-clever (at least IMHO) handing of TRACE_SORT in
favor of just using #ifdef around each occurrence.
- I also declared trace_sort in guc.h, where various other GUCs are
declared, instead of declaring it privately in each file that needed
it.
- Changed some definitions to depend on SIZEOF_DATUM rather than
USE_FLOAT8_BYVAL. Hopefully I didn't muff this; please check it.
- Fixed an OID conflict.
- And of course, bumped catversion.
Thanks,
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Thu, Apr 2, 2015 at 2:07 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> I think this is really nice work, so I have committed this version. I
> made a few fairly minor changes, hopefully without breaking anything
> in the process:
>
> - I adjusted things for recent commits around INT{32,63}_{MIN_MAX}.
> - I removed (void) ssup; which I do not think is normal PostgreSQL style.
> - Removed the #if DEC_DIGITS != 4 check. The comment is great, but I
> think we don't need protect against #if 0 code get re-enabled.
> - I removed the too-clever (at least IMHO) handing of TRACE_SORT in
> favor of just using #ifdef around each occurrence.
> - I also declared trace_sort in guc.h, where various other GUCs are
> declared, instead of declaring it privately in each file that needed
> it.
> - Changed some definitions to depend on SIZEOF_DATUM rather than
> USE_FLOAT8_BYVAL. Hopefully I didn't muff this; please check it.
> - Fixed an OID conflict.
> - And of course, bumped catversion.
And that's broken the buildfarm. Argh.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 02/04/15 21:21, Robert Haas wrote:
> On Thu, Apr 2, 2015 at 2:07 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> I think this is really nice work, so I have committed this version. I
>> made a few fairly minor changes, hopefully without breaking anything
>> in the process:
>>
>> - I adjusted things for recent commits around INT{32,63}_{MIN_MAX}.
>> - I removed (void) ssup; which I do not think is normal PostgreSQL style.
>> - Removed the #if DEC_DIGITS != 4 check. The comment is great, but I
>> think we don't need protect against #if 0 code get re-enabled.
>> - I removed the too-clever (at least IMHO) handing of TRACE_SORT in
>> favor of just using #ifdef around each occurrence.
>> - I also declared trace_sort in guc.h, where various other GUCs are
>> declared, instead of declaring it privately in each file that needed
>> it.
>> - Changed some definitions to depend on SIZEOF_DATUM rather than
>> USE_FLOAT8_BYVAL. Hopefully I didn't muff this; please check it.
>> - Fixed an OID conflict.
>> - And of course, bumped catversion.
>
> And that's broken the buildfarm. Argh.
>
That's because of this:
+#ifdef SIZEOF_DATUM == 8
You need just #if there.
-- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services
On Thu, Apr 2, 2015 at 4:21 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:
> On 02/04/15 21:21, Robert Haas wrote:
>> On Thu, Apr 2, 2015 at 2:07 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>>
>>> I think this is really nice work, so I have committed this version. I
>>> made a few fairly minor changes, hopefully without breaking anything
>>> in the process:
>>>
>>> - I adjusted things for recent commits around INT{32,63}_{MIN_MAX}.
>>> - I removed (void) ssup; which I do not think is normal PostgreSQL style.
>>> - Removed the #if DEC_DIGITS != 4 check. The comment is great, but I
>>> think we don't need protect against #if 0 code get re-enabled.
>>> - I removed the too-clever (at least IMHO) handing of TRACE_SORT in
>>> favor of just using #ifdef around each occurrence.
>>> - I also declared trace_sort in guc.h, where various other GUCs are
>>> declared, instead of declaring it privately in each file that needed
>>> it.
>>> - Changed some definitions to depend on SIZEOF_DATUM rather than
>>> USE_FLOAT8_BYVAL. Hopefully I didn't muff this; please check it.
>>> - Fixed an OID conflict.
>>> - And of course, bumped catversion.
>>
>>
>> And that's broken the buildfarm. Argh.
>
> That's because of this:
>
> +#ifdef SIZEOF_DATUM == 8
>
> You need just #if there.
Thanks. I actually pushed a fix for that about 25 minutes ago;
hopefully that is all that is needed.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 02/04/15 22:22, Robert Haas wrote:
> On Thu, Apr 2, 2015 at 4:21 PM, Petr Jelinek <petr@2ndquadrant.com> wrote:
>> On 02/04/15 21:21, Robert Haas wrote:
>>> On Thu, Apr 2, 2015 at 2:07 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>>>
>>>> I think this is really nice work, so I have committed this version. I
>>>> made a few fairly minor changes, hopefully without breaking anything
>>>> in the process:
>>>>
>>>> - I adjusted things for recent commits around INT{32,63}_{MIN_MAX}.
>>>> - I removed (void) ssup; which I do not think is normal PostgreSQL style.
>>>> - Removed the #if DEC_DIGITS != 4 check. The comment is great, but I
>>>> think we don't need protect against #if 0 code get re-enabled.
>>>> - I removed the too-clever (at least IMHO) handing of TRACE_SORT in
>>>> favor of just using #ifdef around each occurrence.
>>>> - I also declared trace_sort in guc.h, where various other GUCs are
>>>> declared, instead of declaring it privately in each file that needed
>>>> it.
>>>> - Changed some definitions to depend on SIZEOF_DATUM rather than
>>>> USE_FLOAT8_BYVAL. Hopefully I didn't muff this; please check it.
>>>> - Fixed an OID conflict.
>>>> - And of course, bumped catversion.
>>>
>>>
>>> And that's broken the buildfarm. Argh.
>>
>> That's because of this:
>>
>> +#ifdef SIZEOF_DATUM == 8
>>
>> You need just #if there.
>
> Thanks. I actually pushed a fix for that about 25 minutes ago;
> hopefully that is all that is needed.
>
Ok, the git.pg.org was somewhat behind. It did fix it for me when I
tested it locally.
-- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services
On Thu, Apr 2, 2015 at 4:24 PM, Petr Jelinek <petr@2ndquadrant.com> wrote: >> Thanks. I actually pushed a fix for that about 25 minutes ago; >> hopefully that is all that is needed. > > Ok, the git.pg.org was somewhat behind. It did fix it for me when I tested > it locally. OK, that's good to know. So far the buildfarm looks happy too, but I'll try to keep an eye on it at least for the next hour or so. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
>>>>> "Robert" == Robert Haas <robertmhaas@gmail.com> writes:
Robert> I think this is really nice work, so I have committed thisRobert> version. I made a few fairly minor changes,
hopefullywithoutRobert> breaking anything in the process:
Robert> - I adjusted things for recent commits around INT{32,63}_{MIN_MAX}.Robert> - I removed (void) ssup; which I do
notthink is normal PostgreSQL style.Robert> - Removed the #if DEC_DIGITS != 4 check. The comment is great, but
IRobert>think we don't need protect against #if 0 code get re-enabled.
No complaints in principle for any of these
Robert> - I removed the too-clever (at least IMHO) handing of TRACE_SORT inRobert> favor of just using #ifdef around
eachoccurrence.
My idea here - which I have admittedly not got round to posting a patch
for yet - was that TRACE_SORT (which has been defined by default since
8.1) should be deprecated in favour of unconditional use of trace_sort.
(The actual definition of TRACE_SORT could be kept in case any extension
or whatever uses it)
Robert> - I also declared trace_sort in guc.h, where various other GUCsRobert> are declared, instead of declaring it
privatelyin each fileRobert> that needed it.
I was thinking miscadmin.h but whatever.
Robert> - Changed some definitions to depend on SIZEOF_DATUM ratherRobert> than USE_FLOAT8_BYVAL. Hopefully I didn't
muffthis; pleaseRobert> check it.
No, that's wrong; it must use USE_FLOAT8_BYVAL since that's what the
definitions of Int64GetDatum / DatumGetInt64 are conditional on. As you
have it now, it'll break if you build with --disable-float8-byval on a
64bit platform.
--
Andrew (irc:RhodiumToad)
On Thu, Apr 2, 2015 at 10:41 PM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: > Robert> - Changed some definitions to depend on SIZEOF_DATUM rather > Robert> than USE_FLOAT8_BYVAL. Hopefully I didn't muff this; please > Robert> check it. > > No, that's wrong; it must use USE_FLOAT8_BYVAL since that's what the > definitions of Int64GetDatum / DatumGetInt64 are conditional on. As you > have it now, it'll break if you build with --disable-float8-byval on a > 64bit platform. I'd prefer to avoid that by avoiding the use of those macros in this code path rather than by leaving the top 32 bits of the Datum unused in that configuration. I tried to get there by using a cast for DatumGetNumericAbbrev(), but I forgot to update the NAN case, so at least this much is probably needed: --- a/src/backend/utils/adt/numeric.c +++ b/src/backend/utils/adt/numeric.c @@ -299,10 +299,10 @@ typedef struct#define NUMERIC_ABBREV_BITS (SIZEOF_DATUM * BITS_PER_BYTE)#if SIZEOF_DATUM == 8#defineDatumGetNumericAbbrev(d) ((int64) d) -#define NUMERIC_ABBREV_NAN Int64GetDatum(PG_INT64_MIN) +#define NUMERIC_ABBREV_NAN ((Datum) PG_INT64_MIN)#else#define DatumGetNumericAbbrev(d) ((int32) d) -#define NUMERIC_ABBREV_NAN Int32GetDatum(PG_INT32_MIN) +#define NUMERIC_ABBREV_NAN ((Datum) PG_INT32_MIN)#endif What other risks do you see? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
>>>>> "Robert" == Robert Haas <robertmhaas@gmail.com> writes: >> No, that's wrong; it must use USE_FLOAT8_BYVAL since that's what the>> definitions of Int64GetDatum / DatumGetInt64 areconditional on. As>> you have it now, it'll break if you build with>> --disable-float8-byval on a 64bit platform. Robert> I'd prefer to avoid that by avoiding the use of those macros inRobert> this code path rather than by leaving thetop 32 bits of theRobert> Datum unused I don't consider it appropriate to override ./configure in this way. -- Andrew (irc:RhodiumToad)
On Fri, Apr 3, 2015 at 9:06 AM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: >>>>>> "Robert" == Robert Haas <robertmhaas@gmail.com> writes: > >> No, that's wrong; it must use USE_FLOAT8_BYVAL since that's what the > >> definitions of Int64GetDatum / DatumGetInt64 are conditional on. As > >> you have it now, it'll break if you build with > >> --disable-float8-byval on a 64bit platform. > > Robert> I'd prefer to avoid that by avoiding the use of those macros in > Robert> this code path rather than by leaving the top 32 bits of the > Robert> Datum unused > > I don't consider it appropriate to override ./configure in this way. I don't see how that's overriding configure. Commit 8472bf7a73487b0535c95e299773b882f7523463, which introduced --disable-float8-byval in 2008, states that the motivation for this is that it might break functions using the version 0 calling convention. It should not propagate, like kudzu, into things that having nothing to do with how values are passed to V0 functions. And as far as I can see, the algorithm that we use to create an abbreviated key for numeric is entirely decoupled from that question, because none of the datums were are building here will ever get passed to one. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
>>>>> "Robert" == Robert Haas <robertmhaas@gmail.com> writes: >> I don't consider it appropriate to override ./configure in this way. Robert> I don't see how that's overriding configure. CommitRobert> 8472bf7a73487b0535c95e299773b882f7523463, which introducedRobert>--disable-float8-byval in 2008, states that the motivation forRobert> this is that it might break functionsusing the version 0Robert> calling convention. It should not propagate, like kudzu, intoRobert> things that havingnothing to do with how values are passed toRobert> V0 functions. It already does; it changes how int64 values are expected to be stored in Datum variables. _Everything_ that currently stores an int64 value in a Datum is affected. As I see it, you need a really good reason to override that in a specific case, and supporting 64-bit abbreviations on a --disable-float8-byval build really isn't a good reason (since 32-bit abbrevs work fine and such builds should be vanishingly rare anyway). The fact that making this one low-benefit change has introduced no less than three separate bugs - the compile error with that #ifdef, the use of Int64GetDatum for NANs, and the use of Int64GetDatum for the return value of the abbreviation function should possibly be taken as a hint to how bad an idea is. If you're determined to go this route - over my protest - then you need to do something like define a NumericAbbrevGetDatum(x) macro and use it in place of the Int64GetDatum / Int32GetDatum ones for both NAN and the return from numeric_abbrev_convert_var. -- Andrew (irc:RhodiumToad)
On Fri, Apr 3, 2015 at 1:39 PM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: > It already does; it changes how int64 values are expected to be stored > in Datum variables. _Everything_ that currently stores an int64 value in > a Datum is affected. But this isn't a value of the SQL type "int64". It's just a bit pattern that has to fit inside however big a Datum happens to be. > As I see it, you need a really good reason to override that in a > specific case, and supporting 64-bit abbreviations on a > --disable-float8-byval build really isn't a good reason (since 32-bit > abbrevs work fine and such builds should be vanishingly rare anyway). In my opinion, there is way too much crap around already just to cater to --disable-float8-byval. I think not adding more is a perfectly reasonable goal. If somebody wants to remove --disable-float8-byval some day, I want to minimize the amount of stuff they have to change in order to do that. I think that keeping this off the list of stuff that will require modification is a worthy goal. > The fact that making this one low-benefit change has introduced no less > than three separate bugs - the compile error with that #ifdef, the use > of Int64GetDatum for NANs, and the use of Int64GetDatum for the return > value of the abbreviation function should possibly be taken as a hint to > how bad an idea is. But all of those are trivial, and the first would have been caught by my compiler if I weren't using such a crappy old compiler. If anything that might require as much as 10 lines of code churn to fix is not worth doing, very little is worth doing. > If you're determined to go this route - over my protest - then you need > to do something like define a NumericAbbrevGetDatum(x) macro and use it > in place of the Int64GetDatum / Int32GetDatum ones for both NAN and the > return from numeric_abbrev_convert_var. Patch for that attached. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Apr 3, 2015 at 1:39 PM, Andrew Gierth
> <andrew@tao11.riddles.org.uk> wrote:
>> If you're determined to go this route - over my protest - then you need
>> to do something like define a NumericAbbrevGetDatum(x) macro and use it
>> in place of the Int64GetDatum / Int32GetDatum ones for both NAN and the
>> return from numeric_abbrev_convert_var.
> Patch for that attached.
FWIW, I think it's sensible to define NumericAbbrevGetDatum and the
converse, but I'd suggest you just do it like
#define NumericAbbrevGetDatum(X) Int64GetDatum(X)
or
#define NumericAbbrevGetDatum(X) Int32GetDatum(X)
I'm not especially a fan of reaching inside the GetDatum macros when
you don't have to. And the code that's calling these certainly knows
that it's supplying an int64 or int32 respectively.
regards, tom lane
I wrote:
> FWIW, I think it's sensible to define NumericAbbrevGetDatum and the
> converse, but I'd suggest you just do it like
> #define NumericAbbrevGetDatum(X) Int64GetDatum(X)
> or
> #define NumericAbbrevGetDatum(X) Int32GetDatum(X)
Oh, scratch that, I'd failed to look at the upthread messages.
regards, tom lane
>>>>> "Robert" == Robert Haas <robertmhaas@gmail.com> writes: >> It already does; it changes how int64 values are expected to be>> stored in Datum variables. _Everything_ that currentlystores an>> int64 value in a Datum is affected. Robert> But this isn't a value of the SQL type "int64". It's just aRobert> bit pattern that has to fit inside however biga Datum happensRobert> to be. It's a bit pattern which is a signed 32-bit or 64-bit integer, computed in an int32 or int64. Using something other than Int32GetDatum / Int64GetDatum for it seems unnecessarily surprising. >> The fact that making this one low-benefit change has introduced no>> less than three separate bugs - the compile errorwith that #ifdef,>> the use of Int64GetDatum for NANs, and the use of Int64GetDatum for>> the return value of the abbreviationfunction should possibly be>> taken as a hint to how bad an idea is. Robert> But all of those are trivial, and the first would have beenRobert> caught by my compiler if I weren't using sucha crappy oldRobert> compiler. If anything that might require as much as 10 linesRobert> of code churn to fix is notworth doing, very little is worthRobert> doing. Trivial maybe, but subtle enough that you missed them (which suggests that others might too), and a --disable-float8-byval build of the buggy version only fails regression by a fluke. (This does rather suggest to me that some better regression tests for sorting would be a good idea, possibly even including on-disk sorts.) >> If you're determined to go this route - over my protest - then you>> need to do something like define a NumericAbbrevGetDatum(x)macro>> and use it in place of the Int64GetDatum / Int32GetDatum ones for>> both NAN and the returnfrom numeric_abbrev_convert_var. Robert> Patch for that attached. That looks reasonable, though I think it could do with a comment explaining _why_ it's defining its own macros rather than using Int32*/Int64*. (And I wrote that before seeing Tom's message, even.) -- Andrew (irc:RhodiumToad)
... btw, has anyone noticed that this patch broke hamerkop and
bowerbird? Or at least, it's hard to see what other recent commit
would explain the failures they're showing.
regards, tom lane
On Fri, Apr 3, 2015 at 5:45 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > ... btw, has anyone noticed that this patch broke hamerkop and > bowerbird? Or at least, it's hard to see what other recent commit > would explain the failures they're showing. I noticed the failure on bowerbird today and I agree that looks an awful lot like it might be this patch's fault. The failure on hamerkop looks like the same issue. I'm a bit at a loss to explain exactly what has gone wrong there. What those machines have in common is that they are the only 64-bit Windows machines using the Microsoft toolchain in the buildfarm, but I don't know why that should provoke a failure. I seem to remember having some trouble previously with Microsoft compilers being finickier than others about a shift with a width greater than or equal to the bit-width of the value, but if there is such a problem here, I can't spot it. Nor do I see a compiler warning in numeric.c which might provide a clue. For those following along at home, the failures are on these queries: SELECT 1.1 AS two UNION SELECT 2.2; SELECT 1.1 AS two UNION SELECT 2; SELECT 1 AS two UNION SELECT 2.2; SELECT 1.1 AS three UNION SELECT 2 UNION ALL SELECT 2; In each case, the expected result is with the values in ascending numerical order. In each case, the 1 or 1.1 value which ought to appear before 2 or 2.2 instead appears after it. Strictly speaking, this is not the wrong answer to the query, and could be perhaps explained by the planner choosing a hash aggregate rather than a sort + unique plan. But this patch doesn't change the planner at all, so the plan should be the same as it has always been. And if it is choosing to sort, then it's clearly doing it wrong, but there doesn't seem to be anything platform-specific about this code, so I'm mystified. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Apr 3, 2015 at 3:46 PM, Andrew Gierth <andrew@tao11.riddles.org.uk> wrote: > (This does rather suggest to me that some better regression tests for > sorting would be a good idea, possibly even including on-disk sorts.) Yeah. I've been unpleasantly surprised by how easy it is to pass the regression tests with sorting broken. > >> If you're determined to go this route - over my protest - then you > >> need to do something like define a NumericAbbrevGetDatum(x) macro > >> and use it in place of the Int64GetDatum / Int32GetDatum ones for > >> both NAN and the return from numeric_abbrev_convert_var. > > Robert> Patch for that attached. > > That looks reasonable, though I think it could do with a comment > explaining _why_ it's defining its own macros rather than using > Int32*/Int64*. (And I wrote that before seeing Tom's message, even.) Agreed. I have added that and committed this. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> For those following along at home, the failures are on these queries:
> SELECT 1.1 AS two UNION SELECT 2.2;
> SELECT 1.1 AS two UNION SELECT 2;
> SELECT 1 AS two UNION SELECT 2.2;
> SELECT 1.1 AS three UNION SELECT 2 UNION ALL SELECT 2;
> In each case, the expected result is with the values in ascending
> numerical order. In each case, the 1 or 1.1 value which ought to
> appear before 2 or 2.2 instead appears after it. Strictly speaking,
> this is not the wrong answer to the query, and could be perhaps
> explained by the planner choosing a hash aggregate rather than a sort
> + unique plan. But this patch doesn't change the planner at all, so
> the plan should be the same as it has always been.
Yeah. We could add an EXPLAIN to make certain, perhaps, but since
none of the other critters are failing I doubt this explanation.
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> For those following along at home, the failures are on these queries: >> SELECT 1.1 AS two UNION SELECT 2.2;>> SELECT 1.1 AS two UNION SELECT 2;>> SELECT 1 AS two UNION SELECT 2.2;>> SELECT 1.1AS three UNION SELECT 2 UNION ALL SELECT 2; >> In each case, the expected result is with the values in ascending>> numerical order. In each case, the 1 or 1.1 valuewhich ought to>> appear before 2 or 2.2 instead appears after it. Strictly speaking,>> this is not the wrong answerto the query, and could be perhaps>> explained by the planner choosing a hash aggregate rather than a sort>> + uniqueplan. But this patch doesn't change the planner at all, so>> the plan should be the same as it has always been. Tom> Yeah. We could add an EXPLAIN to make certain, perhaps, but sinceTom> none of the other critters are failing I doubtthis explanation. This failure in the union.sql test is exactly the one that happens if you build with --disable-float8-byval, for what that's worth. Does the windows build perhaps not define USE_FLOAT8_BYVAL? that would explain the breakage. -- Andrew (irc:RhodiumToad)
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: Tom> ... btw, has anyone noticed that this patch broke hamerkop andTom> bowerbird? Or at least, it's hard to see what otherrecent commitTom> would explain the failures they're showing. Now that Robert committed the fix for 64bit Datum w/o USE_FLOAT8_BYVAL, bowerbird seems fixed (hamerkop hasn't run yet). I see nothing in the win32 stuff that tries to define USE_FLOAT8_BYVAL on 64-bit windows, is this just an oversight or does it not actually work there? or is it for on-disk compatibility with 32-bit windows? -- Andrew (irc:RhodiumToad)
Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes:
> Tom> ... btw, has anyone noticed that this patch broke hamerkop and
> Tom> bowerbird? Or at least, it's hard to see what other recent commit
> Tom> would explain the failures they're showing.
> Now that Robert committed the fix for 64bit Datum w/o USE_FLOAT8_BYVAL,
> bowerbird seems fixed (hamerkop hasn't run yet).
> I see nothing in the win32 stuff that tries to define USE_FLOAT8_BYVAL
> on 64-bit windows, is this just an oversight or does it not actually
> work there? or is it for on-disk compatibility with 32-bit windows?
That flag doesn't affect on-disk compatibility. It could certainly break
third-party extensions, but we accept the same hazard on non-Windows with
equanimity. I suspect this point simply wasn't revisited when we added
support for 64-bit Windows.
Having said that, I'm fine with leaving this as-is, if only because
it means we're exercising the --disable-float8-byval code paths in
the buildfarm ;-)
regards, tom lane
>>>>> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> I see nothing in the win32 stuff that tries to define>> USE_FLOAT8_BYVAL on 64-bit windows, is this just an oversightor>> does it not actually work there? or is it for on-disk compatibility>> with 32-bit windows? Tom> That flag doesn't affect on-disk compatibility. pg_type.typbyval ? But I don't know if anything else differs between 32-bit and 64-bit windows that would impact on compatibility. -- Andrew (irc:RhodiumToad)
On Sat, Apr 4, 2015 at 10:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I see nothing in the win32 stuff that tries to define USE_FLOAT8_BYVAL >> on 64-bit windows, is this just an oversight or does it not actually >> work there? or is it for on-disk compatibility with 32-bit windows? > > That flag doesn't affect on-disk compatibility. It could certainly break > third-party extensions, but we accept the same hazard on non-Windows with > equanimity. I suspect this point simply wasn't revisited when we added > support for 64-bit Windows. > > Having said that, I'm fine with leaving this as-is, if only because > it means we're exercising the --disable-float8-byval code paths in > the buildfarm ;-) But... are we? I mean, I don't see anything in the Windows code that defines USE_FLOAT8_BYVAL. (Admittedly, if we're not, I have no theory for why that patch fixed anything, but all the same I don't see where it's getting defined.) -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 04/04/2015 10:27 AM, Tom Lane wrote: > Andrew Gierth <andrew@tao11.riddles.org.uk> writes: >> "Tom" == Tom Lane <tgl@sss.pgh.pa.us> writes: >> Tom> ... btw, has anyone noticed that this patch broke hamerkop and >> Tom> bowerbird? Or at least, it's hard to see what other recent commit >> Tom> would explain the failures they're showing. >> Now that Robert committed the fix for 64bit Datum w/o USE_FLOAT8_BYVAL, >> bowerbird seems fixed (hamerkop hasn't run yet). >> I see nothing in the win32 stuff that tries to define USE_FLOAT8_BYVAL >> on 64-bit windows, is this just an oversight or does it not actually >> work there? or is it for on-disk compatibility with 32-bit windows? > That flag doesn't affect on-disk compatibility. It could certainly break > third-party extensions, but we accept the same hazard on non-Windows with > equanimity. I suspect this point simply wasn't revisited when we added > support for 64-bit Windows. > > Having said that, I'm fine with leaving this as-is, if only because > it means we're exercising the --disable-float8-byval code paths in > the buildfarm ;-) > > This seems quite wrong. If we want those paths tested we should ensure that buildfarm members are set up with that explicit setting. I think not making this the default for 64 bit MSVC builds was simply an error of omission. The attached patch should set float8byval as the default on 64 bit MSVC builds and error out if it is explicitly set on 32 bit platforms. cheers andrew
Вложения
On 04/04/2015 04:27 PM, Robert Haas wrote: > On Sat, Apr 4, 2015 at 10:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> I see nothing in the win32 stuff that tries to define USE_FLOAT8_BYVAL >>> on 64-bit windows, is this just an oversight or does it not actually >>> work there? or is it for on-disk compatibility with 32-bit windows? >> That flag doesn't affect on-disk compatibility. It could certainly break >> third-party extensions, but we accept the same hazard on non-Windows with >> equanimity. I suspect this point simply wasn't revisited when we added >> support for 64-bit Windows. >> >> Having said that, I'm fine with leaving this as-is, if only because >> it means we're exercising the --disable-float8-byval code paths in >> the buildfarm ;-) > But... are we? I mean, I don't see anything in the Windows code that > defines USE_FLOAT8_BYVAL. > > (Admittedly, if we're not, I have no theory for why that patch fixed > anything, but all the same I don't see where it's getting defined.) > See src/tools/msvc/Solution.pm cheers andrew
On Sat, Apr 4, 2015 at 4:37 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > This seems quite wrong. If we want those paths tested we should ensure that > buildfarm members are set up with that explicit setting. > > I think not making this the default for 64 bit MSVC builds was simply an > error of omission. +1. I don't feel that having every optimization available on Windows is worth bending over backwards for, but this omission is kind of silly. -- Peter Geoghegan
Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, Apr 4, 2015 at 10:27 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Having said that, I'm fine with leaving this as-is, if only because
>> it means we're exercising the --disable-float8-byval code paths in
>> the buildfarm ;-)
> But... are we? I mean, I don't see anything in the Windows code that
> defines USE_FLOAT8_BYVAL.
Precisely --- it *isn't* getting set, even on 64-bit Windows where it
could be.
regards, tom lane
On Apr 4, 2015, at 4:37 PM, Andrew Dunstan <andrew@dunslane.net> wrote: > The attached patch should set float8byval as the default on 64 bit MSVC builds and error out if it is explicitly set on32 bit platforms. +1 for changing this. ...Robert