RE: Partial aggregates pushdown - Mailing list pgsql-hackers

Hi Jelte, hackers.

Thank you for explanations.

Actually, I have other tasks about "PARTIAL_AGGREAGATE" keyword
to respond Requirement1 and Requirement2 in the following mail.
https://www.postgresql.org/message-id/TYAPR01MB3088755F2281D41F5EEF06D495F92%40TYAPR01MB3088.jpnprd01.prod.outlook.com

After that tasks, I plan to compare your proposal with mine seriously, with additional POC patches if necessary.

I think that your proposal might seem to be a more fundamental solution.
However, to be honest, so far, I don't perfectly get the benefits and impacts by stopping usage of internal types
instead using a native types, especially on handling memory contexts of existing deserialization functions and
on the amount of codes to be modified or added.
The followings are the answers with the knowledge I have right now.

> From: Jelte Fennema-Nio <postgres@jeltef.nl>
> Sent: Tuesday, June 25, 2024 5:49 AM
> > However, I still have the following two questions.
> >
> > 1. Not necessary components of new native types Refer to pg_type.dat,
> > typinput and typoutput are required.
> > I think that in your proposal they are not necessary, so waste. I
> > think that it is not acceptable.
> > How can I resolve the problem?
> 
> I think requiring typinput/typoutput is a benefit personally, because that makes it possible to do PARTIAL_AGGREGATE
> pushdown to a different PG major version. Also it makes it easier to debug the partial aggregate values when using
> psql/pgregress. So yes, it requires implementing both binary (send/receive) and text (input/output) serialization,
butit also
 
> has some benefits. And in theory you might be able to skip implementing the binary serialization, and rely purely on
thetext
 
> serialization to send partial aggregates between servers.
I see. It seems that adding new natives might make it easier to transmit the state values between local and remote have
differentmajor versions.
 
However, in my opinion, we should be careful to support the case in which local and remote have different major
versions,
because the transtype of an aggregate function would may change in future major version due to
something related to the implementation.
Actually, something like that occurs recently, see
https://github.com/postgres/postgres/commit/519fc1bd9e9d7b408903e44f55f83f6db30742b7
I think the transtype of an aggregate function quite more changeable than retype.
Consequently, so far, I want to support the cases in which local and remote have the same major version.
If we try to resolve the limitation, it seems to need more additional codes.

And, I'm afraid that adding typinput/typoutput bothers the developers.
They also have to create a new native types in addition to create their new aggregate functions.
I wonder if this concern might outweigh the benefits for debugging.
And, if skipping send/receive, they have to select only the text representation on
the transmission of the state value. I think it is narrow.

> > 2. Many new native types
> > I think that, basically, each aggregate function does need a new native type.
> > For example,
> > avg(int8), avg(numeric), and var_pop(int4) has the same transtype, PolyNumAggState.
> > You said that it is enough to add only one native type like
> > state_poly_num_agg for supporting them, right?
> 
> Yes, correct. That's what I had in mind.
> 
> > But the combine functions of them might have quite different
> > expectation on the data items of PolyNumAggState like the range of
> > N(means count) and the true/false of calcSumX2 (the flag of
> > calculating sum of squares).
> > The final functions of them have the similar expectation.
> > So, I think that, responded to your proposal, each of them need a
> > native data type like state_int8_avg, state_numeric_avg, for safety.
> >
> > And, we do need a native type for an aggregate function whose
> > transtype is not internal and not pseudo.
> > For avg(int4), the current transtype is _int8.
> > However, I do need a validation check on the number of the array And
> > the positiveness of count(the first element of the array).
> > Responded to your proposal,
> > I do need a new native type like state_int4_avg.
> 
> To help avoid confusion let me try to restate what I think you mean
> here: You're worried about someone passing in a bogus native type into the final/combine functions and then getting
> crashes and/or wrong results. With internal type people cannot do this because they cannot manually call the
> combinefunc/finalfunc because the argument type is internal. To solve this problem your suggestion is to make the
type
> specific to the specific aggregate such that send/receive or input/output can validate the input as reasonable. But
this
> would then mean that we have many native types (and also many deserialize/serialize functions).
Yes, right.

> Assuming that's indeed what you meant, that's an interesting thought, I didn't consider that much indeed. My thinking
was
> that we only need to implement send/receive & input/output functions for these types, and even though their meaning
is
> very different we can serialize them in the same way.
> 
> As you say though, something like that is already true for avg(int4) today. The way avg(int4) handles this issue is
bydoing
 
> some input validation for every call to its trans/final/combinefunc (see int4_avg_accum, int4_avg_combine, and
int8_avg).
> It checks the length of the array there, but it doesn't check the positiveness of the count. I think that makes
sense.IMHO
 
> these functions only need to protect against crashes (e.g. null pointer dereferences). But I don't think there is a
goodreason
 
> for them to protect the user against passing in weird data. These functions aren't really meant to be called manually
inthe
 
> first place anyway, so if the user does that and they pass in weird data then I'm fine with them getting a weird
resultback,
 
> even errors are fine (only crashes are not).
> 
> So as long as our input/output & send/receive functions for state_poly_num_agg handle all the inconsistencies that
could
> cause crashes later on (which I think is pretty simple to do for PolyNumAggState), then I don't think we need
state_int8_avg,
> state_numeric_avg, etc.
I see. Certainly, it might sufficient that, receive functions perform validation checks to avoid crash,
and combine functions and final functions are responsible for avoiding crash
in the range of values of data items of the native type.

Best regards, Yuki Fujii
--
Yuki Fujii
Information Technology R&D Center, Mitsubishi Electric Corporation