public inbox for [email protected]help / color / mirror / Atom feed
Re: Partial aggregates pushdown 5+ messages / 4 participants [nested] [flat]
* Re: Partial aggregates pushdown @ 2024-03-19 21:29 Tom Lane <[email protected]> 2024-03-19 23:09 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Tom Lane @ 2024-03-19 21:29 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; +Cc: [email protected] <[email protected]>; Alexander Pyhalov <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers; Stephen Frost <[email protected]>; Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]>; vignesh C <[email protected]> Bruce Momjian <[email protected]> writes: > The current patch has: > if ((OidIsValid(aggform->aggfinalfn) || > (aggform->aggtranstype == INTERNALOID)) && > fpinfo->check_partial_aggregate_support) > { > if (fpinfo->remoteversion == 0) > { > PGconn *conn = GetConnection(fpinfo->user, false, NULL); > fpinfo->remoteversion = PQserverVersion(conn); > } > if (fpinfo->remoteversion < PG_VERSION_NUM) > partial_agg_ok = false; > } > It uses check_partial_aggregate_support, which defaults to false, > meaning partial aggregates will be pushed down with no version check by > default. If set to true, pushdown will happen if the remote server is > the same version or newer, which seems acceptable to me. I'd like to vociferously protest both of those decisions. "No version check by default" means "unsafe by default", which is not project style in general and is especially not so for postgres_fdw. We have tried very hard for years to ensure that postgres_fdw will work with a wide range of remote server versions, and generally been extremely conservative about what we think will work (example: collations); but this patch seems ready to throw that principle away. Also, surely "remoteversion < PG_VERSION_NUM" is backwards. What this would mean is that nobody can ever change a partial aggregate's implementation, because that would break queries issued from older servers (that couldn't know about the change) to newer ones. Realistically, I think it's fairly unsafe to try aggregate pushdown to anything but the same PG major version; otherwise, you're buying into knowing which aggregates have partial support in which versions, as well as predicting the future about incompatible state changes. Even that isn't bulletproof --- e.g, maybe somebody wasn't careful about endianness-independence of the serialized partial state, making it unsafe to ship --- so there had better be a switch whereby the user can disable it. Maybe we could define a three-way setting: * default: push down partial aggs only to same major PG version * disable: don't push down, period * force: push down regardless of remote version With the "force" setting, it's the user's responsibility not to issue any remote-able aggregation that would be unsafe to push down. This is still a pretty crude tool: I can foresee people wanting to have per-aggregate control over things, especially extension-supplied aggregates. But it'd do for starters. I'm not super thrilled by the fact that the patch contains zero user-facing documentation, even though it's created new SQL syntax, not to mention a new postgres_fdw option. I assume this means that nobody thinks it's anywhere near ready to commit. regards, tom lane ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Partial aggregates pushdown 2024-03-19 21:29 Re: Partial aggregates pushdown Tom Lane <[email protected]> @ 2024-03-19 23:09 ` Bruce Momjian <[email protected]> 2024-03-21 11:37 ` RE: Partial aggregates pushdown [email protected] <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Bruce Momjian @ 2024-03-19 23:09 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: [email protected] <[email protected]>; Alexander Pyhalov <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers; Stephen Frost <[email protected]>; Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]>; vignesh C <[email protected]> On Tue, Mar 19, 2024 at 05:29:07PM -0400, Tom Lane wrote: > I'd like to vociferously protest both of those decisions. > > "No version check by default" means "unsafe by default", which is not > project style in general and is especially not so for postgres_fdw. > We have tried very hard for years to ensure that postgres_fdw will > work with a wide range of remote server versions, and generally been > extremely conservative about what we think will work (example: > collations); but this patch seems ready to throw that principle away. > > Also, surely "remoteversion < PG_VERSION_NUM" is backwards. What > this would mean is that nobody can ever change a partial aggregate's > implementation, because that would break queries issued from older > servers (that couldn't know about the change) to newer ones. Well it is the origin server that is issuing the PUSHDOWN syntax, so an older origin server should be able to push to a newer remote server. > Realistically, I think it's fairly unsafe to try aggregate pushdown > to anything but the same PG major version; otherwise, you're buying > into knowing which aggregates have partial support in which versions, > as well as predicting the future about incompatible state changes. Yes, incompatible state changes would be a problem with an older origin server with a newer remote server setup. If we require matching versions, we must accept that upgrades will require more downtime. > Even that isn't bulletproof --- e.g, maybe somebody wasn't careful > about endianness-independence of the serialized partial state, making > it unsafe to ship --- so there had better be a switch whereby the user > can disable it. Makes sense. I was also wondering how a user would know whether the pushdown is happening, or not. > Maybe we could define a three-way setting: > > * default: push down partial aggs only to same major PG version > * disable: don't push down, period > * force: push down regardless of remote version What would be the default? If it is the first one, it requires a remote version check on first in the session. > With the "force" setting, it's the user's responsibility not to > issue any remote-able aggregation that would be unsafe to push > down. This is still a pretty crude tool: I can foresee people > wanting to have per-aggregate control over things, especially > extension-supplied aggregates. But it'd do for starters. We have the postgres_fdw extensions option to control function pushdown to extensions. > I'm not super thrilled by the fact that the patch contains zero > user-facing documentation, even though it's created new SQL syntax, > not to mention a new postgres_fdw option. I assume this means that > nobody thinks it's anywhere near ready to commit. Previous versions of the patch had docs since I know I worked on improving them. I am not sure what happened to them. -- Bruce Momjian <[email protected]> https://momjian.us EDB https://enterprisedb.com Only you can decide what is important to you. ^ permalink raw reply [nested|flat] 5+ messages in thread
* RE: Partial aggregates pushdown 2024-03-19 21:29 Re: Partial aggregates pushdown Tom Lane <[email protected]> 2024-03-19 23:09 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> @ 2024-03-21 11:37 ` [email protected] <[email protected]> 2024-03-21 22:01 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: [email protected] @ 2024-03-21 11:37 UTC (permalink / raw) To: Bruce Momjian <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers; +Cc: Alexander Pyhalov <[email protected]>; Stephen Frost <[email protected]>; Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]>; vignesh C <[email protected]>; [email protected] <[email protected]> Hi. Mr.Momjian, Mr.Lane, Mr.Haas, hackers. I apologize for any misunderstanding regarding the context of the attached patch and the points on which I requested a review. Could you please allow me to clarify? In the review around early December 2023, I received the following three issues pointed out by Mr.Haas[1]. 1. Transmitting state value safely between machines 2. Making the patch clearer by adding SQL keywords 3. Fixing the behavior when the HAVING clause is present In the email sent on February 22, 2024[2], I provided an update on the progress made in addressing these issues. Regarding issue 1, I have only provided a proposed solution in the email and have not started the programming. Therefore, the latest patch is not in a commit-ready state. As mentioned later, we have also temporarily reverted the changes made to the documentation. Before proceeding with the programming, I would like to discuss the proposed solution with the community and seek consensus. If it is necessary to have source code in order to discuss, I can create a simple prototype so that I can receive your feedback. Would you be able to provide your opinions on it? Regarding issue 2., I have confirmed that creating a prototype allows us to address the issue and clear the patch. In this prototype creation, the main purpose was to verify if the patch can be cleared and significant revisions were made to the previous version. Therefore, I have removed all the document differences. I have submitted a patch [3] that includes the fixes for issue 3. to the patch that was posted in [2]. Regarding the proposed solution for issue 1, unlike the patch posted in [3], we have a policy of not performing partial aggregation pushdown if we cannot guarantee compatibility and safety. The latest patch in [3] is a POC patch. The patch that Mr. Momjian reviewed is this. If user-facing documentation is needed for this POC patch, it can be added. I apologize for the lack of explanation regarding this positioning, which may have caused misunderstandings regarding the patch posted in [3]. [1] https://www.postgresql.org/message-id/CA%2BTgmoYCrtOvk2f32qQKZV%3DjNL35tandf2A2Dp_2F5ASuiG1BA%40mail... [2] https://www.postgresql.org/message-id/TYAPR01MB5514F0CBD9CD4F84A261198195562%40TYAPR01MB5514.jpnprd0... [3] https://www.postgresql.org/message-id/TYAPR01MB55141D18188AC86ADCE35FCB952F2%40TYAPR01MB5514.jpnprd0... Sincerely yours, Yuuki Fujii -- Yuuki Fujii Information Technology R&D Center Mitsubishi Electric Corporation ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Partial aggregates pushdown 2024-03-19 21:29 Re: Partial aggregates pushdown Tom Lane <[email protected]> 2024-03-19 23:09 ` Re: Partial aggregates pushdown Bruce Momjian <[email protected]> 2024-03-21 11:37 ` RE: Partial aggregates pushdown [email protected] <[email protected]> @ 2024-03-21 22:01 ` Bruce Momjian <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Bruce Momjian @ 2024-03-21 22:01 UTC (permalink / raw) To: [email protected] <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers; Alexander Pyhalov <[email protected]>; Stephen Frost <[email protected]>; Finnerty, Jim <[email protected]>; Andres Freund <[email protected]>; Tomas Vondra <[email protected]>; Julien Rouhaud <[email protected]>; Daniel Gustafsson <[email protected]>; vignesh C <[email protected]> On Thu, Mar 21, 2024 at 11:37:50AM +0000, [email protected] wrote: > Hi. Mr.Momjian, Mr.Lane, Mr.Haas, hackers. > > I apologize for any misunderstanding regarding the context of the attached patch and > the points on which I requested a review. Could you please allow me to clarify? > > In the review around early December 2023, I received the following three issues pointed out by Mr.Haas[1]. > 1. Transmitting state value safely between machines > 2. Making the patch clearer by adding SQL keywords > 3. Fixing the behavior when the HAVING clause is present > > In the email sent on February 22, 2024[2], I provided an update on the progress made in addressing these issues. > Regarding issue 1, I have only provided a proposed solution in the email and have not started the programming. > Therefore, the latest patch is not in a commit-ready state. As mentioned later, we have also temporarily reverted the changes made to the documentation. > Before proceeding with the programming, I would like to discuss the proposed solution with the community and seek consensus. > If it is necessary to have source code in order to discuss, I can create a simple prototype so that I can receive your feedback. > Would you be able to provide your opinions on it? > > Regarding issue 2., I have confirmed that creating a prototype allows us to address the issue and clear the patch. > In this prototype creation, the main purpose was to verify if the patch can be cleared and significant revisions were made to the previous version. > Therefore, I have removed all the document differences. > I have submitted a patch [3] that includes the fixes for issue 3. to the patch that was posted in [2]. > Regarding the proposed solution for issue 1, unlike the patch posted in [3], > we have a policy of not performing partial aggregation pushdown if we cannot guarantee compatibility and safety. > The latest patch in [3] is a POC patch. The patch that Mr. Momjian reviewed is this. > If user-facing documentation is needed for this POC patch, it can be added. > > I apologize for the lack of explanation regarding this positioning, which may have caused misunderstandings regarding the patch posted in [3]. That makes sense. Let's get you answers to those questions first before you continue. -- 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] 5+ messages in thread
* [PATCH v15 6/8] Row pattern recognition patch (docs). @ 2024-03-28 10:30 Tatsuo Ishii <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Tatsuo Ishii @ 2024-03-28 10:30 UTC (permalink / raw) --- doc/src/sgml/advanced.sgml | 80 ++++++++++++++++++++++++++++++++++++ doc/src/sgml/func.sgml | 54 ++++++++++++++++++++++++ doc/src/sgml/ref/select.sgml | 38 ++++++++++++++++- 3 files changed, 170 insertions(+), 2 deletions(-) diff --git a/doc/src/sgml/advanced.sgml b/doc/src/sgml/advanced.sgml index 755c9f1485..cf18dd887e 100644 --- a/doc/src/sgml/advanced.sgml +++ b/doc/src/sgml/advanced.sgml @@ -537,6 +537,86 @@ WHERE pos < 3; <literal>rank</literal> less than 3. </para> + <para> + Row pattern common syntax can be used to perform row pattern recognition + in a query. Row pattern common syntax includes two sub + clauses: <literal>DEFINE</literal> + and <literal>PATTERN</literal>. <literal>DEFINE</literal> defines + definition variables along with an expression. The expression must be a + logical expression, which means it must + return <literal>TRUE</literal>, <literal>FALSE</literal> + or <literal>NULL</literal>. The expression may comprise column references + and functions. Window functions, aggregate functions and subqueries are + not allowed. An example of <literal>DEFINE</literal> is as follows. + +<programlisting> +DEFINE + LOWPRICE AS price <= 100, + UP AS price > PREV(price), + DOWN AS price < PREV(price) +</programlisting> + + Note that <function>PREV</function> returns the price column in the + previous row if it's called in a context of row pattern recognition. So in + the second line the definition variable "UP" is <literal>TRUE</literal> + when the price column in the current row is greater than the price column + in the previous row. Likewise, "DOWN" is <literal>TRUE</literal> when when + the price column in the current row is lower than the price column in the + previous row. + </para> + <para> + Once <literal>DEFINE</literal> exists, <literal>PATTERN</literal> can be + used. <literal>PATTERN</literal> defines a sequence of rows that satisfies + certain conditions. For example following <literal>PATTERN</literal> + defines that a row starts with the condition "LOWPRICE", then one or more + rows satisfy "UP" and finally one or more rows satisfy "DOWN". Note that + "+" means one or more matches. Also you can use "*", which means zero or + more matches. If a sequence of rows which satisfies the PATTERN is found, + in the starting row of the sequence of rows all window functions and + aggregates are shown in the target list. Note that aggregations only look + into the matched rows, rather than whole frame. In the second or + subsequent rows all window functions and aggregates are NULL. For rows + that do not match the PATTERN, all window functions and aggregates are + shown AS NULL too, except count which shows 0. This is because the + unmatched rows are in an empty frame. Example of + a <literal>SELECT</literal> using the <literal>DEFINE</literal> + and <literal>PATTERN</literal> clause is as follows. + +<programlisting> +SELECT company, tdate, price, + first_value(price) OVER w, + max(price) OVER w, + count(price) OVER w +FROM stock, + WINDOW w AS ( + PARTITION BY company + ORDER BY tdate + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING + AFTER MATCH SKIP PAST LAST ROW + INITIAL + PATTERN (LOWPRICE UP+ DOWN+) + DEFINE + LOWPRICE AS price <= 100, + UP AS price > PREV(price), + DOWN AS price < PREV(price) +); +</programlisting> +<screen> + company | tdate | price | first_value | max | count +----------+------------+-------+-------------+-----+------- + company1 | 2023-07-01 | 100 | 100 | 200 | 4 + company1 | 2023-07-02 | 200 | | | + company1 | 2023-07-03 | 150 | | | + company1 | 2023-07-04 | 140 | | | + company1 | 2023-07-05 | 150 | | | 0 + company1 | 2023-07-06 | 90 | 90 | 130 | 4 + company1 | 2023-07-07 | 110 | | | + company1 | 2023-07-08 | 130 | | | + company1 | 2023-07-09 | 120 | | | + company1 | 2023-07-10 | 130 | | | 0 +</screen> + </para> + <para> When a query involves multiple window functions, it is possible to write out each one with a separate <literal>OVER</literal> clause, but this is diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 93b0bc2bc6..d25eeb3327 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -22637,6 +22637,7 @@ SELECT count(*) FROM sometable; returns <literal>NULL</literal> if there is no such row. </para></entry> </row> + </tbody> </tgroup> </table> @@ -22676,6 +22677,59 @@ SELECT count(*) FROM sometable; Other frame specifications can be used to obtain other effects. </para> + <para> + Row pattern recognition navigation functions are listed in + <xref linkend="functions-rpr-navigation-table"/>. These functions + can be used to describe DEFINE clause of Row pattern recognition. + </para> + + <table id="functions-rpr-navigation-table"> + <title>Row Pattern Navigation Functions</title> + <tgroup cols="1"> + <thead> + <row> + <entry role="func_table_entry"><para role="func_signature"> + Function + </para> + <para> + Description + </para></entry> + </row> + </thead> + + <tbody> + <row> + <entry role="func_table_entry"><para role="func_signature"> + <indexterm> + <primary>prev</primary> + </indexterm> + <function>prev</function> ( <parameter>value</parameter> <type>anyelement</type> ) + <returnvalue>anyelement</returnvalue> + </para> + <para> + Returns the column value at the previous row; + returns NULL if there is no previous row in the window frame. + </para></entry> + </row> + + <row> + <entry role="func_table_entry"><para role="func_signature"> + <indexterm> + <primary>next</primary> + </indexterm> + <function>next</function> ( <parameter>value</parameter> <type>anyelement</type> ) + <returnvalue>anyelement</returnvalue> + </para> + <para> + Returns the column value at the next row; + returns NULL if there is no next row in the window frame. + </para></entry> + </row> + + </tbody> + </tgroup> + </table> + <note> <para> The SQL standard defines a <literal>RESPECT NULLS</literal> or diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml index 066aed44e6..8f18718d58 100644 --- a/doc/src/sgml/ref/select.sgml +++ b/doc/src/sgml/ref/select.sgml @@ -969,8 +969,8 @@ WINDOW <replaceable class="parameter">window_name</replaceable> AS ( <replaceabl The <replaceable class="parameter">frame_clause</replaceable> can be one of <synopsis> -{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ] -{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ] +{ RANGE | ROWS | GROUPS } <replaceable>frame_start</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax] +{ RANGE | ROWS | GROUPS } BETWEEN <replaceable>frame_start</replaceable> AND <replaceable>frame_end</replaceable> [ <replaceable>frame_exclusion</replaceable> ] [row_pattern_common_syntax] </synopsis> where <replaceable>frame_start</replaceable> @@ -1077,6 +1077,40 @@ EXCLUDE NO OTHERS a given peer group will be in the frame or excluded from it. </para> + <para> + The + optional <replaceable class="parameter">row_pattern_common_syntax</replaceable> + defines the <firstterm>row pattern recognition condition</firstterm> for + this + window. <replaceable class="parameter">row_pattern_common_syntax</replaceable> + includes following subclauses. <literal>AFTER MATCH SKIP PAST LAST + ROW</literal> or <literal>AFTER MATCH SKIP TO NEXT ROW</literal> controls + how to proceed to next row position after a match + found. With <literal>AFTER MATCH SKIP PAST LAST ROW</literal> (the + default) next row position is next to the last row of previous match. On + the other hand, with <literal>AFTER MATCH SKIP TO NEXT ROW</literal> next + row position is always next to the last row of previous + match. <literal>DEFINE</literal> defines definition variables along with a + boolean expression. <literal>PATTERN</literal> defines a sequence of rows + that satisfies certain conditions using variables defined + in <literal>DEFINE</literal> clause. If the variable is not defined in + the <literal>DEFINE</literal> clause, it is implicitly assumed + following is defined in the <literal>DEFINE</literal> clause. + +<synopsis> +<literal>variable_name</literal> AS TRUE +</synopsis> + + Note that the maximu number of variables defined + in <literal>DEFINE</literal> clause is 26. + +<synopsis> +[ AFTER MATCH SKIP PAST LAST ROW | AFTER MATCH SKIP TO NEXT ROW ] +PATTERN <replaceable class="parameter">pattern_variable_name</replaceable>[+] [, ...] +DEFINE <replaceable class="parameter">definition_varible_name</replaceable> AS <replaceable class="parameter">expression</replaceable> [, ...] +</synopsis> + </para> + <para> The purpose of a <literal>WINDOW</literal> clause is to specify the behavior of <firstterm>window functions</firstterm> appearing in the query's -- 2.25.1 ----Next_Part(Thu_Mar_28_19_59_25_2024_076)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v15-0007-Row-pattern-recognition-patch-tests.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2024-03-28 10:30 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-03-19 21:29 Re: Partial aggregates pushdown Tom Lane <[email protected]> 2024-03-19 23:09 ` Bruce Momjian <[email protected]> 2024-03-21 11:37 ` [email protected] <[email protected]> 2024-03-21 22:01 ` Bruce Momjian <[email protected]> 2024-03-28 10:30 [PATCH v15 6/8] Row pattern recognition patch (docs). Tatsuo Ishii <[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