On 4/4/22 12:33, Andres Freund wrote:
> Hi,
>
> On 2022-04-04 11:54:23 -0400, Greg Stark wrote:
>> Are we missing regression tests using these functions as window functions?
> So far, yes.
>
> ISTM that 4eb97988796 should have at least included the crashing statement as
> a test... The statement can be simpler too:
>
> SELECT json_objectagg(k : v with unique keys) OVER (ORDER BY k) FROM (VALUES (1,1), (2,2)) a(k,v);
>
> is sufficient to trigger the crash for me, without even using asan (after
> reverting the bugfix, of course).
>
I will add some regression tests.
>> Hm. I suppose it's possible to write a general purpose regression test
>> that loops over all aggregate functions and runs them as window
>> functions and aggregates over the same data sets and compares results.
>> At least for the case of aggregate functions with a single parameter
>> belonging to a chosen set of data types.
> I was wondering about that too. Hardest part would be to come up with values
> to pass to the aggregates.
>
> I don't think it'd help in this case though, since it depends on special case
> grammar stuff to even be reached. json_objectagg(k : v with unique
> keys). "Normal" use of aggregates can't even reach the problematic path
> afaics.
>
It can, as Jaime's original post showed.
But on further consideration I'm thinking this area needs some rework.
ISTM that it might be a whole lot simpler and comprehensible to generate
the json first without bothering about null values or duplicate keys and
then in the finalizer check for null values to be skipped and duplicate
keys. That way we would need to keep far less state for the aggregation
functions, although it might be marginally less efficient. Thoughts?
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com