Subject: RE: Partial aggregates pushdown

From: "[email-masked]" <[email-masked]>
Date: Mon, 12 Jun 2023 08:51:30 +0000

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 <[email-masked]>
> 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".

> From: Bruce Momjian <[email-masked]>
> 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).

Sincerely yours,
Yuuki Fujii

--
Yuuki Fujii
Information Technology R&D Center Mitsubishi Electric Corporation

---

From: Bruce Momjian <[email-masked]>
Date: Mon, 12 Jun 2023 09:37:35 -0400

> The title of this section should be changed to something like "Aggregate on partitioned table".

Sounds good.

> (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  <[email-masked]>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 13 Jun 2023 02:18:15 +0000

Hi Mr.Momjian.

Thank you for advises.

> Sent: Monday, June 12, 2023 10:38 PM
> > 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.

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 buit-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.

Sincerely yours,
Yuuki Fujii

---

From: Bruce Momjian <[email-masked]>
Date: Mon, 19 Jun 2023 20:42:45 -0400

On Tue, Jun 13, 2023 at 02:18:15AM +0000, [email-masked] wrote:
> > 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
> # If Approach 1-1 is taken, the problem is that this feature cannot be used for all bait-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.

---

From: Alexander Pyhalov <[email-masked]>
Date: Tue, 20 Jun 2023 09:59:11 +0300

> you can push down partial aggregates.

Hi.
Currently we don't get remote connection while planning if 
use_remote_estimate is not set.
Such change would require to get remote connection in planner, not in 
executor.
This can lead to change of behavior (like errors in explain when user 
mapping is wrong - e.g. bad password is specified).
Also this potentially can lead to establishing connections even when 
plan node is not actually used
(like extreme example - select sum(score) from t limit 0).
I'm not saying we shouldn't do it - just hint at possible consequences.

-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

---

From: Bruce Momjian <[email-masked]>
Date: Wed, 21 Jun 2023 11:43:57 -0400

On Tue, Jun 20, 2023 at 09:59:11AM +0300, Alexander Pyhalov wrote:
> Also this potentially can lead to establishing connections even when plan
> node is not actually used

Agreed.  I noticed it was doing FDW connections during optimization, but
didn't see the postgres_fdw option that would turn it off. 
Interestingly, it is disabled by default.

After considering the options, I think we should have a postgres_fdw
option called "planner_version_check", and default that false.  When
false, a remote server version check will not be performed during
planning and partial aggregates will be always be considered.  When
true, a version check will be performed during planning and partial
aggregate pushdown disabled for pre-PG 17 foreign servers during the
query.

If we want to be more specific, we can call it
"check_partial_aggregate_support".

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 22 Jun 2023 05:23:33 +0000

Hi Mr.Momjian, Mr.Pyhalov, hackers.

> Sent: Thursday, June 22, 2023 12:44 AM
> > > You can then use the return value of PQserverVersion() to determine
> > > if you can push down partial aggregates.
> >
> > Hi.
> > plan node is not actually used (like extreme example - select
> > sum(score) from t limit 0).
> Agreed.  I noticed it was doing FDW connections during optimization, but didn't see the postgres_fdw option that would
> turn it off.
> After considering the options, I think we should have a postgres_fdw option called "planner_version_check", and default
> that false.  When false, a remote server version check will not be performed during planning and partial aggregates will be
> always be considered.  When true, a version check will be performed during planning and partial aggregate pushdown
> disabled for pre-PG 17 foreign servers during the query.
> 
> If we want to be more specific, we can call it "check_partial_aggregate_support".
Thank you both for your advice.
We will address the compatibility issues as follows.

Approach1-3:
I will add a postgres_fdw option "check_partial_aggregate_support".
This option is false, default.
Only if this option is true, postgres_fdw connect to the remote server and get the version of the remote server.
And if the version of the remote server is less than PG17, then partial aggregate push down to the remote server is disable.

Sincerely yours,
Yuuki Fujii

> -----Original Message-----
> Sent: Thursday, June 22, 2023 12:44 AM
> To: Alexander Pyhalov <[email-masked]>
> Cc: Fujii Yuki/藤井 雄規(MELCO/情報総研 ＤＭ最適Ｇ) <[email-masked]>;
> PostgreSQL-development <[email-masked]>; Andres Freund <[email-masked]>; Tom Lane
> <[email-masked]>; Tomas Vondra <[email-masked]>; Julien Rouhaud <[email-masked]>;
> Daniel Gustafsson <[email-masked]>; Ilya Gladyshev <[email-masked]>
> Subject: Re: Partial aggregates pushdown
> 
> > > You can then use the return value of PQserverVersion() to determine
> > > if you can push down partial aggregates.
> >
> > Hi.
> > plan node is not actually used (like extreme example - select
> > sum(score) from t limit 0).
> Agreed.  I noticed it was doing FDW connections during optimization, but didn't see the postgres_fdw option that would
> turn it off.
> After considering the options, I think we should have a postgres_fdw option called "planner_version_check", and default
> that false.  When false, a remote server version check will not be performed during planning and partial aggregates will be
> always be considered.  When true, a version check will be performed during planning and partial aggregate pushdown
> disabled for pre-PG 17 foreign servers during the query.
> 
> If we want to be more specific, we can call it "check_partial_aggregate_support".
> 
> --

---

From: Bruce Momjian <[email-masked]>
Date: Thu, 22 Jun 2023 07:39:14 -0400

> And if the version of the remote server is less than PG17, then partial aggregate push down to the remote server is disable.

Great!

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 10 Jul 2023 07:35:27 +0000

> > 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.
> >
> > > 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.
> > 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.
I have modified documents according to the above policy.

> Sent: Thursday, June 22, 2023 8:39 PM
I have modified the program except for the point "if the version of the remote server is less than PG17".
Instead, we have addressed the following.
"If check_partial_aggregate_support is true and the remote server version is older than the local server
version, postgres_fdw does not assume that the partial aggregate function is on the remote server unless
the partial aggregate function and the aggregate function match."
The reason for this is to maintain compatibility with any aggregate function that does not support partial
aggregate in one version of V1 (V1 is PG17 or higher), even if the next version supports partial aggregate.
For example, string_agg does not support partial aggregation in PG15, but it will support partial aggregation
in PG16.

We have not been able to add a test for the case where the remote server version is older than the
local server version to the regression test. Is there any way to add such tests to the existing regression
tests?

Sincerely yours,
Yuuki Fujii

---

From: Alexander Pyhalov <[email-masked]>
Date: Fri, 14 Jul 2023 16:40:16 +0300

Hi.

1) In foreign_join_ok() should we set fpinfo->user if 
fpinfo->check_partial_aggregate_support is set like it's done for 
fpinfo->use_remote_estimate? It seems we can end up with fpinfo->user = 
NULL if use_remote_estimate is not set.

