On Thu, Mar 5, 2015 at 12:54 AM, Stephen Frost <sfrost@snowman.net> wrote:
> * Robert Haas (robertmhaas@gmail.com) wrote:
>> On Sun, Mar 1, 2015 at 6:58 AM, Michael Paquier
>> <michael.paquier@gmail.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.
OK, if you folks think that there is no point to add some assertions
on the freeze params for an ANALYZE, let's move on then.
>> 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 */.
Fine for me.
--
Michael