public inbox for [email protected]help / color / mirror / Atom feed
[PATCH] Remove the only [optional] in synopses with [only optionals in it]. 16+ messages / 7 participants [nested] [flat]
* [PATCH] Remove the only [optional] in synopses with [only optionals in it]. @ 2021-10-15 15:29 rir <[email protected]> 0 siblings, 0 replies; 16+ messages in thread From: rir @ 2021-10-15 15:29 UTC (permalink / raw) --- doc/src/sgml/ref/create_database.sgml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/doc/src/sgml/ref/create_database.sgml b/doc/src/sgml/ref/create_database.sgml index 41cb4068ec..ec831bb57f 100644 --- a/doc/src/sgml/ref/create_database.sgml +++ b/doc/src/sgml/ref/create_database.sgml @@ -22,7 +22,7 @@ PostgreSQL documentation <refsynopsisdiv> <synopsis> CREATE DATABASE <replaceable class="parameter">name</replaceable> - [ [ WITH ] [ OWNER [=] <replaceable class="parameter">user_name</replaceable> ] + [ WITH ] [ OWNER [=] <replaceable class="parameter">user_name</replaceable> ] [ TEMPLATE [=] <replaceable class="parameter">template</replaceable> ] [ ENCODING [=] <replaceable class="parameter">encoding</replaceable> ] [ LOCALE [=] <replaceable class="parameter">locale</replaceable> ] @@ -31,7 +31,7 @@ CREATE DATABASE <replaceable class="parameter">name</replaceable> [ TABLESPACE [=] <replaceable class="parameter">tablespace_name</replaceable> ] [ ALLOW_CONNECTIONS [=] <replaceable class="parameter">allowconn</replaceable> ] [ CONNECTION LIMIT [=] <replaceable class="parameter">connlimit</replaceable> ] - [ IS_TEMPLATE [=] <replaceable class="parameter">istemplate</replaceable> ] ] + [ IS_TEMPLATE [=] <replaceable class="parameter">istemplate</replaceable> ] </synopsis> </refsynopsisdiv> -- 2.20.1 --laf6vxg5pxajfm6t-- ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Partial aggregates pushdown @ 2023-11-20 22:48 Bruce Momjian <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Bruce Momjian @ 2023-11-20 22:48 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: [email protected] <[email protected]>; pgsql-hackers; Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]>; Alexander Pyhalov <[email protected]> On Mon, Nov 20, 2023 at 03:51:33PM -0500, Robert Haas wrote: > On Mon, Nov 13, 2023 at 3:26 AM [email protected] > <[email protected]> wrote: > > In postgres_fdw.sql, I have corrected the output format for floating point numbers > > by extra_float_digits. > > 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. Yes, I had to figure that out myself, and I was wondering how much detail to have in our docs vs README files vs. C comments. I think we should put more details somewhere. > 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. Agreed. I wasn't sure enough about this to add it when I was reviewing the patch. > 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. So use an SQL keyword to indicates a pushdown call? We could then automate the behavior rather than requiring special catalog functions? > 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 It does allow specification of extensions that can be pushed down. > 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. Right. I am not sure how to improve what the patch does. > 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. Makes sense. > 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. Understood. I wish I knew enough to add them myself. I can help if someone can supply the details. -- Bruce Momjian <[email protected]> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you. ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Partial aggregates pushdown @ 2023-11-21 17:16 Robert Haas <[email protected]> parent: Bruce Momjian <[email protected]> 0 siblings, 2 replies; 16+ messages in thread From: Robert Haas @ 2023-11-21 17:16 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; +Cc: [email protected] <[email protected]>; pgsql-hackers; Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]>; Alexander Pyhalov <[email protected]> On Mon, Nov 20, 2023 at 5:48 PM Bruce Momjian <[email protected]> wrote: > > 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. > > So use an SQL keyword to indicates a pushdown call? We could then > automate the behavior rather than requiring special catalog functions? Right. It would require more infrastructure in the parser, planner, and executor, but it would be infinitely reusable instead of needing a new thing for every aggregate. I think that might be better, but to be honest I'm not totally sure. > > 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. > > Makes sense. Actually, I think I was wrong about this. We can't handle ORDER BY or DISTINCT because we can't distinct-ify or order after we've already partially aggregated. At least not in general, and not without additional aggregate support functions. So what I said above was wrong with respect to those. Or so I believe, anyway. But I still don't see why HAVING should be a problem. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Partial aggregates pushdown @ 2023-11-21 20:34 Bruce Momjian <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 0 replies; 16+ messages in thread From: Bruce Momjian @ 2023-11-21 20:34 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: [email protected] <[email protected]>; pgsql-hackers; Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]>; Alexander Pyhalov <[email protected]> On Tue, Nov 21, 2023 at 12:16:41PM -0500, Robert Haas wrote: > On Mon, Nov 20, 2023 at 5:48 PM Bruce Momjian <[email protected]> wrote: > > > 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. > > > > So use an SQL keyword to indicates a pushdown call? We could then > > automate the behavior rather than requiring special catalog functions? > > Right. It would require more infrastructure in the parser, planner, > and executor, but it would be infinitely reusable instead of needing a > new thing for every aggregate. I think that might be better, but to be > honest I'm not totally sure. It would make it automatic. I guess we need to look at how big the patch is to do it. > > > 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. > > > > Makes sense. > > Actually, I think I was wrong about this. We can't handle ORDER BY or > DISTINCT because we can't distinct-ify or order after we've already > partially aggregated. At least not in general, and not without > additional aggregate support functions. So what I said above was wrong > with respect to those. Or so I believe, anyway. But I still don't see > why HAVING should be a problem. This should probably be documented in the patch. -- Bruce Momjian <[email protected]> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you. ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Partial aggregates pushdown @ 2023-11-22 06:32 Alexander Pyhalov <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 2 replies; 16+ messages in thread From: Alexander Pyhalov @ 2023-11-22 06:32 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Bruce Momjian <[email protected]>; [email protected] <[email protected]>; pgsql-hackers; Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]> Robert Haas писал 2023-11-21 20:16: >> > 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. >> >> Makes sense. > > Actually, I think I was wrong about this. We can't handle ORDER BY or > DISTINCT because we can't distinct-ify or order after we've already > partially aggregated. At least not in general, and not without > additional aggregate support functions. So what I said above was wrong > with respect to those. Or so I believe, anyway. But I still don't see > why HAVING should be a problem. Hi. HAVING is also a problem. Consider the following query SELECT count(a) FROM t HAVING count(a) > 10 - we can't push it down to foreign server as HAVING needs full aggregate result, but foreign server don't know it. -- Best regards, Alexander Pyhalov, Postgres Professional ^ permalink raw reply [nested|flat] 16+ messages in thread
* RE: Partial aggregates pushdown @ 2023-11-22 10:16 [email protected] <[email protected]> parent: Alexander Pyhalov <[email protected]> 1 sibling, 2 replies; 16+ messages in thread From: [email protected] @ 2023-11-22 10:16 UTC (permalink / raw) To: Alexander Pyhalov <[email protected]>; Robert Haas <[email protected]>; Bruce Momjian <[email protected]>; pgsql-hackers; +Cc: Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]>; [email protected] <[email protected]> Hi Mr. Haas, hackers. Thank you for your thoughtful comments. > From: Robert Haas <[email protected]> > Sent: Tuesday, November 21, 2023 5:52 AM > 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 believe this comment addresses a fundamental aspect of the approach. So, firstly, could we discuss whether we should fundamentally reconsider the approach? The approach adopted in this patch is as follows. Approach 1: Adding partial aggregation functions to the catalogs(pg_aggregate, pg_proc) The approach proposed by Mr.Haas is as follows. Approach 2: Adding a keyword to the SQL syntax to indicate partial aggregation requests The amount of code required to implement Approach 2 has not been investigated, but comparing Approach 1 and Approach 2 in other aspects, I believe they each have the following advantages and disadvantages. 1. Approach 1 (1) Advantages (a) No need to change the SQL syntax (2) Disadvantages (a) Catalog bloat As Mr.Haas pointed out, the catalog will bloat by adding partial aggregation functions (e.g. avg_p_int8(int8)) for each individual aggregate function (e.g. avg(int8)) in pg_aggregate and pg_proc (theoretically doubling the size). Some PostgreSQL developers and users may find this uncomfortable. (b) Increase in manual procedures Developers of new aggregate functions (both built-in and user-defined) need to manually add the partial aggregation functions when defining the aggregate functions. However, the procedure for adding partial aggregation functions for a certain aggregate function can be automated, so this problem can be resolved by improving the patch. The automation method involves the core part (AggregateCreate() and related functions) that executes the CREATE AGGREGATE command for user-defined functions. For built-in functions, it involves generating the initial data for the pg_aggregate catalog and pg_proc catalog from pg_aggregate.dat and pg_proc.dat (using the genbki.pl script and related scripts). 2. Approach 2 (1) Advantages (a) No need to add partial aggregate functions to the catalogs for each aggregation (2) Disadvantages (a) Need to add non-standard keywords to the SQL syntax. I did not choose Approach2 because I was not confident that the disadvantage mentioned in 2.(2)(a) would be accepted by the PostgreSQL development community. If it is accepted, I think Approach 2 is smarter. Could you please provide your opinion on which approach is preferable after comparing these two approaches? If we cannot say anything without comparing the amount of source code, as Mr.Momjian mentioned, we need to estimate the amount of source code required to implement Approach2. Sincerely yours, Yuuki Fujii -- Yuuki Fujii Information Technology R&D Center Mitsubishi Electric Corporation ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Partial aggregates pushdown @ 2023-11-22 21:16 Bruce Momjian <[email protected]> parent: [email protected] <[email protected]> 1 sibling, 1 reply; 16+ messages in thread From: Bruce Momjian @ 2023-11-22 21:16 UTC (permalink / raw) To: [email protected] <[email protected]>; +Cc: Alexander Pyhalov <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers; Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]> On Wed, Nov 22, 2023 at 10:16:16AM +0000, [email protected] wrote: > 2. Approach 2 > (1) Advantages > (a) No need to add partial aggregate functions to the catalogs for each aggregation > (2) Disadvantages > (a) Need to add non-standard keywords to the SQL syntax. > > I did not choose Approach2 because I was not confident that the disadvantage mentioned in 2.(2)(a) > would be accepted by the PostgreSQL development community. > If it is accepted, I think Approach 2 is smarter. > Could you please provide your opinion on which > approach is preferable after comparing these two approaches? I didn't know #2 was possible, but given the great number of catalog entries, doing it in the SQL grammar seems cleaner to me. -- Bruce Momjian <[email protected]> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you. ^ permalink raw reply [nested|flat] 16+ messages in thread
* RE: Partial aggregates pushdown @ 2023-11-27 07:04 [email protected] <[email protected]> parent: Bruce Momjian <[email protected]> 0 siblings, 0 replies; 16+ messages in thread From: [email protected] @ 2023-11-27 07:04 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers; +Cc: Alexander Pyhalov <[email protected]>; Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]>; [email protected] <[email protected]> Hi Mr.Momjian, Mr.Haas, hackers. > From: Bruce Momjian <[email protected]> > Sent: Thursday, November 23, 2023 6:16 AM > On Wed, Nov 22, 2023 at 10:16:16AM +0000, [email protected] wrote: > > 2. Approach 2 > > (1) Advantages > > (a) No need to add partial aggregate functions to the catalogs for > > each aggregation > > (2) Disadvantages > > (a) Need to add non-standard keywords to the SQL syntax. > > > > I did not choose Approach2 because I was not confident that the > > disadvantage mentioned in 2.(2)(a) would be accepted by the PostgreSQL development community. > > If it is accepted, I think Approach 2 is smarter. > > Could you please provide your opinion on which approach is preferable > > after comparing these two approaches? > > I didn't know #2 was possible, but given the great number of catalog entries, doing it in the SQL grammar seems cleaner > to me. Thank you for comments. Yes, I understand. > From: Bruce Momjian <[email protected]> > Sent: Wednesday, November 22, 2023 5:34 AM > On Tue, Nov 21, 2023 at 12:16:41PM -0500, Robert Haas wrote: > > On Mon, Nov 20, 2023 at 5:48 PM Bruce Momjian <[email protected]> wrote: > > > > 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. > > > > > > So use an SQL keyword to indicates a pushdown call? We could then > > > automate the behavior rather than requiring special catalog functions? > > > > Right. It would require more infrastructure in the parser, planner, > > and executor, but it would be infinitely reusable instead of needing a > > new thing for every aggregate. I think that might be better, but to be > > honest I'm not totally sure. > > It would make it automatic. I guess we need to look at how big the patch is to do it. I will investigate specifically which parts of the PostgreSQL source code need to be modified and how big the patch will be if you take this approach. Sincerely yours, Yuuki Fujii -- Yuuki Fujii Information Technology R&D Center Mitsubishi Electric Corporation ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Partial aggregates pushdown @ 2023-11-27 19:07 Robert Haas <[email protected]> parent: Alexander Pyhalov <[email protected]> 1 sibling, 0 replies; 16+ messages in thread From: Robert Haas @ 2023-11-27 19:07 UTC (permalink / raw) To: Alexander Pyhalov <[email protected]>; +Cc: Bruce Momjian <[email protected]>; [email protected] <[email protected]>; pgsql-hackers; Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]> On Wed, Nov 22, 2023 at 1:32 AM Alexander Pyhalov <[email protected]> wrote: > Hi. HAVING is also a problem. Consider the following query > > SELECT count(a) FROM t HAVING count(a) > 10 - we can't push it down to > foreign server as HAVING needs full aggregate result, but foreign server > don't know it. I don't see it that way. What we would push to the foreign server would be something like SELECT count(a) FROM t. Then, after we get the results back and combine the various partial counts locally, we would locally evaluate the HAVING clause afterward. That is, partial aggregation is a barrier to pushing down HAVING clause itself, but it doesn't preclude pushing down the aggregation. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Partial aggregates pushdown @ 2023-11-27 20:03 Robert Haas <[email protected]> parent: [email protected] <[email protected]> 1 sibling, 1 reply; 16+ messages in thread From: Robert Haas @ 2023-11-27 20:03 UTC (permalink / raw) To: [email protected] <[email protected]>; +Cc: Alexander Pyhalov <[email protected]>; Bruce Momjian <[email protected]>; pgsql-hackers; Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tom Lane <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]> On Wed, Nov 22, 2023 at 5:16 AM [email protected] <[email protected]> wrote: > I did not choose Approach2 because I was not confident that the disadvantage mentioned in 2.(2)(a) > would be accepted by the PostgreSQL development community. > If it is accepted, I think Approach 2 is smarter. > Could you please provide your opinion on which > approach is preferable after comparing these two approaches? > If we cannot say anything without comparing the amount of source code, as Mr.Momjian mentioned, > we need to estimate the amount of source code required to implement Approach2. I've had the same concern, that approach #2 would draw objections, so I think you were right to be cautious about it. I don't think it is a wonderful approach in all ways, but I do think that it is superior to approach #1. If we add dedicated support to the grammar, it is mostly a one-time effort, and after that, there should not be much need for anyone to be concerned about it. If we instead add extra aggregates, then that generates extra work every time someone writes a patch that adds a new aggregate to core. I have a difficult time believing that anyone will prefer an approach that involves an ongoing maintenance effort of that type over one that doesn't. One point that seems to me to be of particular importance is that if we add new aggregates, there is a risk that some future aggregate might do that incorrectly, so that the main aggregate works, but the secondary aggregate created for this feature does not work. That seems like it would be very frustrating for future code authors so I'd like to avoid the risk as much as we can. Also, I want to make one other point here about security and reliability. Right now, there is no way for a user to feed arbitrary data to a deserialization function. Since serialization and deserialization functions are only used in the context of parallel query, we always know that the data fed to the deserialization function must have come from the serialization function on the same machine. Nor can users call the deserialization function directly with arbitrary data of their own choosing, because users cannot call functions that take or return internal. But with this system, it becomes possible to feed arbitrary data to a deserialization function. The user could redefine the function on the remote side so that it produces arbitrary data of their choosing, and the local deserialization function will ingest it. That's potentially quite a significant problem. Consider for example that numericvar_deserialize() does no validity checking on any of the weight, sign, or dscale, but not all values for those fields are legal. Right now that doesn't matter, but if you can feed arbitrary data to that function, then it is. I don't know exactly what the consequences are if you can get that function to spit out a NumericVar with values outside the normal legal range. What can you do then? Store a bogus numeric on disk? Crash the server? Worst case, some problem like this could be a security issue allowing for escalation to superuser; more likely, it would be a crash bug, corrupt your database, or lead to unexpected and strange error messages. Unfortunately, I have the unpleasant suspicion that most internal-type aggregates will be affected by this problem. Consider, for example, string_agg_deserialize(). Generally, strings are one of the least-constrained data types, so you might hope that this function would be OK. But it doesn't look very promising. The first int4 in the serialized representation is the cursor, which would have to be bounds-checked, lest someone provide a cursor that falls outside the bounds of the StringInfo and, maybe, cause a reference to an arbitrary memory location. Then the rest of the representation is the actual data, which could be anything. This function is used for both bytea and for text, and for bytea, letting the payload be anything is OK. But for text, the supplied data shouldn't contain an embedded zero byte, or otherwise be invalid in the server encoding. If it were, that would provide a vector to inject invalidly encoded data into the database. This feature can't be allowed to do that. What could be a solution to this class of problems? One option is to just give up on supporting this feature for internal-type aggregates for now. That's easy enough to do, and just means we have less functionality, but it's sad because that's functionality we'd like to have. Another approach is to add necessary sanity checks to the relevant deserialization functions, but that seems a little hard to get right, and it would slow down parallel query cases which are probably going to be more common than the use of this feature. I think the slowdown might be significant, too. A third option is to change those aggregates in some way, like giving them a transition function that operates on some data type other than internal, but there again we have to be careful of slowdowns. A final option is to rethink the infrastructure in some way, like having a way to serialize to something other than bytea, for which we already have input functions with adequate checks. For instance, if string_agg_serialize() produced a record containing an integer column and a text or bytea column, we could attempt to ingest that record on the other side and presumably the right things would happen in the case of any invalid data. But I'm not quite sure what infrastructure would be required to make this kind of idea work. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Partial aggregates pushdown @ 2023-11-27 20:59 Tom Lane <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Tom Lane @ 2023-11-27 20:59 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: [email protected] <[email protected]>; Alexander Pyhalov <[email protected]>; Bruce Momjian <[email protected]>; pgsql-hackers; Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]> Robert Haas <[email protected]> writes: > Also, I want to make one other point here about security and > reliability. Right now, there is no way for a user to feed arbitrary > data to a deserialization function. Since serialization and > deserialization functions are only used in the context of parallel > query, we always know that the data fed to the deserialization > function must have come from the serialization function on the same > machine. Nor can users call the deserialization function directly with > arbitrary data of their own choosing, because users cannot call > functions that take or return internal. But with this system, it > becomes possible to feed arbitrary data to a deserialization function. Ouch. That is absolutely horrid --- we have a lot of stuff that depends on users not being able to get at "internal" values, and it sounds like the current proposal breaks all of that. Quite aside from security concerns, there is no justification for assuming that the "internal" values used on one platform/PG version are identical to those used on another. So if the idea is to ship back "internal" values from the remote server to the local one, I think it's basically impossible to make that work. Even if the partial-aggregate serialization value isn't "internal" but some more-narrowly-defined type, it is still an internal implementation detail of the aggregate. You have no right to assume that the remote server implements the aggregate the same way the local one does. If we start making such an assumption then we'll be unable to revise the implementation of an aggregate ever again. TBH, I think this entire proposal is dead in the water. Which is sad from a performance standpoint, but I can't see any way that we would not regret shipping a feature that makes such assumptions. regards, tom lane ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Partial aggregates pushdown @ 2023-11-27 21:10 Robert Haas <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Robert Haas @ 2023-11-27 21:10 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: [email protected] <[email protected]>; Alexander Pyhalov <[email protected]>; Bruce Momjian <[email protected]>; pgsql-hackers; Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]> On Mon, Nov 27, 2023 at 3:59 PM Tom Lane <[email protected]> wrote: > Even if the partial-aggregate serialization value isn't "internal" > but some more-narrowly-defined type, it is still an internal > implementation detail of the aggregate. You have no right to assume > that the remote server implements the aggregate the same way the > local one does. If we start making such an assumption then we'll > be unable to revise the implementation of an aggregate ever again. > > TBH, I think this entire proposal is dead in the water. Which is > sad from a performance standpoint, but I can't see any way that > we would not regret shipping a feature that makes such assumptions. I think it's ridiculous to just hold our breath and pretend like this feature isn't needed -- it's at least half a decade overdue. We engage in endless hand-wringing over local-remote symmetry in cases where other systems seem to effortlessly make that assumption and then get on with building new features. It's not that I disagree with the concern; we're *already* doing stuff that is unprincipled in a bunch of different areas and that could and occasionally does cause queries that push things to the remote side to return wrong answers, and I hate that. But the response to that can't be to refuse to add new features and maybe rip out the features we already have. Users don't like it when pushdown causes queries to return wrong answers, but they like it even less when the pushdown doesn't happen in the first place and the query runs until the heat death of the universe. I'm not entirely sure what the right design ideas are here, but giving up and refusing to add features ought to be completely off the table. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Partial aggregates pushdown @ 2023-11-27 21:23 Tom Lane <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Tom Lane @ 2023-11-27 21:23 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: [email protected] <[email protected]>; Alexander Pyhalov <[email protected]>; Bruce Momjian <[email protected]>; pgsql-hackers; Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]> Robert Haas <[email protected]> writes: > On Mon, Nov 27, 2023 at 3:59 PM Tom Lane <[email protected]> wrote: >> TBH, I think this entire proposal is dead in the water. Which is >> sad from a performance standpoint, but I can't see any way that >> we would not regret shipping a feature that makes such assumptions. > I think it's ridiculous to just hold our breath and pretend like this > feature isn't needed -- it's at least half a decade overdue. We engage > in endless hand-wringing over local-remote symmetry in cases where > other systems seem to effortlessly make that assumption and then get > on with building new features. Well, one of the founding principles of postgres_fdw was to be able to talk to PG servers that are not of the same version as yours. If we break that in the name of performance, we are going to have a lot of unhappy users. Even the ones who do get the benefit of the speedup are going to be unhappy when it breaks because they didn't upgrade local and remote at exactly the same time. Just because we'd like to have it doesn't make the patch workable in the real world. regards, tom lane ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Partial aggregates pushdown @ 2023-11-27 23:50 Robert Haas <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Robert Haas @ 2023-11-27 23:50 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: [email protected] <[email protected]>; Alexander Pyhalov <[email protected]>; Bruce Momjian <[email protected]>; pgsql-hackers; Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]> First of all, that last email of mine was snippy, and I apologize for it. Sorry. That said: On Mon, Nov 27, 2023 at 4:23 PM Tom Lane <[email protected]> wrote: > Well, one of the founding principles of postgres_fdw was to be able > to talk to PG servers that are not of the same version as yours. > If we break that in the name of performance, we are going to have > a lot of unhappy users. Even the ones who do get the benefit of > the speedup are going to be unhappy when it breaks because they > didn't upgrade local and remote at exactly the same time. I agree with this. > Just because we'd like to have it doesn't make the patch workable > in the real world. And also with this in concept - I'd like to plan arbitrarily complicated queries perfectly and near-instantly, and then execute them at faster-than-light speed, but we can't. However, I don't understand the fatalism with respect to the feature at hand. As I said before, it's not like no other product has made this work. Sure, some of those products may not have the extensible system of data types that we do, or may not care about cross-version communication, but those don't seem like good enough reasons to just immediately give up. TBH, I suspect even some PG forks have made this work, like maybe PGXC or PGXL, although I don't know for certain. We might not like the trade-offs they made to get there, but we haven't even talked through possible design ideas yet, so it seems way too early to give up. One of the things that I think is a problem in this area is that the ways we have to configure FDW connections are just not very rich. We're trying to cram everything into a set of strings that can be attached to the foreign server or the user mapping, but that's not a very good fit for something like how all the local SQL functions that might exist map onto all of the remote SQL functions that might exist. Now you might well say that we don't want the act of configuring a foreign data wrapper to be insanely complicated, and I would agree with that. But, on the other hand, as Larry Wall once said, a good programming language makes simple things simple and complicated things possible. I think our current configuration system is only accomplishing the first of those goals. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Partial aggregates pushdown @ 2023-11-28 10:23 Ashutosh Bapat <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Ashutosh Bapat @ 2023-11-28 10:23 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Tom Lane <[email protected]>; [email protected] <[email protected]>; Alexander Pyhalov <[email protected]>; Bruce Momjian <[email protected]>; pgsql-hackers; Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]> On Tue, Nov 28, 2023 at 5:21 AM Robert Haas <[email protected]> wrote: > > TBH, I suspect even some PG forks have made this work, like maybe PGXC > or PGXL, although I don't know for certain. We might not like the > trade-offs they made to get there, but we haven't even talked through > possible design ideas yet, so it seems way too early to give up. If my memory serves me right, PGXC implemented partial aggregation only when the output of partial aggregate was a SQL data type (non-Internal, non-Unknown). But I may be wrong. But at that time, JSONB wasn't there or wasn't that widespread. Problem with Internal is it's just a binary string whose content can change across version and which can be interpreted differently across different versions. There is no metadata in it to know how to interpret it. We can add that metadata to JSONB. The result of partial aggregate can be sent as a JSONB. If the local server finds the JSONB familiar it will construct the right partial aggregate value otherwise it will throw an error. If there's a way to even avoid that error (by looking at server version etc.) the error can be avoided too. But JSONB leaves very very less chance that the value will be interpreted wrong. Downside is we are tying PARTIAL's output to be JSONB thus tying SQL syntax with a data type. Does that look acceptable? -- Best Wishes, Ashutosh Bapat ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Partial aggregates pushdown @ 2023-11-28 12:44 Robert Haas <[email protected]> parent: Ashutosh Bapat <[email protected]> 0 siblings, 0 replies; 16+ messages in thread From: Robert Haas @ 2023-11-28 12:44 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: Tom Lane <[email protected]>; [email protected] <[email protected]>; Alexander Pyhalov <[email protected]>; Bruce Momjian <[email protected]>; pgsql-hackers; Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]> On Tue, Nov 28, 2023 at 5:24 AM Ashutosh Bapat <[email protected]> wrote: > If my memory serves me right, PGXC implemented partial aggregation > only when the output of partial aggregate was a SQL data type > (non-Internal, non-Unknown). But I may be wrong. But at that time, > JSONB wasn't there or wasn't that widespread. > > Problem with Internal is it's just a binary string whose content can > change across version and which can be interpreted differently across > different versions. There is no metadata in it to know how to > interpret it. We can add that metadata to JSONB. The result of partial > aggregate can be sent as a JSONB. If the local server finds the JSONB > familiar it will construct the right partial aggregate value otherwise > it will throw an error. If there's a way to even avoid that error (by > looking at server version etc.) the error can be avoided too. But > JSONB leaves very very less chance that the value will be interpreted > wrong. Downside is we are tying PARTIAL's output to be JSONB thus > tying SQL syntax with a data type. > > Does that look acceptable? If somebody had gone to the trouble of making this work, and had done a good job, I wouldn't vote against it, but in a vacuum, I'm not sure it's the best design. The problem in my view is that working with JSON is not actually very pleasant. It's super-easy to generate, and super-easy for humans to read. But parsing and validating it is a pain. You basically have to have two parsers, one to do syntactical validation and then a second one to ensure that the structure of the document and the contents of each item are as expected. See parse_manifest.c for an example of what I mean by that. Now, if we add new code, it can reuse the JSON parser we've already got, so it's not that you need to write a new JSON parser for every new application of JSON, but the semantic validator (a la parse_manifest.c) isn't necessarily any less code than a whole new parser for a bespoke format. To make that a bit more concrete, for something like string_agg(), is it easier to write a validator for the existing deserialization function that accepts a bytea blob, or to write a validator for a JSON blob that we could be passing instead? My suspicion is that the former is less work and easier to verify, but it's possible I'm wrong about that and they're more or less equal. I don't really see any way that the JSON thing is straight-up better; at best it's a toss-up in terms of amount of code. Now somebody could still make an argument that they would like JSON better because it would be more useful for some purpose other than this feature, and that is fine, but here I'm just thinking about this feature in particular. My personal suspicion is that the easiest way to support internal-type aggregates here is to convert them to use an array or record type as a transition state instead, or maybe serialize the internal state to one of those things instead of to bytea. I suspect that would allow us to leverage more of our existing validation infrastructure than using JSON or sticking with bytea. But I'm certainly amenable to other points of view. I'm not trying to pretend that my gut feeling is necessarily correct; I'm just explaining what I currently think. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2023-11-28 12:44 UTC | newest] Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-10-15 15:29 [PATCH] Remove the only [optional] in synopses with [only optionals in it]. rir <[email protected]> 2023-11-20 22:48 Re: Partial aggregates pushdown Bruce Momjian <[email protected]> 2023-11-21 17:16 ` Re: Partial aggregates pushdown Robert Haas <[email protected]> 2023-11-21 20:34 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> 2023-11-22 06:32 ` Re: Partial aggregates pushdown Alexander Pyhalov <[email protected]> 2023-11-22 10:16 ` RE: Partial aggregates pushdown [email protected] <[email protected]> 2023-11-22 21:16 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> 2023-11-27 07:04 ` RE: Partial aggregates pushdown [email protected] <[email protected]> 2023-11-27 20:03 ` Re: Partial aggregates pushdown Robert Haas <[email protected]> 2023-11-27 20:59 ` Re: Partial aggregates pushdown Tom Lane <[email protected]> 2023-11-27 21:10 ` Re: Partial aggregates pushdown Robert Haas <[email protected]> 2023-11-27 21:23 ` Re: Partial aggregates pushdown Tom Lane <[email protected]> 2023-11-27 23:50 ` Re: Partial aggregates pushdown Robert Haas <[email protected]> 2023-11-28 10:23 ` Re: Partial aggregates pushdown Ashutosh Bapat <[email protected]> 2023-11-28 12:44 ` Re: Partial aggregates pushdown Robert Haas <[email protected]> 2023-11-27 19:07 ` Re: Partial aggregates pushdown Robert Haas <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox