Thread
Commits
-
Refactor grammar to create opt_utility_option_list
- 1dfe3ef3f960 19 (unreleased) landed
-
trivial grammar refactor
Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-23T15:38:34Z
Hello I noticed some duplicative coding while hacking on REPACK[1]. We can save a few lines now with a trivial change to the rules for CHECKPOINT and REINDEX, and allow to save a few extra lines in that patch. Any objections to this? [1] https://commitfest.postgresql.org/patch/5117/ -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
-
Re: trivial grammar refactor
Nathan Bossart <nathandbossart@gmail.com> — 2025-07-23T15:43:57Z
On Wed, Jul 23, 2025 at 05:38:34PM +0200, Álvaro Herrera wrote: > I noticed some duplicative coding while hacking on REPACK[1]. We can > save a few lines now with a trivial change to the rules for CHECKPOINT > and REINDEX, and allow to save a few extra lines in that patch. > > Any objections to this? Seems reasonable to me. Any chance we can use this for CLUSTER, VACUUM, ANALYZE, and EXPLAIN, too? -- nathan
-
Re: trivial grammar refactor
Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-23T16:50:59Z
On 2025-Jul-23, Nathan Bossart wrote: > On Wed, Jul 23, 2025 at 05:38:34PM +0200, Álvaro Herrera wrote: > > I noticed some duplicative coding while hacking on REPACK[1]. We can > > save a few lines now with a trivial change to the rules for CHECKPOINT > > and REINDEX, and allow to save a few extra lines in that patch. > > > > Any objections to this? > > Seems reasonable to me. Any chance we can use this for CLUSTER, VACUUM, > ANALYZE, and EXPLAIN, too? ANALYZE and VACUUM cannot be changed this way, because the productions where options are optional are the legacy ones; so running "VACUUM/ANALYZE table" uses that one, and we cannot change that: VacuumStmt: VACUUM opt_full opt_freeze opt_verbose opt_analyze opt_vacuum_relation_list EXPLAIN doesn't work because the '(' starts either a query (via select_with_parens) or an option list, so you get a shift/reduce conflict. I hadn't looked at CLUSTER, because the REPACK patch is absorbing that rule into a larger rule for both REPACK and CLUSTER. But now that I look again, I realize that apparently my REPACK branch is bogus, because I forgot to add a production for "CLUSTER name ON qualified_name". And with that one put back, I cannot use opt_utility_option_list there either, because the above conflicts with "CLUSTER opt_utility_option_list qualified_name cluster_index_specification". So we can still do this, and I still think it's a win, but unfortunately it won't help for the REPACK patch. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ Voy a acabar con todos los humanos / con los humanos yo acabaré voy a acabar con todos (bis) / con todos los humanos acabaré ¡acabaré! (Bender) -
Re: trivial grammar refactor
Nathan Bossart <nathandbossart@gmail.com> — 2025-07-23T16:54:52Z
On Wed, Jul 23, 2025 at 06:50:59PM +0200, Álvaro Herrera wrote: > So we can still do this, and I still think it's a win, +1 > but unfortunately it won't help for the REPACK patch. Darn. -- nathan
-
Re: trivial grammar refactor
Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-23T17:26:28Z
On 2025-Jul-23, Álvaro Herrera wrote: > So we can still do this, and I still think it's a win, but unfortunately > it won't help for the REPACK patch. Ah no, I can still use it: RepackStmt: REPACK opt_utility_option_list qualified_name USING INDEX name | REPACK opt_utility_option_list qualified_name USING INDEX | REPACK opt_utility_option_list qualified_name | REPACK USING INDEX | CLUSTER '(' utility_option_list ')' qualified_name cluster_index_specification | CLUSTER qualified_name cluster_index_specification | CLUSTER opt_utility_option_list | CLUSTER VERBOSE qualified_name cluster_index_specification | CLUSTER VERBOSE | CLUSTER VERBOSE name ON qualified_name | CLUSTER name ON qualified_name ; -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ Essentially, you're proposing Kevlar shoes as a solution for the problem that you want to walk around carrying a loaded gun aimed at your foot. (Tom Lane) -
Re: trivial grammar refactor
Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-23T17:59:52Z
... so using the same set of productions, I can rewrite the current CLUSTER rule in this way and it won't be a problem for the REPACK changes. Thanks for looking! -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
-
Re: trivial grammar refactor
Andres Freund <andres@anarazel.de> — 2025-07-23T21:42:35Z
Hi, On 2025-07-23 19:59:52 +0200, Álvaro Herrera wrote: > ... so using the same set of productions, I can rewrite the current > CLUSTER rule in this way and it won't be a problem for the REPACK > changes. But it comes at the price of henceforth duplicating all ClusterStmt, once for VERBOSE and once without. That's not exactly a free lunch... Greetings, Andres Freund
-
Re: trivial grammar refactor
Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-24T09:54:10Z
Hello, On 2025-Jul-23, Andres Freund wrote: > On 2025-07-23 19:59:52 +0200, Álvaro Herrera wrote: > > ... so using the same set of productions, I can rewrite the current > > CLUSTER rule in this way and it won't be a problem for the REPACK > > changes. > > But it comes at the price of henceforth duplicating all ClusterStmt, once for > VERBOSE and once without. That's not exactly a free lunch... Yeah, thanks for taking a look. That duplication is just me being dumb. Here's a version without that. The only thing that needed to change was changing "CLUSTER opt_verbose" to "CLUSTER VERBOSE" so that the unadorned CLUSTER is handled by "CLUSTER opt_utility_option_list" instead. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "How amazing is that? I call it a night and come back to find that a bug has been identified and patched while I sleep." (Robert Davidson) http://archives.postgresql.org/pgsql-sql/2006-03/msg00378.php -
Re: trivial grammar refactor
Nathan Bossart <nathandbossart@gmail.com> — 2025-07-24T14:31:32Z
On Thu, Jul 24, 2025 at 11:54:10AM +0200, Álvaro Herrera wrote: > Yeah, thanks for taking a look. That duplication is just me being dumb. > Here's a version without that. The only thing that needed to change was > changing "CLUSTER opt_verbose" to "CLUSTER VERBOSE" so that the > unadorned CLUSTER is handled by "CLUSTER opt_utility_option_list" > instead. I think we can do something similar for ANALYZE. But AFAICT you're right that we can't use it for VACUUM and EXPLAIN, at least not easily. diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index fc9a8d64c08..7d341a319e7 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -11987,24 +11987,21 @@ VacuumStmt: VACUUM opt_full opt_freeze opt_verbose opt_analyze opt_vacuum_relati } ; -AnalyzeStmt: analyze_keyword opt_verbose opt_vacuum_relation_list +AnalyzeStmt: analyze_keyword opt_utility_option_list opt_vacuum_relation_list { VacuumStmt *n = makeNode(VacuumStmt); - n->options = NIL; - if ($2) - n->options = lappend(n->options, - makeDefElem("verbose", NULL, @2)); + n->options = $2; n->rels = $3; n->is_vacuumcmd = false; $$ = (Node *) n; } - | analyze_keyword '(' utility_option_list ')' opt_vacuum_relation_list + | analyze_keyword VERBOSE opt_vacuum_relation_list { VacuumStmt *n = makeNode(VacuumStmt); - n->options = $3; - n->rels = $5; + n->options = list_make1(makeDefElem("verbose", NULL, @2)); + n->rels = $3; n->is_vacuumcmd = false; $$ = (Node *) n; } -- nathan -
Re: trivial grammar refactor
Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-25T10:25:15Z
On 2025-Jul-24, Nathan Bossart wrote: > On Thu, Jul 24, 2025 at 11:54:10AM +0200, Álvaro Herrera wrote: > > Yeah, thanks for taking a look. That duplication is just me being dumb. > > Here's a version without that. The only thing that needed to change was > > changing "CLUSTER opt_verbose" to "CLUSTER VERBOSE" so that the > > unadorned CLUSTER is handled by "CLUSTER opt_utility_option_list" > > instead. > > I think we can do something similar for ANALYZE. But AFAICT you're right > that we can't use it for VACUUM and EXPLAIN, at least not easily. Thank you! Pushed with that change. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/