trivial grammar refactor

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

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)

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

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)

... 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/

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

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

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

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/