Alvaro,
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Robert Haas wrote:
> > On Sun, Mar 1, 2015 at 6:58 AM, Michael Paquier
> > <michael.paquier@gmail.com> wrote:
> > > Hm, why not. That would remove all inconsistencies between the parser
> > > and the autovacuum code path. Perhaps something like the attached
> > > makes sense then?
> >
> > I really don't see this patch, or any of the previous ones, as solving
> > any actual problem. There's no bug here, and no reason to suspect
> > that future code changes would be particularly like to introduce one.
> > Assertions are a great way to help developers catch coding mistakes,
> > but it's a real stretch to think that a developer is going to add a
> > new syntax for ANALYZE that involves setting options proper to VACUUM
> > and not notice it.
>
> That was my opinion of previous patches in this thread. But I think
> this last one makes a lot more sense: why is the parser concerned with
> figuring out the right defaults given FREEZE/not-FREEZE? I think there
> is a real gain in clarity here by deferring those decisions until vacuum
> time. The parser's job should be to pass the FREEZE flag down only,
> which is what this patch does, and consequently results in a (small) net
> reduction of LOC in gram.y.
Yeah, that was my thinking also in my earlier review.
> Here's a simple idea to improve the patch: make VacuumParams include
> VacuumStmt and the for_wraparound and do_toast flags. ISTM that reduces
> the number of arguments to be passed down in a couple of places. In
> particular:
>
> vacuum(VacuumParams *params, BufferAccessStrategy bstrategy, bool isTopLevel)
>
> vacuum_rel(VacuumParams *params)
>
> Does that sound more attractive?
I had been hoping we'd be able to provide an API which didn't require
autovacuum to build up a VacuumStmt, but that's not a big deal and it's
doing it currently anyway. Just mentioning it as that was one of the
other things that I had been hoping to get out of this.
Thanks!
Stephen