On Mon, Mar 16, 2020 at 04:01:42PM +0900, Amit Langote wrote:
> I came across a commit that recently went in:
>
> commit 1cc9c2412cc9a2fbe6a381170097d315fd40ccca
> Author: Peter Eisentraut <pe...@eisentraut.org>
> Date: Fri Mar 13 11:28:11 2020 +0100
>
> Preserve replica identity index across ALTER TABLE rewrite
>
> which fixes something very similar to what we are trying to with this
> patch. The way it's done looks to me very close to what you are
> telling. I have updated the patch to be similar to the above fix.
Yes, I noticed it too.
Should we use your get_index_isclustered more widely ?
Also, should we call it "is_index_clustered", since otherwise it sounds alot
like "+get_index_clustered" (without "is"), which sounds like it takes a table
and returns which index is clustered. That might be just as useful for some of
these callers.
--
Justin
>From add5e8481c70b6b66342b264a243f26f4c634e53 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlangot...@gmail.com>
Date: Mon, 16 Mar 2020 16:01:42 +0900
Subject: [PATCH v7 1/2] ALTER tbl rewrite loses CLUSTER ON index
On Fri, Mar 13, 2020 at 2:19 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
> Justin Pryzby <pry...@telsasoft.com> writes:
> > @cfbot: resending with only Amit's 0001, since Michael pushed a variation on
> > 0002.
Thank you for taking a look at it.
> Boy, I really dislike this patch. ATPostAlterTypeParse is documented as
> using the supplied definition string, and nothing else, to reconstruct
> the index. This breaks that without even the courtesy of documenting
> the breakage. Moreover, the reason why it's designed like that is to
> avoid requiring the old index objects to still be accessible. So I'm
> surprised that this hack works at all. I don't think it would have
> worked at the time the code was first written, and I think it's imposing
> a constraint we'll have problems with (again?) in future.
Okay, so maybe in the middle of ATPostAlterTypeParse() is not a place
to do it, but don't these arguments apply to
RebuildConstraintComment(), which I based the patch on?
> The right way to fix this is to note the presence of the indisclustered
> flag when we're examining the index earlier, and include a suitable
> command in the definition string. So probably pg_get_indexdef_string()
> is what needs to change. It doesn't look like that's used anywhere
> else, so we can just redefine its behavior as needed.
I came across a commit that recently went in:
commit 1cc9c2412cc9a2fbe6a381170097d315fd40ccca
Author: Peter Eisentraut <pe...@eisentraut.org>
Date: Fri Mar 13 11:28:11 2020 +0100
Preserve replica identity index across ALTER TABLE rewrite
which fixes something very similar to what we are trying to with this
patch. The way it's done looks to me very close to what you are
telling. I have updated the patch to be similar to the above fix.
--
Thank you,
Amit
---
src/backend/commands/tablecmds.c | 49 +++++++++++++++++++++++
src/backend/utils/cache/lsyscache.c | 23 +++++++++++
src/include/utils/lsyscache.h | 1 +
src/test/regress/expected/alter_table.out | 34 ++++++++++++++++
src/test/regress/sql/alter_table.sql | 15 +++++++
5 files changed, 122 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8c33b67c1b..73d9392878 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -177,6 +177,7 @@ typedef struct AlteredTableInfo
List *changedIndexOids; /* OIDs of indexes to rebuild */
List *changedIndexDefs; /* string definitions of same */
char *replicaIdentityIndex; /* index to reset as REPLICA IDENTITY */
+ char *clusterOnIndex; /* index to CLUSTER ON */
} AlteredTableInfo;
/* Struct describing one new constraint to check in Phase 3 scan */
@@ -11579,9 +11580,28 @@ RememberReplicaIdentityForRebuilding(Oid indoid, AlteredTableInfo *tab)
tab->replicaIdentityIndex = get_rel_name(indoid);
}
+/*
+ * Subroutine for ATExecAlterColumnType: remember any clustered index.
+ */
+static void
+RememberClusterOnForRebuilding(Oid indoid, AlteredTableInfo *tab)
+{
+ if (!get_index_isclustered(indoid))
+ return;
+
+ if (tab->clusterOnIndex)
+ elog(ERROR, "relation %u has multiple clustered indexes", tab->relid);
+
+ tab->clusterOnIndex = get_rel_name(indoid);
+}
+
/*
* Subroutine for ATExecAlterColumnType: remember that a constraint needs
* to be rebuilt (which we might already know).
+ *
+ * For constraint's index (if any), also remember if it's the table's replica
+ * identity or its clustered index, so that ATPostAlterTypeCleanup() can
+ * queue up commands necessary to restore that property.
*/
static void
RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab)
@@ -11604,9 +11624,18 @@ RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab)
tab->changedConstraintDefs = lappend(tab->changedConstraintDefs,
defstring);
+ /*
+ * For constraint's index (if any), remember if it's the table's
+ * replica identity or its clustered index, so that
+ * ATPostAlterTypeCleanup() can queue up commands necessary to restore
+ * that property.
+ */
indoid = get_constraint_index(conoid);
if (OidIsValid(indoid))
+ {
RememberReplicaIdentityForRebuilding(indoid, tab);
+ RememberClusterOnForRebuilding(indoid, tab);
+ }
}
}
@@ -11650,7 +11679,13 @@ RememberIndexForRebuilding(Oid indoid, AlteredTableInfo *tab)
tab->changedIndexDefs = lappend(tab->changedIndexDefs,
defstring);
+ /*
+ * Remember if it's the table's replica identity or its clustered
+ * index, so that ATPostAlterTypeCleanup() can queue up commands
+ * necessary to restore that property.
+ */
RememberReplicaIdentityForRebuilding(indoid, tab);
+ RememberClusterOnForRebuilding(indoid, tab);
}
}
}
@@ -11777,6 +11812,20 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
}
+ /*
+ * Queue up command to restore clustered index marking
+ */
+ if (tab->clusterOnIndex)
+ {
+ AlterTableCmd *cmd = makeNode(AlterTableCmd);
+
+ cmd->subtype = AT_ClusterOn;
+ cmd->name = tab->clusterOnIndex;
+
+ /* do it after indexes and constraints */
+ tab->subcmds[AT_PASS_OLD_CONSTR] =
+ lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
+ }
/*
* It should be okay to use DROP_RESTRICT here, since nothing else should
* be depending on these objects.
diff --git a/src/backend/utils/cache/lsyscache.c b/src/backend/utils/cache/lsyscache.c
index 27bbb58f56..9c5b806c22 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -3299,3 +3299,26 @@ get_index_isvalid(Oid index_oid)
return isvalid;
}
+
+/*
+ * get_index_isclustered
+ *
+ * Given the index OID, return pg_index.indisclustered.
+ */
+bool
+get_index_isclustered(Oid index_oid)
+{
+ bool isclustered;
+ HeapTuple tuple;
+ Form_pg_index rd_index;
+
+ tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(index_oid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for index %u", index_oid);
+
+ rd_index = (Form_pg_index) GETSTRUCT(tuple);
+ isclustered = rd_index->indisclustered;
+ ReleaseSysCache(tuple);
+
+ return isclustered;
+}
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index 4e646c55e9..9459c6a94f 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -184,6 +184,7 @@ extern Oid get_range_collation(Oid rangeOid);
extern Oid get_index_column_opclass(Oid index_oid, int attno);
extern bool get_index_isreplident(Oid index_oid);
extern bool get_index_isvalid(Oid index_oid);
+extern bool get_index_isclustered(Oid index_oid);
#define type_is_array(typid) (get_element_type(typid) != InvalidOid)
/* type_is_array_domain accepts both plain arrays and domains over arrays */
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index fb6d86a269..a01c6d6ec5 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4296,3 +4296,37 @@ create trigger xtrig
update bar1 set a = a + 1;
INFO: a=1, b=1
/* End test case for bug #16242 */
+-- alter type rewrite/rebuild should preserve cluster marking on index
+create table alttype_cluster (a int);
+create index alttype_cluster_a on alttype_cluster (a);
+alter table alttype_cluster cluster on alttype_cluster_a;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+ indisclustered
+----------------
+ t
+(1 row)
+
+alter table alttype_cluster alter a type bigint;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+ indisclustered
+----------------
+ t
+(1 row)
+
+drop index alttype_cluster_a;
+alter table alttype_cluster add primary key (a);
+alter table alttype_cluster cluster on alttype_cluster_pkey;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+ indisclustered
+----------------
+ t
+(1 row)
+
+alter table alttype_cluster alter a type int;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+ indisclustered
+----------------
+ t
+(1 row)
+
+drop table alttype_cluster;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 3801f19c58..4eeec24e58 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2840,3 +2840,18 @@ create trigger xtrig
update bar1 set a = a + 1;
/* End test case for bug #16242 */
+
+-- alter type rewrite/rebuild should preserve cluster marking on index
+create table alttype_cluster (a int);
+create index alttype_cluster_a on alttype_cluster (a);
+alter table alttype_cluster cluster on alttype_cluster_a;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+alter table alttype_cluster alter a type bigint;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+drop index alttype_cluster_a;
+alter table alttype_cluster add primary key (a);
+alter table alttype_cluster cluster on alttype_cluster_pkey;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+alter table alttype_cluster alter a type int;
+select indisclustered from pg_index where indrelid = 'alttype_cluster'::regclass;
+drop table alttype_cluster;
--
2.17.0
>From ed12fd247ec382b6e1f248596d12ae0d897c8d17 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryz...@telsasoft.com>
Date: Mon, 16 Mar 2020 08:21:08 -0500
Subject: [PATCH v7 2/2] Use get_index_isclustered in cluster.c
---
src/backend/commands/cluster.c | 41 +++-------------------------------
1 file changed, 3 insertions(+), 38 deletions(-)
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 11ce1bb404..2040023ad6 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -139,21 +139,9 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
/* We need to find the index that has indisclustered set. */
foreach(index, RelationGetIndexList(rel))
{
- HeapTuple idxtuple;
- Form_pg_index indexForm;
-
indexOid = lfirst_oid(index);
- idxtuple = SearchSysCache1(INDEXRELID,
- ObjectIdGetDatum(indexOid));
- if (!HeapTupleIsValid(idxtuple))
- elog(ERROR, "cache lookup failed for index %u", indexOid);
- indexForm = (Form_pg_index) GETSTRUCT(idxtuple);
- if (indexForm->indisclustered)
- {
- ReleaseSysCache(idxtuple);
+ if (get_index_isclustered(indexOid))
break;
- }
- ReleaseSysCache(idxtuple);
indexOid = InvalidOid;
}
@@ -304,9 +292,6 @@ cluster_rel(Oid tableOid, Oid indexOid, int options)
*/
if (recheck)
{
- HeapTuple tuple;
- Form_pg_index indexForm;
-
/* Check that the user still owns the relation */
if (!pg_class_ownercheck(tableOid, GetUserId()))
{
@@ -345,22 +330,12 @@ cluster_rel(Oid tableOid, Oid indexOid, int options)
/*
* Check that the index is still the one with indisclustered set.
*/
- tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
- if (!HeapTupleIsValid(tuple)) /* probably can't happen */
- {
- relation_close(OldHeap, AccessExclusiveLock);
- pgstat_progress_end_command();
- return;
- }
- indexForm = (Form_pg_index) GETSTRUCT(tuple);
- if (!indexForm->indisclustered)
+ if (!get_index_isclustered(indexOid))
{
- ReleaseSysCache(tuple);
relation_close(OldHeap, AccessExclusiveLock);
pgstat_progress_end_command();
return;
}
- ReleaseSysCache(tuple);
}
}
@@ -519,18 +494,8 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
*/
if (OidIsValid(indexOid))
{
- indexTuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
- if (!HeapTupleIsValid(indexTuple))
- elog(ERROR, "cache lookup failed for index %u", indexOid);
- indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
-
- if (indexForm->indisclustered)
- {
- ReleaseSysCache(indexTuple);
+ if (get_index_isclustered(indexOid))
return;
- }
-
- ReleaseSysCache(indexTuple);
}
/*
--
2.17.0
