public inbox for [email protected]  

help / color / mirror / Atom feed

From: Ilya Gladyshev <[email protected]>
To: Alexander Pyhalov <[email protected]>
Cc: [email protected]
Subject: Re: Partial aggregates pushdown
Date: Tue, 2 Nov 2021 00:31:35 +0300
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

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

view thread (42+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: Partial aggregates pushdown
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox