shihao zhong <[email protected]> writes:
> Since b61aa76e458, jsonb_object_agg as a window function can return
> an old value for a key.
> ...
> In a window the final function runs once per row. Each run ends the
> object on the shared state, which sorts the pairs array and removes
> duplicate keys in place. A pair added after that can lose to an older
> pair with the same key. The comment there assumes this step can run
> many times on the same state, and that does not hold.
Ouch. Clearly I didn't think hard enough about what invariant
uniqueifyJsonbObject() actually maintains.
> 0001 makes the final function work on a copy of the pairs array.
This patch makes me very sad, because it looks like a big hit
performance-wise. Using this benchmark derived from your example:
\timing on
do $$ begin
perform g, jsonb_object_agg('a', g*2) over (order by g) from
generate_series(1,10000) g;
end $$;
On my machine with a non-assert build, HEAD does this in about 12ms,
while the patch makes it take 7200ms. That's partly from the extra
palloc/memcpy overhead, but (I think) mostly from losing the
intermediate de-duplications. Maybe that's an edge case that nobody
will care about, but even without removable duplicates, the extra
palloc/memcpy overhead seems bad.
> A smaller fix is to renumber the pairs after the dedup.
Yeah, I independently came to the same thought. That would partially
fix uniqueifyJsonbObject's failure to compose across repeat
applications.
> That fixes
> the queries above. But the same step drops null pairs from the state,
> so this still differs from 18:
Right, and that seems seriously messy to fix.
One alternative could be to use the extra palloc/memcpy only when
skip_nulls is true, after fixing uniqueifyJsonbObject to renumber
the pairs. That at least removes the performance issue from the
simplest case.
Perhaps we could separate out the null-removal step and perform it
only later, but I don't see a simple way to do that.
(Independently of all this, the documentation for the json-object
aggregates is seriously awful. It doesn't explain how duplicate
keys are handled in the non-unique cases, and it leaves the
wrong impression about whether duplicate keys are rejected before
or after dropping nulls.)
regards, tom lane
