On Thu, Jul 19, 2018 at 10:54:23AM +0900, Michael Paquier wrote: > If you can get this refactoring done, let's look into getting those two > parts committed first to simplify the next steps. No need to extend the > grammar of cluster either. The set of options for CLUSTER should be a > separate enum list on top of ClusterStmt in parsenodes.h, as VACUUM has > no recheck option. I would recommend to get cluster_rel reshaped first, > and the ereport() calls done after.
The refactoring for CLUSTER is pretty obvious, and makes the API a bit cleaner, so attached is a proposal of patch to do so. Thoughts? -- Michael
From ce4cd47c78f917827d94b13bac99457bc9d17223 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Mon, 23 Jul 2018 12:10:15 +0900
Subject: [PATCH] Refactor ClusterStmt to handle more options
This extends cluster_rel() in such a way that more options can be added
in the future, which will reduce the amount of chunk code for an
upcoming SKIP_LOCKED aimed for VACUUM. As VACUUM FULL is a different
flavor of CLUSTER, we want to make that extensible as well to ease
integration.
This only reworks the API and its callers, without providing anything
user-facing. Two options are present now: verbose mode and relation
recheck when doing the cluster work across multiple transactions.
---
src/backend/commands/cluster.c | 9 ++++++---
src/backend/commands/vacuum.c | 8 ++++++--
src/backend/nodes/copyfuncs.c | 2 +-
src/backend/nodes/equalfuncs.c | 2 +-
src/backend/parser/gram.y | 12 +++++++++---
src/include/commands/cluster.h | 3 +--
src/include/nodes/parsenodes.h | 8 +++++++-
7 files changed, 31 insertions(+), 13 deletions(-)
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 0112a87224..68be470977 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -186,7 +186,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
heap_close(rel, NoLock);
/* Do the job. */
- cluster_rel(tableOid, indexOid, false, stmt->verbose);
+ cluster_rel(tableOid, indexOid, stmt->options);
}
else
{
@@ -234,7 +234,8 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
/* functions in indexes may want a snapshot set */
PushActiveSnapshot(GetTransactionSnapshot());
/* Do the job. */
- cluster_rel(rvtc->tableOid, rvtc->indexOid, true, stmt->verbose);
+ cluster_rel(rvtc->tableOid, rvtc->indexOid,
+ stmt->options | CLUOPT_RECHECK);
PopActiveSnapshot();
CommitTransactionCommand();
}
@@ -265,9 +266,11 @@ 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)
+cluster_rel(Oid tableOid, Oid indexOid, int options)
{
Relation OldHeap;
+ bool verbose = ((options & CLUOPT_VERBOSE) != 0);
+ bool recheck = ((options & CLUOPT_RECHECK) != 0);
/* Check for user-requested abort. */
CHECK_FOR_INTERRUPTS();
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index bd0f04c7e2..5736f12b8f 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -1551,13 +1551,17 @@ vacuum_rel(Oid relid, RangeVar *relation, int options, VacuumParams *params)
*/
if (options & VACOPT_FULL)
{
+ int options = 0;
+
/* close relation before vacuuming, but hold lock until commit */
relation_close(onerel, NoLock);
onerel = NULL;
+ if ((options & VACOPT_VERBOSE) != 0)
+ options |= CLUOPT_VERBOSE;
+
/* VACUUM FULL is now a variant of CLUSTER; see cluster.c */
- cluster_rel(relid, InvalidOid, false,
- (options & VACOPT_VERBOSE) != 0);
+ cluster_rel(relid, InvalidOid, options);
}
else
lazy_vacuum_rel(onerel, options, params, vac_strategy);
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 96836ef19c..17b650b8cb 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -3284,7 +3284,7 @@ _copyClusterStmt(const ClusterStmt *from)
COPY_NODE_FIELD(relation);
COPY_STRING_FIELD(indexname);
- COPY_SCALAR_FIELD(verbose);
+ COPY_SCALAR_FIELD(options);
return newnode;
}
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 6a971d0141..378f2facb8 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -1206,7 +1206,7 @@ _equalClusterStmt(const ClusterStmt *a, const ClusterStmt *b)
{
COMPARE_NODE_FIELD(relation);
COMPARE_STRING_FIELD(indexname);
- COMPARE_SCALAR_FIELD(verbose);
+ COMPARE_SCALAR_FIELD(options);
return true;
}
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 90dfac2cb1..87f5e95827 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -10478,7 +10478,9 @@ ClusterStmt:
ClusterStmt *n = makeNode(ClusterStmt);
n->relation = $3;
n->indexname = $4;
- n->verbose = $2;
+ n->options = 0;
+ if ($2)
+ n->options |= CLUOPT_VERBOSE;
$$ = (Node*)n;
}
| CLUSTER opt_verbose
@@ -10486,7 +10488,9 @@ ClusterStmt:
ClusterStmt *n = makeNode(ClusterStmt);
n->relation = NULL;
n->indexname = NULL;
- n->verbose = $2;
+ n->options = 0;
+ if ($2)
+ n->options |= CLUOPT_VERBOSE;
$$ = (Node*)n;
}
/* kept for pre-8.3 compatibility */
@@ -10495,7 +10499,9 @@ ClusterStmt:
ClusterStmt *n = makeNode(ClusterStmt);
n->relation = $5;
n->indexname = $3;
- n->verbose = $2;
+ n->options = 0;
+ if ($2)
+ n->options |= CLUOPT_VERBOSE;
$$ = (Node*)n;
}
;
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index b338cb1098..f37a60c1c1 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -19,8 +19,7 @@
extern void cluster(ClusterStmt *stmt, bool isTopLevel);
-extern void cluster_rel(Oid tableOid, Oid indexOid, bool recheck,
- bool verbose);
+extern void cluster_rel(Oid tableOid, Oid indexOid, int options);
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);
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 9a5d91a198..000064d642 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -3112,12 +3112,18 @@ typedef struct AlterSystemStmt
* Cluster Statement (support pbrown's cluster index implementation)
* ----------------------
*/
+typedef enum ClusterOption
+{
+ CLUOPT_RECHECK, /* recheck relation state */
+ CLUOPT_VERBOSE /* print progress info */
+} ClusterOption;
+
typedef struct ClusterStmt
{
NodeTag type;
RangeVar *relation; /* relation being indexed, or NULL if all */
char *indexname; /* original index defined */
- bool verbose; /* print progress info */
+ int options; /* OR of ClusterOption flags */
} ClusterStmt;
/* ----------------------
--
2.18.0
signature.asc
Description: PGP signature
