Re: Strange assertion using VACOPT_FREEZE in vacuum.c

On Thu, Mar 5, 2015 at 9:58 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> 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.

Sure, I'll buy that. If you want to refactor things that way, I have
no issue with it - I just want to point out that it seems to have zip
to do with what started this thread, which is why I wondered if we
were just talking for the sake of talking.

> 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 dislike passing down parser nodes straight into utility commands.
It tends to make those those functions hard for in-core users to call,
and also to lead to security vulnerabilities where we look up the same
names more than once and just hope that we get the same OID every
time. Stuffing the VacuumStmt pointer inside the VacuumParams object
doesn't, for me, help anything. It'd be a lot more interesting if we
could get rid of that altogether.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company