Re: Partial aggregates pushdown

On Tue, Mar 19, 2024 at 05:29:07PM -0400, Tom Lane wrote:
> 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.

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.

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

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.

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

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

> 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

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

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

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

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

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  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Only you can decide what is important to you.