On Tue, 14 Apr 2020 at 06:14, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Yeah, they're relying exactly on the assumption that nodeAgg is not
> going to try to copy a value declared "internal", and therefore they
> can be loosey-goosey about whether the value pointer is null or not.
> However, if you want to claim that that's wrong, you have to explain
> why it's okay for some other code to be accessing a value that's
> declared "internal". I'd say that the meaning of that is precisely
> "keepa u hands off".
>
> In the case at hand, the current situation is that we only expect the
> values returned by these combine functions to be read by the associated
> final functions, which are on board with the null-pointer representation
> of an empty result. Your argument is essentially that it should be
> possible to feed the values to the aggregate's associated serialization
> function as well. But the core code never does that, so I'm not convinced
> that we should add it to the requirements; we'd be unable to test it.
Casting my mind back to when I originally wrote that code, I attempted
to do so in such a way so that it could one day be used for a 3-stage
aggregation. e.g Parallel Partial Aggregate -> Gather -> Combine
Serial Aggregate on one node, then on some master node a Deserial
Combine Finalize Aggregate. You're very right that we can't craft
such a plan with today's master (We didn't even add a supporting enum
for it in AggSplit). However, it does appear that there are
extensions or forks out there which attempt to use the code in this
way, so it would be good to not leave those people out in the cold
regarding this.
For testing, can't we just have an Assert() in
advance_transition_function that verifies isnull matches the
nullability of the return value for INTERNAL returning transfns? i.e,
the attached
I don't have a test case to hand that could cause this to fail, but it
sounds like Jesse might.
David
