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

On Wed, Sep 13, 2017 at 12:31 AM, Bossart, Nathan <bossartn(at)amazon(dot)com> wrote:
> Sorry for the spam. I am re-sending these patches with modified names so that
> the apply order is obvious to the new automated testing framework (and to
> everybody else).

- * 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.)
[...]
+typedef struct VacuumRelation
+{
+ NodeTag type;
+ RangeVar *relation; /* single table to process */
+ List *va_cols; /* list of column names, or NIL for all */
+ Oid oid; /* corresponding OID (filled in by [auto]vacuum.c) */
+} VacuumRelation;
We lose a bit of information here. I think that it would be good to
mention in the declaration of VacuumRelation that the RangeVar is used
for error processing, and needs to be filled. I have complained about
that upthread already, perhaps this has slipped away when rebasing.

+ int i = attnameAttNum(rel, col, false);
+
+ if (i != InvalidAttrNumber)
+ continue;
Nit: allocating "i" makes little sense here. You are not using it for
any other checks.

/*
- * Build a list of Oids for each relation to be processed
+ * Determine the OID for each relation to be processed
*
* The list is built in vac_context so that it will survive across our
* per-relation transactions.
*/
-static List *
-get_rel_oids(Oid relid, const RangeVar *vacrel)
+static void
+get_rel_oids(List **vacrels)
Yeah, that's not completely correct either. This would be more like
"Fill in the list of VacuumRelation entries with their corresponding
OIDs, adding extra entries for partitioned tables".

Those are minor points. The patch seems to be in good shape, and
passes all my tests, including some pgbench'ing to make sure that
nothing goes weird. So I'll be happy to finally switch both patches to
"ready for committer" once those minor points are addressed.
--
Michael