Re: Strange assertion using VACOPT_FREEZE in vacuum.c

* Robert Haas (robertmhaas(at)gmail(dot)com) 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.

Yeah, I haven't been terribly excited about it for the same reasons.
Had Michael's latest patch meant that we didn't need to pass VacuumStmt
down into the other functions then I might have been a bit more behind
it, but as is we seem to be simply duplicating everything except the
actual Node entry in the struct, which kind of missed the point.

> This thread started out because Michael read an assertion in the code
> and misunderstood what that assertion was trying to guard against.
> I'm not sure there's any code change needed here at all, but if there
> is, I suggest we confine it to adding a one-line comment above that
> assertion clarifying its purpose, like /* check that parser didn't
> produce ANALYZE FULL or ANALYZE FREEZE */.

I'd be fine with that.

Thanks!

Stephen