Re: CFbot does not recognize patch contents
Hi Thomas, > This obviously fixed itself automatically soon after this message, but > I figured out what happened: I had not actually fixed that referenced > bug in cfbot :-(. It was checking for HTTP error codes correctly in > the place that reads emails from the archives, but not the place that > downloads patches, so in this case I think when it tried to follow the > link[1] to download the patch, I guess it must have pulled down a > transient Varnish error message (I don't know what, I don't store it > anywhere), and tried to apply that as a patch. Oops. Fixed[2]. > > [1] > https://www.postgresql.org/message-id/attachment/160138/v18-0001-Row-pattern-recognition-patch-for-raw-parser.patch > [2] > https://github.com/macdice/cfbot/commit/ec33a65a877a88befc29ea220e87b98c89b27dcc Thank you for looking into this. I understand the situation. BTW I have just posted a v19 patch [1] and cfbot took care of it nicely. [1] https://www.postgresql.org/message-id/20240515.090203.2255390780622503596.t-ishii%40sranhm.sra.co.jp -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: CFbot does not recognize patch contents
> I am able to apply all your patches. I found that a similar thing > happened before [0] and I guess your case is similar. Adding Thomas to > CC, he may be able to help more. Ok. Thanks for the info. > Nitpick: There is a trailing space warning while applying one of your patches: > Applying: Row pattern recognition patch (docs). > .git/rebase-apply/patch:81: trailing whitespace. > company | tdate| price | first_value | max | count Yes, I know. The reason why there's a trailing whitespace is, I copied the psql output and pasted it into the docs. I wonder why psql adds the whitespace. Unless there's a good reason to do that, I think it's better to fix psql so that it does not emit trailing spaces in its output. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
CFbot does not recognize patch contents
After sending out my v18 patches: https://www.postgresql.org/message-id/20240511.162307.2246647987352188848.t-ishii%40sranhm.sra.co.jp CFbot complains that the patch was broken: http://cfbot.cputube.org/patch_48_4460.log === Applying patches on top of PostgreSQL commit ID 31e8f4e619d9b5856fa2bd5713cb1e2e170a9c7d === === applying patch ./v18-0001-Row-pattern-recognition-patch-for-raw-parser.patch gpatch: Only garbage was found in the patch input. The patch was generated by git-format-patch (same as previous patches). I failed to find any patch format problem in the patch. Does anybody know what's wrong here? Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: New committers: Melanie Plageman, Richard Guo,New committers: Melanie Plageman, Richard Guo
> The Core Team would like to extend our congratulations to Melanie > Plageman and Richard Guo, who have accepted invitations to become our > newest PostgreSQL committers. > > Please join us in wishing them much success and few reverts! Congratulations! -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
> On Tue, Apr 23, 2024 at 8:13 PM Tatsuo Ishii wrote:
>> SELECT v.a, count(*) OVER w
>> FROM (VALUES ('A'),('B'),('B'),('C')) AS v (a)
>> WINDOW w AS (
>> ORDER BY v.a
>> ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
>> PATTERN (B+)
>> DEFINE B AS a = 'B'
>> )
>> a | count
>> ---+---
>> A | 0
>> B | 2
>> B |
>> C | 0
>> (4 rows)
>>
>> Here row 3 is skipped because the pattern B matches row 2 and 3. In
>> this case I think cont(*) should return 0 rathern than NULL for row 3.
>
> I think returning zero would match Vik's explanation upthread [1],
> yes. Unfortunately I don't have a spec handy to double-check for
> myself right now.
Ok. I believe you and Vik are correct.
I am modifying the patch in this direction.
Attached is the regression diff after modifying the patch.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
diff -U3
/usr/local/src/pgsql/current/postgresql/src/test/regress/expected/rpr.out
/usr/local/src/pgsql/current/postgresql/src/test/regress/results/rpr.out
--- /usr/local/src/pgsql/current/postgresql/src/test/regress/expected/rpr.out
2024-04-24 11:30:27.710523139 +0900
+++ /usr/local/src/pgsql/current/postgresql/src/test/regress/results/rpr.out
2024-04-26 14:39:03.543759205 +0900
@@ -181,8 +181,8 @@
company1 | 07-01-2023 | 100 | 0
company1 | 07-02-2023 | 200 | 0
company1 | 07-03-2023 | 150 | 3
- company1 | 07-04-2023 | 140 |
- company1 | 07-05-2023 | 150 |
+ company1 | 07-04-2023 | 140 | 0
+ company1 | 07-05-2023 | 150 | 0
company1 | 07-06-2023 |90 | 0
company1 | 07-07-2023 | 110 | 0
company1 | 07-08-2023 | 130 | 0
@@ -556,24 +556,24 @@
company | tdate| price | first_value | last_value | count
--++---+-++---
company1 | 07-01-2023 | 100 | 07-01-2023 | 07-03-2023 | 3
- company1 | 07-02-2023 | 200 | ||
- company1 | 07-03-2023 | 150 | ||
+ company1 | 07-02-2023 | 200 | || 0
+ company1 | 07-03-2023 | 150 | || 0
company1 | 07-04-2023 | 140 | 07-04-2023 | 07-06-2023 | 3
- company1 | 07-05-2023 | 150 | ||
- company1 | 07-06-2023 |90 | ||
+ company1 | 07-05-2023 | 150 | || 0
+ company1 | 07-06-2023 |90 | || 0
company1 | 07-07-2023 | 110 | 07-07-2023 | 07-09-2023 | 3
- company1 | 07-08-2023 | 130 | ||
- company1 | 07-09-2023 | 120 | ||
+ company1 | 07-08-2023 | 130 | || 0
+ company1 | 07-09-2023 | 120 | || 0
company1 | 07-10-2023 | 130 | || 0
company2 | 07-01-2023 |50 | 07-01-2023 | 07-03-2023 | 3
- company2 | 07-02-2023 | 2000 | ||
- company2 | 07-03-2023 | 1500 | ||
+ company2 | 07-02-2023 | 2000 | || 0
+ company2 | 07-03-2023 | 1500 | || 0
company2 | 07-04-2023 | 1400 | 07-04-2023 | 07-06-2023 | 3
- company2 | 07-05-2023 | 1500 | ||
- company2 | 07-06-2023 |60 | ||
+ company2 | 07-05-2023 | 1500 | || 0
+ company2 | 07-06-2023 |60 | || 0
company2 | 07-07-2023 | 1100 | 07-07-2023 | 07-09-2023 | 3
- company2 | 07-08-2023 | 1300 | ||
- company2 | 07-09-2023 | 1200 | ||
+ company2 | 07-08-2023 | 1300 | || 0
+ company2 | 07-09-2023 | 1200 | || 0
company2 | 07-10-2023 | 1300 | || 0
(20 rows)
@@ -604,24 +604,24 @@
company | tdate| price | first_value | last_value | max | min | sum
| avg | count
--++---+-++--+-+--+---+---
company1 | 07-01-2023 | 100 | 100 |140 | 200 | 100 | 590
| 147.5000 | 4
- company1 | 07-02-2023 | 200 | || | |
| |
- company1 | 07-03-2023 | 150 | || | |
| |
- company1 | 07-04-2023 | 140 | || | |
| |
+ company1 | 07-02-2023 | 200 | || | |
| | 0
+ co
Re: Row pattern recognition
Hi Vik and Champion,
I think the current RPR patch is not quite correct in handling
count(*).
(using slightly modified version of Vik's example query)
SELECT v.a, count(*) OVER w
FROM (VALUES ('A'),('B'),('B'),('C')) AS v (a)
WINDOW w AS (
ORDER BY v.a
ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
PATTERN (B+)
DEFINE B AS a = 'B'
)
a | count
---+---
A | 0
B | 2
B |
C | 0
(4 rows)
Here row 3 is skipped because the pattern B matches row 2 and 3. In
this case I think cont(*) should return 0 rathern than NULL for row 3.
What do you think?
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Re: Minor document typo
>> I think "too low a" should be "too low" ('a' is not
>> necessary). Attached is the patch.
>
> The existing text looks fine to me. The other form would use "of a"
> and become "too low of a wal_level on the primary".
>
> "too low wal_level on the primary" sounds wrong to my native
> English-speaking ear.
>
> There's some discussion in [1] that might be of interest to you.
>
> David
>
> [1]
> https://www.reddit.com/r/grammar/comments/qr9z6e/i_need_help_with_sothat_adj_of_a_sing_noun/?ref=share_source=link
Thank you for the explanation. English is difficult :-)
Just out of a curiosity, is it possible to say "low a wal_level on the
primary"? (just "too" is removed)
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Minor document typo
Hi,
doc/src/sgml/monitoring.sgml seems to have a minor typo:
In pg_stat_database_conflicts section (around line 3621) we have:
Number of uses of logical slots in this database that have been
canceled due to old snapshots or too low a
on the primary
I think "too low a" should be "too low" ('a' is not
necessary). Attached is the patch.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index 053da8d6e4..56e79b1304 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -3618,7 +3618,7 @@ description | Waiting for a newly initialized WAL file to reach durable storage
Number of uses of logical slots in this database that have been
- canceled due to old snapshots or too low a
+ canceled due to old snapshots or too low
on the primary
Re: When extended query protocol ends?
> And that's also where sending a Query instead of a Sync introduces a > problem: What is supposed to happen if something fails. Let's take the > simple example Bind-Execute-Query: If the Execute causes an error, is > the Query still executed or not? And what about if the Query fails, is > the Execute before committed or rolled back? > > That's why we have the Sync messages, clarify what happens when a > failure occurs somewhere in the pipeline. And only extended protocol > messages are allowed to be part of a pipeline: "Use of the extended > query protocol allows pipelining" Good point. If we have started extended protocol messages, I think there's no way to avoid the Sync messages before issuing simple protocol messages. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: When extended query protocol ends?
Hi Dave, Oh, I see. > Hi Tatsuo, > > Actually no need, I figured it out. > > I don't have a solution yet though. > > Dave Cramer > www.postgres.rocks > > > On Thu, 15 Feb 2024 at 19:43, Tatsuo Ishii wrote: > >> > Can you ask the OP what they are doing in the startup. I'm trying to >> > replicate their situation. >> > Looks like possibly 'setReadOnly' and 'select version()' >> >> Sure I will. By the way 'select version()' may be issued by Pgpool-II >> itself. In this case it should be 'SELECT version()', not 'select >> version()'. >> >> Best reagards, >> -- >> Tatsuo Ishii >> SRA OSS LLC >> English: http://www.sraoss.co.jp/index_en/ >> Japanese:http://www.sraoss.co.jp >>
Re: When extended query protocol ends?
> Can you ask the OP what they are doing in the startup. I'm trying to > replicate their situation. > Looks like possibly 'setReadOnly' and 'select version()' Sure I will. By the way 'select version()' may be issued by Pgpool-II itself. In this case it should be 'SELECT version()', not 'select version()'. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: When extended query protocol ends?
>>> From [1] I think the JDBC driver sends something like below if >>> autosave=always option is specified. >>> >>> "BEGIN READ ONLY" Parse/Bind/Eexecute (in the extended query protocol) >>> "SAVEPOINT PGJDBC_AUTOSAVE" (in the simple query protocol) >>> >>> It seems the SAVEPOINT is sent without finishing the extended query >>> protocol (i.e. without Sync message). Is it possible for the JDBC >>> driver to issue a Sync message before sending SAVEPOINT in simple >>> query protocol? Or you can send SAVEPOINT using the extended query >>> protocol. >>> >>> [1] >>> https://www.pgpool.net/pipermail/pgpool-general/2023-December/009051.html >> >> >> Can you ask the OP what version of the driver they are using. From what I >> can tell we send BEGIN using SimpleQuery. > > Sure. I will get back once I get the JDBC version. Here it is: > JDBC driver version used:42.5.1 Regards, Karel. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: When extended query protocol ends?
Hi Dave, >> From [1] I think the JDBC driver sends something like below if >> autosave=always option is specified. >> >> "BEGIN READ ONLY" Parse/Bind/Eexecute (in the extended query protocol) >> "SAVEPOINT PGJDBC_AUTOSAVE" (in the simple query protocol) >> >> It seems the SAVEPOINT is sent without finishing the extended query >> protocol (i.e. without Sync message). Is it possible for the JDBC >> driver to issue a Sync message before sending SAVEPOINT in simple >> query protocol? Or you can send SAVEPOINT using the extended query >> protocol. >> >> [1] >> https://www.pgpool.net/pipermail/pgpool-general/2023-December/009051.html > > > Can you ask the OP what version of the driver they are using. From what I > can tell we send BEGIN using SimpleQuery. Sure. I will get back once I get the JDBC version. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: When extended query protocol ends?
>> Hello Dave, >> >> > Tatsuo Ishii writes: >> >> Below is outputs from "pgproto" command coming with Pgpool-II. >> >> (Lines starting "FE" represents a message from frontend to backend. >> >> Lines starting "BE" represents a message from backend to frontend.) >> > >> >> FE=> Parse(stmt="", query="SET extra_float_digits = 3") >> >> FE=> Bind(stmt="", portal="") >> >> FE=> Execute(portal="") >> >> FE=> Parse(stmt="", query="SET extra_float_digits = 3") >> >> FE=> Bind(stmt="", portal="") >> >> FE=> Execute(portal="") >> >> FE=> Query (query="SET extra_float_digits = 3") >> >> <= BE ParseComplete >> >> <= BE BindComplete >> >> <= BE CommandComplete(SET) >> >> <= BE ParseComplete >> >> <= BE BindComplete >> >> <= BE CommandComplete(SET) >> >> <= BE CommandComplete(SET) >> >> <= BE ReadyForQuery(I) >> >> FE=> Terminate >> > >> >> As you can see, two "SET extra_float_digits = 3" is sent in the >> >> extended query protocol, then one "SET extra_float_digits = 3" follows >> >> in the simple query protocol. No sync message is sent. However, I get >> >> ReadyForQuery at the end. It seems the extended query protocol is >> >> ended by a simple query protocol message instead of a sync message. >> > >> >> My question is, is this legal in terms of fronted/backend protocol? >> > >> > I think it's poor practice, at best. You should end the >> > extended-protocol query cycle before invoking simple query. >> >> From [1] I think the JDBC driver sends something like below if >> autosave=always option is specified. >> >> "BEGIN READ ONLY" Parse/Bind/Eexecute (in the extended query protocol) >> "SAVEPOINT PGJDBC_AUTOSAVE" (in the simple query protocol) >> >> It seems the SAVEPOINT is sent without finishing the extended query >> protocol (i.e. without Sync message). Is it possible for the JDBC >> driver to issue a Sync message before sending SAVEPOINT in simple >> query protocol? Or you can send SAVEPOINT using the extended query >> protocol. >> >> [1] >> https://www.pgpool.net/pipermail/pgpool-general/2023-December/009051.html >> >> > Hi Tatsuo, > > Yes, it would be possible. > > Can you create an issue on github? Issues · pgjdbc/pgjdbc (github.com) > <https://github.com/pgjdbc/pgjdbc/issues> Sure. https://github.com/pgjdbc/pgjdbc/issues/3107 Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: When extended query protocol ends?
Hello Dave, > Tatsuo Ishii writes: >> Below is outputs from "pgproto" command coming with Pgpool-II. >> (Lines starting "FE" represents a message from frontend to backend. >> Lines starting "BE" represents a message from backend to frontend.) > >> FE=> Parse(stmt="", query="SET extra_float_digits = 3") >> FE=> Bind(stmt="", portal="") >> FE=> Execute(portal="") >> FE=> Parse(stmt="", query="SET extra_float_digits = 3") >> FE=> Bind(stmt="", portal="") >> FE=> Execute(portal="") >> FE=> Query (query="SET extra_float_digits = 3") >> <= BE ParseComplete >> <= BE BindComplete >> <= BE CommandComplete(SET) >> <= BE ParseComplete >> <= BE BindComplete >> <= BE CommandComplete(SET) >> <= BE CommandComplete(SET) >> <= BE ReadyForQuery(I) >> FE=> Terminate > >> As you can see, two "SET extra_float_digits = 3" is sent in the >> extended query protocol, then one "SET extra_float_digits = 3" follows >> in the simple query protocol. No sync message is sent. However, I get >> ReadyForQuery at the end. It seems the extended query protocol is >> ended by a simple query protocol message instead of a sync message. > >> My question is, is this legal in terms of fronted/backend protocol? > > I think it's poor practice, at best. You should end the > extended-protocol query cycle before invoking simple query. >From [1] I think the JDBC driver sends something like below if autosave=always option is specified. "BEGIN READ ONLY" Parse/Bind/Eexecute (in the extended query protocol) "SAVEPOINT PGJDBC_AUTOSAVE" (in the simple query protocol) It seems the SAVEPOINT is sent without finishing the extended query protocol (i.e. without Sync message). Is it possible for the JDBC driver to issue a Sync message before sending SAVEPOINT in simple query protocol? Or you can send SAVEPOINT using the extended query protocol. [1] https://www.pgpool.net/pipermail/pgpool-general/2023-December/009051.html Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
When extended query protocol ends?
While taking care of a Pgpool-II trouble report from user [1], I found an interesting usage pattern of the extended query protocol. In my understanding a series of queries in the extended query protocol is ended by a sync message. Then one ReadyForQuery response comes for one sync message. However this does not apply if simple query message is sent instead of sync. Below is outputs from "pgproto" command coming with Pgpool-II. (Lines starting "FE" represents a message from frontend to backend. Lines starting "BE" represents a message from backend to frontend.) FE=> Parse(stmt="", query="SET extra_float_digits = 3") FE=> Bind(stmt="", portal="") FE=> Execute(portal="") FE=> Parse(stmt="", query="SET extra_float_digits = 3") FE=> Bind(stmt="", portal="") FE=> Execute(portal="") FE=> Query (query="SET extra_float_digits = 3") <= BE ParseComplete <= BE BindComplete <= BE CommandComplete(SET) <= BE ParseComplete <= BE BindComplete <= BE CommandComplete(SET) <= BE CommandComplete(SET) <= BE ReadyForQuery(I) FE=> Terminate As you can see, two "SET extra_float_digits = 3" is sent in the extended query protocol, then one "SET extra_float_digits = 3" follows in the simple query protocol. No sync message is sent. However, I get ReadyForQuery at the end. It seems the extended query protocol is ended by a simple query protocol message instead of a sync message. My question is, is this legal in terms of fronted/backend protocol? If it's legal, I think we'd better to explicitly mention in our document. Otherwise, users may be confused. For example, in "55.2.4. Pipelining": "When using this method, completion of the pipeline must be determined by counting ReadyForQuery messages and waiting for that to reach the number of Syncs sent." Apparently this does not apply to the above example because there's 0 sync message. Best reagards, [1] https://www.pgpool.net/pipermail/pgpool-general/2023-December/009051.html -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
> Thank you very much for providing the patch for the RPR implementation. > > After applying the v12-patches, I noticed an issue that > the rpr related parts in window clauses were not displayed in the > view definitions (the definition column of pg_views). > > To address this, I have taken the liberty of adding an additional patch > that modifies the relevant rewriter source code. > I have attached the rewriter patch for your review and would greatly > appreciate your feedback. > > Thank you for your time and consideration. Thank you so much for spotting the issue and creating the patch. I confirmed that your patch applies cleanly and solve the issue. I will include the patches into upcoming v13 patches. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: pgbnech: allow to cancel queries during benchmark
>> +/* send cancel requests to all connections */
>> +static void
>> +cancel_all()
>> +{
>> +for (int i = 0; i < nclients; i++)
>> +{
>> +char errbuf[1];
>> +if (client_states[i].cancel != NULL)
>> +(void) PQcancel(client_states[i].cancel, errbuf,
>> sizeof(errbuf));
>> +}
>> +}
>> +
>>
>> Why in case of errors from PQCancel the error message is neglected? I
>> think it's better to print out the error message in case of error.
>
> Is the message useful for pgbench users? I saw the error is ignored
> in pg_dump, for example in bin/pg_dump/parallel.c
I think the situation is different from pg_dump. Unlike pg_dump, if
PQcancel does not work, users can fix the problem by using
pg_terminate_backend or kill command. In order to make this work, an
appropriate error message is essential.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Re: pgbnech: allow to cancel queries during benchmark
> On Wed, 6 Sep 2023 20:13:34 +0900
> Yugo NAGATA wrote:
>
>> I attached the updated patch v3. The changes since the previous
>> patch includes the following;
>>
>> I removed the unnecessary condition (&& false) that you
>> pointed out in [1].
>>
>> The test was rewritten by using IPC::Run signal() and integrated
>> to "001_pgbench_with_server.pl". This test is skipped on Windows
>> because SIGINT causes to terminate the test itself as discussed
>> in [2] about query cancellation test in psql.
>>
>> I added some comments to describe how query cancellation is
>> handled as I explained in [1].
>>
>> Also, I found the previous patch didn't work on Windows so fixed it.
>> On non-Windows system, a thread waiting a response of long query can
>> be interrupted by SIGINT, but on Windows, threads do not return from
>> waiting until queries they are running are cancelled. This is because,
>> when the signal is received, the system just creates a new thread to
>> execute the callback function specified by setup_cancel_handler, and
>> other thread continue to run[3]. Therefore, the queries have to be
>> cancelled in the callback function.
>>
>> [1]
>> https://www.postgresql.org/message-id/a58388ac-5411-4760-ea46-71324d8324cb%40mines-paristech.fr
>> [2]
>> https://www.postgresql.org/message-id/20230906004524.2fd6ee049f8a6c6f2690b99c%40sraoss.co.jp
>> [3] https://learn.microsoft.com/en-us/windows/console/handlerroutine
>
> I found that --disable-thread-safety option was removed in 68a4b58eca0329.
> So, I removed codes involving ENABLE_THREAD_SAFETY from the patch.
>
> Also, I wrote a commit log draft.
> Previously, Ctrl+C during benchmark killed pgbench immediately,
> but queries running at that time were not cancelled.
Better to mention explicitely that queries keep on running on the
backend. What about this?
Previously, Ctrl+C during benchmark killed pgbench immediately, but
queries were not canceled and they keep on running on the backend
until they tried to send the result to pgbench.
> The commit
> fixes this so that cancel requests are sent for all connections
> before pgbench exits.
"sent for" -> "sent to"
> Attached is the updated version, v4.
+/* send cancel requests to all connections */
+static void
+cancel_all()
+{
+ for (int i = 0; i < nclients; i++)
+ {
+ char errbuf[1];
+ if (client_states[i].cancel != NULL)
+ (void) PQcancel(client_states[i].cancel, errbuf,
sizeof(errbuf));
+ }
+}
+
Why in case of errors from PQCancel the error message is neglected? I
think it's better to print out the error message in case of error.
+* On non-Windows, any callback function is not set. When SIGINT is
+* received, CancelRequested is just set, and only thread #0 is
interrupted
+* and returns from waiting input from the backend. After that, the
thread
+* sends cancel requests to all benchmark queries.
The second line is a little bit long according to the coding
standard. Fix like this?
* On non-Windows, any callback function is not set. When SIGINT is
* received, CancelRequested is just set, and only thread #0 is
* interrupted and returns from waiting input from the backend. After
* that, the thread sends cancel requests to all benchmark queries.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Re: INFORMATION_SCHEMA note
>> On 9 Jan 2024, at 00:54, Tatsuo Ishii wrote: >> >>>> On 4 Jan 2024, at 13:39, Tatsuo Ishii wrote: >>> >>>>> Attached is the patch that does this. >>> >>> I don't think the patch was attached? >>> >>>> Any objection? >>> >>> I didn't study the RFC in depth but as expected it seems to back up your >>> change >>> so the change seems reasonable. >> >> Oops. Sorry. Patch attached. > > That's exactly what I expected it to be, and it LGTM. Thanks for looking into it. Pushed to all supported branches. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: INFORMATION_SCHEMA note
>> On 4 Jan 2024, at 13:39, Tatsuo Ishii wrote: > >>> Attached is the patch that does this. > > I don't think the patch was attached? > >> Any objection? > > I didn't study the RFC in depth but as expected it seems to back up your > change > so the change seems reasonable. Oops. Sorry. Patch attached. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp diff --git a/doc/src/sgml/information_schema.sgml b/doc/src/sgml/information_schema.sgml index 0ca7d5a9e0..9e66be4e83 100644 --- a/doc/src/sgml/information_schema.sgml +++ b/doc/src/sgml/information_schema.sgml @@ -697,8 +697,8 @@ An encoding of some character repertoire. Most older character repertoires only use one encoding form, and so there are no - separate names for them (e.g., LATIN1 is an - encoding form applicable to the LATIN1 + separate names for them (e.g., LATIN2 is an + encoding form applicable to the LATIN2 repertoire). But for example Unicode has the encoding forms UTF8, UTF16, etc. (not all supported by PostgreSQL). Encoding forms are not exposed
Re: INFORMATION_SCHEMA note
(typo in the subject fixed) > In the following paragraph in information_schema: > > character encoding form > > >An encoding of some character repertoire. Most older character >repertoires only use one encoding form, and so there are no >separate names for them (e.g., LATIN1 is an >encoding form applicable to the LATIN1 >repertoire). But for example Unicode has the encoding forms >UTF8, UTF16, etc. (not >all supported by PostgreSQL). Encoding forms are not exposed >as an SQL object, but are visible in this view. > > This claims that the LATIN1 repertoire only uses one encoding form, > but actually LATIN1 can be encoded in another form: ISO-2022-JP-2 (a 7 > bit encoding. See RFC 1554 > (https://datatracker.ietf.org/doc/html/rfc1554) for more details). > > If we still want to list a use-one-encoding-form example, probably we > could use LATIN2 instead or others that are not supported by > ISO-2022-JP-2 (ISO-2022-JP-2 supports LATIN1 and LATIN7). > > Attached is the patch that does this. Any objection? -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
INFORMATION_SCHEMA node
In the following paragraph in information_schema: character encoding form An encoding of some character repertoire. Most older character repertoires only use one encoding form, and so there are no separate names for them (e.g., LATIN1 is an encoding form applicable to the LATIN1 repertoire). But for example Unicode has the encoding forms UTF8, UTF16, etc. (not all supported by PostgreSQL). Encoding forms are not exposed as an SQL object, but are visible in this view. This claims that the LATIN1 repertoire only uses one encoding form, but actually LATIN1 can be encoded in another form: ISO-2022-JP-2 (a 7 bit encoding. See RFC 1554 (https://datatracker.ietf.org/doc/html/rfc1554) for more details). If we still want to list a use-one-encoding-form example, probably we could use LATIN2 instead or others that are not supported by ISO-2022-JP-2 (ISO-2022-JP-2 supports LATIN1 and LATIN7). Attached is the patch that does this. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Fixing pgbench init overflow
> > On Sat, 23 Dec 2023 at 07:18, Chen Hao Hsu wrote: >> Hello, >> >> pgbench mixes int and int64 to initialize the tables. >> When a large enough scale factor is passed, initPopulateTable >> overflows leading to it never completing, ie. >> >> 214740 of 22 tuples (97%) of >> pgbench_accounts done (elapsed 4038.83 s, remaining 98.93 s) >> -214740 of 22 tuples (-97%) of >> pgbench_accounts done (elapsed 4038.97 s, remaining -8176.86 s) >> >> >> Attached is a patch that fixes this, pgbench -i -s 22000 works now. > > I think only the following line can fix this. > > + int64 k; > > Do not need to modify the type of `n`, right? You are right. n represents the return value of pg_snprintf, which is the byte length of the formatted data, which is int, not int64. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
> On 04.12.23 12:40, Tatsuo Ishii wrote: >> diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y >> index d631ac89a9..5a77fca17f 100644 >> --- a/src/backend/parser/gram.y >> +++ b/src/backend/parser/gram.y >> @@ -251,6 +251,8 @@ static Node *makeRecursiveViewSelect(char >> *relname, List *aliases, Node *query); >> DefElem*defelt; >> SortBy *sortby; >> WindowDef *windef; >> +RPCommonSyntax *rpcom; >> +RPSubsetItem*rpsubset; >> JoinExpr *jexpr; >> IndexElem *ielem; >> StatsElem *selem; >> @@ -278,6 +280,7 @@ static Node *makeRecursiveViewSelect(char >> *relname, List *aliases, Node *query); >> MergeWhenClause *mergewhen; >> struct KeyActions *keyactions; >> struct KeyAction *keyaction; >> +RPSkipToskipto; >> } >> %type stmt toplevel_stmt schema_stmt routine_body_stmt > > It is usually not the style to add an entry for every node type to the > %union. Otherwise, we'd have hundreds of entries in there. Ok, I have removed the node types and used existing node types. Also I have moved RPR related %types to same place to make it easier to know what are added by RPR. >> @@ -866,6 +878,7 @@ static Node *makeRecursiveViewSelect(char >> *relname, List *aliases, Node *query); >> %nonassoc UNBOUNDED /* ideally would have same precedence as IDENT */ >> %nonassoc IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE >> %ROLLUP >> SET KEYS OBJECT_P SCALAR VALUE_P WITH WITHOUT >> +%nonassoc MEASURES AFTER INITIAL SEEK PATTERN_P >> %left Op OPERATOR /* multi-character ops and user-defined operators */ >> %left '+' '-' >> %left '*' '/' '%' > > It was recently discussed that these %nonassoc should ideally all have > the same precedence. Did you consider that here? No, I didn't realize it. Thanks for pointing it out. I have removed %nonassoc so that MEASURES etc. have the same precedence as IDENT etc. Attached is the new diff of gram.y against master branch. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index d631ac89a9..6c41aa2e9f 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -659,6 +659,21 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); json_object_constructor_null_clause_opt json_array_constructor_null_clause_opt +%type row_pattern_measure_item + row_pattern_definition +%typeopt_row_pattern_common_syntax + opt_row_pattern_skip_to + row_pattern_subset_item + row_pattern_term +%typeopt_row_pattern_measures + row_pattern_measure_list + row_pattern_definition_list + opt_row_pattern_subset_clause + row_pattern_subset_list + row_pattern_subset_rhs + row_pattern +%type opt_row_pattern_initial_or_seek + first_or_last /* * Non-keyword token types. These are hard-wired into the "flex" lexer. @@ -702,7 +717,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); CURRENT_TIME CURRENT_TIMESTAMP CURRENT_USER CURSOR CYCLE DATA_P DATABASE DAY_P DEALLOCATE DEC DECIMAL_P DECLARE DEFAULT DEFAULTS - DEFERRABLE DEFERRED DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC + DEFERRABLE DEFERRED DEFINE DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC DETACH DICTIONARY DISABLE_P DISCARD DISTINCT DO DOCUMENT_P DOMAIN_P DOUBLE_P DROP @@ -718,7 +733,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); HANDLER HAVING HEADER_P HOLD HOUR_P IDENTITY_P IF_P ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE - INCLUDING INCREMENT INDENT INDEX INDEXES INHERIT INHERITS INITIALLY INLINE_P + INCLUDING INCREMENT INDENT INDEX INDEXES INHERIT INHERITS INITIAL INITIALLY INLINE_P INNER_P INOUT INPUT_P INSENSITIVE INSERT INSTEAD INT_P INTEGER INTERSECT INTERVAL INTO INVOKER IS ISNULL ISOLATION @@ -731,7 +746,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); LEADING LEAKPROOF LEAST LEFT LEVEL LIKE LIMIT LISTEN LOAD LOCAL LOCALTIME LOCALTIMESTAMP LOCATION LOCK_P LOCKED LOGGED - MAPPING MATCH MATCHED MATERIALIZ
Re: Row pattern recognition
> Great. I will look into this. I am impressed the simple NFA implementation. It would be nicer if it could be implemented without using recursion. > By the way, I tested my patch (v10) to handle more large data set and > tried to following query with pgbench database. On my laptop it works > with 100k rows pgbench_accounts table but with beyond the number I got ~~~ I meant 10k. > OOM killer. I would like to enhance this in the next patch. > > SELECT aid, first_value(aid) OVER w, > count(*) OVER w > FROM pgbench_accounts > WINDOW w AS ( > PARTITION BY bid > ORDER BY aid > ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING > AFTER MATCH SKIP PAST LAST ROW > INITIAL > PATTERN (START UP+) > DEFINE > START AS TRUE, > UP AS aid > PREV(aid) > ); I ran this against your patch. It failed around > 60k rows. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
> On Sat, Oct 21, 2023 at 7:39 PM Tatsuo Ishii wrote: >> Attached is the v10 patch. This version enhances the performance of >> pattern matching. > > Nice! I've attached a couple of more stressful tests (window > partitions of 1000 rows each). Beware that the second one runs my > desktop out of memory fairly quickly with the v10 implementation. > > I was able to carve out some time this week to implement a very basic > recursive NFA, which handles both the + and * qualifiers (attached). Great. I will look into this. > It's not production quality -- a frame on the call stack for every row > isn't going to work Yeah. > -- but with only those two features, it's pretty > tiny, and it's able to run the new stress tests with no issue. If I > continue to have time, I hope to keep updating this parallel > implementation as you add features to the StringSet implementation, > and we can see how it evolves. I expect that alternation and grouping > will ratchet up the complexity. Sounds like a plan. By the way, I tested my patch (v10) to handle more large data set and tried to following query with pgbench database. On my laptop it works with 100k rows pgbench_accounts table but with beyond the number I got OOM killer. I would like to enhance this in the next patch. SELECT aid, first_value(aid) OVER w, count(*) OVER w FROM pgbench_accounts WINDOW w AS ( PARTITION BY bid ORDER BY aid ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING AFTER MATCH SKIP PAST LAST ROW INITIAL PATTERN (START UP+) DEFINE START AS TRUE, UP AS aid > PREV(aid) ); Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
Attached is the v10 patch. This version enhances the performance of
pattern matching. Previously it generated all possible pattern string
candidates. This resulted in unnecessarily large number of
candidates. For example if you have 2 pattern variables and the target
frame includes 100 rows, the number of candidates can reach to 2^100
in the worst case. To avoid this, I do a pruning in the v10
patch. Suppose you have:
PATTERN (A B+ C+)
Candidates like "BAC" "CAB" cannot survive because they never satisfy
the search pattern. To judge this, I assign sequence numbers (0, 1, 2)
to (A B C). If the pattern generator tries to generate BA, this is
not allowed because the sequence number for B is 1 and for A is 0, and
0 < 1: B cannot be followed by A. Note that this technique can be
applied when the quantifiers are "+" or "*". Maybe other quantifiers
such as '?' or '{n, m}' can be applied too but I don't confirm yet
because I have not implemented them yet.
Besides this improvement, I fixed a bug in the previous and older
patches: when an expression in DEFINE uses text operators, it errors
out:
ERROR: could not determine which collation to use for string comparison
HINT: Use the COLLATE clause to set the collation explicitly.
This was fixed by adding assign_expr_collations() in
transformDefineClause().
Also I have updated documentation "3.5. Window Functions"
- It still mentioned about rpr(). It's not applied anymore.
- Enhance the description about DEFINE and PATTERN.
- Mention that quantifier '*' is supported.
Finally I have added more test cases to the regression test.
- same pattern variable appears twice
- case for quantifier '*'
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
>From a8b41dd155a2d9ce969083fcfc53bbae3c28b263 Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii
Date: Sun, 22 Oct 2023 11:22:10 +0900
Subject: [PATCH v10 1/7] Row pattern recognition patch for raw parser.
---
src/backend/parser/gram.y | 222 ++--
src/include/nodes/parsenodes.h | 56
src/include/parser/kwlist.h | 8 ++
src/include/parser/parse_node.h | 1 +
4 files changed, 273 insertions(+), 14 deletions(-)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index c224df4ecc..e09eb061f8 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -251,6 +251,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
DefElem *defelt;
SortBy *sortby;
WindowDef *windef;
+ RPCommonSyntax *rpcom;
+ RPSubsetItem *rpsubset;
JoinExpr *jexpr;
IndexElem *ielem;
StatsElem *selem;
@@ -278,6 +280,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
MergeWhenClause *mergewhen;
struct KeyActions *keyactions;
struct KeyAction *keyaction;
+ RPSkipTo skipto;
}
%type stmt toplevel_stmt schema_stmt routine_body_stmt
@@ -453,8 +456,12 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
TriggerTransitions TriggerReferencing
vacuum_relation_list opt_vacuum_relation_list
drop_option_list pub_obj_list
-
-%type opt_routine_body
+row_pattern_measure_list row_pattern_definition_list
+opt_row_pattern_subset_clause
+row_pattern_subset_list row_pattern_subset_rhs
+row_pattern
+%type row_pattern_subset_item
+%type opt_routine_body row_pattern_term
%type group_clause
%type group_by_list
%type group_by_item empty_grouping_set rollup_clause cube_clause
@@ -551,6 +558,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type relation_expr_opt_alias
%type tablesample_clause opt_repeatable_clause
%type target_el set_target insert_column_item
+row_pattern_measure_item row_pattern_definition
+%type first_or_last
%type generic_option_name
%type generic_option_arg
@@ -633,6 +642,9 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type window_clause window_definition_list opt_partition_clause
%type window_definition over_clause window_specification
opt_frame_clause frame_extent frame_bound
+%type opt_row_pattern_common_syntax opt_row_pattern_skip_to
+%type opt_row_pattern_initial_or_seek
+%type opt_row_pattern_measures
%type opt_window_exclusion_clause
%type opt_existing_window_name
%type opt_if_not_exists
@@ -659,7 +671,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
json_object_constructor_null_clause_opt
json_array_constructor_null_clause_opt
-
/*
* Non-keyword token types. These are hard-wired into the "flex" lexer.
* They must be listed first so that their numeric codes do not depend on
@@ -702,7 +713,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
CURRENT_TIME CURRENT_TIMESTAMP
Re: Row pattern recognition
> By the way, I was thinking about eliminating recusrive calls in > pattern matching. Attached is the first cut of the implementation. In > the attached v8 patch: > > - No recursive calls anymore. search_str_set_recurse was removed. > > - Instead it generates all possible pattern variable name initial > strings before pattern matching. Suppose we have "ab" (row 0) and > "ac" (row 1). "a" and "b" represents the pattern variable names > which are evaluated to true. In this case it will generate "aa", > "ac", "ba" and "bc" and they are examined by do_pattern_match one by > one, which performs pattern matching. > > - To implement this, an infrastructure string_set* are created. They > take care of set of StringInfo. > > I found that the previous implementation did not search all possible > cases. I believe the bug is fixed in v8. The v8 patch does not apply anymore due to commit d060e921ea "Remove obsolete executor cleanup code". So I rebased and created v9 patch. The differences from the v8 include: - Fix bug with get_slots. It did not correctly detect the end of full frame. - Add test case using "ROWS BETWEEN CURRENT ROW AND offset FOLLOWING". Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp >From d8fd143ab717fd62814e73fd24bf1a1694143dfc Mon Sep 17 00:00:00 2001 From: Tatsuo Ishii Date: Wed, 4 Oct 2023 14:51:48 +0900 Subject: [PATCH v9 1/7] Row pattern recognition patch for raw parser. --- src/backend/parser/gram.y | 222 ++-- src/include/nodes/parsenodes.h | 56 src/include/parser/kwlist.h | 8 ++ src/include/parser/parse_node.h | 1 + 4 files changed, 273 insertions(+), 14 deletions(-) diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index e56cbe77cb..730c51bc87 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -251,6 +251,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); DefElem *defelt; SortBy *sortby; WindowDef *windef; + RPCommonSyntax *rpcom; + RPSubsetItem *rpsubset; JoinExpr *jexpr; IndexElem *ielem; StatsElem *selem; @@ -278,6 +280,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); MergeWhenClause *mergewhen; struct KeyActions *keyactions; struct KeyAction *keyaction; + RPSkipTo skipto; } %type stmt toplevel_stmt schema_stmt routine_body_stmt @@ -453,8 +456,12 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); TriggerTransitions TriggerReferencing vacuum_relation_list opt_vacuum_relation_list drop_option_list pub_obj_list - -%type opt_routine_body +row_pattern_measure_list row_pattern_definition_list +opt_row_pattern_subset_clause +row_pattern_subset_list row_pattern_subset_rhs +row_pattern +%type row_pattern_subset_item +%type opt_routine_body row_pattern_term %type group_clause %type group_by_list %type group_by_item empty_grouping_set rollup_clause cube_clause @@ -551,6 +558,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type relation_expr_opt_alias %type tablesample_clause opt_repeatable_clause %type target_el set_target insert_column_item +row_pattern_measure_item row_pattern_definition +%type first_or_last %type generic_option_name %type generic_option_arg @@ -633,6 +642,9 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type window_clause window_definition_list opt_partition_clause %type window_definition over_clause window_specification opt_frame_clause frame_extent frame_bound +%type opt_row_pattern_common_syntax opt_row_pattern_skip_to +%type opt_row_pattern_initial_or_seek +%type opt_row_pattern_measures %type opt_window_exclusion_clause %type opt_existing_window_name %type opt_if_not_exists @@ -659,7 +671,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); json_object_constructor_null_clause_opt json_array_constructor_null_clause_opt - /* * Non-keyword token types. These are hard-wired into the "flex" lexer. * They must be listed first so that their numeric codes do not depend on @@ -702,7 +713,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); CURRENT_TIME CURRENT_TIMESTAMP CURRENT_USER CURSOR CYCLE DATA_P DATABASE DAY_P DEALLOCATE DEC DECIMAL_P DECLARE DEFAULT DEFAULTS - DEFERRABLE DEFERRED DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC + DEFERRABLE DEFERRED DEFINE DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC DETACH DICTIONARY DISABLE_P DISCARD DISTINCT DO DOCUMENT_P DOMAIN_P DOUBLE_P DROP @@ -718,7 +729,7 @@ static Node *make
Re: Row pattern recognition
> On Fri, Sep 22, 2023, 3:13 AM Tatsuo Ishii wrote: > >> > Op 9/22/23 om 07:16 schreef Tatsuo Ishii: >> >>> Attached is the fix against v6 patch. I will include this in upcoming >> >>> v7 patch. >> >> Attached is the v7 patch. It includes the fix mentioned above. Also >> > (Champion's address bounced; removed) >> >> On my side his adress bounced too:-< >> > > Yep. I'm still here, just lurking for now. It'll take a little time for me > to get back to this thread, as my schedule has changed significantly. :D Hope you get back soon... By the way, I was thinking about eliminating recusrive calls in pattern matching. Attached is the first cut of the implementation. In the attached v8 patch: - No recursive calls anymore. search_str_set_recurse was removed. - Instead it generates all possible pattern variable name initial strings before pattern matching. Suppose we have "ab" (row 0) and "ac" (row 1). "a" and "b" represents the pattern variable names which are evaluated to true. In this case it will generate "aa", "ac", "ba" and "bc" and they are examined by do_pattern_match one by one, which performs pattern matching. - To implement this, an infrastructure string_set* are created. They take care of set of StringInfo. I found that the previous implementation did not search all possible cases. I believe the bug is fixed in v8. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp >From 281ee5c3eae85f96783422cb2f9987c3af8c8a68 Mon Sep 17 00:00:00 2001 From: Tatsuo Ishii Date: Mon, 25 Sep 2023 14:01:14 +0900 Subject: [PATCH v8 1/7] Row pattern recognition patch for raw parser. --- src/backend/parser/gram.y | 222 ++-- src/include/nodes/parsenodes.h | 56 src/include/parser/kwlist.h | 8 ++ src/include/parser/parse_node.h | 1 + 4 files changed, 273 insertions(+), 14 deletions(-) diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 7d2032885e..74c2069050 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -251,6 +251,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); DefElem *defelt; SortBy *sortby; WindowDef *windef; + RPCommonSyntax *rpcom; + RPSubsetItem *rpsubset; JoinExpr *jexpr; IndexElem *ielem; StatsElem *selem; @@ -278,6 +280,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); MergeWhenClause *mergewhen; struct KeyActions *keyactions; struct KeyAction *keyaction; + RPSkipTo skipto; } %type stmt toplevel_stmt schema_stmt routine_body_stmt @@ -453,8 +456,12 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); TriggerTransitions TriggerReferencing vacuum_relation_list opt_vacuum_relation_list drop_option_list pub_obj_list - -%type opt_routine_body +row_pattern_measure_list row_pattern_definition_list +opt_row_pattern_subset_clause +row_pattern_subset_list row_pattern_subset_rhs +row_pattern +%type row_pattern_subset_item +%type opt_routine_body row_pattern_term %type group_clause %type group_by_list %type group_by_item empty_grouping_set rollup_clause cube_clause @@ -551,6 +558,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type relation_expr_opt_alias %type tablesample_clause opt_repeatable_clause %type target_el set_target insert_column_item +row_pattern_measure_item row_pattern_definition +%type first_or_last %type generic_option_name %type generic_option_arg @@ -633,6 +642,9 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type window_clause window_definition_list opt_partition_clause %type window_definition over_clause window_specification opt_frame_clause frame_extent frame_bound +%type opt_row_pattern_common_syntax opt_row_pattern_skip_to +%type opt_row_pattern_initial_or_seek +%type opt_row_pattern_measures %type opt_window_exclusion_clause %type opt_existing_window_name %type opt_if_not_exists @@ -659,7 +671,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); json_object_constructor_null_clause_opt json_array_constructor_null_clause_opt - /* * Non-keyword token types. These are hard-wired into the "flex" lexer. * They must be listed first so that their numeric codes do not depend on @@ -702,7 +713,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); CURRENT_TIME CURRENT_TIMESTAMP CURRENT_USER CURSOR CYCLE DATA_P DATABASE DAY_P DEALLOCATE DEC DECIMAL_P DECLARE DEFAULT DEFAULTS - DEFERRABLE DEFERRED DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC +
Re: Row pattern recognition
> Op 9/22/23 om 07:16 schreef Tatsuo Ishii: >>> Attached is the fix against v6 patch. I will include this in upcoming >>> v7 patch. >> Attached is the v7 patch. It includes the fix mentioned above. Also > (Champion's address bounced; removed) On my side his adress bounced too:-< > Hi, > > In my hands, make check fails on the rpr test; see attached .diff > file. > In these two statements: > -- using NEXT > -- using AFTER MATCH SKIP TO NEXT ROW > result of first_value(price) and next_value(price) are empty. Strange. I have checked out fresh master branch and applied the v7 patches, then ran make check. All tests including the rpr test passed. This is Ubuntu 20.04. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
> Attached is the fix against v6 patch. I will include this in upcoming v7 > patch. Attached is the v7 patch. It includes the fix mentioned above. Also this time the pattern matching engine is enhanced: previously it recursed to row direction, which means if the number of rows in a frame is large, it could exceed the limit of stack depth. The new version recurses over matched pattern variables in a row, which is at most 26 which should be small enough. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp >From 4d5be4c27ae5e4a50924520574e2c1ca3e398551 Mon Sep 17 00:00:00 2001 From: Tatsuo Ishii Date: Fri, 22 Sep 2023 13:53:35 +0900 Subject: [PATCH v7 1/7] Row pattern recognition patch for raw parser. --- src/backend/parser/gram.y | 222 ++-- src/include/nodes/parsenodes.h | 56 src/include/parser/kwlist.h | 8 ++ src/include/parser/parse_node.h | 1 + 4 files changed, 273 insertions(+), 14 deletions(-) diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 7d2032885e..74c2069050 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -251,6 +251,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); DefElem *defelt; SortBy *sortby; WindowDef *windef; + RPCommonSyntax *rpcom; + RPSubsetItem *rpsubset; JoinExpr *jexpr; IndexElem *ielem; StatsElem *selem; @@ -278,6 +280,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); MergeWhenClause *mergewhen; struct KeyActions *keyactions; struct KeyAction *keyaction; + RPSkipTo skipto; } %type stmt toplevel_stmt schema_stmt routine_body_stmt @@ -453,8 +456,12 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); TriggerTransitions TriggerReferencing vacuum_relation_list opt_vacuum_relation_list drop_option_list pub_obj_list - -%type opt_routine_body +row_pattern_measure_list row_pattern_definition_list +opt_row_pattern_subset_clause +row_pattern_subset_list row_pattern_subset_rhs +row_pattern +%type row_pattern_subset_item +%type opt_routine_body row_pattern_term %type group_clause %type group_by_list %type group_by_item empty_grouping_set rollup_clause cube_clause @@ -551,6 +558,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type relation_expr_opt_alias %type tablesample_clause opt_repeatable_clause %type target_el set_target insert_column_item +row_pattern_measure_item row_pattern_definition +%type first_or_last %type generic_option_name %type generic_option_arg @@ -633,6 +642,9 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type window_clause window_definition_list opt_partition_clause %type window_definition over_clause window_specification opt_frame_clause frame_extent frame_bound +%type opt_row_pattern_common_syntax opt_row_pattern_skip_to +%type opt_row_pattern_initial_or_seek +%type opt_row_pattern_measures %type opt_window_exclusion_clause %type opt_existing_window_name %type opt_if_not_exists @@ -659,7 +671,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); json_object_constructor_null_clause_opt json_array_constructor_null_clause_opt - /* * Non-keyword token types. These are hard-wired into the "flex" lexer. * They must be listed first so that their numeric codes do not depend on @@ -702,7 +713,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); CURRENT_TIME CURRENT_TIMESTAMP CURRENT_USER CURSOR CYCLE DATA_P DATABASE DAY_P DEALLOCATE DEC DECIMAL_P DECLARE DEFAULT DEFAULTS - DEFERRABLE DEFERRED DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC + DEFERRABLE DEFERRED DEFINE DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC DETACH DICTIONARY DISABLE_P DISCARD DISTINCT DO DOCUMENT_P DOMAIN_P DOUBLE_P DROP @@ -718,7 +729,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); HANDLER HAVING HEADER_P HOLD HOUR_P IDENTITY_P IF_P ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE - INCLUDING INCREMENT INDENT INDEX INDEXES INHERIT INHERITS INITIALLY INLINE_P + INCLUDING INCREMENT INDENT INDEX INDEXES INHERIT INHERITS INITIAL INITIALLY INLINE_P INNER_P INOUT INPUT_P INSENSITIVE INSERT INSTEAD INT_P INTEGER INTERSECT INTERVAL INTO INVOKER IS ISNULL ISOLATION @@ -731,7 +742,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); LEADING LEAKPROOF LEAST LEFT LEVEL LIKE LIMIT LISTEN LOAD LOCAL LOCALTIME LOCALTIMESTAMP LOCATION LOCK_P LOCKED LOGGED - MAPPING MATCH MATCHED MATERIALIZED MAXVALUE MERGE METHOD + MAPPING MATCH MATCHED MATERIALIZED MAXVALUE MEASURES MERGE METHOD MINUTE_P MINVALUE MODE MONTH_P MOVE NA
Re: Row pattern recognition
> On 9/13/23 07:14, Tatsuo Ishii wrote: >>> >> I was looking for this but I only found ISO/IEC 19075-5:2021. >> https://www.iso.org/standard/78936.html >> Maybe 19075-5:2021 is the latest one? > > Yes, probably. Sorry. No problem. Thanks for confirmation. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
> I was looking for this but I only found ISO/IEC 19075-5:2021. https://www.iso.org/standard/78936.html Maybe 19075-5:2021 is the latest one? Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
| 1400 | || | | |
|
company2 | 07-05-2023 | 1500 | || | | |
| 0
company2 | 07-06-2023 |60 | 60 | 1200 | 1300 | 60 | 3660 |
915. | 4
company2 | 07-07-2023 | 1100 | || | | |
|
company2 | 07-08-2023 | 1300 | || | | |
|
company2 | 07-09-2023 | 1200 | || | | |
|
company2 | 07-10-2023 | 1300 | || | | |
| 0
(20 rows)
Attached is the fix against v6 patch. I will include this in upcoming v7 patch.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index 32270d051a..2b78cb6722 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -968,12 +968,12 @@ eval_windowaggregates(WindowAggState *winstate)
{
/*
* If the skip mode is SKIP TO PAST LAST ROW and we already know that
- * current row is a skipped row or an unmatched row, we don't need to
- * accumulate rows, just return NULL.
+ * current row is a skipped row, we don't need to accumulate rows,
+ * just return NULL. Note that for unamtched row, we need to do
+ * aggregation so that count(*) shows 0, rather than NULL.
*/
if (winstate->rpSkipTo == ST_PAST_LAST_ROW &&
- (get_reduced_frame_map(winstate, winstate->currentpos) == RF_SKIPPED ||
- get_reduced_frame_map(winstate, winstate->currentpos) == RF_UNMATCHED))
+ get_reduced_frame_map(winstate, winstate->currentpos) == RF_SKIPPED)
agg_result_isnull = true;
}
@@ -1080,8 +1080,8 @@ next_tuple:
result, isnull);
/*
- * RPR is enabled and we just return NULL. because skip mode is SKIP
- * TO PAST LAST ROW and current row is skipped row or unmatched row.
+ * RPR is defined and we just return NULL because skip mode is SKIP
+ * TO PAST LAST ROW and current row is skipped row.
*/
if (agg_result_isnull)
{
diff --git a/src/test/regress/expected/rpr.out b/src/test/regress/expected/rpr.out
index 63bed05f05..97bdc630d1 100644
--- a/src/test/regress/expected/rpr.out
+++ b/src/test/regress/expected/rpr.out
@@ -457,22 +457,22 @@ DOWN AS price < PREV(price)
company1 | 07-02-2023 | 200 | || | | | |
company1 | 07-03-2023 | 150 | || | | | |
company1 | 07-04-2023 | 140 | || | | | |
- company1 | 07-05-2023 | 150 | || | | | |
+ company1 | 07-05-2023 | 150 | || | | | | 0
company1 | 07-06-2023 |90 | 90 |120 | 130 | 90 | 450 | 112.5000 | 4
company1 | 07-07-2023 | 110 | || | | | |
company1 | 07-08-2023 | 130 | || | | | |
company1 | 07-09-2023 | 120 | || | | | |
- company1 | 07-10-2023 | 130 | || | | | |
+ company1 | 07-10-2023 | 130 | || | | | | 0
company2 | 07-01-2023 |50 | 50 | 1400 | 2000 | 50 | 4950 | 1237.5000 | 4
company2 | 07-02-2023 | 2000 | || | | | |
company2 | 07-03-2023 | 1500 | || | | | |
company2 | 07-04-2023 | 1400 | || | | | |
- company2 | 07-05-2023 | 1500 | || | | | |
+ company2 | 07-05-2023 | 1500 | || | | | | 0
company2 | 07-06-2023 |60 | 60 | 1200 | 1300 | 60 | 3660 | 915. | 4
company2 | 07-07-2023 | 1100 | || | | | |
company2 | 07-08-2023 | 1300 | || | | | |
company2 | 07-09-2023 | 1200 | || | | | |
- company2 | 07-10-2023 | 1300 | || | | | |
+ company2 | 07-10-2023 | 1300 | || | | | | 0
(20 rows)
-- using AFTER MATCH SKIP TO NEXT ROW
Re: Row pattern recognition
| company2 | 07-09-2023 | 1200 | || | | | | company2 | 07-10-2023 | 1300 | || | | | | (20 rows) Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp >From c5699cdf64df9b102c5d9e3b6f6002e504c93fc6 Mon Sep 17 00:00:00 2001 From: Tatsuo Ishii Date: Tue, 12 Sep 2023 14:22:22 +0900 Subject: [PATCH v6 1/7] Row pattern recognition patch for raw parser. --- src/backend/parser/gram.y | 216 +--- src/include/nodes/parsenodes.h | 56 + src/include/parser/kwlist.h | 8 ++ src/include/parser/parse_node.h | 1 + 4 files changed, 267 insertions(+), 14 deletions(-) diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 7d2032885e..70409cdc9a 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -251,6 +251,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); DefElem *defelt; SortBy *sortby; WindowDef *windef; + RPCommonSyntax *rpcom; + RPSubsetItem *rpsubset; JoinExpr *jexpr; IndexElem *ielem; StatsElem *selem; @@ -453,8 +455,12 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); TriggerTransitions TriggerReferencing vacuum_relation_list opt_vacuum_relation_list drop_option_list pub_obj_list - -%type opt_routine_body +row_pattern_measure_list row_pattern_definition_list +opt_row_pattern_subset_clause +row_pattern_subset_list row_pattern_subset_rhs +row_pattern +%type row_pattern_subset_item +%type opt_routine_body row_pattern_term %type group_clause %type group_by_list %type group_by_item empty_grouping_set rollup_clause cube_clause @@ -551,6 +557,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type relation_expr_opt_alias %type tablesample_clause opt_repeatable_clause %type target_el set_target insert_column_item +row_pattern_measure_item row_pattern_definition %type generic_option_name %type generic_option_arg @@ -633,6 +640,9 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type window_clause window_definition_list opt_partition_clause %type window_definition over_clause window_specification opt_frame_clause frame_extent frame_bound +%type opt_row_pattern_common_syntax opt_row_pattern_skip_to +%type opt_row_pattern_initial_or_seek +%type opt_row_pattern_measures %type opt_window_exclusion_clause %type opt_existing_window_name %type opt_if_not_exists @@ -659,7 +669,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); json_object_constructor_null_clause_opt json_array_constructor_null_clause_opt - /* * Non-keyword token types. These are hard-wired into the "flex" lexer. * They must be listed first so that their numeric codes do not depend on @@ -702,7 +711,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); CURRENT_TIME CURRENT_TIMESTAMP CURRENT_USER CURSOR CYCLE DATA_P DATABASE DAY_P DEALLOCATE DEC DECIMAL_P DECLARE DEFAULT DEFAULTS - DEFERRABLE DEFERRED DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC + DEFERRABLE DEFERRED DEFINE DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC DETACH DICTIONARY DISABLE_P DISCARD DISTINCT DO DOCUMENT_P DOMAIN_P DOUBLE_P DROP @@ -718,7 +727,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); HANDLER HAVING HEADER_P HOLD HOUR_P IDENTITY_P IF_P ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE - INCLUDING INCREMENT INDENT INDEX INDEXES INHERIT INHERITS INITIALLY INLINE_P + INCLUDING INCREMENT INDENT INDEX INDEXES INHERIT INHERITS INITIAL INITIALLY INLINE_P INNER_P INOUT INPUT_P INSENSITIVE INSERT INSTEAD INT_P INTEGER INTERSECT INTERVAL INTO INVOKER IS ISNULL ISOLATION @@ -731,7 +740,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); LEADING LEAKPROOF LEAST LEFT LEVEL LIKE LIMIT LISTEN LOAD LOCAL LOCALTIME LOCALTIMESTAMP LOCATION LOCK_P LOCKED LOGGED - MAPPING MATCH MATCHED MATERIALIZED MAXVALUE MERGE METHOD + MAPPING MATCH MATCHED MATERIALIZED MAXVALUE MEASURES MERGE METHOD MINUTE_P MINVALUE MODE MONTH_P MOVE NAME_P NAMES NATIONAL NATURAL NCHAR NEW NEXT NFC NFD NFKC NFKD NO NONE @@ -743,8 +752,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); ORDER ORDINALITY OTHERS OUT_P OUTER_P OVER OVERLAPS OVERLAY OVERRIDING OWNED OWNER - PARALLEL PARAMETER PARSER PARTIAL PARTITION PASSING PASSWORD - PLACING PLANS POLICY + PARALLEL PARAMETER PARSER PARTIAL PARTITION PASSING PASSWORD PAST + PATTERN_P PERMUTE PLACING PLANS POLICY POSITION PRECEDING PRECISION PRESERVE PREPARE PREPARED PRIMARY PRIOR PRIVILEGES PROCEDURAL PROCEDURE PROCEDURES PR
Re: Row pattern recognition
e backtracking than the current > pg_reg* API is going to give us, or else I'm worried performance is > going to fall off a cliff with usefully-large partitions Agreed. > - there's a lot of stuff in POSIX EREs that we don't need, and of the > features we do need, the + quantifier is probably one of the easiest > - it seems easier to prove the correctness of a slow, naive, > row-at-a-time engine, because we can compare it directly to the spec > > So what I'm thinking is: if I start by open-coding the + quantifier, and > slowly add more pieces in, then it might be easier to see the parts of > src/backend/regex that I've duplicated. We can try to expose those parts > directly from the internal API to replace my bad implementation. And if > there are parts that aren't duplicated, then it'll be easier to explain > why we need something different from the current engine. > > Does that seem like a workable approach? (Worst-case, my code is just > horrible, and we throw it in the trash.) Yes, it seems workable. I think for the first cut of RPR needs at least the +quantifier with reasonable performance. The current naive implementation seems to have issue because of exhaustive search. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
Hi, > Hello! > >> (1) I completely changed the pattern matching engine so that it >> performs backtracking. Now the engine evaluates all pattern elements >> defined in PATTER against each row, saving matched pattern variables >> in a string per row. For example if the pattern element A and B >> evaluated to true, a string "AB" is created for current row. > > If I understand correctly, this strategy assumes that one row's > membership in a pattern variable is independent of the other rows' > membership. But I don't think that's necessarily true: > > DEFINE > A AS PREV(CLASSIFIER()) IS DISTINCT FROM 'A', > ... But: UP AS price > PREV(price) also depends on previous row, no? Can you please elaborate how your example could break current implementation? I cannot test it because CLASSIFIER is not implemented yet. >> See row_is_in_reduced_frame, search_str_set and search_str_set_recurse >> in nodeWindowAggs.c for more details. For now I use a naive depth >> first search and probably there is a lot of rooms for optimization >> (for example rewriting it without using >> recursion). > > The depth-first match is doing a lot of subtle work here. For example, with > > PATTERN ( A+ B+ ) > DEFINE A AS TRUE, >B AS TRUE > > (i.e. all rows match both variables), and three rows in the partition, > our candidates will be tried in the order > > aaa > aab > aba > abb > ... > bbb > > The only possible matches against our regex `^a+b+` are "aab" and "abb", > and that order matches the preferment order, so it's fine. But it's easy > to come up with a pattern where that's the wrong order, like > > PATTERN ( A+ (B|A)+ ) > > Now "aaa" will be considered before "aab", which isn't correct. Can you explain a little bit more? I think 'aaa' matches a regular expression 'a+(b|a)+' and should be no problem before "aab" is considered. > Similarly, the assumption that we want to match the longest string only > works because we don't allow alternation yet. Can you please clarify more on this? > Cool, I will give this piece some more thought. Do you mind if I try to > add some more complicated pattern quantifiers to stress the > architecture, or would you prefer to tackle that later? Just alternation > by itself will open up a world of corner cases. Do you mean you want to provide a better patch for the pattern matching part? That will be helpfull. Because I am currently working on the aggregation part and have no time to do it. However, the aggregation work affects the v5 patch: it needs a refactoring. So can you wait until I release v6 patch? I hope it will be released in two weeks or so. > On 8/9/23 01:41, Tatsuo Ishii wrote: >> - I found a bug with pattern matching code. It creates a string for >> subsequent regular expression matching. It uses the initial letter >> of each define variable name. For example, if the varname is "foo", >> then "f" is used. Obviously this makes trouble if we have two or >> more variables starting with same "f" (e.g. "food"). To fix this, I >> assign [a-z] to each variable instead of its initial letter. However >> this way limits us not to have more than 26 variables. I hope 26 is >> enough for most use cases. > > There are still plenty of alphanumerics left that could be assigned... > > But I'm wondering if we might want to just implement the NFA directly? > The current implementation's Cartesian explosion can probably be pruned > aggressively, but replaying the entire regex match once for every > backtracked step will still duplicate a lot of work. Not sure if you mean implementing new regular expression engine besides src/backend/regexp. I am afraid it's not a trivial work. The current regexp code consists of over 10k lines. What do you think? > I've attached another test case; it looks like last_value() is depending > on some sort of side effect from either first_value() or nth_value(). I > know the window frame itself is still under construction, so apologies > if this is an expected failure. Thanks. Fortunately current code which I am working passes the new test. I will include it in the next (v6) patch. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
> Hi, > > The patches compile & tests run fine but this statement from the > documentation crashes an assert-enabled server: > > SELECT company, tdate, price, max(price) OVER w FROM stock > WINDOW w AS ( > PARTITION BY company > 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) > ); > server closed the connection unexpectedly > This probably means the server terminated abnormally > before or while processing the request. > connection to server was lost Thank you for the report. Currently the patch has an issue with aggregate functions including max. I have been working on aggregations in row pattern recognition but will take more time to complete the part. In the mean time if you want to play with RPR, you can try window functions. Examples can be found in src/test/regress/sql/rpr.sql. Here is one of this: -- the first row start with less than or equal to 100 SELECT company, tdate, price, first_value(price) OVER w, last_value(price) OVER w FROM stock WINDOW w AS ( PARTITION BY company ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING INITIAL PATTERN (LOWPRICE UP+ DOWN+) DEFINE LOWPRICE AS price <= 100, UP AS price > PREV(price), DOWN AS price < PREV(price) ); -- second row raises 120% SELECT company, tdate, price, first_value(price) OVER w, last_value(price) OVER w FROM stock WINDOW w AS ( PARTITION BY company ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING INITIAL PATTERN (LOWPRICE UP+ DOWN+) DEFINE LOWPRICE AS price <= 100, UP AS price > PREV(price) * 1.2, DOWN AS price < PREV(price) ); Sorry for inconvenience. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Incremental View Maintenance, take 2
> attached is my refactor. there is some whitespace errors in the > patches, you need use > git apply --reject --whitespace=fix > basedon_v29_matview_c_refactor_update_set_clause.patch > > Also you patch cannot use git apply, i finally found out bulk apply I have no problem with applying Yugo's v29 patches using git apply, no white space errors. $ git apply ~/v29* (the patches are saved under my home directory). I suggest you to check your email application whether it correctly saved the patch files for you. FYI, here are results from sha256sum: ffac37cb455788c1105ffc01c6b606de75f53321c2f235f7efa19f3f52d12b9e v29-0001-Add-a-syntax-to-create-Incrementally-Maintainabl.patch f684485e7c9ac1b2990943a3c73fa49a9091a268917547d9e116baef5118cca7 v29-0002-Add-relisivm-column-to-pg_class-system-catalog.patch fcf5bc8ae562ed1c2ab397b499544ddab03ad2c3acb2263d0195a3ec799b131c v29-0003-Allow-to-prolong-life-span-of-transition-tables-.patch a7a13ef8e73c4717166db079d5607f78d21199379de341a0e8175beef5ea1c1a v29-0004-Add-Incremental-View-Maintenance-support-to-pg_d.patch a2aa51d035774867bfab1580ef14143998dc71c1b941bd1a3721dc019bc62649 v29-0005-Add-Incremental-View-Maintenance-support-to-psql.patch fe0225d761a08eb80082f1a2c039b9b8b20626169b03abaf649db9c74fe99194 v29-0006-Add-Incremental-View-Maintenance-support.patch 68b007befedcf92fc83ab8c3347ac047a50816f061c77b69281e12d52944db82 v29-0007-Add-DISTINCT-support-for-IVM.patch 2201241a22095f736a17383fc8b26d48a459ebf1c2f5cf120896cfc0ce5e03e4 v29-0008-Add-aggregates-support-in-IVM.patch 6390117c559bf1585349c5a09b77b784e086ccc22eb530cd364ce78371c66741 v29-0009-Add-support-for-min-max-aggregates-for-IVM.patch 7019a116c64127783bd9c682ddf1ee3792286d0e41c91a33010111e7be2c9459 v29-0010-Add-regression-tests-for-Incremental-View-Mainte.patch 189afdc7da866bd958e2d554ba12adf93d7e6d0acb581290a48d72fcf640e243 v29-0011-Add-documentations-about-Incremental-View-Mainte.patch Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
Attached is the v5 patch. Differences from previous patch include: * Resolve complaint from "PostgreSQL Patch Tester" https://commitfest.postgresql.org/44/4460/ - Change gram.y to use PATTERN_P instead of PATTERN. Using PATTERN seems to make trouble with Visual Studio build. : : [10:07:57.853] FAILED: src/backend/parser/parser.a.p/meson-generated_.._gram.c.obj [10:07:57.853] "cl" "-Isrc\backend\parser\parser.a.p" "-Isrc\backend\parser" "-I..\src\backend\parser" "-Isrc\include" "-I..\src\include" "-Ic:\openssl\1.1\include" "-I..\src\include\port\win32" "-I..\src\include\port\win32_msvc" "/MDd" "/nologo" "/showIncludes" "/utf-8" "/W2" "/Od" "/Zi" "/DWIN32" "/DWINDOWS" "/D__WINDOWS__" "/D__WIN32__" "/D_CRT_SECURE_NO_DEPRECATE" "/D_CRT_NONSTDC_NO_DEPRECATE" "/wd4018" "/wd4244" "/wd4273" "/wd4101" "/wd4102" "/wd4090" "/wd4267" "-DBUILDING_DLL" "/Fdsrc\backend\parser\parser.a.p\meson-generated_.._gram.c.pdb" /Fosrc/backend/parser/parser.a.p/meson-generated_.._gram.c.obj "/c" src/backend/parser/gram.c [10:07:57.860] c:\cirrus\build\src\backend\parser\gram.h(379): error C2365: 'PATTERN': redefinition; previous definition was 'typedef' [10:07:57.860] C:\Program Files (x86)\Windows Kits\10\include\10.0.20348.0\um\wingdi.h(1375): note: see declaration of 'PATTERN' [10:07:57.860] c:\cirrus\build\src\backend\parser\gram.h(379): error C2086: 'yytokentype PATTERN': redefinition [10:07:57.860] c:\cirrus\build\src\backend\parser\gram.h(379): note: see declaration of 'PATTERN' [10:07:57.860] ninja: build stopped: subcommand failed. * Resolve complaint from "make headerscheck" - Change Windowapi.h and nodeWindowAgg.c to remove unecessary extern and public functions. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp >From 3e02bccbd3dc02304d6dc34f5ab6cc6dd2ee26d1 Mon Sep 17 00:00:00 2001 From: Tatsuo Ishii Date: Sat, 2 Sep 2023 15:32:49 +0900 Subject: [PATCH v5 1/7] Row pattern recognition patch for raw parser. --- src/backend/parser/gram.y | 216 +--- src/include/nodes/parsenodes.h | 56 + src/include/parser/kwlist.h | 8 ++ src/include/parser/parse_node.h | 1 + 4 files changed, 267 insertions(+), 14 deletions(-) diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 7d2032885e..70409cdc9a 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -251,6 +251,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); DefElem *defelt; SortBy *sortby; WindowDef *windef; + RPCommonSyntax *rpcom; + RPSubsetItem *rpsubset; JoinExpr *jexpr; IndexElem *ielem; StatsElem *selem; @@ -453,8 +455,12 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); TriggerTransitions TriggerReferencing vacuum_relation_list opt_vacuum_relation_list drop_option_list pub_obj_list - -%type opt_routine_body +row_pattern_measure_list row_pattern_definition_list +opt_row_pattern_subset_clause +row_pattern_subset_list row_pattern_subset_rhs +row_pattern +%type row_pattern_subset_item +%type opt_routine_body row_pattern_term %type group_clause %type group_by_list %type group_by_item empty_grouping_set rollup_clause cube_clause @@ -551,6 +557,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type relation_expr_opt_alias %type tablesample_clause opt_repeatable_clause %type target_el set_target insert_column_item +row_pattern_measure_item row_pattern_definition %type generic_option_name %type generic_option_arg @@ -633,6 +640,9 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type window_clause window_definition_list opt_partition_clause %type window_definition over_clause window_specification opt_frame_clause frame_extent frame_bound +%type opt_row_pattern_common_syntax opt_row_pattern_skip_to +%type opt_row_pattern_initial_or_seek +%type opt_row_pattern_measures %type opt_window_exclusion_clause %type opt_existing_window_name %type opt_if_not_exists @@ -659,7 +669,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); json_object_constructor_null_clause_opt json_array_constructor_null_clause_opt - /* * Non-keyword token types. These are hard-wired into the "flex" lexer. * They must be listed first so that their numeric codes do not depend on @@ -702,7 +711,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); CURRE
Re: pgbench: allow to exit immediately when any client is aborted
Yugo, Fabien, >>> I start to think this behavior is ok and consistent with previous >>> behavior of pgbench because serialization (and dealock) errors have >>> been treated specially from other types of errors, such as accessing >>> non existing tables. However, I suggest to add more sentences to the >>> explanation of this option so that users are not confused by this. >>> >>> + >>> + --exit-on-abort >>> + >>> + >>> +Exit immediately when any client is aborted due to some error. >>> Without >>> +this option, even when a client is aborted, other clients could >>> continue >>> +their run as specified by -t or >>> -T option, >>> +and pgbench will print an incomplete >>> results >>> +in this case. >>> + >>> + >>> + >>> + >>> >>> What about inserting "Note that serialization failures or deadlock >>> failures will not abort client. See >> linkend="failures-and-retries"/> for more information." into the end >>> of this paragraph? >> >> --exit-on-abort is related to "abort" of a client instead of error or >> failure itself, so rather I wonder a bit that mentioning >> serialization/deadlock >> failures might be confusing. However, if users may think of such failures >> from >> "abort", it could be beneficial to add the sentences with some modification >> as >> below. > > I myself confused by this and believe that adding extra paragraph is > beneficial to users. > >> "Note that serialization failures or deadlock failures does not abort the >> client, so they are not affected by this option. >> See for more information." > > "does not" --> "do not". > >>> BTW, I think: >>> Exit immediately when any client is aborted due to some error. >>> Without >>> >>> should be: >>> Exit immediately when any client is aborted due to some errors. >>> Without >>> >>> (error vs. erros) >> >> Well, I chose "some" to mean "unknown or unspecified", not "an unspecified >> amount >> or number of", so singular form "error" is used. > > Ok. > >> Instead, should we use "due to a error"? > > I don't think so. > >>> Also: >>> + --exit-on-abort is specified . Otherwise in the >>> worst >>> >>> There is an extra space between "specified" and ".". >> >> Fixed. >> >> Also, I fixed the place of the description in the documentation >> to alphabetical order >> >> Attached is the updated patch. > > Looks good to me. If there's no objection, I will commit this next week. I have pushed the patch. Thank you for the conributions! Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Wrong usage of pqMsg_Close message code?
> Yeah, both of you are right here. Anyway, it seems to me that there > is a bit more going on in protocol.h. I have noticed two more things > that are incorrect: > - HandleParallelMessage is missing a message for 'P', but I think that > we should have a code for it as well as part of the parallel query > protocol. I did not know this. Why is this not explained in the frontend/backend protocol document? > - PqMsg_Terminate can be sent by the frontend *and* the backend, see > fe-connect.c and parallel.c. However, protocol.h documents it as a > frontend-only code. I do not blame protocol.h because our frontend/backend protocol document also stats that it's a frontend only message. Someone who started to use 'X' in backend should have added that in the documentation. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Wrong usage of pqMsg_Close message code?
>> Hi Pavel,
>>
>> > There is message PqMsgClose, but this should be used from client side.
>> Here should be used PqMsg_CommandComplete instead?
>>
>> It seems so. This change was introduced in f4b54e1ed98 [1]:
>>
>> ```
>> --- a/src/backend/tcop/dest.c
>> +++ b/src/backend/tcop/dest.c
>> @@ -176,7 +176,7 @@ EndCommand(const QueryCompletion *qc, CommandDest
>> dest, bool force_undecorated_o
>>
>> len = BuildQueryCompletionString(completionTag, qc,
>>
>> force_undecorated_output);
>> - pq_putmessage('C', completionTag, len + 1);
>> + pq_putmessage(PqMsg_Close, completionTag, len + 1);
>>
>> case DestNone:
>> case DestDebug
>> ```
>>
>> It should have been PqMsg_CommandComplete.
>>
>> [1]:
>> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=f4b54e1ed98
>
>
> here is a patch - all tests passed
I think EndReplicationCommand needs to be fixed as well.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Re: pgbench: allow to exit immediately when any client is aborted
>> I start to think this behavior is ok and consistent with previous >> behavior of pgbench because serialization (and dealock) errors have >> been treated specially from other types of errors, such as accessing >> non existing tables. However, I suggest to add more sentences to the >> explanation of this option so that users are not confused by this. >> >> + >> + --exit-on-abort >> + >> + >> +Exit immediately when any client is aborted due to some error. >> Without >> +this option, even when a client is aborted, other clients could >> continue >> +their run as specified by -t or >> -T option, >> +and pgbench will print an incomplete >> results >> +in this case. >> + >> + >> + >> + >> >> What about inserting "Note that serialization failures or deadlock >> failures will not abort client. See > linkend="failures-and-retries"/> for more information." into the end >> of this paragraph? > > --exit-on-abort is related to "abort" of a client instead of error or > failure itself, so rather I wonder a bit that mentioning > serialization/deadlock > failures might be confusing. However, if users may think of such failures > from > "abort", it could be beneficial to add the sentences with some modification as > below. I myself confused by this and believe that adding extra paragraph is beneficial to users. > "Note that serialization failures or deadlock failures does not abort the > client, so they are not affected by this option. > See for more information." "does not" --> "do not". >> BTW, I think: >> Exit immediately when any client is aborted due to some error. >> Without >> >> should be: >> Exit immediately when any client is aborted due to some errors. >> Without >> >> (error vs. erros) > > Well, I chose "some" to mean "unknown or unspecified", not "an unspecified > amount > or number of", so singular form "error" is used. Ok. > Instead, should we use "due to a error"? I don't think so. >> Also: >> + --exit-on-abort is specified . Otherwise in the >> worst >> >> There is an extra space between "specified" and ".". > > Fixed. > > Also, I fixed the place of the description in the documentation > to alphabetical order > > Attached is the updated patch. Looks good to me. If there's no objection, I will commit this next week. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: pgbench: allow to exit immediately when any client is aborted
> I have tested the v4 patch with default_transaction_isolation = > 'repeatable read'. > > pgbench --exit-on-abort -N -p 11002 -c 10 -T 30 test > pgbench (17devel, server 15.3) > starting vacuum...end. > transaction type: > scaling factor: 1 > query mode: simple > number of clients: 10 > number of threads: 1 > maximum number of tries: 1 > duration: 30 s > number of transactions actually processed: 64854 > number of failed transactions: 4 (0.006%) > latency average = 4.628 ms (including failures) > initial connection time = 49.526 ms > tps = 2160.827556 (without initial connection time) > > Depite the 4 failed transactions (could not serialize access due to > concurrent update) pgbench ran normally, rather than aborting the > test. Is this an expected behavior? I start to think this behavior is ok and consistent with previous behavior of pgbench because serialization (and dealock) errors have been treated specially from other types of errors, such as accessing non existing tables. However, I suggest to add more sentences to the explanation of this option so that users are not confused by this. + + --exit-on-abort + + +Exit immediately when any client is aborted due to some error. Without +this option, even when a client is aborted, other clients could continue +their run as specified by -t or -T option, +and pgbench will print an incomplete results +in this case. + + + + What about inserting "Note that serialization failures or deadlock failures will not abort client. See for more information." into the end of this paragraph? BTW, I think: Exit immediately when any client is aborted due to some error. Without should be: Exit immediately when any client is aborted due to some errors. Without (error vs. erros) Also: + --exit-on-abort is specified . Otherwise in the worst There is an extra space between "specified" and ".". Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: pgbench: allow to exit immediately when any client is aborted
> About pgbench exit on abort v4: > > Patch applies cleanly, compiles, local make check ok, doc looks ok. > > This looks ok to me. I have tested the v4 patch with default_transaction_isolation = 'repeatable read'. pgbench --exit-on-abort -N -p 11002 -c 10 -T 30 test pgbench (17devel, server 15.3) starting vacuum...end. transaction type: scaling factor: 1 query mode: simple number of clients: 10 number of threads: 1 maximum number of tries: 1 duration: 30 s number of transactions actually processed: 64854 number of failed transactions: 4 (0.006%) latency average = 4.628 ms (including failures) initial connection time = 49.526 ms tps = 2160.827556 (without initial connection time) Depite the 4 failed transactions (could not serialize access due to concurrent update) pgbench ran normally, rather than aborting the test. Is this an expected behavior? Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Using defines for protocol characters
> On Wed, Aug 16, 2023 at 06:25:09AM +0900, Tatsuo Ishii wrote: >> Currently pqcomm.h needs c.h which is not problem for Pgpool-II. But >> what about other middleware? > > Why do you need to include directly c.h? There are definitions in > there that are not intended to be exposed. pqcomm.h has this: #define UNIXSOCK_PATH(path, port, sockdir) \ (AssertMacro(sockdir), \ AssertMacro(*(sockdir) != '\0'), \ snprintf(path, sizeof(path), "%s/.s.PGSQL.%d", \ (sockdir), (port))) AssertMacro is defined in c.h. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Using defines for protocol characters
> On Tue, Aug 15, 2023 at 02:46:24PM +0900, Tatsuo Ishii wrote: >> Is it possible to put the new define staff into protocol.h then let >> pqcomm.h include protocol.h? This makes Pgpool-II and other middle >> ware/drivers written in C easier to use the defines so that they only >> include protocol.h to use the defines. > > It is possible, of course, but are there any reasons such programs couldn't > include pqcomm.h? It looks relatively inexpensive to me. That being said, > I'm fine with the approach you describe if the folks in this thread agree. Currently pqcomm.h needs c.h which is not problem for Pgpool-II. But what about other middleware? Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Using defines for protocol characters
> I tried to address all the feedback in v5 of the patch, which is attached.
> I limited the patch to only the characters that have names in the "Message
> Formats" section of protocol.sgml instead of trying to invent names for
> unnamed characters.
>
> I'm aware of one inconsistency. While I grouped all the authentication
> request messages ('R') into PqMsg_AuthenticationRequest, I added separate
> macros for all the different meanings of 'p', i.e., GSSResponse,
> PasswordMessage, SASLInitialResponse, and SASLResponse. I wanted to list
> everything in protocol.sgml (even the duplicate ) to ease greppability, but
> the code is structure such that adding macros for all the different
> authentication messages would be kind of pointless. Plus, there is a
> separate set of authentication request codes just below the added macros.
> So, this is where I landed...
Is it possible to put the new define staff into protocol.h then let
pqcomm.h include protocol.h? This makes Pgpool-II and other middle
ware/drivers written in C easier to use the defines so that they only
include protocol.h to use the defines.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Re: pgbench with libevent?
> It could be refactored to support a different subset of event types -- > maybe just sockets, no latches and obviously no 'postmaster death'. Ok. > But figuring out how to make latches work between threads might also > be interesting for future projects... Maybe. Some people are working on threading PostgreSQL. They may already know... > Maybe Fabien has completion-based I/O in mind (not just "readiness"). > That's something that some of those libraries can do, IIUC. For > example, when your thread wakes up, it tells you "your socket read is > finished, the data is already in your target buffer". As opposed to > "you can now call recv() without blocking", so you avoid another trip > into the kernel. But that's also something we'll eventually want to > figure out in the server. Agreed. -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: pgbench with libevent?
> On Mon, Aug 14, 2023 at 12:35 PM Fabien COELHO wrote: >> > Pgbench is managing clients I/Os manually with select or poll. Much of this >> > could be managed by libevent. >> >> Or maybe libuv (used by nodejs?). >> >> From preliminary testing libevent seems not too good at fine grain time >> management which are used for throttling, whereas libuv advertised that it >> is good at it, although what it does is yet to be seen. > > Do you think our WaitEventSet stuff could be good here, if made > frontend-friendly? Interesting. In my understanding this also needs to make Latch frontend-friendly? Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
Attached is the v4 patch. Differences from previous patch include: > - PERMUTE is still misspelled as PREMUTE Fixed. > - PATTERN variables do not have to exist in the DEFINE clause. They are > - considered TRUE if not present. Fixed. Moreover new regression test case is added. - It was possible that tle nodes in DEFINE clause do not appear in the plan's target list. This makes impossible to evaluate expressions in the DEFINE because it does not appear in the outer plan's target list. To fix this, call findTargetlistEntrySQL99 (with resjunk is true) so that the missing TargetEntry is added to the outer plan later on. - I eliminated some hacks in handling the Var node in DEFINE clause. Previously I replaced varattno of Var node in a plan tree by hand so that it refers to correct varattno in the outer plan node. In this patch I modified set_upper_references so that it calls fix_upper_expr for those Var nodes in the DEFINE clause. See v4-0003 patch for more details. - I found a bug with pattern matching code. It creates a string for subsequent regular expression matching. It uses the initial letter of each define variable name. For example, if the varname is "foo", then "f" is used. Obviously this makes trouble if we have two or more variables starting with same "f" (e.g. "food"). To fix this, I assign [a-z] to each variable instead of its initial letter. However this way limits us not to have more than 26 variables. I hope 26 is enough for most use cases. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp >From 5c6430e171ab9f9a75df92fac25949cb96fe1da2 Mon Sep 17 00:00:00 2001 From: Tatsuo Ishii Date: Wed, 9 Aug 2023 16:56:17 +0900 Subject: [PATCH v4 1/7] Row pattern recognition patch for raw parser. --- src/backend/parser/gram.y | 216 +--- src/include/nodes/parsenodes.h | 56 + src/include/parser/kwlist.h | 8 ++ src/include/parser/parse_node.h | 1 + 4 files changed, 267 insertions(+), 14 deletions(-) diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 15ece871a0..62c1919538 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -251,6 +251,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); DefElem *defelt; SortBy *sortby; WindowDef *windef; + RPCommonSyntax *rpcom; + RPSubsetItem *rpsubset; JoinExpr *jexpr; IndexElem *ielem; StatsElem *selem; @@ -453,8 +455,12 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); TriggerTransitions TriggerReferencing vacuum_relation_list opt_vacuum_relation_list drop_option_list pub_obj_list - -%type opt_routine_body +row_pattern_measure_list row_pattern_definition_list +opt_row_pattern_subset_clause +row_pattern_subset_list row_pattern_subset_rhs +row_pattern +%type row_pattern_subset_item +%type opt_routine_body row_pattern_term %type group_clause %type group_by_list %type group_by_item empty_grouping_set rollup_clause cube_clause @@ -551,6 +557,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type relation_expr_opt_alias %type tablesample_clause opt_repeatable_clause %type target_el set_target insert_column_item +row_pattern_measure_item row_pattern_definition %type generic_option_name %type generic_option_arg @@ -633,6 +640,9 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type window_clause window_definition_list opt_partition_clause %type window_definition over_clause window_specification opt_frame_clause frame_extent frame_bound +%type opt_row_pattern_common_syntax opt_row_pattern_skip_to +%type opt_row_pattern_initial_or_seek +%type opt_row_pattern_measures %type opt_window_exclusion_clause %type opt_existing_window_name %type opt_if_not_exists @@ -659,7 +669,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); json_object_constructor_null_clause_opt json_array_constructor_null_clause_opt - /* * Non-keyword token types. These are hard-wired into the "flex" lexer. * They must be listed first so that their numeric codes do not depend on @@ -702,7 +711,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); CURRENT_TIME CURRENT_TIMESTAMP CURRENT_USER CURSOR CYCLE DATA_P DATABASE DAY_P DEALLOCATE DEC DECIMAL_P DECLARE DEFAULT DEFAULTS - DEFERRABLE DEFERRED DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC + DEFERRABLE DEFERRED DEFINE DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC DETACH DICTIONARY DISABLE_P DISCARD DISTINCT DO DOCUMENT_P DOMAIN_P DOUBLE_P DROP @@ -718,7 +727,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
Re: Using defines for protocol characters
> On Mon, 7 Aug 2023 at 16:50, Tatsuo Ishii wrote: > >> > On Mon, Aug 07, 2023 at 04:02:08PM -0400, Tom Lane wrote: >> >> Dave Cramer writes: >> >>> On Mon, 7 Aug 2023 at 12:59, Robert Haas >> wrote: >> >>>> PqMsgEmptyQueryResponse or something like that seems better, if we >> >>>> want to keep the current capitalization. I'm not a huge fan of the way >> >>>> we vary our capitalization conventions so much all over the code base, >> >>>> but I think we would at least do well to keep it consistent from one >> >>>> end of a certain identifier to the other. >> >> >> >>> I don't have a strong preference, but before I make the changes I'd >> like to >> >>> get consensus. >> >>> Can we vote or whatever it takes to decide on a naming pattern that is >> >>> acceptable ? >> >> >> >> I'm good with Robert's proposal above. >> > >> > +1 >> >> +1. >> >> Also we need to decide what to do with them: >> >> > #define PQMSG_REQ_PREPARED 'S' >> > #define PQMSG_REQ_PORTAL 'P' >> >> If we go "PqMsgEmptyQueryResponse", probably we should go something >> like for these? >> >> #define PqMsgReqPrepared 'S' >> #define PqMsgReqPortal 'P' >> > > I went with PqMsgPortalSubCommand and PqMsgPreparedSubCommand > > See attached patch Looks good to me. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Using defines for protocol characters
> On Mon, Aug 07, 2023 at 04:02:08PM -0400, Tom Lane wrote: >> Dave Cramer writes: >>> On Mon, 7 Aug 2023 at 12:59, Robert Haas wrote: >>>> PqMsgEmptyQueryResponse or something like that seems better, if we >>>> want to keep the current capitalization. I'm not a huge fan of the way >>>> we vary our capitalization conventions so much all over the code base, >>>> but I think we would at least do well to keep it consistent from one >>>> end of a certain identifier to the other. >> >>> I don't have a strong preference, but before I make the changes I'd like to >>> get consensus. >>> Can we vote or whatever it takes to decide on a naming pattern that is >>> acceptable ? >> >> I'm good with Robert's proposal above. > > +1 +1. Also we need to decide what to do with them: > #define PQMSG_REQ_PREPARED 'S' > #define PQMSG_REQ_PORTAL 'P' If we go "PqMsgEmptyQueryResponse", probably we should go something like for these? #define PqMsgReqPrepared 'S' #define PqMsgReqPortal 'P' Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: pgbench: allow to exit immediately when any client is aborted
> Hi, > > I would like to propose to add an option to pgbench so that benchmark > can quit immediately when any client is aborted. Currently, when a > client is aborted due to some error, for example, network trouble, > other clients continue their run until a certain number of transactions > specified -t is reached or the time specified by -T is expired. At the > end, the results are printed, but they are not useful, as the message > "Run was aborted; the above results are incomplete" shows. Sounds like a good idea. It's a waste of resources waiting for unusable benchmark results until t/T expired. If we graze on the screen, then it's easy to cancel the pgbench run. But I frequently let pgbench run without sitting in front of the screen especially when t/T is large (it's recommended that running pgbench with large enough t/T to get usable results). > For precise benchmark purpose, we would not want to wait to get such > incomplete results, rather we would like to know some trouble happened > to allow a quick retry. Therefore, it would be nice to add an option to > make pgbench exit instead of continuing run in other clients when any > client is aborted. I think adding the optional is better than whole > behavioural change because some users that use pgbench just in order > to stress on backends for testing purpose rather than benchmark might > not want to stop pgbench even a client is aborted. > > Attached is the patch to add the option --exit-on-abort. > If this option is specified, when any client is aborted, pgbench > immediately quit by calling exit(2). > > What do you think about it? I think aborting pgbench by calling exit(2) is enough. It's not worth the trouble to add more codes for this purpose. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Using defines for protocol characters
> On Thu, 3 Aug 2023 at 16:59, Tatsuo Ishii wrote: > >> > On Thu, 3 Aug 2023 at 13:25, Tatsuo Ishii wrote: >> > >> >> > Greetings, >> >> > >> >> > Attached is a patch which introduces a file protocol.h. Instead of >> using >> >> > the actual characters everywhere in the code this patch names the >> >> > characters and removes the comments beside each usage. >> >> >> >> > +#define DESCRIBE_PREPARED 'S' >> >> > +#define DESCRIBE_PORTAL 'P' >> >> >> >> You use these for Close message as well. I don't like the idea because >> >> Close is different message from Describe message. >> >> >> >> What about adding following for Close too use them instead? >> >> >> >> #define CLOSE_PREPARED 'S' >> >> #define CLOSE_PORTAL 'P' >> >> >> > >> > Good catch. >> > I recall when writing this it was a bit hacky. >> > What do you think of PREPARED_SUB_COMMAND and PORTAL_SUB_COMMAND >> instead >> > of duplicating them ? >> >> Nice. Looks good to me. >> > New patch attached which uses PREPARED_SUB_COMMAND and > PORTAL_SUB_COMMAND instead > and uses > PQMSG_REQ_* and PQMSG_RESP_* as per Alvaro's suggestion Looks good to me. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Using defines for protocol characters
> On Thu, 3 Aug 2023 at 13:25, Tatsuo Ishii wrote: > >> > Greetings, >> > >> > Attached is a patch which introduces a file protocol.h. Instead of using >> > the actual characters everywhere in the code this patch names the >> > characters and removes the comments beside each usage. >> >> > +#define DESCRIBE_PREPARED 'S' >> > +#define DESCRIBE_PORTAL 'P' >> >> You use these for Close message as well. I don't like the idea because >> Close is different message from Describe message. >> >> What about adding following for Close too use them instead? >> >> #define CLOSE_PREPARED 'S' >> #define CLOSE_PORTAL 'P' >> > > Good catch. > I recall when writing this it was a bit hacky. > What do you think of PREPARED_SUB_COMMAND and PORTAL_SUB_COMMAND instead > of duplicating them ? Nice. Looks good to me. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Using defines for protocol characters
> Greetings, > > Attached is a patch which introduces a file protocol.h. Instead of using > the actual characters everywhere in the code this patch names the > characters and removes the comments beside each usage. > +#define DESCRIBE_PREPARED 'S' > +#define DESCRIBE_PORTAL 'P' You use these for Close message as well. I don't like the idea because Close is different message from Describe message. What about adding following for Close too use them instead? #define CLOSE_PREPARED 'S' #define CLOSE_PORTAL 'P' Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
>>> - PATTERN variables do not have to exist in the DEFINE clause. They are
>>> - considered TRUE if not present.
>> Do you think we really need this? I found a criticism regarding this.
>> https://link.springer.com/article/10.1007/s13222-022-00404-3
>> "3.2 Explicit Definition of All Row Pattern Variables"
>> What do you think?
>
> I think that a large part of obeying the standard is to allow queries
> from other engines to run the same on ours. The standard does not
> require the pattern variables to be defined and so there are certainly
> queries out there without them, and that hurts migrating to
> PostgreSQL.
Yeah, migration is good point. I agree we should have the feature.
>>> When we get to adding count in the MEASURES clause, there will be a
>>> difference between no match and empty match, but that does not apply
>>> here.
>> Can you elaborate more? I understand that "no match" and "empty match"
>> are different things. But I do not understand how the difference
>> affects the result of count.
>
> This query:
>
> SELECT v.a, wcnt OVER w, count(*) OVER w
> FROM (VALUES ('A')) AS v (a)
> WINDOW w AS (
> ORDER BY v.a
> MEASURES count(*) AS wcnt
> ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
> PATTERN (B)
> DEFINE B AS B.a = 'B'
> )
>
> produces this result:
>
> a | wcnt | count
> ---+--+---
> A | | 0
> (1 row)
>
> Inside the window specification, *no match* was found and so all of
> the MEASURES are null. The count(*) in the target list however, still
> exists and operates over zero rows.
>
> This very similar query:
>
> SELECT v.a, wcnt OVER w, count(*) OVER w
> FROM (VALUES ('A')) AS v (a)
> WINDOW w AS (
> ORDER BY v.a
> MEASURES count(*) AS wcnt
> ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
> PATTERN (B?)
> DEFINE B AS B.a = 'B'
> )
>
> produces this result:
>
> a | wcnt | count
> ---+------+---
> A |0 | 0
> (1 row)
>
> In this case, the pattern is B? instead of just B, which produces an
> *empty match* for the MEASURES to be applied over.
Thank you for the detailed explanation. I think I understand now.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
>> Attached is the v3 patch. In this patch following changes are made. > > Excellent. Thanks! You are welcome! > A few quick comments: > > - PERMUTE is still misspelled as PREMUTE Oops. Will fix. > - PATTERN variables do not have to exist in the DEFINE clause. They are > - considered TRUE if not present. Do you think we really need this? I found a criticism regarding this. https://link.springer.com/article/10.1007/s13222-022-00404-3 "3.2 Explicit Definition of All Row Pattern Variables" What do you think? >> - I am working on making window aggregates RPR aware now. The >>implementation is in progress and far from completeness. An example >>is below. I think row 2, 3, 4 of "count" column should be NULL >>instead of 3, 2, 0, 0. Same thing can be said to other >>rows. Probably this is an effect of moving aggregate but I still >>studying the window aggregation code. > > This tells me again that RPR is not being run in the right place. All > windowed aggregates and frame-level window functions should Just Work > with no modification. I am not touching each aggregate function. I am modifying eval_windowaggregates() in nodeWindowAgg.c, which calls each aggregate function. Do you think it's not the right place to make window aggregates RPR aware? >> SELECT company, tdate, first_value(price) OVER W, count(*) 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 (START UP+ DOWN+) >> DEFINE >>START AS TRUE, >>UP AS price > PREV(price), >>DOWN AS price < PREV(price) >> ); >> company | tdate| first_value | count >> --++-+--- >> company1 | 2023-07-01 | 100 | 4 >> company1 | 2023-07-02 | | 3 >> company1 | 2023-07-03 | | 2 >> company1 | 2023-07-04 | | 0 >> company1 | 2023-07-05 | | 0 >> company1 | 2023-07-06 | 90 | 4 >> company1 | 2023-07-07 | | 3 >> company1 | 2023-07-08 | | 2 >> company1 | 2023-07-09 | | 0 >> company1 | 2023-07-10 | | 0 >> company2 | 2023-07-01 | 50 | 4 >> company2 | 2023-07-02 | | 3 >> company2 | 2023-07-03 | | 2 >> company2 | 2023-07-04 | | 0 >> company2 | 2023-07-05 | | 0 >> company2 | 2023-07-06 | 60 | 4 >> company2 | 2023-07-07 | | 3 >> company2 | 2023-07-08 | | 2 >> company2 | 2023-07-09 | | 0 >> company2 | 2023-07-10 | | 0 > > In this scenario, row 1's frame is the first 5 rows and specified SKIP > PAST LAST ROW, so rows 2-5 don't have *any* frame (because they are > skipped) and the result of the outer count should be 0 for all of them > because there are no rows in the frame. Ok. Just I want to make sure. If it's other aggregates like sum or avg, the result of the outer aggregates should be NULL. > When we get to adding count in the MEASURES clause, there will be a > difference between no match and empty match, but that does not apply > here. Can you elaborate more? I understand that "no match" and "empty match" are different things. But I do not understand how the difference affects the result of count. >> I am going to add this thread to CommitFest and plan to add both of >> you as reviewers. Thanks in advance. > > My pleasure. Thank you for working on this difficult feature. Thank you for accepting being registered as a reviewer. Your comments are really helpful. -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
> I am going to add this thread to CommitFest and plan to add both of > you as reviewers. Thanks in advance. Done. https://commitfest.postgresql.org/44/4460/ Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
ce) ); ERROR: attribute number 3 exceeds number of columns 2 This is because attributes appearing in DEFINE are not added to the target list. I am looking for way to teach planner to add attributes appearing in DEFINE. I am going to add this thread to CommitFest and plan to add both of you as reviewers. Thanks in advance. -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp >From 16cab4e23f38b447a814773fea83a74c337a08d5 Mon Sep 17 00:00:00 2001 From: Tatsuo Ishii Date: Wed, 26 Jul 2023 19:49:09 +0900 Subject: [PATCH v3 1/7] Row pattern recognition patch for raw parser. --- src/backend/parser/gram.y | 218 +--- src/include/nodes/parsenodes.h | 54 src/include/parser/kwlist.h | 8 ++ src/include/parser/parse_node.h | 1 + 4 files changed, 266 insertions(+), 15 deletions(-) diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 856d5dee0e..a4c97c28ff 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -251,6 +251,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); DefElem *defelt; SortBy *sortby; WindowDef *windef; + RPCommonSyntax *rpcom; + RPSubsetItem *rpsubset; JoinExpr *jexpr; IndexElem *ielem; StatsElem *selem; @@ -453,8 +455,12 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); TriggerTransitions TriggerReferencing vacuum_relation_list opt_vacuum_relation_list drop_option_list pub_obj_list - -%type opt_routine_body +row_pattern_measure_list row_pattern_definition_list +opt_row_pattern_subset_clause +row_pattern_subset_list row_pattern_subset_rhs +row_pattern +%type row_pattern_subset_item +%type opt_routine_body row_pattern_term %type group_clause %type group_by_list %type group_by_item empty_grouping_set rollup_clause cube_clause @@ -551,6 +557,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type relation_expr_opt_alias %type tablesample_clause opt_repeatable_clause %type target_el set_target insert_column_item +row_pattern_measure_item row_pattern_definition %type generic_option_name %type generic_option_arg @@ -633,6 +640,9 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type window_clause window_definition_list opt_partition_clause %type window_definition over_clause window_specification opt_frame_clause frame_extent frame_bound +%type opt_row_pattern_common_syntax opt_row_pattern_skip_to +%type opt_row_pattern_initial_or_seek +%type opt_row_pattern_measures %type opt_window_exclusion_clause %type opt_existing_window_name %type opt_if_not_exists @@ -659,7 +669,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); json_object_constructor_null_clause_opt json_array_constructor_null_clause_opt - /* * Non-keyword token types. These are hard-wired into the "flex" lexer. * They must be listed first so that their numeric codes do not depend on @@ -702,7 +711,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); CURRENT_TIME CURRENT_TIMESTAMP CURRENT_USER CURSOR CYCLE DATA_P DATABASE DAY_P DEALLOCATE DEC DECIMAL_P DECLARE DEFAULT DEFAULTS - DEFERRABLE DEFERRED DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC + DEFERRABLE DEFERRED DEFINE DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC DETACH DICTIONARY DISABLE_P DISCARD DISTINCT DO DOCUMENT_P DOMAIN_P DOUBLE_P DROP @@ -718,7 +727,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); HANDLER HAVING HEADER_P HOLD HOUR_P IDENTITY_P IF_P ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE - INCLUDING INCREMENT INDENT INDEX INDEXES INHERIT INHERITS INITIALLY INLINE_P + INCLUDING INCREMENT INDENT INDEX INDEXES INHERIT INHERITS INITIAL INITIALLY INLINE_P INNER_P INOUT INPUT_P INSENSITIVE INSERT INSTEAD INT_P INTEGER INTERSECT INTERVAL INTO INVOKER IS ISNULL ISOLATION @@ -731,7 +740,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); LEADING LEAKPROOF LEAST LEFT LEVEL LIKE LIMIT LISTEN LOAD LOCAL LOCALTIME LOCALTIMESTAMP LOCATION LOCK_P LOCKED LOGGED - MAPPING MATCH MATCHED MATERIALIZED MAXVALUE MERGE METHOD + MAPPING MATCH MATCHED MATERIALIZED MAXVALUE MEASURES MERGE METHOD MINUTE_P MINVALUE MODE MONTH_P MOVE NAME_P NAMES NATIONAL NATURAL NCHAR NEW NEXT NFC NFD NFKC NFKD NO NONE @@ -743,9 +752,9 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); ORDER ORDINALITY OTHERS OUT_P OUTER_P OVER OVERLAPS OVERLAY OVERRIDING OWNED OWNER - PARALLEL PARAMETER PARSER PARTIAL PARTITION PASSING PASSWORD - PLACING PLANS POLICY - POSITION PRECEDING PRECISION PRESERVE PREPARE PREPARED PRIMARY + PARALLEL PARAMETER PARSER PARTIAL PARTITI
Re: Row pattern recognition
Hi, > diff --git a/src/test/regress/expected/rpr.out > b/src/test/regress/expected/rpr.out > index 6bf8818911..f3fd22de2a 100644 > --- a/src/test/regress/expected/rpr.out > +++ b/src/test/regress/expected/rpr.out > @@ -230,6 +230,79 @@ SELECT company, tdate, price, rpr(price) OVER w FROM > stock > company2 | 07-10-2023 | 1300 | > (20 rows) > > +-- match everything > +SELECT company, tdate, price, rpr(price) OVER w FROM stock > + WINDOW w AS ( > + PARTITION BY company > + ORDER BY tdate > + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING > + AFTER MATCH SKIP TO NEXT ROW It seems it's a result with AFTER MATCH SKIP PAST LAST ROW. > + INITIAL > + PATTERN (A+) > + DEFINE > + A AS TRUE > +); > + company | tdate| price | rpr > +--++---+- > + company1 | 07-01-2023 | 100 | 100 > + company1 | 07-02-2023 | 200 | > + company1 | 07-03-2023 | 150 | > + company1 | 07-04-2023 | 140 | > + company1 | 07-05-2023 | 150 | > + company1 | 07-06-2023 |90 | > + company1 | 07-07-2023 | 110 | > + company1 | 07-08-2023 | 130 | > + company1 | 07-09-2023 | 120 | > + company1 | 07-10-2023 | 130 | > + company2 | 07-01-2023 |50 | 50 > + company2 | 07-02-2023 | 2000 | > + company2 | 07-03-2023 | 1500 | > + company2 | 07-04-2023 | 1400 | > + company2 | 07-05-2023 | 1500 | > + company2 | 07-06-2023 |60 | > + company2 | 07-07-2023 | 1100 | > + company2 | 07-08-2023 | 1300 | > + company2 | 07-09-2023 | 1200 | > + company2 | 07-10-2023 | 1300 | > +(20 rows) > + > +-- backtracking with reclassification of rows > +SELECT company, tdate, price, rpr(price) OVER w FROM stock > + WINDOW w AS ( > + PARTITION BY company > + ORDER BY tdate > + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING > + AFTER MATCH SKIP TO NEXT ROW > + INITIAL > + PATTERN (A+ B+) > + DEFINE > + A AS price > 100, > + B AS price > 100 > +); > + company | tdate| price | rpr > +--++---+-- > + company1 | 07-01-2023 | 100 | > + company1 | 07-02-2023 | 200 | 200 > + company1 | 07-03-2023 | 150 | > + company1 | 07-04-2023 | 140 | > + company1 | 07-05-2023 | 150 | > + company1 | 07-06-2023 |90 | > + company1 | 07-07-2023 | 110 | 110 > + company1 | 07-08-2023 | 130 | > + company1 | 07-09-2023 | 120 | > + company1 | 07-10-2023 | 130 | > + company2 | 07-01-2023 |50 | > + company2 | 07-02-2023 | 2000 | 2000 > + company2 | 07-03-2023 | 1500 | > + company2 | 07-04-2023 | 1400 | > + company2 | 07-05-2023 | 1500 | > + company2 | 07-06-2023 |60 | > + company2 | 07-07-2023 | 1100 | 1100 > + company2 | 07-08-2023 | 1300 | > + company2 | 07-09-2023 | 1200 | > + company2 | 07-10-2023 | 1300 | > +(20 rows) > + > -- > -- Error cases > -- > diff --git a/src/test/regress/sql/rpr.sql b/src/test/regress/sql/rpr.sql > index 951c9abfe9..f1cd0369f4 100644 > --- a/src/test/regress/sql/rpr.sql > +++ b/src/test/regress/sql/rpr.sql > @@ -94,6 +94,33 @@ SELECT company, tdate, price, rpr(price) OVER w FROM stock >UPDOWN AS price > PREV(price) AND price > NEXT(price) > ); > > +-- match everything > +SELECT company, tdate, price, rpr(price) OVER w FROM stock > + WINDOW w AS ( > + PARTITION BY company > + ORDER BY tdate > + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING > + AFTER MATCH SKIP TO NEXT ROW > + INITIAL > + PATTERN (A+) > + DEFINE > + A AS TRUE > +); > + > +-- backtracking with reclassification of rows > +SELECT company, tdate, price, rpr(price) OVER w FROM stock > + WINDOW w AS ( > + PARTITION BY company > + ORDER BY tdate > + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING > + AFTER MATCH SKIP TO NEXT ROW > + INITIAL > + PATTERN (A+ B+) > + DEFINE > + A AS price > 100, > + B AS price > 100 > +); > + > -- > -- Error cases > --
Re: Row pattern recognition
>>> What we are talking about here is *defining* a window >>> frame. >> Well, we are defining a "reduced" window frame within a (full) window >> frame. A "reduced" window frame is calculated each time when a window >> function is called. > > > Why? It should only be recalculated when the current row changes and > we need a new frame. The reduced window frame *is* the window frame > for all functions over that window. We already recalculate a frame each time a row is processed even without RPR. See ExecWindowAgg. Also RPR always requires a frame option ROWS BETWEEN CURRENT ROW, which means the frame head is changed each time current row position changes. > I strongly disagree with this. Window function do not need to know > how the frame is defined, and indeed they should not. We already break the rule by defining *support functions. See windowfuncs.c. > WinGetFuncArgInFrame should answer yes or no and the window function > just works on that. Otherwise we will get extension (and possibly even > core) functions that don't handle the frame properly. Maybe I can move row_is_in_reduced_frame into WinGetFuncArgInFrame just for convenience. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
> On 7/22/23 03:11, Tatsuo Ishii wrote:
>>>>> Maybe
>>>>> that can help clarify the design? It feels like it'll need to
>>>>> eventually
>>>>> be a "real" function that operates on the window state, even if it
>>>>> doesn't support all of the possible complexities in v1.
>>>> Unfortunately an window function can not call other window functions.
>>> Can that restriction be lifted for the EXPR_KIND_RPR_DEFINE case?
>
>> I am not sure at this point. Current PostgreSQL executor creates
>> WindowStatePerFuncData for each window function and aggregate
>> appearing in OVER clause. This means PREV/NEXT and other row pattern
>> navigation operators cannot have their own WindowStatePerFuncData if
>> they do not appear in OVER clauses in a query even if PREV/NEXT
>> etc. are defined as window function.
>>
>>> Or
>>> does it make sense to split the pattern navigation "functions" into
>>> their own new concept, and maybe borrow some of the window function
>>> infrastructure for it?
>
>> Maybe. Suppose a window function executes row pattern matching using
>> price > PREV(price). The window function already receives
>> WindowStatePerFuncData. If we can pass the WindowStatePerFuncData to
>> PREV, we could let PREV do the real work (getting previous tuple).
>> I have not tried this yet, though.
>
>
> I don't understand this logic. Window functions work over a window
> frame.
Yes.
> What we are talking about here is *defining* a window
> frame.
Well, we are defining a "reduced" window frame within a (full) window
frame. A "reduced" window frame is calculated each time when a window
function is called.
> How can a window function execute row pattern matching?
A window function is called for each row fed by an outer plan. It
fetches current, previous and next row to execute pattern matching. If
it matches, the window function moves to next row and repeat the
process, until pattern match fails.
Below is an example window function to execute pattern matching (I
will include this in the v3 patch). row_is_in_reduced_frame() is a
function to execute pattern matching. It returns the number of rows in
the reduced frame if pattern match succeeds. If succeeds, the function
returns the last row in the reduced frame instead of the last row in
the full window frame.
/*
* last_value
* return the value of VE evaluated on the last row of the
* window frame, per spec.
*/
Datum
window_last_value(PG_FUNCTION_ARGS)
{
WindowObject winobj = PG_WINDOW_OBJECT();
Datum result;
boolisnull;
int64 abspos;
int num_reduced_frame;
abspos = WinGetCurrentPosition(winobj);
num_reduced_frame = row_is_in_reduced_frame(winobj, abspos);
if (num_reduced_frame == 0)
/* no RPR is involved */
result = WinGetFuncArgInFrame(winobj, 0,
0,
WINDOW_SEEK_TAIL, true,
, NULL);
else if (num_reduced_frame > 0)
/* get last row value in the reduced frame */
result = WinGetFuncArgInFrame(winobj, 0,
num_reduced_frame - 1, WINDOW_SEEK_HEAD, true,
, NULL);
else
/* RPR is involved and current row is unmatched or skipped */
isnull = true;
if (isnull)
PG_RETURN_NULL();
PG_RETURN_DATUM(result);
}
Re: Row pattern recognition
>> One small concern is how to convert pattern variables to regex
>> expression which our regex enginer understands. Suppose,
>>
>> PATTERN UP+
>>
>> Currently I convert "UP+" to "U+" so that it can be fed to the regexp
>> engine. In order to do that, we need to know which part of the pattern
>> (UP+) is the pattern variable ("UP"). For "UP+" it's quite easy. But
>> for more complex regular expressions it would be not, unless PATTERN
>> grammer can be analyzed by our parser to know which part is the
>> pattern variable.
>
> Is the eventual plan to generate multiple alternatives, and run the
> regex against them one at a time?
Yes, that's my plan.
>>> I'm not familiar enough with this code yet to offer very concrete
>>> suggestions, sorry... But at some point in the future, we need to be
>>> able to skip forward and backward from arbitrary points, like
>>>
>>> DEFINE B AS B.price > PREV(FIRST(A.price), 3)
>>>
>>> so there won't be just one pair of "previous and next" tuples.
>>
>> Yes, I know.
>
> I apologize. I got overexplain-y.
No problem. Thank you for reminding me it.
>>> Maybe
>>> that can help clarify the design? It feels like it'll need to eventually
>>> be a "real" function that operates on the window state, even if it
>>> doesn't support all of the possible complexities in v1.
>>
>> Unfortunately an window function can not call other window functions.
>
> Can that restriction be lifted for the EXPR_KIND_RPR_DEFINE case?
I am not sure at this point. Current PostgreSQL executor creates
WindowStatePerFuncData for each window function and aggregate
appearing in OVER clause. This means PREV/NEXT and other row pattern
navigation operators cannot have their own WindowStatePerFuncData if
they do not appear in OVER clauses in a query even if PREV/NEXT
etc. are defined as window function.
> Or
> does it make sense to split the pattern navigation "functions" into
> their own new concept, and maybe borrow some of the window function
> infrastructure for it?
Maybe. Suppose a window function executes row pattern matching using
price > PREV(price). The window function already receives
WindowStatePerFuncData. If we can pass the WindowStatePerFuncData to
PREV, we could let PREV do the real work (getting previous tuple).
I have not tried this yet, though.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
Hi,
> Hi Ishii-san,
>
> On 7/19/23 22:15, Tatsuo Ishii wrote:
>> Currently my patch has a limitation for the sake of simple
>> implementation: a pattern like "A+" is parsed and analyzed in the raw
>> parser. This makes subsequent process much easier because the pattern
>> element variable (in this case "A") and the quantifier (in this case
>> "+") is already identified by the raw parser. However there are much
>> more cases are allowed in the standard as you already pointed out. For
>> those cases probably we should give up to parse PATTERN items in the
>> raw parser, and instead the raw parser just accepts the elements as
>> Sconst?
>
> Is there a concern that the PATTERN grammar can't be represented in
> Bison? I thought it was all context-free...
I don't know at this point. I think context-free is not enough to be
repsented in Bison. The grammer also needs to be LALR(1). Moreover,
adding the grammer to existing parser may generate shift/reduce
errors.
> Or is the concern that the
> parse tree of the pattern is hard to feed into a regex engine?
One small concern is how to convert pattern variables to regex
expression which our regex enginer understands. Suppose,
PATTERN UP+
Currently I convert "UP+" to "U+" so that it can be fed to the regexp
engine. In order to do that, we need to know which part of the pattern
(UP+) is the pattern variable ("UP"). For "UP+" it's quite easy. But
for more complex regular expressions it would be not, unless PATTERN
grammer can be analyzed by our parser to know which part is the
pattern variable.
>> Any comments, especially on the PREV/NEXT implementation part is
>> welcome. Currently the DEFINE expression like "price > PREV(price)" is
>> prepared in ExecInitWindowAgg using ExecInitExpr,tweaking var->varno
>> in Var node so that PREV uses OUTER_VAR, NEXT uses INNER_VAR. Then
>> evaluate the expression in ExecWindowAgg using ExecEvalExpr, setting
>> previous row TupleSlot to ExprContext->ecxt_outertuple, and next row
>> TupleSlot to ExprContext->ecxt_innertuple. I think this is temporary
>> hack and should be gotten ride of before v1 is committed. Better idea?
>
> I'm not familiar enough with this code yet to offer very concrete
> suggestions, sorry... But at some point in the future, we need to be
> able to skip forward and backward from arbitrary points, like
>
> DEFINE B AS B.price > PREV(FIRST(A.price), 3)
>
> so there won't be just one pair of "previous and next" tuples.
Yes, I know.
> Maybe
> that can help clarify the design? It feels like it'll need to eventually
> be a "real" function that operates on the window state, even if it
> doesn't support all of the possible complexities in v1.
Unfortunately an window function can not call other window functions.
> Taking a closer look at the regex engine:
>
> It looks like the + qualifier has trouble when it meets the end of the
> frame. For instance, try removing the last row of the 'stock' table in
> rpr.sql; some of the final matches will disappear unexpectedly. Or try a
> pattern like
>
> PATTERN ( a+ )
> DEFINE a AS TRUE
>
> which doesn't seem to match anything in my testing.
>
> There's also the issue of backtracking in the face of reclassification,
> as I think Vik was alluding to upthread. The pattern
>
> PATTERN ( a+ b+ )
> DEFINE a AS col = 2,
> b AS col = 2
>
> doesn't match a sequence of values (2 2 ...) with the current
> implementation, even with a dummy row at the end to avoid the
> end-of-frame bug.
>
> (I've attached two failing tests against v2, to hopefully better
> illustrate, along with what I _think_ should be the correct results.)
Thanks. I will look into this.
> I'm not quite understanding the match loop in evaluate_pattern(). It
> looks like we're building up a string to pass to the regex engine, but
> by the we call regexp_instr, don't we already know whether or not the
> pattern will match based on the expression evaluation we've done?
For "+" yes. But for more complex regular expression like '{n}', we
need to call our regexp engine to check if the pattern matches.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
> Hello,
>
> Thanks for working on this! We're interested in RPR as well, and I've
> been trying to get up to speed with the specs, to maybe make myself
> useful.
Thank you for being interested in this.
> 19075-5 discusses that, at least; not sure about other parts of the spec.
Thanks for the info. Unfortunately I don't have 19075-5 though.
>> Maybe an insane idea but what about rewriting MATCH_RECOGNIZE clause
>> into Window clause with RPR?
>
> Are we guaranteed to always have an equivalent window clause? There seem
> to be many differences between the two, especially when it comes to ONE
> ROW/ALL ROWS PER MATCH.
You are right. I am not 100% sure if the rewriting is possible at this
point.
> To add onto what Vik said above:
>
>>> It seems RPR in the standard is quite complex. I think we can start
>>> with a small subset of RPR then we could gradually enhance the
>>> implementation.
>>
>> I have no problem with that as long as we don't paint ourselves into a
>> corner.
>
> To me, PATTERN looks like an area where we may want to support a broader
> set of operations in the first version.
Me too but...
> The spec has a bunch of
> discussion around cases like empty matches, match order of alternation
> and permutation, etc., which are not possible to express or test with
> only the + quantifier. Those might be harder to get right in a v2, if we
> don't at least keep them in mind for v1?
Currently my patch has a limitation for the sake of simple
implementation: a pattern like "A+" is parsed and analyzed in the raw
parser. This makes subsequent process much easier because the pattern
element variable (in this case "A") and the quantifier (in this case
"+") is already identified by the raw parser. However there are much
more cases are allowed in the standard as you already pointed out. For
those cases probably we should give up to parse PATTERN items in the
raw parser, and instead the raw parser just accepts the elements as
Sconst?
>> +static List *
>> +transformPatternClause(ParseState *pstate, WindowClause *wc, WindowDef
>> *windef)
>> +{
>> + List*patterns;
>
> My compiler complains about the `patterns` variable here, which is
> returned without ever being initialized. (The caller doesn't seem to use
> it.)
Will fix.
>> +-- basic test using PREV
>> +SELECT company, tdate, price, rpr(price) OVER w FROM stock
>> + WINDOW w AS (
>> + PARTITION BY company
>> + ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
>> + INITIAL
>> + PATTERN (START UP+ DOWN+)
>> + DEFINE
>> + START AS TRUE,
>> + UP AS price > PREV(price),
>> + DOWN AS price < PREV(price)
>> +);
>
> nitpick: IMO the tests should be making use of ORDER BY in the window
> clauses.
Right. Will fix.
> This is a very big feature. I agree with you that MEASURES seems like a
> very important "next piece" to add. Are there other areas where you
> would like reviewers to focus on right now (or avoid)?
Any comments, especially on the PREV/NEXT implementation part is
welcome. Currently the DEFINE expression like "price > PREV(price)" is
prepared in ExecInitWindowAgg using ExecInitExpr,tweaking var->varno
in Var node so that PREV uses OUTER_VAR, NEXT uses INNER_VAR. Then
evaluate the expression in ExecWindowAgg using ExecEvalExpr, setting
previous row TupleSlot to ExprContext->ecxt_outertuple, and next row
TupleSlot to ExprContext->ecxt_innertuple. I think this is temporary
hack and should be gotten ride of before v1 is committed. Better idea?
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
Small question. > This query (with all the defaults made explicit): > > SELECT s.company, s.tdate, s.price, >FIRST_VALUE(s.tdate) OVER w, >LAST_VALUE(s.tdate) OVER w, >lowest OVER w > FROM stock AS s > WINDOW w AS ( > PARTITION BY s.company > ORDER BY s.tdate > ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING > EXCLUDE NO OTHERS > MEASURES > LAST(DOWN) AS lowest > AFTER MATCH SKIP PAST LAST ROW > INITIAL PATTERN (START DOWN+ UP+) > DEFINE > START AS TRUE, > UP AS price > PREV(price), > DOWN AS price < PREV(price) > ); > LAST(DOWN) AS lowest should be "LAST(DOWN.price) AS lowest"? Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
> Okay, I see the problem now, and why you need the rpr() function. > > You are doing this as something that happens over a window frame, but > it is actually something that *reduces* the window frame. The pattern > matching needs to be done when the frame is calculated and not when > any particular function is applied over it. Yes. (I think the standard calls the window frame as "full window frame" in context of RPR to make a contrast with the subset of the frame rows restricted by RPR. The paper I refered to as [2] claims that the latter window frame is called "reduced window frame" in the standard but I wasn't able to find the term in the standard.) I wanted to demonstate that pattern matching logic is basically correct in the PoC patch. Now what I need to do is, move the row pattern matching logic to somewhere inside nodeWindowAgg so that "restricted window frame" can be applied to all window functions and window aggregates. Currently I am looking into update_frameheadpos() and update_frametailpos() which calculate the frame head and tail against current row. What do you think? > This query (with all the defaults made explicit): > > SELECT s.company, s.tdate, s.price, >FIRST_VALUE(s.tdate) OVER w, >LAST_VALUE(s.tdate) OVER w, >lowest OVER w > FROM stock AS s > WINDOW w AS ( > PARTITION BY s.company > ORDER BY s.tdate > ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING > EXCLUDE NO OTHERS > MEASURES > LAST(DOWN) AS lowest > AFTER MATCH SKIP PAST LAST ROW > INITIAL PATTERN (START DOWN+ UP+) > DEFINE > START AS TRUE, > UP AS price > PREV(price), > DOWN AS price < PREV(price) > ); > > Should produce this result: [snip] Thanks for the examples. I agree with the expected query results. >>>> o SUBSET is not supported >>> >>> Is this because you haven't done it yet, or because you ran into >>> problems trying to do it? >> Because it seems SUBSET is not useful without MEASURES support. Thus >> my plan is, firstly implement MEASURES, then SUBSET. What do you >> think? > > > SUBSET elements can be used in DEFINE clauses, but I do not think this > is important compared to other features. Ok. >>> I have not looked at the patch yet, but is the reason for doing R020 >>> before R010 because you haven't done the MEASURES clause yet? >> One of the reasons is, implementing MATCH_RECOGNIZE (R010) looked >> harder for me because modifying main SELECT clause could be a hard >> work. Another reason is, I had no idea how to implement PREV/NEXT in >> other than in WINDOW clause. Other people might feel differently >> though. > > > I think we could do this with a single tuplesort if we use > backtracking (which might be really slow for some patterns). I have > not looked into it in any detail. > > We would need to be able to remove tuples from the end (even if only > logically), and be able to update tuples inside the store. Both of > those needs come from backtracking and possibly changing the > classifier. > > Without backtracking, I don't see how we could do it without have a > separate tuplestore for every current possible match. Maybe an insane idea but what about rewriting MATCH_RECOGNIZE clause into Window clause with RPR? > I looked at your v2 patches a little bit and the only comment that I > currently have on the code is you spelled PERMUTE as > PREMUTE. Everything else is hopefully explained above. Thanks. Will fix. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Row pattern recognition
>> In this case, we should require the user to specify AFTER MATCH SKIP >> TO NEXT ROW so that behavior doesn't change when we implement the >> standard default. (Your patch might do this already.) > > Agreed. I will implement AFTER MATCH SKIP PAST LAST ROW in the next > patch and I will change the default to AFTER MATCH SKIP PAST LAST ROW. Attached is the v2 patch to add support for AFTER MATCH SKIP PAST LAST ROW and AFTER MATCH SKIP PAST LAST ROW. The default is AFTER MATCH SKIP PAST LAST ROW as the standard default. Here are some examples to demonstrate how those clauses affect the query result. SELECT i, rpr(i) OVER w FROM (VALUES (1), (2), (3), (4)) AS v (i) WINDOW w AS ( ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING AFTER MATCH SKIP PAST LAST ROW PATTERN (A B) DEFINE A AS i <= 2, B AS i <= 3 ); i | rpr ---+- 1 | 1 2 | 3 | 4 | (4 rows) In this example rpr starts from i = 1 and find that row i = 1 satisfies A, and row i = 2 satisfies B. Then rpr moves to row i = 3 and find that it does not satisfy A, thus the result is NULL. Same thing can be said to row i = 4. SELECT i, rpr(i) OVER w FROM (VALUES (1), (2), (3), (4)) AS v (i) WINDOW w AS ( ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING AFTER MATCH SKIP TO NEXT ROW PATTERN (A B) DEFINE A AS i <= 2, B AS i <= 3 ); i | rpr ---+- 1 | 1 2 | 2 3 | 4 | (4 rows) In this example rpr starts from i = 1 and find that row i = 1 satisfies A, and row i = 2 satisfies B (same as above). Then rpr moves to row i = 2, rather than 3 because AFTER MATCH SKIP TO NEXT ROW is specified. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp >From 92aebdb8a8c6d3739ee49b755f281e77c34f5d36 Mon Sep 17 00:00:00 2001 From: Tatsuo Ishii Date: Mon, 26 Jun 2023 17:05:35 +0900 Subject: [PATCH v2 1/7] Row pattern recognition patch for raw parser. --- src/backend/parser/gram.y | 218 +--- src/include/nodes/parsenodes.h | 54 src/include/parser/kwlist.h | 8 ++ src/include/parser/parse_node.h | 1 + 4 files changed, 266 insertions(+), 15 deletions(-) diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 39ab7eac0d..5cd3ebaa98 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -251,6 +251,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); DefElem *defelt; SortBy *sortby; WindowDef *windef; + RPCommonSyntax *rpcom; + RPSubsetItem *rpsubset; JoinExpr *jexpr; IndexElem *ielem; StatsElem *selem; @@ -453,8 +455,12 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); TriggerTransitions TriggerReferencing vacuum_relation_list opt_vacuum_relation_list drop_option_list pub_obj_list - -%type opt_routine_body +row_pattern_measure_list row_pattern_definition_list +opt_row_pattern_subset_clause +row_pattern_subset_list row_pattern_subset_rhs +row_pattern +%type row_pattern_subset_item +%type opt_routine_body row_pattern_term %type group_clause %type group_by_list %type group_by_item empty_grouping_set rollup_clause cube_clause @@ -551,6 +557,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type relation_expr_opt_alias %type tablesample_clause opt_repeatable_clause %type target_el set_target insert_column_item +row_pattern_measure_item row_pattern_definition %type generic_option_name %type generic_option_arg @@ -633,6 +640,9 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type window_clause window_definition_list opt_partition_clause %type window_definition over_clause window_specification opt_frame_clause frame_extent frame_bound +%type opt_row_pattern_common_syntax opt_row_pattern_skip_to +%type opt_row_pattern_initial_or_seek +%type opt_row_pattern_measures %type opt_window_exclusion_clause %type opt_existing_window_name %type opt_if_not_exists @@ -645,7 +655,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); %type hash_partbound %type hash_partbound_elem - %type json_format_clause_opt json_value_expr json_output_clause_opt @@ -705,7 +714,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); CURRENT_TIME CURRENT_TIMESTAMP CURRENT_USER CURSOR CYCLE DATA_P DATABASE DAY_P DEALLOCATE DEC DECIMAL_P DECLARE DEFAULT DEFAULTS - DEFERRABLE DEFERRED DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC + DEFERRABLE DEFERRED DEFINE DEFINER DELETE_P DELIMITER DELIMITERS DEPENDS DEPTH DESC DETACH DICTIONARY DISABLE_P DISCARD DISTINCT DO DOCUMENT_P DOMAIN_P DOUBLE_P DROP @@ -721,7 +730,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *que
Re: Row pattern recognition
| 3 company1 | 2023-07-09 | 120 | | 2 company1 | 2023-07-10 | 130 | | 1 >> It seems RPR in the standard is quite complex. I think we can start >> with a small subset of RPR then we could gradually enhance the >> implementation. > > I have no problem with that as long as we don't paint ourselves into a > corner. Totally agreed. >> Comments and suggestions are welcome. > > I have not looked at the patch yet, but is the reason for doing R020 > before R010 because you haven't done the MEASURES clause yet? One of the reasons is, implementing MATCH_RECOGNIZE (R010) looked harder for me because modifying main SELECT clause could be a hard work. Another reason is, I had no idea how to implement PREV/NEXT in other than in WINDOW clause. Other people might feel differently though. > In any case, I will be watching this with a close eye, and I am eager > to help in any way I can. Thank you! I am looking forward to comments on my patch. Also any idea how to implement MEASURES clause is welcome. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Row pattern recognition
Attached is a PoC patch to implement "Row pattern recognition" (RPR)
in SQL:2016 (I know SQL:2023 is already out, but I don't have access
to it). Actually SQL:2016 defines RPR in two places[1]:
Feature R010, “Row pattern recognition: FROM clause”
Feature R020, “Row pattern recognition: WINDOW clause”
The patch includes partial support for R020 part.
- What is RPR?
RPR provides a way to search series of data using regular expression
patterns. Suppose you have a stock database.
CREATE TABLE stock (
company TEXT,
tdate DATE,
price BIGINT);
You want to find a "V-shaped" pattern: i.e. price goes down for 1 or
more days, then goes up for 1 or more days. If we try to implement the
query in PostgreSQL, it could be quite complex and inefficient.
RPR provides convenient way to implement the query.
SELECT company, tdate, price, rpr(price) OVER w FROM stock
WINDOW w AS (
PARTITION BY company
ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
PATTERN (START DOWN+ UP+)
DEFINE
START AS TRUE,
UP AS price > PREV(price),
DOWN AS price < PREV(price)
);
"PATTERN" and "DEFINE" are the key clauses of RPR. DEFINE defines 3
"row pattern variables" namely START, UP and DOWN. They are associated
with logical expressions namely "TRUE", "price > PREV(price)", and
"price < PREV(price)". Note that "PREV" function returns price column
in the previous row. So, UP is true if price is higher than previous
day. On the other hand, DOWN is true if price is lower than previous
day. PATTERN uses the row pattern variables to create a necessary
pattern. In this case, the first row is always match because START is
always true, and second or more rows match with "UP" ('+' is a regular
expression representing one or more), and subsequent rows match with
"DOWN".
Here is the sample output.
company | tdate| price | rpr
--++---+--
company1 | 2023-07-01 | 100 |
company1 | 2023-07-02 | 200 | 200 -- 200->150->140->150
company1 | 2023-07-03 | 150 | 150 -- 150->140->150
company1 | 2023-07-04 | 140 |
company1 | 2023-07-05 | 150 | 150 -- 150->90->110->130
company1 | 2023-07-06 |90 |
company1 | 2023-07-07 | 110 |
company1 | 2023-07-08 | 130 |
company1 | 2023-07-09 | 120 |
company1 | 2023-07-10 | 130 |
rpr shows the first row if all the patterns are satisfied. In the
example above 200, 150, 150 are the cases. Other rows are shown as
NULL. For example, on 2023-07-02 price starts with 200, then goes down
to 150 then 140 but goes up 150 next day.
As far as I know, only Oracle implements RPR (only R010. R020 is not
implemented) among OSS/commercial RDBMSs. There are a few DWH software
having RPR. According to [2] they are Snowflake and MS Stream
Analytics. It seems Trino is another one[3].
- Note about the patch
The current implementation is not only a subset of the standard, but
is different from it in some places. The example above is written as
follows according to the standard:
SELECT company, tdate, startprice OVER w FROM stock
WINDOW w AS (
PARTITION BY company
MEASURES
START.price AS startprice
ROWS BETWEEN CURRENT ROW AND UNBOUNDED FOLLOWING
PATTERN (START DOWN+ UP+)
DEFINE
START AS TRUE,
UP AS UP.price > PREV(UP.price),
DOWN AS DOWN.price < PREV(DOWN.price)
);
Notice that rpr(price) is written as START.price and startprice in the
standard. MEASURES defines variable names used in the target list used
with "OVER window". As OVER only allows functions in PostgreSQL, I had
to make up a window function "rpr" which performs the row pattern
recognition task. I was not able to find a way to implement
expressions like START.price (START is not a table alias). Any
suggestion is greatly appreciated.
The differences from the standard include:
o MEASURES is not supported
o SUBSET is not supported
o FIRST, LAST, CLASSIFIER are not supported
o PREV/NEXT in the standard accept more complex arguments
o Regular expressions other than "+" are not supported
o Only AFTER MATCH SKIP TO NEXT ROW is supported (if AFTER MATCH is
not specified, AFTER MATCH SKIP TO NEXT ROW is assumed. In the
standard AFTER MATCH SKIP PAST LAST ROW is assumed in this case). I
have a plan to implement AFTER MATCH SKIP PAST LAST ROW though.
o INITIAL or SEEK are not supported ((behaves as if INITIAL is specified)
o Aggregate functions associated with window clause using RPR do not respect RPR
It seems RPR in the standard is quite complex. I think we can start
with a small subset of RPR then we could gradually enhance the
implementation.
Comments and suggestions are welcome.
[1]
https://sqlperformance.com/2019/04/t-sql-queries/row-pattern-recognition-in-sql
[2] https://link.springer.com/article/10.1007/s13222-022-00404-3
Re: make_ctags: use -I option to ignore pg_node_attr macro
> Hi Sawada-san, > >> In my Mac environment where non-Exuberant ctags and emacs 28.2 are >> installed, the generated etags file cannot be loaded by emacs due to >> file format error. The generated TAGS file is: >> >> % head -10 TAGS >> >> ) / >> sizeof(BlockNumber)sizeof(BlockNumber)117,3750 >> my >> @newa newa395,10443 >> >> variadic array[1,2]:array[1,2]56,1803 >> >> variadic array[]::inarray[]::i72,2331 >> >> variadic array[array64,2111 >> >> variadic array[array68, >> >> variadic array[array76,2441 >> (2 * (2 53,1353 >> my $fn fn387,10147 >> startblock 101,4876 >> >> Since the etags files consist of multiple sections[1] we cannot sort >> the generated etags file. With the attached patch, make_etags (with >> non-Exuberant ctags) generates a correct format etags file and it >> works: >> >> % head -10 TAGS >> >> /Users/masahiko/pgsql/source/postgresql/GNUmakefile,1187 >> subdir 7,56 >> top_builddir 8,65 >> docs:docs13,167 >> world-contrib-recurse:world-contrib-recurse19,273 >> world-bin-contrib-recurse:world-bin-contrib-recurse24,394 >> html man:html man26,444 >> install-docs:install-docs29,474 >> install-world-contrib-recurse:install-world-contrib-recurse35,604 >> >> BTW regarding the following comment, as far as I can read the >> Wikipedia page for ctags[1], Exuberant ctags file doesn't have a >> header section. >> >> # Exuberant tags has a header that we cannot sort in with the other entries >> # so we skip the sort step >> # Why are we sorting this? I guess some tag implementation need this, >> # particularly for append mode. bjm 2012-02-24 >> if [ ! "$IS_EXUBERANT" ] >> >> Instead, the page says that sorting non-Exuberant tags file allows for >> faster searching on of the tags file. I've fixed the comment >> accordingly too. >> >> Regards, >> >> [1] https://en.wikipedia.org/wiki/Ctags#Etags_2 > > Sorry for late reply and thanks for the patch! > > I have confirmed the error with make_etags on my Mac (emacs 28.1 + > non-Exuberant ctags), and the error is fixed by your patch. Also I > have confirmed the patch does not affect make_etags on my Linux (emacs > 26.3 + Exuberant ctags). > > I will push the fix to REL_15_STABLE and master branch if there's no > objection. Fix pushded. Thanks! Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: make_ctags: use -I option to ignore pg_node_attr macro
Hi Sawada-san, > In my Mac environment where non-Exuberant ctags and emacs 28.2 are > installed, the generated etags file cannot be loaded by emacs due to > file format error. The generated TAGS file is: > > % head -10 TAGS > > ) / > sizeof(BlockNumber)sizeof(BlockNumber)117,3750 > my > @newa newa395,10443 > > variadic array[1,2]:array[1,2]56,1803 > > variadic array[]::inarray[]::i72,2331 > > variadic array[array64,2111 > > variadic array[array68, > > variadic array[array76,2441 > (2 * (2 53,1353 > my $fn fn387,10147 > startblock 101,4876 > > Since the etags files consist of multiple sections[1] we cannot sort > the generated etags file. With the attached patch, make_etags (with > non-Exuberant ctags) generates a correct format etags file and it > works: > > % head -10 TAGS > > /Users/masahiko/pgsql/source/postgresql/GNUmakefile,1187 > subdir 7,56 > top_builddir 8,65 > docs:docs13,167 > world-contrib-recurse:world-contrib-recurse19,273 > world-bin-contrib-recurse:world-bin-contrib-recurse24,394 > html man:html man26,444 > install-docs:install-docs29,474 > install-world-contrib-recurse:install-world-contrib-recurse35,604 > > BTW regarding the following comment, as far as I can read the > Wikipedia page for ctags[1], Exuberant ctags file doesn't have a > header section. > > # Exuberant tags has a header that we cannot sort in with the other entries > # so we skip the sort step > # Why are we sorting this? I guess some tag implementation need this, > # particularly for append mode. bjm 2012-02-24 > if [ ! "$IS_EXUBERANT" ] > > Instead, the page says that sorting non-Exuberant tags file allows for > faster searching on of the tags file. I've fixed the comment > accordingly too. > > Regards, > > [1] https://en.wikipedia.org/wiki/Ctags#Etags_2 Sorry for late reply and thanks for the patch! I have confirmed the error with make_etags on my Mac (emacs 28.1 + non-Exuberant ctags), and the error is fixed by your patch. Also I have confirmed the patch does not affect make_etags on my Linux (emacs 26.3 + Exuberant ctags). I will push the fix to REL_15_STABLE and master branch if there's no objection. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Order changes in PG16 since ICU introduction
>> As I replied in that subthread, that creates a worse problem: if you >> only change the provider when the locale is C, then what about when the >> locale is *not* C? >> >> export LANG=en_US.UTF-8 >> initdb -D data --locale=fr_FR.UTF-8 >> ... >> provider:icu >> ICU locale: en-US >> >> I believe that case is an order of magnitude worse than the other cases >> you brought up in that subthread. >> >> It also leaves the fundamental problem in place that LOCALE only >> applies to the libc provider, which multiple people have agreed is not >> acceptable. Note that most of PostgreSQL users in Japan do initdb --no-locale. Almost never use other than C locale because the users do not rely on system collation. Most database have an extra column which represents the pronunciation in Hiragana or Katakana. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Order changes in PG16 since ICU introduction
Hi, > On Wed, 2023-06-07 at 23:50 +0200, Daniel Verite wrote: >> The simplest way to obtain that in v16 is to teach initdb that >> --locale=C without the --locale-provider option implies that >> --locale-provider=libc ([1]) > > As I replied in that subthread, that creates a worse problem: if you > only change the provider when the locale is C, then what about when the > locale is *not* C? > > export LANG=en_US.UTF-8 > initdb -D data --locale=fr_FR.UTF-8 > ... > provider:icu > ICU locale: en-US > > I believe that case is an order of magnitude worse than the other cases > you brought up in that subthread. > > It also leaves the fundamental problem in place that LOCALE only > applies to the libc provider, which multiple people have agreed is not > acceptable. Daniels comment: >> Yes it's a special case but when doing initdb --locale=C, a user does >> not need or want an ICU locale. They want the same thing than what v15 >> does with the same arguments: a template0 database with >> datlocprovider='c', datcollate='C', datctype='C', dateiculocale=NULL. So in this case the only way to keep the same behavior in v16 with "initdb --locale=C" (--no-locale) in v15 is, bulding PostgreSQL --without-icu? Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Let's make PostgreSQL multi-threaded
>> For the record, I think this will be a disaster. There is far too >> much >> code that will get broken, largely silently, and much of it is not >> under our control. >> >> > > > If we were starting out today we would probably choose a threaded > implementation. But moving to threaded now seems to me like a > multi-year-multi-person project with the prospect of years to come > chasing bugs and the prospect of fairly modest advantages. The risk to > reward doesn't look great. +1. Long time ago (PostgreSQL 7 days) I modified PostgreSQL to threaded implementation so that it runs on Windows because there's was no Windows port of PostgreSQL at that time. I don't remember the details but it was desperately hard for me. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Questionable coding in nth_value
> On Sat, May 6, 2023 at 4:44 PM Tatsuo Ishii wrote: > >> Currently Window function nth_value is coded as following: >> >> nth = DatumGetInt32(WinGetFuncArgCurrent(winobj, 1, )); >> if (isnull) >> PG_RETURN_NULL(); >> const_offset = get_fn_expr_arg_stable(fcinfo->flinfo, 1); >> >> if (nth <= 0) >> ereport(ERROR, >> : >> : >> >> Is there any reason why argument 'nth' is not checked earlier? >> IMO, it is more natural "if (nth <= 0)..." is placed right after "nth = >> DatumGetInt32...". >> >> Attached is the patch which does this. > > > Hmm, shouldn't we check if the argument of nth_value is null before we > check if it is greater than zero? So maybe we need to do this. That makes sense. I thought since this function is marked as strict, it would not be called if argument is NULL, but I was wrong. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
> The last time this was discussed ( > https://www.postgresql.org/message-id/1037735.1610402426%40sss.pgh.pa.us) > it was suggested to make the feature generalizable, beyond what the > standard says it should be limited to. I have read the mail. In my understanding nobody said that standard window functions should all accept the null treatment clause. Also Tom said: https://www.postgresql.org/message-id/5567.1537884439%40sss.pgh.pa.us > The questions of how we interface to the individual window functions > are really independent of how we handle the parsing problem. My > first inclination is to just pass the flags down to the window functions > (store them in WindowObject and provide some additional inquiry functions > in windowapi.h) and let them deal with it. As I said before I totally agree with this. With my patch if a (custom) window function does not want to accept null treatment clause, it just calls ErrorOutNullTreatment(). It will raise an error if IGNORE NULLS or RESPECT NULLS is provided. If it does call the function, it is up to the function how to deal with the null treatment. In another word, the infrastructure does not have fixed rules to allow/disallow null treatment clause for each window function. It's "delegated" to each window function. Anyway we can change the rule for other than nth_value etc. later easily once my patch is brought in. > With it generalizable, there would need to be extra checks for custom > functions, such as if they allow multiple column arguments (which I'll add > in v2 of the patch if the design's accepted). I am not sure if allowing-multiple-column-arguments patch should be provided with null-treatment patch. > So I think we need a consensus on whether to stick to limiting it to > several specific functions, or making it generalized yet agreeing the rules > to limit it (such as no agg functions, and no functions with multiple > column arguments). Let's see the discussion... Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Questionable coding in nth_value
Currently Window function nth_value is coded as following:
nth = DatumGetInt32(WinGetFuncArgCurrent(winobj, 1, ));
if (isnull)
PG_RETURN_NULL();
const_offset = get_fn_expr_arg_stable(fcinfo->flinfo, 1);
if (nth <= 0)
ereport(ERROR,
:
:
Is there any reason why argument 'nth' is not checked earlier?
IMO, it is more natural "if (nth <= 0)..." is placed right after "nth =
DatumGetInt32...".
Attached is the patch which does this.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
diff --git a/src/backend/utils/adt/windowfuncs.c b/src/backend/utils/adt/windowfuncs.c
index b87a624fb2..f4ff060930 100644
--- a/src/backend/utils/adt/windowfuncs.c
+++ b/src/backend/utils/adt/windowfuncs.c
@@ -696,15 +696,16 @@ window_nth_value(PG_FUNCTION_ARGS)
int32 nth;
nth = DatumGetInt32(WinGetFuncArgCurrent(winobj, 1, ));
- if (isnull)
- PG_RETURN_NULL();
- const_offset = get_fn_expr_arg_stable(fcinfo->flinfo, 1);
-
if (nth <= 0)
ereport(ERROR,
(errcode(ERRCODE_INVALID_ARGUMENT_FOR_NTH_VALUE),
errmsg("argument of nth_value must be greater than zero")));
+ if (isnull)
+ PG_RETURN_NULL();
+
+ const_offset = get_fn_expr_arg_stable(fcinfo->flinfo, 1);
+
result = WinGetFuncArgInFrame(winobj, 0,
nth - 1, WINDOW_SEEK_HEAD, const_offset,
, NULL);
Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
>> The attached test patch is mostly the same as in the previous patch
>> set, but it doesn't fail on row_number anymore as the main patch
>> only rejects aggregate functions. The test patch also adds a test for
>
>> +SELECT sum(orbit) RESPECT NULLS OVER () FROM planets; -- succeeds
>
> I think the standard does not allow to specify RESPECT NULLS other
> than lead, lag, first_value, last_value and nth_value. Unless we agree
> that PostgreSQL violates the standard in this regard, you should not
> allow to use RESPECT NULLS for the window functions, expect lead etc.
> and aggregates.
>
> See my patch.
>
>> +/*
>> + * Window function option clauses
>> + */
>> +opt_null_treatment:
>> +RESPECT NULLS_P
>> { $$ = RESPECT_NULLS; }
>> +| IGNORE_P NULLS_P
>> { $$ = IGNORE_NULLS; }
>> +| /*EMPTY*/
>> { $$ = NULL_TREATMENT_NOT_SET; }
>> +;
>
> With this, you can check if null treatment clause is used or not in
> each window function.
>
> In my previous patch I did the check in parse/analysis but I think
> it's better to be checked in each window function. This way,
>
> - need not to add a column to pg_proc.
>
> - allow user defined window functions to decide by themselves whether
> they can accept null treatment option.
Attached is the patch to implement this (on top of your patch).
test=# SELECT row_number() RESPECT NULLS OVER () FROM (SELECT 1) AS s;
ERROR: window function row_number cannot have RESPECT NULLS or IGNORE NULLS
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index fac0e05dee..b8519d9890 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -74,6 +74,7 @@ typedef struct WindowObjectData
int64 *win_nonnulls; /* tracks non-nulls in ignore nulls mode */
int nonnulls_size; /* track size of the win_nonnulls array */
int nonnulls_len; /* track length of the win_nonnulls array */
+ WindowFunc *wfunc; /* WindowFunc of this function */
} WindowObjectData;
/*
@@ -2634,6 +2635,8 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
winobj->nonnulls_len = 0;
}
+ winobj->wfunc = wfunc;
+
/* It's a real window function, so set up to call it. */
fmgr_info_cxt(wfunc->winfnoid, >flinfo,
econtext->ecxt_per_query_memory);
@@ -3881,3 +3884,24 @@ WinGetFuncArgCurrent(WindowObject winobj, int argno, bool *isnull)
return ExecEvalExpr((ExprState *) list_nth(winobj->argstates, argno),
econtext, isnull);
}
+
+/*
+ * Error out that this window function cannot have null treatement.
+ */
+void
+ErrorOutNullTreatment(WindowObject winobj)
+{
+ char *fname;
+
+ Assert(WindowObjectIsValid(winobj));
+
+ if (winobj->wfunc->null_treatment == NULL_TREATMENT_NOT_SET)
+ return;
+
+ fname = get_func_name(winobj->wfunc->winfnoid);
+
+ ereport(ERROR,
+ (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("window function %s cannot have RESPECT NULLS or IGNORE NULLS",
+ fname)));
+}
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 01fd16acf9..05e64c4569 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -2475,6 +2475,7 @@ eval_const_expressions_mutator(Node *node,
newexpr->winstar = expr->winstar;
newexpr->winagg = expr->winagg;
newexpr->ignore_nulls = expr->ignore_nulls;
+newexpr->null_treatment = expr->null_treatment;
newexpr->location = expr->location;
return (Node *) newexpr;
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 58c00bfd4f..e131428e85 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -276,6 +276,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
MergeWhenClause *mergewhen;
struct KeyActions *keyactions;
struct KeyAction *keyaction;
+ NullTreatment nulltreatment;
}
%type stmt toplevel_stmt schema_stmt routine_body_stmt
@@ -633,7 +634,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
opt_frame_clause frame_extent frame_bound
%type opt_window_exclusion_clause
%type opt_existing_window_name
-%type null_treatment
%type opt_if_not_exists
%type opt_unique_null_treatment
%type generated_when override_kind
@@ -662,6 +662,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
json_object_constructor_null
Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
> The attached test patch is mostly the same as in the previous patch
> set, but it doesn't fail on row_number anymore as the main patch
> only rejects aggregate functions. The test patch also adds a test for
> +SELECT sum(orbit) RESPECT NULLS OVER () FROM planets; -- succeeds
I think the standard does not allow to specify RESPECT NULLS other
than lead, lag, first_value, last_value and nth_value. Unless we agree
that PostgreSQL violates the standard in this regard, you should not
allow to use RESPECT NULLS for the window functions, expect lead etc.
and aggregates.
See my patch.
> +/*
> + * Window function option clauses
> + */
> +opt_null_treatment:
> + RESPECT NULLS_P
> { $$ = RESPECT_NULLS; }
> + | IGNORE_P NULLS_P
> { $$ = IGNORE_NULLS; }
> + | /*EMPTY*/
> { $$ = NULL_TREATMENT_NOT_SET; }
> + ;
With this, you can check if null treatment clause is used or not in
each window function.
In my previous patch I did the check in parse/analysis but I think
it's better to be checked in each window function. This way,
- need not to add a column to pg_proc.
- allow user defined window functions to decide by themselves whether
they can accept null treatment option.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
> Vik Fearing writes: >> On 4/22/23 14:14, Tatsuo Ishii wrote: >>> Note that RESPECT/IGNORE are not registered as reserved keywords in >>> this patch (but registered as unreserved keywords). I am not sure if >>> this is acceptable or not. > >> For me, this is perfectly okay. Keep them at the lowest level of >> reservation as possible. > > Yeah, keep them unreserved if at all possible. Any higher reservation > level risks breaking existing applications that might be using these > words as column or function names. Agreed. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
> Excellent. I was thinking about picking my version of this patch up > again, but I think this might be better than mine. Thanks. > I am curious why set_mark is false in the IGNORE version instead of > also being const_offset. Surely the nth non-null in the frame will > never go backwards. Initially I thought that too. But when I used const_offset instead of false. I got an error: ERROR: cannot fetch row before WindowObject's mark position > I do agree that we can have without last> so let's move forward with this and handle the latter later. Agreed. >> Currently in the patch only nth_value is allowed to use RESPECT/IGNORE >> NULLS. > > This should not be hard coded. It should be a new field in pg_proc > (with a sanity check that it is only true for window functions). That > way custom window functions can implement it. There were some discussions on this in the past. https://www.postgresql.org/message-id/flat/CAGMVOdsbtRwE_4%2Bv8zjH1d9xfovDeQAGLkP_B6k69_VoFEgX-A%40mail.gmail.com It seems Tom and Andrew thought that "1.1.2. Change the behavior of the windowapi in some consistent way" is ambitious. If we follow this direction, I think each window function should check WindowFunc struct passed by WinGetWindowFunc (added in my patch) to check whether IGNORE NULLS can be applied or not in the function. If not, error out. This way, we don't need to add a new field to pg_proc. >> No document nor test patches are included for now. > > I can volunteer to work on these if you want. Thanks! I think you can work on top of the last patch posted by Krasiyan Andreev: https://www.postgresql.org/message-id/CAN1PwonAnC-KkRyY%2BDtRmxQ8rjdJw%2BgcOsHruLr6EnF7zSMH%3DQ%40mail.gmail.com Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options
I revisited the thread:
https://www.postgresql.org/message-id/flat/CAGMVOdsbtRwE_4%2Bv8zjH1d9xfovDeQAGLkP_B6k69_VoFEgX-A%40mail.gmail.com
and came up with attached POC patch (I used some varibale names
appearing in the Krasiyan Andreev's patch). I really love to have
RESPECT/IGNORE NULLS because I believe they are convenient for
users. For FIRST/LAST I am not so excited since there are alternatives
as our document stats, so FIRST/LAST are not included in the patch.
Currently in the patch only nth_value is allowed to use RESPECT/IGNORE
NULLS. I think it's not hard to implement it for others (lead, lag,
first_value and last_value). No document nor test patches are
included for now.
Note that RESPECT/IGNORE are not registered as reserved keywords in
this patch (but registered as unreserved keywords). I am not sure if
this is acceptable or not.
> The questions of how we interface to the individual window functions
> are really independent of how we handle the parsing problem. My
> first inclination is to just pass the flags down to the window functions
> (store them in WindowObject and provide some additional inquiry functions
> in windowapi.h) and let them deal with it.
I agree with this. Also I do not change the prototype of
nth_value. So I pass RESPECT/IGNORE NULLS information from the raw
parser to parse/analysis and finally to WindowObject.
> It's also worth wondering if we couldn't just implement the flags in
> some generic fashion and not need to involve the window functions at
> all. FROM LAST, for example, could and perhaps should be implemented
> by inverting the sort order. Possibly IGNORE NULLS could be implemented
> inside the WinGetFuncArgXXX functions? These behaviors might or might
> not make much sense with other window functions, but that doesn't seem
> like it's our problem.
Yes, probably we could make WinGetFuncArgXXX a little bit smarter in
this direction (not implemented in the patch at this point).
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
>From 07f01f8859e159c908ada72e8f53daf51e0b8bdf Mon Sep 17 00:00:00 2001
From: Tatsuo Ishii
Date: Sat, 22 Apr 2023 16:52:50 +0900
Subject: [PATCH v1 1/3] Implement IGNORE or RESPECT NULLS parse/analysis part.
Implement SQL standard's IGNORE/RESPECT NULLS clause for window functions.
For now, only nth_value() can use this option.
---
src/backend/parser/gram.y | 22 ++
src/backend/parser/parse_func.c | 13 +
src/include/nodes/parsenodes.h | 8
src/include/nodes/primnodes.h | 2 ++
src/include/parser/kwlist.h | 2 ++
5 files changed, 43 insertions(+), 4 deletions(-)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index acf6cf4866..2980ecd666 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -276,6 +276,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
MergeWhenClause *mergewhen;
struct KeyActions *keyactions;
struct KeyAction *keyaction;
+ NullTreatment nulltreatment;
}
%type stmt toplevel_stmt schema_stmt routine_body_stmt
@@ -661,6 +662,8 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
json_object_constructor_null_clause_opt
json_array_constructor_null_clause_opt
+%type opt_null_treatment
+
/*
* Non-keyword token types. These are hard-wired into the "flex" lexer.
* They must be listed first so that their numeric codes do not depend on
@@ -718,7 +721,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
HANDLER HAVING HEADER_P HOLD HOUR_P
- IDENTITY_P IF_P ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE
+ IDENTITY_P IF_P IGNORE_P ILIKE IMMEDIATE IMMUTABLE IMPLICIT_P IMPORT_P IN_P INCLUDE
INCLUDING INCREMENT INDENT INDEX INDEXES INHERIT INHERITS INITIALLY INLINE_P
INNER_P INOUT INPUT_P INSENSITIVE INSERT INSTEAD INT_P INTEGER
INTERSECT INTERVAL INTO INVOKER IS ISNULL ISOLATION
@@ -752,7 +755,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
RANGE READ REAL REASSIGN RECHECK RECURSIVE REF_P REFERENCES REFERENCING
REFRESH REINDEX RELATIVE_P RELEASE RENAME REPEATABLE REPLACE REPLICA
- RESET RESTART RESTRICT RETURN RETURNING RETURNS REVOKE RIGHT ROLE ROLLBACK ROLLUP
+ RESET RESPECT RESTART RESTRICT RETURN RETURNING RETURNS REVOKE RIGHT ROLE ROLLBACK ROLLUP
ROUTINE ROUTINES ROW ROWS RULE
SAVEPOINT SCALAR SCHEMA SCHEMAS SCROLL SEARCH SECOND_P SECURITY SELECT
@@ -15213,7 +15216,7 @@ func_application: func_name '(' ')'
* (Note that many of the special SQL functions wouldn't actually make any
* sense as functional index entries, but we ignore that consideration here.)
*/
-func_expr: func_application within_group_clause filter_clause over_clause
+func_expr: func_application within_group_clause filter_clause opt_null
Re: [EXTERNAL] Support load balancing in libpq
> I think it's fine to remove it. It originated from postmaster.c, where > I copied the original implementation of libpq_prng_init from. I agree to remove unlikely macro here. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: [EXTERNAL] Support load balancing in libpq
>> "unlikely" macro is used in libpq_prng_init() in the patch. I wonder
>> if the place is really 'hot' to use "unlikely" macro.
>
> I don't think it is, I was thinking to rewrite as the below sketch:
>
> {
> if (pg_prng_strong_seed(>prng_state)))
> return;
>
> /* fallback seeding */
> }
+1.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Re: [EXTERNAL] Support load balancing in libpq
Hi, "unlikely" macro is used in libpq_prng_init() in the patch. I wonder if the place is really 'hot' to use "unlikely" macro. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Doc: Improve note about copying into postgres_fdw foreign tables in batch
> While not the fault of this patch I find it confusing that we mix > and for marking up "postgres_fdw", the latter seemingly more correct > (and less commonly used) than . I think we traditionally use for an extension module (file) name. It seems the is used when we want to refer to objects other than files. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: Doc: Improve note about copying into postgres_fdw foreign tables in batch
> On Fri, Feb 17, 2023 at 5:45 PM Etsuro Fujita wrote:
>> Here is a small patch to improve the note, which was added by commit
>> 97da48246 ("Allow batch insertion during COPY into a foreign table."),
>> by adding an explanation about how the actual number of rows
>> postgres_fdw inserts at once is determined in the COPY case, including
>> a limitation that does not apply to the INSERT case.
>
> Does anyone want to comment on this?
>
> - This option also applies when copying into foreign tables.
> + This option also applies when copying into foreign tables. In that
> case
> + the actual number of rows postgres_fdw copies at
> + once is determined in a similar way to in the insert case, but it is
"similar way to in" should be "similar way to", maybe?
> + limited to at most 1000 due to implementation restrictions of the
> + COPY command.
>
>
>
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Re: make_ctags: use -I option to ignore pg_node_attr macro
>> I fixed the above issues and refactored the code. >> Attached is the updated version of the patch. Thought? > > Thank you! Looks good to me. Fix pushed. Thank you! Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: make_ctags: use -I option to ignore pg_node_attr macro
>> Oops. Thank you for pointing it out. BTW, just out of curiosity, do >> you have etags on you Mac? > > Yes. > > $ etags --version > etags (GNU Emacs 28.2) > Copyright (C) 2022 Free Software Foundation, Inc. > This program is distributed under the terms in ETAGS.README Ok. Probably that was installed with emacs. For some reason I don't know, I don't have etags even I already installed emacs. So I decided to test using my old Ubuntu 18 vm, which has old ctags and etags. > How about just applying the following into make_etags? > > +if [ $# -gt 1 ] || ( [ $# -eq 1 ] && [ $1 != "-n" ] ) > +then echo "Usage: $0 [-n]" > + exit 1 > +fi Ok from me. Looks simple enough. > With the patch, make_etags caused the following error messages > on my MacOS. > > $ ./src/tools/make_etags > No such file or directory > No such file or directory > > To fix this error, probaby we should get rid of double-quotes > from "$FLAGS" "$IGNORE_IDENTIFIES" in the following command. > > - xargs ctags $MODE -a -f $TAGS_FILE "$FLAGS" "$IGNORE_IDENTIFIES" > + xargs $PROG $MODE $TAGS_OPT $TAGS_FILE "$FLAGS" "$IGNORE_IDENTIFIES" Oh, I see. > + elsectags --help 2>&1 | grep -- -e >/dev/null > + # Note that "ctags --help" does not always work. Even certain ctags > does not have the option. > > This code seems to assume that there is non-Exuberant ctags > supporting -e option. But does such ctags really exist? Good question. I vaguely recalled so、but I haven't been able to find any evidence for it. > > I fixed the above issues and refactored the code. > Attached is the updated version of the patch. Thought? Thank you! Looks good to me. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: make_ctags: use -I option to ignore pg_node_attr macro
>> Attached is the v2 patch.
>
> Thanks for the patch!
>
> With the patch, I got the following error when executing make_etags..
>
> $ ./src/tools/make_etags
> etags: invalid option -- 'e'
> Try 'etags --help' for a complete list of options.
> sort: No such file or directory
Oops. Thank you for pointing it out. BTW, just out of curiosity, do
you have etags on you Mac? Mine doesn't have etags. That's why I
missed the error.
> + if [ $? != 0 -a -z "$ETAGS_EXISTS" ]
> + then echo "'ctags' does not support emacs mode and etags does not
> exist" 1>&2; exit 1
> + fi
>
> This code can be reached after "rm -f ./$TAGS_FILE" is executed in
> make_ctags.
> But we should check whether the required program has been already
> installed
> and exit immediately if not, before modifying anything?
Agreed.
> This is the comment for the commit d1e2a380cb. I found that make_etags
> with
> an invalid option reported the following usage message mentioning
> make_ctags
> (not make_etags). Isn't this confusing?
>
> $ ./src/tools/make_etags -a
> Usage: /.../make_ctags [-e][-n]
That's hard to fix without some code duplication. We decided that
make_etags is not a symlink to make_ctags, rather execs make_ctags. Of
course we could let make_etags perform the same option check, but I
doubt it's worth the trouble.
Anyway, attached is the v3 patch.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
diff --git a/src/tools/make_ctags b/src/tools/make_ctags
index 102881667b..38f4d8e2d5 100755
--- a/src/tools/make_ctags
+++ b/src/tools/make_ctags
@@ -12,12 +12,15 @@ fi
MODE=
NO_SYMLINK=
TAGS_FILE="tags"
+ETAGS_EXISTS=
+PROG=ctags
while [ $# -gt 0 ]
do
if [ $1 = "-e" ]
then MODE="-e"
TAGS_FILE="TAGS"
+ command -v etags >/dev/null && ETAGS_EXISTS="Y"
elif [ $1 = "-n" ]
then NO_SYMLINK="Y"
else
@@ -27,15 +30,33 @@ do
shift
done
-command -v ctags >/dev/null || \
+if test -z "$MODE"
+then (command -v ctags >/dev/null) || \
{ echo "'ctags' program not found" 1>&2; exit 1; }
+fi
trap "ret=$?; rm -rf /tmp/$$; exit $ret" 0 1 2 3 15
-rm -f ./$TAGS_FILE
IS_EXUBERANT=""
ctags --version 2>&1 | grep Exuberant && IS_EXUBERANT="Y"
+# ctags is not exuberant and emacs mode is specified
+if [ -z "$IS_EXUBERANT" -a -n "$MODE" ]
+then
+ if [ -n "$ETAGS_EXISTS" ]
+ then
+ # etags exists. Use it.
+ PROG=etags
+ MODE=
+ else ctags --help 2>&1 | grep -- -e >/dev/null
+ # Note that "ctags --help" does not always work. Even certain ctags does not have the option.
+ # In that case we assume that the ctags does not support emacs mode.
+ if [ $? != 0 -a -z "$ETAGS_EXISTS" ]
+ then echo "'ctags' does not support emacs mode and etags does not exist" 1>&2; exit 1
+ fi
+ fi
+fi
+
# List of kinds supported by Exuberant Ctags 5.8
# generated by ctags --list-kinds
# --c-kinds was called --c-types before 2003
@@ -65,11 +86,19 @@ then IGNORE_IDENTIFIES="-I pg_node_attr+"
else IGNORE_IDENTIFIES=
fi
+if [ $PROG = "ctags" ]
+then TAGS_OPT="-a -f"
+else TAGS_OPT="-a -o"
+ FLAGS=
+fi
+
+rm -f ./$TAGS_FILE
+
# this is outputting the tags into the file 'tags', and appending
find `pwd`/ \( -name tmp_install -prune -o -name tmp_check -prune \) \
-o \( -name "*.[chly]" -o -iname "*makefile*" -o -name "*.mk" -o -name "*.in" \
-o -name "*.sql" -o -name "*.p[lm]" \) -type f -print |
- xargs ctags $MODE -a -f $TAGS_FILE "$FLAGS" "$IGNORE_IDENTIFIES"
+ xargs $PROG $MODE $TAGS_OPT $TAGS_FILE "$FLAGS" "$IGNORE_IDENTIFIES"
# Exuberant tags has a header that we cannot sort in with the other entries
# so we skip the sort step
Re: make_ctags: use -I option to ignore pg_node_attr macro
>> I am not sure if this is good way to check if ctags supports "-e" or not.
>>
>> +thenctags --version 2>&1 | grep -- -e >/dev/null
>>
>> Perhaps, "--help" might be intended rather than "--version" to check
>> supported options?
>
> Yeah, that was my mistake.
>
>> Even so, ctags would have other option whose name contains
>> "-e" than Emacs support, so this check could end in a wrong result.
>> Therefore,
>> it seems to me that it is better to check immediately if etags is available
>> in case that we don't have Exuberant-type ctags.
>
> That makes sense.
Attached is the v2 patch.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
diff --git a/src/tools/make_ctags b/src/tools/make_ctags
index 102881667b..14089c5a7c 100755
--- a/src/tools/make_ctags
+++ b/src/tools/make_ctags
@@ -12,12 +12,15 @@ fi
MODE=
NO_SYMLINK=
TAGS_FILE="tags"
+ETAGS_EXISTS=
+PROG=ctags
while [ $# -gt 0 ]
do
if [ $1 = "-e" ]
then MODE="-e"
TAGS_FILE="TAGS"
+ command -v etags >/dev/null && ETAGS_EXISTS="Y"
elif [ $1 = "-n" ]
then NO_SYMLINK="Y"
else
@@ -27,8 +30,10 @@ do
shift
done
-command -v ctags >/dev/null || \
+if test -z "$MODE"
+then (command -v ctags >/dev/null) || \
{ echo "'ctags' program not found" 1>&2; exit 1; }
+fi
trap "ret=$?; rm -rf /tmp/$$; exit $ret" 0 1 2 3 15
rm -f ./$TAGS_FILE
@@ -36,6 +41,22 @@ rm -f ./$TAGS_FILE
IS_EXUBERANT=""
ctags --version 2>&1 | grep Exuberant && IS_EXUBERANT="Y"
+# ctags is not exuberant and emacs mode is specified
+if [ -z "$IS_EXUBERANT" -a -n "$MODE" ]
+then
+ if [ -n "$ETAGS_EXISTS" ]
+ then
+ # etags exists. Use it.
+ PROG=etags
+ else ctags --help 2>&1 | grep -- -e >/dev/null
+ # Note that "ctags --help" does not always work. Even certain ctags does not have the option.
+ # In that case we assume that the ctags does not support emacs mode.
+ if [ $? != 0 -a -z "$ETAGS_EXISTS" ]
+ then echo "'ctags' does not support emacs mode and etags does not exist" 1>&2; exit 1
+ fi
+ fi
+fi
+
# List of kinds supported by Exuberant Ctags 5.8
# generated by ctags --list-kinds
# --c-kinds was called --c-types before 2003
@@ -65,11 +86,16 @@ then IGNORE_IDENTIFIES="-I pg_node_attr+"
else IGNORE_IDENTIFIES=
fi
+if [ $PROG = "ctags" ]
+then TAGS_OPT="-a -f"
+else TAGS_OPT="-a -o"
+fi
+
# this is outputting the tags into the file 'tags', and appending
find `pwd`/ \( -name tmp_install -prune -o -name tmp_check -prune \) \
-o \( -name "*.[chly]" -o -iname "*makefile*" -o -name "*.mk" -o -name "*.in" \
-o -name "*.sql" -o -name "*.p[lm]" \) -type f -print |
- xargs ctags $MODE -a -f $TAGS_FILE "$FLAGS" "$IGNORE_IDENTIFIES"
+ xargs $PROG $MODE $TAGS_OPT $TAGS_FILE "$FLAGS" "$IGNORE_IDENTIFIES"
# Exuberant tags has a header that we cannot sort in with the other entries
# so we skip the sort step
Re: make_ctags: use -I option to ignore pg_node_attr macro
>> The patch drops support for "-n" option :-< >> >> Attached is the patch by fixing make_ctags (make_etags is not >> touched). >> >> If Exuberant-type ctags is available, use it (not changed). >> If Exuberant-type ctags is not available, try old ctags (not changed). >> If the old ctags does not support "-e" option, try etags (new). > > I am not sure if this is good way to check if ctags supports "-e" or not. > > + thenctags --version 2>&1 | grep -- -e >/dev/null > > Perhaps, "--help" might be intended rather than "--version" to check > supported options? Yeah, that was my mistake. > Even so, ctags would have other option whose name contains > "-e" than Emacs support, so this check could end in a wrong result. > Therefore, > it seems to me that it is better to check immediately if etags is available > in case that we don't have Exuberant-type ctags. That makes sense. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
Re: make_ctags: use -I option to ignore pg_node_attr macro
> Does is make sense to change make_etags as the attached patch does?
> This allows make_etags to use etags if Exuberant-type ctags is not
> available. This allows users to use make_etags if hey has either
> Exuberant-type ctags or etags.
The patch drops support for "-n" option :-<
Attached is the patch by fixing make_ctags (make_etags is not
touched).
If Exuberant-type ctags is available, use it (not changed).
If Exuberant-type ctags is not available, try old ctags (not changed).
If the old ctags does not support "-e" option, try etags (new).
If etags is not available, give up (new).
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
diff --git a/src/tools/make_ctags b/src/tools/make_ctags
index 102881667b..8fb9991db0 100755
--- a/src/tools/make_ctags
+++ b/src/tools/make_ctags
@@ -12,12 +12,15 @@ fi
MODE=
NO_SYMLINK=
TAGS_FILE="tags"
+ETAGS_EXISTS=
+PROG=ctags
while [ $# -gt 0 ]
do
if [ $1 = "-e" ]
then MODE="-e"
TAGS_FILE="TAGS"
+ command -v etags >/dev/null && ETAGS_EXISTS="Y"
elif [ $1 = "-n" ]
then NO_SYMLINK="Y"
else
@@ -27,8 +30,10 @@ do
shift
done
-command -v ctags >/dev/null || \
+if test -z "$MODE"
+then (command -v ctags >/dev/null) || \
{ echo "'ctags' program not found" 1>&2; exit 1; }
+fi
trap "ret=$?; rm -rf /tmp/$$; exit $ret" 0 1 2 3 15
rm -f ./$TAGS_FILE
@@ -36,6 +41,20 @@ rm -f ./$TAGS_FILE
IS_EXUBERANT=""
ctags --version 2>&1 | grep Exuberant && IS_EXUBERANT="Y"
+if test -z "$IS_EXUBERANT"
+then
+ if test -n "$MODE"
+ then ctags --version 2>&1 | grep -- -e >/dev/null
+ if [ $? != 0 ]
+ then
+ if [ -z $ETAGS_EXISTS ]
+ then echo "'ctags' does not support emacs mode and etags does not exist" 1>&2; exit 1
+ else PROG=etags
+ fi
+ fi
+ fi
+fi
+
# List of kinds supported by Exuberant Ctags 5.8
# generated by ctags --list-kinds
# --c-kinds was called --c-types before 2003
@@ -65,11 +84,16 @@ then IGNORE_IDENTIFIES="-I pg_node_attr+"
else IGNORE_IDENTIFIES=
fi
+if [ $PROG = "ctags" ]
+then TAGS_OPT="-a -f"
+else TAGS_OPT="-a -o"
+fi
+
# this is outputting the tags into the file 'tags', and appending
find `pwd`/ \( -name tmp_install -prune -o -name tmp_check -prune \) \
-o \( -name "*.[chly]" -o -iname "*makefile*" -o -name "*.mk" -o -name "*.in" \
-o -name "*.sql" -o -name "*.p[lm]" \) -type f -print |
- xargs ctags $MODE -a -f $TAGS_FILE "$FLAGS" "$IGNORE_IDENTIFIES"
+ xargs $PROG $MODE $TAGS_OPT $TAGS_FILE "$FLAGS" "$IGNORE_IDENTIFIES"
# Exuberant tags has a header that we cannot sort in with the other entries
# so we skip the sort step
Re: make_ctags: use -I option to ignore pg_node_attr macro
>> Since this commit, make_etags has started failing to generate
>> tags files with the following error messages, on my MacOS.
>>
>> $ src/tools/make_etags
>> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ctags:
>> illegal option -- e
>> usage: ctags [-BFadtuwvx] [-f tagsfile] file ...
>> sort: No such file or directory
>>
>>
>> In my MacOS, non-Exuberant ctags is installed and doesn't support
>> -e option. But the commit changed make_etags so that it always
>> calls ctags with -e option via make_ctags. This seems the cause of
>> the above failure.
>>
>> IS_EXUBERANT=""
>> ctags --version 2>&1 | grep Exuberant && IS_EXUBERANT="Y"
>>
>> make_ctags has the above code and seems to support non-Exuberant
>> ctags.
>> If so, we should revert the changes of make_etags by the commit and
>> make make_etags work with that ctags? Or, we should support
>> only Exuberant-type ctags (btw, I'm ok with this) and get rid of
>> something like the above code?
>
> Thanks for the report. I will look into this.
Previous make_etags relied on etags command:
#!/bin/sh
# src/tools/make_etags
command -v etags >/dev/null || \
{ echo "'etags' program not found" 1>&2; exit 1; }
:
:
My Mac (M1 Mac running macOS 12.6) does not have etags. Thus before
the commit make_etags on Mac failed anyway. Do we want make_etags to
restore the previous behavior? i.e. 'etags' program not found
>> If so, we should revert the changes of make_etags by the commit and
>> make make_etags work with that ctags?
I think ctags on Mac cannot produce tags file for emacs.
Best reagards,
--
Tatsuo Ishii
SRA OSS LLC
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp
Re: make_ctags: use -I option to ignore pg_node_attr macro
> On 2022/10/19 13:25, Tatsuo Ishii wrote: >> Thanks. the v6 patch pushed to master branch. > > Since this commit, make_etags has started failing to generate > tags files with the following error messages, on my MacOS. > > $ src/tools/make_etags > /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ctags: > illegal option -- e > usage: ctags [-BFadtuwvx] [-f tagsfile] file ... > sort: No such file or directory > > > In my MacOS, non-Exuberant ctags is installed and doesn't support > -e option. But the commit changed make_etags so that it always > calls ctags with -e option via make_ctags. This seems the cause of > the above failure. > > IS_EXUBERANT="" > ctags --version 2>&1 | grep Exuberant && IS_EXUBERANT="Y" > > make_ctags has the above code and seems to support non-Exuberant > ctags. > If so, we should revert the changes of make_etags by the commit and > make make_etags work with that ctags? Or, we should support > only Exuberant-type ctags (btw, I'm ok with this) and get rid of > something like the above code? Thanks for the report. I will look into this. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
