Thread: Move un-parenthesized syntax docs to "compatibility" for few SQL commands

Hi,

I find the inclusion of the un-parenthesized syntax for VACUUM, ANALYZE,
and EXPLAIN in the docs makes it harder to understand the preferred,
parenthesized syntax (see [1] as an example).

Over in [2], it was suggested that moving the un-parenthesized syntax to
the "Compatibility" section of their respective docs pages would be
okay. Patch attached.

I left out CLUSTER since that syntax change is so new.

- Melanie

[1] https://www.postgresql.org/docs/devel/sql-analyze.html
[2] https://www.postgresql.org/message-id/3024596.1681940741%40sss.pgh.pa.us

On Fri, Apr 21, 2023 at 06:29:16PM -0400, Melanie Plageman wrote:
> Over in [2], it was suggested that moving the un-parenthesized syntax to
> the "Compatibility" section of their respective docs pages would be
> okay. Patch attached.

I think that's reasonable.

> I left out CLUSTER since that syntax change is so new.

I think it'd be okay to move this one, too.  The parenthesized syntax has
been available since v14, and I suspect CLUSTER isn't terribly widely used,
anyway.  (Side note: it looks like "CLUSTER (VERBOSE)" doesn't work, which
seems weird to me.)

Most of these commands have an existing note about the deprecated syntax.
Could those be removed with this change?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, Apr 21, 2023 at 6:55 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Fri, Apr 21, 2023 at 06:29:16PM -0400, Melanie Plageman wrote:
> > Over in [2], it was suggested that moving the un-parenthesized syntax to
> > the "Compatibility" section of their respective docs pages would be
> > okay. Patch attached.
>
> I think that's reasonable.
>
> > I left out CLUSTER since that syntax change is so new.
>
> I think it'd be okay to move this one, too.  The parenthesized syntax has
> been available since v14, and I suspect CLUSTER isn't terribly widely used,
> anyway.  (Side note: it looks like "CLUSTER (VERBOSE)" doesn't work, which
> seems weird to me.)

Attached v2 includes changes for CLUSTER syntax docs. I wasn't quite
sure that we can move down CLUSTER [VERBOSE] syntax to the compatibility
section since CLUSTER (VERBOSE) doesn't work. This seems like it should
work (VACUUM and ANALYZE do). I put extra "[ ]" around the main CLUSTER
syntax but I don't know that this is actually the same as documenting
that CLUSTER VERBOSE does work but CLUSTER (VERBOSE) doesn't.

> Most of these commands have an existing note about the deprecated syntax.
> Could those be removed with this change?

Ah, yes. Good catch! I have removed these.

- Melanie

On Fri, Apr 21, 2023 at 07:29:59PM -0400, Melanie Plageman wrote:
> Attached v2 includes changes for CLUSTER syntax docs. I wasn't quite
> sure that we can move down CLUSTER [VERBOSE] syntax to the compatibility
> section since CLUSTER (VERBOSE) doesn't work. This seems like it should
> work (VACUUM and ANALYZE do). I put extra "[ ]" around the main CLUSTER
> syntax but I don't know that this is actually the same as documenting
> that CLUSTER VERBOSE does work but CLUSTER (VERBOSE) doesn't.

AFAICT there isn't a strong reason to disallow CLUSTER (VERBOSE).  The
following short patch seems to do the trick:

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index acf6cf4866..215c4ba39c 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -11603,6 +11603,15 @@ ClusterStmt:
                     n->params = $3;
                     $$ = (Node *) n;
                 }
