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

On 8/23/17, 11:59 PM, "Michael Paquier" <michael.paquier@gmail.com> wrote:
> + * relations is a list of VacuumRelations to process.  If it is NIL, all
> + * relations in the database are processed.
> Typo here, VacuumRelation is singular.

This should be fixed in v9.

On 8/24/17, 5:45 PM, "Michael Paquier" <michael.paquier@gmail.com> wrote:
> This makes me think that it could be a good idea to revisit this bit
> in a separate patch. ANALYZE fails as well now when the same column is
> defined multiple times with an incomprehensible error message.

The de-duplication code is now in a separate patch,
dedupe_vacuum_relations_v1.patch.  I believe it fixes the incomprehensible
error message you were experiencing, but please let me know if you are
still hitting it.

On 8/25/17, 6:00 PM, "Robert Haas" <robertmhaas@gmail.com> wrote:
> +    oldcontext = MemoryContextSwitchTo(vac_context);
> +    foreach(lc, relations)
> +        temp_relations = lappend(temp_relations, copyObject(lfirst(lc)));
> +    MemoryContextSwitchTo(oldcontext);
> +    relations = temp_relations;
>
> Can't we just copyObject() on the whole list?

I've made this change.

> -        ListCell   *cur;
> -
>
> Why change this?  Generally, declaring a separate variable in an inner
> scope seems like better style than reusing one that happens to be
> lying around in the outer scope.

I've removed this change.

> +            VacuumRelation *relinfo = (VacuumRelation *) lfirst(lc);
>
> Could use lfirst_node.

Done.

On 8/28/17, 5:28 PM, "Michael Paquier" <michael.paquier@gmail.com> wrote:
> Yes, if I understand that correctly. That's the point I am exactly
> coming at. My suggestion is to have one VacuumRelation entry per
> relation vacuumed, even for partitioned tables, and copy the list of
> columns to each one.

I've made this change in v9.  It does clean up the patch quite a bit.

Nathan