CLUSTER FREEZE
Hi
I noticed that CLUSTER doesn't have a FREEZE option. Here is a patch to
add that, for consistency with VACUUM. Is it useful?
Thanks
Thomas Munro
Attachments:
cluster-freeze.patchapplication/octet-stream; name=cluster-freeze.patchDownload
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 339990f..d6c1d84 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -21,8 +21,8 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
-CLUSTER [VERBOSE] <replaceable class="PARAMETER">table_name</replaceable> [ USING <replaceable class="PARAMETER">index_name</replaceable> ]
-CLUSTER [VERBOSE]
+CLUSTER [FREEZE] [VERBOSE] <replaceable class="PARAMETER">table_name</replaceable> [ USING <replaceable class="PARAMETER">index_name</replaceable> ]
+CLUSTER [FREEZE] [VERBOSE]
</synopsis>
</refsynopsisdiv>
@@ -100,6 +100,19 @@ CLUSTER [VERBOSE]
</varlistentry>
<varlistentry>
+ <term><literal>FREEZE</literal></term>
+ <listitem>
+ <para>
+ Selects aggressive <quote>freezing</quote> of tuples.
+ Specifying <literal>FREEZE</literal> is equivalent to performing
+ <command>CLUSTER</command> with the
+ <xref linkend="guc-vacuum-freeze-min-age"> parameter
+ set to zero.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><literal>VERBOSE</literal></term>
<listitem>
<para>
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index f6a5bfe..9a548b2 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -177,7 +177,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
heap_close(rel, NoLock);
/* Do the job */
- cluster_rel(tableOid, indexOid, false, stmt->verbose, -1, -1);
+ cluster_rel(tableOid, indexOid, false, stmt->verbose, stmt->freeze_min_age, -1);
}
else
{
@@ -227,7 +227,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
/* functions in indexes may want a snapshot set */
PushActiveSnapshot(GetTransactionSnapshot());
cluster_rel(rvtc->tableOid, rvtc->indexOid, true, stmt->verbose,
- -1, -1);
+ stmt->freeze_min_age, -1);
PopActiveSnapshot();
CommitTransactionCommand();
}
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 65f3b98..5b53308 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2711,6 +2711,7 @@ _copyClusterStmt(const ClusterStmt *from)
ClusterStmt *newnode = makeNode(ClusterStmt);
COPY_NODE_FIELD(relation);
+ COPY_SCALAR_FIELD(freeze_min_age);
COPY_STRING_FIELD(indexname);
COPY_SCALAR_FIELD(verbose);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 4c9b05e..842f125 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1072,6 +1072,7 @@ static bool
_equalClusterStmt(const ClusterStmt *a, const ClusterStmt *b)
{
COMPARE_NODE_FIELD(relation);
+ COMPARE_SCALAR_FIELD(freeze_min_age);
COMPARE_STRING_FIELD(indexname);
COMPARE_SCALAR_FIELD(verbose);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 363c603..f1a96dd 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -8456,36 +8456,39 @@ CreateConversionStmt:
/*****************************************************************************
*
* QUERY:
- * CLUSTER [VERBOSE] <qualified_name> [ USING <index_name> ]
- * CLUSTER [VERBOSE]
- * CLUSTER [VERBOSE] <index_name> ON <qualified_name> (for pre-8.3)
+ * CLUSTER [FREEZE] [VERBOSE] <qualified_name> [ USING <index_name> ]
+ * CLUSTER [FREEZE] [VERBOSE]
+ * CLUSTER [FREEZE] [VERBOSE] <index_name> ON <qualified_name> (for pre-8.3)
*
*****************************************************************************/
ClusterStmt:
- CLUSTER opt_verbose qualified_name cluster_index_specification
+ CLUSTER opt_freeze opt_verbose qualified_name cluster_index_specification
{
ClusterStmt *n = makeNode(ClusterStmt);
- n->relation = $3;
- n->indexname = $4;
- n->verbose = $2;
+ n->relation = $4;
+ n->freeze_min_age = $2 ? 0 : -1;
+ n->indexname = $5;
+ n->verbose = $3;
$$ = (Node*)n;
}
- | CLUSTER opt_verbose
+ | CLUSTER opt_freeze opt_verbose
{
ClusterStmt *n = makeNode(ClusterStmt);
n->relation = NULL;
+ n->freeze_min_age = $2 ? 0 : -1;
n->indexname = NULL;
- n->verbose = $2;
+ n->verbose = $3;
$$ = (Node*)n;
}
/* kept for pre-8.3 compatibility */
- | CLUSTER opt_verbose index_name ON qualified_name
+ | CLUSTER opt_freeze opt_verbose index_name ON qualified_name
{
ClusterStmt *n = makeNode(ClusterStmt);
- n->relation = $5;
- n->indexname = $3;
- n->verbose = $2;
+ n->relation = $6;
+ n->freeze_min_age = $2 ? 0 : -1;
+ n->indexname = $4;
+ n->verbose = $3;
$$ = (Node*)n;
}
;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index e5235cb..71e450c 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2441,6 +2441,7 @@ typedef struct DropdbStmt
typedef struct ClusterStmt
{
NodeTag type;
+ int freeze_min_age; /* min freeze age, or -1 to use default */
RangeVar *relation; /* relation being indexed, or NULL if all */
char *indexname; /* original index defined */
bool verbose; /* print progress info */
On Thu, Oct 24, 2013 at 4:58 AM, Thomas Munro <munro@ip9.org> wrote:
Hi
I noticed that CLUSTER doesn't have a FREEZE option. Here is a patch to add
that, for consistency with VACUUM. Is it useful?
I wonder why anyone would like to freeze during CLUSTER command when
they already have separate way (VACUUM FREEZE) to achieve it, do you
know or can think of any case where user wants to do it along with
Cluster command?
Anyway code side, I think you need to set both feeze_min_age as well
as freeze_table_age, see VACUUM command in gram.y
CLUSTER opt_freeze opt_verbose qualified_name cluster_index_specification
{
ClusterStmt *n = makeNode(ClusterStmt);
- n->relation = $3;
- n->indexname = $4;
- n->verbose = $2;
+ n->relation = $4;
+ n->freeze_min_age = $2 ? 0 : -1;
+ n->indexname = $5;
+ n->verbose = $3;
..With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 24 October 2013 05:58, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Oct 24, 2013 at 4:58 AM, Thomas Munro <munro@ip9.org> wrote:
Hi
I noticed that CLUSTER doesn't have a FREEZE option. Here is a patch to add
that, for consistency with VACUUM. Is it useful?I wonder why anyone would like to freeze during CLUSTER command when
they already have separate way (VACUUM FREEZE) to achieve it, do you
know or can think of any case where user wants to do it along with
Cluster command?
Well I can't speak for Thomas, but if you're doing such a heavy-duty
operation on a table that involves an exclusive lock, you may as well
freeze it. And in the case of CLUSTER, I imagine that in most
instances you'd be confident the table isn't going to undergo much
change (or at least not quickly). CLUSTER FREEZE would mean not
having to run a separate VACUUM FREEZE after, and on a 10GB table,
that's a big deal as it means not having to rescan the whole table
again to freeze every row.
Note that REFRESH MATERIALIZED VIEW freezes rows too as it's already
writing all the data from scratch again, and has an exclusive lock.
(this isn't the case with the CONCURRENTLY option obviously)
--
Thom
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi,
On 2013-10-24 00:28:44 +0100, Thomas Munro wrote:
I noticed that CLUSTER doesn't have a FREEZE option. Here is a patch to
add that, for consistency with VACUUM. Is it useful?
I think you'd need to prevent that form from working on system catalogs
- xmin has a meaning there sometimes (e.g. indcheckxmin) at least.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
On Thu, Oct 24, 2013 at 10:28:43AM +0530, Amit Kapila wrote:
On Thu, Oct 24, 2013 at 4:58 AM, Thomas Munro <munro@ip9.org> wrote:
Hi
I noticed that CLUSTER doesn't have a FREEZE option. Here is a patch to add
that, for consistency with VACUUM. Is it useful?I wonder why anyone would like to freeze during CLUSTER command when
they already have separate way (VACUUM FREEZE) to achieve it, do you
know or can think of any case where user wants to do it along with
Cluster command?Anyway code side, I think you need to set both feeze_min_age as well
as freeze_table_age, see VACUUM command in gram.yCLUSTER opt_freeze opt_verbose qualified_name cluster_index_specification
{ ClusterStmt *n = makeNode(ClusterStmt); - n->relation = $3; - n->indexname = $4; - n->verbose = $2; + n->relation = $4; + n->freeze_min_age = $2 ? 0 : -1; + n->indexname = $5; + n->verbose = $3; ..With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Hi Amit,
If the FREEZE is part of the CLUSTER, you would only read/write the table
once. With a follow-up VACUUM FREEZE, you would re-read/write a second time.
I, for one, would appreciate being able to perform both in the same run. (+1)
Regards,
Ken
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 10/23/2013 09:58 PM, Amit Kapila wrote:
I wonder why anyone would like to freeze during CLUSTER command when
they already have separate way (VACUUM FREEZE) to achieve it, do you
know or can think of any case where user wants to do it along with
Cluster command?
"If I'm rewriting the table anyway, let's freeze it".
Otherwise, you have to write the same pages twice, if both CLUSTER and
FREEZE are required.
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: WMd71cb723c3ba841a7bc70c91320b49883c10adb5f9c9846499b6dc2582d00e2c52256dce34b1a413b62f26abbc97cfe1@asav-2.01.com
On 24 October 2013 05:58, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Oct 24, 2013 at 4:58 AM, Thomas Munro <munro@ip9.org> wrote:
Hi
I noticed that CLUSTER doesn't have a FREEZE option. Here is a patch toadd
that, for consistency with VACUUM. Is it useful?
I wonder why anyone would like to freeze during CLUSTER command when
they already have separate way (VACUUM FREEZE) to achieve it, do you
know or can think of any case where user wants to do it along with
Cluster command?
As others have said, the goal is to freeze and cluster in a single step.
You can already do that if you know how things work under the covers with:
SET vacuum_freeze_min_age = 0;
CLUSTER my_table;
This patch lets you say CLUSTER FREEZE instead. It mirrors VACUUM, which
can freeze tuples based on the GUC and tuple age or the FREEZE keyword.
Anyway code side, I think you need to set both feeze_min_age as well
as freeze_table_age, see VACUUM command in gram.y
Ok, I attach a new version that is more like VACUUM in gram.y. (Although
I'm not sure why it isn't a single boolean flag).
Thanks,
Thomas Munro
Attachments:
cluster-freeze-v2.patchapplication/octet-stream; name=cluster-freeze-v2.patchDownload
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 339990f..d6c1d84 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -21,8 +21,8 @@ PostgreSQL documentation
<refsynopsisdiv>
<synopsis>
-CLUSTER [VERBOSE] <replaceable class="PARAMETER">table_name</replaceable> [ USING <replaceable class="PARAMETER">index_name</replaceable> ]
-CLUSTER [VERBOSE]
+CLUSTER [FREEZE] [VERBOSE] <replaceable class="PARAMETER">table_name</replaceable> [ USING <replaceable class="PARAMETER">index_name</replaceable> ]
+CLUSTER [FREEZE] [VERBOSE]
</synopsis>
</refsynopsisdiv>
@@ -100,6 +100,19 @@ CLUSTER [VERBOSE]
</varlistentry>
<varlistentry>
+ <term><literal>FREEZE</literal></term>
+ <listitem>
+ <para>
+ Selects aggressive <quote>freezing</quote> of tuples.
+ Specifying <literal>FREEZE</literal> is equivalent to performing
+ <command>CLUSTER</command> with the
+ <xref linkend="guc-vacuum-freeze-min-age"> parameter
+ set to zero.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
<term><literal>VERBOSE</literal></term>
<listitem>
<para>
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index f6a5bfe..aa4b85a 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -177,7 +177,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
heap_close(rel, NoLock);
/* Do the job */
- cluster_rel(tableOid, indexOid, false, stmt->verbose, -1, -1);
+ cluster_rel(tableOid, indexOid, false, stmt->verbose, stmt->freeze_min_age, stmt->freeze_table_age);
}
else
{
@@ -227,7 +227,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
/* functions in indexes may want a snapshot set */
PushActiveSnapshot(GetTransactionSnapshot());
cluster_rel(rvtc->tableOid, rvtc->indexOid, true, stmt->verbose,
- -1, -1);
+ stmt->freeze_min_age, stmt->freeze_table_age);
PopActiveSnapshot();
CommitTransactionCommand();
}
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 65f3b98..16dd7a8 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2711,6 +2711,8 @@ _copyClusterStmt(const ClusterStmt *from)
ClusterStmt *newnode = makeNode(ClusterStmt);
COPY_NODE_FIELD(relation);
+ COPY_SCALAR_FIELD(freeze_min_age);
+ COPY_SCALAR_FIELD(freeze_table_age);
COPY_STRING_FIELD(indexname);
COPY_SCALAR_FIELD(verbose);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 4c9b05e..94e9ab6 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1072,6 +1072,8 @@ static bool
_equalClusterStmt(const ClusterStmt *a, const ClusterStmt *b)
{
COMPARE_NODE_FIELD(relation);
+ COMPARE_SCALAR_FIELD(freeze_min_age);
+ COMPARE_SCALAR_FIELD(freeze_table_age);
COMPARE_STRING_FIELD(indexname);
COMPARE_SCALAR_FIELD(verbose);
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 363c603..d9504ba 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -8456,36 +8456,42 @@ CreateConversionStmt:
/*****************************************************************************
*
* QUERY:
- * CLUSTER [VERBOSE] <qualified_name> [ USING <index_name> ]
- * CLUSTER [VERBOSE]
- * CLUSTER [VERBOSE] <index_name> ON <qualified_name> (for pre-8.3)
+ * CLUSTER [FREEZE] [VERBOSE] <qualified_name> [ USING <index_name> ]
+ * CLUSTER [FREEZE] [VERBOSE]
+ * CLUSTER [FREEZE] [VERBOSE] <index_name> ON <qualified_name> (for pre-8.3)
*
*****************************************************************************/
ClusterStmt:
- CLUSTER opt_verbose qualified_name cluster_index_specification
+ CLUSTER opt_freeze opt_verbose qualified_name cluster_index_specification
{
ClusterStmt *n = makeNode(ClusterStmt);
- n->relation = $3;
- n->indexname = $4;
- n->verbose = $2;
+ n->relation = $4;
+ n->freeze_min_age = $2 ? 0 : -1;
+ n->freeze_table_age = $2 ? 0 : -1;
+ n->indexname = $5;
+ n->verbose = $3;
$$ = (Node*)n;
}
- | CLUSTER opt_verbose
+ | CLUSTER opt_freeze opt_verbose
{
ClusterStmt *n = makeNode(ClusterStmt);
n->relation = NULL;
+ n->freeze_min_age = $2 ? 0 : -1;
+ n->freeze_table_age = $2 ? 0 : -1;
n->indexname = NULL;
- n->verbose = $2;
+ n->verbose = $3;
$$ = (Node*)n;
}
/* kept for pre-8.3 compatibility */
- | CLUSTER opt_verbose index_name ON qualified_name
+ | CLUSTER opt_freeze opt_verbose index_name ON qualified_name
{
ClusterStmt *n = makeNode(ClusterStmt);
- n->relation = $5;
- n->indexname = $3;
- n->verbose = $2;
+ n->relation = $6;
+ n->freeze_min_age = $2 ? 0 : -1;
+ n->freeze_table_age = $2 ? 0 : -1;
+ n->indexname = $4;
+ n->verbose = $3;
$$ = (Node*)n;
}
;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index e5235cb..d5b43a0 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -2441,6 +2441,8 @@ typedef struct DropdbStmt
typedef struct ClusterStmt
{
NodeTag type;
+ int freeze_min_age; /* min freeze age, or -1 to use default */
+ int freeze_table_age; /* age at which to scan whole table */
RangeVar *relation; /* relation being indexed, or NULL if all */
char *indexname; /* original index defined */
bool verbose; /* print progress info */
On Thu, Oct 24, 2013 at 1:09 PM, Josh Berkus <josh@agliodbs.com> wrote:
On 10/23/2013 09:58 PM, Amit Kapila wrote:
I wonder why anyone would like to freeze during CLUSTER command when
they already have separate way (VACUUM FREEZE) to achieve it, do you
know or can think of any case where user wants to do it along with
Cluster command?"If I'm rewriting the table anyway, let's freeze it".
Otherwise, you have to write the same pages twice, if both CLUSTER and
FREEZE are required.
I wonder if we should go so far as to make this the default behavior,
instead of just making it an option.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 10/24/2013 04:55 PM, Robert Haas wrote:
On Thu, Oct 24, 2013 at 1:09 PM, Josh Berkus <josh@agliodbs.com> wrote:
On 10/23/2013 09:58 PM, Amit Kapila wrote:
I wonder why anyone would like to freeze during CLUSTER command when
they already have separate way (VACUUM FREEZE) to achieve it, do you
know or can think of any case where user wants to do it along with
Cluster command?"If I'm rewriting the table anyway, let's freeze it".
Otherwise, you have to write the same pages twice, if both CLUSTER and
FREEZE are required.I wonder if we should go so far as to make this the default behavior,
instead of just making it an option.
+1 from me. Can you think of a reason you *wouldn't* want to freeze?
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: WMeca754c82c639b4582411802967769c83cb7be5ea2113432ac1ef4a8cc2beda981b255695554e0f8ac1412b541ec7e16@asav-1.01.com
Robert Haas <robertmhaas@gmail.com> writes:
I wonder if we should go so far as to make this the default behavior,
instead of just making it an option.
In that case you'd have to invent a NOFREEZE keyword, no? Ick.
In any case, it's very far from obvious to me that CLUSTER ought
to throw away information by default, which is what you're proposing.
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
On Thu, Oct 24, 2013 at 10:39 PM, Josh Berkus <josh@agliodbs.com> wrote:
On 10/23/2013 09:58 PM, Amit Kapila wrote:
I wonder why anyone would like to freeze during CLUSTER command when
they already have separate way (VACUUM FREEZE) to achieve it, do you
know or can think of any case where user wants to do it along with
Cluster command?"If I'm rewriting the table anyway, let's freeze it".
So do you think that other places where we are rewriting the table
with exclusive lock, we should have such mechanism or option and even
if it is not there, it is kind of Todo item and tomorrow someone can
write a patch to improve that situation.
Otherwise, you have to write the same pages twice, if both CLUSTER and
FREEZE are required.
Yes, this is completely right and I understand this point, but the
question I had in my mind before writing my last mail was that whether
any or all places having this concept deserves to have an option like
FREEZE?
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Oct 25, 2013 at 4:29 AM, Thomas Munro <munro@ip9.org> wrote:
On 24 October 2013 05:58, Amit Kapila <amit.kapila16@gmail.com> wrote:
On Thu, Oct 24, 2013 at 4:58 AM, Thomas Munro <munro@ip9.org> wrote:
Hi
I noticed that CLUSTER doesn't have a FREEZE option. Here is a patch to
add
that, for consistency with VACUUM. Is it useful?I wonder why anyone would like to freeze during CLUSTER command when
they already have separate way (VACUUM FREEZE) to achieve it, do you
know or can think of any case where user wants to do it along with
Cluster command?As others have said, the goal is to freeze and cluster in a single step.
You can already do that if you know how things work under the covers with:SET vacuum_freeze_min_age = 0;
CLUSTER my_table;
True, but in that case why don't we just mention above in the
documentation of CLUSTER command, so that if user wants to freeze
along with Cluster, he can use above way to Cluster.
Some of the reason's, I could think of adding FREEZE as an option are:
a. it's more explicit and easy to use for user.
b. if by chance underlying mechanism changes (which is less likely)
then above way of doing Cluster might not result into freeze.
This patch lets you say CLUSTER FREEZE instead. It mirrors VACUUM, which
can freeze tuples based on the GUC and tuple age or the FREEZE keyword.Anyway code side, I think you need to set both feeze_min_age as well
as freeze_table_age, see VACUUM command in gram.yOk, I attach a new version that is more like VACUUM in gram.y. (Although
I'm not sure why it isn't a single boolean flag).
The reason of separate flags is that both are used to decide different things,
freeze_min_age - this is used to decide the cutoff_xid, based on which
FrozenTransactionId will be placed on tuple.
freeze_table_age - used do decide, whether to scan all pages of a
relation in Vacuum and in Cluster command it is ignored as it needs to
scan all pages anyway.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-10-24 17:17:22 -0700, Josh Berkus wrote:
On 10/24/2013 04:55 PM, Robert Haas wrote:
On Thu, Oct 24, 2013 at 1:09 PM, Josh Berkus <josh@agliodbs.com> wrote:
On 10/23/2013 09:58 PM, Amit Kapila wrote:
I wonder why anyone would like to freeze during CLUSTER command when
they already have separate way (VACUUM FREEZE) to achieve it, do you
know or can think of any case where user wants to do it along with
Cluster command?"If I'm rewriting the table anyway, let's freeze it".
Otherwise, you have to write the same pages twice, if both CLUSTER and
FREEZE are required.I wonder if we should go so far as to make this the default behavior,
instead of just making it an option.+1 from me. Can you think of a reason you *wouldn't* want to freeze?
It makes content from the future appear when you start using the
relation in a query/session with an older snapshot. Currently CLUSTER is
safe against that.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
On Fri, Oct 25, 2013 at 2:12 AM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2013-10-24 17:17:22 -0700, Josh Berkus wrote:
On 10/24/2013 04:55 PM, Robert Haas wrote:
On Thu, Oct 24, 2013 at 1:09 PM, Josh Berkus <josh@agliodbs.com> wrote:
On 10/23/2013 09:58 PM, Amit Kapila wrote:
I wonder why anyone would like to freeze during CLUSTER command when
they already have separate way (VACUUM FREEZE) to achieve it, do you
know or can think of any case where user wants to do it along with
Cluster command?"If I'm rewriting the table anyway, let's freeze it".
Otherwise, you have to write the same pages twice, if both CLUSTER and
FREEZE are required.I wonder if we should go so far as to make this the default behavior,
instead of just making it an option.+1 from me. Can you think of a reason you *wouldn't* want to freeze?
It makes content from the future appear when you start using the
relation in a query/session with an older snapshot. Currently CLUSTER is
safe against that.
Eh, what? We wouldn't freeze XIDs that don't precede RecentGlobalXmin.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-10-25 09:13:20 -0400, Robert Haas wrote:
On Fri, Oct 25, 2013 at 2:12 AM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2013-10-24 17:17:22 -0700, Josh Berkus wrote:
On 10/24/2013 04:55 PM, Robert Haas wrote:
On Thu, Oct 24, 2013 at 1:09 PM, Josh Berkus <josh@agliodbs.com> wrote:
On 10/23/2013 09:58 PM, Amit Kapila wrote:
I wonder why anyone would like to freeze during CLUSTER command when
they already have separate way (VACUUM FREEZE) to achieve it, do you
know or can think of any case where user wants to do it along with
Cluster command?"If I'm rewriting the table anyway, let's freeze it".
Otherwise, you have to write the same pages twice, if both CLUSTER and
FREEZE are required.I wonder if we should go so far as to make this the default behavior,
instead of just making it an option.+1 from me. Can you think of a reason you *wouldn't* want to freeze?
It makes content from the future appear when you start using the
relation in a query/session with an older snapshot. Currently CLUSTER is
safe against that.Eh, what? We wouldn't freeze XIDs that don't precede RecentGlobalXmin.
Ah sorry, I thought that'd be the plan, similar to COPY FREEZE. Maybe
because I've wished for it in the past ;)
In that case I agree it should be the default. There really isn't any
reason to not to immediately freeze tuples that can be frozen according
to the xmin horizon. We don't immediately do it during normal vacuums
because it would possibly cause superflous io - but that's not the case here.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
On Thu, Oct 24, 2013 at 10:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Robert Haas <robertmhaas@gmail.com> writes:
I wonder if we should go so far as to make this the default behavior,
instead of just making it an option.In that case you'd have to invent a NOFREEZE keyword, no? Ick.
Only if we think anyone would ever NOT want to freeze.
In any case, it's very far from obvious to me that CLUSTER ought
to throw away information by default, which is what you're proposing.
I find it odd to referring to this as throwing away information. I
know that you have a general concern about throwing away XIDs that are
still needed for forensic purposes, but that is clearly the ONLY
purpose that those XIDs serve, and the I/O advantages of freezing by
default could be massive for many of our users. What's going to
happen in practice is that experienced users will simply recommend
CLUSTER FREEZE rather than plain CLUSTER, and you won't have the
forensic information *anyway*.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 10/24/2013 07:19 PM, Tom Lane wrote:
In any case, it's very far from obvious to me that CLUSTER ought
to throw away information by default, which is what you're proposing.
The problem here is that you're thinking of the 1/10 of 1% of our users
who have a serious PostgreSQL failure and post something on the lists
for help, for which XID forensic information is useful. As opposed to
the 99.9% of our users for whom deferred freezing is a performance
burden. While I realize that the 0.1% of users are more likely to have
contact with you, personally, it's still bad policy for the project.
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: WM639033dfece437f673cfe615cd41534dea857cdfa0775b83b26f682ae06a62ee36a6d137f4e05a02013b637f330e0583@asav-1.01.com
On 25 October 2013 01:17, Josh Berkus <josh@agliodbs.com> wrote:
On 10/24/2013 04:55 PM, Robert Haas wrote:
I wonder if we should go so far as to make this the default behavior,
instead of just making it an option.+1 from me. Can you think of a reason you *wouldn't* want to freeze?
Ok, I attach an alternative patch that makes CLUSTER *always* freeze,
without any option (but doesn't affect VACUUM FULL in the same way). I will
post both alternatives to the commitfest app since there seems to be some
disagreement about whether tuple freezing should be an optional.
Thanks
Thomas Munro
Attachments:
cluster-freeze-always.patchapplication/octet-stream; name=cluster-freeze-always.patchDownload
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index f6a5bfe..a3e098e 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -177,7 +177,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
heap_close(rel, NoLock);
/* Do the job */
- cluster_rel(tableOid, indexOid, false, stmt->verbose, -1, -1);
+ cluster_rel(tableOid, indexOid, false, stmt->verbose, 0, 0);
}
else
{
@@ -227,7 +227,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
/* functions in indexes may want a snapshot set */
PushActiveSnapshot(GetTransactionSnapshot());
cluster_rel(rvtc->tableOid, rvtc->indexOid, true, stmt->verbose,
- -1, -1);
+ 0, 0);
PopActiveSnapshot();
CommitTransactionCommand();
}
@@ -853,9 +853,7 @@ copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
*pSwapToastByContent = false;
/*
- * compute xids used to freeze and weed out dead tuples. We use -1
- * freeze_min_age to avoid having CLUSTER freeze tuples earlier than a
- * plain VACUUM would.
+ * compute xids used to freeze and weed out dead tuples.
*/
vacuum_set_xid_limits(freeze_min_age, freeze_table_age,
OldHeap->rd_rel->relisshared,
On 10/26/2013 01:20 AM, Josh Berkus wrote:
On 10/24/2013 07:19 PM, Tom Lane wrote:
In any case, it's very far from obvious to me that CLUSTER ought
to throw away information by default, which is what you're proposing.The problem here is that you're thinking of the 1/10 of 1% of our users
who have a serious PostgreSQL failure and post something on the lists
for help, for which XID forensic information is useful. As opposed to
the 99.9% of our users for whom deferred freezing is a performance
burden. While I realize that the 0.1% of users are more likely to have
contact with you, personally, it's still bad policy for the project.
Strong +1 from me. Doing the performant, sensible, low-admin thing by
default is really important if you don't want a database that requires a
two year training course and a professional DBA to use. Some DB vendors
make that part of their business model, but personally at least that's
certainly not the direction I'd like to see Pg go in.
Autovacuum wrap-around prevention already gets rid of this info, it's
not like it's kept forever anyway.
Anything that makes the mechanics of bloat and vacuum less visible is a
big win as far as I'm concerned.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
On 2013-10-25 09:26:29 -0400, Robert Haas wrote:
In any case, it's very far from obvious to me that CLUSTER ought
to throw away information by default, which is what you're proposing.I find it odd to referring to this as throwing away information. I
know that you have a general concern about throwing away XIDs that are
still needed for forensic purposes, but that is clearly the ONLY
purpose that those XIDs serve, and the I/O advantages of freezing by
default could be massive for many of our users. What's going to
happen in practice is that experienced users will simply recommend
CLUSTER FREEZE rather than plain CLUSTER, and you won't have the
forensic information *anyway*.
I think we should just apply your "preserve forensic information when
freezing" patch. Then we're good to go without big arguments ;)
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
On Tue, Oct 29, 2013 at 10:32 AM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2013-10-25 09:26:29 -0400, Robert Haas wrote:
In any case, it's very far from obvious to me that CLUSTER ought
to throw away information by default, which is what you're proposing.I find it odd to referring to this as throwing away information. I
know that you have a general concern about throwing away XIDs that are
still needed for forensic purposes, but that is clearly the ONLY
purpose that those XIDs serve, and the I/O advantages of freezing by
default could be massive for many of our users. What's going to
happen in practice is that experienced users will simply recommend
CLUSTER FREEZE rather than plain CLUSTER, and you won't have the
forensic information *anyway*.I think we should just apply your "preserve forensic information when
freezing" patch. Then we're good to go without big arguments ;)
Well, I'm happy with that, too. But you wanted it significantly
reworked and I haven't had time to do that.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-10-29 11:29:24 -0400, Robert Haas wrote:
On Tue, Oct 29, 2013 at 10:32 AM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2013-10-25 09:26:29 -0400, Robert Haas wrote:
In any case, it's very far from obvious to me that CLUSTER ought
to throw away information by default, which is what you're proposing.I find it odd to referring to this as throwing away information. I
know that you have a general concern about throwing away XIDs that are
still needed for forensic purposes, but that is clearly the ONLY
purpose that those XIDs serve, and the I/O advantages of freezing by
default could be massive for many of our users. What's going to
happen in practice is that experienced users will simply recommend
CLUSTER FREEZE rather than plain CLUSTER, and you won't have the
forensic information *anyway*.I think we should just apply your "preserve forensic information when
freezing" patch. Then we're good to go without big arguments ;)Well, I'm happy with that, too. But you wanted it significantly
reworked and I haven't had time to do that.
I did? I only seem to remember suggesting to introduce
HeapTupleHeaderGetRawXmin() and some bugfix around rewriteheap.c? I
think the RawXmin() thing is a judgement call...
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
On Tue, Oct 29, 2013 at 11:37 AM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2013-10-29 11:29:24 -0400, Robert Haas wrote:
On Tue, Oct 29, 2013 at 10:32 AM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2013-10-25 09:26:29 -0400, Robert Haas wrote:
In any case, it's very far from obvious to me that CLUSTER ought
to throw away information by default, which is what you're proposing.I find it odd to referring to this as throwing away information. I
know that you have a general concern about throwing away XIDs that are
still needed for forensic purposes, but that is clearly the ONLY
purpose that those XIDs serve, and the I/O advantages of freezing by
default could be massive for many of our users. What's going to
happen in practice is that experienced users will simply recommend
CLUSTER FREEZE rather than plain CLUSTER, and you won't have the
forensic information *anyway*.I think we should just apply your "preserve forensic information when
freezing" patch. Then we're good to go without big arguments ;)Well, I'm happy with that, too. But you wanted it significantly
reworked and I haven't had time to do that.I did? I only seem to remember suggesting to introduce
HeapTupleHeaderGetRawXmin() and some bugfix around rewriteheap.c? I
think the RawXmin() thing is a judgement call...
Well every place that currently gets the xmin will have to be changed
to get the raw-xmin instead, with the exception of hunks like this:
- targetxmin = HeapTupleHeaderGetXmin(tuple->t_data);
+ if (HeapTupleHeaderXminFrozen(tuple->t_data))
+ targetxmin = FrozenTransactionId;
+ else
+ targetxmin = HeapTupleHeaderGetXmin(tuple->t_data);...which will instead need to be reverted. The rename is mostly
mechanical, but going through and looking for places where the
difference between Xmin() and RawXmin() means that other hunks can be
reverted is less so. I suppose it wouldn't take more than a few
hours; I've just been up to my ears in parallelism stuff.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Oct 30, 2013 at 3:32 AM, Andres Freund <andres@2ndquadrant.com>wrote:
On 2013-10-25 09:26:29 -0400, Robert Haas wrote:
In any case, it's very far from obvious to me that CLUSTER ought
to throw away information by default, which is what you're proposing.I find it odd to referring to this as throwing away information. I
know that you have a general concern about throwing away XIDs that are
still needed for forensic purposes, but that is clearly the ONLY
purpose that those XIDs serve, and the I/O advantages of freezing by
default could be massive for many of our users. What's going to
happen in practice is that experienced users will simply recommend
CLUSTER FREEZE rather than plain CLUSTER, and you won't have the
forensic information *anyway*.I think we should just apply your "preserve forensic information when
freezing" patch. Then we're good to go without big arguments ;)
Ok, here's a recap on the thread as I see it now.
1. Thomas wrote the patch with the idea that FREEZE could be an option for
cluster to freeze the tuples during the cluster operation, which would save
a vacuum freeze somewhere down the line. Sounds like a good idea.
2. Robert introduced the idea that this perhaps could be the default option
for cluster.
3. Tom highlighted that making freeze the default would wipe out xmin
values so they wouldn't be available to any forensics which might to take
place in the event of a disaster.
4. Andres mentioned that we might want to wait for a patch which Robert has
been working on which, currently I don't know much about but it sounds like
it freezes without setting xmin to frozenXid?
5. Robert stated that he's not had much time to work on this patch due to
having other commitments.
In the meantime Thomas sent in a patch which gets rid of the FREEZE option
from cluster and makes it the default.
So now I'm wondering what the next move should be for this patch?
a. Are we waiting on Robert's patch to be commited before we can apply
Thomas's cluster with freeze as default?
b. Are we waiting on me reviewing one or both of the patches? Which one?
I think probably it sounds safer not to make freeze the default, but then
if we release 9.4 with CLUSTER FREEZE then in 9.5/10 decide it should be
the default then we then have a redundant syntax to support for ever and
ever.
Decision time?
Regards
David Rowley
Show quoted text
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
On Mon, Nov 18, 2013 at 09:22:58PM +1300, David Rowley wrote:
So now I'm wondering what the next move should be for this patch?
a. Are we waiting on Robert's patch to be committed before we can apply Thomas's
cluster with freeze as default?
b. Are we waiting on me reviewing one or both of the patches? Which one?I think probably it sounds safer not to make freeze the default, but then if we
release 9.4 with CLUSTER FREEZE then in 9.5/10 decide it should be the default
then we then have a redundant syntax to support for ever and ever.Decision time?
Yes, we probably should make a decision, unless Robert's idea can be
implemented. We have to balance the ease of debugging MVCC failures
with the interface we give to the user community.
From my perspective, I don't see how we can justify the addition of a
FREEZE option to CLUSTER. If we explain that you would always use
FREEZE unless you want to preserve MVCC information for later debugging,
users will reply with "Huh, why would I want that?" MVCC debugging is
just too rare an event for them to understand its usefulness.
If we do add FREEZE, all we would be doing is delaying the time when all
CLUSTER operations will use FREEZE, and hence debugging will be just as
difficult. My point is that will full knowledge, everyone would use
FREEZE unless they expect MVCC bugs, which is going to be an almost zero
population.
Many MVCC failures are reproducible, so if we provide a C define that
disables the freezing so MVCC information can be preserved, that might
be good enough. Developers could enable the macro, and the macro could
be used in other places where MVCC information is lost.
This will make some MVCC harder, and will perhaps allow some MVCC bugs
to exist longer.
I believe there are other cases where rows could be frozen but we have
avoided it for MVCC debugging reasons. Another idea would be the
addition of a GUC that can disable optimistic freezing. This could be
enabled by sites that suspect MVCC bugs.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-11-18 11:39:44 -0500, Bruce Momjian wrote:
On Mon, Nov 18, 2013 at 09:22:58PM +1300, David Rowley wrote:
So now I'm wondering what the next move should be for this patch?
a. Are we waiting on Robert's patch to be committed before we can apply Thomas's
cluster with freeze as default?
b. Are we waiting on me reviewing one or both of the patches? Which one?I think probably it sounds safer not to make freeze the default, but then if we
release 9.4 with CLUSTER FREEZE then in 9.5/10 decide it should be the default
then we then have a redundant syntax to support for ever and ever.Decision time?
Yes, we probably should make a decision, unless Robert's idea can be
implemented. We have to balance the ease of debugging MVCC failures
with the interface we give to the user community.
Imo that patch really doesn't need too much further work.
From my perspective, I don't see how we can justify the addition of a
FREEZE option to CLUSTER. If we explain that you would always use
FREEZE unless you want to preserve MVCC information for later debugging,
users will reply with "Huh, why would I want that?"
I tend to agree that we should work towards not needing that option.
Many MVCC failures are reproducible, so if we provide a C define that
disables the freezing so MVCC information can be preserved, that might
be good enough. Developers could enable the macro, and the macro could
be used in other places where MVCC information is lost.
This will make some MVCC harder, and will perhaps allow some MVCC bugs
to exist longer.
I believe there are other cases where rows could be frozen but we have
avoided it for MVCC debugging reasons. Another idea would be the
addition of a GUC that can disable optimistic freezing. This could be
enabled by sites that suspect MVCC bugs.
I think that'd be an enormous failure. You don't usually need to look at
those because there's a bug in postgres' mvcc logic but somewhere
else (application, other postgres code). And looking at the mvcc
information helps you to narrow it down, so you have a chance of
actually finding a reproducible bug.
Besides, in many of the !rewrite cases it's far from clear in which
cases it's a benefit to freeze earlier.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
On 11/18/2013 08:39 AM, Bruce Momjian wrote:
If we do add FREEZE, all we would be doing is delaying the time when all
CLUSTER operations will use FREEZE, and hence debugging will be just as
difficult. My point is that will full knowledge, everyone would use
FREEZE unless they expect MVCC bugs, which is going to be an almost zero
population.
+1
None of our users would willingly choose worse performance over the 0.1%
possibility of needing to analyze a transaction failure.
--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Import Notes
Reply to msg id not found: WM54ddde6a2b598cb56f5cfde073a0ac1c07daf5e8dde73bc1cbe94fae5414472d75ded77e8a0c6a4e1df5f13da7d9511c@asav-3.01.com
Josh Berkus <josh@agliodbs.com> wrote:
On 11/18/2013 08:39 AM, Bruce Momjian wrote:
If we do add FREEZE, all we would be doing is delaying the time
when all CLUSTER operations will use FREEZE, and hence debugging
will be just as difficult. My point is that will full
knowledge, everyone would use FREEZE unless they expect MVCC
bugs, which is going to be an almost zero population.+1
+1
None of our users would willingly choose worse performance over
the 0.1% possibility of needing to analyze a transaction failure.
I assume that's intended to represent the lifetime probability that
a typical user would ever benefit, not per year or per transaction.
Even as a lifetime number, it seems high unless we're specifically
talking about hackers.
--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Oct 26, 2013 at 11:19 AM, Thomas Munro <munro@ip9.org> wrote:
On 25 October 2013 01:17, Josh Berkus <josh@agliodbs.com> wrote:
On 10/24/2013 04:55 PM, Robert Haas wrote:
I wonder if we should go so far as to make this the default behavior,
instead of just making it an option.+1 from me. Can you think of a reason you *wouldn't* want to freeze?
Ok, I attach an alternative patch that makes CLUSTER *always* freeze,
without any option (but doesn't affect VACUUM FULL in the same way). I will
post both alternatives to the commitfest app since there seems to be some
disagreement about whether tuple freezing should be an optional.
It seems that most people to voice their opinion are leaning towards this
being the more desired behaviour rather than adding the FREEZE as an
option, so I reviewed this patch tonight.
I followed the code around and checked that we do still need the freeze age
parameters in cluster_rel and we do because vacuum full uses the
cluster_rel code and it will only perform a freeze if a user does VACUUM
FULL FREEZE.
I have mixed feelings about updating the comment before the call
to vacuum_set_xid_limits. It looks like the previous comment probably had
not been looked at since vacuum full started using cluster_rel, so perhaps
removing that was good, but on the other hand maybe it should be mentioning
vacuum full and vacuum full freeze? Though I'm probably leaning more
towards what you've changed it to as previously the comment was being a bit
too clever and assuming things about the calling code which turned out bad
as it seemed out of date and lacked knowledge of vacuum full using it.
I think that the patch should include some sort of notes in the documents
to say that cluster performs freezing of tuples. I've attached a patch
which adds something there, but I'm not 100% sure it is the right thing.
Perhaps it should just read:
Cluster also performs aggressive "freezing" of tuples similar to VACUUM
FULL FREEZE.
Although it's not exactly the same as you can perform a vacuum full freeze
on a relation which does not have the clustered index set.
I'll delay a bit to see if anyone else has any comments about what the docs
should read, but I think it is pretty much ready for a commiter's eyes.
Regards
David Rowley
Show quoted text
Thanks
Thomas Munro--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments:
cluster_freeze_always_with_docs.patchapplication/octet-stream; name=cluster_freeze_always_with_docs.patchDownload
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 339990f..a44a7f9 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -172,6 +172,12 @@ CLUSTER [VERBOSE]
are periodically reclustered.
</para>
+ <para>
+ The <command>CLUSTER</command> command also perform freezing of eligible
+ tuples during its operation, this may reduce the need for a subsequent
+ <command>VACUUM FREEZE</command> on the table.
+ </para>
+
</refsect1>
<refsect1>
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index f6a5bfe..8e867ea 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -177,7 +177,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
heap_close(rel, NoLock);
/* Do the job */
- cluster_rel(tableOid, indexOid, false, stmt->verbose, -1, -1);
+ cluster_rel(tableOid, indexOid, false, stmt->verbose, 0, 0);
}
else
{
@@ -227,7 +227,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
/* functions in indexes may want a snapshot set */
PushActiveSnapshot(GetTransactionSnapshot());
cluster_rel(rvtc->tableOid, rvtc->indexOid, true, stmt->verbose,
- -1, -1);
+ 0, 0);
PopActiveSnapshot();
CommitTransactionCommand();
}
@@ -852,11 +852,7 @@ copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
else
*pSwapToastByContent = false;
- /*
- * compute xids used to freeze and weed out dead tuples. We use -1
- * freeze_min_age to avoid having CLUSTER freeze tuples earlier than a
- * plain VACUUM would.
- */
+ /* compute xids used to freeze and weed out dead tuples. */
vacuum_set_xid_limits(freeze_min_age, freeze_table_age,
OldHeap->rd_rel->relisshared,
&OldestXmin, &FreezeXid, NULL, &MultiXactCutoff);
On Mon, Nov 18, 2013 at 11:45 AM, Andres Freund <andres@2ndquadrant.com> wrote:
Yes, we probably should make a decision, unless Robert's idea can be
implemented. We have to balance the ease of debugging MVCC failures
with the interface we give to the user community.Imo that patch really doesn't need too much further work.
Would you care to submit a version you're happy with?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2013-11-19 12:23:30 -0500, Robert Haas wrote:
On Mon, Nov 18, 2013 at 11:45 AM, Andres Freund <andres@2ndquadrant.com> wrote:
Yes, we probably should make a decision, unless Robert's idea can be
implemented. We have to balance the ease of debugging MVCC failures
with the interface we give to the user community.Imo that patch really doesn't need too much further work.
Would you care to submit a version you're happy with?
I'll give it a spin sometime this week.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
On Tue, Nov 19, 2013 at 11:35 PM, David Rowley <dgrowleyml@gmail.com> wrote:
I think that the patch should include some sort of notes in the documents
to say that cluster performs freezing of tuples. I've attached a patch
which adds something there, but I'm not 100% sure it is the right thing.
Perhaps it should just read:Cluster also performs aggressive "freezing" of tuples similar to VACUUM
FULL FREEZE.Although it's not exactly the same as you can perform a vacuum full freeze
on a relation which does not have the clustered index set.I'll delay a bit to see if anyone else has any comments about what the
docs should read, but I think it is pretty much ready for a commiter's eyes.
Hi Thomas,
I'm just going to mark the patch as waiting for author for now until you
get the chance to add some changes to the documents.
Everything else looks ok.
Regards
David Rowley
Show quoted text
Regards
David Rowley
Thanks
Thomas Munro--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, 2013-11-19 at 18:24 +0100, Andres Freund wrote:
On 2013-11-19 12:23:30 -0500, Robert Haas wrote:
On Mon, Nov 18, 2013 at 11:45 AM, Andres Freund <andres@2ndquadrant.com> wrote:
Yes, we probably should make a decision, unless Robert's idea can be
implemented. We have to balance the ease of debugging MVCC failures
with the interface we give to the user community.Imo that patch really doesn't need too much further work.
Would you care to submit a version you're happy with?
I'll give it a spin sometime this week.
I'm setting the CLUSTER FREEZE patch as returned with feedback, until
this other patch has been considered.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Dec 8, 2013 at 10:51 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
On Tue, 2013-11-19 at 18:24 +0100, Andres Freund wrote:
On 2013-11-19 12:23:30 -0500, Robert Haas wrote:
On Mon, Nov 18, 2013 at 11:45 AM, Andres Freund <andres@2ndquadrant.com> wrote:
Yes, we probably should make a decision, unless Robert's idea can be
implemented. We have to balance the ease of debugging MVCC failures
with the interface we give to the user community.Imo that patch really doesn't need too much further work.
Would you care to submit a version you're happy with?
I'll give it a spin sometime this week.
I'm setting the CLUSTER FREEZE patch as returned with feedback, until
this other patch has been considered.
The other patch in question is now committed. Here's a patch for
this, which does somewhat more extensive surgery than previously
proposed (actually, I wrote it from scratch, before looking at the
prior submission). It's basically the same idea, though. Note that
both versions of the patch affect not only CLUSTER, but also VACUUM
FULL.
I suspect we ought to extend this to rewriting variants of ALTER TABLE
as well, but a little thought is needed there. ATRewriteTables()
appears to just call heap_insert() for each updated row, which if I'm
not mistaken is an MVCC violation - offhand, it seems like a
transaction with an older MVCC snapshot could access the table for
this first time after the rewriter commits, and fail to see rows which
would have appeared to be there before the rewrite. (I haven't
actually tested this, so watch me be wrong.) If we're OK with an MVCC
violation here, we could just pass HEAP_INSERT_FROZEN and have a
slightly different flavor of violation; if not, this needs some kind
of more extensive surgery quite apart from what we do about freezing.
Review of the attached appreciated...
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
cluster-freeze.patchtext/x-patch; charset=US-ASCII; name=cluster-freeze.patchDownload
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index eb71581..1e98473 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -102,7 +102,9 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] ANALYZE [ <replaceable class="PARAMETER">
Specifying <literal>FREEZE</literal> is equivalent to performing
<command>VACUUM</command> with the
<xref linkend="guc-vacuum-freeze-min-age"> parameter
- set to zero.
+ set to zero. Aggressive freezing is always performed when the
+ table is rewritten, so this option is redundant when <literal>FULL</>
+ is specified.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index deec77d..634754c 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -345,7 +345,7 @@ rewrite_heap_tuple(RewriteState state,
/*
* While we have our hands on the tuple, we may as well freeze any
- * very-old xmin or xmax, so that future VACUUM effort can be saved.
+ * eligible xmin or xmax, so that future VACUUM effort can be saved.
*/
heap_freeze_tuple(new_tuple->t_data, state->rs_freeze_xid,
state->rs_cutoff_multi);
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 0b8ac8c..9f41278 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -64,12 +64,10 @@ typedef struct
} RelToCluster;
-static void rebuild_relation(Relation OldHeap, Oid indexOid,
- int freeze_min_age, int freeze_table_age, bool verbose);
+static void rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose);
static void copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
- int freeze_min_age, int freeze_table_age, bool verbose,
- bool *pSwapToastByContent, TransactionId *pFreezeXid,
- MultiXactId *pCutoffMulti);
+ bool verbose, bool *pSwapToastByContent,
+ TransactionId *pFreezeXid, MultiXactId *pCutoffMulti);
static List *get_tables_to_cluster(MemoryContext cluster_context);
static void reform_and_rewrite_tuple(HeapTuple tuple,
TupleDesc oldTupDesc, TupleDesc newTupDesc,
@@ -176,11 +174,8 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
/* close relation, keep lock till commit */
heap_close(rel, NoLock);
- /*
- * Do the job. We use a -1 freeze_min_age to avoid having CLUSTER
- * freeze tuples earlier than a plain VACUUM would.
- */
- cluster_rel(tableOid, indexOid, false, stmt->verbose, -1, -1);
+ /* Do the job. */
+ cluster_rel(tableOid, indexOid, false, stmt->verbose);
}
else
{
@@ -229,9 +224,8 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
StartTransactionCommand();
/* functions in indexes may want a snapshot set */
PushActiveSnapshot(GetTransactionSnapshot());
- /* Do the job. As above, use a -1 freeze_min_age. */
- cluster_rel(rvtc->tableOid, rvtc->indexOid, true, stmt->verbose,
- -1, -1);
+ /* Do the job. */
+ cluster_rel(rvtc->tableOid, rvtc->indexOid, true, stmt->verbose);
PopActiveSnapshot();
CommitTransactionCommand();
}
@@ -262,8 +256,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
* and error messages should refer to the operation as VACUUM not CLUSTER.
*/
void
-cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose,
- int freeze_min_age, int freeze_table_age)
+cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose)
{
Relation OldHeap;
@@ -407,8 +400,7 @@ cluster_rel(Oid tableOid, Oid indexOid, bool recheck, bool verbose,
TransferPredicateLocksToHeapRelation(OldHeap);
/* rebuild_relation does all the dirty work */
- rebuild_relation(OldHeap, indexOid, freeze_min_age, freeze_table_age,
- verbose);
+ rebuild_relation(OldHeap, indexOid, verbose);
/* NB: rebuild_relation does heap_close() on OldHeap */
}
@@ -561,8 +553,7 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
* NB: this routine closes OldHeap at the right time; caller should not.
*/
static void
-rebuild_relation(Relation OldHeap, Oid indexOid,
- int freeze_min_age, int freeze_table_age, bool verbose)
+rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose)
{
Oid tableOid = RelationGetRelid(OldHeap);
Oid tableSpace = OldHeap->rd_rel->reltablespace;
@@ -587,8 +578,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid,
AccessExclusiveLock);
/* Copy the heap data into the new table in the desired order */
- copy_heap_data(OIDNewHeap, tableOid, indexOid,
- freeze_min_age, freeze_table_age, verbose,
+ copy_heap_data(OIDNewHeap, tableOid, indexOid, verbose,
&swap_toast_by_content, &frozenXid, &cutoffMulti);
/*
@@ -743,8 +733,7 @@ make_new_heap(Oid OIDOldHeap, Oid NewTableSpace, bool forcetemp,
* *pCutoffMulti receives the MultiXactId used as a cutoff point.
*/
static void
-copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
- int freeze_min_age, int freeze_table_age, bool verbose,
+copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex, bool verbose,
bool *pSwapToastByContent, TransactionId *pFreezeXid,
MultiXactId *pCutoffMulti)
{
@@ -857,10 +846,11 @@ copy_heap_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
*pSwapToastByContent = false;
/*
- * compute xids used to freeze and weed out dead tuples.
+ * Compute xids used to freeze and weed out dead tuples and multixacts.
+ * Since we're going to rewrite the whole table anyway, there's no reason
+ * not to be aggressive about this.
*/
- vacuum_set_xid_limits(freeze_min_age, freeze_table_age,
- OldHeap->rd_rel->relisshared,
+ vacuum_set_xid_limits(0, 0, OldHeap->rd_rel->relisshared,
&OldestXmin, &FreezeXid, NULL, &MultiXactCutoff,
NULL);
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 1199060..ba0e841 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -1149,8 +1149,7 @@ 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,
- vacstmt->freeze_min_age, vacstmt->freeze_table_age);
+ (vacstmt->options & VACOPT_VERBOSE) != 0);
}
else
lazy_vacuum_rel(onerel, vacstmt, vac_strategy);
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index a887318..5dfa998 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -20,7 +20,7 @@
extern void cluster(ClusterStmt *stmt, bool isTopLevel);
extern void cluster_rel(Oid tableOid, Oid indexOid, bool recheck,
- bool verbose, int freeze_min_age, int freeze_table_age);
+ bool verbose);
extern void check_index_is_clusterable(Relation OldHeap, Oid indexOid,
bool recheck, LOCKMODE lockmode);
extern void mark_index_clustered(Relation rel, Oid indexOid, bool is_internal);
On 2013-12-22 20:45:02 -0500, Robert Haas wrote:
I suspect we ought to extend this to rewriting variants of ALTER TABLE
as well, but a little thought is needed there. ATRewriteTables()
appears to just call heap_insert() for each updated row, which if I'm
not mistaken is an MVCC violation - offhand, it seems like a
transaction with an older MVCC snapshot could access the table for
this first time after the rewriter commits, and fail to see rows which
would have appeared to be there before the rewrite. (I haven't
actually tested this, so watch me be wrong.) If we're OK with an MVCC
violation here, we could just pass HEAP_INSERT_FROZEN and have a
slightly different flavor of violation; if not, this needs some kind
of more extensive surgery quite apart from what we do about freezing.
Yes, rewriting ALTER TABLEs certainly aren't MVCC safe. I thought that
was documented, but apparently not.
I am not sure it can be fixed easily using the tricks CLUSTER plays,
there might be nasty edge-cases because of the changes in the column
definition. Certainly not a trivial project.
I think we should leave ALTER TABLE as a completely separate project and
just improve CLUSTER for now. The practical impact of rewriting ALTER
TABLEs not freezing is far smaller, because they are very seldomly
performed in bigger databases.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, 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
On Mon, Dec 23, 2013 at 6:53 AM, Andres Freund <andres@2ndquadrant.com> wrote:
On 2013-12-22 20:45:02 -0500, Robert Haas wrote:
I suspect we ought to extend this to rewriting variants of ALTER TABLE
as well, but a little thought is needed there. ATRewriteTables()
appears to just call heap_insert() for each updated row, which if I'm
not mistaken is an MVCC violation - offhand, it seems like a
transaction with an older MVCC snapshot could access the table for
this first time after the rewriter commits, and fail to see rows which
would have appeared to be there before the rewrite. (I haven't
actually tested this, so watch me be wrong.) If we're OK with an MVCC
violation here, we could just pass HEAP_INSERT_FROZEN and have a
slightly different flavor of violation; if not, this needs some kind
of more extensive surgery quite apart from what we do about freezing.Yes, rewriting ALTER TABLEs certainly aren't MVCC safe. I thought that
was documented, but apparently not.
I am not sure it can be fixed easily using the tricks CLUSTER plays,
there might be nasty edge-cases because of the changes in the column
definition. Certainly not a trivial project.I think we should leave ALTER TABLE as a completely separate project and
just improve CLUSTER for now. The practical impact of rewriting ALTER
TABLEs not freezing is far smaller, because they are very seldomly
performed in bigger databases.
OK, I have committed my patch to make CLUSTER and VACUUM FULL freeze
aggressively, and have left ALTER TABLE alone for now.
It would be nice to get to the point where a database-wide VACUUM FULL
would serve to bump datfrozenxid, so as to avoid having to give this
sort of advice:
/messages/by-id/CA+Tgmobth=AQkwMWtCsQLAEnV59GT4g3oqPQS45CB+fvG9MxSA@mail.gmail.com
However, it doesn't, quite: a bare VACUUM FULL now bumps relfrozenxid
for every table in the database *except pg_class*. It does call
vac_update_datfrozenxid() afterwards, but that only helps if pg_class
is not among the things holding back datfrozenxid. I haven't dug into
this much yet, but I think it might be worth fixing.
--
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