Re: Partial aggregates pushdown

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.

A few minor review notes to the patch:

+static List *build_conv_list(RelOptInfo *foreignrel);

this should probably be up top among other declarations.

@@ -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 "expec*x*ted".

@@ -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.

The last thing - the patch needs to be rebased, it doesn't apply cleanly
on top of current master.

Thanks,

Ilya Gladyshev