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>
Cc: "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, "Masahiko Sawada" <sawada(dot)mshk(at)gmail(dot)com>, "David G(dot) Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: Re: [Proposal] Allow users to specify multiple tables in VACUUM commands
Date: 2017-09-13 04:12:12
Message-ID: C237BB73-E7F3-4AC9-8B88-C8810EEB002D@amazon.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On 9/12/17, 9:47 PM, "Michael Paquier" <michael(dot)paquier(at)gmail(dot)com> wrote:
> - * 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.

I've added a comment to this effect in v18 of the main patch.

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

True. I've removed "i" in v18.

> /*
> - * 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".

I've added some more accurate comments for get_rel_oids() in v18.

> 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.

Awesome.

Nathan

Attachment Content-Type Size
0002-error_on_duplicate_columns_in_analyze_v5.patch application/octet-stream 6.1 KB
0001-vacuum_multiple_tables_v18.patch application/octet-stream 34.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2017-09-13 04:12:54 Re: [PATCH] Call RelationDropStorage() for broader range of object drops.
Previous Message Amit Langote 2017-09-13 04:10:31 Re: Race between SELECT and ALTER TABLE NO INHERIT