Hello

I started to read through 0001 and my first reaction is that I would
like to make a few more breaking changes.  It appears that the current
patch tries to keep things unchanged, or to keep some things with the
CLUSTER name.  I'm going to try and get rid of that.  For instance, in
the grammar, I'm going to change the productions for CLUSTER so that
they produce a RepackStmt of the appropriate form; ClusterStmt as a
parse node should just be removed as no longer useful.  Also, the
cluster() function becomes ExecRepack(), and things flow down from
there.

This leads to a small problem: right now you can say "CLUSTER;" which
processes all tables for which a clustered index has been defined.
In the submitted patch, REPACK doesn't support that mode, and we need a
way to distinguish the mode where it VACUUM FULL all tables from the
mode where it does the all-table CLUSTER.  So I propose we allow that
mode with simply
  CLUSTER USING INDEX;
which is consistent with the idea of "REPACK table USING INDEX foo"
being "CLUSTER table USING foo".

Implementation-wise, I'm toying with adding a new "command" for REPACK
called REPACK_COMMAND_CLUSTER_ALL, supporting this mode of operation.

That leads to a grammar like

> RepackStmt:
>                       REPACK USING INDEX
>                               {
>                                       RepackStmt *n = makeNode(RepackStmt);
> 
>                                       n->command = REPACK_COMMAND_CLUSTER_ALL;
>                                       n->relation = NULL;
>                                       n->indexname = NULL;
>                                       n->params = NIL;
>                                       $$ = (Node *) n;
>                               }
>                       | REPACK qualified_name opt_using_index
>                               {
>                                       RepackStmt *n = makeNode(RepackStmt);
> 
>                                       n->command = REPACK_COMMAND_REPACK;
>                                       n->relation = $2;
>                                       n->indexname = $3;
>                                       n->params = NIL;
>                                       $$ = (Node *) n;
>                               }
>                       | REPACK '(' utility_option_list ')' qualified_name 
> opt_using_index
>                               {
>                                       RepackStmt *n = makeNode(RepackStmt);
> 
>                                       n->command = REPACK_COMMAND_REPACK;
>                                       n->relation = $5;
>                                       n->indexname = $6;
>                                       n->params = $3;
>                                       $$ = (Node *) n;
>                               }
>                       | CLUSTER '(' utility_option_list ')'
>                               {
>                                       RepackStmt *n = makeNode(RepackStmt);
> 
>                                       n->command = REPACK_COMMAND_CLUSTER_ALL;
>                                       n->relation = NULL;
>                                       n->indexname = NULL;
>                                       n->params = $3;
>                                       $$ = (Node *) n;
>                               }
>                       | CLUSTER '(' utility_option_list ')' qualified_name 
> cluster_index_specification
>                               {
>                                       RepackStmt *n = makeNode(RepackStmt);
> 
>                                       n->command = REPACK_COMMAND_CLUSTER;
>                                       n->relation = $5;
>                                       n->indexname = $6;
>                                       n->params = $3;
>                                       $$ = (Node *) n;
>                               }
>                       /* unparenthesized VERBOSE kept for pre-14 
> compatibility */
>                       | CLUSTER opt_verbose qualified_name 
> cluster_index_specification
>                               {
>                                       RepackStmt *n = makeNode(RepackStmt);
> 
>                                       n->command = REPACK_COMMAND_CLUSTER;
>                                       n->relation = $3;
>                                       n->indexname = $4;
>                                       n->params = NIL;
>                                       if ($2)
>                                               n->params = lappend(n->params, 
> makeDefElem("verbose", NULL, @2));
>                                       $$ = (Node *) n;
>                               }
>                       /* unparenthesized VERBOSE kept for pre-17 
> compatibility */
>                       | CLUSTER opt_verbose
>                               {
>                                       RepackStmt *n = makeNode(RepackStmt);
> 
>                                       n->command = REPACK_COMMAND_CLUSTER_ALL;
>                                       n->relation = NULL;
>                                       n->indexname = NULL;
>                                       n->params = NIL;
>                                       if ($2)
>                                               n->params = lappend(n->params, 
> makeDefElem("verbose", NULL, @2));
>                                       $$ = (Node *) n;
>                               }
>                       /* kept for pre-8.3 compatibility */
>                       | CLUSTER opt_verbose name ON qualified_name
>                               {
>                                       RepackStmt *n = makeNode(RepackStmt);
> 
>                                       n->command = REPACK_COMMAND_CLUSTER;
>                                       n->relation = $5;
>                                       n->indexname = $3;
>                                       n->params = NIL;
>                                       if ($2)
>                                               n->params = lappend(n->params, 
> makeDefElem("verbose", NULL, @2));
>                                       $$ = (Node *) n;
>                               }
>               ;
> 
> opt_using_index:
>                       ExistingIndex                   { $$ = $1; }
>                       | /*EMPTY*/                             { $$ = NULL; }
>               ;
> 
> cluster_index_specification:
>                       USING name                              { $$ = $2; }
>                       | /*EMPTY*/                             { $$ = NULL; }
>               ;

It's a bit weird that CLUSTER uses just "USING" while REPACK uses
"USING INDEX", but of course we cannot change CLUSTER now; and I think
it's better to have the noise word INDEX for REPACK because it allows
the case of not specifying an index name as described above.

In the current patch we don't yet have a way to use REPACK for an
unadorned "VACUUM FULL" (which processes all tables), but I think I'll
add that as well, if only so that we can claim that the REPACK commands
handles all possible legacy command modes, even if they are not useful
in practice.  That would probably be unadorned "REPACK;".  With this, we
can easily state that all legacy CLUSTER and VACUUM FULL commands have
an equivalent REPACK formulation.

We're of course not going to _remove_ support for any of those legacy
commands.  At the same time, we're not planning to add support for
CONCURRENTLY in the all-tables modes (patch 0004), but I don't think
that's a concern.  Somebody could later implement that if they wanted
to, but I think it's pretty useless so IMO it's a waste of time.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Hay quien adquiere la mala costumbre de ser infeliz" (M. A. Evans)


Reply via email to