Subject: Re: Partial aggregates pushdown

From: Tom Lane <[email-masked]>
Date: Tue, 19 Mar 2024 17:29:07 -0400

Bruce Momjian <[email-masked]> writes:
> The current patch has:

>       if ((OidIsValid(aggform->aggfinalfn) ||
>               (aggform->aggtranstype == INTERNALOID)) &&
>               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)
>                       partial_agg_ok = false;
>       }

> It uses check_partial_aggregate_support, which defaults to false,
> meaning partial aggregates will be pushed down with no version check by
> default.  If set to true, pushdown will happen if the remote server is
> the same version or newer, which seems acceptable to me.

I'd like to vociferously protest both of those decisions.

"No version check by default" means "unsafe by default", which is not
project style in general and is especially not so for postgres_fdw.
We have tried very hard for years to ensure that postgres_fdw will
work with a wide range of remote server versions, and generally been
extremely conservative about what we think will work (example:
collations); but this patch seems ready to throw that principle away.

Also, surely "remoteversion < PG_VERSION_NUM" is backwards.  What
this would mean is that nobody can ever change a partial aggregate's
implementation, because that would break queries issued from older
servers (that couldn't know about the change) to newer ones.

Realistically, I think it's fairly unsafe to try aggregate pushdown
to anything but the same PG major version; otherwise, you're buying
into knowing which aggregates have partial support in which versions,
as well as predicting the future about incompatible state changes.
Even that isn't bulletproof --- e.g, maybe somebody wasn't careful
about endianness-independence of the serialized partial state, making
it unsafe to ship --- so there had better be a switch whereby the user
can disable it.

Maybe we could define a three-way setting:

* default: push down partial aggs only to same major PG version
* disable: don't push down, period
* force: push down regardless of remote version

With the "force" setting, it's the user's responsibility not to
issue any remote-able aggregation that would be unsafe to push
down.  This is still a pretty crude tool: I can foresee people
wanting to have per-aggregate control over things, especially
extension-supplied aggregates.  But it'd do for starters.

I'm not super thrilled by the fact that the patch contains zero
user-facing documentation, even though it's created new SQL syntax,
not to mention a new postgres_fdw option.  I assume this means that
nobody thinks it's anywhere near ready to commit.

			regards, tom lane

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 19 Mar 2024 19:09:14 -0400

> servers (that couldn't know about the change) to newer ones.

Well it is the origin server that is issuing the PUSHDOWN syntax, so an
older origin server should be able to push to a newer remote server.

> as well as predicting the future about incompatible state changes.

Yes, incompatible state changes would be a problem with an older origin
server with a newer remote server setup.

If we require matching versions, we must accept that upgrades will
require more downtime.

> it unsafe to ship --- so there had better be a switch whereby the user

Makes sense.  I was also wondering how a user would know whether the
pushdown is happening, or not.

> * force: push down regardless of remote version

What would be the default?  If it is the first one, it requires a
remote version check on first in the session.

> extension-supplied aggregates.  But it'd do for starters.

We have the postgres_fdw extensions option to control function pushdown
to extensions.

> nobody thinks it's anywhere near ready to commit.

Previous versions of the patch had docs since I know I worked on
improving them.  I am not sure what happened to them.

-- 
  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: Thu, 21 Mar 2024 11:37:50 +0000

Hi. Mr.Momjian, Mr.Lane, Mr.Haas, hackers.

I apologize for any misunderstanding regarding the context of the attached patch and
the points on which I requested a review. Could you please allow me to clarify?

In the review around early December 2023, I received the following three issues pointed out by Mr.Haas[1].
1. Transmitting state value safely between machines
2. Making the patch clearer by adding SQL keywords
3. Fixing the behavior when the HAVING clause is present

In the email sent on February 22, 2024[2], I provided an update on the progress made in addressing these issues.
Regarding issue 1, I have only provided a proposed solution in the email and have not started the programming. 
Therefore, the latest patch is not in a commit-ready state. As mentioned later, we have also temporarily reverted the changes made to the documentation.
Before proceeding with the programming, I would like to discuss the proposed solution with the community and seek consensus.
If it is necessary to have source code in order to discuss, I can create a simple prototype so that I can receive your feedback.
Would you be able to provide your opinions on it?

Regarding issue 2., I have confirmed that creating a prototype allows us to address the issue and clear the patch.
In this prototype creation, the main purpose was to verify if the patch can be cleared and significant revisions were made to the previous version.
Therefore, I have removed all the document differences.
I have submitted a patch [3] that includes the fixes for issue 3. to the patch that was posted in [2].
Regarding the proposed solution for issue 1, unlike the patch posted in [3], 
we have a policy of not performing partial aggregation pushdown if we cannot guarantee compatibility and safety.
The latest patch in [3] is a POC patch. The patch that Mr. Momjian reviewed is this.
If user-facing documentation is needed for this POC patch, it can be added.

I apologize for the lack of explanation regarding this positioning, which may have caused misunderstandings regarding the patch posted in [3].

[1] https://www.postgresql.org/message-id/CA%2BTgmoYCrtOvk2f32qQKZV%3DjNL35tandf2A2Dp_2F5ASuiG1BA%40mail.gmail.com
[2] https://www.postgresql.org/message-id/TYAPR01MB5514F0CBD9CD4F84A261198195562%40TYAPR01MB5514.jpnprd01.prod.outlook.com
[3] https://www.postgresql.org/message-id/TYAPR01MB55141D18188AC86ADCE35FCB952F2%40TYAPR01MB5514.jpnprd01.prod.outlook.com

Sincerely yours,
Yuuki Fujii

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

---

From: Bruce Momjian <[email-masked]>
Date: Thu, 21 Mar 2024 18:01:00 -0400

> I apologize for the lack of explanation regarding this positioning, which may have caused misunderstandings regarding the patch posted in [3].

That makes sense.  Let's get you answers to those questions first before
you continue.