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
