On Fri, May 12, 2017 at 9:47 AM, Bossart, Nathan <bossa...@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, ¶ms,
- vacstmt->va_cols, NULL, isTopLevel);
+ vacuum(vacstmt->options, vacstmt->rels, InvalidOid, ¶ms,
+ 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
