Re: Strange assertion using VACOPT_FREEZE in vacuum.c

On Wed, Feb 18, 2015 at 12:09 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Feb 12, 2015 at 11:54 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> Hi all,
>>
>> When calling vacuum(), there is the following assertion using VACOPT_FREEZE:
>> Assert((vacstmt->options & VACOPT_VACUUM) ||
>>     !(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE)));
>> I think that this should be changed with sanity checks based on the
>> parameter values of freeze_* in VacuumStmt as we do not set up
>> VACOPT_FREEZE when VACUUM is used without options in parenthesis, for
>> something like that:
>>         Assert((vacstmt->options & VACOPT_VACUUM) ||
>> -                  !(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE)));
>> +                  ((vacstmt->options & VACOPT_FULL) == 0 &&
>> +                       vacstmt->freeze_min_age < 0 &&
>> +                       vacstmt->freeze_table_age < 0 &&
>> +                       vacstmt->multixact_freeze_min_age < 0 &&
>> +                       vacstmt->multixact_freeze_table_age < 0));
>> This would also have the advantage to limit the use of VACOPT_FREEZE
>> in the query parser.
>> A patch is attached.
>> Thoughts?
>
> I think it's right the way it is.  The parser constructs a VacuumStmt
> for either a VACUUM or an ANALYZE command.  That statement is checking
> that if you are doing an ANALYZE, you can't specify FULL or FREEZE.
> That makes sense, because there is no ANALYZE FULL or ANALYZE FREEZE
> command.

Yes, the existing assertion is right. My point is that it is strange
that we do not check the values of freeze parameters for an ANALYZE
query, which should be set to -1 all the time. If this is thought as
not worth checking, I'll drop this patch and my concerns.
-- 
Michael