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

From: "Bossart, Nathan" <bossartn(at)amazon(dot)com>
To: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
Subject: Re: [Proposal] Allow users to specify multiple tables in VACUUM commands
Date: 2017-08-28 22:56:14
Message-ID: 39FA1BCE-E0AF-4F54-93A0-BECADA95F062@amazon.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On 8/23/17, 11:59 PM, "Michael Paquier" <michael(dot)paquier(at)gmail(dot)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(dot)paquier(at)gmail(dot)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(at)gmail(dot)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(dot)paquier(at)gmail(dot)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

Attachment Content-Type Size
dedupe_vacuum_relations_v1.patch application/octet-stream 4.7 KB
vacuum_multiple_tables_v9.patch application/octet-stream 30.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bruce Momjian 2017-08-28 23:05:51 Re: Challenges preventing us moving to 64 bit transaction id (XID)?
Previous Message Tom Lane 2017-08-28 22:35:17 Re: [HACKERS] [postgresql 10 beta3] unrecognized node type: 90