Re: [Proposal] Allow users to specify multiple tables in VACUUM commands

On Fri, May 12, 2017 at 9:47 AM, Bossart, Nathan <bossartn@amazon.com> wrote:
> Attached is a more complete first attempt at adding this functionality.  I added two node types: one for parsing the “relation and columns” list in the grammar, and one for holding the relation information we need for each call to vacuum_rel(…)/analyze_rel(…).  I also added assertions and comments for some undocumented assumptions that we currently rely upon.
>
> Adjustments to the documentation for VACUUM/ANALYZE and new checks in the VACUUM regression test are included in this patch as well.
>
> Looking forward to any feedback that you have.

Browsing the code....

 <synopsis>
-ANALYZE [ VERBOSE ] [ <replaceable
class="PARAMETER">table_name</replaceable> [ ( <replaceable
class="PARAMETER">column_name</replaceable> [, ...] ) ] ]
+ANALYZE [ VERBOSE ] [ <replaceable
class="PARAMETER">table_name</replaceable> [ [ ( <replaceable
class="PARAMETER">column_name</replaceable> [, ...] ) ] [, ...] ]
 </synopsis>
It seems to me that you don't need those extra square brackets around
the column list. Same remark for vacuum.sgml.

     <listitem>
      <para>
-      The name (possibly schema-qualified) of a specific table to
+      The name (possibly schema-qualified) of the specific tables to
       analyze.  If omitted, all regular tables, partitioned tables, and
       materialized views in the current database are analyzed (but not
-      foreign tables).  If the specified table is a partitioned table, both the
+      foreign tables).  If a specified table is a partitioned table, both the
       inheritance statistics of the partitioned table as a whole and
       statistics of the individual partitions are updated.
      </para>
Don't think that's needed. table_name is still referencing a single
table name. And similar remark for vacuum.sgml.

In short for all that, it is enough to have "[, ... ]" to document
that a list is accepted.

    /* Now go through the common routine */
-   vacuum(vacstmt->options, vacstmt->relation, InvalidOid, &params,
-          vacstmt->va_cols, NULL, isTopLevel);
+   vacuum(vacstmt->options, vacstmt->rels, InvalidOid, &params,
+          NULL, isTopLevel);
 }
It seems to me that it would have been less invasive to loop through
vacuum() for each relation. Do you foresee advantages in allowing
vacuum() to handle multiple? I am not sure if is is worth complicating
the current logic more considering that you have as well so extra
logic to carry on option values.

+ * used for error messages.  In the case that relid is valid, rels
+ * must have exactly one element corresponding to the specified relid.
s/rels/relations/ as variable name?
-- 
Michael