Subject: Re: Partial aggregates pushdown
To: Alexander Pyhalov <a.pyhalov@postgrespro.ru>
References: <cf744a8ee4d47bdabe1da9174d4f3dc9@postgrespro.ru>
 <b5a2bb24-ba7d-4053-b298-7e6247fd4043@enterprisedb.com>
 <2486a6db3f068efb97c1277c23402e0d@postgrespro.ru>
 <504e576d-984f-d73c-2694-8ee9624f8def@enterprisedb.com>
 <07fa4d5a56f402732acfce25e7207cae@postgrespro.ru>
From: Ilya Gladyshev <i.gladyshev@postgrespro.ru>
Cc: pgsql-hackers@postgresql.org
Message-ID: <0fbeb9d7-0a8f-8e2c-0687-2d4c32c66a9c@postgrespro.ru>
Date: Tue, 2 Nov 2021 00:31:35 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.13.0
MIME-Version: 1.0
In-Reply-To: <07fa4d5a56f402732acfce25e7207cae@postgrespro.ru>
Content-Type: multipart/alternative;
 boundary="------------178E80676017AB4326D0E3E9"
Content-Language: en-US
Archived-At: 
 <https://www.postgresql.org/message-id/0fbeb9d7-0a8f-8e2c-0687-2d4c32c66a9c%40postgrespro.ru>
Precedence: bulk

This is a multi-part message in MIME format.
--------------178E80676017AB4326D0E3E9
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit

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


--------------178E80676017AB4326D0E3E9
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: 8bit

<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p>Hi,<br>
    </p>
    <div class="moz-cite-prefix">On 21.10.2021 13:55, Alexander Pyhalov
      wrote:<br>
    </div>
    <blockquote type="cite"
      cite="mid:07fa4d5a56f402732acfce25e7207cae@postgrespro.ru">Hi.
      Updated patch.
      <br>
      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),
      <br>
      have internal states and associated converters.</blockquote>
    <p>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.<br>
    </p>
    <p>A few minor review notes to the patch:</p>
    <p><br>
      +static List *build_conv_list(RelOptInfo *foreignrel);</p>
    <p>this should probably be up top among other declarations.</p>
    <p><br>
    </p>
    <p>@@ -1433,6 +1453,48 @@ postgresGetForeignPlan(PlannerInfo *root,<br>
                                   outer_plan);<br>
       }<br>
       <br>
      +/*<br>
      + * Generate attinmeta if there are some converters:<br>
      + * they are expecxted to return BYTEA, but real input type is
      likely different.<br>
      + */</p>
    <p><br>
    </p>
    <p>typo in word "expec<b>x</b>ted".</p>
    <p><br>
    </p>
    <p>@@ -139,10 +147,13 @@ typedef struct PgFdwScanState<br>
                                        * for a foreign join scan. */<br>
           TupleDesc    tupdesc;        /* tuple descriptor of scan */<br>
           AttInMetadata *attinmeta;    /* attribute datatype conversion
      metadata */<br>
      +    AttInMetadata *rcvd_attinmeta;    /* metadata for received
      tuples, NULL if<br>
      +                                     * there's no converters */</p>
    <p><br>
    </p>
    <p>Looks like rcvd_attinmeta is redundant and you could use
      attinmeta for conversion metadata.</p>
    <p>The last thing - the patch needs to be rebased, it doesn't apply
      cleanly on top of current master.</p>
    <p>Thanks,</p>
    <p>Ilya Gladyshev<br>
    </p>
    <p><br>
    </p>
  </body>
</html>

--------------178E80676017AB4326D0E3E9--