|From:||Ilya Gladyshev <i(dot)gladyshev(at)postgrespro(dot)ru>|
|To:||Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru>|
|Subject:||Re: Partial aggregates pushdown|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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,
+ * Generate attinmeta if there are some converters:
+ * they are expecxted to return BYTEA, but real input type is likely
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
+ AttInMetadata *rcvd_attinmeta; /* metadata for received tuples,
+ * there's no converters */
Looks like rcvd_attinmeta is redundant and you could use attinmeta for
The last thing - the patch needs to be rebased, it doesn't apply cleanly
on top of current master.
|Next Message||Mark Dilger||2021-11-01 21:42:42||Re: Delegating superuser tasks to new security roles (Was: Granting control of SUSET gucs to non-superusers)|
|Previous Message||Tomas Vondra||2021-11-01 21:15:59||Re: Feature request for adoptive indexes|