Date: Tue, 21 Nov 2023 15:34:19 -0500
From: Bruce Momjian <bruce@momjian.us>
To: Robert Haas <robertmhaas@gmail.com>
Cc: "Fujii.Yuki@df.MitsubishiElectric.co.jp" <Fujii.Yuki@df.mitsubishielectric.co.jp>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>,
	"Finnerty, Jim" <jfinnert@amazon.com>,
	Andres Freund <andres@anarazel.de>, Tom Lane <tgl@sss.pgh.pa.us>,
	Tomas Vondra <tomas.vondra@enterprisedb.com>,
	Julien Rouhaud <rjuju123@gmail.com>,
	Daniel Gustafsson <daniel@yesql.se>,
	Alexander Pyhalov <a.pyhalov@postgrespro.ru>
Subject: Re: Partial aggregates pushdown
Message-ID: <ZV0US_1taS8Utkxk@momjian.us>
References: <TYTPR01MB10954D890D8F729E1BCE42D4F95DDA@TYTPR01MB10954.jpnprd01.prod.outlook.com>
 <ZTrBYS71CrnnA7xa@momjian.us>
 <TYTPR01MB10954B4C9A791FA9D2BF692F095DCA@TYTPR01MB10954.jpnprd01.prod.outlook.com>
 <ZTspKdU1sZ4qKLD8@momjian.us>
 <TYTPR01MB10954B96601911FE868BF88E895DCA@TYTPR01MB10954.jpnprd01.prod.outlook.com>
 <OSZPR01MB9424BB7E885791D88B80F7B395B3A@OSZPR01MB9424.jpnprd01.prod.outlook.com>
 <OSZPR01MB94244C52ABBE67C4A033B5E495B3A@OSZPR01MB9424.jpnprd01.prod.outlook.com>
 <CA+Tgmobvja+jytj5zcEcYgqzOaeJiqrrJxgqDf1q=3k8FepuWQ@mail.gmail.com>
 <ZVviSttXc7kff_I5@momjian.us>
 <CA+TgmoaiH_Na3y17PankZH59EgJWbgdPM-szb1eP6DQoOAMgYg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CA+TgmoaiH_Na3y17PankZH59EgJWbgdPM-szb1eP6DQoOAMgYg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZV0US_1taS8Utkxk%40momjian.us>
Precedence: bulk

On Tue, Nov 21, 2023 at 12:16:41PM -0500, Robert Haas wrote:
> On Mon, Nov 20, 2023 at 5:48 PM Bruce Momjian <bruce@momjian.us> wrote:
> > > I do have a concern about this, though. It adds a lot of bloat. It
> > > adds a whole lot of additional entries to pg_aggregate, and every new
> > > aggregate we add in the future will require a bonus entry for this,
> > > and it needs a bunch of new pg_proc entries as well. One idea that
> > > I've had in the past is to instead introduce syntax that just does
> > > this, without requiring a separate aggregate definition in each case.
> > > For example, maybe instead of changing string_agg(whatever) to
> > > string_agg_p_text_text(whatever), you can say PARTIAL_AGGREGATE
> > > string_agg(whatever) or string_agg(PARTIAL_AGGREGATE whatever) or
> > > something. Then all aggregates could be treated in a generic way. I'm
> > > not completely sure that's better, but I think it's worth considering.
> >
> > So use an SQL keyword to indicates a pushdown call?  We could then
> > automate the behavior rather than requiring special catalog functions?
> 
> Right. It would require more infrastructure in the parser, planner,
> and executor, but it would be infinitely reusable instead of needing a
> new thing for every aggregate. I think that might be better, but to be
> honest I'm not totally sure.

It would make it automatic.  I guess we need to look at how big the
patch is to do it.

> > > I don't think the patch does a good job explaining why HAVING,
> > > DISTINCT, and ORDER BY are a problem. It seems to me that HAVING
> > > shouldn't really be a problem, because HAVING is basically a WHERE
> > > clause that occurs after aggregation is complete, and whether or not
> > > the aggregation is safe shouldn't depend on what we're going to do
> > > with the value afterward. The HAVING clause can't necessarily be
> > > pushed to the remote side, but I don't see how or why it could make
> > > the aggregate itself unsafe to push down. DISTINCT and ORDER BY are a
> > > little trickier: if we pushed down DISTINCT, we'd still have to
> > > re-DISTINCT-ify when combining locally, and if we pushed down ORDER
> > > BY, we'd have to do a merge pass to combine the returned values unless
> > > we could prove that the partitions were non-overlapping ranges that
> > > would be visited in the correct order. Although that all sounds
> > > doable, I think it's probably a good thing that the current patch
> > > doesn't try to handle it -- this is complicated already. But it should
> > > explain why it's not handling it and maybe even a bit about how it
> > > could be handling in the future, rather than just saying "well, this
> > > kind of thing is not safe." The trouble with that explanation is that
> > > it does nothing to help the reader understand whether the thing in
> > > question is *fundamentally* unsafe or whether we just don't have the
> > > right code to make it work.
> >
> > Makes sense.
> 
> Actually, I think I was wrong about this. We can't handle ORDER BY or
> DISTINCT because we can't distinct-ify or order after we've already
> partially aggregated. At least not in general, and not without
> additional aggregate support functions. So what I said above was wrong
> with respect to those. Or so I believe, anyway. But I still don't see
> why HAVING should be a problem.

This should probably be documented in the patch.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.