Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
On Thu, Mar 19, 2015 at 2:44 AM, Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
On Tue, Mar 17, 2015 at 6:31 PM, Stephen Frost sfr...@snowman.net wrote: * Alvaro Herrera (alvhe...@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. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
* Alvaro Herrera (alvhe...@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 signature.asc Description: Digital signature
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
On Wed, Mar 18, 2015 at 10:51 AM, Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
On 3/11/15 3:57 PM, Tom Lane wrote: Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
On Fri, Mar 6, 2015 at 1:39 AM, Michael Paquier michael.paqu...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
Robert Haas wrote: On Fri, Mar 6, 2015 at 1:39 AM, Michael Paquier michael.paqu...@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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
On Wed, Mar 11, 2015 at 3:09 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Robert Haas wrote: On Fri, Mar 6, 2015 at 1:39 AM, Michael Paquier michael.paqu...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
On Thu, Mar 5, 2015 at 9:58 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Robert Haas wrote: On Sun, Mar 1, 2015 at 6:58 AM, Michael Paquier michael.paqu...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Robert Haas wrote: On Sun, Mar 1, 2015 at 6:58 AM, Michael Paquier michael.paqu...@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 signature.asc Description: Digital signature
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
Robert Haas wrote: On Sun, Mar 1, 2015 at 6:58 AM, Michael Paquier michael.paqu...@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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
* Robert Haas (robertmh...@gmail.com) wrote: On Sun, Mar 1, 2015 at 6:58 AM, Michael Paquier michael.paqu...@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 signature.asc Description: Digital signature
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
On Sun, Mar 1, 2015 at 6:58 AM, Michael Paquier michael.paqu...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
On Thu, Mar 5, 2015 at 12:54 AM, Stephen Frost sfr...@snowman.net wrote: * Robert Haas (robertmh...@gmail.com) wrote: On Sun, Mar 1, 2015 at 6:58 AM, Michael Paquier michael.paqu...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
Michael,
* Michael Paquier (michael.paqu...@gmail.com) wrote:
On Sat, Feb 28, 2015 at 2:45 PM, Stephen Frost sfr...@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
signature.asc
Description: Digital signature
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
On Sat, Feb 28, 2015 at 2:45 PM, Stephen Frost sfr...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
Michael, * Michael Paquier (michael.paqu...@gmail.com) wrote: On Wed, Feb 18, 2015 at 12:09 AM, Robert Haas robertmh...@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 signature.asc Description: Digital signature
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
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 diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 2f3f79d..d2f5baa 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -111,6 +111,14 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast, Assert(vacstmt-options (VACOPT_VACUUM | VACOPT_ANALYZE)); Assert((vacstmt-options VACOPT_VACUUM) || !(vacstmt-options (VACOPT_FULL | VACOPT_FREEZE))); + Assert((vacstmt-options VACOPT_VACUUM) || + vacstmt-freeze_min_age == -1); + Assert((vacstmt-options VACOPT_VACUUM) || + vacstmt-freeze_table_age == -1); + Assert((vacstmt-options VACOPT_VACUUM) || + vacstmt-multixact_freeze_min_age == -1); + Assert((vacstmt-options VACOPT_VACUUM) || + vacstmt-multixact_freeze_table_age == -1); Assert((vacstmt-options VACOPT_ANALYZE) || vacstmt-va_cols == NIL); stmttype = (vacstmt-options VACOPT_VACUUM) ? VACUUM : ANALYZE; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
On Wed, Feb 18, 2015 at 12:09 AM, Robert Haas robertmh...@gmail.com wrote: On Thu, Feb 12, 2015 at 11:54 PM, Michael Paquier michael.paqu...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
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 diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index 2f3f79d..e1472ad 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -110,7 +110,11 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast, /* sanity checks on options */ Assert(vacstmt-options (VACOPT_VACUUM | VACOPT_ANALYZE)); 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)); Assert((vacstmt-options VACOPT_ANALYZE) || vacstmt-va_cols == NIL); stmttype = (vacstmt-options VACOPT_VACUUM) ? VACUUM : ANALYZE; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
On Thu, Feb 12, 2015 at 11:54 PM, Michael Paquier michael.paqu...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Strange assertion using VACOPT_FREEZE in vacuum.c
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 of sanity 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