2) It seeems we found an additional issue with original patch, which is 
present in current one. I'm attaching a patch which seems to fix it, but 
I'm not quite sure in it.

-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

From 187d15185200aabc22c5219bbe636bc950670a78 Mon Sep 17 00:00:00 2001
From: Alexander Pyhalov <[email-masked]>
Date: Fri, 14 Jul 2023 16:34:02 +0300
Subject: [PATCH] For partial aggregation we can't rely on the fact that every
 var is a part of some GROUP BY expression

---
 .../postgres_fdw/expected/postgres_fdw.out    | 120 ++++++++++++++++++
 contrib/postgres_fdw/postgres_fdw.c           |  31 +++--
 contrib/postgres_fdw/sql/postgres_fdw.sql     |   9 ++
 3 files changed, 152 insertions(+), 8 deletions(-)

diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 8d80ba0a6be..31ee461045c 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -9955,6 +9955,126 @@ SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b ORDER BY 1;
  49 | 19.0000000000000000 |  29 |    60
 (50 rows)
 
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT b/2, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b/2 ORDER BY 1;
+                                                           QUERY PLAN                                                           
+--------------------------------------------------------------------------------------------------------------------------------
+ Sort
+   Output: ((pagg_tab.b / 2)), (avg(pagg_tab.a)), (max(pagg_tab.a)), (count(*))
+   Sort Key: ((pagg_tab.b / 2))
+   ->  Finalize HashAggregate
+         Output: ((pagg_tab.b / 2)), avg(pagg_tab.a), max(pagg_tab.a), count(*)
+         Group Key: ((pagg_tab.b / 2))
+         ->  Append
+               ->  Foreign Scan
+                     Output: ((pagg_tab.b / 2)), (PARTIAL avg(pagg_tab.a)), (PARTIAL max(pagg_tab.a)), (PARTIAL count(*))
+                     Relations: Aggregate on (public.fpagg_tab_p1 pagg_tab)
+                     Remote SQL: SELECT (b / 2), avg_p_int4(a), max(a), count(*) FROM public.pagg_tab_p1 GROUP BY 1
+               ->  Foreign Scan
+                     Output: ((pagg_tab_1.b / 2)), (PARTIAL avg(pagg_tab_1.a)), (PARTIAL max(pagg_tab_1.a)), (PARTIAL count(*))
+                     Relations: Aggregate on (public.fpagg_tab_p2 pagg_tab_1)
+                     Remote SQL: SELECT (b / 2), avg_p_int4(a), max(a), count(*) FROM public.pagg_tab_p2 GROUP BY 1
+               ->  Foreign Scan
+                     Output: ((pagg_tab_2.b / 2)), (PARTIAL avg(pagg_tab_2.a)), (PARTIAL max(pagg_tab_2.a)), (PARTIAL count(*))
+                     Relations: Aggregate on (public.fpagg_tab_p3 pagg_tab_2)
+                     Remote SQL: SELECT (b / 2), avg_p_int4(a), max(a), count(*) FROM public.pagg_tab_p3 GROUP BY 1
+(19 rows)
+
+SELECT b/2, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b/2 ORDER BY 1;
+ ?column? |         avg         | max | count 
+----------+---------------------+-----+-------
+        0 | 10.5000000000000000 |  21 |   120
+        1 | 12.5000000000000000 |  23 |   120
+        2 | 14.5000000000000000 |  25 |   120
+        3 | 16.5000000000000000 |  27 |   120
+        4 | 18.5000000000000000 |  29 |   120
+        5 | 10.5000000000000000 |  21 |   120
+        6 | 12.5000000000000000 |  23 |   120
+        7 | 14.5000000000000000 |  25 |   120
+        8 | 16.5000000000000000 |  27 |   120
+        9 | 18.5000000000000000 |  29 |   120
+       10 | 10.5000000000000000 |  21 |   120
+       11 | 12.5000000000000000 |  23 |   120
+       12 | 14.5000000000000000 |  25 |   120
+       13 | 16.5000000000000000 |  27 |   120
+       14 | 18.5000000000000000 |  29 |   120
+       15 | 10.5000000000000000 |  21 |   120
+       16 | 12.5000000000000000 |  23 |   120
+       17 | 14.5000000000000000 |  25 |   120
+       18 | 16.5000000000000000 |  27 |   120
+       19 | 18.5000000000000000 |  29 |   120
+       20 | 10.5000000000000000 |  21 |   120
+       21 | 12.5000000000000000 |  23 |   120
+       22 | 14.5000000000000000 |  25 |   120
+       23 | 16.5000000000000000 |  27 |   120
+       24 | 18.5000000000000000 |  29 |   120
+(25 rows)
+
+-- It's unsafe to pushdown variables if we need both variable and variable-based expression
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT (b/2)::numeric, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b/2 ORDER BY 1;
+                                                                  QUERY PLAN                                                                  
+----------------------------------------------------------------------------------------------------------------------------------------------
+ Sort
+   Output: ((((pagg_tab.b / 2)))::numeric), (avg(pagg_tab.a)), (max(pagg_tab.a)), (count(*)), ((pagg_tab.b / 2))
+   Sort Key: ((((pagg_tab.b / 2)))::numeric)
+   ->  Finalize GroupAggregate
+         Output: (((pagg_tab.b / 2)))::numeric, avg(pagg_tab.a), max(pagg_tab.a), count(*), ((pagg_tab.b / 2))
+         Group Key: ((pagg_tab.b / 2))
+         ->  Sort
+               Output: ((pagg_tab.b / 2)), pagg_tab.b, (PARTIAL avg(pagg_tab.a)), (PARTIAL max(pagg_tab.a)), (PARTIAL count(*))
+               Sort Key: ((pagg_tab.b / 2))
+               ->  Append
+                     ->  Partial HashAggregate
+                           Output: ((pagg_tab.b / 2)), pagg_tab.b, PARTIAL avg(pagg_tab.a), PARTIAL max(pagg_tab.a), PARTIAL count(*)
+                           Group Key: (pagg_tab.b / 2)
+                           ->  Foreign Scan on public.fpagg_tab_p1 pagg_tab
+                                 Output: (pagg_tab.b / 2), pagg_tab.b, pagg_tab.a
+                                 Remote SQL: SELECT a, b FROM public.pagg_tab_p1
+                     ->  Partial HashAggregate
+                           Output: ((pagg_tab_1.b / 2)), pagg_tab_1.b, PARTIAL avg(pagg_tab_1.a), PARTIAL max(pagg_tab_1.a), PARTIAL count(*)
+                           Group Key: (pagg_tab_1.b / 2)
+                           ->  Foreign Scan on public.fpagg_tab_p2 pagg_tab_1
+                                 Output: (pagg_tab_1.b / 2), pagg_tab_1.b, pagg_tab_1.a
+                                 Remote SQL: SELECT a, b FROM public.pagg_tab_p2
+                     ->  Partial HashAggregate
+                           Output: ((pagg_tab_2.b / 2)), pagg_tab_2.b, PARTIAL avg(pagg_tab_2.a), PARTIAL max(pagg_tab_2.a), PARTIAL count(*)
+                           Group Key: (pagg_tab_2.b / 2)
+                           ->  Foreign Scan on public.fpagg_tab_p3 pagg_tab_2
+                                 Output: (pagg_tab_2.b / 2), pagg_tab_2.b, pagg_tab_2.a
+                                 Remote SQL: SELECT a, b FROM public.pagg_tab_p3
+(28 rows)
+
+SELECT (b/2)::numeric, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b/2 ORDER BY 1;
+ numeric |         avg         | max | count 
+---------+---------------------+-----+-------
+       0 | 10.5000000000000000 |  21 |   120
+       1 | 12.5000000000000000 |  23 |   120
+       2 | 14.5000000000000000 |  25 |   120
+       3 | 16.5000000000000000 |  27 |   120
+       4 | 18.5000000000000000 |  29 |   120
+       5 | 10.5000000000000000 |  21 |   120
+       6 | 12.5000000000000000 |  23 |   120
+       7 | 14.5000000000000000 |  25 |   120
+       8 | 16.5000000000000000 |  27 |   120
+       9 | 18.5000000000000000 |  29 |   120
+      10 | 10.5000000000000000 |  21 |   120
+      11 | 12.5000000000000000 |  23 |   120
+      12 | 14.5000000000000000 |  25 |   120
+      13 | 16.5000000000000000 |  27 |   120
+      14 | 18.5000000000000000 |  29 |   120
+      15 | 10.5000000000000000 |  21 |   120
+      16 | 12.5000000000000000 |  23 |   120
+      17 | 14.5000000000000000 |  25 |   120
+      18 | 16.5000000000000000 |  27 |   120
+      19 | 18.5000000000000000 |  29 |   120
+      20 | 10.5000000000000000 |  21 |   120
+      21 | 12.5000000000000000 |  23 |   120
+      22 | 14.5000000000000000 |  25 |   120
+      23 | 16.5000000000000000 |  27 |   120
+      24 | 18.5000000000000000 |  29 |   120
+(25 rows)
+
 -- Partial aggregates are safe to push down for all built-in aggregates
 EXPLAIN (VERBOSE, COSTS OFF)
 SELECT /* aggregate function <> aggpartialfunc */
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 5c48c5d50dc..eec76a00488 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -6381,13 +6381,17 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel,
 	ListCell   *lc;
 	int			i;
 	List	   *tlist = NIL;
