Обсуждение: trivial grammar refactor

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/

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/