Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Robert Haas wrote: > > On Sun, Mar 1, 2015 at 6:58 AM, Michael Paquier > > <michael.paqu...@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
signature.asc
Description: Digital signature