+	bool	partial = false;
 
 	/* We currently don't support pushing Grouping Sets. */
 	if (query->groupingSets)
 		return false;
 
+	if (patype == PARTITIONWISE_AGGREGATE_PARTIAL)
+		partial = true;
+
 	/* It's unsafe to push having statements with partial aggregates */
-	if ((patype == PARTITIONWISE_AGGREGATE_PARTIAL) && havingQual)
+	if (partial && havingQual)
 		return false;
 
 	/* Get the fpinfo of the underlying scan relation. */
@@ -6461,11 +6465,17 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel,
 		}
 		else
 		{
+			/*
+			 * For partial aggregation we can't rely on the fact that every
+			 * var is a part of some GROUP BY expression, so extract variables
+			 * and if we found some, avoid to push it down.
+			 */
+
 			/*
 			 * Non-grouping expression we need to compute.  Can we ship it
 			 * as-is to the foreign server?
 			 */
-			if (is_foreign_expr(root, grouped_rel, expr) &&
+			if (!partial && is_foreign_expr(root, grouped_rel, expr) &&
 				!is_foreign_param(root, grouped_rel, expr))
 			{
 				/* Yes, so add to tlist as-is; OK to suppress duplicates */
@@ -6489,14 +6499,17 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel,
 					return false;
 
 				/*
-				 * Add aggregates, if any, into the targetlist.  Plain Vars
-				 * outside an aggregate can be ignored, because they should be
+				 * Add aggregates, if any, into the targetlist.  When dealing
+				 * with usual aggregations,  plain Vars outside an aggregate
+				 * can be ignored, because they should be
 				 * either same as some GROUP BY column or part of some GROUP
 				 * BY expression.  In either case, they are already part of
-				 * the targetlist and thus no need to add them again.  In fact
-				 * including plain Vars in the tlist when they do not match a
-				 * GROUP BY column would cause the foreign server to complain
-				 * that the shipped query is invalid.
+				 * the targetlist and thus no need to add them again. When
+				 * looking at partial aggregation, we can get additional vars
+				 * needed for partial aggregation itself, which do not match
+				 * group by column. In fact including plain Vars in the tlist
+				 * when they do not match a GROUP BY column would cause the
+				 * foreign server to complain that the shipped query is invalid.
 				 */
 				foreach(l, aggvars)
 				{
@@ -6504,6 +6517,8 @@ foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel,
 
 					if (IsA(aggref, Aggref))
 						tlist = add_to_flat_tlist(tlist, list_make1(aggref));
+					else if (partial)
+						return false;
 				}
 			}
 		}
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 827637b001a..4f93e1931de 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3030,6 +3030,15 @@ EXPLAIN (VERBOSE, COSTS OFF)
 SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b ORDER BY 1;
 SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b ORDER BY 1;
 
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT b/2, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b/2 ORDER BY 1;
+SELECT b/2, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b/2 ORDER BY 1;
+
+-- It's unsafe to pushdown variables if we need both variable and variable-based expression
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT (b/2)::numeric, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b/2 ORDER BY 1;
+SELECT (b/2)::numeric, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b/2 ORDER BY 1;
+
 -- Partial aggregates are safe to push down for all built-in aggregates
 EXPLAIN (VERBOSE, COSTS OFF)
 SELECT /* aggregate function <> aggpartialfunc */
-- 
2.34.1

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 18 Jul 2023 01:35:53 +0000

Hi Mr.Pyhalov.

> Sent: Friday, July 14, 2023 10:40 PM
> fpinfo->use_remote_estimate? It seems we can end up with fpinfo->user 
> fpinfo->=
You are right. I will modify this patch according to your advice.
Thank you for advice.

> 2) It seeems we found an additional issue with original patch, which 
> is present in current one. I'm attaching a patch which seems to fix 
> it, but I'm not quite sure in it.
Thank you for pointing out the issue.
If a query's group-by clause contains variable based expression(not variable)
and the query's select clause contains another expression,
the partial aggregate could be unsafe to push down.

