Strange assertion using VACOPT_FREEZE in vacuum.c

Started by Michael Paquieralmost 11 years ago35 messages
#1Michael Paquier
michael.paquier@gmail.com
1 attachment(s)

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

Attachments:

20150213_vacuum_freeze_fix_assertion.patchtext/x-patch; charset=US-ASCII; name=20150213_vacuum_freeze_fix_assertion.patchDownload
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 2f3f79d..3ddc077 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) == 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";
#2Jim Nasby
Jim.Nasby@BlueTreble.com
In reply to: Michael Paquier (#1)
Re: 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

#3Michael Paquier
michael.paquier@gmail.com
In reply to: Jim Nasby (#2)
Re: 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

#4Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#1)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

#5Michael Paquier
michael.paquier@gmail.com
In reply to: Robert Haas (#4)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

#6Michael Paquier
michael.paquier@gmail.com
In reply to: Michael Paquier (#5)
1 attachment(s)
Re: 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

Attachments:

20150218_vacuum_freeze_fix_assertion_v2.patchapplication/x-patch; name=20150218_vacuum_freeze_fix_assertion_v2.patchDownload
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";
#7Peter Eisentraut
peter_e@gmx.net
In reply to: Michael Paquier (#6)
Re: 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

#8Michael Paquier
michael.paquier@gmail.com
In reply to: Peter Eisentraut (#7)
1 attachment(s)
Re: 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

Attachments:

20150220_vacuum_freeze_fix_assertion_v3.patchapplication/x-patch; name=20150220_vacuum_freeze_fix_assertion_v3.patchDownload
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";
#9Stephen Frost
sfrost@snowman.net
In reply to: Michael Paquier (#5)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

#10Michael Paquier
michael.paquier@gmail.com
In reply to: Stephen Frost (#9)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

#11Stephen Frost
sfrost@snowman.net
In reply to: Michael Paquier (#10)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

#12Michael Paquier
michael.paquier@gmail.com
In reply to: Stephen Frost (#11)
1 attachment(s)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

Attachments:

0001-Move-freeze-parameters-of-VacuumStmt-into-a-separate.patchapplication/x-patch; name=0001-Move-freeze-parameters-of-VacuumStmt-into-a-separate.patchDownload
From a3dce99b48a6c97c1f72da63f38adbfcc22d462d Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@otacoo.com>
Date: Sun, 1 Mar 2015 20:45:06 +0900
Subject: [PATCH] Move freeze parameters of VacuumStmt into a separate
 structure

VACOPT_FREEZE was being limited in the query parser and still be used
in a set of assertions at the beginning of vacuum(). On the contrary,
the set of parameters controlling if FREEZE is used was being set in the
query parser but not used in the same set of assertions, leading to
confusion regarding both aspects of VacuumStmt.

This commit fixes this inconsistency by moving all the freeze parameters
to a dedicated structure used by autovacuum and by setting unconditionally
VACOPT_FREEZE for all the code paths of the query parser, making the whole
control of FREEZE more consistent in the autovacuum code path as well as
in the query parser.

Per an idea from Stephen Frost.

to be used in the grammar with and still use it in the set of assertions
---
 src/backend/commands/vacuum.c       | 18 +++++++------
 src/backend/commands/vacuumlazy.c   | 38 +++++++++++++++++++++++++---
 src/backend/nodes/copyfuncs.c       |  4 ---
 src/backend/nodes/equalfuncs.c      |  4 ---
 src/backend/parser/gram.y           | 50 +++++--------------------------------
 src/backend/postmaster/autovacuum.c | 17 +++++++------
 src/backend/tcop/utility.c          |  2 +-
 src/include/commands/vacuum.h       | 19 +++++++++++---
 src/include/nodes/parsenodes.h      | 14 +++--------
 9 files changed, 81 insertions(+), 85 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 2f3f79d..c2234e6 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -71,13 +71,16 @@ static void vac_truncate_clog(TransactionId frozenXID,
 				  MultiXactId minMulti,
 				  TransactionId lastSaneFrozenXid,
 				  MultiXactId lastSaneMinMulti);
-static bool vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast,
-		   bool for_wraparound);
+static bool vacuum_rel(Oid relid, VacuumStmt *vacstmt, VacuumParams *params,
+					   bool do_toast, bool for_wraparound);
 
 
 /*
  * Primary entry point for VACUUM and ANALYZE commands.
  *
+ * params contains a set of parameters used by autovacuum to customize the
+ * behavior of process.
+ *
  * relid is normally InvalidOid; if it is not, then it provides the relation
  * OID to be processed, and vacstmt->relation is ignored.  (The non-invalid
  * case is currently only used by autovacuum.)
@@ -98,7 +101,7 @@ static bool vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast,
  * at transaction commit.
  */
 void
-vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
+vacuum(VacuumStmt *vacstmt, VacuumParams *params, Oid relid, bool do_toast,
 	   BufferAccessStrategy bstrategy, bool for_wraparound, bool isTopLevel)
 {
 	const char *stmttype;
@@ -247,7 +250,7 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 
 			if (vacstmt->options & VACOPT_VACUUM)
 			{
-				if (!vacuum_rel(relid, vacstmt, do_toast, for_wraparound))
+				if (!vacuum_rel(relid, vacstmt, params, do_toast, for_wraparound))
 					continue;
 			}
 
@@ -1113,7 +1116,8 @@ vac_truncate_clog(TransactionId frozenXID,
  *		At entry and exit, we are not inside a transaction.
  */
 static bool
-vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
+vacuum_rel(Oid relid, VacuumStmt *vacstmt, VacuumParams *params,
+		   bool do_toast, bool for_wraparound)
 {
 	LOCKMODE	lmode;
 	Relation	onerel;
@@ -1316,7 +1320,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 					(vacstmt->options & VACOPT_VERBOSE) != 0);
 	}
 	else
-		lazy_vacuum_rel(onerel, vacstmt, vac_strategy);
+		lazy_vacuum_rel(onerel, vacstmt, params, vac_strategy);
 
 	/* Roll back any GUC changes executed by index functions */
 	AtEOXact_GUC(false, save_nestlevel);
@@ -1342,7 +1346,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 	 * totally unimportant for toast relations.
 	 */
 	if (toast_relid != InvalidOid)
-		vacuum_rel(toast_relid, vacstmt, false, for_wraparound);
+		vacuum_rel(toast_relid, vacstmt, params, false, for_wraparound);
 
 	/*
 	 * Now release the session-level lock on the master table.
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 7d9e49e..adab3b7 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -169,7 +169,7 @@ static bool heap_page_is_all_visible(Relation rel, Buffer buf,
  *		and locked the relation.
  */
 void
-lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
+lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt, VacuumParams *params,
 				BufferAccessStrategy bstrategy)
 {
 	LVRelStats *vacrelstats;
@@ -192,6 +192,10 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
 	double		new_live_tuples;
 	TransactionId new_frozen_xid;
 	MultiXactId new_min_multi;
+	int			freeze_min_age,
+				freeze_table_age,
+				multixact_freeze_min_age,
+				multixact_freeze_table_age;
 
 	/* measure elapsed time iff autovacuum logging requires it */
 	if (IsAutoVacuumWorkerProcess() && Log_autovacuum_min_duration >= 0)
@@ -207,10 +211,36 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
 
 	vac_strategy = bstrategy;
 
+	/* set up FREEZE parameters */
+	if (params != NULL)
+	{
+		freeze_min_age = params->freeze_min_age;
+		freeze_table_age = params->freeze_table_age;
+		multixact_freeze_min_age = params->multixact_freeze_min_age;
+		multixact_freeze_table_age = params->multixact_freeze_table_age;
+	}
+	else
+	{
+		if (vacstmt->options & VACOPT_FREEZE)
+		{
+			freeze_min_age = 0;
+			freeze_table_age = 0;
+			multixact_freeze_min_age = 0;
+			multixact_freeze_table_age = 0;
+		}
+		else
+		{
+			freeze_min_age = -1;
+			freeze_table_age = -1;
+			multixact_freeze_min_age = -1;
+			multixact_freeze_table_age = -1;
+		}
+	}
+
 	vacuum_set_xid_limits(onerel,
-						  vacstmt->freeze_min_age, vacstmt->freeze_table_age,
-						  vacstmt->multixact_freeze_min_age,
-						  vacstmt->multixact_freeze_table_age,
+						  freeze_min_age, freeze_table_age,
+						  multixact_freeze_min_age,
+						  multixact_freeze_table_age,
 						  &OldestXmin, &FreezeLimit, &xidFullScanLimit,
 						  &MultiXactCutoff, &mxactFullScanLimit);
 
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 9fe8008..4f0e278 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -3292,10 +3292,6 @@ _copyVacuumStmt(const VacuumStmt *from)
 	VacuumStmt *newnode = makeNode(VacuumStmt);
 
 	COPY_SCALAR_FIELD(options);
-	COPY_SCALAR_FIELD(freeze_min_age);
-	COPY_SCALAR_FIELD(freeze_table_age);
-	COPY_SCALAR_FIELD(multixact_freeze_min_age);
-	COPY_SCALAR_FIELD(multixact_freeze_table_age);
 	COPY_NODE_FIELD(relation);
 	COPY_NODE_FIELD(va_cols);
 
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index fe509b0..6b55500 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1507,10 +1507,6 @@ static bool
 _equalVacuumStmt(const VacuumStmt *a, const VacuumStmt *b)
 {
 	COMPARE_SCALAR_FIELD(options);
-	COMPARE_SCALAR_FIELD(freeze_min_age);
-	COMPARE_SCALAR_FIELD(freeze_table_age);
-	COMPARE_SCALAR_FIELD(multixact_freeze_min_age);
-	COMPARE_SCALAR_FIELD(multixact_freeze_table_age);
 	COMPARE_NODE_FIELD(relation);
 	COMPARE_NODE_FIELD(va_cols);
 
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 581f7a1..4b9a17f 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -9068,12 +9068,10 @@ VacuumStmt: VACUUM opt_full opt_freeze opt_verbose
 					n->options = VACOPT_VACUUM;
 					if ($2)
 						n->options |= VACOPT_FULL;
+					if ($3)
+						n->options |= VACOPT_FREEZE;
 					if ($4)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = $3 ? 0 : -1;
-					n->freeze_table_age = $3 ? 0 : -1;
-					n->multixact_freeze_min_age = $3 ? 0 : -1;
-					n->multixact_freeze_table_age = $3 ? 0 : -1;
 					n->relation = NULL;
 					n->va_cols = NIL;
 					$$ = (Node *)n;
@@ -9084,12 +9082,10 @@ VacuumStmt: VACUUM opt_full opt_freeze opt_verbose
 					n->options = VACOPT_VACUUM;
 					if ($2)
 						n->options |= VACOPT_FULL;
+					if ($3)
+						n->options |= VACOPT_FREEZE;
 					if ($4)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = $3 ? 0 : -1;
-					n->freeze_table_age = $3 ? 0 : -1;
-					n->multixact_freeze_min_age = $3 ? 0 : -1;
-					n->multixact_freeze_table_age = $3 ? 0 : -1;
 					n->relation = $5;
 					n->va_cols = NIL;
 					$$ = (Node *)n;
@@ -9100,30 +9096,16 @@ VacuumStmt: VACUUM opt_full opt_freeze opt_verbose
 					n->options |= VACOPT_VACUUM;
 					if ($2)
 						n->options |= VACOPT_FULL;
+					if ($3)
+						n->options |= VACOPT_FREEZE;
 					if ($4)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = $3 ? 0 : -1;
-					n->freeze_table_age = $3 ? 0 : -1;
-					n->multixact_freeze_min_age = $3 ? 0 : -1;
-					n->multixact_freeze_table_age = $3 ? 0 : -1;
 					$$ = (Node *)n;
 				}
 			| VACUUM '(' vacuum_option_list ')'
 				{
 					VacuumStmt *n = makeNode(VacuumStmt);
 					n->options = VACOPT_VACUUM | $3;
-					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;
-					}
 					n->relation = NULL;
 					n->va_cols = NIL;
 					$$ = (Node *) n;
@@ -9132,18 +9114,6 @@ VacuumStmt: VACUUM opt_full opt_freeze opt_verbose
 				{
 					VacuumStmt *n = makeNode(VacuumStmt);
 					n->options = VACOPT_VACUUM | $3;
-					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;
-					}
 					n->relation = $5;
 					n->va_cols = $6;
 					if (n->va_cols != NIL)	/* implies analyze */
@@ -9171,10 +9141,6 @@ AnalyzeStmt:
 					n->options = VACOPT_ANALYZE;
 					if ($2)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = -1;
-					n->freeze_table_age = -1;
-					n->multixact_freeze_min_age = -1;
-					n->multixact_freeze_table_age = -1;
 					n->relation = NULL;
 					n->va_cols = NIL;
 					$$ = (Node *)n;
@@ -9185,10 +9151,6 @@ AnalyzeStmt:
 					n->options = VACOPT_ANALYZE;
 					if ($2)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = -1;
-					n->freeze_table_age = -1;
-					n->multixact_freeze_min_age = -1;
-					n->multixact_freeze_table_age = -1;
 					n->relation = $3;
 					n->va_cols = $4;
 					$$ = (Node *)n;
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 77158c1..67c8cdc 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2740,8 +2740,9 @@ static void
 autovacuum_do_vac_analyze(autovac_table *tab,
 						  BufferAccessStrategy bstrategy)
 {
-	VacuumStmt	vacstmt;
-	RangeVar	rangevar;
+	VacuumStmt		vacstmt;
+	VacuumParams	params;
+	RangeVar		rangevar;
 
 	/* Set up command parameters --- use local variables instead of palloc */
 	MemSet(&vacstmt, 0, sizeof(vacstmt));
@@ -2758,18 +2759,20 @@ autovacuum_do_vac_analyze(autovac_table *tab,
 		vacstmt.options |= VACOPT_VACUUM;
 	if (tab->at_doanalyze)
 		vacstmt.options |= VACOPT_ANALYZE;
-	vacstmt.freeze_min_age = tab->at_freeze_min_age;
-	vacstmt.freeze_table_age = tab->at_freeze_table_age;
-	vacstmt.multixact_freeze_min_age = tab->at_multixact_freeze_min_age;
-	vacstmt.multixact_freeze_table_age = tab->at_multixact_freeze_table_age;
 	/* we pass the OID, but might need this anyway for an error message */
 	vacstmt.relation = &rangevar;
 	vacstmt.va_cols = NIL;
 
+	params.freeze_min_age = tab->at_freeze_min_age;
+	params.freeze_table_age = tab->at_freeze_table_age;
+	params.multixact_freeze_min_age = tab->at_multixact_freeze_min_age;
+	params.multixact_freeze_table_age = tab->at_multixact_freeze_table_age;
+
 	/* Let pgstat know what we're doing */
 	autovac_report_activity(tab);
 
-	vacuum(&vacstmt, tab->at_relid, false, bstrategy, tab->at_wraparound, true);
+	vacuum(&vacstmt, &params, tab->at_relid, false, bstrategy,
+		   tab->at_wraparound, true);
 }
 
 /*
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 6d26986..8ac7e3b 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -627,7 +627,7 @@ standard_ProcessUtility(Node *parsetree,
 				/* we choose to allow this during "read only" transactions */
 				PreventCommandDuringRecovery((stmt->options & VACOPT_VACUUM) ?
 											 "VACUUM" : "ANALYZE");
-				vacuum(stmt, InvalidOid, true, NULL, false, isTopLevel);
+				vacuum(stmt, NULL, InvalidOid, true, NULL, false, isTopLevel);
 			}
 			break;
 
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 4275484..2c6a5b5 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -130,6 +130,18 @@ typedef struct VacAttrStats
 	int			rowstride;
 } VacAttrStats;
 
+/*
+ * Parameters customizing behavior of a VACUUM or ANALYZE query.
+ */
+typedef struct VacuumParams
+{
+	int		freeze_min_age;		/* min freeze age, or -1 to use default */
+	int		freeze_table_age;       /* age at which to scan whole table */
+	int		multixact_freeze_min_age;       /* min multixact freeze age,
+												 * or -1 to use default */
+	int		multixact_freeze_table_age;     /* multixact age at which to
+											 * scan whole table */
+} VacuumParams;
 
 /* GUC parameters */
 extern PGDLLIMPORT int default_statistics_target;		/* PGDLLIMPORT for
@@ -141,8 +153,9 @@ extern int	vacuum_multixact_freeze_table_age;
 
 
 /* in commands/vacuum.c */
-extern void vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
-	   BufferAccessStrategy bstrategy, bool for_wraparound, bool isTopLevel);
+extern void vacuum(VacuumStmt *vacstmt, VacuumParams *params, Oid relid,
+				   bool do_toast, BufferAccessStrategy bstrategy,
+				   bool for_wraparound, bool isTopLevel);
 extern void vac_open_indexes(Relation relation, LOCKMODE lockmode,
 				 int *nindexes, Relation **Irel);
 extern void vac_close_indexes(int nindexes, Relation *Irel, LOCKMODE lockmode);
@@ -172,7 +185,7 @@ extern void vacuum_delay_point(void);
 
 /* in commands/vacuumlazy.c */
 extern void lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
-				BufferAccessStrategy bstrategy);
+				VacuumParams *params, BufferAccessStrategy bstrategy);
 
 /* in commands/analyze.c */
 extern void analyze_rel(Oid relid, VacuumStmt *vacstmt,
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index ac13302..4c8b14c 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2598,9 +2598,7 @@ typedef struct ClusterStmt
  *
  * Even though these are nominally two statements, it's convenient to use
  * just one node type for both.  Note that at least one of VACOPT_VACUUM
- * and VACOPT_ANALYZE must be set in options.  VACOPT_FREEZE is an internal
- * convenience for the grammar and is not examined at runtime --- the
- * freeze_min_age and freeze_table_age fields are what matter.
+ * and VACOPT_ANALYZE must be set in options.
  * ----------------------
  */
 typedef enum VacuumOption
@@ -2615,14 +2613,8 @@ typedef enum VacuumOption
 
 typedef struct VacuumStmt
 {
-	NodeTag		type;
-	int			options;		/* OR of VacuumOption flags */
-	int			freeze_min_age; /* min freeze age, or -1 to use default */
-	int			freeze_table_age;		/* age at which to scan whole table */
-	int			multixact_freeze_min_age;		/* min multixact freeze age,
-												 * or -1 to use default */
-	int			multixact_freeze_table_age;		/* multixact age at which to
-												 * scan whole table */
+	NodeTag			type;
+	int				options;	/* OR of VacuumOption flags */
 	RangeVar   *relation;		/* single table to process, or NULL */
 	List	   *va_cols;		/* list of column names, or NIL for all */
 } VacuumStmt;
-- 
2.3.1

#13Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#12)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

#14Stephen Frost
sfrost@snowman.net
In reply to: Robert Haas (#13)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

* 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

#15Michael Paquier
michael.paquier@gmail.com
In reply to: Stephen Frost (#14)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

#16Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Robert Haas (#13)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

#17Stephen Frost
sfrost@snowman.net
In reply to: Alvaro Herrera (#16)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

#18Robert Haas
robertmhaas@gmail.com
In reply to: Alvaro Herrera (#16)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

#19Michael Paquier
michael.paquier@gmail.com
In reply to: Robert Haas (#18)
Re: 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

#20Michael Paquier
michael.paquier@gmail.com
In reply to: Michael Paquier (#19)
3 attachment(s)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

Attachments:

0001-Move-freeze-parameters-of-VacuumStmt-into-a-separate.patchapplication/x-patch; name=0001-Move-freeze-parameters-of-VacuumStmt-into-a-separate.patchDownload
From a2b22ae6a929d344c3886a305220d110258d04da Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@otacoo.com>
Date: Sun, 1 Mar 2015 20:45:06 +0900
Subject: [PATCH 1/3] Move freeze parameters of VacuumStmt into a separate
 structure

VACOPT_FREEZE was being limited in the query parser and still be used
in a set of assertions at the beginning of vacuum(). On the contrary,
the set of parameters controlling if FREEZE is used was being set in the
query parser but not used in the same set of assertions, leading to
confusion regarding both aspects of VacuumStmt.

This commit fixes this inconsistency by moving all the freeze parameters
to a dedicated structure used by autovacuum and by setting unconditionally
VACOPT_FREEZE for all the code paths of the query parser, making the whole
control of FREEZE more consistent in the autovacuum code path as well as
in the query parser.
---
 src/backend/commands/vacuum.c       | 18 +++++++------
 src/backend/commands/vacuumlazy.c   | 38 +++++++++++++++++++++++++---
 src/backend/nodes/copyfuncs.c       |  4 ---
 src/backend/nodes/equalfuncs.c      |  4 ---
 src/backend/parser/gram.y           | 50 +++++--------------------------------
 src/backend/postmaster/autovacuum.c | 17 +++++++------
 src/backend/tcop/utility.c          |  2 +-
 src/include/commands/vacuum.h       | 19 +++++++++++---
 src/include/nodes/parsenodes.h      | 14 +++--------
 9 files changed, 81 insertions(+), 85 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 2f3f79d..c2234e6 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -71,13 +71,16 @@ static void vac_truncate_clog(TransactionId frozenXID,
 				  MultiXactId minMulti,
 				  TransactionId lastSaneFrozenXid,
 				  MultiXactId lastSaneMinMulti);
-static bool vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast,
-		   bool for_wraparound);
+static bool vacuum_rel(Oid relid, VacuumStmt *vacstmt, VacuumParams *params,
+					   bool do_toast, bool for_wraparound);
 
 
 /*
  * Primary entry point for VACUUM and ANALYZE commands.
  *
+ * params contains a set of parameters used by autovacuum to customize the
+ * behavior of process.
+ *
  * relid is normally InvalidOid; if it is not, then it provides the relation
  * OID to be processed, and vacstmt->relation is ignored.  (The non-invalid
  * case is currently only used by autovacuum.)
@@ -98,7 +101,7 @@ static bool vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast,
  * at transaction commit.
  */
 void
-vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
+vacuum(VacuumStmt *vacstmt, VacuumParams *params, Oid relid, bool do_toast,
 	   BufferAccessStrategy bstrategy, bool for_wraparound, bool isTopLevel)
 {
 	const char *stmttype;
@@ -247,7 +250,7 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 
 			if (vacstmt->options & VACOPT_VACUUM)
 			{
-				if (!vacuum_rel(relid, vacstmt, do_toast, for_wraparound))
+				if (!vacuum_rel(relid, vacstmt, params, do_toast, for_wraparound))
 					continue;
 			}
 
@@ -1113,7 +1116,8 @@ vac_truncate_clog(TransactionId frozenXID,
  *		At entry and exit, we are not inside a transaction.
  */
 static bool
-vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
+vacuum_rel(Oid relid, VacuumStmt *vacstmt, VacuumParams *params,
+		   bool do_toast, bool for_wraparound)
 {
 	LOCKMODE	lmode;
 	Relation	onerel;
@@ -1316,7 +1320,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 					(vacstmt->options & VACOPT_VERBOSE) != 0);
 	}
 	else
-		lazy_vacuum_rel(onerel, vacstmt, vac_strategy);
+		lazy_vacuum_rel(onerel, vacstmt, params, vac_strategy);
 
 	/* Roll back any GUC changes executed by index functions */
 	AtEOXact_GUC(false, save_nestlevel);
@@ -1342,7 +1346,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 	 * totally unimportant for toast relations.
 	 */
 	if (toast_relid != InvalidOid)
-		vacuum_rel(toast_relid, vacstmt, false, for_wraparound);
+		vacuum_rel(toast_relid, vacstmt, params, false, for_wraparound);
 
 	/*
 	 * Now release the session-level lock on the master table.
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 7d9e49e..adab3b7 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -169,7 +169,7 @@ static bool heap_page_is_all_visible(Relation rel, Buffer buf,
  *		and locked the relation.
  */
 void
-lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
+lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt, VacuumParams *params,
 				BufferAccessStrategy bstrategy)
 {
 	LVRelStats *vacrelstats;
@@ -192,6 +192,10 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
 	double		new_live_tuples;
 	TransactionId new_frozen_xid;
 	MultiXactId new_min_multi;
+	int			freeze_min_age,
+				freeze_table_age,
+				multixact_freeze_min_age,
+				multixact_freeze_table_age;
 
 	/* measure elapsed time iff autovacuum logging requires it */
 	if (IsAutoVacuumWorkerProcess() && Log_autovacuum_min_duration >= 0)
@@ -207,10 +211,36 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
 
 	vac_strategy = bstrategy;
 
+	/* set up FREEZE parameters */
+	if (params != NULL)
+	{
+		freeze_min_age = params->freeze_min_age;
+		freeze_table_age = params->freeze_table_age;
+		multixact_freeze_min_age = params->multixact_freeze_min_age;
+		multixact_freeze_table_age = params->multixact_freeze_table_age;
+	}
+	else
+	{
+		if (vacstmt->options & VACOPT_FREEZE)
+		{
+			freeze_min_age = 0;
+			freeze_table_age = 0;
+			multixact_freeze_min_age = 0;
+			multixact_freeze_table_age = 0;
+		}
+		else
+		{
+			freeze_min_age = -1;
+			freeze_table_age = -1;
+			multixact_freeze_min_age = -1;
+			multixact_freeze_table_age = -1;
+		}
+	}
+
 	vacuum_set_xid_limits(onerel,
-						  vacstmt->freeze_min_age, vacstmt->freeze_table_age,
-						  vacstmt->multixact_freeze_min_age,
-						  vacstmt->multixact_freeze_table_age,
+						  freeze_min_age, freeze_table_age,
+						  multixact_freeze_min_age,
+						  multixact_freeze_table_age,
 						  &OldestXmin, &FreezeLimit, &xidFullScanLimit,
 						  &MultiXactCutoff, &mxactFullScanLimit);
 
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 9fe8008..4f0e278 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -3292,10 +3292,6 @@ _copyVacuumStmt(const VacuumStmt *from)
 	VacuumStmt *newnode = makeNode(VacuumStmt);
 
 	COPY_SCALAR_FIELD(options);
-	COPY_SCALAR_FIELD(freeze_min_age);
-	COPY_SCALAR_FIELD(freeze_table_age);
-	COPY_SCALAR_FIELD(multixact_freeze_min_age);
-	COPY_SCALAR_FIELD(multixact_freeze_table_age);
 	COPY_NODE_FIELD(relation);
 	COPY_NODE_FIELD(va_cols);
 
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index fe509b0..6b55500 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1507,10 +1507,6 @@ static bool
 _equalVacuumStmt(const VacuumStmt *a, const VacuumStmt *b)
 {
 	COMPARE_SCALAR_FIELD(options);
-	COMPARE_SCALAR_FIELD(freeze_min_age);
-	COMPARE_SCALAR_FIELD(freeze_table_age);
-	COMPARE_SCALAR_FIELD(multixact_freeze_min_age);
-	COMPARE_SCALAR_FIELD(multixact_freeze_table_age);
 	COMPARE_NODE_FIELD(relation);
 	COMPARE_NODE_FIELD(va_cols);
 
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 581f7a1..4b9a17f 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -9068,12 +9068,10 @@ VacuumStmt: VACUUM opt_full opt_freeze opt_verbose
 					n->options = VACOPT_VACUUM;
 					if ($2)
 						n->options |= VACOPT_FULL;
+					if ($3)
+						n->options |= VACOPT_FREEZE;
 					if ($4)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = $3 ? 0 : -1;
-					n->freeze_table_age = $3 ? 0 : -1;
-					n->multixact_freeze_min_age = $3 ? 0 : -1;
-					n->multixact_freeze_table_age = $3 ? 0 : -1;
 					n->relation = NULL;
 					n->va_cols = NIL;
 					$$ = (Node *)n;
@@ -9084,12 +9082,10 @@ VacuumStmt: VACUUM opt_full opt_freeze opt_verbose
 					n->options = VACOPT_VACUUM;
 					if ($2)
 						n->options |= VACOPT_FULL;
+					if ($3)
+						n->options |= VACOPT_FREEZE;
 					if ($4)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = $3 ? 0 : -1;
-					n->freeze_table_age = $3 ? 0 : -1;
-					n->multixact_freeze_min_age = $3 ? 0 : -1;
-					n->multixact_freeze_table_age = $3 ? 0 : -1;
 					n->relation = $5;
 					n->va_cols = NIL;
 					$$ = (Node *)n;
@@ -9100,30 +9096,16 @@ VacuumStmt: VACUUM opt_full opt_freeze opt_verbose
 					n->options |= VACOPT_VACUUM;
 					if ($2)
 						n->options |= VACOPT_FULL;
+					if ($3)
+						n->options |= VACOPT_FREEZE;
 					if ($4)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = $3 ? 0 : -1;
-					n->freeze_table_age = $3 ? 0 : -1;
-					n->multixact_freeze_min_age = $3 ? 0 : -1;
-					n->multixact_freeze_table_age = $3 ? 0 : -1;
 					$$ = (Node *)n;
 				}
 			| VACUUM '(' vacuum_option_list ')'
 				{
 					VacuumStmt *n = makeNode(VacuumStmt);
 					n->options = VACOPT_VACUUM | $3;
-					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;
-					}
 					n->relation = NULL;
 					n->va_cols = NIL;
 					$$ = (Node *) n;
@@ -9132,18 +9114,6 @@ VacuumStmt: VACUUM opt_full opt_freeze opt_verbose
 				{
 					VacuumStmt *n = makeNode(VacuumStmt);
 					n->options = VACOPT_VACUUM | $3;
-					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;
-					}
 					n->relation = $5;
 					n->va_cols = $6;
 					if (n->va_cols != NIL)	/* implies analyze */
@@ -9171,10 +9141,6 @@ AnalyzeStmt:
 					n->options = VACOPT_ANALYZE;
 					if ($2)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = -1;
-					n->freeze_table_age = -1;
-					n->multixact_freeze_min_age = -1;
-					n->multixact_freeze_table_age = -1;
 					n->relation = NULL;
 					n->va_cols = NIL;
 					$$ = (Node *)n;
@@ -9185,10 +9151,6 @@ AnalyzeStmt:
 					n->options = VACOPT_ANALYZE;
 					if ($2)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = -1;
-					n->freeze_table_age = -1;
-					n->multixact_freeze_min_age = -1;
-					n->multixact_freeze_table_age = -1;
 					n->relation = $3;
 					n->va_cols = $4;
 					$$ = (Node *)n;
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 77158c1..67c8cdc 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2740,8 +2740,9 @@ static void
 autovacuum_do_vac_analyze(autovac_table *tab,
 						  BufferAccessStrategy bstrategy)
 {
-	VacuumStmt	vacstmt;
-	RangeVar	rangevar;
+	VacuumStmt		vacstmt;
+	VacuumParams	params;
+	RangeVar		rangevar;
 
 	/* Set up command parameters --- use local variables instead of palloc */
 	MemSet(&vacstmt, 0, sizeof(vacstmt));
@@ -2758,18 +2759,20 @@ autovacuum_do_vac_analyze(autovac_table *tab,
 		vacstmt.options |= VACOPT_VACUUM;
 	if (tab->at_doanalyze)
 		vacstmt.options |= VACOPT_ANALYZE;
-	vacstmt.freeze_min_age = tab->at_freeze_min_age;
-	vacstmt.freeze_table_age = tab->at_freeze_table_age;
-	vacstmt.multixact_freeze_min_age = tab->at_multixact_freeze_min_age;
-	vacstmt.multixact_freeze_table_age = tab->at_multixact_freeze_table_age;
 	/* we pass the OID, but might need this anyway for an error message */
 	vacstmt.relation = &rangevar;
 	vacstmt.va_cols = NIL;
 
+	params.freeze_min_age = tab->at_freeze_min_age;
+	params.freeze_table_age = tab->at_freeze_table_age;
+	params.multixact_freeze_min_age = tab->at_multixact_freeze_min_age;
+	params.multixact_freeze_table_age = tab->at_multixact_freeze_table_age;
+
 	/* Let pgstat know what we're doing */
 	autovac_report_activity(tab);
 
-	vacuum(&vacstmt, tab->at_relid, false, bstrategy, tab->at_wraparound, true);
+	vacuum(&vacstmt, &params, tab->at_relid, false, bstrategy,
+		   tab->at_wraparound, true);
 }
 
 /*
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index daf5326..774b99f 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -627,7 +627,7 @@ standard_ProcessUtility(Node *parsetree,
 				/* we choose to allow this during "read only" transactions */
 				PreventCommandDuringRecovery((stmt->options & VACOPT_VACUUM) ?
 											 "VACUUM" : "ANALYZE");
-				vacuum(stmt, InvalidOid, true, NULL, false, isTopLevel);
+				vacuum(stmt, NULL, InvalidOid, true, NULL, false, isTopLevel);
 			}
 			break;
 
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 4275484..78f4019 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -130,6 +130,18 @@ typedef struct VacAttrStats
 	int			rowstride;
 } VacAttrStats;
 
+/*
+ * Parameters customizing behavior of a VACUUM or ANALYZE query.
+ */
+typedef struct VacuumParams
+{
+	int		freeze_min_age;		/* min freeze age, or -1 to use default */
+	int		freeze_table_age;	/* age at which to scan whole table */
+	int		multixact_freeze_min_age;	/* min multixact freeze age,
+										 * or -1 to use default */
+	int		multixact_freeze_table_age;	/* multixact age at which to
+										 * scan whole table */
+} VacuumParams;
 
 /* GUC parameters */
 extern PGDLLIMPORT int default_statistics_target;		/* PGDLLIMPORT for
@@ -141,8 +153,9 @@ extern int	vacuum_multixact_freeze_table_age;
 
 
 /* in commands/vacuum.c */
-extern void vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
-	   BufferAccessStrategy bstrategy, bool for_wraparound, bool isTopLevel);
+extern void vacuum(VacuumStmt *vacstmt, VacuumParams *params, Oid relid,
+				   bool do_toast, BufferAccessStrategy bstrategy,
+				   bool for_wraparound, bool isTopLevel);
 extern void vac_open_indexes(Relation relation, LOCKMODE lockmode,
 				 int *nindexes, Relation **Irel);
 extern void vac_close_indexes(int nindexes, Relation *Irel, LOCKMODE lockmode);
@@ -172,7 +185,7 @@ extern void vacuum_delay_point(void);
 
 /* in commands/vacuumlazy.c */
 extern void lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
-				BufferAccessStrategy bstrategy);
+				VacuumParams *params, BufferAccessStrategy bstrategy);
 
 /* in commands/analyze.c */
 extern void analyze_rel(Oid relid, VacuumStmt *vacstmt,
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index ac13302..4c8b14c 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2598,9 +2598,7 @@ typedef struct ClusterStmt
  *
  * Even though these are nominally two statements, it's convenient to use
  * just one node type for both.  Note that at least one of VACOPT_VACUUM
- * and VACOPT_ANALYZE must be set in options.  VACOPT_FREEZE is an internal
- * convenience for the grammar and is not examined at runtime --- the
- * freeze_min_age and freeze_table_age fields are what matter.
+ * and VACOPT_ANALYZE must be set in options.
  * ----------------------
  */
 typedef enum VacuumOption
@@ -2615,14 +2613,8 @@ typedef enum VacuumOption
 
 typedef struct VacuumStmt
 {
-	NodeTag		type;
-	int			options;		/* OR of VacuumOption flags */
-	int			freeze_min_age; /* min freeze age, or -1 to use default */
-	int			freeze_table_age;		/* age at which to scan whole table */
-	int			multixact_freeze_min_age;		/* min multixact freeze age,
-												 * or -1 to use default */
-	int			multixact_freeze_table_age;		/* multixact age at which to
-												 * scan whole table */
+	NodeTag			type;
+	int				options;	/* OR of VacuumOption flags */
 	RangeVar   *relation;		/* single table to process, or NULL */
 	List	   *va_cols;		/* list of column names, or NIL for all */
 } VacuumStmt;
-- 
2.3.1

0002-Eliminate-VacuumStmt-from-lower-level-routines-of-AN.patchapplication/x-patch; name=0002-Eliminate-VacuumStmt-from-lower-level-routines-of-AN.patchDownload
From e051390451e9c631383248ce75e2f6422197cc1e Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@otacoo.com>
Date: Fri, 6 Mar 2015 14:48:21 +0900
Subject: [PATCH 2/3] Eliminate VacuumStmt from lower-level routines of ANALYZE
 and VACUUM

With this patch it is only kept as part of vacuum().
---
 src/backend/commands/analyze.c    | 32 ++++++++++++++++----------------
 src/backend/commands/vacuum.c     | 33 +++++++++++++++++++--------------
 src/backend/commands/vacuumlazy.c |  6 +++---
 src/include/commands/vacuum.h     |  7 ++++---
 4 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index d2856a3..75b45f7 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -85,7 +85,7 @@ static MemoryContext anl_context = NULL;
 static BufferAccessStrategy vac_strategy;
 
 
-static void do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
+static void do_analyze_rel(Relation onerel, int options, List *va_cols,
 			   AcquireSampleRowsFunc acquirefunc, BlockNumber relpages,
 			   bool inh, bool in_outer_xact, int elevel);
 static void BlockSampler_Init(BlockSampler bs, BlockNumber nblocks,
@@ -115,7 +115,7 @@ static Datum ind_fetch_func(VacAttrStatsP stats, int rownum, bool *isNull);
  *	analyze_rel() -- analyze one relation
  */
 void
-analyze_rel(Oid relid, VacuumStmt *vacstmt,
+analyze_rel(Oid relid, RangeVar *relation, int options, List *va_cols,
 			bool in_outer_xact, BufferAccessStrategy bstrategy)
 {
 	Relation	onerel;
@@ -124,7 +124,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
 	BlockNumber relpages = 0;
 
 	/* Select logging level */
-	if (vacstmt->options & VACOPT_VERBOSE)
+	if (options & VACOPT_VERBOSE)
 		elevel = INFO;
 	else
 		elevel = DEBUG2;
@@ -144,7 +144,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
 	 * matter if we ever try to accumulate stats on dead tuples.) If the rel
 	 * has been dropped since we last saw it, we don't need to process it.
 	 */
-	if (!(vacstmt->options & VACOPT_NOWAIT))
+	if (!(options & VACOPT_NOWAIT))
 		onerel = try_relation_open(relid, ShareUpdateExclusiveLock);
 	else if (ConditionalLockRelationOid(relid, ShareUpdateExclusiveLock))
 		onerel = try_relation_open(relid, NoLock);
@@ -155,7 +155,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
 			ereport(LOG,
 					(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
 				  errmsg("skipping analyze of \"%s\" --- lock not available",
-						 vacstmt->relation->relname)));
+						 relation->relname)));
 	}
 	if (!onerel)
 		return;
@@ -167,7 +167,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
 		  (pg_database_ownercheck(MyDatabaseId, GetUserId()) && !onerel->rd_rel->relisshared)))
 	{
 		/* No need for a WARNING if we already complained during VACUUM */
-		if (!(vacstmt->options & VACOPT_VACUUM))
+		if (!(options & VACOPT_VACUUM))
 		{
 			if (onerel->rd_rel->relisshared)
 				ereport(WARNING,
@@ -248,7 +248,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
 	else
 	{
 		/* No need for a WARNING if we already complained during VACUUM */
-		if (!(vacstmt->options & VACOPT_VACUUM))
+		if (!(options & VACOPT_VACUUM))
 			ereport(WARNING,
 					(errmsg("skipping \"%s\" --- cannot analyze non-tables or special system tables",
 							RelationGetRelationName(onerel))));
@@ -266,14 +266,14 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
 	/*
 	 * Do the normal non-recursive ANALYZE.
 	 */
-	do_analyze_rel(onerel, vacstmt, acquirefunc, relpages,
+	do_analyze_rel(onerel, options, va_cols, acquirefunc, relpages,
 				   false, in_outer_xact, elevel);
 
 	/*
 	 * If there are child tables, do recursive ANALYZE.
 	 */
 	if (onerel->rd_rel->relhassubclass)
-		do_analyze_rel(onerel, vacstmt, acquirefunc, relpages,
+		do_analyze_rel(onerel, options, va_cols, acquirefunc, relpages,
 					   true, in_outer_xact, elevel);
 
 	/*
@@ -302,7 +302,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
  * acquirefunc for each child table.
  */
 static void
-do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
+do_analyze_rel(Relation onerel, int options, List *va_cols,
 			   AcquireSampleRowsFunc acquirefunc, BlockNumber relpages,
 			   bool inh, bool in_outer_xact, int elevel)
 {
@@ -372,14 +372,14 @@ do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
 	 *
 	 * Note that system attributes are never analyzed.
 	 */
-	if (vacstmt->va_cols != NIL)
+	if (va_cols != NIL)
 	{
 		ListCell   *le;
 
-		vacattrstats = (VacAttrStats **) palloc(list_length(vacstmt->va_cols) *
+		vacattrstats = (VacAttrStats **) palloc(list_length(va_cols) *
 												sizeof(VacAttrStats *));
 		tcnt = 0;
-		foreach(le, vacstmt->va_cols)
+		foreach(le, va_cols)
 		{
 			char	   *col = strVal(lfirst(le));
 
@@ -436,7 +436,7 @@ do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
 
 			thisdata->indexInfo = indexInfo = BuildIndexInfo(Irel[ind]);
 			thisdata->tupleFract = 1.0; /* fix later if partial */
-			if (indexInfo->ii_Expressions != NIL && vacstmt->va_cols == NIL)
+			if (indexInfo->ii_Expressions != NIL && va_cols == NIL)
 			{
 				ListCell   *indexpr_item = list_head(indexInfo->ii_Expressions);
 
@@ -595,7 +595,7 @@ do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
 	 * VACUUM ANALYZE, don't overwrite the accurate count already inserted by
 	 * VACUUM.
 	 */
-	if (!inh && !(vacstmt->options & VACOPT_VACUUM))
+	if (!inh && !(options & VACOPT_VACUUM))
 	{
 		for (ind = 0; ind < nindexes; ind++)
 		{
@@ -623,7 +623,7 @@ do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
 		pgstat_report_analyze(onerel, totalrows, totaldeadrows);
 
 	/* If this isn't part of VACUUM ANALYZE, let index AMs do cleanup */
-	if (!(vacstmt->options & VACOPT_VACUUM))
+	if (!(options & VACOPT_VACUUM))
 	{
 		for (ind = 0; ind < nindexes; ind++)
 		{
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index c2234e6..d6091c0 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -71,8 +71,9 @@ static void vac_truncate_clog(TransactionId frozenXID,
 				  MultiXactId minMulti,
 				  TransactionId lastSaneFrozenXid,
 				  MultiXactId lastSaneMinMulti);
-static bool vacuum_rel(Oid relid, VacuumStmt *vacstmt, VacuumParams *params,
-					   bool do_toast, bool for_wraparound);
+static bool vacuum_rel(Oid relid, RangeVar *relation, int options,
+					   VacuumParams *params, bool do_toast,
+					   bool for_wraparound);
 
 
 /*
@@ -250,7 +251,9 @@ vacuum(VacuumStmt *vacstmt, VacuumParams *params, Oid relid, bool do_toast,
 
 			if (vacstmt->options & VACOPT_VACUUM)
 			{
-				if (!vacuum_rel(relid, vacstmt, params, do_toast, for_wraparound))
+				if (!vacuum_rel(relid, vacstmt->relation,
+								vacstmt->options, params, do_toast,
+								for_wraparound))
 					continue;
 			}
 
@@ -267,7 +270,8 @@ vacuum(VacuumStmt *vacstmt, VacuumParams *params, Oid relid, bool do_toast,
 					PushActiveSnapshot(GetTransactionSnapshot());
 				}
 
-				analyze_rel(relid, vacstmt, in_outer_xact, vac_strategy);
+				analyze_rel(relid, vacstmt->relation, vacstmt->options,
+							vacstmt->va_cols, in_outer_xact, vac_strategy);
 
 				if (use_own_xacts)
 				{
@@ -1116,7 +1120,7 @@ vac_truncate_clog(TransactionId frozenXID,
  *		At entry and exit, we are not inside a transaction.
  */
 static bool
-vacuum_rel(Oid relid, VacuumStmt *vacstmt, VacuumParams *params,
+vacuum_rel(Oid relid, RangeVar *relation, int options, VacuumParams *params,
 		   bool do_toast, bool for_wraparound)
 {
 	LOCKMODE	lmode;
@@ -1136,7 +1140,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, VacuumParams *params,
 	 */
 	PushActiveSnapshot(GetTransactionSnapshot());
 
-	if (!(vacstmt->options & VACOPT_FULL))
+	if (!(options & VACOPT_FULL))
 	{
 		/*
 		 * In lazy vacuum, we can set the PROC_IN_VACUUM flag, which lets
@@ -1176,7 +1180,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, VacuumParams *params,
 	 * vacuum, but just ShareUpdateExclusiveLock for concurrent vacuum. Either
 	 * way, we can be sure that no other backend is vacuuming the same table.
 	 */
-	lmode = (vacstmt->options & VACOPT_FULL) ? AccessExclusiveLock : ShareUpdateExclusiveLock;
+	lmode = (options & VACOPT_FULL) ? AccessExclusiveLock : ShareUpdateExclusiveLock;
 
 	/*
 	 * Open the relation and get the appropriate lock on it.
@@ -1187,7 +1191,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, VacuumParams *params,
 	 * If we've been asked not to wait for the relation lock, acquire it first
 	 * in non-blocking mode, before calling try_relation_open().
 	 */
-	if (!(vacstmt->options & VACOPT_NOWAIT))
+	if (!(options & VACOPT_NOWAIT))
 		onerel = try_relation_open(relid, lmode);
 	else if (ConditionalLockRelationOid(relid, lmode))
 		onerel = try_relation_open(relid, NoLock);
@@ -1198,7 +1202,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, VacuumParams *params,
 			ereport(LOG,
 					(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
 				   errmsg("skipping vacuum of \"%s\" --- lock not available",
-						  vacstmt->relation->relname)));
+						  relation->relname)));
 	}
 
 	if (!onerel)
@@ -1290,7 +1294,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, VacuumParams *params,
 	 * us to process it.  In VACUUM FULL, though, the toast table is
 	 * automatically rebuilt by cluster_rel so we shouldn't recurse to it.
 	 */
-	if (do_toast && !(vacstmt->options & VACOPT_FULL))
+	if (do_toast && !(options & VACOPT_FULL))
 		toast_relid = onerel->rd_rel->reltoastrelid;
 	else
 		toast_relid = InvalidOid;
@@ -1309,7 +1313,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, VacuumParams *params,
 	/*
 	 * Do the actual work --- either FULL or "lazy" vacuum
 	 */
-	if (vacstmt->options & VACOPT_FULL)
+	if (options & VACOPT_FULL)
 	{
 		/* close relation before vacuuming, but hold lock until commit */
 		relation_close(onerel, NoLock);
@@ -1317,10 +1321,10 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, VacuumParams *params,
 
 		/* VACUUM FULL is now a variant of CLUSTER; see cluster.c */
 		cluster_rel(relid, InvalidOid, false,
-					(vacstmt->options & VACOPT_VERBOSE) != 0);
+					(options & VACOPT_VERBOSE) != 0);
 	}
 	else
-		lazy_vacuum_rel(onerel, vacstmt, params, vac_strategy);
+		lazy_vacuum_rel(onerel, options, params, vac_strategy);
 
 	/* Roll back any GUC changes executed by index functions */
 	AtEOXact_GUC(false, save_nestlevel);
@@ -1346,7 +1350,8 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, VacuumParams *params,
 	 * totally unimportant for toast relations.
 	 */
 	if (toast_relid != InvalidOid)
-		vacuum_rel(toast_relid, vacstmt, params, false, for_wraparound);
+		vacuum_rel(toast_relid, relation, options, params, false,
+				   for_wraparound);
 
 	/*
 	 * Now release the session-level lock on the master table.
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index adab3b7..f1074cc 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -169,7 +169,7 @@ static bool heap_page_is_all_visible(Relation rel, Buffer buf,
  *		and locked the relation.
  */
 void
-lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt, VacuumParams *params,
+lazy_vacuum_rel(Relation onerel, int options, VacuumParams *params,
 				BufferAccessStrategy bstrategy)
 {
 	LVRelStats *vacrelstats;
@@ -204,7 +204,7 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt, VacuumParams *params,
 		starttime = GetCurrentTimestamp();
 	}
 
-	if (vacstmt->options & VACOPT_VERBOSE)
+	if (options & VACOPT_VERBOSE)
 		elevel = INFO;
 	else
 		elevel = DEBUG2;
@@ -221,7 +221,7 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt, VacuumParams *params,
 	}
 	else
 	{
-		if (vacstmt->options & VACOPT_FREEZE)
+		if (options & VACOPT_FREEZE)
 		{
 			freeze_min_age = 0;
 			freeze_table_age = 0;
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 78f4019..58dd2ee 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -184,12 +184,13 @@ extern void vac_update_datfrozenxid(void);
 extern void vacuum_delay_point(void);
 
 /* in commands/vacuumlazy.c */
-extern void lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
+extern void lazy_vacuum_rel(Relation onerel, int options,
 				VacuumParams *params, BufferAccessStrategy bstrategy);
 
 /* in commands/analyze.c */
-extern void analyze_rel(Oid relid, VacuumStmt *vacstmt,
-			bool in_outer_xact, BufferAccessStrategy bstrategy);
+extern void analyze_rel(Oid relid, RangeVar *relation, int options,
+			List *va_cols, bool in_outer_xact,
+			BufferAccessStrategy bstrategy);
 extern bool std_typanalyze(VacAttrStats *stats);
 extern double anl_random_fract(void);
 extern double anl_init_selection_state(int n);
-- 
2.3.1

0003-Add-wraparound-control-parameters-in-VacuumStmt.patchapplication/x-patch; name=0003-Add-wraparound-control-parameters-in-VacuumStmt.patchDownload
From 2fb3f61e7b5eeef7c6ac35bb643c730230ef180c Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@otacoo.com>
Date: Fri, 6 Mar 2015 15:34:23 +0900
Subject: [PATCH 3/3] Add wraparound control parameters in VacuumStmt

This is easy to manipulate across the stack of vacuum routines as it
does not need special handling, contrary to relid and do_toast when
vacuum_rel is called for toast relations.
---
 src/backend/commands/vacuum.c       | 20 +++++++++-----------
 src/backend/postmaster/autovacuum.c |  4 ++--
 src/backend/tcop/utility.c          |  2 +-
 src/include/commands/vacuum.h       |  4 +++-
 4 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index d6091c0..385e478 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -72,8 +72,7 @@ static void vac_truncate_clog(TransactionId frozenXID,
 				  TransactionId lastSaneFrozenXid,
 				  MultiXactId lastSaneMinMulti);
 static bool vacuum_rel(Oid relid, RangeVar *relation, int options,
-					   VacuumParams *params, bool do_toast,
-					   bool for_wraparound);
+					   VacuumParams *params, bool do_toast);
 
 
 /*
@@ -89,9 +88,6 @@ static bool vacuum_rel(Oid relid, RangeVar *relation, int options,
  * do_toast is passed as FALSE by autovacuum, because it processes TOAST
  * tables separately.
  *
- * for_wraparound is used by autovacuum to let us know when it's forcing
- * a vacuum for wraparound, which should not be auto-canceled.
- *
  * bstrategy is normally given as NULL, but in autovacuum it can be passed
  * in to use the same buffer strategy object across multiple vacuum() calls.
  *
@@ -103,7 +99,7 @@ static bool vacuum_rel(Oid relid, RangeVar *relation, int options,
  */
 void
 vacuum(VacuumStmt *vacstmt, VacuumParams *params, Oid relid, bool do_toast,
-	   BufferAccessStrategy bstrategy, bool for_wraparound, bool isTopLevel)
+	   BufferAccessStrategy bstrategy, bool isTopLevel)
 {
 	const char *stmttype;
 	volatile bool in_outer_xact,
@@ -252,8 +248,7 @@ vacuum(VacuumStmt *vacstmt, VacuumParams *params, Oid relid, bool do_toast,
 			if (vacstmt->options & VACOPT_VACUUM)
 			{
 				if (!vacuum_rel(relid, vacstmt->relation,
-								vacstmt->options, params, do_toast,
-								for_wraparound))
+								vacstmt->options, params, do_toast))
 					continue;
 			}
 
@@ -1121,7 +1116,7 @@ vac_truncate_clog(TransactionId frozenXID,
  */
 static bool
 vacuum_rel(Oid relid, RangeVar *relation, int options, VacuumParams *params,
-		   bool do_toast, bool for_wraparound)
+		   bool do_toast)
 {
 	LOCKMODE	lmode;
 	Relation	onerel;
@@ -1130,6 +1125,10 @@ vacuum_rel(Oid relid, RangeVar *relation, int options, VacuumParams *params,
 	Oid			save_userid;
 	int			save_sec_context;
 	int			save_nestlevel;
+	bool		for_wraparound;
+
+	/* Set parameters if present */
+	for_wraparound = params ? params->is_wraparound : false;
 
 	/* Begin a transaction for vacuuming this relation */
 	StartTransactionCommand();
@@ -1350,8 +1349,7 @@ vacuum_rel(Oid relid, RangeVar *relation, int options, VacuumParams *params,
 	 * totally unimportant for toast relations.
 	 */
 	if (toast_relid != InvalidOid)
-		vacuum_rel(toast_relid, relation, options, params, false,
-				   for_wraparound);
+		vacuum_rel(toast_relid, relation, options, params, false);
 
 	/*
 	 * Now release the session-level lock on the master table.
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 67c8cdc..8bcfee9 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2767,12 +2767,12 @@ autovacuum_do_vac_analyze(autovac_table *tab,
 	params.freeze_table_age = tab->at_freeze_table_age;
 	params.multixact_freeze_min_age = tab->at_multixact_freeze_min_age;
 	params.multixact_freeze_table_age = tab->at_multixact_freeze_table_age;
+	params.is_wraparound = tab->at_wraparound;
 
 	/* Let pgstat know what we're doing */
 	autovac_report_activity(tab);
 
-	vacuum(&vacstmt, &params, tab->at_relid, false, bstrategy,
-		   tab->at_wraparound, true);
+	vacuum(&vacstmt, &params, tab->at_relid, false, bstrategy, true);
 }
 
 /*
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 774b99f..4e4b11f 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -627,7 +627,7 @@ standard_ProcessUtility(Node *parsetree,
 				/* we choose to allow this during "read only" transactions */
 				PreventCommandDuringRecovery((stmt->options & VACOPT_VACUUM) ?
 											 "VACUUM" : "ANALYZE");
-				vacuum(stmt, NULL, InvalidOid, true, NULL, false, isTopLevel);
+				vacuum(stmt, NULL, InvalidOid, true, NULL, isTopLevel);
 			}
 			break;
 
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 58dd2ee..f636618 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -141,6 +141,8 @@ typedef struct VacuumParams
 										 * or -1 to use default */
 	int		multixact_freeze_table_age;	/* multixact age at which to
 										 * scan whole table */
+	bool	is_wraparound;		/* enforce a wraparound for vacuum, which
+								 * should not be auto-canceled */
 } VacuumParams;
 
 /* GUC parameters */
@@ -155,7 +157,7 @@ extern int	vacuum_multixact_freeze_table_age;
 /* in commands/vacuum.c */
 extern void vacuum(VacuumStmt *vacstmt, VacuumParams *params, Oid relid,
 				   bool do_toast, BufferAccessStrategy bstrategy,
-				   bool for_wraparound, bool isTopLevel);
+				   bool isTopLevel);
 extern void vac_open_indexes(Relation relation, LOCKMODE lockmode,
 				 int *nindexes, Relation **Irel);
 extern void vac_close_indexes(int nindexes, Relation *Irel, LOCKMODE lockmode);
-- 
2.3.1

#21Robert Haas
robertmhaas@gmail.com
In reply to: Michael Paquier (#20)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

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

#22Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Robert Haas (#21)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

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

#23Robert Haas
robertmhaas@gmail.com
In reply to: Alvaro Herrera (#22)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

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

#24Tom Lane
tgl@sss.pgh.pa.us
In reply to: Alvaro Herrera (#22)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

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

#25Jim Nasby
Jim.Nasby@BlueTreble.com
In reply to: Tom Lane (#24)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

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

#26Michael Paquier
michael.paquier@gmail.com
In reply to: Robert Haas (#23)
1 attachment(s)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

Attachments:

0001-Move-freeze-parameters-of-VacuumStmt-into-a-separate.patchtext/x-diff; charset=US-ASCII; name=0001-Move-freeze-parameters-of-VacuumStmt-into-a-separate.patchDownload
From 65052d1efea09c429f8ba37b85178e6c8c2c49aa Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@otacoo.com>
Date: Sun, 1 Mar 2015 20:45:06 +0900
Subject: [PATCH] Move freeze parameters of VacuumStmt into a separate
 structure

VACOPT_FREEZE was being limited in the query parser and still be used
in a set of assertions at the beginning of vacuum(). On the contrary,
the set of parameters controlling if FREEZE is used was being set in the
query parser but not used in the same set of assertions, leading to
confusion regarding both aspects of VacuumStmt.

This commit fixes this inconsistency by moving all the freeze parameters
to a dedicated structure used by autovacuum and by setting unconditionally
VACOPT_FREEZE for all the code paths of the query parser, making the whole
control of FREEZE more consistent in the autovacuum code path as well as
in the query parser.

At the same time, VacuumStmt is removed from all the existing code path,
making its use limited at the highest level of vacuum.c where its
execution is done as a utility. This has the advantage of making autovacuum
code path more simple.
---
 src/backend/commands/analyze.c      |  32 +++++-----
 src/backend/commands/vacuum.c       | 121 +++++++++++++++++++++++++-----------
 src/backend/commands/vacuumlazy.c   |  13 ++--
 src/backend/nodes/copyfuncs.c       |   4 --
 src/backend/nodes/equalfuncs.c      |   4 --
 src/backend/parser/gram.y           |  50 ++-------------
 src/backend/postmaster/autovacuum.c |  33 +++++-----
 src/backend/tcop/utility.c          |   2 +-
 src/include/commands/vacuum.h       |  29 +++++++--
 src/include/nodes/parsenodes.h      |  14 +----
 10 files changed, 159 insertions(+), 143 deletions(-)

diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index d2856a3..75b45f7 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -85,7 +85,7 @@ static MemoryContext anl_context = NULL;
 static BufferAccessStrategy vac_strategy;
 
 
-static void do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
+static void do_analyze_rel(Relation onerel, int options, List *va_cols,
 			   AcquireSampleRowsFunc acquirefunc, BlockNumber relpages,
 			   bool inh, bool in_outer_xact, int elevel);
 static void BlockSampler_Init(BlockSampler bs, BlockNumber nblocks,
@@ -115,7 +115,7 @@ static Datum ind_fetch_func(VacAttrStatsP stats, int rownum, bool *isNull);
  *	analyze_rel() -- analyze one relation
  */
 void
-analyze_rel(Oid relid, VacuumStmt *vacstmt,
+analyze_rel(Oid relid, RangeVar *relation, int options, List *va_cols,
 			bool in_outer_xact, BufferAccessStrategy bstrategy)
 {
 	Relation	onerel;
@@ -124,7 +124,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
 	BlockNumber relpages = 0;
 
 	/* Select logging level */
-	if (vacstmt->options & VACOPT_VERBOSE)
+	if (options & VACOPT_VERBOSE)
 		elevel = INFO;
 	else
 		elevel = DEBUG2;
@@ -144,7 +144,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
 	 * matter if we ever try to accumulate stats on dead tuples.) If the rel
 	 * has been dropped since we last saw it, we don't need to process it.
 	 */
-	if (!(vacstmt->options & VACOPT_NOWAIT))
+	if (!(options & VACOPT_NOWAIT))
 		onerel = try_relation_open(relid, ShareUpdateExclusiveLock);
 	else if (ConditionalLockRelationOid(relid, ShareUpdateExclusiveLock))
 		onerel = try_relation_open(relid, NoLock);
@@ -155,7 +155,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
 			ereport(LOG,
 					(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
 				  errmsg("skipping analyze of \"%s\" --- lock not available",
-						 vacstmt->relation->relname)));
+						 relation->relname)));
 	}
 	if (!onerel)
 		return;
@@ -167,7 +167,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
 		  (pg_database_ownercheck(MyDatabaseId, GetUserId()) && !onerel->rd_rel->relisshared)))
 	{
 		/* No need for a WARNING if we already complained during VACUUM */
-		if (!(vacstmt->options & VACOPT_VACUUM))
+		if (!(options & VACOPT_VACUUM))
 		{
 			if (onerel->rd_rel->relisshared)
 				ereport(WARNING,
@@ -248,7 +248,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
 	else
 	{
 		/* No need for a WARNING if we already complained during VACUUM */
-		if (!(vacstmt->options & VACOPT_VACUUM))
+		if (!(options & VACOPT_VACUUM))
 			ereport(WARNING,
 					(errmsg("skipping \"%s\" --- cannot analyze non-tables or special system tables",
 							RelationGetRelationName(onerel))));
@@ -266,14 +266,14 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
 	/*
 	 * Do the normal non-recursive ANALYZE.
 	 */
-	do_analyze_rel(onerel, vacstmt, acquirefunc, relpages,
+	do_analyze_rel(onerel, options, va_cols, acquirefunc, relpages,
 				   false, in_outer_xact, elevel);
 
 	/*
 	 * If there are child tables, do recursive ANALYZE.
 	 */
 	if (onerel->rd_rel->relhassubclass)
-		do_analyze_rel(onerel, vacstmt, acquirefunc, relpages,
+		do_analyze_rel(onerel, options, va_cols, acquirefunc, relpages,
 					   true, in_outer_xact, elevel);
 
 	/*
@@ -302,7 +302,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
  * acquirefunc for each child table.
  */
 static void
-do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
+do_analyze_rel(Relation onerel, int options, List *va_cols,
 			   AcquireSampleRowsFunc acquirefunc, BlockNumber relpages,
 			   bool inh, bool in_outer_xact, int elevel)
 {
@@ -372,14 +372,14 @@ do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
 	 *
 	 * Note that system attributes are never analyzed.
 	 */
-	if (vacstmt->va_cols != NIL)
+	if (va_cols != NIL)
 	{
 		ListCell   *le;
 
-		vacattrstats = (VacAttrStats **) palloc(list_length(vacstmt->va_cols) *
+		vacattrstats = (VacAttrStats **) palloc(list_length(va_cols) *
 												sizeof(VacAttrStats *));
 		tcnt = 0;
-		foreach(le, vacstmt->va_cols)
+		foreach(le, va_cols)
 		{
 			char	   *col = strVal(lfirst(le));
 
@@ -436,7 +436,7 @@ do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
 
 			thisdata->indexInfo = indexInfo = BuildIndexInfo(Irel[ind]);
 			thisdata->tupleFract = 1.0; /* fix later if partial */
-			if (indexInfo->ii_Expressions != NIL && vacstmt->va_cols == NIL)
+			if (indexInfo->ii_Expressions != NIL && va_cols == NIL)
 			{
 				ListCell   *indexpr_item = list_head(indexInfo->ii_Expressions);
 
@@ -595,7 +595,7 @@ do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
 	 * VACUUM ANALYZE, don't overwrite the accurate count already inserted by
 	 * VACUUM.
 	 */
-	if (!inh && !(vacstmt->options & VACOPT_VACUUM))
+	if (!inh && !(options & VACOPT_VACUUM))
 	{
 		for (ind = 0; ind < nindexes; ind++)
 		{
@@ -623,7 +623,7 @@ do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
 		pgstat_report_analyze(onerel, totalrows, totaldeadrows);
 
 	/* If this isn't part of VACUUM ANALYZE, let index AMs do cleanup */
-	if (!(vacstmt->options & VACOPT_VACUUM))
+	if (!(options & VACOPT_VACUUM))
 	{
 		for (ind = 0; ind < nindexes; ind++)
 		{
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 2f3f79d..885b5cd 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -71,35 +71,81 @@ static void vac_truncate_clog(TransactionId frozenXID,
 				  MultiXactId minMulti,
 				  TransactionId lastSaneFrozenXid,
 				  MultiXactId lastSaneMinMulti);
-static bool vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast,
-		   bool for_wraparound);
+static bool vacuum_rel(Oid relid, RangeVar *relation, int options,
+					   VacuumParams *params, bool do_toast);
 
+/*
+ * Primary entry point for manual VACUUM and ANALYZE commands
+ *
+ * This is mainly a preparation wrapper for the real operations that will
+ * happen in vacuum().
+ */
+void
+ExecVacuum(VacuumStmt *vacstmt, bool isTopLevel)
+{
+	VacuumParams *params;
+
+	/* sanity checks on options */
+	Assert(vacstmt->options & (VACOPT_VACUUM | VACOPT_ANALYZE));
+	Assert((vacstmt->options & VACOPT_VACUUM) ||
+		   !(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE)));
+	Assert((vacstmt->options & VACOPT_ANALYZE) || vacstmt->va_cols == NIL);
+
+	/* Build list of parameters */
+	params = (VacuumParams *) palloc(sizeof(VacuumParams));
+	if (vacstmt->options & VACOPT_FREEZE)
+	{
+		params->freeze_min_age = 0;
+		params->freeze_table_age = 0;
+		params->multixact_freeze_min_age = 0;
+		params->multixact_freeze_table_age = 0;
+	}
+	else
+	{
+		params->freeze_min_age = -1;
+		params->freeze_table_age = -1;
+		params->multixact_freeze_min_age = -1;
+		params->multixact_freeze_table_age = -1;
+	}
+	params->is_wraparound = false;
+
+	/* Now go through the common routine */
+	vacuum(params, vacstmt->relation, vacstmt->options, vacstmt->va_cols,
+		   InvalidOid, true, NULL, isTopLevel);
+}
 
 /*
  * Primary entry point for VACUUM and ANALYZE commands.
  *
+ * params contains a set of parameters used by autovacuum to customize the
+ * behavior of process.
+ *
+ * relation is the relation RangeVar that will be operated.
+ *
+ * options is the list of options passed by the parser in case of a manual
+ * VACUUM or ANALYZE, or the options defined by autovacuum.
+ *
  * relid is normally InvalidOid; if it is not, then it provides the relation
  * OID to be processed, and vacstmt->relation is ignored.  (The non-invalid
  * case is currently only used by autovacuum.)
  *
+ * va_cols is a list of columns to analyze, or NIL to list all the columns.
+ *
  * do_toast is passed as FALSE by autovacuum, because it processes TOAST
  * tables separately.
  *
- * for_wraparound is used by autovacuum to let us know when it's forcing
- * a vacuum for wraparound, which should not be auto-canceled.
- *
  * bstrategy is normally given as NULL, but in autovacuum it can be passed
  * in to use the same buffer strategy object across multiple vacuum() calls.
  *
  * isTopLevel should be passed down from ProcessUtility.
  *
- * It is the caller's responsibility that vacstmt and bstrategy
- * (if given) be allocated in a memory context that won't disappear
- * at transaction commit.
+ * It is the caller's responsibility that params and bstrategy are allocated
+ * in a memory context that will not disappear at transaction commit.
  */
 void
-vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
-	   BufferAccessStrategy bstrategy, bool for_wraparound, bool isTopLevel)
+vacuum(VacuumParams *params, RangeVar *relation, int options,
+	   List *va_cols, Oid relid, bool do_toast, BufferAccessStrategy bstrategy,
+	   bool isTopLevel)
 {
 	const char *stmttype;
 	volatile bool in_outer_xact,
@@ -107,13 +153,9 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 	List	   *relations;
 	static bool in_vacuum = false;
 
-	/* sanity checks on options */
-	Assert(vacstmt->options & (VACOPT_VACUUM | VACOPT_ANALYZE));
-	Assert((vacstmt->options & VACOPT_VACUUM) ||
-		   !(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE)));
-	Assert((vacstmt->options & VACOPT_ANALYZE) || vacstmt->va_cols == NIL);
+	Assert(params != NULL);
 
-	stmttype = (vacstmt->options & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE";
+	stmttype = (options & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE";
 
 	/*
 	 * We cannot run VACUUM inside a user transaction block; if we were inside
@@ -123,7 +165,7 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 	 *
 	 * ANALYZE (without VACUUM) can run either way.
 	 */
-	if (vacstmt->options & VACOPT_VACUUM)
+	if (options & VACOPT_VACUUM)
 	{
 		PreventTransactionChain(isTopLevel, stmttype);
 		in_outer_xact = false;
@@ -143,7 +185,7 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 	 * Send info about dead objects to the statistics collector, unless we are
 	 * in autovacuum --- autovacuum.c does this for itself.
 	 */
-	if ((vacstmt->options & VACOPT_VACUUM) && !IsAutoVacuumWorkerProcess())
+	if ((options & VACOPT_VACUUM) && !IsAutoVacuumWorkerProcess())
 		pgstat_vacuum_stat();
 
 	/*
@@ -175,7 +217,7 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 	 * Build list of relations to process, unless caller gave us one. (If we
 	 * build one, we put it in vac_context for safekeeping.)
 	 */
-	relations = get_rel_oids(relid, vacstmt->relation);
+	relations = get_rel_oids(relid, relation);
 
 	/*
 	 * Decide whether we need to start/commit our own transactions.
@@ -191,11 +233,11 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 	 * transaction block, and also in an autovacuum worker, use own
 	 * transactions so we can release locks sooner.
 	 */
-	if (vacstmt->options & VACOPT_VACUUM)
+	if (options & VACOPT_VACUUM)
 		use_own_xacts = true;
 	else
 	{
-		Assert(vacstmt->options & VACOPT_ANALYZE);
+		Assert(options & VACOPT_ANALYZE);
 		if (IsAutoVacuumWorkerProcess())
 			use_own_xacts = true;
 		else if (in_outer_xact)
@@ -245,13 +287,14 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 		{
 			Oid			relid = lfirst_oid(cur);
 
-			if (vacstmt->options & VACOPT_VACUUM)
+			if (options & VACOPT_VACUUM)
 			{
-				if (!vacuum_rel(relid, vacstmt, do_toast, for_wraparound))
+				if (!vacuum_rel(relid, relation,
+								options, params, do_toast))
 					continue;
 			}
 
-			if (vacstmt->options & VACOPT_ANALYZE)
+			if (options & VACOPT_ANALYZE)
 			{
 				/*
 				 * If using separate xacts, start one for analyze. Otherwise,
@@ -264,7 +307,8 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 					PushActiveSnapshot(GetTransactionSnapshot());
 				}
 
-				analyze_rel(relid, vacstmt, in_outer_xact, vac_strategy);
+				analyze_rel(relid, relation, options,
+							va_cols, in_outer_xact, vac_strategy);
 
 				if (use_own_xacts)
 				{
@@ -299,7 +343,7 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 		StartTransactionCommand();
 	}
 
-	if ((vacstmt->options & VACOPT_VACUUM) && !IsAutoVacuumWorkerProcess())
+	if ((options & VACOPT_VACUUM) && !IsAutoVacuumWorkerProcess())
 	{
 		/*
 		 * Update pg_database.datfrozenxid, and truncate pg_clog if possible.
@@ -1113,7 +1157,8 @@ vac_truncate_clog(TransactionId frozenXID,
  *		At entry and exit, we are not inside a transaction.
  */
 static bool
-vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
+vacuum_rel(Oid relid, RangeVar *relation, int options, VacuumParams *params,
+		   bool do_toast)
 {
 	LOCKMODE	lmode;
 	Relation	onerel;
@@ -1123,6 +1168,8 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 	int			save_sec_context;
 	int			save_nestlevel;
 
+	Assert(params != NULL);
+
 	/* Begin a transaction for vacuuming this relation */
 	StartTransactionCommand();
 
@@ -1132,7 +1179,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 	 */
 	PushActiveSnapshot(GetTransactionSnapshot());
 
-	if (!(vacstmt->options & VACOPT_FULL))
+	if (!(options & VACOPT_FULL))
 	{
 		/*
 		 * In lazy vacuum, we can set the PROC_IN_VACUUM flag, which lets
@@ -1156,7 +1203,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 		 */
 		LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
 		MyPgXact->vacuumFlags |= PROC_IN_VACUUM;
-		if (for_wraparound)
+		if (params->is_wraparound)
 			MyPgXact->vacuumFlags |= PROC_VACUUM_FOR_WRAPAROUND;
 		LWLockRelease(ProcArrayLock);
 	}
@@ -1172,7 +1219,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 	 * vacuum, but just ShareUpdateExclusiveLock for concurrent vacuum. Either
 	 * way, we can be sure that no other backend is vacuuming the same table.
 	 */
-	lmode = (vacstmt->options & VACOPT_FULL) ? AccessExclusiveLock : ShareUpdateExclusiveLock;
+	lmode = (options & VACOPT_FULL) ? AccessExclusiveLock : ShareUpdateExclusiveLock;
 
 	/*
 	 * Open the relation and get the appropriate lock on it.
@@ -1183,7 +1230,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 	 * If we've been asked not to wait for the relation lock, acquire it first
 	 * in non-blocking mode, before calling try_relation_open().
 	 */
-	if (!(vacstmt->options & VACOPT_NOWAIT))
+	if (!(options & VACOPT_NOWAIT))
 		onerel = try_relation_open(relid, lmode);
 	else if (ConditionalLockRelationOid(relid, lmode))
 		onerel = try_relation_open(relid, NoLock);
@@ -1194,7 +1241,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 			ereport(LOG,
 					(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
 				   errmsg("skipping vacuum of \"%s\" --- lock not available",
-						  vacstmt->relation->relname)));
+						  relation->relname)));
 	}
 
 	if (!onerel)
@@ -1286,7 +1333,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 	 * us to process it.  In VACUUM FULL, though, the toast table is
 	 * automatically rebuilt by cluster_rel so we shouldn't recurse to it.
 	 */
-	if (do_toast && !(vacstmt->options & VACOPT_FULL))
+	if (do_toast && !(options & VACOPT_FULL))
 		toast_relid = onerel->rd_rel->reltoastrelid;
 	else
 		toast_relid = InvalidOid;
@@ -1305,7 +1352,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 	/*
 	 * Do the actual work --- either FULL or "lazy" vacuum
 	 */
-	if (vacstmt->options & VACOPT_FULL)
+	if (options & VACOPT_FULL)
 	{
 		/* close relation before vacuuming, but hold lock until commit */
 		relation_close(onerel, NoLock);
@@ -1313,10 +1360,10 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 
 		/* VACUUM FULL is now a variant of CLUSTER; see cluster.c */
 		cluster_rel(relid, InvalidOid, false,
-					(vacstmt->options & VACOPT_VERBOSE) != 0);
+					(options & VACOPT_VERBOSE) != 0);
 	}
 	else
-		lazy_vacuum_rel(onerel, vacstmt, vac_strategy);
+		lazy_vacuum_rel(onerel, options, params, vac_strategy);
 
 	/* Roll back any GUC changes executed by index functions */
 	AtEOXact_GUC(false, save_nestlevel);
@@ -1342,7 +1389,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 	 * totally unimportant for toast relations.
 	 */
 	if (toast_relid != InvalidOid)
-		vacuum_rel(toast_relid, vacstmt, false, for_wraparound);
+		vacuum_rel(toast_relid, relation, options, params, false);
 
 	/*
 	 * Now release the session-level lock on the master table.
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 7d9e49e..cd5ca4c 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -169,7 +169,7 @@ static bool heap_page_is_all_visible(Relation rel, Buffer buf,
  *		and locked the relation.
  */
 void
-lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
+lazy_vacuum_rel(Relation onerel, int options, VacuumParams *params,
 				BufferAccessStrategy bstrategy)
 {
 	LVRelStats *vacrelstats;
@@ -193,6 +193,8 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
 	TransactionId new_frozen_xid;
 	MultiXactId new_min_multi;
 
+	Assert(params != NULL);
+
 	/* measure elapsed time iff autovacuum logging requires it */
 	if (IsAutoVacuumWorkerProcess() && Log_autovacuum_min_duration >= 0)
 	{
@@ -200,7 +202,7 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
 		starttime = GetCurrentTimestamp();
 	}
 
-	if (vacstmt->options & VACOPT_VERBOSE)
+	if (options & VACOPT_VERBOSE)
 		elevel = INFO;
 	else
 		elevel = DEBUG2;
@@ -208,9 +210,10 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
 	vac_strategy = bstrategy;
 
 	vacuum_set_xid_limits(onerel,
-						  vacstmt->freeze_min_age, vacstmt->freeze_table_age,
-						  vacstmt->multixact_freeze_min_age,
-						  vacstmt->multixact_freeze_table_age,
+						  params->freeze_min_age,
+						  params->freeze_table_age,
+						  params->multixact_freeze_min_age,
+						  params->multixact_freeze_table_age,
 						  &OldestXmin, &FreezeLimit, &xidFullScanLimit,
 						  &MultiXactCutoff, &mxactFullScanLimit);
 
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 9fe8008..4f0e278 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -3292,10 +3292,6 @@ _copyVacuumStmt(const VacuumStmt *from)
 	VacuumStmt *newnode = makeNode(VacuumStmt);
 
 	COPY_SCALAR_FIELD(options);
-	COPY_SCALAR_FIELD(freeze_min_age);
-	COPY_SCALAR_FIELD(freeze_table_age);
-	COPY_SCALAR_FIELD(multixact_freeze_min_age);
-	COPY_SCALAR_FIELD(multixact_freeze_table_age);
 	COPY_NODE_FIELD(relation);
 	COPY_NODE_FIELD(va_cols);
 
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index fe509b0..6b55500 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1507,10 +1507,6 @@ static bool
 _equalVacuumStmt(const VacuumStmt *a, const VacuumStmt *b)
 {
 	COMPARE_SCALAR_FIELD(options);
-	COMPARE_SCALAR_FIELD(freeze_min_age);
-	COMPARE_SCALAR_FIELD(freeze_table_age);
-	COMPARE_SCALAR_FIELD(multixact_freeze_min_age);
-	COMPARE_SCALAR_FIELD(multixact_freeze_table_age);
 	COMPARE_NODE_FIELD(relation);
 	COMPARE_NODE_FIELD(va_cols);
 
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 581f7a1..4b9a17f 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -9068,12 +9068,10 @@ VacuumStmt: VACUUM opt_full opt_freeze opt_verbose
 					n->options = VACOPT_VACUUM;
 					if ($2)
 						n->options |= VACOPT_FULL;
+					if ($3)
+						n->options |= VACOPT_FREEZE;
 					if ($4)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = $3 ? 0 : -1;
-					n->freeze_table_age = $3 ? 0 : -1;
-					n->multixact_freeze_min_age = $3 ? 0 : -1;
-					n->multixact_freeze_table_age = $3 ? 0 : -1;
 					n->relation = NULL;
 					n->va_cols = NIL;
 					$$ = (Node *)n;
@@ -9084,12 +9082,10 @@ VacuumStmt: VACUUM opt_full opt_freeze opt_verbose
 					n->options = VACOPT_VACUUM;
 					if ($2)
 						n->options |= VACOPT_FULL;
+					if ($3)
+						n->options |= VACOPT_FREEZE;
 					if ($4)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = $3 ? 0 : -1;
-					n->freeze_table_age = $3 ? 0 : -1;
-					n->multixact_freeze_min_age = $3 ? 0 : -1;
-					n->multixact_freeze_table_age = $3 ? 0 : -1;
 					n->relation = $5;
 					n->va_cols = NIL;
 					$$ = (Node *)n;
@@ -9100,30 +9096,16 @@ VacuumStmt: VACUUM opt_full opt_freeze opt_verbose
 					n->options |= VACOPT_VACUUM;
 					if ($2)
 						n->options |= VACOPT_FULL;
+					if ($3)
+						n->options |= VACOPT_FREEZE;
 					if ($4)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = $3 ? 0 : -1;
-					n->freeze_table_age = $3 ? 0 : -1;
-					n->multixact_freeze_min_age = $3 ? 0 : -1;
-					n->multixact_freeze_table_age = $3 ? 0 : -1;
 					$$ = (Node *)n;
 				}
 			| VACUUM '(' vacuum_option_list ')'
 				{
 					VacuumStmt *n = makeNode(VacuumStmt);
 					n->options = VACOPT_VACUUM | $3;
-					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;
-					}
 					n->relation = NULL;
 					n->va_cols = NIL;
 					$$ = (Node *) n;
@@ -9132,18 +9114,6 @@ VacuumStmt: VACUUM opt_full opt_freeze opt_verbose
 				{
 					VacuumStmt *n = makeNode(VacuumStmt);
 					n->options = VACOPT_VACUUM | $3;
-					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;
-					}
 					n->relation = $5;
 					n->va_cols = $6;
 					if (n->va_cols != NIL)	/* implies analyze */
@@ -9171,10 +9141,6 @@ AnalyzeStmt:
 					n->options = VACOPT_ANALYZE;
 					if ($2)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = -1;
-					n->freeze_table_age = -1;
-					n->multixact_freeze_min_age = -1;
-					n->multixact_freeze_table_age = -1;
 					n->relation = NULL;
 					n->va_cols = NIL;
 					$$ = (Node *)n;
@@ -9185,10 +9151,6 @@ AnalyzeStmt:
 					n->options = VACOPT_ANALYZE;
 					if ($2)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = -1;
-					n->freeze_table_age = -1;
-					n->multixact_freeze_min_age = -1;
-					n->multixact_freeze_table_age = -1;
 					n->relation = $3;
 					n->va_cols = $4;
 					$$ = (Node *)n;
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 77158c1..447531e 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2740,36 +2740,39 @@ static void
 autovacuum_do_vac_analyze(autovac_table *tab,
 						  BufferAccessStrategy bstrategy)
 {
-	VacuumStmt	vacstmt;
-	RangeVar	rangevar;
+	VacuumParams	params;
+	RangeVar		rangevar;
+	int				options = 0;
 
 	/* Set up command parameters --- use local variables instead of palloc */
-	MemSet(&vacstmt, 0, sizeof(vacstmt));
 	MemSet(&rangevar, 0, sizeof(rangevar));
 
 	rangevar.schemaname = tab->at_nspname;
 	rangevar.relname = tab->at_relname;
 	rangevar.location = -1;
 
-	vacstmt.type = T_VacuumStmt;
 	if (!tab->at_wraparound)
-		vacstmt.options = VACOPT_NOWAIT;
+		options = VACOPT_NOWAIT;
 	if (tab->at_dovacuum)
-		vacstmt.options |= VACOPT_VACUUM;
+		options |= VACOPT_VACUUM;
 	if (tab->at_doanalyze)
-		vacstmt.options |= VACOPT_ANALYZE;
-	vacstmt.freeze_min_age = tab->at_freeze_min_age;
-	vacstmt.freeze_table_age = tab->at_freeze_table_age;
-	vacstmt.multixact_freeze_min_age = tab->at_multixact_freeze_min_age;
-	vacstmt.multixact_freeze_table_age = tab->at_multixact_freeze_table_age;
-	/* we pass the OID, but might need this anyway for an error message */
-	vacstmt.relation = &rangevar;
-	vacstmt.va_cols = NIL;
+		options |= VACOPT_ANALYZE;
+
+	params.freeze_min_age = tab->at_freeze_min_age;
+	params.freeze_table_age = tab->at_freeze_table_age;
+	params.multixact_freeze_min_age = tab->at_multixact_freeze_min_age;
+	params.multixact_freeze_table_age = tab->at_multixact_freeze_table_age;
+	params.is_wraparound = tab->at_wraparound;
 
 	/* Let pgstat know what we're doing */
 	autovac_report_activity(tab);
 
-	vacuum(&vacstmt, tab->at_relid, false, bstrategy, tab->at_wraparound, true);
+	/*
+	 * Now go through the common routine. Note that we pass the OID, as it
+	 * might be needed anyway for an error message.
+	 */
+	vacuum(&params, &rangevar, options, NIL,
+		   tab->at_relid, false, bstrategy, true);
 }
 
 /*
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index daf5326..bd26227 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -627,7 +627,7 @@ standard_ProcessUtility(Node *parsetree,
 				/* we choose to allow this during "read only" transactions */
 				PreventCommandDuringRecovery((stmt->options & VACOPT_VACUUM) ?
 											 "VACUUM" : "ANALYZE");
-				vacuum(stmt, InvalidOid, true, NULL, false, isTopLevel);
+				ExecVacuum(stmt, isTopLevel);
 			}
 			break;
 
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 4275484..11d618e 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -130,6 +130,20 @@ typedef struct VacAttrStats
 	int			rowstride;
 } VacAttrStats;
 
+/*
+ * Parameters customizing behavior of a VACUUM or ANALYZE query.
+ */
+typedef struct VacuumParams
+{
+	int		freeze_min_age;		/* min freeze age, or -1 to use default */
+	int		freeze_table_age;	/* age at which to scan whole table */
+	int		multixact_freeze_min_age;	/* min multixact freeze age,
+										 * or -1 to use default */
+	int		multixact_freeze_table_age;	/* multixact age at which to
+										 * scan whole table */
+	bool	is_wraparound;		/* enforce a wraparound for vacuum, which
+								 * should not be auto-canceled */
+} VacuumParams;
 
 /* GUC parameters */
 extern PGDLLIMPORT int default_statistics_target;		/* PGDLLIMPORT for
@@ -141,8 +155,10 @@ extern int	vacuum_multixact_freeze_table_age;
 
 
 /* in commands/vacuum.c */
-extern void vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
-	   BufferAccessStrategy bstrategy, bool for_wraparound, bool isTopLevel);
+extern void ExecVacuum(VacuumStmt *vacstmt, bool isTopLevel);
+extern void vacuum(VacuumParams *params, RangeVar *relation, int options,
+				   List *va_cols, Oid relid, bool do_toast, BufferAccessStrategy bstrategy,
+				   bool isTopLevel);
 extern void vac_open_indexes(Relation relation, LOCKMODE lockmode,
 				 int *nindexes, Relation **Irel);
 extern void vac_close_indexes(int nindexes, Relation *Irel, LOCKMODE lockmode);
@@ -171,12 +187,13 @@ extern void vac_update_datfrozenxid(void);
 extern void vacuum_delay_point(void);
 
 /* in commands/vacuumlazy.c */
-extern void lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
-				BufferAccessStrategy bstrategy);
+extern void lazy_vacuum_rel(Relation onerel, int options,
+				VacuumParams *params, BufferAccessStrategy bstrategy);
 
 /* in commands/analyze.c */
-extern void analyze_rel(Oid relid, VacuumStmt *vacstmt,
-			bool in_outer_xact, BufferAccessStrategy bstrategy);
+extern void analyze_rel(Oid relid, RangeVar *relation, int options,
+			List *va_cols, bool in_outer_xact,
+			BufferAccessStrategy bstrategy);
 extern bool std_typanalyze(VacAttrStats *stats);
 extern double anl_random_fract(void);
 extern double anl_init_selection_state(int n);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index ac13302..4c8b14c 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2598,9 +2598,7 @@ typedef struct ClusterStmt
  *
  * Even though these are nominally two statements, it's convenient to use
  * just one node type for both.  Note that at least one of VACOPT_VACUUM
- * and VACOPT_ANALYZE must be set in options.  VACOPT_FREEZE is an internal
- * convenience for the grammar and is not examined at runtime --- the
- * freeze_min_age and freeze_table_age fields are what matter.
+ * and VACOPT_ANALYZE must be set in options.
  * ----------------------
  */
 typedef enum VacuumOption
@@ -2615,14 +2613,8 @@ typedef enum VacuumOption
 
 typedef struct VacuumStmt
 {
-	NodeTag		type;
-	int			options;		/* OR of VacuumOption flags */
-	int			freeze_min_age; /* min freeze age, or -1 to use default */
-	int			freeze_table_age;		/* age at which to scan whole table */
-	int			multixact_freeze_min_age;		/* min multixact freeze age,
-												 * or -1 to use default */
-	int			multixact_freeze_table_age;		/* multixact age at which to
-												 * scan whole table */
+	NodeTag			type;
+	int				options;	/* OR of VacuumOption flags */
 	RangeVar   *relation;		/* single table to process, or NULL */
 	List	   *va_cols;		/* list of column names, or NIL for all */
 } VacuumStmt;
-- 
2.3.1

#27Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Michael Paquier (#26)
1 attachment(s)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

Attachments:

0001-Move-freeze-parameters-of-VacuumStmt-into-a-separate-2.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c
index d2856a3..75b45f7 100644
--- a/src/backend/commands/analyze.c
+++ b/src/backend/commands/analyze.c
@@ -85,7 +85,7 @@ static MemoryContext anl_context = NULL;
 static BufferAccessStrategy vac_strategy;
 
 
-static void do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
+static void do_analyze_rel(Relation onerel, int options, List *va_cols,
 			   AcquireSampleRowsFunc acquirefunc, BlockNumber relpages,
 			   bool inh, bool in_outer_xact, int elevel);
 static void BlockSampler_Init(BlockSampler bs, BlockNumber nblocks,
@@ -115,7 +115,7 @@ static Datum ind_fetch_func(VacAttrStatsP stats, int rownum, bool *isNull);
  *	analyze_rel() -- analyze one relation
  */
 void
-analyze_rel(Oid relid, VacuumStmt *vacstmt,
+analyze_rel(Oid relid, RangeVar *relation, int options, List *va_cols,
 			bool in_outer_xact, BufferAccessStrategy bstrategy)
 {
 	Relation	onerel;
@@ -124,7 +124,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
 	BlockNumber relpages = 0;
 
 	/* Select logging level */
-	if (vacstmt->options & VACOPT_VERBOSE)
+	if (options & VACOPT_VERBOSE)
 		elevel = INFO;
 	else
 		elevel = DEBUG2;
@@ -144,7 +144,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
 	 * matter if we ever try to accumulate stats on dead tuples.) If the rel
 	 * has been dropped since we last saw it, we don't need to process it.
 	 */
-	if (!(vacstmt->options & VACOPT_NOWAIT))
+	if (!(options & VACOPT_NOWAIT))
 		onerel = try_relation_open(relid, ShareUpdateExclusiveLock);
 	else if (ConditionalLockRelationOid(relid, ShareUpdateExclusiveLock))
 		onerel = try_relation_open(relid, NoLock);
@@ -155,7 +155,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
 			ereport(LOG,
 					(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
 				  errmsg("skipping analyze of \"%s\" --- lock not available",
-						 vacstmt->relation->relname)));
+						 relation->relname)));
 	}
 	if (!onerel)
 		return;
@@ -167,7 +167,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
 		  (pg_database_ownercheck(MyDatabaseId, GetUserId()) && !onerel->rd_rel->relisshared)))
 	{
 		/* No need for a WARNING if we already complained during VACUUM */
-		if (!(vacstmt->options & VACOPT_VACUUM))
+		if (!(options & VACOPT_VACUUM))
 		{
 			if (onerel->rd_rel->relisshared)
 				ereport(WARNING,
@@ -248,7 +248,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
 	else
 	{
 		/* No need for a WARNING if we already complained during VACUUM */
-		if (!(vacstmt->options & VACOPT_VACUUM))
+		if (!(options & VACOPT_VACUUM))
 			ereport(WARNING,
 					(errmsg("skipping \"%s\" --- cannot analyze non-tables or special system tables",
 							RelationGetRelationName(onerel))));
@@ -266,14 +266,14 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
 	/*
 	 * Do the normal non-recursive ANALYZE.
 	 */
-	do_analyze_rel(onerel, vacstmt, acquirefunc, relpages,
+	do_analyze_rel(onerel, options, va_cols, acquirefunc, relpages,
 				   false, in_outer_xact, elevel);
 
 	/*
 	 * If there are child tables, do recursive ANALYZE.
 	 */
 	if (onerel->rd_rel->relhassubclass)
-		do_analyze_rel(onerel, vacstmt, acquirefunc, relpages,
+		do_analyze_rel(onerel, options, va_cols, acquirefunc, relpages,
 					   true, in_outer_xact, elevel);
 
 	/*
@@ -302,7 +302,7 @@ analyze_rel(Oid relid, VacuumStmt *vacstmt,
  * acquirefunc for each child table.
  */
 static void
-do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
+do_analyze_rel(Relation onerel, int options, List *va_cols,
 			   AcquireSampleRowsFunc acquirefunc, BlockNumber relpages,
 			   bool inh, bool in_outer_xact, int elevel)
 {
@@ -372,14 +372,14 @@ do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
 	 *
 	 * Note that system attributes are never analyzed.
 	 */
-	if (vacstmt->va_cols != NIL)
+	if (va_cols != NIL)
 	{
 		ListCell   *le;
 
-		vacattrstats = (VacAttrStats **) palloc(list_length(vacstmt->va_cols) *
+		vacattrstats = (VacAttrStats **) palloc(list_length(va_cols) *
 												sizeof(VacAttrStats *));
 		tcnt = 0;
-		foreach(le, vacstmt->va_cols)
+		foreach(le, va_cols)
 		{
 			char	   *col = strVal(lfirst(le));
 
@@ -436,7 +436,7 @@ do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
 
 			thisdata->indexInfo = indexInfo = BuildIndexInfo(Irel[ind]);
 			thisdata->tupleFract = 1.0; /* fix later if partial */
-			if (indexInfo->ii_Expressions != NIL && vacstmt->va_cols == NIL)
+			if (indexInfo->ii_Expressions != NIL && va_cols == NIL)
 			{
 				ListCell   *indexpr_item = list_head(indexInfo->ii_Expressions);
 
@@ -595,7 +595,7 @@ do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
 	 * VACUUM ANALYZE, don't overwrite the accurate count already inserted by
 	 * VACUUM.
 	 */
-	if (!inh && !(vacstmt->options & VACOPT_VACUUM))
+	if (!inh && !(options & VACOPT_VACUUM))
 	{
 		for (ind = 0; ind < nindexes; ind++)
 		{
@@ -623,7 +623,7 @@ do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
 		pgstat_report_analyze(onerel, totalrows, totaldeadrows);
 
 	/* If this isn't part of VACUUM ANALYZE, let index AMs do cleanup */
-	if (!(vacstmt->options & VACOPT_VACUUM))
+	if (!(options & VACOPT_VACUUM))
 	{
 		for (ind = 0; ind < nindexes; ind++)
 		{
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 7447568..78ab58d 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -71,35 +71,79 @@ static void vac_truncate_clog(TransactionId frozenXID,
 				  MultiXactId minMulti,
 				  TransactionId lastSaneFrozenXid,
 				  MultiXactId lastSaneMinMulti);
-static bool vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast,
-		   bool for_wraparound);
+static bool vacuum_rel(Oid relid, RangeVar *relation, int options,
+		   VacuumParams *params);
 
+/*
+ * Primary entry point for manual VACUUM and ANALYZE commands
+ *
+ * This is mainly a preparation wrapper for the real operations that will
+ * happen in vacuum().
+ */
+void
+ExecVacuum(VacuumStmt *vacstmt, bool isTopLevel)
+{
+	VacuumParams	params;
+
+	/* sanity checks on options */
+	Assert(vacstmt->options & (VACOPT_VACUUM | VACOPT_ANALYZE));
+	Assert((vacstmt->options & VACOPT_VACUUM) ||
+		   !(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE)));
+	Assert((vacstmt->options & VACOPT_ANALYZE) || vacstmt->va_cols == NIL);
+	Assert(!(vacstmt->options & VACOPT_SKIPTOAST));
+
+	/*
+	 * All freeze ages are zero if the FREEZE options are given; otherwise
+	 * pass them as -1 which means to use the default values.
+	 */
+	if (vacstmt->options & VACOPT_FREEZE)
+	{
+		params.freeze_min_age = 0;
+		params.freeze_table_age = 0;
+		params.multixact_freeze_min_age = 0;
+		params.multixact_freeze_table_age = 0;
+	}
+	else
+	{
+		params.freeze_min_age = -1;
+		params.freeze_table_age = -1;
+		params.multixact_freeze_min_age = -1;
+		params.multixact_freeze_table_age = -1;
+	}
+
+	/* user-invoked vacuum is never "for wraparound" */
+	params.is_wraparound = false;
+
+	/* Now go through the common routine */
+	vacuum(vacstmt->options, vacstmt->relation, InvalidOid, &params,
+		   vacstmt->va_cols, NULL, isTopLevel);
+}
 
 /*
  * Primary entry point for VACUUM and ANALYZE commands.
  *
- * relid is normally InvalidOid; if it is not, then it provides the relation
- * OID to be processed, and vacstmt->relation is ignored.  (The non-invalid
- * case is currently only used by autovacuum.)
+ * options is a bitmask of VacuumOption flags, indicating what to do.
  *
- * do_toast is passed as FALSE by autovacuum, because it processes TOAST
- * tables separately.
+ * relid, if not InvalidOid, indicate the relation to process; otherwise,
+ * the RangeVar is used.  (The latter must always be passed, because it's
+ * used for error messages.)
  *
- * for_wraparound is used by autovacuum to let us know when it's forcing
- * a vacuum for wraparound, which should not be auto-canceled.
+ * params contains a set of parameters that can be used to customize the
+ * behavior.
+ *
+ * va_cols is a list of columns to analyze, or NIL to process them all.
  *
  * bstrategy is normally given as NULL, but in autovacuum it can be passed
  * in to use the same buffer strategy object across multiple vacuum() calls.
  *
  * isTopLevel should be passed down from ProcessUtility.
  *
- * It is the caller's responsibility that vacstmt and bstrategy
- * (if given) be allocated in a memory context that won't disappear
- * at transaction commit.
+ * It is the caller's responsibility that all parameters are allocated in a
+ * memory context that will not disappear at transaction commit.
  */
 void
-vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
-	   BufferAccessStrategy bstrategy, bool for_wraparound, bool isTopLevel)
+vacuum(int options, RangeVar *relation, Oid relid, VacuumParams *params,
+	   List *va_cols, BufferAccessStrategy bstrategy, bool isTopLevel)
 {
 	const char *stmttype;
 	volatile bool in_outer_xact,
@@ -107,13 +151,9 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 	List	   *relations;
 	static bool in_vacuum = false;
 
-	/* sanity checks on options */
-	Assert(vacstmt->options & (VACOPT_VACUUM | VACOPT_ANALYZE));
-	Assert((vacstmt->options & VACOPT_VACUUM) ||
-		   !(vacstmt->options & (VACOPT_FULL | VACOPT_FREEZE)));
-	Assert((vacstmt->options & VACOPT_ANALYZE) || vacstmt->va_cols == NIL);
+	Assert(params != NULL);
 
-	stmttype = (vacstmt->options & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE";
+	stmttype = (options & VACOPT_VACUUM) ? "VACUUM" : "ANALYZE";
 
 	/*
 	 * We cannot run VACUUM inside a user transaction block; if we were inside
@@ -123,7 +163,7 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 	 *
 	 * ANALYZE (without VACUUM) can run either way.
 	 */
-	if (vacstmt->options & VACOPT_VACUUM)
+	if (options & VACOPT_VACUUM)
 	{
 		PreventTransactionChain(isTopLevel, stmttype);
 		in_outer_xact = false;
@@ -143,7 +183,7 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 	 * Send info about dead objects to the statistics collector, unless we are
 	 * in autovacuum --- autovacuum.c does this for itself.
 	 */
-	if ((vacstmt->options & VACOPT_VACUUM) && !IsAutoVacuumWorkerProcess())
+	if ((options & VACOPT_VACUUM) && !IsAutoVacuumWorkerProcess())
 		pgstat_vacuum_stat();
 
 	/*
@@ -175,7 +215,7 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 	 * Build list of relations to process, unless caller gave us one. (If we
 	 * build one, we put it in vac_context for safekeeping.)
 	 */
-	relations = get_rel_oids(relid, vacstmt->relation);
+	relations = get_rel_oids(relid, relation);
 
 	/*
 	 * Decide whether we need to start/commit our own transactions.
@@ -191,11 +231,11 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 	 * transaction block, and also in an autovacuum worker, use own
 	 * transactions so we can release locks sooner.
 	 */
-	if (vacstmt->options & VACOPT_VACUUM)
+	if (options & VACOPT_VACUUM)
 		use_own_xacts = true;
 	else
 	{
-		Assert(vacstmt->options & VACOPT_ANALYZE);
+		Assert(options & VACOPT_ANALYZE);
 		if (IsAutoVacuumWorkerProcess())
 			use_own_xacts = true;
 		else if (in_outer_xact)
@@ -245,13 +285,13 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 		{
 			Oid			relid = lfirst_oid(cur);
 
-			if (vacstmt->options & VACOPT_VACUUM)
+			if (options & VACOPT_VACUUM)
 			{
-				if (!vacuum_rel(relid, vacstmt, do_toast, for_wraparound))
+				if (!vacuum_rel(relid, relation, options, params))
 					continue;
 			}
 
-			if (vacstmt->options & VACOPT_ANALYZE)
+			if (options & VACOPT_ANALYZE)
 			{
 				/*
 				 * If using separate xacts, start one for analyze. Otherwise,
@@ -264,7 +304,8 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 					PushActiveSnapshot(GetTransactionSnapshot());
 				}
 
-				analyze_rel(relid, vacstmt, in_outer_xact, vac_strategy);
+				analyze_rel(relid, relation, options,
+							va_cols, in_outer_xact, vac_strategy);
 
 				if (use_own_xacts)
 				{
@@ -299,7 +340,7 @@ vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
 		StartTransactionCommand();
 	}
 
-	if ((vacstmt->options & VACOPT_VACUUM) && !IsAutoVacuumWorkerProcess())
+	if ((options & VACOPT_VACUUM) && !IsAutoVacuumWorkerProcess())
 	{
 		/*
 		 * Update pg_database.datfrozenxid, and truncate pg_clog if possible.
@@ -1113,7 +1154,7 @@ vac_truncate_clog(TransactionId frozenXID,
  *		At entry and exit, we are not inside a transaction.
  */
 static bool
-vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
+vacuum_rel(Oid relid, RangeVar *relation, int options, VacuumParams *params)
 {
 	LOCKMODE	lmode;
 	Relation	onerel;
@@ -1123,6 +1164,8 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 	int			save_sec_context;
 	int			save_nestlevel;
 
+	Assert(params != NULL);
+
 	/* Begin a transaction for vacuuming this relation */
 	StartTransactionCommand();
 
@@ -1132,7 +1175,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 	 */
 	PushActiveSnapshot(GetTransactionSnapshot());
 
-	if (!(vacstmt->options & VACOPT_FULL))
+	if (!(options & VACOPT_FULL))
 	{
 		/*
 		 * In lazy vacuum, we can set the PROC_IN_VACUUM flag, which lets
@@ -1156,7 +1199,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 		 */
 		LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
 		MyPgXact->vacuumFlags |= PROC_IN_VACUUM;
-		if (for_wraparound)
+		if (params->is_wraparound)
 			MyPgXact->vacuumFlags |= PROC_VACUUM_FOR_WRAPAROUND;
 		LWLockRelease(ProcArrayLock);
 	}
@@ -1172,7 +1215,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 	 * vacuum, but just ShareUpdateExclusiveLock for concurrent vacuum. Either
 	 * way, we can be sure that no other backend is vacuuming the same table.
 	 */
-	lmode = (vacstmt->options & VACOPT_FULL) ? AccessExclusiveLock : ShareUpdateExclusiveLock;
+	lmode = (options & VACOPT_FULL) ? AccessExclusiveLock : ShareUpdateExclusiveLock;
 
 	/*
 	 * Open the relation and get the appropriate lock on it.
@@ -1183,7 +1226,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 	 * If we've been asked not to wait for the relation lock, acquire it first
 	 * in non-blocking mode, before calling try_relation_open().
 	 */
-	if (!(vacstmt->options & VACOPT_NOWAIT))
+	if (!(options & VACOPT_NOWAIT))
 		onerel = try_relation_open(relid, lmode);
 	else if (ConditionalLockRelationOid(relid, lmode))
 		onerel = try_relation_open(relid, NoLock);
@@ -1194,7 +1237,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 			ereport(LOG,
 					(errcode(ERRCODE_LOCK_NOT_AVAILABLE),
 				   errmsg("skipping vacuum of \"%s\" --- lock not available",
-						  vacstmt->relation->relname)));
+						  relation->relname)));
 	}
 
 	if (!onerel)
@@ -1286,7 +1329,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 	 * us to process it.  In VACUUM FULL, though, the toast table is
 	 * automatically rebuilt by cluster_rel so we shouldn't recurse to it.
 	 */
-	if (do_toast && !(vacstmt->options & VACOPT_FULL))
+	if (!(options & VACOPT_SKIPTOAST) && !(options & VACOPT_FULL))
 		toast_relid = onerel->rd_rel->reltoastrelid;
 	else
 		toast_relid = InvalidOid;
@@ -1305,7 +1348,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 	/*
 	 * Do the actual work --- either FULL or "lazy" vacuum
 	 */
-	if (vacstmt->options & VACOPT_FULL)
+	if (options & VACOPT_FULL)
 	{
 		/* close relation before vacuuming, but hold lock until commit */
 		relation_close(onerel, NoLock);
@@ -1313,10 +1356,10 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 
 		/* VACUUM FULL is now a variant of CLUSTER; see cluster.c */
 		cluster_rel(relid, InvalidOid, false,
-					(vacstmt->options & VACOPT_VERBOSE) != 0);
+					(options & VACOPT_VERBOSE) != 0);
 	}
 	else
-		lazy_vacuum_rel(onerel, vacstmt, vac_strategy);
+		lazy_vacuum_rel(onerel, options, params, vac_strategy);
 
 	/* Roll back any GUC changes executed by index functions */
 	AtEOXact_GUC(false, save_nestlevel);
@@ -1342,7 +1385,7 @@ vacuum_rel(Oid relid, VacuumStmt *vacstmt, bool do_toast, bool for_wraparound)
 	 * totally unimportant for toast relations.
 	 */
 	if (toast_relid != InvalidOid)
-		vacuum_rel(toast_relid, vacstmt, false, for_wraparound);
+		vacuum_rel(toast_relid, relation, options, params);
 
 	/*
 	 * Now release the session-level lock on the master table.
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 7d9e49e..cd5ca4c 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -169,7 +169,7 @@ static bool heap_page_is_all_visible(Relation rel, Buffer buf,
  *		and locked the relation.
  */
 void
-lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
+lazy_vacuum_rel(Relation onerel, int options, VacuumParams *params,
 				BufferAccessStrategy bstrategy)
 {
 	LVRelStats *vacrelstats;
@@ -193,6 +193,8 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
 	TransactionId new_frozen_xid;
 	MultiXactId new_min_multi;
 
+	Assert(params != NULL);
+
 	/* measure elapsed time iff autovacuum logging requires it */
 	if (IsAutoVacuumWorkerProcess() && Log_autovacuum_min_duration >= 0)
 	{
@@ -200,7 +202,7 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
 		starttime = GetCurrentTimestamp();
 	}
 
-	if (vacstmt->options & VACOPT_VERBOSE)
+	if (options & VACOPT_VERBOSE)
 		elevel = INFO;
 	else
 		elevel = DEBUG2;
@@ -208,9 +210,10 @@ lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
 	vac_strategy = bstrategy;
 
 	vacuum_set_xid_limits(onerel,
-						  vacstmt->freeze_min_age, vacstmt->freeze_table_age,
-						  vacstmt->multixact_freeze_min_age,
-						  vacstmt->multixact_freeze_table_age,
+						  params->freeze_min_age,
+						  params->freeze_table_age,
+						  params->multixact_freeze_min_age,
+						  params->multixact_freeze_table_age,
 						  &OldestXmin, &FreezeLimit, &xidFullScanLimit,
 						  &MultiXactCutoff, &mxactFullScanLimit);
 
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 3c6a964..029761e 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -3300,10 +3300,6 @@ _copyVacuumStmt(const VacuumStmt *from)
 	VacuumStmt *newnode = makeNode(VacuumStmt);
 
 	COPY_SCALAR_FIELD(options);
-	COPY_SCALAR_FIELD(freeze_min_age);
-	COPY_SCALAR_FIELD(freeze_table_age);
-	COPY_SCALAR_FIELD(multixact_freeze_min_age);
-	COPY_SCALAR_FIELD(multixact_freeze_table_age);
 	COPY_NODE_FIELD(relation);
 	COPY_NODE_FIELD(va_cols);
 
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index fcd58ad..190e50a 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1503,10 +1503,6 @@ static bool
 _equalVacuumStmt(const VacuumStmt *a, const VacuumStmt *b)
 {
 	COMPARE_SCALAR_FIELD(options);
-	COMPARE_SCALAR_FIELD(freeze_min_age);
-	COMPARE_SCALAR_FIELD(freeze_table_age);
-	COMPARE_SCALAR_FIELD(multixact_freeze_min_age);
-	COMPARE_SCALAR_FIELD(multixact_freeze_table_age);
 	COMPARE_NODE_FIELD(relation);
 	COMPARE_NODE_FIELD(va_cols);
 
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 1499620..82405b9 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -9034,12 +9034,10 @@ VacuumStmt: VACUUM opt_full opt_freeze opt_verbose
 					n->options = VACOPT_VACUUM;
 					if ($2)
 						n->options |= VACOPT_FULL;
+					if ($3)
+						n->options |= VACOPT_FREEZE;
 					if ($4)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = $3 ? 0 : -1;
-					n->freeze_table_age = $3 ? 0 : -1;
-					n->multixact_freeze_min_age = $3 ? 0 : -1;
-					n->multixact_freeze_table_age = $3 ? 0 : -1;
 					n->relation = NULL;
 					n->va_cols = NIL;
 					$$ = (Node *)n;
@@ -9050,12 +9048,10 @@ VacuumStmt: VACUUM opt_full opt_freeze opt_verbose
 					n->options = VACOPT_VACUUM;
 					if ($2)
 						n->options |= VACOPT_FULL;
+					if ($3)
+						n->options |= VACOPT_FREEZE;
 					if ($4)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = $3 ? 0 : -1;
-					n->freeze_table_age = $3 ? 0 : -1;
-					n->multixact_freeze_min_age = $3 ? 0 : -1;
-					n->multixact_freeze_table_age = $3 ? 0 : -1;
 					n->relation = $5;
 					n->va_cols = NIL;
 					$$ = (Node *)n;
@@ -9066,30 +9062,16 @@ VacuumStmt: VACUUM opt_full opt_freeze opt_verbose
 					n->options |= VACOPT_VACUUM;
 					if ($2)
 						n->options |= VACOPT_FULL;
+					if ($3)
+						n->options |= VACOPT_FREEZE;
 					if ($4)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = $3 ? 0 : -1;
-					n->freeze_table_age = $3 ? 0 : -1;
-					n->multixact_freeze_min_age = $3 ? 0 : -1;
-					n->multixact_freeze_table_age = $3 ? 0 : -1;
 					$$ = (Node *)n;
 				}
 			| VACUUM '(' vacuum_option_list ')'
 				{
 					VacuumStmt *n = makeNode(VacuumStmt);
 					n->options = VACOPT_VACUUM | $3;
-					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;
-					}
 					n->relation = NULL;
 					n->va_cols = NIL;
 					$$ = (Node *) n;
@@ -9098,18 +9080,6 @@ VacuumStmt: VACUUM opt_full opt_freeze opt_verbose
 				{
 					VacuumStmt *n = makeNode(VacuumStmt);
 					n->options = VACOPT_VACUUM | $3;
-					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;
-					}
 					n->relation = $5;
 					n->va_cols = $6;
 					if (n->va_cols != NIL)	/* implies analyze */
@@ -9137,10 +9107,6 @@ AnalyzeStmt:
 					n->options = VACOPT_ANALYZE;
 					if ($2)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = -1;
-					n->freeze_table_age = -1;
-					n->multixact_freeze_min_age = -1;
-					n->multixact_freeze_table_age = -1;
 					n->relation = NULL;
 					n->va_cols = NIL;
 					$$ = (Node *)n;
@@ -9151,10 +9117,6 @@ AnalyzeStmt:
 					n->options = VACOPT_ANALYZE;
 					if ($2)
 						n->options |= VACOPT_VERBOSE;
-					n->freeze_min_age = -1;
-					n->freeze_table_age = -1;
-					n->multixact_freeze_min_age = -1;
-					n->multixact_freeze_table_age = -1;
 					n->relation = $3;
 					n->va_cols = $4;
 					$$ = (Node *)n;
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index ee556f3..5ccae24 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -184,16 +184,11 @@ typedef struct av_relation
 typedef struct autovac_table
 {
 	Oid			at_relid;
-	bool		at_dovacuum;
-	bool		at_doanalyze;
-	int			at_freeze_min_age;
-	int			at_freeze_table_age;
-	int			at_multixact_freeze_min_age;
-	int			at_multixact_freeze_table_age;
+	int			at_vacoptions;	/* bitmask of VacuumOption */
+	VacuumParams at_params;
 	int			at_vacuum_cost_delay;
 	int			at_vacuum_cost_limit;
 	bool		at_dobalance;
-	bool		at_wraparound;
 	char	   *at_relname;
 	char	   *at_nspname;
 	char	   *at_datname;
@@ -2301,7 +2296,7 @@ do_autovacuum(void)
 			 * next table in our list.
 			 */
 			HOLD_INTERRUPTS();
-			if (tab->at_dovacuum)
+			if (tab->at_vacoptions & VACOPT_VACUUM)
 				errcontext("automatic vacuum of table \"%s.%s.%s\"",
 						   tab->at_datname, tab->at_nspname, tab->at_relname);
 			else
@@ -2528,15 +2523,17 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
 
 		tab = palloc(sizeof(autovac_table));
 		tab->at_relid = relid;
-		tab->at_dovacuum = dovacuum;
-		tab->at_doanalyze = doanalyze;
-		tab->at_freeze_min_age = freeze_min_age;
-		tab->at_freeze_table_age = freeze_table_age;
-		tab->at_multixact_freeze_min_age = multixact_freeze_min_age;
-		tab->at_multixact_freeze_table_age = multixact_freeze_table_age;
+		tab->at_vacoptions = VACOPT_SKIPTOAST |
+			(dovacuum ? VACOPT_VACUUM : 0) |
+			(doanalyze ? VACOPT_ANALYZE : 0) |
+			(wraparound ? VACOPT_NOWAIT : 0);
+		tab->at_params.freeze_min_age = freeze_min_age;
+		tab->at_params.freeze_table_age = freeze_table_age;
+		tab->at_params.multixact_freeze_min_age = multixact_freeze_min_age;
+		tab->at_params.multixact_freeze_table_age = multixact_freeze_table_age;
+		tab->at_params.is_wraparound = wraparound;
 		tab->at_vacuum_cost_limit = vac_cost_limit;
 		tab->at_vacuum_cost_delay = vac_cost_delay;
-		tab->at_wraparound = wraparound;
 		tab->at_relname = NULL;
 		tab->at_nspname = NULL;
 		tab->at_datname = NULL;
@@ -2737,39 +2734,22 @@ relation_needs_vacanalyze(Oid relid,
  *		Vacuum and/or analyze the specified table
  */
 static void
-autovacuum_do_vac_analyze(autovac_table *tab,
-						  BufferAccessStrategy bstrategy)
+autovacuum_do_vac_analyze(autovac_table *tab, BufferAccessStrategy bstrategy)
 {
-	VacuumStmt	vacstmt;
-	RangeVar	rangevar;
+	RangeVar		rangevar;
 
 	/* Set up command parameters --- use local variables instead of palloc */
-	MemSet(&vacstmt, 0, sizeof(vacstmt));
 	MemSet(&rangevar, 0, sizeof(rangevar));
 
 	rangevar.schemaname = tab->at_nspname;
 	rangevar.relname = tab->at_relname;
 	rangevar.location = -1;
 
-	vacstmt.type = T_VacuumStmt;
-	if (!tab->at_wraparound)
-		vacstmt.options = VACOPT_NOWAIT;
-	if (tab->at_dovacuum)
-		vacstmt.options |= VACOPT_VACUUM;
-	if (tab->at_doanalyze)
-		vacstmt.options |= VACOPT_ANALYZE;
-	vacstmt.freeze_min_age = tab->at_freeze_min_age;
-	vacstmt.freeze_table_age = tab->at_freeze_table_age;
-	vacstmt.multixact_freeze_min_age = tab->at_multixact_freeze_min_age;
-	vacstmt.multixact_freeze_table_age = tab->at_multixact_freeze_table_age;
-	/* we pass the OID, but might need this anyway for an error message */
-	vacstmt.relation = &rangevar;
-	vacstmt.va_cols = NIL;
-
 	/* Let pgstat know what we're doing */
 	autovac_report_activity(tab);
 
-	vacuum(&vacstmt, tab->at_relid, false, bstrategy, tab->at_wraparound, true);
+	vacuum(tab->at_vacoptions, &rangevar, tab->at_relid, &tab->at_params, NIL,
+		   bstrategy, true);
 }
 
 /*
@@ -2791,10 +2771,10 @@ autovac_report_activity(autovac_table *tab)
 	int			len;
 
 	/* Report the command and possible options */
-	if (tab->at_dovacuum)
+	if (tab->at_vacoptions & VACOPT_VACUUM)
 		snprintf(activity, MAX_AUTOVAC_ACTIV_LEN,
 				 "autovacuum: VACUUM%s",
-				 tab->at_doanalyze ? " ANALYZE" : "");
+				 tab->at_vacoptions & VACOPT_ANALYZE ? " ANALYZE" : "");
 	else
 		snprintf(activity, MAX_AUTOVAC_ACTIV_LEN,
 				 "autovacuum: ANALYZE");
@@ -2806,7 +2786,7 @@ autovac_report_activity(autovac_table *tab)
 
 	snprintf(activity + len, MAX_AUTOVAC_ACTIV_LEN - len,
 			 " %s.%s%s", tab->at_nspname, tab->at_relname,
-			 tab->at_wraparound ? " (to prevent wraparound)" : "");
+			 tab->at_params.is_wraparound ? " (to prevent wraparound)" : "");
 
 	/* Set statement_timestamp() to current time for pg_stat_activity */
 	SetCurrentStatementStartTimestamp();
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index 065475d..d9443b1 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -627,7 +627,7 @@ standard_ProcessUtility(Node *parsetree,
 				/* we choose to allow this during "read only" transactions */
 				PreventCommandDuringRecovery((stmt->options & VACOPT_VACUUM) ?
 											 "VACUUM" : "ANALYZE");
-				vacuum(stmt, InvalidOid, true, NULL, false, isTopLevel);
+				ExecVacuum(stmt, isTopLevel);
 			}
 			break;
 
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 4275484..9fd2516 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -130,6 +130,19 @@ typedef struct VacAttrStats
 	int			rowstride;
 } VacAttrStats;
 
+/*
+ * Parameters customizing behavior of VACUUM and ANALYZE.
+ */
+typedef struct VacuumParams
+{
+	int		freeze_min_age;		/* min freeze age, -1 to use default */
+	int		freeze_table_age;	/* age at which to scan whole table */
+	int		multixact_freeze_min_age;	/* min multixact freeze age,
+										 * -1 to use default */
+	int		multixact_freeze_table_age;	/* multixact age at which to
+										 * scan whole table */
+	bool	is_wraparound;		/* force a for-wraparound vacuum */
+} VacuumParams;
 
 /* GUC parameters */
 extern PGDLLIMPORT int default_statistics_target;		/* PGDLLIMPORT for
@@ -141,8 +154,10 @@ extern int	vacuum_multixact_freeze_table_age;
 
 
 /* in commands/vacuum.c */
-extern void vacuum(VacuumStmt *vacstmt, Oid relid, bool do_toast,
-	   BufferAccessStrategy bstrategy, bool for_wraparound, bool isTopLevel);
+extern void ExecVacuum(VacuumStmt *vacstmt, bool isTopLevel);
+extern void vacuum(int options, RangeVar *relation, Oid relid,
+	   VacuumParams *params, List *va_cols,
+	   BufferAccessStrategy bstrategy, bool isTopLevel);
 extern void vac_open_indexes(Relation relation, LOCKMODE lockmode,
 				 int *nindexes, Relation **Irel);
 extern void vac_close_indexes(int nindexes, Relation *Irel, LOCKMODE lockmode);
@@ -171,12 +186,13 @@ extern void vac_update_datfrozenxid(void);
 extern void vacuum_delay_point(void);
 
 /* in commands/vacuumlazy.c */
-extern void lazy_vacuum_rel(Relation onerel, VacuumStmt *vacstmt,
-				BufferAccessStrategy bstrategy);
+extern void lazy_vacuum_rel(Relation onerel, int options,
+				VacuumParams *params, BufferAccessStrategy bstrategy);
 
 /* in commands/analyze.c */
-extern void analyze_rel(Oid relid, VacuumStmt *vacstmt,
-			bool in_outer_xact, BufferAccessStrategy bstrategy);
+extern void analyze_rel(Oid relid, RangeVar *relation, int options,
+			List *va_cols, bool in_outer_xact,
+			BufferAccessStrategy bstrategy);
 extern bool std_typanalyze(VacAttrStats *stats);
 extern double anl_random_fract(void);
 extern double anl_init_selection_state(int n);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index a575353..ec0d0ea 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2608,9 +2608,7 @@ typedef struct ClusterStmt
  *
  * Even though these are nominally two statements, it's convenient to use
  * just one node type for both.  Note that at least one of VACOPT_VACUUM
- * and VACOPT_ANALYZE must be set in options.  VACOPT_FREEZE is an internal
- * convenience for the grammar and is not examined at runtime --- the
- * freeze_min_age and freeze_table_age fields are what matter.
+ * and VACOPT_ANALYZE must be set in options.
  * ----------------------
  */
 typedef enum VacuumOption
@@ -2620,19 +2618,14 @@ typedef enum VacuumOption
 	VACOPT_VERBOSE = 1 << 2,	/* print progress info */
 	VACOPT_FREEZE = 1 << 3,		/* FREEZE option */
 	VACOPT_FULL = 1 << 4,		/* FULL (non-concurrent) vacuum */
-	VACOPT_NOWAIT = 1 << 5		/* don't wait to get lock (autovacuum only) */
+	VACOPT_NOWAIT = 1 << 5,		/* don't wait to get lock (autovacuum only) */
+	VACOPT_SKIPTOAST = 1 << 6	/* don't process the TOAST table, if any */
 } VacuumOption;
 
 typedef struct VacuumStmt
 {
 	NodeTag		type;
 	int			options;		/* OR of VacuumOption flags */
-	int			freeze_min_age; /* min freeze age, or -1 to use default */
-	int			freeze_table_age;		/* age at which to scan whole table */
-	int			multixact_freeze_min_age;		/* min multixact freeze age,
-												 * or -1 to use default */
-	int			multixact_freeze_table_age;		/* multixact age at which to
-												 * scan whole table */
 	RangeVar   *relation;		/* single table to process, or NULL */
 	List	   *va_cols;		/* list of column names, or NIL for all */
 } VacuumStmt;
#28Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Alvaro Herrera (#27)
Re: 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 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

#29Stephen Frost
sfrost@snowman.net
In reply to: Alvaro Herrera (#28)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

* 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

#30Magnus Hagander
magnus@hagander.net
In reply to: Stephen Frost (#29)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

#31Michael Paquier
michael.paquier@gmail.com
In reply to: Alvaro Herrera (#27)
Re: 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

#32Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Michael Paquier (#31)
Re: 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 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

#33Michael Paquier
michael.paquier@gmail.com
In reply to: Alvaro Herrera (#32)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

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

#34Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Michael Paquier (#31)
Re: 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 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

#35Michael Paquier
michael.paquier@gmail.com
In reply to: Alvaro Herrera (#34)
Re: Strange assertion using VACOPT_FREEZE in vacuum.c

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

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