public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 03/10] Fix comment wrong since bf2a691e02d7766f185d9d8e0f092222a5c0a129 13+ messages / 5 participants [nested] [flat]
* [PATCH 03/10] Fix comment wrong since bf2a691e02d7766f185d9d8e0f092222a5c0a129 @ 2021-08-21 00:40 Justin Pryzby <[email protected]> 0 siblings, 0 replies; 13+ messages in thread From: Justin Pryzby @ 2021-08-21 00:40 UTC (permalink / raw) --- src/backend/statistics/extended_stats.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c index 5fa36e0036..542271c9e4 100644 --- a/src/backend/statistics/extended_stats.c +++ b/src/backend/statistics/extended_stats.c @@ -701,7 +701,7 @@ examine_expression(Node *expr, int stattarget) /* * Using 'vacatts' of size 'nvacatts' as input data, return a newly built * VacAttrStats array which includes only the items corresponding to - * attributes indicated by 'stxkeys'. If we don't have all of the per column + * attributes indicated by 'attrs'. If we don't have all of the per column * stats available to compute the extended stats, then we return NULL to indicate * to the caller that the stats should not be built. */ -- 2.17.0 --udcq9yAoWb9A4FsZ Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="0004-tablefunc.c-Fix-comment.patch" ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Partial aggregates pushdown @ 2024-07-08 12:30 Jelte Fennema-Nio <[email protected]> 2024-08-08 11:48 ` Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Jelte Fennema-Nio @ 2024-07-08 12:30 UTC (permalink / raw) To: [email protected] <[email protected]>; +Cc: pgsql-hackers; Bruce Momjian <[email protected]>; Robert Haas <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]>; vignesh C <[email protected]>; Alexander Pyhalov <[email protected]> On Mon, 8 Jul 2024 at 14:12, [email protected] <[email protected]> wrote: > The best thing about Approach2 is that it lets us send state values using the existing data type system. > I'm worried that if we choose Approach2, we might face some limits because we have to create new types. > But, we might be able to fix these limits if we look into it more. > > Approach1 doesn't make new types, so we can avoid these limits. > But, it means we have to make export/import functions that are similar to the typsend/typreceive functions. > So, we need to make sure if we really need this method. > > Is this the right understanding? Yeah, correct. To clarify my reasoning a bit more: IMHO, the main downside of implementing Approach1 is that we then end up with two different mechanisms to "take data from memory and serialize it in a way in which it can be sent over the network". I'd very much prefer if we could have a single system responsible for that task. So if there's issues with the current system (e.g. having to implement typinput/typoutput), then I'd rather address these problems in the existing system. Only if that turns out to be impossible for some reason, then I think I'd prefer Approach1. Personally, even if the Approach2 requires a bit more code, then I'd still prefer a single serialization system over having two serializiation systems. ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Partial aggregates pushdown 2024-07-08 12:30 Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> @ 2024-08-08 11:48 ` Jelte Fennema-Nio <[email protected]> 2024-08-15 21:12 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> 2024-08-21 15:41 ` Re: Partial aggregates pushdown Tomas Vondra <[email protected]> 0 siblings, 2 replies; 13+ messages in thread From: Jelte Fennema-Nio @ 2024-08-08 11:48 UTC (permalink / raw) To: [email protected] <[email protected]>; +Cc: pgsql-hackers; Bruce Momjian <[email protected]>; Robert Haas <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]>; vignesh C <[email protected]>; Alexander Pyhalov <[email protected]> SUMMARY OF THREAD The design of patch 0001 is agreed upon by everyone on the thread (so far). This adds the PARTIAL_AGGREGATE label for aggregates, which will cause the finalfunc not to run. It also starts using PARTIAL_AGGREGATE for pushdown of aggregates in postgres_fdw. In 0001 PARTIAL_AGGREGATE is only supported for aggregates with a non-internal/pseudo type as the stype. The design for patch 0002 is still under debate. This would expand on the functionality added by adding support for PARTIAL_AGGREGATE for aggregates with an internal stype. This is done by returning a byte array containing the bytes that the serialfunc of the aggregate returns. A competing proposal for 0002 is to instead change aggregates to not use an internal stype anymore, and create dedicated types. The main downside here is that infunc and outfunc would need to be added for text serialization, in addition to the binary serialization. An open question is: Can we change the requirements for CREATE TYPE, so that types can be created without infunc and outfunc. WHAT IS NEEDED? The things needed for this patch are that docs need to be added, and detailed codereview needs to be done. Feedback from more people on the two competing proposals for 0002 would be very helpful in making a decision. ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Partial aggregates pushdown 2024-07-08 12:30 Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-08 11:48 ` Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> @ 2024-08-15 21:12 ` Bruce Momjian <[email protected]> 2024-08-20 08:07 ` Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 1 sibling, 1 reply; 13+ messages in thread From: Bruce Momjian @ 2024-08-15 21:12 UTC (permalink / raw) To: Jelte Fennema-Nio <[email protected]>; +Cc: [email protected] <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]>; vignesh C <[email protected]>; Alexander Pyhalov <[email protected]> On Thu, Aug 8, 2024 at 01:48:49PM +0200, Jelte Fennema-Nio wrote: > SUMMARY OF THREAD > > The design of patch 0001 is agreed upon by everyone on the thread (so > far). This adds the PARTIAL_AGGREGATE label for aggregates, which will > cause the finalfunc not to run. It also starts using PARTIAL_AGGREGATE > for pushdown of aggregates in postgres_fdw. In 0001 PARTIAL_AGGREGATE > is only supported for aggregates with a non-internal/pseudo type as > the stype. > > The design for patch 0002 is still under debate. This would expand on > the functionality added by adding support for PARTIAL_AGGREGATE for > aggregates with an internal stype. This is done by returning a byte > array containing the bytes that the serialfunc of the aggregate > returns. > > A competing proposal for 0002 is to instead change aggregates to not > use an internal stype anymore, and create dedicated types. The main > downside here is that infunc and outfunc would need to be added for > text serialization, in addition to the binary serialization. An open > question is: Can we change the requirements for CREATE TYPE, so that > types can be created without infunc and outfunc. > > WHAT IS NEEDED? > > The things needed for this patch are that docs need to be added, and > detailed codereview needs to be done. > > Feedback from more people on the two competing proposals for 0002 > would be very helpful in making a decision. First, I am sorry to be replying so late --- I have been traveling for the past four weeks. Second, I consider this feature a big part of sharding, and I think sharding is Postgres's biggest missing feature. I talk about this patch often when asked about what Postgres is working on. Third, I would like to show a more specific example to clarify what is being considered above. If we look at MAX(), we can have FDWs return the max for each FDW, and the coordinator can chose the highest value. This is the patch 1 listed above. These can return the pg_aggregate.aggtranstype data type using the pg_type.typoutput text output. The second case is for something like AVG(), which must return the SUM() and COUNT(), and we currently have no way to return multiple text values on the wire. For patch 0002, we have the option of creating functions that can do this and record them in new pg_attribute columns, or we can create a data type with these functions, and assign the data type to pg_aggregate.aggtranstype. Is that accurate? -- 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] 13+ messages in thread
* Re: Partial aggregates pushdown 2024-07-08 12:30 Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-08 11:48 ` Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-15 21:12 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> @ 2024-08-20 08:07 ` Jelte Fennema-Nio <[email protected]> 2024-08-20 16:50 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Jelte Fennema-Nio @ 2024-08-20 08:07 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; +Cc: [email protected] <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]>; vignesh C <[email protected]>; Alexander Pyhalov <[email protected]> On Thu, 15 Aug 2024 at 23:12, Bruce Momjian <[email protected]> wrote: > Third, I would like to show a more specific example to clarify what is > being considered above. If we look at MAX(), we can have FDWs return > the max for each FDW, and the coordinator can chose the highest value. > This is the patch 1 listed above. These can return the > pg_aggregate.aggtranstype data type using the pg_type.typoutput text > output. > > The second case is for something like AVG(), which must return the SUM() > and COUNT(), and we currently have no way to return multiple text values > on the wire. For patch 0002, we have the option of creating functions > that can do this and record them in new pg_attribute columns, or we can > create a data type with these functions, and assign the data type to > pg_aggregate.aggtranstype. > > Is that accurate? It's close to accurate, but not entirely. Patch 1 would actually solves some AVG cases too, because some AVG implementations use an SQL array type to store the transtype instead of an internal type. And by using an SQL array type we *can* send multiple text values on the wire. See below for a list of those aggregates: > select p.oid::regprocedure from pg_aggregate a join pg_proc p on a.aggfnoid = p.oid where aggfinalfn != 0 and aggtranstype::regtype not in ('internal', 'anyenum', 'anyelement', 'anyrange', 'anyarray', 'anymultirange'); oid ─────────────────────────────────────────────────── avg(integer) avg(smallint) avg(real) avg(double precision) avg(interval) var_pop(real) var_pop(double precision) var_samp(real) var_samp(double precision) variance(real) variance(double precision) stddev_pop(real) stddev_pop(double precision) stddev_samp(real) stddev_samp(double precision) stddev(real) stddev(double precision) regr_sxx(double precision,double precision) regr_syy(double precision,double precision) regr_sxy(double precision,double precision) regr_avgx(double precision,double precision) regr_avgy(double precision,double precision) regr_r2(double precision,double precision) regr_slope(double precision,double precision) regr_intercept(double precision,double precision) covar_pop(double precision,double precision) covar_samp(double precision,double precision) corr(double precision,double precision) (28 rows) And to be clear, these are in addition to the MAX type of aggregates you were describing: > select p.oid::regprocedure from pg_aggregate a join pg_proc p on a.aggfnoid = p.oid where aggfinalfn = 0 and aggtranstype::regtype not in ('internal', 'anyenum', 'anyelement', 'anyrange', 'anyarray', 'anymultirange'); oid ─────────────────────────────────────────────── sum(integer) sum(smallint) sum(real) sum(double precision) sum(money) sum(interval) max(bigint) max(integer) max(smallint) max(oid) max(real) max(double precision) max(date) max(time without time zone) max(time with time zone) max(money) max(timestamp without time zone) max(timestamp with time zone) max(interval) max(text) max(numeric) max(character) max(tid) max(inet) max(pg_lsn) max(xid8) min(bigint) min(integer) min(smallint) min(oid) min(real) min(double precision) min(date) min(time without time zone) min(time with time zone) min(money) min(timestamp without time zone) min(timestamp with time zone) min(interval) min(text) min(numeric) min(character) min(tid) min(inet) min(pg_lsn) min(xid8) count("any") count() regr_count(double precision,double precision) bool_and(boolean) bool_or(boolean) every(boolean) bit_and(smallint) bit_or(smallint) bit_xor(smallint) bit_and(integer) bit_or(integer) bit_xor(integer) bit_and(bigint) bit_or(bigint) bit_xor(bigint) bit_and(bit) bit_or(bit) bit_xor(bit) xmlagg(xml) (65 rows) ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Partial aggregates pushdown 2024-07-08 12:30 Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-08 11:48 ` Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-15 21:12 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> 2024-08-20 08:07 ` Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> @ 2024-08-20 16:50 ` Bruce Momjian <[email protected]> 2024-08-20 17:03 ` Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Bruce Momjian @ 2024-08-20 16:50 UTC (permalink / raw) To: Jelte Fennema-Nio <[email protected]>; +Cc: [email protected] <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]>; vignesh C <[email protected]>; Alexander Pyhalov <[email protected]> On Tue, Aug 20, 2024 at 10:07:32AM +0200, Jelte Fennema-Nio wrote: > On Thu, 15 Aug 2024 at 23:12, Bruce Momjian <[email protected]> wrote: > > Third, I would like to show a more specific example to clarify what is > > being considered above. If we look at MAX(), we can have FDWs return > > the max for each FDW, and the coordinator can chose the highest value. > > This is the patch 1 listed above. These can return the > > pg_aggregate.aggtranstype data type using the pg_type.typoutput text > > output. > > > > The second case is for something like AVG(), which must return the SUM() > > and COUNT(), and we currently have no way to return multiple text values > > on the wire. For patch 0002, we have the option of creating functions > > that can do this and record them in new pg_attribute columns, or we can > > create a data type with these functions, and assign the data type to > > pg_aggregate.aggtranstype. > > > > Is that accurate? > > It's close to accurate, but not entirely. Patch 1 would actually > solves some AVG cases too, because some AVG implementations use an SQL > array type to store the transtype instead of an internal type. And by > using an SQL array type we *can* send multiple text values on the > wire. See below for a list of those aggregates: Okay, so we can do MAX easily, and AVG if the count can be represented as the same data type as the sum? Is that correct? Our only problem is that something like AVG(interval) can't use an array because arrays have to have the same data type for all array elements, and an interval can't represent a count? -- 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] 13+ messages in thread
* Re: Partial aggregates pushdown 2024-07-08 12:30 Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-08 11:48 ` Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-15 21:12 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> 2024-08-20 08:07 ` Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-20 16:50 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> @ 2024-08-20 17:03 ` Jelte Fennema-Nio <[email protected]> 0 siblings, 0 replies; 13+ messages in thread From: Jelte Fennema-Nio @ 2024-08-20 17:03 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; +Cc: [email protected] <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Tom Lane <[email protected]>; Stephen Frost <[email protected]>; Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]>; vignesh C <[email protected]>; Alexander Pyhalov <[email protected]> On Tue, 20 Aug 2024 at 18:50, Bruce Momjian <[email protected]> wrote: > Okay, so we can do MAX easily, and AVG if the count can be represented > as the same data type as the sum? Is that correct? Our only problem is > that something like AVG(interval) can't use an array because arrays have > to have the same data type for all array elements, and an interval can't > represent a count? Close, but still not completely correct. AVG(bigint) can also not be supported by patch 1, because the sum and the count for that both stored using an int128. So we'd need an array of int128, and there's currently no int128 SQL type. ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Partial aggregates pushdown 2024-07-08 12:30 Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-08 11:48 ` Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> @ 2024-08-21 15:41 ` Tomas Vondra <[email protected]> 2024-08-22 11:05 ` Re: Partial aggregates pushdown Ashutosh Bapat <[email protected]> 2024-08-22 18:56 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> 1 sibling, 2 replies; 13+ messages in thread From: Tomas Vondra @ 2024-08-21 15:41 UTC (permalink / raw) To: [email protected] On 8/8/24 13:48, Jelte Fennema-Nio wrote: > SUMMARY OF THREAD > > The design of patch 0001 is agreed upon by everyone on the thread (so > far). This adds the PARTIAL_AGGREGATE label for aggregates, which will > cause the finalfunc not to run. It also starts using PARTIAL_AGGREGATE > for pushdown of aggregates in postgres_fdw. In 0001 PARTIAL_AGGREGATE > is only supported for aggregates with a non-internal/pseudo type as > the stype. > I don't have a strong opinion on this, but I wonder if someone might object this essentially extends the syntax with something that is not (and never will be) in the SQL standard. I wonder if there's some precedent for encoding such explicit execution instructions into the query itself? That reminds me - the PARTIAL_AGGREGATE label is per aggregate, but I don't think it makes much sense to do this only for some aggregates, right? Do we have a way to make sure the query is "consistent"? I'm not sure if doing this on the source (before pushdown) is enough. Could there be a difference in what the remote instance supports? The only alternative that I can think of (and that I believe was already mentioned in this thread) is to set some GUC that forces the top-most query level to do this (all aggregates at that level). That's have the benefit of always affecting all aggregates. > The design for patch 0002 is still under debate. This would expand on > the functionality added by adding support for PARTIAL_AGGREGATE for > aggregates with an internal stype. This is done by returning a byte > array containing the bytes that the serialfunc of the aggregate > returns. > > A competing proposal for 0002 is to instead change aggregates to not > use an internal stype anymore, and create dedicated types. The main > downside here is that infunc and outfunc would need to be added for > text serialization, in addition to the binary serialization. An open > question is: Can we change the requirements for CREATE TYPE, so that > types can be created without infunc and outfunc. > I think it's +0.5 for the new dedicated data types from me. I admit I'm too lazy to read the whole thread from scratch, but I believe we did discuss the possibility to reuse the serial/deserial functions we already have, but the reason against that was the missing cross-version stability. Parallel queries always run within a single instance, hence there are no concerns about other versions. But this is meant to support the remote node having a wildly different version. I guess we might introduce another pair of serial/deserial functions, with this guarantee. I know we (me and Jelte) discussed that in person at some point, and there were arguments for doing the data types. But I managed to forget the details :-( > WHAT IS NEEDED? > > The things needed for this patch are that docs need to be added, and > detailed codereview needs to be done. Yeah, I think the docs are must-have for a proper review. > Feedback from more people on the two competing proposals for 0002 > would be very helpful in making a decision. > regards -- Tomas Vondra ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Partial aggregates pushdown 2024-07-08 12:30 Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-08 11:48 ` Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-21 15:41 ` Re: Partial aggregates pushdown Tomas Vondra <[email protected]> @ 2024-08-22 11:05 ` Ashutosh Bapat <[email protected]> 1 sibling, 0 replies; 13+ messages in thread From: Ashutosh Bapat @ 2024-08-22 11:05 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: [email protected] On Wed, Aug 21, 2024 at 9:11 PM Tomas Vondra <[email protected]> wrote: > > > > On 8/8/24 13:48, Jelte Fennema-Nio wrote: > > SUMMARY OF THREAD > > > > The design of patch 0001 is agreed upon by everyone on the thread (so > > far). This adds the PARTIAL_AGGREGATE label for aggregates, which will > > cause the finalfunc not to run. It also starts using PARTIAL_AGGREGATE > > for pushdown of aggregates in postgres_fdw. In 0001 PARTIAL_AGGREGATE > > is only supported for aggregates with a non-internal/pseudo type as > > the stype. > > > > I don't have a strong opinion on this, but I wonder if someone might > object this essentially extends the syntax with something that is not > (and never will be) in the SQL standard. I wonder if there's some > precedent for encoding such explicit execution instructions into the > query itself? This feature might be a useful feature to run aggregation in a federated database across many source databases. So people in the community who participate in SQL standard may add it there. While implementing the feature, we might think of it as influencing the execution but I don't see it that way. It's a feature allowing users to access a pre-finalization state of an aggregate which they can use to combine with such states from other data sources. There may be other uses as well. But adding it as a SQL feature means some standardization of what is partial aggregate for each aggregate function - the notions of which are intuitive but they need to be standardized. Of course, going this way means that it will take longer for the feature to be available but it won't look like a kludge at least. -- Best Wishes, Ashutosh Bapat ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Partial aggregates pushdown 2024-07-08 12:30 Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-08 11:48 ` Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-21 15:41 ` Re: Partial aggregates pushdown Tomas Vondra <[email protected]> @ 2024-08-22 18:56 ` Bruce Momjian <[email protected]> 2024-08-22 19:54 ` Re: Partial aggregates pushdown Tomas Vondra <[email protected]> 1 sibling, 1 reply; 13+ messages in thread From: Bruce Momjian @ 2024-08-22 18:56 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: [email protected] On Wed, Aug 21, 2024 at 05:41:02PM +0200, Tomas Vondra wrote: > On 8/8/24 13:48, Jelte Fennema-Nio wrote: > > SUMMARY OF THREAD > > > > The design of patch 0001 is agreed upon by everyone on the thread (so > > far). This adds the PARTIAL_AGGREGATE label for aggregates, which will > > cause the finalfunc not to run. It also starts using PARTIAL_AGGREGATE > > for pushdown of aggregates in postgres_fdw. In 0001 PARTIAL_AGGREGATE > > is only supported for aggregates with a non-internal/pseudo type as > > the stype. > > I don't have a strong opinion on this, but I wonder if someone might > object this essentially extends the syntax with something that is not > (and never will be) in the SQL standard. I wonder if there's some > precedent for encoding such explicit execution instructions into the > query itself? > > That reminds me - the PARTIAL_AGGREGATE label is per aggregate, but I > don't think it makes much sense to do this only for some aggregates, > right? Do we have a way to make sure the query is "consistent"? I'm not > sure if doing this on the source (before pushdown) is enough. Could > there be a difference in what the remote instance supports? > > The only alternative that I can think of (and that I believe was already > mentioned in this thread) is to set some GUC that forces the top-most > query level to do this (all aggregates at that level). That's have the > benefit of always affecting all aggregates. You make a very good point above. Would there ever be cases where a targetlist would have multiple aggregates, and some can be pushed down, and some have to return all matching rows so the sender can compute the aggregate? If so, how would we handle that? How does parallelism handle that now? > > The design for patch 0002 is still under debate. This would expand on > > the functionality added by adding support for PARTIAL_AGGREGATE for > > aggregates with an internal stype. This is done by returning a byte > > array containing the bytes that the serialfunc of the aggregate > > returns. > > > > A competing proposal for 0002 is to instead change aggregates to not > > use an internal stype anymore, and create dedicated types. The main > > downside here is that infunc and outfunc would need to be added for > > text serialization, in addition to the binary serialization. An open > > question is: Can we change the requirements for CREATE TYPE, so that > > types can be created without infunc and outfunc. > > > > I think it's +0.5 for the new dedicated data types from me. > > I admit I'm too lazy to read the whole thread from scratch, but I > believe we did discuss the possibility to reuse the serial/deserial > functions we already have, but the reason against that was the missing > cross-version stability. Parallel queries always run within a single > instance, hence there are no concerns about other versions. But this is > meant to support the remote node having a wildly different version. It is more than different versions. Different CPU architectures can store data types differently in binary. It would be kind of interesting to have a serial/deserial that was in text format. I guess that is where my RECORD idea came from that I just emailed to the list. What I would really like is something similar to pg_proc.proargtypes (which is data type oidvector) for pg_aggregate where we can supply the oids of the pg_aggregate.aggtranstype and construct a record on the fly to return to the FDW caller, rather than having to create specialized functions for every aggregate that needs to return several different data types, e.g., AVG(interval), #4 in my previous email. I realize this would require the creation of data types int128 and int128 array. > I guess we might introduce another pair of serial/deserial functions, > with this guarantee. I know we (me and Jelte) discussed that in person > at some point, and there were arguments for doing the data types. But I > managed to forget the details :-( > > > WHAT IS NEEDED? > > > > The things needed for this patch are that docs need to be added, and > > detailed codereview needs to be done. > > Yeah, I think the docs are must-have for a proper review. I think the docs are on hold until we decide on a transfer method. -- 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] 13+ messages in thread
* Re: Partial aggregates pushdown 2024-07-08 12:30 Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-08 11:48 ` Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-21 15:41 ` Re: Partial aggregates pushdown Tomas Vondra <[email protected]> 2024-08-22 18:56 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> @ 2024-08-22 19:54 ` Tomas Vondra <[email protected]> 2024-08-22 20:07 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Tomas Vondra @ 2024-08-22 19:54 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; +Cc: [email protected] On 8/22/24 20:56, Bruce Momjian wrote: > On Wed, Aug 21, 2024 at 05:41:02PM +0200, Tomas Vondra wrote: >> On 8/8/24 13:48, Jelte Fennema-Nio wrote: >>> SUMMARY OF THREAD >>> >>> The design of patch 0001 is agreed upon by everyone on the thread (so >>> far). This adds the PARTIAL_AGGREGATE label for aggregates, which will >>> cause the finalfunc not to run. It also starts using PARTIAL_AGGREGATE >>> for pushdown of aggregates in postgres_fdw. In 0001 PARTIAL_AGGREGATE >>> is only supported for aggregates with a non-internal/pseudo type as >>> the stype. >> >> I don't have a strong opinion on this, but I wonder if someone might >> object this essentially extends the syntax with something that is not >> (and never will be) in the SQL standard. I wonder if there's some >> precedent for encoding such explicit execution instructions into the >> query itself? >> >> That reminds me - the PARTIAL_AGGREGATE label is per aggregate, but I >> don't think it makes much sense to do this only for some aggregates, >> right? Do we have a way to make sure the query is "consistent"? I'm not >> sure if doing this on the source (before pushdown) is enough. Could >> there be a difference in what the remote instance supports? >> >> The only alternative that I can think of (and that I believe was already >> mentioned in this thread) is to set some GUC that forces the top-most >> query level to do this (all aggregates at that level). That's have the >> benefit of always affecting all aggregates. > > You make a very good point above. Would there ever be cases where a > targetlist would have multiple aggregates, and some can be pushed down, > and some have to return all matching rows so the sender can compute the > aggregate? If so, how would we handle that? How does parallelism > handle that now? I think the only sane way to handle this is to disable partial pushdown for that query, fetch all rows and do the aggregate locally. >>> The design for patch 0002 is still under debate. This would expand on >>> the functionality added by adding support for PARTIAL_AGGREGATE for >>> aggregates with an internal stype. This is done by returning a byte >>> array containing the bytes that the serialfunc of the aggregate >>> returns. >>> >>> A competing proposal for 0002 is to instead change aggregates to not >>> use an internal stype anymore, and create dedicated types. The main >>> downside here is that infunc and outfunc would need to be added for >>> text serialization, in addition to the binary serialization. An open >>> question is: Can we change the requirements for CREATE TYPE, so that >>> types can be created without infunc and outfunc. >>> >> >> I think it's +0.5 for the new dedicated data types from me. >> >> I admit I'm too lazy to read the whole thread from scratch, but I >> believe we did discuss the possibility to reuse the serial/deserial >> functions we already have, but the reason against that was the missing >> cross-version stability. Parallel queries always run within a single >> instance, hence there are no concerns about other versions. But this is >> meant to support the remote node having a wildly different version. > > It is more than different versions. Different CPU architectures can > store data types differently in binary. It would be kind of interesting > to have a serial/deserial that was in text format. I guess that is > where my RECORD idea came from that I just emailed to the list. > I believe everything in this thread assumes "binary" in the same sense as the protocol, with the same rules for sending data in text/binary formats, etc. With binary for integers meaning "network order" and that sort of stuff. Which is mostly independent of PG version. When I say version dependency, I mean the structure of the aggregate state itself. That is, in one version the state may store (A,B), while in the other version it may be (B,C). For example, we could store different values in the array, or something like that. > What I would really like is something similar to pg_proc.proargtypes > (which is data type oidvector) for pg_aggregate where we can supply the > oids of the pg_aggregate.aggtranstype and construct a record on the fly > to return to the FDW caller, rather than having to create specialized > functions for every aggregate that needs to return several different > data types, e.g., AVG(interval), #4 in my previous email. I realize > this would require the creation of data types int128 and int128 array. > Isn't that really just a definition of a composite type? I still don't understand which part of the code constructs the record. If the aggregate has aggstate internal, I don't see how could it be anything else than the aggregate itself. >> I guess we might introduce another pair of serial/deserial functions, >> with this guarantee. I know we (me and Jelte) discussed that in person >> at some point, and there were arguments for doing the data types. But I >> managed to forget the details :-( >> >>> WHAT IS NEEDED? >>> >>> The things needed for this patch are that docs need to be added, and >>> detailed codereview needs to be done. >> >> Yeah, I think the docs are must-have for a proper review. > > I think the docs are on hold until we decide on a transfer method. > -- Tomas Vondra ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Partial aggregates pushdown 2024-07-08 12:30 Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-08 11:48 ` Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-21 15:41 ` Re: Partial aggregates pushdown Tomas Vondra <[email protected]> 2024-08-22 18:56 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> 2024-08-22 19:54 ` Re: Partial aggregates pushdown Tomas Vondra <[email protected]> @ 2024-08-22 20:07 ` Bruce Momjian <[email protected]> 2024-08-23 00:07 ` Re: Partial aggregates pushdown Tomas Vondra <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Bruce Momjian @ 2024-08-22 20:07 UTC (permalink / raw) To: Tomas Vondra <[email protected]>; +Cc: [email protected] On Thu, Aug 22, 2024 at 09:54:02PM +0200, Tomas Vondra wrote: > On 8/22/24 20:56, Bruce Momjian wrote: > > You make a very good point above. Would there ever be cases where a > > targetlist would have multiple aggregates, and some can be pushed down, > > and some have to return all matching rows so the sender can compute the > > aggregate? If so, how would we handle that? How does parallelism > > handle that now? > > I think the only sane way to handle this is to disable partial pushdown > for that query, fetch all rows and do the aggregate locally. Okay. > > It is more than different versions. Different CPU architectures can > > store data types differently in binary. It would be kind of interesting > > to have a serial/deserial that was in text format. I guess that is > > where my RECORD idea came from that I just emailed to the list. > > > > I believe everything in this thread assumes "binary" in the same sense > as the protocol, with the same rules for sending data in text/binary > formats, etc. With binary for integers meaning "network order" and that > sort of stuff. Which is mostly independent of PG version. If the binary is tranferable between servers of the same PG version but perhaps different CPU architectures, you are saying we only are worrying about not using the exact same aggregate passing method we use for parallelism, except for PG version differences. Seems we should just assume the same version for this optimization and call it done. Except we don't always check for the same PG version and that could lead to crashes or security problems? > > What I would really like is something similar to pg_proc.proargtypes > > (which is data type oidvector) for pg_aggregate where we can supply the > > oids of the pg_aggregate.aggtranstype and construct a record on the fly > > to return to the FDW caller, rather than having to create specialized > > functions for every aggregate that needs to return several different > > data types, e.g., AVG(interval), #4 in my previous email. I realize > > this would require the creation of data types int128 and int128 array. > > > > Isn't that really just a definition of a composite type? Probably. I am trying to leverage what we already have. > I still don't understand which part of the code constructs the record. > If the aggregate has aggstate internal, I don't see how could it be > anything else than the aggregate itself. Yes, but I was unclear if we needed a special function for every construction. -- 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] 13+ messages in thread
* Re: Partial aggregates pushdown 2024-07-08 12:30 Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-08 11:48 ` Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-21 15:41 ` Re: Partial aggregates pushdown Tomas Vondra <[email protected]> 2024-08-22 18:56 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> 2024-08-22 19:54 ` Re: Partial aggregates pushdown Tomas Vondra <[email protected]> 2024-08-22 20:07 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> @ 2024-08-23 00:07 ` Tomas Vondra <[email protected]> 0 siblings, 0 replies; 13+ messages in thread From: Tomas Vondra @ 2024-08-23 00:07 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; +Cc: [email protected] On 8/22/24 22:07, Bruce Momjian wrote: > On Thu, Aug 22, 2024 at 09:54:02PM +0200, Tomas Vondra wrote: >> On 8/22/24 20:56, Bruce Momjian wrote: >>> You make a very good point above. Would there ever be cases where a >>> targetlist would have multiple aggregates, and some can be pushed down, >>> and some have to return all matching rows so the sender can compute the >>> aggregate? If so, how would we handle that? How does parallelism >>> handle that now? >> >> I think the only sane way to handle this is to disable partial pushdown >> for that query, fetch all rows and do the aggregate locally. > > Okay. > >>> It is more than different versions. Different CPU architectures can >>> store data types differently in binary. It would be kind of interesting >>> to have a serial/deserial that was in text format. I guess that is >>> where my RECORD idea came from that I just emailed to the list. >>> >> >> I believe everything in this thread assumes "binary" in the same sense >> as the protocol, with the same rules for sending data in text/binary >> formats, etc. With binary for integers meaning "network order" and that >> sort of stuff. Which is mostly independent of PG version. > > If the binary is tranferable between servers of the same PG version but > perhaps different CPU architectures, you are saying we only are worrying > about not using the exact same aggregate passing method we use for > parallelism, except for PG version differences. Seems we should just > assume the same version for this optimization and call it done. IMHO restricting this to the exact same version (it'd really have to be the same minor version, not just major) would be pretty annoying and I'm afraid it would seriously restrict how often we'd be able to enable this optimization. Consider a big sharded cluster using postgres_fdw to run queries. AFAIK it's not uncommon to do minor version upgrades node by node, possibly even with failover to make it as smooth as possible. For the duration of this upgrade the pushdown would be impossible, because at least one node has a different minor version. That doesn't seem great. > Except > we don't always check for the same PG version and that could lead to > crashes or security problems? > Yes, we'd need to check this comprehensively. I'm not sure if this might cause crashes - presumably the input functions should be careful about this (because otherwise it'd be a problem even without this feature). Also not sure about security issues. >>> What I would really like is something similar to pg_proc.proargtypes >>> (which is data type oidvector) for pg_aggregate where we can supply the >>> oids of the pg_aggregate.aggtranstype and construct a record on the fly >>> to return to the FDW caller, rather than having to create specialized >>> functions for every aggregate that needs to return several different >>> data types, e.g., AVG(interval), #4 in my previous email. I realize >>> this would require the creation of data types int128 and int128 array. >>> >> >> Isn't that really just a definition of a composite type? > > Probably. I am trying to leverage what we already have. > >> I still don't understand which part of the code constructs the record. >> If the aggregate has aggstate internal, I don't see how could it be >> anything else than the aggregate itself. > > Yes, but I was unclear if we needed a special function for every > construction. > OK -- Tomas Vondra ^ permalink raw reply [nested|flat] 13+ messages in thread
end of thread, other threads:[~2024-08-23 00:07 UTC | newest] Thread overview: 13+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-08-21 00:40 [PATCH 03/10] Fix comment wrong since bf2a691e02d7766f185d9d8e0f092222a5c0a129 Justin Pryzby <[email protected]> 2024-07-08 12:30 Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-08 11:48 ` Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-15 21:12 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> 2024-08-20 08:07 ` Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-20 16:50 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> 2024-08-20 17:03 ` Re: Partial aggregates pushdown Jelte Fennema-Nio <[email protected]> 2024-08-21 15:41 ` Re: Partial aggregates pushdown Tomas Vondra <[email protected]> 2024-08-22 11:05 ` Re: Partial aggregates pushdown Ashutosh Bapat <[email protected]> 2024-08-22 18:56 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> 2024-08-22 19:54 ` Re: Partial aggregates pushdown Tomas Vondra <[email protected]> 2024-08-22 20:07 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> 2024-08-23 00:07 ` Re: Partial aggregates pushdown Tomas Vondra <[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