An example of such queries:
SELECT (b/2)::numeric, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b/2

Your patch disables partial aggregate pushdown for such queries.
I'll see if we can modify the patch to safely do a partial aggregate pushdown for such queries as well.
Such a query expects the variable in the select clause expression to be included in the target of the grouped rel
(let see make_partial_grouping_target), 
but the original groupby clause has no reference to this variable,
this seems to be the direct cause(let see foreign_grouping_ok). 
I will examine whether a safe pushdown can be achieved by matching the
groupby clause information referenced by foreign_grouping_ok with the grouped rel target information.

Sincerely yours,
Yuuki Fujii

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 19 Jul 2023 00:43:38 +0000

Hi Mr.Pyhalov, hackers.

I have made the following three modifications about this patch.

1)
> <[email-masked]>
> Sent: Tuesday, July 18, 2023 10:36 AM
Done.

2)
> <[email-masked]>
> Sent: Tuesday, July 18, 2023 10:36 AM
I modified the patch to safely do a partial aggregate pushdown for such queries as well
 by matching the groupby clause information referenced by foreign_grouping_ok with the grouped rel target information.

3)
I modified the patch to safely do a partial aggregate pushdown for queries which contain having clauses.

Sincerely yours,
Yuuki Fujii

---

From: Alexander Pyhalov <[email-masked]>
Date: Thu, 20 Jul 2023 13:23:44 +0300

> queries which contain having clauses.

Hi.
Sorry, but I don't see how it could work.
For example, the attached test returns wrong result:

CREATE FUNCTION f() RETURNS INT AS $$
begin
   return 10;
end
$$ LANGUAGE PLPGSQL;

SELECT b, sum(a) FROM pagg_tab GROUP BY b HAVING sum(a) < f() ORDER BY 
1;
  b  | sum
----+-----
   0 |   0
  10 |   0
  20 |   0
  30 |   0
  40 |   0
+(5 rows)

In fact the above query should have returned 0 rows, as

SELECT b, sum(a) FROM pagg_tab GROUP BY b ORDER BY 1;
  b  | sum
----+------
   0 |  600
   1 |  660
   2 |  720
   3 |  780
   4 |  840
   5 |  900
   6 |  960
   7 | 1020
   8 | 1080
   9 | 1140
  10 |  600
  11 |  660
  12 |  720
....
shows no such rows.

Or, on the same data

SELECT b, sum(a) FROM pagg_tab GROUP BY b HAVING sum(a) > 660 ORDER BY 
1;

You'll get 0 rows.

But
SELECT b, sum(a) FROM pagg_tab GROUP BY b;
  b  | sum
----+------
  42 |  720
  29 | 1140
   4 |  840
  34 |  840
  41 |  660
   0 |  600
  40 |  600
gives.

The issue is that you can't calculate "partial" having. You should 
compare full aggregate in filter, but it's not possible on the level of 
one partition.
And you have this in plans

  Finalize GroupAggregate
    Output: pagg_tab.b, avg(pagg_tab.a), max(pagg_tab.a), count(*)
    Group Key: pagg_tab.b
    Filter: (sum(pagg_tab.a) < 700)
    ->  Sort
          Output: pagg_tab.b, (PARTIAL avg(pagg_tab.a)), (PARTIAL 
max(pagg_tab.a)), (PARTIAL count(*)), (PARTIAL sum(pagg_tab.a))
          Sort Key: pagg_tab.b
          ->  Append
                ->  Foreign Scan
                      Output: pagg_tab.b, (PARTIAL avg(pagg_tab.a)), 
(PARTIAL max(pagg_tab.a)), (PARTIAL count(*)), (PARTIAL sum(pagg_tab.a))
                      Filter: ((PARTIAL sum(pagg_tab.a)) < 700)   !!!! 
<--- here we can't compare anything yet, sum is incomplete.
                      Relations: Aggregate on (public.fpagg_tab_p1 
pagg_tab)
                      Remote SQL: SELECT b, avg_p_int4(a), max(a), 
count(*), sum(a) FROM public.pagg_tab_p1 GROUP BY 1
                ->  Foreign Scan
                      Output: pagg_tab_1.b, (PARTIAL avg(pagg_tab_1.a)), 
(PARTIAL max(pagg_tab_1.a)), (PARTIAL count(*)), (PARTIAL 
sum(pagg_tab_1.a))
                      Filter: ((PARTIAL sum(pagg_tab_1.a)) < 700)
                      Relations: Aggregate on (public.fpagg_tab_p2 
pagg_tab_1)
                      Remote SQL: SELECT b, avg_p_int4(a), max(a), 
count(*), sum(a) FROM public.pagg_tab_p2 GROUP BY 1
                ->  Foreign Scan
                      Output: pagg_tab_2.b, (PARTIAL avg(pagg_tab_2.a)), 
(PARTIAL max(pagg_tab_2.a)), (PARTIAL count(*)), (PARTIAL 
sum(pagg_tab_2.a))
                      Filter: ((PARTIAL sum(pagg_tab_2.a)) < 700)
                      Relations: Aggregate on (public.fpagg_tab_p3 
pagg_tab_2)
                      Remote SQL: SELECT b, avg_p_int4(a), max(a), 
count(*), sum(a) FROM public.pagg_tab_p3 GROUP BY 1

In foreign_grouping_ok()
6586                         if (IsA(expr, Aggref))
6587                         {
6588                                 if (partial)
6589                                 {
6590                                         mark_partial_aggref((Aggref 
*) expr, AGGSPLIT_INITIAL_SERIAL);
6591                                         continue;
6592                                 }
6593                                 else if (!is_foreign_expr(root, 
grouped_rel, expr))
6594                                         return false;
6595
6596                                 tlist = add_to_flat_tlist(tlist, 
list_make1(expr));
6597                         }

