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

From: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
To: Robert Haas <robertmhaas(at)gmail(dot)com>
Cc: "Bossart, Nathan" <bossartn(at)amazon(dot)com>, "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:28:17
Message-ID: CAB7nPqQLnDxu+7mXK-YZ-zCWxQovtOvAv4cyzg6QHkFsDOeL1Q@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Sat, Aug 26, 2017 at 8:00 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Aug 24, 2017 at 12:59 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)com> wrote:
>> 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.
>
> So ... why have a double loop? I mean, you could just expand this out
> to one entry per relation actually being vacuumed, couldn't you?

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.

> + 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?

Yup.
--
Michael

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2017-08-28 22:35:17 Re: [HACKERS] [postgresql 10 beta3] unrecognized node type: 90
Previous Message Robert Haas 2017-08-28 22:03:42 Re: [HACKERS] [postgresql 10 beta3] unrecognized node type: 90