Date: Thu, 7 Dec 2023 16:12:18 -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>,
	Tom Lane <tgl@sss.pgh.pa.us>,
	Alexander Pyhalov <a.pyhalov@postgrespro.ru>,
	Stephen Frost <sfrost@snowman.net>,
	"Finnerty, Jim" <jfinnert@amazon.com>,
	Andres Freund <andres@anarazel.de>,
	Tomas Vondra <tomas.vondra@enterprisedb.com>,
	Julien Rouhaud <rjuju123@gmail.com>,
	Daniel Gustafsson <daniel@yesql.se>
Subject: Re: [CAUTION!! freemail] Re: Partial aggregates pushdown
Message-ID: <ZXI1MiGbvEtm3oQM@momjian.us>
References: <TYAPR01MB55149B8569F2FDA262654C4F95BAA@TYAPR01MB5514.jpnprd01.prod.outlook.com>
 <CA+TgmoYQKF-8vL8sctWuw_dskBiQWcspuNPJuH4ttusuiP529g@mail.gmail.com>
 <CA+TgmoZHtdFEOULVXqhWgSg36R1e1ZEdSKJKb+VzTfRU0HaSZQ@mail.gmail.com>
 <4012625.1701120204@sss.pgh.pa.us>
 <CA+Tgmob7Dn_+UN1qgb_NUEoeg1jdug5mh+qz7q3Ed1NHAL_LNQ@mail.gmail.com>
 <ZWX4H9EDA7fHiIaU@tamriel.snowman.net>
 <TYAPR01MB551411A7DDFF0B38C5AAEDFE9584A@TYAPR01MB5514.jpnprd01.prod.outlook.com>
 <CA+TgmoYCrtOvk2f32qQKZV=jNL35tandf2A2Dp_2F5ASuiG1BA@mail.gmail.com>
 <TYAPR01MB5514C939F39F97BC171F58FD958BA@TYAPR01MB5514.jpnprd01.prod.outlook.com>
 <CA+TgmoZVsJgVcUdCeqOH-WXPZVVuiTawhAbS5jWmBZ+CRrOYKw@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+TgmoZVsJgVcUdCeqOH-WXPZVVuiTawhAbS5jWmBZ+CRrOYKw@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZXI1MiGbvEtm3oQM%40momjian.us>
Precedence: bulk

On Thu, Dec  7, 2023 at 09:56:08AM -0500, Robert Haas wrote:
> On Wed, Dec 6, 2023 at 7:11 PM Fujii.Yuki@df.MitsubishiElectric.co.jp
> <Fujii.Yuki@df.mitsubishielectric.co.jp> wrote:
> > I would be grateful if we can resolve this issue gradually. I would also like to continue the discussion if possible in the future.
> 
> I think that would be good. Thanks for your work on this. It is a hard problem.

Agreed.  First, Robert is right that this feature is long overdue.  It
might not help many of our existing workloads, but it opens us up to
handling new, larger workloads.

Second, the patch already has a mechanism to check the remote server
version to see if it is the same or newer.   Here is the version check
documentation patch:

	check_partial_aggregate_support (boolean)
	
	If this option is false, <filename>postgres_fdw</filename> always
	uses partial aggregate pushdown by assuming that each built-in
	aggregate function has a partial aggregate function defined on
	the remote server.  If this option is true, local aggregates
	whose partial computation function references itself are assumed
	to exist on the remote server.	If not, during query planning,
	<filename>postgres_fdw</filename> will connect to the remote
	server and retrieve the remote server version.	If the remote
	version is the same or newer, partial aggregate functions will be
	assumed to exist.  If older, <filename>postgres_fdw</filename>
	checks that the remote server has a matching partial aggregate
	function before performing partial aggregate pushdown.	The default
	is <literal>false</literal>.

There is also an extension list that specifies which extension-owned
functions can be pushed down;  from the doc patch:

	To reduce the risk of misexecution of queries, WHERE clauses and
	aggregate expressions are not sent to the remote server unless they
	only use data types, operators, and functions that are built-in
	or belong to an extension that is listed in the foreign server's
	<literal>extensions</literal> option.

Third, we already have a way of creating records for tables:

	SELECT pg_language FROM pg_language;
	                pg_language
	-------------------------------------------
	 (12,internal,10,f,f,0,0,2246,)
	 (13,c,10,f,f,0,0,2247,)
	 (14,sql,10,f,t,0,0,2248,)
	 (13576,plpgsql,10,t,t,13573,13574,13575,)

And we do have record input functionality:

	CREATE TABLE test (x int, language pg_language);
	
	INSERT INTO test SELECT 0, pg_language FROM pg_language;
	
	SELECT * FROM test;
	 x |                 language
	---+-------------------------------------------
	 0 | (12,internal,10,f,f,0,0,2246,)
	 0 | (13,c,10,f,f,0,0,2247,)
	 0 | (14,sql,10,f,t,0,0,2248,)
	 0 | (13576,plpgsql,10,t,t,13573,13574,13575,)
	(4 rows)

However, functions don't have pre-created records, and internal
functions don't see to have an SQL-defined structure, but as I remember
the internal aggregate functions all take the same internal structure,
so I guess we only need one fixed input and one output that would
output/input such records.  Performance might be an issue, but at this
point let's just implement this and measure the overhead since there are
few/any(?) other viable options.

Fourth, going with #2 where we do the pushdown using an SQL keyword also
allows extensions to automatically work, while requiring partial
aggregate functions for every non-partial aggregate will require work
for extensions, and potentially lead to more version mismatch issues.

Finally, I am now concerned that this will not be able to be in PG 17,
which I was hoping for.

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

  Only you can decide what is important to you.