at least you shouldn't do anything with expr, if is_foreign_expr() 
returned false. If we restrict pushing down queries with havingQuals, 
I'm not quite sure how Aggref can appear in local_conds.

As for changes in planner.c (setGroupClausePartial()) I have several 
questions.

1) Why don't we add non_group_exprs to pathtarget->exprs when 
partial_target->exprs is not set?

2) We replace extra->partial_target->exprs with partial_target->exprs 
after processing. Why are we sure that after this tleSortGroupRef is 
correct?

-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 1824cb67fe9..c6f613019c3 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -3025,6 +3025,22 @@ EXPLAIN (VERBOSE, COSTS OFF)
 SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b HAVING sum(a) < 700 ORDER BY 1;
 SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b HAVING sum(a) < 700 ORDER BY 1;
 
+CREATE FUNCTION f() RETURNS INT AS $$
+begin
+  return 10;
+end 
+$$ LANGUAGE PLPGSQL;
+
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT b, sum(a) FROM pagg_tab GROUP BY b HAVING sum(a) < f() ORDER BY 1;
+SELECT b, sum(a) FROM pagg_tab GROUP BY b HAVING sum(a) < f() ORDER BY 1;
+
+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT b, sum(a) FROM pagg_tab GROUP BY b ORDER BY 1;
+SELECT b, sum(a) FROM pagg_tab GROUP BY b ORDER BY 1;
+
+DROP function f();
+
 -- Partial aggregates are safe to push down without having clause
 EXPLAIN (VERBOSE, COSTS OFF)
 SELECT b, avg(a), max(a), count(*) FROM pagg_tab GROUP BY b ORDER BY 1;

---

From: "Finnerty, Jim" <[email-masked]>
Date: Tue, 1 Aug 2023 20:25:55 +0000

When it is valid to filter based on a HAVING clause predicate, it should already have been converted into a WHERE clause predicate, except in the special case of an LIMIT TO .k .. ORDER BY case where the HAVING clause predicate can be determined approximately after having found k fully qualified tuples and then that predicate is successively tightened as more qualified records are found.

*that*, by the way, is a very powerful optimization.

    /Jim F

On 7/20/23, 6:24 AM, "Alexander Pyhalov" <[email-masked] <mailto:[email-masked]>> wrote:

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.

Hi.

Or, on the same data

You'll get 0 rows.

But

Finalize GroupAggregate

In foreign_grouping_ok()

--
Best regards,
Alexander Pyhalov,
Postgres Professional

---

From: Bruce Momjian <[email-masked]>
Date: Mon, 7 Aug 2023 14:30:54 -0400

> For example, string_agg does not support partial aggregation in PG15, but it will support partial aggregation

Just to clarify, I think you are saying:

	If check_partial_aggregate_support is true and the remote server
	version is older than the local server version, postgres_fdw
	checks if the partial aggregate function exists on the remote
	server during planning and only uses it if it does.

I tried to phrase it in a positive way, and mentioned the plan time
distinction.  Also, I am sorry I was away for most of July and am just
getting to this.

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 25 Sep 2023 03:18:13 +0000

Hi Mr.Bruce, Mr.Pyhalov, Mr.Finnerty, hackers.

Thank you for your valuable comments. I sincerely apologize for the very late reply.
Here is a response to your comments or a fix to the patch.

Tuesday, August 8, 2023 at 3:31 Bruce Momjian
> distinction.  Also, I am sorry I was away for most of July and am just
Thanks for your comment. In the documentation, the description of check_partial_aggregate_support is as follows
(please see postgres-fdw.sgml).
--
check_partial_aggregate_support (boolean)
Only if this option is true, during query planning, postgres_fdw connects to the remote server and check if the remote server version is older than the local server version. If so, postgres_fdw assumes that for each built-in aggregate function, the partial aggregate function is not defined on the remote server unless the partial aggregate function and the aggregate function match. The default is false.
--

> Sorry, but I don't see how it could work.
We apologize for any inconvenience caused.
Thanks to Pyhalov's and Jim's comments, I have realized that I have made a fundamental mistake regarding the pushdown of the HAVING clause and the difficulty of achieving it performing Partial aggregate pushdown.
So, I removed the codes about pushdown of the HAVING clause performing Partial aggregate pushdown.

> after processing. Why are we sure that after this tleSortGroupRef is
Response to 1)
The code you pointed out was unnecessary. I have removed this code.
Also, the process of adding PlaceHolderVar's expr to partial_target was missing.
So I fixed this.

Response to 2)
The making procedures extra->groupClausePartial and extra->partial_target 
in make_partial_grouping_target for this patch is as follows.
STEP1. From grouping_target->exprs, extract Aggref, Var and Placeholdervar that are not included in Aggref.
STEP2. setGroupClausePartial sets the copy of original groupClause to extra->groupClausePartial
and sets the copy of original partial_target to extra->partial_target.
STEP3. setGroupClausePartial adds Var and Placeholdervar in STEP1 to partial_target.
The sortgroupref of partial_target->sortgrouprefs to be added to value is set to
(the maximum value of the existing sortgroupref) + 1.
setGroupClausePartial adds data sgc of sortgroupclause type where sgc->tlesortgroupref
matches the sortgroupref to GroupClause.
STEP4. add_new_columns_to_pathtarget adds STEP1's Aggref to partial_target.

Due to STEP2, the list of tlesortgrouprefs set in extra->groupClausePartial is not duplicated.
Also, sortgrouprefs added to extra->partial_target matches with corresponding 
tlesortgrouprefs added to extra->groupClausePartial.
So these tlesortgrouprefs are correct.

Sincerely yours,
Yuuki Fujii

---

From: Bruce Momjian <[email-masked]>
Date: Mon, 25 Sep 2023 18:30:32 -0400

> (please see postgres-fdw.sgml).

My point is that there are three behaviors:

*  false - no check
*  true, remote version >= sender - no check
*  true, remove version < sender - check

Here is your code:

	+ * Check that a buit-in aggpartialfunc exists on the remote server. If
	+ * check_partial_aggregate_support is false, we assume the partial aggregate
	+ * function exsits on the remote server. Otherwise we assume the partial
	+ * aggregate function exsits on the remote server only if the remote server
	+ * version is not less than the local server version.
	+ */
	+static bool
	+is_builtin_aggpartialfunc_shippable(Oid aggpartialfn, PgFdwRelationInfo *fpinfo)
	+{
	+       bool            shippable = true;
	+
	+       if (fpinfo->check_partial_aggregate_support)
	+       {
	+               if (fpinfo->remoteversion == 0)
	+               {
	+                       PGconn     *conn = GetConnection(fpinfo->user, false, NULL);
	+
	+                       fpinfo->remoteversion = PQserverVersion(conn);
	+               }
	+               if (fpinfo->remoteversion < PG_VERSION_NUM)
	+                       shippable = false;
	+       }
	+       return shippable;
	+}

