Re: Strange assertion using VACOPT_FREEZE in vacuum.c

Michael,

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> On Sat, Feb 28, 2015 at 2:45 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > So, basically, this feels like it's not really the right place
> > for these checks and if there is an existing problem then it's probably
> > with the grammar... Does that make sense?
>
> As long as there is no more inconsistency between the parser, that
> sometimes does not set VACOPT_FREEZE, and those assertions, that do
> not use the freeze_* parameters of VacuumStmt, I think that it will be
> fine.

parsenodes.h points out that VACOPT_FREEZE is just an internal
convenience for the grammar- it doesn't need to be set for vacuum()'s
purposes and, even if it is, except for this Assert(), it isn't looked
at. Now, I wouldn't be against changing the grammar to operate the same
way for all of these and therefore make it a bit easier to follow, eg:

if ($3)
n->options |= VACOPT_FREEZE;
if (n->options & VACOPT_FREEZE)
{
n->freeze_min_age = n->freeze_table_age = 0;
n->multixact_freeze_min_age = 0;
n->multixact_freeze_table_age = 0;
}
else
{
n->freeze_min_age = n->freeze_table_age = -1;
n->multixact_freeze_min_age = -1;
n->multixact_freeze_table_age = -1;
}

but I'm really not sure it's worth it.

> [nitpicking]We could improve things on both sides, aka change gram.y
> to set VACOPT_FREEZE correctly, and add some assertions with the
> params freeze_* at the beginning of vacuum().[/]

The other issue that I have with this is that most production operations
don't run with Asserts enabled, so if there is an actual issue here, we
shouldn't be using Asserts to check but regular conditionals and
throwing ereport()'s.

Another approach might be to change VACOPT_FREEZE to actually be used by
vacuum() instead of having to set 4 variables when it's not passed in.
Perhaps we would actually take those parameters out of VacuumStmt, add a
new argument to vacuum() for autovacuum to use which is a struct
containing those options, and remove all of nonsense of setting those
variables inside gram.y. At least in my head, that'd be a lot cleaner-
have the grammar worry about options that are actually coming from the
grammar and give other callers a way to specify more options if they've
got them.

Thanks!

Stephen