Thread: Strange assertion using VACOPT_FREEZE in vacuum.c

Strange assertion using VACOPT_FREEZE in vacuum.c

From
Michael Paquier
Date:
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?
--
Michael

Attachment

Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Jim Nasby
Date:
On 2/12/15 10:54 PM, Michael Paquier 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?

Looks good. Should we also assert that if VACOPT_FREEZE is set then all 
the other stuff is 0? I don't know what kind of sanity checks we 
normally try and put on the parser, but that seems like a possible hole.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Michael Paquier
Date:
On Sat, Feb 14, 2015 at 8:10 AM, Jim Nasby wrote:
>
> On 2/12/15 10:54 PM, Michael Paquier 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?
>
>
> Looks good. Should we also assert that if VACOPT_FREEZE is set then all the other stuff is 0? I don't know what kind
ofsanity checks we normally try and put on the parser, but that seems like a possible hole.
 

Possible, but this would require at least to change gram.y to update
VacuumStmt->options to set VACOPT_FREEZE for the case where VACUUM is
parsed without parenthesis. I'd rather simply rely on the freeze
parameters for simplicity. That is at least what I guess by looking at
where is used VACOPT_FREEZE.
-- 
Michael



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Robert Haas
Date:
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.

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



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Michael Paquier
Date:
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



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Michael Paquier
Date:
On Wed, Feb 18, 2015 at 10:06 AM, Michael Paquier wrote:
> 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.

Perhaps this explains better what I got in mind, aka making the
assertion stricter:
        Assert((vacstmt->options & VACOPT_VACUUM) ||
-                  !(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE)));
+                  ((vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE)) == 0 &&
+                       vacstmt->freeze_min_age < 0 &&
+                       vacstmt->freeze_table_age < 0 &&
+                       vacstmt->multixact_freeze_min_age < 0 &&
+                       vacstmt->multixact_freeze_table_age < 0));

Regards,
--
Michael

Attachment

Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Peter Eisentraut
Date:
On 2/18/15 1:26 AM, Michael Paquier wrote:
> On Wed, Feb 18, 2015 at 10:06 AM, Michael Paquier wrote:
>> 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.
> 
> Perhaps this explains better what I got in mind, aka making the
> assertion stricter:
>         Assert((vacstmt->options & VACOPT_VACUUM) ||
> -                  !(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE)));
> +                  ((vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE)) == 0 &&
> +                       vacstmt->freeze_min_age < 0 &&
> +                       vacstmt->freeze_table_age < 0 &&
> +                       vacstmt->multixact_freeze_min_age < 0 &&
> +                       vacstmt->multixact_freeze_table_age < 0));

That's cool if you want to add those assertions, but please make them
separate statements each, like

Assert(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE) || vacstmt->freeze_min_age == -1);
Assert(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE) || vacstmt->freeze_table_age == -1);
...

Besides being more readable, this will give you more useful output if
the assertion fails.




Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Michael Paquier
Date:
On Fri, Feb 20, 2015 at 5:41 AM, Peter Eisentraut wrote:
> That's cool if you want to add those assertions, but please make them
> separate statements each, like
>
> Assert(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE) || vacstmt->freeze_min_age == -1);
> Assert(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE) || vacstmt->freeze_table_age == -1);
> ...
>
> Besides being more readable, this will give you more useful output if
> the assertion fails.

It makes sense. When a manual VACUUM FREEZE without options specified
without parenthesis, VACOPT_FREEZE is not used in gram.y, so ISTM that
we should change them to that instead:
Assert((vacstmt->options & VACOPT_VACUUM) ||
vacstmt->multixact_freeze_table_age == -1);
At least this would check that an ANALYZE does not set those
parameters inappropriately...
--
Michael

Attachment

Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Stephen Frost
Date:
Michael,

* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Wed, Feb 18, 2015 at 12:09 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> > 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.

I'm trying to wrap my head around the reasoning for this also and not
sure I'm following.  In general, I don't think we protect all that hard
against functions being called with tokens that aren't allowed by the
parse.  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?
Thanks!
    Stephen

Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Michael Paquier
Date:
On Sat, Feb 28, 2015 at 2:45 PM, Stephen Frost <sfrost@snowman.net> wrote:
> I'm trying to wrap my head around the reasoning for this also and not
> sure I'm following.  In general, I don't think we protect all that hard
> against functions being called with tokens that aren't allowed by the
> parse.

Check.

> 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.