I think this needs to be explained in the docs.  I am ready to adjust
the patch to improve the wording whenever you are ready.  Should I do it
now and post an updated version for you to use?

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 26 Sep 2023 06:26:25 +0000

Hi Mr.Bruce.

Tuesday, September 26, 2023 7:31 Bruce Momjian
> > > > "If check_partial_aggregate_support is true and the remote server
> > > > version is older than the local server version, postgres_fdw does
> > > > not assume that the partial aggregate function is on the remote server unless the partial aggregate function and the
> aggregate function match."
> > > > The reason for this is to maintain compatibility with any
> > > > aggregate function that does not support partial aggregate in one version of V1 (V1 is PG17 or higher), even if the
> next version supports partial aggregate.
> > > > For example, string_agg does not support partial aggregation in
> > > > PG15, but it will support partial aggregation in PG16.
> > >
> > > distinction.  Also, I am sorry I was away for most of July and am
> > > just getting to this.
> > Thanks for your comment. In the documentation, the description of
> > check_partial_aggregate_support is as follows (please see postgres-fdw.sgml).
> > --
> > check_partial_aggregate_support (boolean) Only if this option is true,
> > during query planning, postgres_fdw connects to the remote server and check if the remote server version is older than
> the local server version. If so, postgres_fdw assumes that for each built-in aggregate function, the partial aggregate
> function is not defined on the remote server unless the partial aggregate function and the aggregate function match. The
> default is false.
> > --
> 
> I think this needs to be explained in the docs.  I am ready to adjust the patch to improve the wording whenever you are
> ready.  Should I do it now and post an updated version for you to use?
The following explanation was omitted from the documentation, so I added it.
> *  true, remove version < sender - check
I have responded to your comment, but if there is a problem with the wording, could you please suggest a correction?

Sincerely yours,
Yuuki Fujii

> Sent: Tuesday, September 26, 2023 7:31 AM
> To: Fujii Yuki/藤井 雄規(MELCO/情報総研 ＤＭ最適Ｇ) <[email-masked]>
> Cc: Alexander Pyhalov <[email-masked]>; Finnerty, Jim <[email-masked]>; PostgreSQL-development
> <[email-masked]>; Andres Freund <[email-masked]>; Tom Lane <[email-masked]>; Tomas
> Vondra <[email-masked]>; Julien Rouhaud <[email-masked]>; Daniel Gustafsson
> <[email-masked]>; Ilya Gladyshev <[email-masked]>
> > > > "If check_partial_aggregate_support is true and the remote server
> > > > version is older than the local server version, postgres_fdw does
> > > > not assume that the partial aggregate function is on the remote server unless the partial aggregate function and the
> aggregate function match."
> > > > The reason for this is to maintain compatibility with any
> > > > aggregate function that does not support partial aggregate in one version of V1 (V1 is PG17 or higher), even if the
> next version supports partial aggregate.
> > > > For example, string_agg does not support partial aggregation in
> > > > PG15, but it will support partial aggregation in PG16.
> > >
> > > distinction.  Also, I am sorry I was away for most of July and am
> > > just getting to this.
> > Thanks for your comment. In the documentation, the description of
> > check_partial_aggregate_support is as follows (please see postgres-fdw.sgml).
> > --
> > check_partial_aggregate_support (boolean) Only if this option is true,
> > during query planning, postgres_fdw connects to the remote server and check if the remote server version is older than
> the local server version. If so, postgres_fdw assumes that for each built-in aggregate function, the partial aggregate
> function is not defined on the remote server unless the partial aggregate function and the aggregate function match. The
> default is false.
> > --
> 
> I think this needs to be explained in the docs.  I am ready to adjust the patch to improve the wording whenever you are
> ready.  Should I do it now and post an updated version for you to use?
> 
> --

---

From: Alexander Pyhalov <[email-masked]>
Date: Tue, 26 Sep 2023 16:15:14 +0300

[email-masked] писал 2023-09-25 06:18:
> Only if this option is true, during query planning, postgres_fdw
> connects to the remote server and check if the remote server version
> is older than the local server version. If so, postgres_fdw assumes
> that for each built-in aggregate function, the partial aggregate
> function is not defined on the remote server unless the partial
> aggregate function and the aggregate function match. The default is
> false.
> --
> 
> Thanks to Pyhalov's and Jim's comments, I have realized that I have
> made a fundamental mistake regarding the pushdown of the HAVING clause
> and the difficulty of achieving it performing Partial aggregate
> pushdown.
> STEP1. From grouping_target->exprs, extract Aggref, Var and
> Placeholdervar that are not included in Aggref.
> Due to STEP2, the list of tlesortgrouprefs set in
> extra->groupClausePartial is not duplicated.

Do you mean that extra->partial_target->sortgrouprefs is not replaced, 
and so we preserve tlesortgroupref numbers?
I'm suspicious about rewriting extra->partial_target->exprs with 
partial_target->exprs - I'm still not sure why we
  don't we loose information, added by add_column_to_pathtarget() to 
extra->partial_target->exprs?

Also look at the following example.

EXPLAIN VERBOSE SELECT  count(*) , (b/2)::numeric FROM pagg_tab GROUP BY 
b/2 ORDER BY 1;
                                             QUERY PLAN
---------------------------------------------------------------------------------------------------
  Sort  (cost=511.35..511.47 rows=50 width=44)
    Output: (count(*)), ((((pagg_tab.b / 2)))::numeric), ((pagg_tab.b / 
2))
    Sort Key: (count(*))
    ->  Finalize HashAggregate  (cost=509.06..509.94 rows=50 width=44)
          Output: count(*), (((pagg_tab.b / 2)))::numeric, ((pagg_tab.b / 
2))
          Group Key: ((pagg_tab.b / 2))
          ->  Append  (cost=114.62..506.06 rows=600 width=16)
                ->  Foreign Scan  (cost=114.62..167.69 rows=200 width=16)
                      Output: ((pagg_tab.b / 2)), (PARTIAL count(*)), 
pagg_tab.b
                      Relations: Aggregate on (public.fpagg_tab_p1 
pagg_tab)
                      Remote SQL: SELECT (b / 2), count(*), b FROM 
