Re: Strange assertion using VACOPT_FREEZE in vacuum.c

Robert Haas wrote:
> On Sun, Mar 1, 2015 at 6:58 AM, Michael Paquier
> <michael(dot)paquier(at)gmail(dot)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.

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?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services