search

tvondra / tdigest

PostgreSQL extension for estimating percentiles using t-digest
PostgreSQL License
88 stars 13 forks source link

Regression tests fail on i386 (and probably other 32-bit) #10

Closed Natureshadow closed 4 years ago

Natureshadow commented 4 years ago

Hi,

the test suite fails on i386:

17:13:13 2020-08-24 15:13:03.329 UTC [6159] postgres@contrib_regression ERROR:  compression for t-digest must be in [10, 10000]
17:13:13 2020-08-24 15:13:03.329 UTC [6159] postgres@contrib_regression STATEMENT:  WITH data AS (SELECT i / 1000000.0 AS x FROM generate_series(1,1000000) s(i)),
17:13:13         intermediate AS (SELECT tdigest(x, 10)::text AS intermediate_x FROM data),
17:13:13         tdigest_parsed AS (SELECT tdigest_percentile(intermediate_x::tdigest, ARRAY[0.01, 0.05, 0.1, 0.9, 0.95, 0.99]) AS a FROM intermediate),
17:13:13         pg_percentile AS (SELECT percentile_cont(ARRAY[0.01, 0.05, 0.1, 0.9, 0.95, 0.99]) WITHIN GROUP (ORDER BY x) AS b FROM data)
17:13:13    SELECT
17:13:13        p,
17:13:13        abs(a - b) < 0.01, 
17:13:13        (CASE WHEN abs(a - b) < 0.01 THEN NULL ELSE (a - b) END) AS err
17:13:13    FROM (
17:13:13        SELECT
17:13:13            unnest(ARRAY[0.01, 0.05, 0.1, 0.9, 0.95, 0.99]) AS p,
17:13:13            unnest(a) AS a,
17:13:13            unnest(b) AS b
17:13:13        FROM tdigest_parsed,
17:13:13             pg_percentile
17:13:13    ) foo;
tvondra commented 4 years ago

That's bizarre - the compression is clearly 10, which is in the required range. Can you investigate a bit? I don't have any i386 environment at hand, unfortunately.

tvondra commented 4 years ago

I've realized I have a raspberry pi 4, which is in fact a 32-bit machine (although armv7l, not i386), and I've been able to reproduce the issue. Turns out it's caused by using incorrect printf format (%ld) to read/print int64 variabled, instead of the INT64_FORMAT.

Should be fixed by 486ca8b2e41264944b2b6d825440170044bacf39.

tvondra commented 4 years ago

@Natureshadow Can you test if this fixes the issue on i386 too?

Natureshadow commented 4 years ago

Yep, that fixes it. Thanks for that learning possibility ;).

A new release would be great, so I can finalise uplaod to apt.postgresql.org and Debian main.

tvondra commented 4 years ago

OK, marking as closed. I'll tag a new release in a couple days.