Hi.
Updated and rebased patch.
Ilya Gladyshev писал 2021-11-02 00:31:
> Hi,
> On 21.10.2021 13:55, Alexander Pyhalov wrote:
>
>> Hi. Updated patch.
>> Now aggregates with internal states can be pushed down, if they are
>> marked as pushdown safe (this flag is set to true for min/max/sum),
>> have internal states and associated converters.
>
> I don't quite understand why this is restricted only to aggregates
> that have 'internal' state, I feel like that should be possible for
> any aggregate that has a function to convert its final result back to
> aggregate state to be pushed down. While I couldn't come up with a
> useful example for this, except maybe for an aggregate whose
> aggfinalfn is used purely for cosmetic purposes (e.g. format the
> result into a string), I still feel that it is an unnecessary
> restriction.
>
I don't feel comfortable with it for the following reasons.
- Now partial converters translate aggregate result to serialized
internal representation.
In case when aggregate type is different from internal state,
we'd have to translate it to non-serialized internal representation,
so converters should skip serialization step. This seems like
introducing two
kind of converters.
- I don't see any system aggregates which would benefit from this.
However, it doesn't seem to be complex, and if it seems to be desirable,
it can be done.
For now introduced check that transtype matches aggregate type (or is
internal)
in partial_agg_ok().
> A few minor review notes to the patch:
>
> +static List *build_conv_list(RelOptInfo *foreignrel);
>
> this should probably be up top among other declarations.
>
Moved it upper.
> @@ -1433,6 +1453,48 @@ postgresGetForeignPlan(PlannerInfo *root,
> outer_plan);
> }
>
> +/*
> + * Generate attinmeta if there are some converters:
> + * they are expecxted to return BYTEA, but real input type is likely
> different.
> + */
>
> typo in word "expecxted".
Fixed.
>
> @@ -139,10 +147,13 @@ typedef struct PgFdwScanState
> * for a foreign join scan. */
> TupleDesc tupdesc; /* tuple descriptor of scan */
> AttInMetadata *attinmeta; /* attribute datatype conversion
> metadata */
> + AttInMetadata *rcvd_attinmeta; /* metadata for received
> tuples, NULL if
> + * there's no converters */
>
> Looks like rcvd_attinmeta is redundant and you could use attinmeta for
> conversion metadata.
Seems so, removed it.
--
Best regards,
Alexander Pyhalov,
Postgres Professional