public.pagg_tab_p1 GROUP BY 1, 2
                ->  Foreign Scan  (cost=114.62..167.69 rows=200 width=16)
                      Output: ((pagg_tab_1.b / 2)), (PARTIAL count(*)), 
pagg_tab_1.b
                      Relations: Aggregate on (public.fpagg_tab_p2 
pagg_tab_1)
                      Remote SQL: SELECT (b / 2), count(*), b FROM 
public.pagg_tab_p2 GROUP BY 1, 2
                ->  Foreign Scan  (cost=114.62..167.69 rows=200 width=16)
                      Output: ((pagg_tab_2.b / 2)), (PARTIAL count(*)), 
pagg_tab_2.b
                      Relations: Aggregate on (public.fpagg_tab_p3 
pagg_tab_2)
                      Remote SQL: SELECT (b / 2), count(*), b FROM 
public.pagg_tab_p3 GROUP BY 1, 2

Note that group by is still deparsed incorrectly.
-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 26 Sep 2023 10:16:30 -0400

> I have responded to your comment, but if there is a problem with the wording, could you please suggest a correction?

I like your new wording, thanks.

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 26 Sep 2023 22:35:28 +0000

Hi Mr.Momjian, Mr.Pyhalov.

> and so we preserve tlesortgroupref numbers?
Yes, that is correct.

> EXPLAIN VERBOSE SELECT  count(*) , (b/2)::numeric FROM pagg_tab GROUP BY
Thank you for comments. You are right.
It is a mistake to rewrite extra->partial_target->exprs with partial_target->exprs.
I fixed this point.

September 26, 2023 (Fire) 23:16 Bruce Momjian <[email-masked]>.
> I like your new wording, thanks.
Thanks.

Sincerely yours,
Yuuki Fujii

---

From: Alexander Pyhalov <[email-masked]>
Date: Wed, 27 Sep 2023 15:36:34 +0300

Hi.

In postgres_fdw.sql

"Partial aggregates are unsafe to push down having clause when there are 
partial aggregates" - this comment likely should be fixed.

Some comments should be added to setGroupClausePartial() and to 
make_partial_grouping_target() - especially why setGroupClausePartial()
is called prior to add_new_columns_to_pathtarget().

I'm not sure that I like this mechanics of adding sort group clauses - 
it seems we do in core additional work, which is of use only for
one extension, but at least it seems to be working.

-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 28 Sep 2023 04:40:40 +0000

Hi Mr.Pyhalov.

Thank you for comments.
> partial aggregates" - this comment likely should be fixed.
Fixed.

> is called prior to add_new_columns_to_pathtarget().
I have added comments to setGroupClausePartial() and to make_partial_grouping_target().

> one extension, but at least it seems to be working.
We cannot deparse the original sort group clauses and pathtarget
when performing partial aggreggate pushdown by any FDWs.
So I think the additional sort group clauses and pathtarget are
needed by any FDWs, not only postgres_fdw.

Sincerely yours,
Yuuki Fujii

---

From: Alexander Pyhalov <[email-masked]>
Date: Thu, 28 Sep 2023 09:15:43 +0300

> needed by any FDWs, not only postgres_fdw.

Hi.
It seems to me that *fdw postfixes don't clarify things, but just make 
naming more ugly.

+ * Adding these Vars and PlaceHolderVars to PathTarget,
+ * FDW cannot deparse this by the original List of SortGroupClauses.
+ * So, before this adding process,
+ * setGroupClausePartial generates another Pathtarget and another
+ * List of SortGroupClauses for FDW.

It seems that something like:

/*
  * Modified PathTarget cannot be used by FDW as-is to deparse this 
statement.
  * So, before modifying PathTarget, setGroupClausePartial generates
  * another Pathtarget and another list List of SortGroupClauses
  * to make deparsing possible.
  */

sounds better.

-- 
Best regards,
Alexander Pyhalov,
Postgres Professional

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 28 Sep 2023 07:21:10 +0000

Hi Mr.Pyhalov.

Alexander Pyhalov <[email-masked]>
Thursday, September 28, 2023 3:16 PM
> It seems to me that *fdw postfixes don't clarify things, but just make naming more ugly.
I have removed *fdw postfixes.

>   * Modified PathTarget cannot be used by FDW as-is to deparse this statement.
Thank you for the suggested modifications. I have modified it according to your suggestion.

Sincerely yours,
Yuuki Fujii

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 18 Oct 2023 05:22:34 +0000

Hi hackers.

Because there is a degrade in pg_dump.c, I fixed it.

Sincerely yours,
Yuuki Fujii

---

From: Bruce Momjian <[email-masked]>
Date: Mon, 23 Oct 2023 12:55:46 -0400

> Because there is a degrade in pg_dump.c, I fixed it.

Fujii-san, to get this patch closer to finished, can I modify this
version of the patch to improve some wording and post an updated version
you can use for future changes?

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 24 Oct 2023 00:12:41 +0000

Hi Mr.Momjian.

> Fujii-san, to get this patch closer to finished, can I modify this version of the patch to improve some wording and post an
> updated version you can use for future changes?
Yes, I greatly appreciate your offer.
I would very much appreciate your modifications.

Sincerely yours,
Yuuki Fujii

---

From: Bruce Momjian <[email-masked]>
Date: Wed, 25 Oct 2023 18:08:23 -0400

> I would very much appreciate your modifications.

I am almost done updating the patch, but I got stuck on how the feature
is supposed to work.  This documentation sentence is where I got
confused:

	<varlistentry>
	 <term><literal>check_partial_aggregate_support</literal> (<type>boolean</type>)</term>
	 <listitem>
	  <para>
	   If this option is false, <filename>postgres_fdw</filename> assumes
	   that for each built-in aggregate function,
	   the partial aggregate function is defined on the remote server
	   without checking the remote server version.
	   If this option is true, during query planning,
	   <filename>postgres_fdw</filename> connects to the remote server
	   and checks if the remote server version is older than the local server version.
	   If so,
	   <filename>postgres_fdw</filename>
-->	   assumes that for each built-in aggregate function, the partial aggregate function is not defined
-->	   on the remote server unless the partial aggregate function and the aggregate
-->	   function match.
	   Otherwise <filename>postgres_fdw</filename> assumes that for each built-in aggregate function,
	   the partial aggregate function is defined on the remote server.
	   The default is <literal>false</literal>.
	  </para>
	 </listitem>
	</varlistentry>