+            | CLUSTER '(' utility_option_list ')'
+                {
+                    ClusterStmt *n = makeNode(ClusterStmt);
+
+                    n->relation = NULL;
+                    n->indexname = NULL;
+                    n->params = $3;
+                    $$ = (Node *) n;
+                }
             | CLUSTER opt_verbose
                 {
                     ClusterStmt *n = makeNode(ClusterStmt);

>> Most of these commands have an existing note about the deprecated syntax.
>> Could those be removed with this change?
> 
> Ah, yes. Good catch! I have removed these.

Thanks.  I'll take a closer look at the patch soon.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

I've attached two patches.  0001 adds a parenthesized CLUSTER syntax that
doesn't require a table name.  0002 is your patch with a couple of small
adjustments.

On Fri, Apr 21, 2023 at 07:29:59PM -0400, Melanie Plageman wrote:
> Attached v2 includes changes for CLUSTER syntax docs. I wasn't quite
> sure that we can move down CLUSTER [VERBOSE] syntax to the compatibility
> section since CLUSTER (VERBOSE) doesn't work. This seems like it should
> work (VACUUM and ANALYZE do). I put extra "[ ]" around the main CLUSTER
> syntax but I don't know that this is actually the same as documenting
> that CLUSTER VERBOSE does work but CLUSTER (VERBOSE) doesn't.

I'm not sure about moving CLUSTER [VERBOSE] to the compatibility section,
either.  At the very least, I don't think what I have in 0002 is accurate.
It claims that syntax was used before v14, but it's still the only way to
do a "verbose" CLUSTER without a table name in v15 and v16, too.  Perhaps
we should break it apart, or maybe we can just say it was used before v17.
WDYT?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, Apr 21, 2023 at 09:44:51PM -0700, Nathan Bossart wrote:
> I've attached two patches.  0001 adds a parenthesized CLUSTER syntax that
> doesn't require a table name.  0002 is your patch with a couple of small
> adjustments.
> 
> On Fri, Apr 21, 2023 at 07:29:59PM -0400, Melanie Plageman wrote:
> > Attached v2 includes changes for CLUSTER syntax docs. I wasn't quite
> > sure that we can move down CLUSTER [VERBOSE] syntax to the compatibility
> > section since CLUSTER (VERBOSE) doesn't work. This seems like it should
> > work (VACUUM and ANALYZE do). I put extra "[ ]" around the main CLUSTER
> > syntax but I don't know that this is actually the same as documenting
> > that CLUSTER VERBOSE does work but CLUSTER (VERBOSE) doesn't.
> 
> I'm not sure about moving CLUSTER [VERBOSE] to the compatibility section,
> either.  At the very least, I don't think what I have in 0002 is accurate.
> It claims that syntax was used before v14, but it's still the only way to
> do a "verbose" CLUSTER without a table name in v15 and v16, too.  Perhaps
> we should break it apart, or maybe we can just say it was used before v17.
> WDYT?

If you are planning to wait and commit the change to support CLUSTER
(VERBOSE) until July, then you can consolidate the two and say before
v17. If you plan to commit it before then (making it available in v16),
I would move CLUSTER [VERBOSE] down to Compatibility and say that both
of the un-parenthesized syntaxes were used before v16. Since all of the
syntaxes still work, I think it is probably more confusing to split them
apart so granularly. The parenthesized syntax was effectively not
"feature complete" without your patch to support CLUSTER (VERBOSE).

Also, isn't this:
  CLUSTER [VERBOSE] [ <qualified_name> [ USING <index_name> ] ]
inclusive of this:
  CLUSTER [VERBOSE]
So, it would have been correct for them to be consolidated in the
existing documentation?

> From 48fff177a8f0096c99c77b4e1368cc73f7e86585 Mon Sep 17 00:00:00 2001
> From: Nathan Bossart <nbossart@postgresql.org>
> Date: Fri, 21 Apr 2023 20:16:25 -0700
> Subject: [PATCH v3 1/2] Support parenthesized syntax for CLUSTER without a
>  table name.
> 
> b5913f6 added a parenthesized syntax for CLUSTER, but it requires
> specifying a table name.  This is unlike commands such as VACUUM
> and ANALYZE, which do not require specifying a table in the
> parenthesized syntax.  This change resolves this inconsistency.
> 
> The documentation for the CLUSTER syntax has also been consolidated
> in anticipation of a follow-up change that will move the
> unparenthesized syntax to the "Compatibility" section.

I suppose we should decide if unparenthesized is a word or if we are
sticking with the hyphen.

>  doc/src/sgml/ref/cluster.sgml |  5 ++---
>  src/backend/parser/gram.y     | 21 ++++++++++++++-------
>  2 files changed, 16 insertions(+), 10 deletions(-)
> 
> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y
...
> @@ -11593,7 +11592,17 @@ ClusterStmt:
>                          n->params = lappend(n->params, makeDefElem("verbose", NULL, @2));
>                      $$ = (Node *) n;
>                  }
> +            | CLUSTER opt_verbose
> +                {
> +                    ClusterStmt *n = makeNode(ClusterStmt);
>  
> +                    n->relation = NULL;
> +                    n->indexname = NULL;
> +                    n->params = NIL;
> +                    if ($2)
> +                        n->params = lappend(n->params, makeDefElem("verbose", NULL, @2));
> +                    $$ = (Node *) n;
> +                }

