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

On Fri, May 12, 2017 at 9:47 AM, Bossart, Nathan <bossartn(at)amazon(dot)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