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

On Thu, Aug 24, 2017 at 12:38 PM, Bossart, Nathan <bossartn@amazon.com> wrote:
> On 8/18/17, 12:56 AM, "Michael Paquier" <michael.paquier@gmail.com> wrote:
> According to the docs, VACUUM and ANALYZE "do not support recursing over
> inheritance hierarchies" [1].  However, we need a list of OIDs for
> partitioned tables.  Namely, this piece of code in get_rel_oids(...):
>
>                 if (include_parts)
>                         oid_list = list_concat(oid_list,
>                                                                    find_all_inheritors(relid, NoLock, NULL));
>                 else
>                         oid_list = lappend_oid(oid_list, relid);
>
> Since all of these OIDs should correspond to the same partitioned table,
> it makes sense to me to leave them together instead of breaking out each
> partition into a VacuumRelation.  If we did so, I think we would also need
> to duplicate the va_cols list for each partition.  What do you think?

Robert, Amit and other folks working on extending the existing
partitioning facility would be in better position to answer that, but
I would think that we should have something as flexible as possible,
and storing a list of relation OID in each VacuumRelation makes it
harder to track the uniqueness of relations vacuumed. I agree that the
concept of a partition with multiple parents induces a lot of
problems. But the patch as proposed worries me as it complicates
vacuum() with a double loop: one for each relation vacuumed, and one
inside it with the list of OIDs. Modules calling vacuum() could also
use flexibility, being able to analyze a custom list of columns for
each relation has value as well.

>> - * relid, if not InvalidOid, indicate the relation to process; otherwise,
>> - * the RangeVar is used.  (The latter must always be passed, because it's
>> - * used for error messages.)
>> + * If we intend to process all relations, the 'relations' argument may be
>> + * NIL.
>> This comment actually applies to RelationAndColumns. If the OID is
>> invalid, then RangeVar is used, and should always be set. You are
>> breaking that promise actually for autovacuum. The comment here should
>> say that if relations is NIL all the relations of the database are
>> processes, and for an ANALYZE all the columns are done.
>
> Makes sense, I've tried to make this comment clearer.

+ * relations is a list of VacuumRelations to process.  If it is NIL, all
+ * relations in the database are processed.
Typo here, VacuumRelation is singular.
-- 
Michael