Re: Partial aggregates pushdown

Hi,

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 "expecxted".

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