Alvaro, * Alvaro Herrera ([email protected]) wrote: > Robert Haas wrote: > > On Sun, Mar 1, 2015 at 6:58 AM, Michael Paquier > > <[email protected]> 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
signature.asc
Description: Digital signature
