Date: Mon, 19 Jun 2023 20:42:45 -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: <ZJD2BWVqRlJgz3n6@momjian.us>
References: <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>
 <ZIcfn8pnO4t29O5g@momjian.us>
 <OS3PR01MB666038A12CE1F9D81785D27E9555A@OS3PR01MB6660.jpnprd01.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <OS3PR01MB666038A12CE1F9D81785D27E9555A@OS3PR01MB6660.jpnprd01.prod.outlook.com>
Archived-At: <https://www.postgresql.org/message-id/ZJD2BWVqRlJgz3n6%40momjian.us>
Precedence: bulk

On Tue, Jun 13, 2023 at 02:18:15AM +0000, Fujii.Yuki@df.MitsubishiElectric.co.jp wrote:
> Hi Mr.Momjian.
> 
> Thank you for advises.
> 
> > From: Bruce Momjian <bruce@momjian.us>
> > Sent: Monday, June 12, 2023 10:38 PM
> > > 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 documentation for the option, let's explicitly say it is useful to disable
> > partial aggregates 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.
> 
> Thank you for your perspective.
> So, of the approaches I have presented, you think that approach 1-2 is
> preferable and that the option name remote_server is preferable?
> Indeed, the option of a remote version may have other uses.
> However, this information can be obtained by connecting to a remote server, 
> I'm concerned that some people may find this option redundant.
> 
> Is the problem with approach 1-1 because the user cannot decide whether to include the compatibility check in the decision to do partial aggregate pushdown or not?
> # If Approach 1-1 is taken, the problem is that this feature cannot be used for all bait-in aggregate functions
> # when the remote server is older than PG17.
> If so, Approache1-3 below seem more desirable.
> Would it be possible for us to hear your thoughts?
> 
> Approache1-3：We add a postgres_fdw option about a compatibility check for partial aggregate pushdown
> (like "enable_aggpartialfunc_compatibility_check"). This option is false, the default.
> When this option is true, postgres_fdw obtains the remote server version by connecting the remote server and using PQserverVersion(). 
> And if the remote server version is older than PG17, then the partial aggregate pushdown feature is enable for all buit-in aggregate functions.
> Otherwise the partial aggregate pushdown feature is disable for all buit-in aggregate functions.

Apologies for the delay in my reply to this email.  I looked into the
existing code and I found three things:

1)  PQserverVersion() just pulls the conn->sversion value from the
existing connection because pqSaveParameterStatus() pulls the
server_version sent by the backend;  no need to issue SELECT version().

2)  postgres_fdw already has nine calls to GetConnection(), and only
opens a connection if it already doesn't have one.  Here is an example:

	/* Get the remote estimate */
	conn = GetConnection(fpinfo->user, false, NULL);
	get_remote_estimate(sql.data, conn, &rows, &width,
			    &startup_cost, &total_cost);
	ReleaseConnection(conn);

Therefore, it seems like it would be near-zero cost to just call conn =
GetConnection() and then PQserverVersion(conn), and ReleaseConnection().
You can then use the return value of PQserverVersion() to determine if
you can push down partial aggregates.

3)  Looking at postgresAcquireSampleRowsFunc(), I see this exact method
used:

    conn = GetConnection(user, false, NULL);

    /* We'll need server version, so fetch it now. */
    server_version_num = PQserverVersion(conn);

    ...

    if ((server_version_num < 95000) &&
	(method == ANALYZE_SAMPLE_SYSTEM ||
	 method == ANALYZE_SAMPLE_BERNOULLI))
	ereport(ERROR,
		(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
		 errmsg("remote server does not support TABLESAMPLE feature")));

I am sorry if you already knew all this, but I didn't.

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

  Only you can decide what is important to you.