Re: [HACKERS] [Proposal] Allow users to specify multiple tables inVACUUM commands - Mailing list pgsql-hackers

From Michael Paquier
Subject Re: [HACKERS] [Proposal] Allow users to specify multiple tables inVACUUM commands
Date
Msg-id CAB7nPqTYbJRU14SG0qwueTLbZHutZ8OWCV0L9NiK1MQ_nzqCkw@mail.gmail.com
Whole thread Raw
In response to Re: [HACKERS] [Proposal] Allow users to specify multiple tables inVACUUM commands  ("Bossart, Nathan" <bossartn@amazon.com>)
Responses Re: [HACKERS] [Proposal] Allow users to specify multiple tables inVACUUM commands  (Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>)
Re: [HACKERS] [Proposal] Allow users to specify multiple tables inVACUUM commands  ("Bossart, Nathan" <bossartn@amazon.com>)
List pgsql-hackers
On Wed, Sep 13, 2017 at 12:31 AM, Bossart, Nathan <bossartn@amazon.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
acrossour * 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

pgsql-hackers by date:

Previous
From: Kyotaro HORIGUCHI
Date:
Subject: Re: [HACKERS] Restricting maximum keep segments by repslots
Next
From: Amit Langote
Date:
Subject: Re: [HACKERS] Setting pd_lower in GIN metapage