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: 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>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, 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-01 18:00:51
Message-ID: 8F20FD46-0833-4A58-A266-668B4390407D@amazon.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

On 9/1/17, 12:11 AM, "Michael Paquier" <michael(dot)paquier(at)gmail(dot)com> wrote:
> Hm. Here is the diff between v11 and v12:
> static void
> autovacuum_do_vac_analyze(autovac_table *tab, BufferAccessStrategy bstrategy)
> {
> - RangeVar rangevar;
> - VacuumRelation *rel;
> -
> - /* Set up command parameters --- use local variables instead of palloc */
> - MemSet(&rangevar, 0, sizeof(rangevar));
> -
> - rangevar.schemaname = tab->at_nspname;
> - rangevar.relname = tab->at_relname;
> - rangevar.location = -1;
> + RangeVar *rangevar;
> + VacuumRelation *rel;
>
> /* Let pgstat know what we're doing */
> autovac_report_activity(tab);
>
> - rel = makeVacuumRelation(&rangevar, NIL, tab->at_relid);
> + rangevar = makeRangeVar(tab->at_nspname, tab->at_relname, -1);
> + rel = makeVacuumRelation(rangevar, NIL, tab->at_relid);
> But there is this commit in vacuum.c:
> * It is the caller's responsibility that all parameters are allocated
> in a
> * memory context that will not disappear at transaction commit.
> And I don't think we want to break that promise as newNode() uses
> palloc0fast() which allocates data in the current memory context (see
> 4873c96f). I think that you had better just use NodeSetTag here and be
> done with it. Also, it seems to me that this could be fixed as a
> separate patch. It is definitely an incorrect pattern...

Don't we have a similar problem with makeVacuumRelation() and list_make1()?

I went ahead and moved the RangeVar, VacuumRelation, and List into local
variables for now, but I agree that this could be improved in a separate
patch. Perhaps these could be allocated in AutovacMemCxt. I see from
4873c96f that autovacuum_do_vac_analyze() used to allocate the list of OIDs
in that "long-lived" memory context.

> - $$ = (Node *)n;
> + $$ = (Node *) n;
> Spurious noise. And the coding pattern in gram.y is to not add a space
> (make new code look like its surroundings as the documentation says).

I've fixed this in v13.

Nathan

Attachment Content-Type Size
dedupe_vacuum_relations_v3.patch application/octet-stream 4.7 KB
vacuum_multiple_tables_v13.patch application/octet-stream 31.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2017-09-01 18:06:40 Re: Rename RECOVERYXLOG to RECOVERYWAL?
Previous Message Robert Haas 2017-09-01 18:00:20 Re: GnuTLS support