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

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* 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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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