From: Antonin Houska <ah@cybertec.at>
To: Richard Guo <guofenglinux@gmail.com>
cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>,
    Alvaro Herrera <alvherre@2ndquadrant.com>,
    Richard Guo <riguo@pivotal.io>, Tom Lane <tgl@sss.pgh.pa.us>,
    Michael Paquier <michael@paquier.xyz>,
    PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] WIP: Aggregation push-down
In-reply-to: 
 <CAMbWs4_F_S22tn7Nmxx=x0jZY2J4qA_hB_SLMDvcJuNhnCLu+g@mail.gmail.com>
References: <20190904202339.GA13644@alvherre.pgsql>
 <7087.1567663858@localhost> <20200111011124.efacqhdsptc36pm2@development>
 <72558.1579188350@antos>
 <CAMbWs4_F_S22tn7Nmxx=x0jZY2J4qA_hB_SLMDvcJuNhnCLu+g@mail.gmail.com>
Comments: In-reply-to Richard Guo <guofenglinux@gmail.com>
   message dated "Thu, 06 Feb 2020 16:30:32 +0800."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <53725.1581066334.1@antos>
Content-Transfer-Encoding: quoted-printable
Date: Fri, 07 Feb 2020 10:05:34 +0100
Message-ID: <53726.1581066334@antos>
Precedence: bulk

Richard Guo <guofenglinux@gmail.com> wrote:

> Hi,
> =

> I've been looking at the 'make_join_rel()' part of the patch, and I'm
> aware that, if we are joining A and B, a 'grouped join rel (AB)' would
> be created besides the 'plain join rel (AB)', and paths are added by 1)
> applying partial aggregate to the paths of the 'plain join rel (AB)', or
> 2) joining grouped A to plain B or joining plain A to grouped B.
> =

> This is a smart idea. One issue I can see is that some logic would have
> to be repeated several times. For example, the validity check for the
> same proposed join would be performed at most three times by
> join_is_legal().
> =

> I'm thinking of another idea that, instead of using a separate
> RelOptInfo for the grouped rel, we add in RelOptInfo a
> 'grouped_pathlist' for the grouped paths, just like how we implement
> parallel query via adding 'partial_pathlist'.
> =

> For base rel, if we decide it can produce grouped paths, we create the
> grouped paths by applying partial aggregation to the paths in 'pathlist'
> and add them into 'grouped_pathlist'.
> =

> For join rel (AB), we can create the grouped paths for it by:
> 1) joining a grouped path from 'A->grouped_pathlist' to a plain path
> from 'B->pathlist', or vice versa;
> 2) applying partial aggregation to the paths in '(AB)->pathlist'.
> =

> This is basically the same idea, just moves the grouped paths from the
> grouped rel to a 'grouped_pathlist'. With it we should not need to make
> any code changes to 'make_join_rel()'. Most code changes would happen in
> 'add_paths_to_joinrel()'.
> =

> Will this idea work? Is it better or worse?

I tried such approach in an earlier version of the patch [1], and I think =
the
reason also was to avoid repeated calls of functions like
join_is_legal(). However there were objections against such approach,
e.g. [2], and I admit that it was more invasive than what the current patc=
h
version does.

Perhaps we can cache the result of join_is_legal() that we get for the pla=
in
relation, and use it for the group relation. I'll consider that. Thanks.

[1] https://www.postgresql.org/message-id/18007.1513957437%40localhost
[2] https://www.postgresql.org/message-id/CA%2BTgmob8og%2B9HzMg1vM%2B3LwDm=
2f_bHUi9%2Bg1bqLDTjqpw5s%2BnQ%40mail.gmail.com

-- =

Antonin Houska
Web: https://www.cybertec-postgresql.com