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

On 9/1/17, 12:11 AM, "Michael Paquier" <michael.paquier@gmail.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