[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().[/]
-- 
Michael



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Stephen Frost
Date:
Michael,

* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Sat, Feb 28, 2015 at 2:45 PM, Stephen Frost <sfrost@snowman.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

Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Michael Paquier
Date:
On Sat, Feb 28, 2015 at 10:09 PM, Stephen Frost <sfrost@snowman.net> wrote:
>> [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.

The only reason why I think they should be improved is for extension
developers, a simple example being a bgworker that directly calls
vacuum with a custom VacuumStmt, at least with those assertions we
could get some directions to the developer that what he is doing is
not consistent with what the code expects.

> 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.

Hm, why not. That would remove all inconsistencies between the parser
and the autovacuum code path. Perhaps something like the attached
makes sense then?
--
Michael

Attachment

Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Robert Haas
Date:
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.

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 */.

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



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Stephen Frost
Date:
* 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.

> 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

Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Michael Paquier
Date:
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



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Alvaro Herrera
Date:
Robert Haas 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.

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.

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?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Stephen Frost
Date:
Alvaro,

* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> Robert Haas 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.
>
> 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.

Yeah, that was my thinking also in my earlier review.

> 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 had been hoping we'd be able to provide an API which didn't require
autovacuum to build up a VacuumStmt, but that's not a big deal and it's
doing it currently anyway.  Just mentioning it as that was one of the
other things that I had been hoping to get out of this.
Thanks!
    Stephen

Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Robert Haas
Date:
On Thu, Mar 5, 2015 at 9:58 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Robert Haas 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.
>
> 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



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Michael Paquier
Date:
On Fri, Mar 6, 2015 at 12:42 AM, Robert Haas wrote:
> On Thu, Mar 5, 2015 at 9:58 AM, Alvaro Herrera wrote:
>> 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.

Do you mean removing totally VacuumStmt from the stack? We would then
need to add relation and va_cols as additional arguments of things
like vacuum_rel, analyze_rel, do_analyze_rel or similar.

FWIW, adding do_toast and for_wraparound into VacuumParams makes sense
to me, but not VacuumStmt. It has little meaning as VacuumParams
should be used for parameters.
-- 
Michael



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Michael Paquier
Date:
On Fri, Mar 6, 2015 at 12:44 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Mar 6, 2015 at 12:42 AM, Robert Haas wrote:
>> On Thu, Mar 5, 2015 at 9:58 AM, Alvaro Herrera wrote:
>>> 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.
>
> Do you mean removing totally VacuumStmt from the stack? We would then
> need to add relation and va_cols as additional arguments of things
> like vacuum_rel, analyze_rel, do_analyze_rel or similar.
>
> FWIW, adding do_toast and for_wraparound into VacuumParams makes sense
> to me, but not VacuumStmt. It has little meaning as VacuumParams
> should be used for parameters.

But code may tell more than words, so here is some. I noticed that
moving for_wraparound in VacuumParams makes more sense than relid and
do_toast as those values need special handling when vacuum_rel is
called for a toast relation. For the patches that are separated for
clarity:
- 0001 is the previous one
- 0002 removes VacuumStmt from the call stack of ANALYZE and VACUUM routines
- 0003 moves for_wraparound in VacuumParams.

Regards,
--
Michael

Attachment

Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Robert Haas
Date:
On Fri, Mar 6, 2015 at 1:39 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
>> Do you mean removing totally VacuumStmt from the stack? We would then
>> need to add relation and va_cols as additional arguments of things
>> like vacuum_rel, analyze_rel, do_analyze_rel or similar.
>>
>> FWIW, adding do_toast and for_wraparound into VacuumParams makes sense
>> to me, but not VacuumStmt. It has little meaning as VacuumParams
>> should be used for parameters.
>
> But code may tell more than words, so here is some. I noticed that
> moving for_wraparound in VacuumParams makes more sense than relid and
> do_toast as those values need special handling when vacuum_rel is
> called for a toast relation. For the patches that are separated for
> clarity:
> - 0001 is the previous one
> - 0002 removes VacuumStmt from the call stack of ANALYZE and VACUUM routines
> - 0003 moves for_wraparound in VacuumParams.

Yeah, I think something like this could be a sensible approach.

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



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Alvaro Herrera
Date:
Robert Haas wrote:
> On Fri, Mar 6, 2015 at 1:39 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:

> > - 0001 is the previous one
> > - 0002 removes VacuumStmt from the call stack of ANALYZE and VACUUM routines
> > - 0003 moves for_wraparound in VacuumParams.
> 
> Yeah, I think something like this could be a sensible approach.

But autovacuum is still manufacturing a VacuumStmt by hand.  If we want
to get rid of that, I think it'd work to have a new
ExecVacuum(VacuumStmt, params) function which is called from
standard_ProcessUtility and does just vacuum(rel, relid, params).  
Autovacuum on the other hand can call vacuum() without having to
construct the parse node.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Robert Haas
Date:
On Wed, Mar 11, 2015 at 3:09 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Robert Haas wrote:
>> On Fri, Mar 6, 2015 at 1:39 AM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>
>> > - 0001 is the previous one
>> > - 0002 removes VacuumStmt from the call stack of ANALYZE and VACUUM routines
>> > - 0003 moves for_wraparound in VacuumParams.
>>
>> Yeah, I think something like this could be a sensible approach.
>
> But autovacuum is still manufacturing a VacuumStmt by hand.  If we want
> to get rid of that, I think it'd work to have a new
> ExecVacuum(VacuumStmt, params) function which is called from
> standard_ProcessUtility and does just vacuum(rel, relid, params).
> Autovacuum on the other hand can call vacuum() without having to
> construct the parse node.

+1.

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



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Tom Lane
Date:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> But autovacuum is still manufacturing a VacuumStmt by hand.  If we want
> to get rid of that, I think it'd work to have a new
> ExecVacuum(VacuumStmt, params) function which is called from
> standard_ProcessUtility and does just vacuum(rel, relid, params).  
> Autovacuum on the other hand can call vacuum() without having to
> construct the parse node.

Why would we want to get rid of that?  A struct is a handy and legible
way to pass a pile of parameters.  I doubt it would be an improvement for
vacuum() to grow a long list of separate parameters.
        regards, tom lane



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Jim Nasby
Date:
On 3/11/15 3:57 PM, Tom Lane wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> But autovacuum is still manufacturing a VacuumStmt by hand.  If we want
>> to get rid of that, I think it'd work to have a new
>> ExecVacuum(VacuumStmt, params) function which is called from
>> standard_ProcessUtility and does just vacuum(rel, relid, params).
>> Autovacuum on the other hand can call vacuum() without having to
>> construct the parse node.
>
> Why would we want to get rid of that?  A struct is a handy and legible
> way to pass a pile of parameters.  I doubt it would be an improvement for
> vacuum() to grow a long list of separate parameters.

We're not exactly getting rid of it; Thomas' patch adds a second struct 
that deals with detailed vacuum parameters that are not actually present 
in VacuumStmt. These are things that are specific to autovac but not 
manual VACUUM. But the patch in it's current form still have autovac 
building a somewhat bogus VacuumStmt.

What's being proposed is to expose VacuumStmt (which only makes sense 
for VACUUM) only where it's needed, and use VacuumParams everywhere 
else. In particular, this means autovac will just deal with VacuumParams 
and will no longer build a fake VacuumStmt.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Michael Paquier
Date:
On Thu, Mar 12, 2015 at 4:14 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Mar 11, 2015 at 3:09 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> Robert Haas wrote:
>>> On Fri, Mar 6, 2015 at 1:39 AM, Michael Paquier
>>> <michael.paquier@gmail.com> wrote:
>>
>>> > - 0001 is the previous one
>>> > - 0002 removes VacuumStmt from the call stack of ANALYZE and VACUUM routines
>>> > - 0003 moves for_wraparound in VacuumParams.
>>>
>>> Yeah, I think something like this could be a sensible approach.
>>
>> But autovacuum is still manufacturing a VacuumStmt by hand.  If we want
>> to get rid of that, I think it'd work to have a new
>> ExecVacuum(VacuumStmt, params) function which is called from
>> standard_ProcessUtility and does just vacuum(rel, relid, params).
>> Autovacuum on the other hand can call vacuum() without having to
>> construct the parse node.
>
> +1.

I have been pondering about that, and code-speaking this gives the
attached, making VacuumStmt only be used when VACUUM/ANALYZE is kicked
through utility.c, and removing its dependency in autovacuum.c.

There are a couple of parameters like va_cols, bstrategy, do_toast or
relid that can change depending on the code path (like presence of
toast relation). I think that it is confusing to add them in
VacuumParams as their value would get duplicated in this structure and
in the modified values that need to be set, so this structure only
contains the freeze control parameters and for_wraparound.

In utility.c, the interface for VACUUM/ANALYZE has this shape:
void ExecVacuum(VacuumStmt *vacstmt, bool isTopLevel);
--
Michael

Attachment

Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Alvaro Herrera
Date:
Michael Paquier wrote:

> I have been pondering about that, and code-speaking this gives the
> attached, making VacuumStmt only be used when VACUUM/ANALYZE is kicked
> through utility.c, and removing its dependency in autovacuum.c.
>
> There are a couple of parameters like va_cols, bstrategy, do_toast or
> relid that can change depending on the code path (like presence of
> toast relation). I think that it is confusing to add them in
> VacuumParams as their value would get duplicated in this structure and
> in the modified values that need to be set, so this structure only
> contains the freeze control parameters and for_wraparound.
>
> In utility.c, the interface for VACUUM/ANALYZE has this shape:
> void ExecVacuum(VacuumStmt *vacstmt, bool isTopLevel);

Here's an updated patch.  I took your latest version and made some extra
changes:

1. ordered the argument list to vacuum(), hopefully it's more sensible
now.

2. changed struct autovac_table so that it uses "options" (the same
VacuumOption bitmask to be passed to vacuum) and VacuumParams, instead
of having each struct member separately.  That way, the parameters to
vacuum() are constructed at once in autovac_recheck_table, and
autovacuum_do_vac_analyze becomes much simpler.

3. Added VACOPT_SKIPTOAST to VacuumOptions, currently only used by
autovacuum.  We remove the do_toast argument.

I think this is pretty sensible and my inclination is to commit as is,
so that we can finally move on to more interesting things (such as the
new reloption being proposed in a nearby thread).

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachment

Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Alvaro Herrera
Date:
I went to change this patch status in the commitfest app, and the app
told me I cannot change the status in the current commitfest.  Please
somebody with commitfest mace superpowers set it as ready for committer.

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Stephen Frost
Date:
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> I went to change this patch status in the commitfest app, and the app
> told me I cannot change the status in the current commitfest.  Please
> somebody with commitfest mace superpowers set it as ready for committer.

I'm afraid the issue is a business decision which is incorrect as it's
complaining that it's in a "Closed" state and you're trying to change it
to an "Open" state.  While it's neat to think a patch could never be
reopened, it's clearly not accurate.

Adding Magnus to this, as I'm pretty sure he has some code to go hack
(or perhaps just remove.. :).
Thanks!
    Stephen

Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Magnus Hagander
Date:
On Tue, Mar 17, 2015 at 6:31 PM, Stephen Frost <sfrost@snowman.net> wrote:
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
> I went to change this patch status in the commitfest app, and the app
> told me I cannot change the status in the current commitfest.  Please
> somebody with commitfest mace superpowers set it as ready for committer.

I'm afraid the issue is a business decision which is incorrect as it's
complaining that it's in a "Closed" state and you're trying to change it
to an "Open" state.  While it's neat to think a patch could never be
reopened, it's clearly not accurate.

Adding Magnus to this, as I'm pretty sure he has some code to go hack
(or perhaps just remove.. :).

I could've sworn I'd fixed that, but pretty obviously I hadn't. Sorry about that.

Fixed now - a patch can now go from closed back to open in the last commitfest where it was closed.
 
--

Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Michael Paquier
Date:
On Wed, Mar 18, 2015 at 2:22 AM, Alvaro Herrera wrote:
> Here's an updated patch.  I took your latest version and made some extra
> changes:

Thanks for taking the time to look at it!

> 1. ordered the argument list to vacuum(), hopefully it's more sensible
> now.

Fine for me.

> 2. changed struct autovac_table so that it uses "options" (the same
> VacuumOption bitmask to be passed to vacuum) and VacuumParams, instead
> of having each struct member separately.  That way, the parameters to
> vacuum() are constructed at once in autovac_recheck_table, and
> autovacuum_do_vac_analyze becomes much simpler.
>
> 3. Added VACOPT_SKIPTOAST to VacuumOptions, currently only used by
> autovacuum.  We remove the do_toast argument.

Those are good ideas, and it simplifies a bit more code.

I had a look at your modified version, and it looks good to me.

> I think this is pretty sensible and my inclination is to commit as is,
> so that we can finally move on to more interesting things (such as the
> new reloption being proposed in a nearby thread).

Thanks. I'll do a rebase if this goes in first.
Regards,
-- 
Michael



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Alvaro Herrera
Date:
Michael Paquier wrote:
> On Wed, Mar 18, 2015 at 2:22 AM, Alvaro Herrera wrote:

> > 1. ordered the argument list to vacuum(), hopefully it's more sensible
> > now.
> 
> Fine for me.

Actually, why don't we move va_cols to VacuumParams too?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Michael Paquier
Date:
On Wed, Mar 18, 2015 at 10:51 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Michael Paquier wrote:
>> On Wed, Mar 18, 2015 at 2:22 AM, Alvaro Herrera wrote:
>
>> > 1. ordered the argument list to vacuum(), hopefully it's more sensible
>> > now.
>>
>> Fine for me.
>
> Actually, why don't we move va_cols to VacuumParams too?

Because AnalyzeStmt assigns it in gram.y. Parameters directly from
VacuumStmt should not be added in Params, at least that's the spirit
of the patch as originally written.
-- 
Michael



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Alvaro Herrera
Date:
Michael Paquier wrote:

> I had a look at your modified version, and it looks good to me.

Thanks, pushed.  (Without the va_cols change proposed downthread.)

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: Strange assertion using VACOPT_FREEZE in vacuum.c

From
Michael Paquier
Date:
On Thu, Mar 19, 2015 at 2:44 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Michael Paquier wrote:
>
>> I had a look at your modified version, and it looks good to me.
>
> Thanks, pushed.  (Without the va_cols change proposed downthread.)

Thanks a lot! I will shortly work on the rebase for the other patch.
-- 
Michael