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