What does that marked sentence mean?  What is match?  Are one or both of
these remote?  It sounds like you are checking the local aggregate
against the remote partial aggregate, but I don't see any code that does
this in the patch.

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 26 Oct 2023 11:11:09 +0000

Hi Mr.Momjian

> Sent: Thursday, October 26, 2023 7:08 AM
> I am almost done updating the patch, but I got stuck on how the feature is supposed to work.  This documentation
> sentence is where I got
> confused:
> 
> 	<varlistentry>
> What does that marked sentence mean?  What is match?  Are one or both of these remote?  It sounds like you are
> checking the local aggregate against the remote partial aggregate, but I don't see any code that does this in the patch.
This sentence means that
"If the partial aggregate function has the same OID as the aggregate function,
then postgres_fdw assumes that for each built-in aggregate function, the partial aggregate function is not defined
 on the remote server."
"Match" means that the partial aggregate function has the same OID as the aggregate function in local server.
But, in v30, there is no code which checks the partial aggregate function has the same OID as the aggregate function in local server.
So I modified the code of is_builtin_aggpartialfunc_shippable().
Also, I modified wording postgres-fdw.sgml.

Sincerely yours,
Yuuki Fujii

---

From: Bruce Momjian <[email-masked]>
Date: Thu, 26 Oct 2023 15:43:29 -0400

> Also, I modified wording postgres-fdw.sgml.

Yes, that is what I needed.  Attached is a modification of your v31
patch (the most recent) that mostly improves the documentation and
comments.  What else needs to be done before committers start to review
this?

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 27 Oct 2023 02:44:42 +0000

Hi Momjian.

Thank you for your improvement.
As a matter of detail, I think that the areas marked below are erroneous.

--
+   Pushdown causes aggregate function cals to send partial aggregate
                                         ^
+   function calls to the remote server. If the partial aggregate
+   function doesn't doesn't exist on the remote server, it causes
                     ^^^^^^^
--

> What else needs to be done before committers start to review
> this?
There are no others. May I make a new version of v31 with your
suggested improvements for the committer's review?

Sincerely yours,
Yuuki Fujii

---

From: Bruce Momjian <[email-masked]>
Date: Thu, 26 Oct 2023 23:06:17 -0400

> As a matter of detail, I think that the areas marked below are erroneous.

Agreed.  Do you want to fix that on your vesion?  I don't have any more
improvements to make.

> suggested improvements for the committer's review?

Yes, please.  I think the updated docs will help people understand how
the patch works.

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 27 Oct 2023 05:32:48 +0000

Hi Mr.Momjian.

> Sent: Friday, October 27, 2023 12:06 PM
> Agreed.  Do you want to fix that on your vesion?  I don't have any more improvements to make.
Yes, I have fixed that in v32(Attached).

> > > What else needs to be done before committers start to review this?
> Yes, please.  I think the updated docs will help people understand how the patch works.
Thank you. v32(Attached) is tha updated version.

Sincerely yours,
Yuuki Fujii

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 13 Nov 2023 05:48:47 +0000

Hi Hackers.

I have rebased this patch.

Sincerely yours,
Yuuki Fujii

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 13 Nov 2023 08:25:48 +0000

Hi Hackers.

In postgres_fdw.sql, I have corrected the output format for floating point numbers
by extra_float_digits.

Sincerely yours,
Yuuki Fujii

---

From: Robert Haas <[email-masked]>
Date: Mon, 20 Nov 2023 15:51:33 -0500

> In postgres_fdw.sql, I have corrected the output format for floating point numbers

Looking at this, I find that it's not at all clear to me how the
partial aggregate function is defined. Let's look at what we have for
documentation:

+  <para>
+   Paraemter <literal>AGGPARTIALFUNC</literal> optionally defines a
+   partial aggregate function used for partial aggregate pushdown; see
+   <xref linkend="xaggr-partial-aggregates"/> for details.
+  </para>

+       Partial aggregate function (zero if none).
+       See <xref linkend="partial-aggregate-pushdown"/> for the definition
+       of partial aggregate function.

+   Partial aggregate pushdown is an optimization for queries that contains
+   aggregate expressions for a partitioned table across one or more remote
+   servers. If multiple conditions are met, partial aggregate function

+   When partial aggregate pushdown is used for aggregate expressions,
+   remote queries replace aggregate function calls with partial
+   aggregate function calls.  If the data type of the state value is not

But there's no definition of what the behavior of the function is
anywhere that I can see, not even in <sect2
id="partial-aggregate-pushdown">. Everywhere it only describes how the
partial aggregate function is used, not what it is supposed to do.

Looking at the changes in pg_aggregate.dat, it seems like the partial
aggregate function is a second aggregate defined in a way that mostly
matches the original, except that (1) if the original final function
would have returned a data type other than internal, then the final
function is removed; and (2) if the original final function would have
returned a value of internal type, then the final function is the
serialization function of the original aggregate. I think that's a
reasonable definition, but the documentation and code comments need to
be a lot clearer.

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.

I think that the control mechanism needs some thought. Right now,
there are two possible behaviors: either we assume that the local and
remote sides are the same unconditionally, or we assume that they're
the same if the remote side is a new enough version. I do like having
those behaviors available, but I wonder if we need to do something
better or different. What if somebody wants to push down a
non-built-in aggregate, for example? I realize that we don't have
great solutions to the problem of knowing which functions are
push-downable in general, and I don't know that partial aggregation
needs to be any better than anything else, but it's probably worth
comparing and contrasting the approach we take here with the
approaches we've taken in other, similar cases. From that point of
view, I think check_partial_aggregate_support is a novelty: we don't
do those kinds of checks in other cases, AFAIK. But on the other hand,
there is the 'extensions' argument to postgres_fdw.

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.

Typo: Paraemter

I'm so sorry to keep complaining about comments, but I think the
comments in src/backend/optimizer are very far from being adequate.
They are strictly formulaic and don't really explain anything. For
example, I see that the patch adds a partial_target to
GroupPathExtraData, but how do I understand the reason why we now need
a second pathtarget beside the one that already exists? Certainly not
from the comments in setGroupClausePartial, because there basically
aren't any. True, there's a header comment, but it just says we
generate this thing, not WHY we generate this thing. There's nothing
meaningful to be found in src/include/nodes/pathnodes.h about why
we're doing this, either.

And this problem really extends throughout the patch: comments are
mostly short and just describe what the code does, not WHY it does
that. And the WHY is really the important part. Otherwise we will not
be able to maintain this code going forward.

-- 
Robert Haas
EDB: http://www.enterprisedb.com