Re: Partial aggregates pushdown

From: Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru>
To: Ilya Gladyshev <i(dot)gladyshev(at)postgrespro(dot)ru>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Partial aggregates pushdown
Date: 2021-11-02 09:12:07
Message-ID: 05d98f67a4f44b31d90dfb977db4ad71@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi.

Updated and rebased patch.

Ilya Gladyshev писал 2021-11-02 00:31:
> 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.
>

I don't feel comfortable with it for the following reasons.
- Now partial converters translate aggregate result to serialized
internal representation.
In case when aggregate type is different from internal state,
we'd have to translate it to non-serialized internal representation,
so converters should skip serialization step. This seems like
introducing two
kind of converters.
- I don't see any system aggregates which would benefit from this.

However, it doesn't seem to be complex, and if it seems to be desirable,
it can be done.
For now introduced check that transtype matches aggregate type (or is
internal)
in partial_agg_ok().

> A few minor review notes to the patch:
>
> +static List *build_conv_list(RelOptInfo *foreignrel);
>
> this should probably be up top among other declarations.
>

Moved it upper.

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

Fixed.

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

Seems so, removed it.

--
Best regards,
Alexander Pyhalov,
Postgres Professional

Attachment Content-Type Size
0001-Partial-aggregates-push-down-v05.patch text/x-diff 58.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2021-11-02 09:27:28 Re: parallel vacuum comments
Previous Message Michael Paquier 2021-11-02 08:51:45 Re: Teach pg_receivewal to use lz4 compression