* Alvaro Herrera ( wrote:
> Robert Haas wrote:
> > On Sun, Mar 1, 2015 at 6:58 AM, Michael Paquier
> > <> 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.



Attachment: signature.asc
Description: Digital signature

Reply via email to