Re: range_agg

čt 4. 7. 2019 v 20:34 odesílatel Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
napsal:

> I noticed that this patch has a // comment about it segfaulting. Did
> you ever figure that out? Is the resulting code the one you intend as
> final?
>
> Did you make any inroads regarding Jeff Davis' suggestion about
> implementing "multiranges"? I wonder if that's going to end up being a
> Pandora's box.

Introduction of new datatype can be better, because we can ensure so data
are correctly ordered

> Stylistically, the code does not match pgindent's choices very closely.
> I can return a diff to a pgindented version of your v0002 for your
> perusal, if it would be useful for you to learn its style. (A committer
> would eventually run pgindent himself[1], but it's good to have
> submissions be at least close to the final form.) BTW, I suggest "git
> format-patch -vN" to prepare files for submission.
>

The first issue is unstable regress tests - there is a problem with
opr_sanity

SELECT p1.oid, p1.proname, p2.oid, p2.proname
FROM pg_proc AS p1, pg_proc AS p2
WHERE p1.oid < p2.oid AND
p1.prosrc = p2.prosrc AND
p1.prolang = 12 AND p2.prolang = 12 AND
(p1.prokind != 'a' OR p2.prokind != 'a') AND
(p1.prolang != p2.prolang OR
p1.prokind != p2.prokind OR
p1.prosecdef != p2.prosecdef OR
p1.proleakproof != p2.proleakproof OR
p1.proisstrict != p2.proisstrict OR
p1.proretset != p2.proretset OR
p1.provolatile != p2.provolatile OR
p1.pronargs != p2.pronargs)
ORDER BY p1.oid, p2.oid; -- requires explicit ORDER BY now

+ rangeTypeId = get_fn_expr_argtype(fcinfo->flinfo, 1);
+ if (!type_is_range(rangeTypeId))
+ {
+ ereport(ERROR, (errmsg("range_agg must be called with a range")));
+ }

???

+ r1Str = "lastRange";
+ r2Str = "currentRange";
+ // TODO: Why is this segfaulting?:
+ // r1Str =
DatumGetCString(DirectFunctionCall1(range_out,
RangeTypePGetDatum(lastRange)));
+ // r2Str =
DatumGetCString(DirectFunctionCall1(range_out,
RangeTypePGetDatum(currentRange)));
+ ereport(ERROR, (errmsg("range_agg: gap detected between
%s and %s", r1Str, r2Str)));
+ }

???

The patch doesn't respect Postgres formatting

+# Making 2- and 3-param range_agg polymorphic is difficult
+# because it would take an anyrange and return an anyrange[],
+# which doesn't exist.
+# As a workaround we define separate functions for each built-in range
type.
+# This is what causes the mess in src/test/regress/expected/opr_sanity.out.
+{ oid => '8003', descr => 'aggregate transition function',

This is strange. Does it means so range_agg will not work with custom range
types?

I am not sure about implementation. I think so accumulating all ranges,
sorting and processing on the end can be memory and CPU expensive.

Regards

Pavel

>
> [1] No female committers yet ... is this 2019?
>
> --
> Álvaro Herrera https://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>
>