public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tom Lane <[email protected]>
To: Bruce Momjian <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Alexander Pyhalov <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Cc: Stephen Frost <[email protected]>
Cc: Finnerty, Jim <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Tomas Vondra <[email protected]>
Cc: Julien Rouhaud <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: vignesh C <[email protected]>
Subject: Re: Partial aggregates pushdown
Date: Tue, 19 Mar 2024 17:29:07 -0400
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CA+Tgmob7Dn_+UN1qgb_NUEoeg1jdug5mh+qz7q3Ed1NHAL_LNQ@mail.gmail.com>
<[email protected]>
<TYAPR01MB551411A7DDFF0B38C5AAEDFE9584A@TYAPR01MB5514.jpnprd01.prod.outlook.com>
<CA+TgmoYCrtOvk2f32qQKZV=jNL35tandf2A2Dp_2F5ASuiG1BA@mail.gmail.com>
<TYAPR01MB5514C939F39F97BC171F58FD958BA@TYAPR01MB5514.jpnprd01.prod.outlook.com>
<CALDaNm0S5fb_8o+TtR8bXf3zOpqabOBLy0EJ3+oVzHH_dzpXvw@mail.gmail.com>
<TYAPR01MB5514F0CBD9CD4F84A261198195562@TYAPR01MB5514.jpnprd01.prod.outlook.com>
<[email protected]>
<TYAPR01MB55141D18188AC86ADCE35FCB952F2@TYAPR01MB5514.jpnprd01.prod.outlook.com>
<[email protected]>
Bruce Momjian <[email protected]> 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
view thread (5+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Partial aggregates pushdown
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox