Date: Mon, 12 Jun 2023 09:37:35 -0400
From: Bruce Momjian <bruce@momjian.us>
To: "Fujii.Yuki@df.MitsubishiElectric.co.jp" <Fujii.Yuki@df.mitsubishielectric.co.jp>
Cc: Alexander Pyhalov <a.pyhalov@postgrespro.ru>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>,
	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>,
	Ilya Gladyshev <i.gladyshev@postgrespro.ru>
Subject: Re: Partial aggregates pushdown
Message-ID: <ZIcfn8pnO4t29O5g@momjian.us>
References: <OS3PR01MB66606C1DA960336516D9A08395959@OS3PR01MB6660.jpnprd01.prod.outlook.com>
 <ZDedXNarI32SmN06@momjian.us>
 <ZDemPMLOyXNzKkhw@momjian.us>
 <OS3PR01MB6660EBFFFA06646F402813A395989@OS3PR01MB6660.jpnprd01.prod.outlook.com>
 <ZDj4nwJANUU+kam8@momjian.us>
 <OS3PR01MB66607DC466F7F3B3909DA3CA954EA@OS3PR01MB6660.jpnprd01.prod.outlook.com>
 <8ffedecae0d21a3cd93805baf1dcc0de@postgrespro.ru>
 <OS3PR01MB66604E404B003A621B0C03A69552A@OS3PR01MB6660.jpnprd01.prod.outlook.com>
 <ZINW022iuYzf2ZwO@momjian.us>
 <OS3PR01MB6660B7C247726BC0E36DF4D69554A@OS3PR01MB6660.jpnprd01.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <OS3PR01MB6660B7C247726BC0E36DF4D69554A@OS3PR01MB6660.jpnprd01.prod.outlook.com>
Archived-At: <https://www.postgresql.org/message-id/ZIcfn8pnO4t29O5g%40momjian.us>
Precedence: bulk

On Mon, Jun 12, 2023 at 08:51:30AM +0000, Fujii.Yuki@df.MitsubishiElectric.co.jp wrote:
> Hi Mr.Bruce, Mr.Pyhalov, hackers.
> 
> Thank you for comments. I will try to respond to both of your comments as follows.
> I plan to start revising the patch next week. If you have any comments on the following
> respondences, I would appreciate it if you could give them to me this week.
> 
> > From: Bruce Momjian <bruce@momjian.us>
> > Sent: Saturday, June 10, 2023 1:44 AM
> > I agree that this feature is designed for built-in sharding, but it is possible people could be using aggregates on partitions
> > backed by foreign tables without sharding.  Adding a requirement for non-sharding setups to need PG 17+ servers might
> > be unreasonable.
> Indeed, it is possible to use partial aggregate pushdown feature for purposes other than sharding.
> The description of the section "F.38.6. Built-in sharding in PostgreSQL" assumes the use of
> Built-in sharding and will be modified to eliminate this assumption.
> The title of this section should be changed to something like "Aggregate on partitioned table".

Sounds good.

> > From: Bruce Momjian <bruce@momjian.us>
> > Sent: Saturday, June 10, 2023 1:44 AM
> > Looking at previous release note incompatibilities, we don't normally change non-administrative functions in a way that
> > causes errors if run on older servers.  Based on Alexander's observations, I wonder if we need to re-add the postgres_fdw
> > option to control partial aggregate pushdown, and default it to enabled.
> > 
> > If we ever add more function breakage we might need more postgres_fdw options.  Fortunately, such changes are rare.
> 
> I understand what the problem is. I will put a mechanism maintaining compatibility into the patch.
> I believe there are three approaches.
> Approach 1-1 is preferable because it does not require additional options for postgres_fdw.
> I will revise the patch according to Approach 1-1, unless otherwise commented.
> 
> Approach1:
> I ensure that postgres_fdw retrieves the version of each remote server
> and does not partial aggregate pushd down if the server version is less than 17.
> There are two approaches to obtaining remote server versions.
> Approach1-1: postgres_fdw connects a remote server and use PQserverVersion().
> Approach1-2: Adding a postgres_fdw option about a remote server version (like "server_version").
> 
> Approach2:
> Adding a postgres_fdw option for partial aggregate pushdown is enable or not
> (like enable_partial_aggregate_pushdown).

These are good questions.  Adding a postgres_fdw option called
enable_partial_aggregate_pushdown helps make the purpose of the option
clear, but remote_version can be used for future breakage as well.

I think remote_version is the best idea, and in the documention for the
option, let's explcitly say it is useful to disable partial aggreates
pushdown on pre-PG 17 servers.  If we need to use the option for other
cases, we can just update the documentation.  When the option is blank,
the default, everything is pushed down.

I see remote_version a logical addition to match our "extensions" option
that controls what extension functions can be pushed down.

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

  Only you can decide what is important to you.