Thread: Strange assertion using VACOPT_FREEZE in vacuum.c
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
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
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
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
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
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
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.
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
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
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
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
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
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
* 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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
* 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
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.
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
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
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
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
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