Maybe it is worth moving the un-parenthesized options all to the end and
specifying what version they were needed for.


>              | CLUSTER '(' utility_option_list ')' qualified_name cluster_index_specification
>                  {
>                      ClusterStmt *n = makeNode(ClusterStmt);
> @@ -11603,15 +11612,13 @@ ClusterStmt:
>                      n->params = $3;
>                      $$ = (Node *) n;
>                  }
> -            | CLUSTER opt_verbose
> +            | CLUSTER '(' utility_option_list ')'

It is too bad we can't do this the way VACUUM and ANALYZE do -- but
since qualified_name is required if USING is included, I suppose we
can't.

> diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml
> index 20c6f9939f..ea42ec30bd 100644
> --- a/doc/src/sgml/ref/analyze.sgml
> +++ b/doc/src/sgml/ref/analyze.sgml
> @@ -22,7 +22,6 @@ PostgreSQL documentation
>   <refsynopsisdiv>
>  <synopsis>
>  ANALYZE [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] [ <replaceable
class="parameter">table_and_columns</replaceable>[, ...] ]
 
> -ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replaceable> [, ...] ]
...
> @@ -346,6 +338,14 @@ ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replacea
>    <para>
>     There is no <command>ANALYZE</command> statement in the SQL standard.
>    </para>
> +
> +  <para>
> +   The following syntax was used before <productname>PostgreSQL</productname>
> +   version 11 and is still supported:

Good call on specifying that the order matters.

> +<synopsis>
> +ANALYZE [ VERBOSE ] [ <replaceable class="parameter">table_and_columns</replaceable> [, ...] ]
> +</synopsis>
> +  </para>

- Melanie

On Sat, Apr 22, 2023 at 10:38:58AM -0400, Melanie Plageman wrote:
> If you are planning to wait and commit the change to support CLUSTER
> (VERBOSE) until July, then you can consolidate the two and say before
> v17. If you plan to commit it before then (making it available in v16),
> I would move CLUSTER [VERBOSE] down to Compatibility and say that both
> of the un-parenthesized syntaxes were used before v16. Since all of the
> syntaxes still work, I think it is probably more confusing to split them
> apart so granularly. The parenthesized syntax was effectively not
> "feature complete" without your patch to support CLUSTER (VERBOSE).

I think this can wait for v17, but if there's a strong argument for doing
some of this sooner, we can reevaluate.

> Also, isn't this:
>   CLUSTER [VERBOSE] [ <qualified_name> [ USING <index_name> ] ]
> inclusive of this:
>   CLUSTER [VERBOSE]
> So, it would have been correct for them to be consolidated in the
> existing documentation?

Yes.  This appears to go pretty far back.  I traced it to 8bc717c (2002).
It looks like the VACUUM syntaxes have been consolidated since 37d2f76
(1998).  So AFAICT this small inconsistency has been around for a while.

>> The documentation for the CLUSTER syntax has also been consolidated
>> in anticipation of a follow-up change that will move the
>> unparenthesized syntax to the "Compatibility" section.
> 
> I suppose we should decide if unparenthesized is a word or if we are
> sticking with the hyphen.

The existing uses in the docs all omit the hypen, but I see it both ways in
some web searches.  Other than keeping the Postgres docs consistent, I
don't have a terribly ѕtrong opinion here.

>> +            | CLUSTER opt_verbose
>> +                {
>> +                    ClusterStmt *n = makeNode(ClusterStmt);
>>  
>> +                    n->relation = NULL;
>> +                    n->indexname = NULL;
>> +                    n->params = NIL;
>> +                    if ($2)
>> +                        n->params = lappend(n->params, makeDefElem("verbose", NULL, @2));
>> +                    $$ = (Node *) n;
>> +                }
> 
> Maybe it is worth moving the un-parenthesized options all to the end and
> specifying what version they were needed for.

Good idea.

>>              | CLUSTER '(' utility_option_list ')' qualified_name cluster_index_specification
>>                  {
>>                      ClusterStmt *n = makeNode(ClusterStmt);
>> @@ -11603,15 +11612,13 @@ ClusterStmt:
>>                      n->params = $3;
>>                      $$ = (Node *) n;
>>                  }
>> -            | CLUSTER opt_verbose
>> +            | CLUSTER '(' utility_option_list ')'
> 
> It is too bad we can't do this the way VACUUM and ANALYZE do -- but
> since qualified_name is required if USING is included, I suppose we
> can't.

It might be possible to extract the name and index part to a separate
optional rule.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Mon, Apr 24, 2023 at 09:52:21AM -0700, Nathan Bossart wrote:
> I think this can wait for v17, but if there's a strong argument for doing
> some of this sooner, we can reevaluate.

FWIW, I agree to hold on this stuff for v17~ once it opens for
business.  There is no urgency here.
--
Michael

On Tue, Apr 25, 2023 at 03:18:44PM +0900, Michael Paquier wrote:
> On Mon, Apr 24, 2023 at 09:52:21AM -0700, Nathan Bossart wrote:
>> I think this can wait for v17, but if there's a strong argument for doing
>> some of this sooner, we can reevaluate.
> 
> FWIW, I agree to hold on this stuff for v17~ once it opens for
> business.  There is no urgency here.

There's still some time before we'll be able to commit any of these, but
here is an attempt at addressing all the feedback thus far.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Mon, May 15, 2023 at 12:36:51PM -0700, Nathan Bossart wrote:
> There's still some time before we'll be able to commit any of these, but
> here is an attempt at addressing all the feedback thus far.

-   The parenthesized syntax was added in
-   <productname>PostgreSQL</productname> 9.0;  the unparenthesized
-   syntax is deprecated.
[...]
+           | CLUSTER '(' utility_option_list ')'
+               {
+                   ClusterStmt *n = makeNode(ClusterStmt);
+
+                   n->relation = NULL;
+                   n->indexname = NULL;
+                   n->params = $3;
+                   $$ = (Node *) n;
+               }

Hmm.  This is older than the oldest version we have to support for
pg_upgrade and co.  Would it be worth switching clusterdb to use the
parenthesized grammar, adding on the way a test for this new grammar
flavor without a table in the TAP tests (too costly for the main
regression test suite)?
--
Michael

On Tue, May 16, 2023 at 03:28:17PM +0900, Michael Paquier wrote:
> Hmm.  This is older than the oldest version we have to support for
> pg_upgrade and co.  Would it be worth switching clusterdb to use the
> parenthesized grammar, adding on the way a test for this new grammar
> flavor without a table in the TAP tests (too costly for the main
> regression test suite)?

Since all currently-supported versions require a table name for the
parenthesized syntax, this would cause v17's clusterdb to fail on older
servers when no table name is specified.  That seems like a deal-breaker to
me.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Mon, May 15, 2023 at 12:36:51PM -0700, Nathan Bossart wrote:
> On Tue, Apr 25, 2023 at 03:18:44PM +0900, Michael Paquier wrote:
>> FWIW, I agree to hold on this stuff for v17~ once it opens for
>> business.  There is no urgency here.
> 
> There's still some time before we'll be able to commit any of these, but
> here is an attempt at addressing all the feedback thus far.

I think these patches are in decent shape, so I'd like to commit them soon,
but I will wait at least a couple more weeks in case anyone has additional
feedback.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, Jun 30, 2023 at 11:25:57AM -0700, Nathan Bossart wrote:
> I think these patches are in decent shape, so I'd like to commit them soon,
> but I will wait at least a couple more weeks in case anyone has additional
> feedback.

Committed.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com