pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedALTER tbl rewrite loses CLUSTER ON index
33+ messages / 6 participants
[nested] [flat]
* ALTER tbl rewrite loses CLUSTER ON index
@ 2020-02-02 16:17 Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 33+ messages in thread
From: Justin Pryzby @ 2020-02-02 16:17 UTC (permalink / raw)
To: pgsql-hackers
Other options are preserved by ALTER (and CLUSTER ON is and most obviously
should be preserved by CLUSTER's rewrite), so I think (SET) CLUSTER should be
preserved by ALTER, too.
As far as I can see, this should be the responsibility of something in the
vicinity of ATPostAlterTypeParse/RememberIndexForRebuilding.
Attach patch sketches a fix.
ts=# SET client_min_messages=debug; DROP TABLE t; CREATE TABLE t(i int); CREATE INDEX ON t(i)WITH(fillfactor=11, vacuum_cleanup_index_scale_factor=12); CLUSTER t USING t_i_key; ALTER TABLE t ALTER i TYPE bigint; \d t
SET
DEBUG: drop auto-cascades to type t
DEBUG: drop auto-cascades to type t[]
DEBUG: drop auto-cascades to index t_i_idx
DROP TABLE
CREATE TABLE
DEBUG: building index "t_i_idx" on table "t" serially
CREATE INDEX
ERROR: index "t_i_key" for table "t" does not exist
DEBUG: rewriting table "t"
DEBUG: building index "t_i_idx" on table "t" serially
DEBUG: drop auto-cascades to type pg_temp_3091172777
DEBUG: drop auto-cascades to type pg_temp_3091172777[]
ALTER TABLE
Table "public.t"
Column | Type | Collation | Nullable | Default
--------+--------+-----------+----------+---------
i | bigint | | |
Indexes:
"t_i_idx" btree (i) WITH (fillfactor='11', vacuum_cleanup_index_scale_factor='12')
Attachments:
[text/x-diff] v1-0001-preserve-CLUSTER-ON-during-ALTER-TABLE.patch (1.8K, ../../20200202161718.GI13621@telsasoft.com/2-v1-0001-preserve-CLUSTER-ON-during-ALTER-TABLE.patch)
download | inline diff:
From f235a691722a464059358cd6b1d744f75d7bf92f Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 2 Feb 2020 09:49:57 -0600
Subject: [PATCH v1] preserve CLUSTER ON during ALTER TABLE
---
src/backend/commands/tablecmds.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f599393..c4e6cbd 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -11616,6 +11616,7 @@ RememberIndexForRebuilding(Oid indoid, AlteredTableInfo *tab)
}
else
{
+ Relation indrel;
/* OK, capture the index's existing definition string */
char *defstring = pg_get_indexdef_string(indoid);
@@ -11623,6 +11624,18 @@ RememberIndexForRebuilding(Oid indoid, AlteredTableInfo *tab)
indoid);
tab->changedIndexDefs = lappend(tab->changedIndexDefs,
defstring);
+ /* Preserve CLUSTER ON if set */
+ indrel = index_open(indoid, AccessShareLock);
+ if (indrel->rd_index->indisclustered) {
+ char buf[3*NAMEDATALEN + 24];
+ sprintf(buf, "ALTER TABLE %s CLUSTER ON %s",
+ get_rel_name(tab->relid), get_rel_name(indoid)); // XXX: schema
+ tab->changedIndexOids = lappend_oid(tab->changedIndexOids,
+ indoid);
+ tab->changedIndexDefs = lappend(tab->changedIndexDefs,
+ pstrdup(buf));
+ }
+ index_close(indrel, NoLock);
}
}
}
@@ -11901,6 +11914,11 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
* the new table definition.
*/
}
+ else if (cmd->subtype == AT_ClusterOn)
+ {
+ tab->subcmds[AT_PASS_OLD_INDEX] =
+ lappend(tab->subcmds[AT_PASS_OLD_INDEX], cmd);
+ }
else
elog(ERROR, "unexpected statement subtype: %d",
(int) cmd->subtype);
--
2.7.4
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-02-05 06:53 Amit Langote <amitlangote09@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 3 replies; 33+ messages in thread
From: Amit Langote @ 2020-02-05 06:53 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: pgsql-hackers
Hi Justin,
On Mon, Feb 3, 2020 at 1:17 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> Other options are preserved by ALTER (and CLUSTER ON is and most obviously
> should be preserved by CLUSTER's rewrite), so I think (SET) CLUSTER should be
> preserved by ALTER, too.
Yes.
create table foo (a int primary key);
cluster foo;
ERROR: there is no previously clustered index for table "foo"
cluster foo using foo_pkey;
alter table foo alter a type bigint;
cluster foo;
ERROR: there is no previously clustered index for table "foo"
With your patch, this last error doesn't occur.
Like you, I too suspect that losing indisclustered like this is
unintentional, so should be fixed.
> As far as I can see, this should be the responsibility of something in the
> vicinity of ATPostAlterTypeParse/RememberIndexForRebuilding.
>
> Attach patch sketches a fix.
While your sketch hits pretty close, it could be done a bit
differently. For one, I don't like the way it's misusing
changedIndexOids and changedIndexDefs.
Instead, we can do something similar to what
RebuildConstraintComments() does for constraint comments. For
example, we can have a PreserveClusterOn() that adds a AT_ClusterOn
command into table's AT_PASS_OLD_INDEX pass commands. Attached patch
shows what I'm thinking. I also added representative tests.
Thanks,
Amit
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f599393473..ea9941278e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -490,6 +490,7 @@ static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
static void RebuildConstraintComment(AlteredTableInfo *tab, int pass,
Oid objid, Relation rel, List *domname,
const char *conname);
+static void PreserveClusterOn(AlteredTableInfo *tab, int pass, Oid indoid);
static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
static void TryReuseForeignKey(Oid oldId, Constraint *con);
static ObjectAddress ATExecAlterColumnGenericOptions(Relation rel, const char *colName,
@@ -11832,6 +11833,9 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
newcmd->def = (Node *) stmt;
tab->subcmds[AT_PASS_OLD_INDEX] =
lappend(tab->subcmds[AT_PASS_OLD_INDEX], newcmd);
+
+ /* Preserve index's indisclustered property, if set. */
+ PreserveClusterOn(tab, AT_PASS_OLD_INDEX, oldId);
}
else if (IsA(stm, AlterTableStmt))
{
@@ -11868,6 +11872,9 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
rel,
NIL,
indstmt->idxname);
+
+ /* Preserve index's indisclustered property, if set. */
+ PreserveClusterOn(tab, AT_PASS_OLD_INDEX, indoid);
}
else if (cmd->subtype == AT_AddConstraint)
{
@@ -11990,6 +11997,37 @@ RebuildConstraintComment(AlteredTableInfo *tab, int pass, Oid objid,
tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
}
+/*
+ * For an index on table that's being recreated due PostAlterType processing,
+ * preserve its indisclustered property by issuing ALTER TABLE CLUSTER ON on
+ * table to run after the command to re-create the index.
+ */
+static void
+PreserveClusterOn(AlteredTableInfo *tab, int pass, Oid indoid)
+{
+ HeapTuple indexTuple;
+ Form_pg_index indexForm;
+
+ Assert(OidIsValid(indoid));
+ Assert(pass == AT_PASS_OLD_INDEX);
+
+ indexTuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indoid));
+ if (!HeapTupleIsValid(indexTuple))
+ elog(ERROR, "cache lookup failed for index %u", indoid);
+ indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
+
+ if (indexForm->indisclustered)
+ {
+ AlterTableCmd *newcmd = makeNode(AlterTableCmd);
+
+ newcmd->subtype = AT_ClusterOn;
+ newcmd->name = get_rel_name(indoid);
+ tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
+ }
+
+ ReleaseSysCache(indexTuple);
+}
+
/*
* Subroutine for ATPostAlterTypeParse(). Calls out to CheckIndexCompatible()
* for the real analysis, then mutates the IndexStmt based on that verdict.
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index e73ce4b6f3..010a7aa98b 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4258,3 +4258,37 @@ alter table at_test_sql_partop attach partition at_test_sql_partop_1 for values
drop table at_test_sql_partop;
drop operator class at_test_sql_partop using btree;
drop function at_test_sql_partop;
+-- alter type shouldn't lose clustered 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 a16e4c9a29..d619a98de7 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2801,3 +2801,18 @@ alter table at_test_sql_partop attach partition at_test_sql_partop_1 for values
drop table at_test_sql_partop;
drop operator class at_test_sql_partop using btree;
drop function at_test_sql_partop;
+
+-- alter type shouldn't lose clustered 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;
Attachments:
[text/plain] preserve-CLUSTER-ON-during-ALTER-TABLE.patch (5.3K, ../../CA+HiwqEEeehFhti-gV5We5uFeqp+TxTbAMvsUSHiGhj2_U57sQ@mail.gmail.com/2-preserve-CLUSTER-ON-during-ALTER-TABLE.patch)
download | inline diff:
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f599393473..ea9941278e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -490,6 +490,7 @@ static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
static void RebuildConstraintComment(AlteredTableInfo *tab, int pass,
Oid objid, Relation rel, List *domname,
const char *conname);
+static void PreserveClusterOn(AlteredTableInfo *tab, int pass, Oid indoid);
static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
static void TryReuseForeignKey(Oid oldId, Constraint *con);
static ObjectAddress ATExecAlterColumnGenericOptions(Relation rel, const char *colName,
@@ -11832,6 +11833,9 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
newcmd->def = (Node *) stmt;
tab->subcmds[AT_PASS_OLD_INDEX] =
lappend(tab->subcmds[AT_PASS_OLD_INDEX], newcmd);
+
+ /* Preserve index's indisclustered property, if set. */
+ PreserveClusterOn(tab, AT_PASS_OLD_INDEX, oldId);
}
else if (IsA(stm, AlterTableStmt))
{
@@ -11868,6 +11872,9 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
rel,
NIL,
indstmt->idxname);
+
+ /* Preserve index's indisclustered property, if set. */
+ PreserveClusterOn(tab, AT_PASS_OLD_INDEX, indoid);
}
else if (cmd->subtype == AT_AddConstraint)
{
@@ -11990,6 +11997,37 @@ RebuildConstraintComment(AlteredTableInfo *tab, int pass, Oid objid,
tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
}
+/*
+ * For an index on table that's being recreated due PostAlterType processing,
+ * preserve its indisclustered property by issuing ALTER TABLE CLUSTER ON on
+ * table to run after the command to re-create the index.
+ */
+static void
+PreserveClusterOn(AlteredTableInfo *tab, int pass, Oid indoid)
+{
+ HeapTuple indexTuple;
+ Form_pg_index indexForm;
+
+ Assert(OidIsValid(indoid));
+ Assert(pass == AT_PASS_OLD_INDEX);
+
+ indexTuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indoid));
+ if (!HeapTupleIsValid(indexTuple))
+ elog(ERROR, "cache lookup failed for index %u", indoid);
+ indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
+
+ if (indexForm->indisclustered)
+ {
+ AlterTableCmd *newcmd = makeNode(AlterTableCmd);
+
+ newcmd->subtype = AT_ClusterOn;
+ newcmd->name = get_rel_name(indoid);
+ tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
+ }
+
+ ReleaseSysCache(indexTuple);
+}
+
/*
* Subroutine for ATPostAlterTypeParse(). Calls out to CheckIndexCompatible()
* for the real analysis, then mutates the IndexStmt based on that verdict.
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index e73ce4b6f3..010a7aa98b 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4258,3 +4258,37 @@ alter table at_test_sql_partop attach partition at_test_sql_partop_1 for values
drop table at_test_sql_partop;
drop operator class at_test_sql_partop using btree;
drop function at_test_sql_partop;
+-- alter type shouldn't lose clustered 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 a16e4c9a29..d619a98de7 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2801,3 +2801,18 @@ alter table at_test_sql_partop attach partition at_test_sql_partop_1 for values
drop table at_test_sql_partop;
drop operator class at_test_sql_partop using btree;
drop function at_test_sql_partop;
+
+-- alter type shouldn't lose clustered 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;
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-02-05 08:32 Justin Pryzby <pryzby@telsasoft.com>
parent: Amit Langote <amitlangote09@gmail.com>
2 siblings, 0 replies; 33+ messages in thread
From: Justin Pryzby @ 2020-02-05 08:32 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: pgsql-hackers
On Wed, Feb 05, 2020 at 03:53:45PM +0900, Amit Langote wrote:
> Hi Justin,
>
> On Mon, Feb 3, 2020 at 1:17 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > Other options are preserved by ALTER (and CLUSTER ON is and most obviously
> > should be preserved by CLUSTER's rewrite), so I think (SET) CLUSTER should be
> > preserved by ALTER, too.
>
> Yes.
>
> create table foo (a int primary key);
> cluster foo;
> ERROR: there is no previously clustered index for table "foo"
> cluster foo using foo_pkey;
> alter table foo alter a type bigint;
> cluster foo;
> ERROR: there is no previously clustered index for table "foo"
>
> With your patch, this last error doesn't occur.
>
> Like you, I too suspect that losing indisclustered like this is
> unintentional, so should be fixed.
Thanks for checking.
It doesn't need to be said, but your patch is obviously superior.
I ran into this while looking into a suggestion from Alvaro that ALTER should
rewrite in order of a clustered index (if any) rather than in pre-existing heap
order (more on that another day). So while this looks like a bug, and I can't
think how a backpatch would break something, my suggestion is that backpatching
a fix is of low value, so it's only worth +0.
Thanks
Justin
^ permalink raw reply [nested|flat] 33+ messages in thread
* [PATCH v5] ALTER tbl rewrite loses CLUSTER ON index
@ 2020-02-06 09:14 Amit Langote <amitlangote09@gmail.com>
0 siblings, 0 replies; 33+ messages in thread
From: Amit Langote @ 2020-02-06 09:14 UTC (permalink / raw)
---
src/backend/commands/tablecmds.c | 39 +++++++++++++++++++++++
src/test/regress/expected/alter_table.out | 34 ++++++++++++++++++++
src/test/regress/sql/alter_table.sql | 16 +++++++++-
3 files changed, 88 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 02a7c04fdb..6b2469bd09 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -490,6 +490,7 @@ static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
static void RebuildConstraintComment(AlteredTableInfo *tab, int pass,
Oid objid, Relation rel, List *domname,
const char *conname);
+static void PreserveClusterOn(AlteredTableInfo *tab, int pass, Oid indoid);
static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
static void TryReuseForeignKey(Oid oldId, Constraint *con);
static ObjectAddress ATExecAlterColumnGenericOptions(Relation rel, const char *colName,
@@ -11838,6 +11839,9 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
newcmd->def = (Node *) stmt;
tab->subcmds[AT_PASS_OLD_INDEX] =
lappend(tab->subcmds[AT_PASS_OLD_INDEX], newcmd);
+
+ /* Preserve index's indisclustered property, if set. */
+ PreserveClusterOn(tab, AT_PASS_OLD_INDEX, oldId);
}
else if (IsA(stm, AlterTableStmt))
{
@@ -11874,6 +11878,9 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
rel,
NIL,
indstmt->idxname);
+
+ /* Preserve index's indisclustered property, if set. */
+ PreserveClusterOn(tab, AT_PASS_OLD_INDEX, indoid);
}
else if (cmd->subtype == AT_AddConstraint)
{
@@ -11996,6 +12003,38 @@ RebuildConstraintComment(AlteredTableInfo *tab, int pass, Oid objid,
tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
}
+/*
+ * For a table's index that is to be recreated due to PostAlterType
+ * processing, preserve its indisclustered property by issuing ALTER TABLE
+ * CLUSTER ON command on the table that will run after the command to recreate
+ * the index.
+ */
+static void
+PreserveClusterOn(AlteredTableInfo *tab, int pass, Oid indoid)
+{
+ HeapTuple indexTuple;
+ Form_pg_index indexForm;
+
+ Assert(OidIsValid(indoid));
+ Assert(pass == AT_PASS_OLD_INDEX);
+
+ indexTuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indoid));
+ if (!HeapTupleIsValid(indexTuple))
+ elog(ERROR, "cache lookup failed for index %u", indoid);
+ indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
+
+ if (indexForm->indisclustered)
+ {
+ AlterTableCmd *newcmd = makeNode(AlterTableCmd);
+
+ newcmd->subtype = AT_ClusterOn;
+ newcmd->name = get_rel_name(indoid);
+ tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
+ }
+
+ ReleaseSysCache(indexTuple);
+}
+
/*
* Subroutine for ATPostAlterTypeParse(). Calls out to CheckIndexCompatible()
* for the real analysis, then mutates the IndexStmt based on that verdict.
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..6e9048bbec 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2802,7 +2802,6 @@ drop table at_test_sql_partop;
drop operator class at_test_sql_partop using btree;
drop function at_test_sql_partop;
-
/* Test case for bug #16242 */
-- We create a parent and child where the child has missing
@@ -2840,3 +2839,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
--bg08WKrSYDhXBjb5--
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-02-06 09:14 Amit Langote <amitlangote09@gmail.com>
parent: Amit Langote <amitlangote09@gmail.com>
2 siblings, 0 replies; 33+ messages in thread
From: Amit Langote @ 2020-02-06 09:14 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: pgsql-hackers
On Thu, Feb 6, 2020 at 10:31 AM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Feb 6, 2020 at 8:44 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > On Wed, Feb 05, 2020 at 03:53:45PM +0900, Amit Langote wrote:
> > > diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
> > > +-- alter type shouldn't lose clustered index
> >
> > My only suggestion is to update the comment
> > +-- alter type rewrite/rebuild should preserve cluster marking on index
>
> Sure, done.
Deja vu. Last two messages weren't sent to the list; updated patch attached.
Thanks,
Amit
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f599393473..bab3ddf67c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -490,6 +490,7 @@ static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
static void RebuildConstraintComment(AlteredTableInfo *tab, int pass,
Oid objid, Relation rel, List *domname,
const char *conname);
+static void PreserveClusterOn(AlteredTableInfo *tab, int pass, Oid indoid);
static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
static void TryReuseForeignKey(Oid oldId, Constraint *con);
static ObjectAddress ATExecAlterColumnGenericOptions(Relation rel, const char *colName,
@@ -11832,6 +11833,9 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
newcmd->def = (Node *) stmt;
tab->subcmds[AT_PASS_OLD_INDEX] =
lappend(tab->subcmds[AT_PASS_OLD_INDEX], newcmd);
+
+ /* Preserve index's indisclustered property, if set. */
+ PreserveClusterOn(tab, AT_PASS_OLD_INDEX, oldId);
}
else if (IsA(stm, AlterTableStmt))
{
@@ -11868,6 +11872,9 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
rel,
NIL,
indstmt->idxname);
+
+ /* Preserve index's indisclustered property, if set. */
+ PreserveClusterOn(tab, AT_PASS_OLD_INDEX, indoid);
}
else if (cmd->subtype == AT_AddConstraint)
{
@@ -11990,6 +11997,38 @@ RebuildConstraintComment(AlteredTableInfo *tab, int pass, Oid objid,
tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
}
+/*
+ * For a table's index that is to be recreated due to PostAlterType
+ * processing, preserve its indisclustered property by issuing ALTER TABLE
+ * CLUSTER ON command on the table that will run after the command to recreate
+ * the index.
+ */
+static void
+PreserveClusterOn(AlteredTableInfo *tab, int pass, Oid indoid)
+{
+ HeapTuple indexTuple;
+ Form_pg_index indexForm;
+
+ Assert(OidIsValid(indoid));
+ Assert(pass == AT_PASS_OLD_INDEX);
+
+ indexTuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indoid));
+ if (!HeapTupleIsValid(indexTuple))
+ elog(ERROR, "cache lookup failed for index %u", indoid);
+ indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
+
+ if (indexForm->indisclustered)
+ {
+ AlterTableCmd *newcmd = makeNode(AlterTableCmd);
+
+ newcmd->subtype = AT_ClusterOn;
+ newcmd->name = get_rel_name(indoid);
+ tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
+ }
+
+ ReleaseSysCache(indexTuple);
+}
+
/*
* Subroutine for ATPostAlterTypeParse(). Calls out to CheckIndexCompatible()
* for the real analysis, then mutates the IndexStmt based on that verdict.
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index e73ce4b6f3..09c00eef05 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4258,3 +4258,37 @@ alter table at_test_sql_partop attach partition at_test_sql_partop_1 for values
drop table at_test_sql_partop;
drop operator class at_test_sql_partop using btree;
drop function at_test_sql_partop;
+-- 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 a16e4c9a29..7e43965a86 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2801,3 +2801,18 @@ alter table at_test_sql_partop attach partition at_test_sql_partop_1 for values
drop table at_test_sql_partop;
drop operator class at_test_sql_partop using btree;
drop function at_test_sql_partop;
+
+-- 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;
Attachments:
[text/plain] preserve-CLUSTER-ON-during-ALTER-TABLE_v3.patch (5.3K, ../../CA+HiwqEt1HnXYckCdaO8+pOoFs7NNS5byoZ6Xg2B7epKbhS85w@mail.gmail.com/2-preserve-CLUSTER-ON-during-ALTER-TABLE_v3.patch)
download | inline diff:
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f599393473..bab3ddf67c 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -490,6 +490,7 @@ static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
static void RebuildConstraintComment(AlteredTableInfo *tab, int pass,
Oid objid, Relation rel, List *domname,
const char *conname);
+static void PreserveClusterOn(AlteredTableInfo *tab, int pass, Oid indoid);
static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
static void TryReuseForeignKey(Oid oldId, Constraint *con);
static ObjectAddress ATExecAlterColumnGenericOptions(Relation rel, const char *colName,
@@ -11832,6 +11833,9 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
newcmd->def = (Node *) stmt;
tab->subcmds[AT_PASS_OLD_INDEX] =
lappend(tab->subcmds[AT_PASS_OLD_INDEX], newcmd);
+
+ /* Preserve index's indisclustered property, if set. */
+ PreserveClusterOn(tab, AT_PASS_OLD_INDEX, oldId);
}
else if (IsA(stm, AlterTableStmt))
{
@@ -11868,6 +11872,9 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
rel,
NIL,
indstmt->idxname);
+
+ /* Preserve index's indisclustered property, if set. */
+ PreserveClusterOn(tab, AT_PASS_OLD_INDEX, indoid);
}
else if (cmd->subtype == AT_AddConstraint)
{
@@ -11990,6 +11997,38 @@ RebuildConstraintComment(AlteredTableInfo *tab, int pass, Oid objid,
tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
}
+/*
+ * For a table's index that is to be recreated due to PostAlterType
+ * processing, preserve its indisclustered property by issuing ALTER TABLE
+ * CLUSTER ON command on the table that will run after the command to recreate
+ * the index.
+ */
+static void
+PreserveClusterOn(AlteredTableInfo *tab, int pass, Oid indoid)
+{
+ HeapTuple indexTuple;
+ Form_pg_index indexForm;
+
+ Assert(OidIsValid(indoid));
+ Assert(pass == AT_PASS_OLD_INDEX);
+
+ indexTuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indoid));
+ if (!HeapTupleIsValid(indexTuple))
+ elog(ERROR, "cache lookup failed for index %u", indoid);
+ indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
+
+ if (indexForm->indisclustered)
+ {
+ AlterTableCmd *newcmd = makeNode(AlterTableCmd);
+
+ newcmd->subtype = AT_ClusterOn;
+ newcmd->name = get_rel_name(indoid);
+ tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
+ }
+
+ ReleaseSysCache(indexTuple);
+}
+
/*
* Subroutine for ATPostAlterTypeParse(). Calls out to CheckIndexCompatible()
* for the real analysis, then mutates the IndexStmt based on that verdict.
diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index e73ce4b6f3..09c00eef05 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4258,3 +4258,37 @@ alter table at_test_sql_partop attach partition at_test_sql_partop_1 for values
drop table at_test_sql_partop;
drop operator class at_test_sql_partop using btree;
drop function at_test_sql_partop;
+-- 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 a16e4c9a29..7e43965a86 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2801,3 +2801,18 @@ alter table at_test_sql_partop attach partition at_test_sql_partop_1 for values
drop table at_test_sql_partop;
drop operator class at_test_sql_partop using btree;
drop function at_test_sql_partop;
+
+-- 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;
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-02-06 14:44 Justin Pryzby <pryzby@telsasoft.com>
parent: Amit Langote <amitlangote09@gmail.com>
2 siblings, 1 reply; 33+ messages in thread
From: Justin Pryzby @ 2020-02-06 14:44 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: pgsql-hackers
I wondered if it wouldn't be better if CLUSTER ON was stored in pg_class as the
Oid of a clustered index, rather than a boolean in pg_index.
That likely would've avoided (or at least exposed) this issue.
And avoids the possibility of having two indices marked as "clustered".
These would be more trivial:
mark_index_clustered
/* We need to find the index that has indisclustered set. */
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-02-06 17:24 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 2 replies; 33+ messages in thread
From: Alvaro Herrera @ 2020-02-06 17:24 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Amit Langote <amitlangote09@gmail.com>; pgsql-hackers
On 2020-Feb-06, Justin Pryzby wrote:
> I wondered if it wouldn't be better if CLUSTER ON was stored in pg_class as the
> Oid of a clustered index, rather than a boolean in pg_index.
Maybe. Do you want to try a patch?
> That likely would've avoided (or at least exposed) this issue.
> And avoids the possibility of having two indices marked as "clustered".
> These would be more trivial:
> mark_index_clustered
> /* We need to find the index that has indisclustered set. */
You need to be careful when dropping the index ...
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-02-07 08:42 Amit Langote <amitlangote09@gmail.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 0 replies; 33+ messages in thread
From: Amit Langote @ 2020-02-07 08:42 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; pgsql-hackers
On Fri, Feb 7, 2020 at 2:24 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> On 2020-Feb-06, Justin Pryzby wrote:
> > I wondered if it wouldn't be better if CLUSTER ON was stored in pg_class as the
> > Oid of a clustered index, rather than a boolean in pg_index.
>
> Maybe. Do you want to try a patch?
+1
Thanksm
Amit
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index (consider moving indisclustered to pg_class)
@ 2020-02-07 14:39 Justin Pryzby <pryzby@telsasoft.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
1 sibling, 2 replies; 33+ messages in thread
From: Justin Pryzby @ 2020-02-07 14:39 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Amit Langote <amitlangote09@gmail.com>; pgsql-hackers
On Thu, Feb 06, 2020 at 02:24:47PM -0300, Alvaro Herrera wrote:
> On 2020-Feb-06, Justin Pryzby wrote:
>
> > I wondered if it wouldn't be better if CLUSTER ON was stored in pg_class as the
> > Oid of a clustered index, rather than a boolean in pg_index.
>
> Maybe. Do you want to try a patch?
I think the attached is 80% complete (I didn't touch pg_dump).
One objection to this change would be that all relations (including indices)
end up with relclustered fields, and pg_index already has a number of bools, so
it's not like this one bool is wasting a byte.
I think relisclustered was a's clever way of avoiding that overhead (c0ad5953).
So I would be -0.5 on moving it to pg_class..
But I think 0001 and 0002 are worthy. Maybe the test in 0002 should live
somewhere else.
Attachments:
[text/x-diff] v1-0001-Update-comment-obsolete-since-b9b8831a.patch (1.1K, ../../20200207143935.GP403@telsasoft.com/2-v1-0001-Update-comment-obsolete-since-b9b8831a.patch)
download | inline diff:
From 7eea0a17e495fe13379ffd589b551f2f145f5672 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Thu, 6 Feb 2020 21:48:13 -0600
Subject: [PATCH v1 1/3] Update comment obsolete since b9b8831a
---
src/backend/commands/cluster.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index e9d7a7f..3adcbeb 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -1539,9 +1539,9 @@ get_tables_to_cluster(MemoryContext cluster_context)
/*
* Get all indexes that have indisclustered set and are owned by
- * appropriate user. System relations or nailed-in relations cannot ever
- * have indisclustered set, because CLUSTER will refuse to set it when
- * called with one of them as argument.
+ * appropriate user. Shared relations cannot ever have indisclustered
+ * set, because CLUSTER will refuse to set it when called with one as
+ * an argument.
*/
indRelation = table_open(IndexRelationId, AccessShareLock);
ScanKeyInit(&entry,
--
2.7.4
[text/x-diff] v1-0002-Give-developer-a-helpful-kick-in-the-pants-if-the.patch (1.9K, ../../20200207143935.GP403@telsasoft.com/3-v1-0002-Give-developer-a-helpful-kick-in-the-pants-if-the.patch)
download | inline diff:
From 4777be522a7aa8b8c77b13f765cbd02043438f2a Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Fri, 7 Feb 2020 08:12:50 -0600
Subject: [PATCH v1 2/3] Give developer a helpful kick in the pants if they
change natts in one place but not another
---
src/backend/bootstrap/bootstrap.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index bfc629c..d5e1888 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -25,7 +25,9 @@
#include "access/xlog_internal.h"
#include "bootstrap/bootstrap.h"
#include "catalog/index.h"
+#include "catalog/pg_class.h"
#include "catalog/pg_collation.h"
+#include "catalog/pg_proc.h"
#include "catalog/pg_type.h"
#include "common/link-canary.h"
#include "libpq/pqsignal.h"
@@ -49,6 +51,7 @@
#include "utils/ps_status.h"
#include "utils/rel.h"
#include "utils/relmapper.h"
+#include "utils/syscache.h"
uint32 bootstrap_data_checksum_version = 0; /* No checksum */
@@ -602,6 +605,26 @@ boot_openrel(char *relname)
TableScanDesc scan;
HeapTuple tup;
+ /* Check that pg_class data is consistent now, rather than failing obscurely later */
+ struct { Oid oid; int natts; }
+ checknatts[] = {
+ {RelationRelationId, Natts_pg_class,},
+ {TypeRelationId, Natts_pg_type,},
+ {AttributeRelationId, Natts_pg_attribute,},
+ {ProcedureRelationId, Natts_pg_proc,},
+ };
+
+ for (int i=0; i<lengthof(checknatts); ++i) {
+ Form_pg_class classForm;
+ HeapTuple tuple;
+ tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(checknatts[i].oid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", checknatts[i].oid);
+ classForm = (Form_pg_class) GETSTRUCT(tuple);
+ Assert(checknatts[i].natts == classForm->relnatts);
+ ReleaseSysCache(tuple);
+ }
+
if (strlen(relname) >= NAMEDATALEN)
relname[NAMEDATALEN - 1] = '\0';
--
2.7.4
[text/x-diff] v1-0003-Make-cluster-a-property-of-table-in-pg_index.patch (18.8K, ../../20200207143935.GP403@telsasoft.com/4-v1-0003-Make-cluster-a-property-of-table-in-pg_index.patch)
download | inline diff:
From ed886f8202486dea8069b719d35a5d0db7f3277c Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Thu, 6 Feb 2020 12:56:34 -0600
Subject: [PATCH v1 3/3] Make cluster a property of table in pg_index..
..rather than of indexes in pg_index.
The only issue with this is that it makes pg_class larger, and the new column
applies not only to tables, but to indices.
---
doc/src/sgml/catalogs.sgml | 14 +--
src/backend/catalog/heap.c | 1 +
src/backend/catalog/index.c | 6 --
src/backend/commands/cluster.c | 172 +++++++++++++++++--------------------
src/backend/commands/tablecmds.c | 5 +-
src/backend/utils/cache/relcache.c | 1 -
src/bin/psql/describe.c | 4 +-
src/include/catalog/pg_class.dat | 2 +-
src/include/catalog/pg_class.h | 3 +
src/include/catalog/pg_index.h | 1 -
10 files changed, 93 insertions(+), 116 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index a10b665..8efeaff 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1752,6 +1752,13 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
</row>
<row>
+ <entry><structfield>relclustered</structfield></entry>
+ <entry><type>oid</type></entry>
+ <entry></entry>
+ <entry>The OID of the index last clustered, or zero</entry>
+ </row>
+
+ <row>
<entry><structfield>relpages</structfield></entry>
<entry><type>int4</type></entry>
<entry></entry>
@@ -3808,13 +3815,6 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
</row>
<row>
- <entry><structfield>indisclustered</structfield></entry>
- <entry><type>bool</type></entry>
- <entry></entry>
- <entry>If true, the table was last clustered on this index</entry>
- </row>
-
- <row>
<entry><structfield>indisvalid</structfield></entry>
<entry><type>bool</type></entry>
<entry></entry>
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 046b3d3..5769d15 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -901,6 +901,7 @@ InsertPgClassTuple(Relation pg_class_desc,
values[Anum_pg_class_relam - 1] = ObjectIdGetDatum(rd_rel->relam);
values[Anum_pg_class_relfilenode - 1] = ObjectIdGetDatum(rd_rel->relfilenode);
values[Anum_pg_class_reltablespace - 1] = ObjectIdGetDatum(rd_rel->reltablespace);
+ values[Anum_pg_class_relclustered - 1] = ObjectIdGetDatum(rd_rel->relclustered);
values[Anum_pg_class_relpages - 1] = Int32GetDatum(rd_rel->relpages);
values[Anum_pg_class_reltuples - 1] = Float4GetDatum(rd_rel->reltuples);
values[Anum_pg_class_relallvisible - 1] = Int32GetDatum(rd_rel->relallvisible);
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 8880586..34ba896 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -607,7 +607,6 @@ UpdateIndexRelation(Oid indexoid,
values[Anum_pg_index_indisprimary - 1] = BoolGetDatum(primary);
values[Anum_pg_index_indisexclusion - 1] = BoolGetDatum(isexclusion);
values[Anum_pg_index_indimmediate - 1] = BoolGetDatum(immediate);
- values[Anum_pg_index_indisclustered - 1] = BoolGetDatum(false);
values[Anum_pg_index_indisvalid - 1] = BoolGetDatum(isvalid);
values[Anum_pg_index_indcheckxmin - 1] = BoolGetDatum(false);
values[Anum_pg_index_indisready - 1] = BoolGetDatum(isready);
@@ -1530,7 +1529,6 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
/* Mark old index as valid and new as invalid as index_set_state_flags */
newIndexForm->indisvalid = true;
oldIndexForm->indisvalid = false;
- oldIndexForm->indisclustered = false;
CatalogTupleUpdate(pg_index, &oldIndexTuple->t_self, oldIndexTuple);
CatalogTupleUpdate(pg_index, &newIndexTuple->t_self, newIndexTuple);
@@ -3351,12 +3349,8 @@ index_set_state_flags(Oid indexId, IndexStateFlagsAction action)
* indisvalid is false. (We don't assert that either is initially
* true, though, since we want to be able to retry a DROP INDEX
* CONCURRENTLY that failed partway through.)
- *
- * Note: the CLUSTER logic assumes that indisclustered cannot be
- * set on any invalid index, so clear that flag too.
*/
indexForm->indisvalid = false;
- indexForm->indisclustered = false;
break;
case INDEX_DROP_SET_DEAD:
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 3adcbeb..187e117 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -94,8 +94,8 @@ static List *get_tables_to_cluster(MemoryContext cluster_context);
* The single-relation case does not have any such overhead.
*
* We also allow a relation to be specified without index. In that case,
- * the indisclustered bit will be looked up, and an ERROR will be thrown
- * if there is no index with the bit set.
+ * the clustered index will be looked up, and an ERROR will be thrown
+ * if no index is clustered.
*---------------------------------------------------------------------------
*/
void
@@ -134,28 +134,17 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
if (stmt->indexname == NULL)
{
- ListCell *index;
-
- /* 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);
- break;
- }
- ReleaseSysCache(idxtuple);
- indexOid = InvalidOid;
- }
+ /* Lookup the clustered index */
+ Form_pg_class classForm;
+ HeapTuple tuple;
+
+ tuple = SearchSysCache1(RELOID,
+ ObjectIdGetDatum(tableOid));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", tableOid);
+ classForm = (Form_pg_class) GETSTRUCT(tuple);
+ indexOid = classForm->relclustered;
+ ReleaseSysCache(tuple);
if (!OidIsValid(indexOid))
ereport(ERROR,
@@ -188,7 +177,7 @@ cluster(ClusterStmt *stmt, bool isTopLevel)
{
/*
* This is the "multi relation" case. We need to cluster all tables
- * that have some index with indisclustered set.
+ * that have cluster_index set.
*/
MemoryContext cluster_context;
List *rvs;
@@ -304,8 +293,8 @@ cluster_rel(Oid tableOid, Oid indexOid, int options)
*/
if (recheck)
{
- HeapTuple tuple;
- Form_pg_index indexForm;
+ HeapTuple tuple;
+ Form_pg_class classForm;
/* Check that the user still owns the relation */
if (!pg_class_ownercheck(tableOid, GetUserId()))
@@ -343,17 +332,17 @@ cluster_rel(Oid tableOid, Oid indexOid, int options)
}
/*
- * Check that the index is still the one with indisclustered set.
+ * Check that the index is still the one clustered.
*/
- tuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indexOid));
+ tuple = SearchSysCache1(RELOID, 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)
+ classForm = (Form_pg_class) GETSTRUCT(tuple);
+ if (classForm->relclustered != indexOid)
{
ReleaseSysCache(tuple);
relation_close(OldHeap, AccessExclusiveLock);
@@ -366,8 +355,8 @@ cluster_rel(Oid tableOid, Oid indexOid, int options)
/*
* We allow VACUUM FULL, but not CLUSTER, on shared catalogs. CLUSTER
- * would work in most respects, but the index would only get marked as
- * indisclustered in the current database, leading to unexpected behavior
+ * would work in most respects, but the table would only be marked as
+ * clustered in the current database, leading to unexpected behavior
* if CLUSTER were later invoked in another database.
*/
if (OidIsValid(indexOid) && OldHeap->rd_rel->relisshared)
@@ -498,15 +487,14 @@ check_index_is_clusterable(Relation OldHeap, Oid indexOid, bool recheck, LOCKMOD
/*
* mark_index_clustered: mark the specified index as the one clustered on
*
- * With indexOid == InvalidOid, will mark all indexes of rel not-clustered.
+ * With indexOid == InvalidOid, will mark as not-clustered.
*/
void
mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
{
- HeapTuple indexTuple;
- Form_pg_index indexForm;
- Relation pg_index;
- ListCell *index;
+ HeapTuple tuple;
+ Form_pg_class classForm;
+ Relation pg_class;
/* Disallow applying to a partitioned table */
if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
@@ -516,63 +504,45 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
/*
* If the index is already marked clustered, no need to do anything.
+ * XXX: just remove this?
*/
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);
+ tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(rel->rd_id));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for relation %u", rel->rd_id);
+ classForm = (Form_pg_class) GETSTRUCT(tuple);
- if (indexForm->indisclustered)
+ if (classForm->relclustered == indexOid)
{
- ReleaseSysCache(indexTuple);
+ ReleaseSysCache(tuple);
return;
}
- ReleaseSysCache(indexTuple);
+ ReleaseSysCache(tuple);
}
/*
- * Check each index of the relation and set/clear the bit as needed.
+ * Need to set the clustered index.
*/
- pg_index = table_open(IndexRelationId, RowExclusiveLock);
-
- foreach(index, RelationGetIndexList(rel))
- {
- Oid thisIndexOid = lfirst_oid(index);
+ pg_class = table_open(RelationRelationId, RowExclusiveLock);
- indexTuple = SearchSysCacheCopy1(INDEXRELID,
- ObjectIdGetDatum(thisIndexOid));
- if (!HeapTupleIsValid(indexTuple))
- elog(ERROR, "cache lookup failed for index %u", thisIndexOid);
- indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
+ tuple = SearchSysCacheCopy1(RELOID,
+ ObjectIdGetDatum(rel->rd_id));
+ if (!HeapTupleIsValid(tuple))
+ elog(ERROR, "cache lookup failed for index %u", indexOid);
+ classForm = (Form_pg_class) GETSTRUCT(tuple);
- /*
- * Unset the bit if set. We know it's wrong because we checked this
- * earlier.
- */
- if (indexForm->indisclustered)
- {
- indexForm->indisclustered = false;
- CatalogTupleUpdate(pg_index, &indexTuple->t_self, indexTuple);
- }
- else if (thisIndexOid == indexOid)
- {
- /* this was checked earlier, but let's be real sure */
- if (!indexForm->indisvalid)
- elog(ERROR, "cannot cluster on invalid index %u", indexOid);
- indexForm->indisclustered = true;
- CatalogTupleUpdate(pg_index, &indexTuple->t_self, indexTuple);
- }
+ classForm->relclustered = indexOid;
+ CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
- InvokeObjectPostAlterHookArg(IndexRelationId, thisIndexOid, 0,
- InvalidOid, is_internal);
+ InvokeObjectPostAlterHookArg(RelationRelationId, rel->rd_id, 0,
+ InvalidOid, is_internal);
- heap_freetuple(indexTuple);
- }
+ heap_freetuple(tuple);
+ // ReleaseSysCache(tuple);
- table_close(pg_index, RowExclusiveLock);
+ table_close(pg_class, RowExclusiveLock);
}
/*
@@ -1521,41 +1491,53 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
/*
* Get a list of tables that the current user owns and
- * have indisclustered set. Return the list in a List * of RelToCluster
+ * have a relclustered. Return the list in a List * of RelToCluster
* with the tableOid and the indexOid on which the table is already
* clustered.
*/
static List *
get_tables_to_cluster(MemoryContext cluster_context)
{
- Relation indRelation;
+ Relation clsRelation;
TableScanDesc scan;
ScanKeyData entry;
- HeapTuple indexTuple;
- Form_pg_index index;
+ HeapTuple tuple;
MemoryContext old_context;
RelToCluster *rvtc;
List *rvs = NIL;
/*
- * Get all indexes that have indisclustered set and are owned by
- * appropriate user. Shared relations cannot ever have indisclustered
+ * Get all tables with clustered indexes and whcih are owned by
+ * appropriate user. Shared relations cannot ever have indisclustered
* set, because CLUSTER will refuse to set it when called with one as
* an argument.
*/
- indRelation = table_open(IndexRelationId, AccessShareLock);
+ clsRelation = table_open(RelationRelationId, AccessShareLock);
ScanKeyInit(&entry,
- Anum_pg_index_indisclustered,
- BTEqualStrategyNumber, F_BOOLEQ,
- BoolGetDatum(true));
- scan = table_beginscan_catalog(indRelation, 1, &entry);
- while ((indexTuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
+ Anum_pg_class_relclustered,
+ BTGreaterStrategyNumber, F_OIDEQ,
+ (Datum) 0);
+
+ scan = table_beginscan_catalog(clsRelation, 1, &entry);
+ while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
{
- index = (Form_pg_index) GETSTRUCT(indexTuple);
+ Form_pg_class classForm;
+ HeapTuple indtuple;
+ classForm = (Form_pg_class) GETSTRUCT(tuple);
+
+ if (!pg_class_ownercheck(classForm->oid, GetUserId()))
+ continue;
- if (!pg_class_ownercheck(index->indrelid, GetUserId()))
+ if (!OidIsValid(classForm->relclustered)) /* Shouldn't happen */
continue;
+ indtuple = SearchSysCache1(RELOID, ObjectIdGetDatum(classForm->relclustered)); // XXX: check that it's an index, and on that relation?
+ if (!HeapTupleIsValid(indtuple)) {
+ ReleaseSysCache(indtuple);
+ continue;
+ }
+ ReleaseSysCache(indtuple);
+
/*
* We have to build the list in a different memory context so it will
* survive the cross-transaction processing
@@ -1563,15 +1545,15 @@ get_tables_to_cluster(MemoryContext cluster_context)
old_context = MemoryContextSwitchTo(cluster_context);
rvtc = (RelToCluster *) palloc(sizeof(RelToCluster));
- rvtc->tableOid = index->indrelid;
- rvtc->indexOid = index->indexrelid;
+ rvtc->tableOid = classForm->oid;
+ rvtc->indexOid = classForm->relclustered;
rvs = lappend(rvs, rvtc);
MemoryContextSwitchTo(old_context);
}
table_endscan(scan);
- relation_close(indRelation, AccessShareLock);
+ relation_close(clsRelation, AccessShareLock);
return rvs;
}
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index f599393..ca2c1f1 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -12547,7 +12547,7 @@ change_owner_recurse_to_sequences(Oid relationOid, Oid newOwnerId, LOCKMODE lock
/*
* ALTER TABLE CLUSTER ON
*
- * The only thing we have to do is to change the indisclustered bits.
+ * The only thing we have to do is to change relclustered.
*
* Return the address of the new clustering index.
*/
@@ -12580,8 +12580,7 @@ ATExecClusterOn(Relation rel, const char *indexName, LOCKMODE lockmode)
/*
* ALTER TABLE SET WITHOUT CLUSTER
*
- * We have to find any indexes on the table that have indisclustered bit
- * set and turn it off.
+ * We have to unset relclustered.
*/
static void
ATExecDropCluster(Relation rel, LOCKMODE lockmode)
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index df025a5..9dae783 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -2206,7 +2206,6 @@ RelationReloadIndexInfo(Relation relation)
relation->rd_index->indisprimary = index->indisprimary;
relation->rd_index->indisexclusion = index->indisexclusion;
relation->rd_index->indimmediate = index->indimmediate;
- relation->rd_index->indisclustered = index->indisclustered;
relation->rd_index->indisvalid = index->indisvalid;
relation->rd_index->indcheckxmin = index->indcheckxmin;
relation->rd_index->indisready = index->indisready;
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index f3c7eb9..71ec575 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -2205,7 +2205,7 @@ describeOneTableDetails(const char *schemaname,
PGresult *result;
printfPQExpBuffer(&buf,
- "SELECT i.indisunique, i.indisprimary, i.indisclustered, ");
+ "SELECT i.indisunique, i.indisprimary, c.relclustered=i.indexrelid,");
if (pset.sversion >= 80200)
appendPQExpBufferStr(&buf, "i.indisvalid,\n");
else
@@ -2319,7 +2319,7 @@ describeOneTableDetails(const char *schemaname,
if (tableinfo.hasindex)
{
printfPQExpBuffer(&buf,
- "SELECT c2.relname, i.indisprimary, i.indisunique, i.indisclustered, ");
+ "SELECT c2.relname, i.indisprimary, i.indisunique, c.relclustered=i.indexrelid,");
if (pset.sversion >= 80200)
appendPQExpBufferStr(&buf, "i.indisvalid, ");
else
diff --git a/src/include/catalog/pg_class.dat b/src/include/catalog/pg_class.dat
index f70d5ba..0ea1b10 100644
--- a/src/include/catalog/pg_class.dat
+++ b/src/include/catalog/pg_class.dat
@@ -54,7 +54,7 @@
relname => 'pg_class', reltype => 'pg_class', relam => 'heap',
relfilenode => '0', relpages => '0', reltuples => '0', relallvisible => '0',
reltoastrelid => '0', relhasindex => 'f', relisshared => 'f',
- relpersistence => 'p', relkind => 'r', relnatts => '33', relchecks => '0',
+ relpersistence => 'p', relkind => 'r', relnatts => '34', relchecks => '0',
relhasrules => 'f', relhastriggers => 'f', relhassubclass => 'f',
relrowsecurity => 'f', relforcerowsecurity => 'f', relispopulated => 't',
relreplident => 'n', relispartition => 'f', relfrozenxid => '3',
diff --git a/src/include/catalog/pg_class.h b/src/include/catalog/pg_class.h
index a12fc1f..228e714 100644
--- a/src/include/catalog/pg_class.h
+++ b/src/include/catalog/pg_class.h
@@ -119,6 +119,9 @@ CATALOG(pg_class,1259,RelationRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(83,Relat
/* heap for rewrite during DDL, link to original rel */
Oid relrewrite BKI_DEFAULT(0);
+ /* identifier of index last clustered by: XXX PG_INDEX XXX */
+ Oid relclustered BKI_DEFAULT(0);
+
/* all Xids < this are frozen in this rel */
TransactionId relfrozenxid;
diff --git a/src/include/catalog/pg_index.h b/src/include/catalog/pg_index.h
index d3d7ea7..e39e23d 100644
--- a/src/include/catalog/pg_index.h
+++ b/src/include/catalog/pg_index.h
@@ -36,7 +36,6 @@ CATALOG(pg_index,2610,IndexRelationId) BKI_SCHEMA_MACRO
bool indisprimary; /* is this index for primary key? */
bool indisexclusion; /* is this index for exclusion constraint? */
bool indimmediate; /* is uniqueness enforced immediately? */
- bool indisclustered; /* is this the index last clustered by? */
bool indisvalid; /* is this index valid for use by queries? */
bool indcheckxmin; /* must we wait for xmin to be old? */
bool indisready; /* is this index ready for inserts? */
--
2.7.4
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index (consider moving indisclustered to pg_class)
@ 2020-02-17 05:31 Amit Langote <amitlangote09@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 1 reply; 33+ messages in thread
From: Amit Langote @ 2020-02-17 05:31 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-hackers
Hi Justin,
On Fri, Feb 7, 2020 at 11:39 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> On Thu, Feb 06, 2020 at 02:24:47PM -0300, Alvaro Herrera wrote:
> > On 2020-Feb-06, Justin Pryzby wrote:
> >
> > > I wondered if it wouldn't be better if CLUSTER ON was stored in pg_class as the
> > > Oid of a clustered index, rather than a boolean in pg_index.
> >
> > Maybe. Do you want to try a patch?
>
> I think the attached is 80% complete (I didn't touch pg_dump).
>
> One objection to this change would be that all relations (including indices)
> end up with relclustered fields, and pg_index already has a number of bools, so
> it's not like this one bool is wasting a byte.
>
> I think relisclustered was a's clever way of avoiding that overhead (c0ad5953).
> So I would be -0.5 on moving it to pg_class..
Are you still for fixing ALTER TABLE losing relisclustered with the
patch we were working on earlier [1], if not for moving relisclustered
to pg_class anymore?
I have read elsewhere [2] that forcing ALTER TABLE to rewrite in
clustered order might not be a good option, but maybe that one is a
more radical proposal than this.
Thanks,
Amit
[1] https://postgr.es/m/CA%2BHiwqEt1HnXYckCdaO8%2BpOoFs7NNS5byoZ6Xg2B7epKbhS85w%40mail.gmail.com
[2] https://postgr.es/m/10984.1581181029%40sss.pgh.pa.us
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index (consider moving indisclustered to pg_class)
@ 2020-02-17 05:49 Justin Pryzby <pryzby@telsasoft.com>
parent: Amit Langote <amitlangote09@gmail.com>
0 siblings, 0 replies; 33+ messages in thread
From: Justin Pryzby @ 2020-02-17 05:49 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-hackers
On Mon, Feb 17, 2020 at 02:31:42PM +0900, Amit Langote wrote:
> Hi Justin,
>
> On Fri, Feb 7, 2020 at 11:39 PM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > On Thu, Feb 06, 2020 at 02:24:47PM -0300, Alvaro Herrera wrote:
> > > On 2020-Feb-06, Justin Pryzby wrote:
> > >
> > > > I wondered if it wouldn't be better if CLUSTER ON was stored in pg_class as the
> > > > Oid of a clustered index, rather than a boolean in pg_index.
> > >
> > > Maybe. Do you want to try a patch?
> >
> > I think the attached is 80% complete (I didn't touch pg_dump).
> >
> > One objection to this change would be that all relations (including indices)
> > end up with relclustered fields, and pg_index already has a number of bools, so
> > it's not like this one bool is wasting a byte.
> >
> > I think relisclustered was a's clever way of avoiding that overhead (c0ad5953).
> > So I would be -0.5 on moving it to pg_class..
In case there's any confusion: "a's" was probably me halfway changing
"someone's" to "a".
> Are you still for fixing ALTER TABLE losing relisclustered with the
> patch we were working on earlier [1], if not for moving relisclustered
> to pg_class anymore?
Thanks for remembering this one.
I think your patch is the correct fix.
I forgot to say it, but moving relisclustered to pg_class doesn't help to avoid
losting indisclustered: it still needs a fix just like this. Anyway, I
withdrew my suggestion for moving to pg_class, since it has more overhead, even
for pg_class rows for relations which can't have indexes.
> I have read elsewhere [2] that forcing ALTER TABLE to rewrite in
> clustered order might not be a good option, but maybe that one is a
> more radical proposal than this.
Right; your fix seems uncontroversial. I ran into this (indisclustered) bug
while starting to write that patch for "ALTER rewrite in clustered order".
--
Justin
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index (consider moving indisclustered to pg_class)
@ 2020-02-28 23:26 Tom Lane <tgl@sss.pgh.pa.us>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 1 reply; 33+ messages in thread
From: Tom Lane @ 2020-02-28 23:26 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Amit Langote <amitlangote09@gmail.com>; pgsql-hackers
Justin Pryzby <pryzby@telsasoft.com> writes:
> I think the attached is 80% complete (I didn't touch pg_dump).
> One objection to this change would be that all relations (including indices)
> end up with relclustered fields, and pg_index already has a number of bools, so
> it's not like this one bool is wasting a byte.
> I think relisclustered was a's clever way of avoiding that overhead (c0ad5953).
> So I would be -0.5 on moving it to pg_class..
> But I think 0001 and 0002 are worthy. Maybe the test in 0002 should live
> somewhere else.
0001 has been superseded by events (faade5d4c), so the cfbot is choking
on that one's failure to apply, and not testing any further. Please
repost without 0001 so that we can get this testing again.
regards, tom lane
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-02-29 02:42 Justin Pryzby <pryzby@telsasoft.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 33+ messages in thread
From: Justin Pryzby @ 2020-02-29 02:42 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Amit Langote <amitlangote09@gmail.com>; pgsql-hackers@postgresql.org, Michael Paquier <michael@paquier.xyz>
On Fri, Feb 28, 2020 at 06:26:04PM -0500, Tom Lane wrote:
> Justin Pryzby <pryzby@telsasoft.com> writes:
> > I think the attached is 80% complete (I didn't touch pg_dump).
> > One objection to this change would be that all relations (including indices)
> > end up with relclustered fields, and pg_index already has a number of bools, so
> > it's not like this one bool is wasting a byte.
> > I think relisclustered was a's clever way of avoiding that overhead (c0ad5953).
> > So I would be -0.5 on moving it to pg_class..
> > But I think 0001 and 0002 are worthy. Maybe the test in 0002 should live
> > somewhere else.
>
> 0001 has been superseded by events (faade5d4c), so the cfbot is choking
> on that one's failure to apply, and not testing any further. Please
> repost without 0001 so that we can get this testing again.
I've just noticed while working on (1) that this separately affects REINDEX
CONCURRENTLY, which would be a new bug in v12. Without CONCURRENTLY there's no
issue. I guess we need a separate patch for that case.
(1) https://commitfest.postgresql.org/27/2269/
The ALTER bug goes back further and its fix should be a kept separate.
postgres=# DROP TABLE tt; CREATE TABLE tt(i int unique); CLUSTER tt USING tt_i_key; CLUSTER tt; REINDEX INDEX tt_i_key; CLUSTER tt;
DROP TABLE
CREATE TABLE
CLUSTER
CLUSTER
REINDEX
CLUSTER
postgres=# DROP TABLE tt; CREATE TABLE tt(i int unique); CLUSTER tt USING tt_i_key; CLUSTER tt; REINDEX INDEX CONCURRENTLY tt_i_key; CLUSTER tt;
DROP TABLE
CREATE TABLE
CLUSTER
CLUSTER
REINDEX
ERROR: there is no previously clustered index for table "tt"
--
Justin
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-02-29 16:52 Justin Pryzby <pryzby@telsasoft.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 33+ messages in thread
From: Justin Pryzby @ 2020-02-29 16:52 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Amit Langote <amitlangote09@gmail.com>; pgsql-hackers@postgresql.org, Michael Paquier <michael@paquier.xyz>
On Fri, Feb 28, 2020 at 08:42:02PM -0600, Justin Pryzby wrote:
> On Fri, Feb 28, 2020 at 06:26:04PM -0500, Tom Lane wrote:
> > Justin Pryzby <pryzby@telsasoft.com> writes:
> > > I think the attached is 80% complete (I didn't touch pg_dump).
> > > One objection to this change would be that all relations (including indices)
> > > end up with relclustered fields, and pg_index already has a number of bools, so
> > > it's not like this one bool is wasting a byte.
> > > I think relisclustered was a's clever way of avoiding that overhead (c0ad5953).
> > > So I would be -0.5 on moving it to pg_class..
> > > But I think 0001 and 0002 are worthy. Maybe the test in 0002 should live
> > > somewhere else.
> >
> > 0001 has been superseded by events (faade5d4c), so the cfbot is choking
> > on that one's failure to apply, and not testing any further. Please
> > repost without 0001 so that we can get this testing again.
>
> I've just noticed while working on (1) that this separately affects REINDEX
> CONCURRENTLY, which would be a new bug in v12. Without CONCURRENTLY there's no
> issue. I guess we need a separate patch for that case.
Rebased Amit's patch and included my own 0002 to fix the REINDEX CONCURRENTLY
issue.
--
Justin
Attachments:
[text/x-diff] v4-0001-ALTER-tbl-rewrite-loses-CLUSTER-ON-index.patch (5.8K, ../../20200229165258.GJ29456@telsasoft.com/2-v4-0001-ALTER-tbl-rewrite-loses-CLUSTER-ON-index.patch)
download | inline diff:
From dccc6f08450eacafc3a08bc8b2836d2feb23a533 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlangote09@gmail.com>
Date: Thu, 6 Feb 2020 18:14:16 +0900
Subject: [PATCH v4 1/2] ALTER tbl rewrite loses CLUSTER ON index
---
src/backend/commands/tablecmds.c | 39 +++++++++++++++++++++++
src/test/regress/expected/alter_table.out | 34 ++++++++++++++++++++
src/test/regress/sql/alter_table.sql | 16 +++++++++-
3 files changed, 88 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 02a7c04fdb..6b2469bd09 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -490,6 +490,7 @@ static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
static void RebuildConstraintComment(AlteredTableInfo *tab, int pass,
Oid objid, Relation rel, List *domname,
const char *conname);
+static void PreserveClusterOn(AlteredTableInfo *tab, int pass, Oid indoid);
static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
static void TryReuseForeignKey(Oid oldId, Constraint *con);
static ObjectAddress ATExecAlterColumnGenericOptions(Relation rel, const char *colName,
@@ -11838,6 +11839,9 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
newcmd->def = (Node *) stmt;
tab->subcmds[AT_PASS_OLD_INDEX] =
lappend(tab->subcmds[AT_PASS_OLD_INDEX], newcmd);
+
+ /* Preserve index's indisclustered property, if set. */
+ PreserveClusterOn(tab, AT_PASS_OLD_INDEX, oldId);
}
else if (IsA(stm, AlterTableStmt))
{
@@ -11874,6 +11878,9 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
rel,
NIL,
indstmt->idxname);
+
+ /* Preserve index's indisclustered property, if set. */
+ PreserveClusterOn(tab, AT_PASS_OLD_INDEX, indoid);
}
else if (cmd->subtype == AT_AddConstraint)
{
@@ -11996,6 +12003,38 @@ RebuildConstraintComment(AlteredTableInfo *tab, int pass, Oid objid,
tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
}
+/*
+ * For a table's index that is to be recreated due to PostAlterType
+ * processing, preserve its indisclustered property by issuing ALTER TABLE
+ * CLUSTER ON command on the table that will run after the command to recreate
+ * the index.
+ */
+static void
+PreserveClusterOn(AlteredTableInfo *tab, int pass, Oid indoid)
+{
+ HeapTuple indexTuple;
+ Form_pg_index indexForm;
+
+ Assert(OidIsValid(indoid));
+ Assert(pass == AT_PASS_OLD_INDEX);
+
+ indexTuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indoid));
+ if (!HeapTupleIsValid(indexTuple))
+ elog(ERROR, "cache lookup failed for index %u", indoid);
+ indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
+
+ if (indexForm->indisclustered)
+ {
+ AlterTableCmd *newcmd = makeNode(AlterTableCmd);
+
+ newcmd->subtype = AT_ClusterOn;
+ newcmd->name = get_rel_name(indoid);
+ tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
+ }
+
+ ReleaseSysCache(indexTuple);
+}
+
/*
* Subroutine for ATPostAlterTypeParse(). Calls out to CheckIndexCompatible()
* for the real analysis, then mutates the IndexStmt based on that verdict.
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..6e9048bbec 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2802,7 +2802,6 @@ drop table at_test_sql_partop;
drop operator class at_test_sql_partop using btree;
drop function at_test_sql_partop;
-
/* Test case for bug #16242 */
-- We create a parent and child where the child has missing
@@ -2840,3 +2839,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
[text/x-diff] v4-0002-CREATE-INDEX-CONCURRENTLY-to-preserve-CLUSTER.patch (3.1K, ../../20200229165258.GJ29456@telsasoft.com/3-v4-0002-CREATE-INDEX-CONCURRENTLY-to-preserve-CLUSTER.patch)
download | inline diff:
From 855344f709a02907a81ccbe950cd99864b1d2030 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sat, 29 Feb 2020 10:38:18 -0600
Subject: [PATCH v4 2/2] CREATE INDEX CONCURRENTLY to preserve CLUSTER..
---
src/backend/catalog/index.c | 5 ++++-
src/test/regress/expected/create_index.out | 12 ++++++++++++
src/test/regress/sql/create_index.sql | 8 ++++++++
3 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 8880586c37..1d0b1a29aa 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1527,10 +1527,13 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId, const char *oldName)
newIndexForm->indimmediate = oldIndexForm->indimmediate;
oldIndexForm->indimmediate = true;
+ /* Preserve indisclustered */
+ newIndexForm->indisclustered = oldIndexForm->indisclustered;
+ oldIndexForm->indisclustered = false;
+
/* Mark old index as valid and new as invalid as index_set_state_flags */
newIndexForm->indisvalid = true;
oldIndexForm->indisvalid = false;
- oldIndexForm->indisclustered = false;
CatalogTupleUpdate(pg_index, &oldIndexTuple->t_self, oldIndexTuple);
CatalogTupleUpdate(pg_index, &newIndexTuple->t_self, newIndexTuple);
diff --git a/src/test/regress/expected/create_index.out b/src/test/regress/expected/create_index.out
index 6ddf3a63c3..2f0776bfd9 100644
--- a/src/test/regress/expected/create_index.out
+++ b/src/test/regress/expected/create_index.out
@@ -2128,6 +2128,18 @@ SELECT obj_description('testcomment_idx1'::regclass, 'pg_class');
(1 row)
DROP TABLE testcomment;
+-- Check that CLUSTER ON is preserved
+CREATE TABLE concur_clustered(i int);
+CREATE INDEX concur_clustered_i_idx ON concur_clustered(i);
+ALTER TABLE concur_clustered CLUSTER ON concur_clustered_i_idx;
+REINDEX INDEX CONCURRENTLY concur_clustered_i_idx;
+SELECT indexrelid::regclass FROM pg_index WHERE indrelid='concur_clustered'::regclass;
+ indexrelid
+------------------------
+ concur_clustered_i_idx
+(1 row)
+
+DROP TABLE concur_clustered;
-- Partitions
-- Create some partitioned tables
CREATE TABLE concur_reindex_part (c1 int, c2 int) PARTITION BY RANGE (c1);
diff --git a/src/test/regress/sql/create_index.sql b/src/test/regress/sql/create_index.sql
index f7fd756189..73f7ff395c 100644
--- a/src/test/regress/sql/create_index.sql
+++ b/src/test/regress/sql/create_index.sql
@@ -858,6 +858,14 @@ SELECT obj_description('testcomment_idx1'::regclass, 'pg_class');
REINDEX TABLE CONCURRENTLY testcomment ;
SELECT obj_description('testcomment_idx1'::regclass, 'pg_class');
DROP TABLE testcomment;
+-- Check that CLUSTER ON is preserved
+CREATE TABLE concur_clustered(i int);
+CREATE INDEX concur_clustered_i_idx ON concur_clustered(i);
+ALTER TABLE concur_clustered CLUSTER ON concur_clustered_i_idx;
+REINDEX INDEX CONCURRENTLY concur_clustered_i_idx;
+SELECT indexrelid::regclass FROM pg_index WHERE indrelid='concur_clustered'::regclass;
+DROP TABLE concur_clustered;
+
-- Partitions
-- Create some partitioned tables
CREATE TABLE concur_reindex_part (c1 int, c2 int) PARTITION BY RANGE (c1);
--
2.17.0
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-03-02 03:28 Michael Paquier <michael@paquier.xyz>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 2 replies; 33+ messages in thread
From: Michael Paquier @ 2020-03-02 03:28 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Alvaro Herrera <alvherre@2ndquadrant.com>; Amit Langote <amitlangote09@gmail.com>; pgsql-hackers
On Sat, Feb 29, 2020 at 10:52:58AM -0600, Justin Pryzby wrote:
> Rebased Amit's patch and included my own 0002 to fix the REINDEX CONCURRENTLY
> issue.
I have looked at 0002 as that concerns me.
> +SELECT indexrelid::regclass FROM pg_index WHERE indrelid='concur_clustered'::regclass;
> + indexrelid
> +------------------------
> + concur_clustered_i_idx
> +(1 row)
This test should check after indisclustered. Except that, the patch
is fine so I'll apply it if there are no objections.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200302032818.GB32059@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-03-03 01:14 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 0 replies; 33+ messages in thread
From: Michael Paquier @ 2020-03-03 01:14 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Alvaro Herrera <alvherre@2ndquadrant.com>; Amit Langote <amitlangote09@gmail.com>; pgsql-hackers
On Mon, Mar 02, 2020 at 12:28:18PM +0900, Michael Paquier wrote:
> This test should check after indisclustered. Except that, the patch
> is fine so I'll apply it if there are no objections.
I got a second look at this one, and applied it down to 12 after some
small modifications in the new test and in the comments.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200303011406.GF32059@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-03-03 01:31 Justin Pryzby <pryzby@telsasoft.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 33+ messages in thread
From: Justin Pryzby @ 2020-03-03 01:31 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Alvaro Herrera <alvherre@2ndquadrant.com>; Amit Langote <amitlangote09@gmail.com>; pgsql-hackers
On Mon, Mar 02, 2020 at 12:28:18PM +0900, Michael Paquier wrote:
> > +SELECT indexrelid::regclass FROM pg_index WHERE indrelid='concur_clustered'::regclass;
>
> This test should check after indisclustered. Except that, the patch
> is fine so I'll apply it if there are no objections.
Oops - I realized that, but didn't send a new patch before you noticed - thanks
for handling it.
--
Justin
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-03-03 01:36 Justin Pryzby <pryzby@telsasoft.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 2 replies; 33+ messages in thread
From: Justin Pryzby @ 2020-03-03 01:36 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Alvaro Herrera <alvherre@2ndquadrant.com>; Amit Langote <amitlangote09@gmail.com>; pgsql-hackers
@cfbot: resending with only Amit's 0001, since Michael pushed a variation on
0002.
--
Justin
Attachments:
[text/x-diff] v5-0001-ALTER-tbl-rewrite-loses-CLUSTER-ON-index.patch (5.8K, ../../20200303013625.GA21225@telsasoft.com/2-v5-0001-ALTER-tbl-rewrite-loses-CLUSTER-ON-index.patch)
download | inline diff:
From 865fc2713ad29d0f8c0f63609a7c15c83cfa5cfe Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlangote09@gmail.com>
Date: Thu, 6 Feb 2020 18:14:16 +0900
Subject: [PATCH v5] ALTER tbl rewrite loses CLUSTER ON index
---
src/backend/commands/tablecmds.c | 39 +++++++++++++++++++++++
src/test/regress/expected/alter_table.out | 34 ++++++++++++++++++++
src/test/regress/sql/alter_table.sql | 16 +++++++++-
3 files changed, 88 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 02a7c04fdb..6b2469bd09 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -490,6 +490,7 @@ static void ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId,
static void RebuildConstraintComment(AlteredTableInfo *tab, int pass,
Oid objid, Relation rel, List *domname,
const char *conname);
+static void PreserveClusterOn(AlteredTableInfo *tab, int pass, Oid indoid);
static void TryReuseIndex(Oid oldId, IndexStmt *stmt);
static void TryReuseForeignKey(Oid oldId, Constraint *con);
static ObjectAddress ATExecAlterColumnGenericOptions(Relation rel, const char *colName,
@@ -11838,6 +11839,9 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
newcmd->def = (Node *) stmt;
tab->subcmds[AT_PASS_OLD_INDEX] =
lappend(tab->subcmds[AT_PASS_OLD_INDEX], newcmd);
+
+ /* Preserve index's indisclustered property, if set. */
+ PreserveClusterOn(tab, AT_PASS_OLD_INDEX, oldId);
}
else if (IsA(stm, AlterTableStmt))
{
@@ -11874,6 +11878,9 @@ ATPostAlterTypeParse(Oid oldId, Oid oldRelId, Oid refRelId, char *cmd,
rel,
NIL,
indstmt->idxname);
+
+ /* Preserve index's indisclustered property, if set. */
+ PreserveClusterOn(tab, AT_PASS_OLD_INDEX, indoid);
}
else if (cmd->subtype == AT_AddConstraint)
{
@@ -11996,6 +12003,38 @@ RebuildConstraintComment(AlteredTableInfo *tab, int pass, Oid objid,
tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
}
+/*
+ * For a table's index that is to be recreated due to PostAlterType
+ * processing, preserve its indisclustered property by issuing ALTER TABLE
+ * CLUSTER ON command on the table that will run after the command to recreate
+ * the index.
+ */
+static void
+PreserveClusterOn(AlteredTableInfo *tab, int pass, Oid indoid)
+{
+ HeapTuple indexTuple;
+ Form_pg_index indexForm;
+
+ Assert(OidIsValid(indoid));
+ Assert(pass == AT_PASS_OLD_INDEX);
+
+ indexTuple = SearchSysCache1(INDEXRELID, ObjectIdGetDatum(indoid));
+ if (!HeapTupleIsValid(indexTuple))
+ elog(ERROR, "cache lookup failed for index %u", indoid);
+ indexForm = (Form_pg_index) GETSTRUCT(indexTuple);
+
+ if (indexForm->indisclustered)
+ {
+ AlterTableCmd *newcmd = makeNode(AlterTableCmd);
+
+ newcmd->subtype = AT_ClusterOn;
+ newcmd->name = get_rel_name(indoid);
+ tab->subcmds[pass] = lappend(tab->subcmds[pass], newcmd);
+ }
+
+ ReleaseSysCache(indexTuple);
+}
+
/*
* Subroutine for ATPostAlterTypeParse(). Calls out to CheckIndexCompatible()
* for the real analysis, then mutates the IndexStmt based on that verdict.
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..6e9048bbec 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2802,7 +2802,6 @@ drop table at_test_sql_partop;
drop operator class at_test_sql_partop using btree;
drop function at_test_sql_partop;
-
/* Test case for bug #16242 */
-- We create a parent and child where the child has missing
@@ -2840,3 +2839,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
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-03-05 20:11 Ibrar Ahmed <ibrar.ahmad@gmail.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 0 replies; 33+ messages in thread
From: Ibrar Ahmed @ 2020-03-05 20:11 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: Amit Langote <amitlangote09@gmail.com>; Justin Pryzby <justin@telsasoft.com>
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: not tested
I tested the patch on the master branch (a77315fdf2a197a925e670be2d8b376c4ac02efc) and it works fine.
The new status of this patch is: Ready for Committer
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-03-12 17:19 Tom Lane <tgl@sss.pgh.pa.us>
parent: Justin Pryzby <pryzby@telsasoft.com>
1 sibling, 1 reply; 33+ messages in thread
From: Tom Lane @ 2020-03-12 17:19 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Alvaro Herrera <alvherre@2ndquadrant.com>; Amit Langote <amitlangote09@gmail.com>; pgsql-hackers
Justin Pryzby <pryzby@telsasoft.com> writes:
> @cfbot: resending with only Amit's 0001, since Michael pushed a variation on
> 0002.
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.
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.
regards, tom lane
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-03-16 07:01 Amit Langote <amitlangote09@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 33+ messages in thread
From: Amit Langote @ 2020-03-16 07:01 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Michael Paquier <michael@paquier.xyz>; Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-hackers
On Fri, Mar 13, 2020 at 2:19 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Justin Pryzby <pryzby@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 <peter@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
Attachments:
[application/octet-stream] v6-0001-ALTER-tbl-rewrite-loses-CLUSTER-ON-index.patch (7.1K, ../../CA+HiwqEOyeHaJtz9rD0B_hhvWW4-zcF+edYi+Kes_oAXRPbN6w@mail.gmail.com/2-v6-0001-ALTER-tbl-rewrite-loses-CLUSTER-ON-index.patch)
download | inline diff:
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;
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-03-16 13:27 Justin Pryzby <pryzby@telsasoft.com>
parent: Amit Langote <amitlangote09@gmail.com>
0 siblings, 1 reply; 33+ messages in thread
From: Justin Pryzby @ 2020-03-16 13:27 UTC (permalink / raw)
To: Amit Langote <amitlangote09@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; Michael Paquier <michael@paquier.xyz>; Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-hackers
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 <peter@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
Attachments:
[text/x-diff] v7-0001-ALTER-tbl-rewrite-loses-CLUSTER-ON-index.patch (9.4K, ../../20200316132731.GJ26184@telsasoft.com/2-v7-0001-ALTER-tbl-rewrite-loses-CLUSTER-ON-index.patch)
download | inline diff:
From add5e8481c70b6b66342b264a243f26f4c634e53 Mon Sep 17 00:00:00 2001
From: Amit Langote <amitlangote09@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 <tgl@sss.pgh.pa.us> wrote:
> Justin Pryzby <pryzby@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 <peter@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
[text/x-diff] v7-0002-Use-get_index_isclustered-in-cluster.c.patch (2.7K, ../../20200316132731.GJ26184@telsasoft.com/3-v7-0002-Use-get_index_isclustered-in-cluster.c.patch)
download | inline diff:
From ed12fd247ec382b6e1f248596d12ae0d897c8d17 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@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
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-03-16 14:25 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 33+ messages in thread
From: Alvaro Herrera @ 2020-03-16 14:25 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Amit Langote <amitlangote09@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; Michael Paquier <michael@paquier.xyz>; pgsql-hackers
On 2020-Mar-16, Justin Pryzby wrote:
> 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.
Amit's proposed name seems to match lsyscache.c usual conventions better.
> Should we use your get_index_isclustered more widely ?
Yeah, in cluster(), mark_index_clustered().
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-03-17 05:33 Michael Paquier <michael@paquier.xyz>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 33+ messages in thread
From: Michael Paquier @ 2020-03-17 05:33 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Amit Langote <amitlangote09@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers
On Mon, Mar 16, 2020 at 11:25:23AM -0300, Alvaro Herrera wrote:
> On 2020-Mar-16, Justin Pryzby wrote:
>
> > 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.
>
> Amit's proposed name seems to match lsyscache.c usual conventions better.
There were no get_index_isvalid() (introduced by me) or
get_index_isreplident() (introduced by Peter) until last week, and
those names have been chosen to be consistent with the existing
get_index_column_opclass(), so using get_index_isclustered is in my
opinion the most consistent choice.
> Yeah, in cluster(), mark_index_clustered().
Patch 0002 from Justin does that, I would keep this refactoring as
HEAD-only material though, and I don't spot any other code paths in
need of patching.
The commit message of patch 0001 is not what you wanted I guess.
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);
}
No need for two layers of if(s) here.
+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;
Would it make sense to add a second index not used for clustering to
check after the case where isclustered is false? A second thing would
be to check if relfilenode values match before and after each DDL to
make sure that a rewrite happened or not, see check_ddl_rewrite() for
example in alter_table.sql.
Keeping both RememberClusterOnForRebuilding and
RememberReplicaIdentityForRebuilding as separate looks fine to me.
The code could be a bit more organized though. We have
RememberIndexForRebuilding which may go through
RememberConstraintForRebuilding if the relation OID changed is a
constraint, and both register the replindent or isclustered
information if present. Not really something for this patch set to
care about, just a thought while reading this code.
While looking at this bug, I have spotted a behavior which is perhaps
not welcome. Take this test case:
create table aa (a int);
insert into aa values (1), (1);
create unique index concurrently aai on aa(a); -- fails
alter table aa alter column a type bigint;
This generates the following error:
ERROR: 23505: could not create unique index "aai"
DETAIL: Key (a)=(1) is duplicated.
SCHEMA NAME: public
TABLE NAME: aa
CONSTRAINT NAME: aai
LOCATION: comparetup_index_btree, tuplesort.c:4049
After a REINDEX CONCURRENTLY, we may leave behind an invalid index
on the relation's toast table or even normal indexes. CREATE INDEX
CONCURRENTLY may also leave behind invalid indexes. If triggering an
ALTER TABLE that causes a rewrite of the relation, we have the
following behavior:
- An invalid toast index is correctly discarded, keeping one valid
toast index. No problem here.
- Invalid non-toast indexes are all rebuilt. If the index relies on a
constraint then ALTER TABLE would fail, like the above.
I am wondering if there is an argument for not including invalid
indexes in the rebuilt version, even if the existing behavior may be
useful for some users.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200317053332.GE2206@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-03-17 16:20 Justin Pryzby <pryzby@telsasoft.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 33+ messages in thread
From: Justin Pryzby @ 2020-03-17 16:20 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Amit Langote <amitlangote09@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers
On Tue, Mar 17, 2020 at 02:33:32PM +0900, Michael Paquier wrote:
> > Yeah, in cluster(), mark_index_clustered().
>
> Patch 0002 from Justin does that, I would keep this refactoring as
> HEAD-only material though, and I don't spot any other code paths in
> need of patching.
>
> The commit message of patch 0001 is not what you wanted I guess.
That's what git-am does, and I didn't find any option to make it less
unreadable. I guess I should just delete the email body it inserts.
| The commit message is formed by the title taken from the "Subject: ", a
| blank line and the body of the message up to where the patch begins. Excess
| whitespace at the end of each line is automatically stripped.
--
Justin
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-03-18 02:48 Michael Paquier <michael@paquier.xyz>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 33+ messages in thread
From: Michael Paquier @ 2020-03-18 02:48 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Amit Langote <amitlangote09@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers@postgresql.org, Justin Pryzby <pryzby@telsasoft.com>
On Tue, Mar 17, 2020 at 11:20:44AM -0500, Justin Pryzby wrote:
> On Tue, Mar 17, 2020 at 02:33:32PM +0900, Michael Paquier wrote:
>> Patch 0002 from Justin does that, I would keep this refactoring as
>> HEAD-only material though, and I don't spot any other code paths in
>> need of patching.
>>
>> The commit message of patch 0001 is not what you wanted I guess.
>
> That's what git-am does, and I didn't find any option to make it less
> unreadable. I guess I should just delete the email body it inserts.
Strange...
Anyway, Tom, Alvaro, are you planning to look at what is proposed on
this thread? I don't want to step on your toes if that's the case and
it seems to me that the approach taken by the patch is sound, using as
basic fix the addition of an AT_ClusterOn sub-command to the list of
commands to execute when rebuilding the table, ensuring that any
follow-up CLUSTER command will use the correct index.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200318024837.GG214947@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-04-02 06:14 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 2 replies; 33+ messages in thread
From: Michael Paquier @ 2020-04-02 06:14 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Amit Langote <amitlangote09@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers
On Wed, Mar 18, 2020 at 11:48:37AM +0900, Michael Paquier wrote:
> Anyway, Tom, Alvaro, are you planning to look at what is proposed on
> this thread? I don't want to step on your toes if that's the case and
> it seems to me that the approach taken by the patch is sound, using as
> basic fix the addition of an AT_ClusterOn sub-command to the list of
> commands to execute when rebuilding the table, ensuring that any
> follow-up CLUSTER command will use the correct index.
Hearing nothing, I have been looking at the patches sent upthread, and
did some modifications as per the attached for 0001. The logic looked
fine to me and it is unchanged as you are taking care of normal
indexes as well as constraint indexes. Please note that I have
tweaked some comments, and removed what was on top of
RememberConstraintForRebuilding() as that was just a duplicate.
Regarding the tests, I was annoyed by the fact that the logic was not
manipulating two indexes at the same time on the relation rewritten
with a normal and a constraint index, and cross-checking both at the
same time to see which one is clustered after each rewrite is good for
consistency.
Now, regarding patch 0002, note that you have a problem for this part:
- 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);
On an invalid tuple for pg_index, the new code would issue an error,
while the old code would just return. And it seems to me that this
can lead to problems because the parent relation is processed and
locked at the beginning of cluster_rel(), *after* we know the index
OID to work on. The refactoring is fine for the other two areas
though, so I think that there is still value to apply
get_index_isclustered() within mark_index_clustered() and cluster(),
and I would suggest to keep 0002 to that.
Justin, what do you think?
--
Michael
Attachments:
[text/x-diff] v8-0001-Preserve-clustered-indexes-after-rewrites-of-ALTE.patch (8.5K, ../../20200402061421.GA112468@paquier.xyz/2-v8-0001-Preserve-clustered-indexes-after-rewrites-of-ALTE.patch)
download | inline diff:
From 60a2e5a615a39b852985166531019ac1baa4146a Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Thu, 2 Apr 2020 14:52:15 +0900
Subject: [PATCH v8] Preserve clustered indexes after rewrites of ALTER TABLE
---
src/include/utils/lsyscache.h | 1 +
src/backend/commands/tablecmds.c | 47 ++++++++++++++++++++++
src/backend/utils/cache/lsyscache.c | 23 +++++++++++
src/test/regress/expected/alter_table.out | 48 +++++++++++++++++++++++
src/test/regress/sql/alter_table.sql | 25 ++++++++++++
5 files changed, 144 insertions(+)
diff --git a/src/include/utils/lsyscache.h b/src/include/utils/lsyscache.h
index 374f57fb43..c9c68e2f4f 100644
--- a/src/include/utils/lsyscache.h
+++ b/src/include/utils/lsyscache.h
@@ -185,6 +185,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/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index c8c88be2c9..ef234075dc 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 use for CLUSTER */
} AlteredTableInfo;
/* Struct describing one new constraint to check in Phase 3 scan */
@@ -11581,6 +11582,21 @@ 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).
@@ -11606,9 +11622,18 @@ RememberConstraintForRebuilding(Oid conoid, AlteredTableInfo *tab)
tab->changedConstraintDefs = lappend(tab->changedConstraintDefs,
defstring);
+ /*
+ * For the index of a constraint, if any, remember if it is
+ * used for the table's replica identity or if it is a
+ * clustered index, so that ATPostAlterTypeCleanup() can queue
+ * up commands necessary to restore those properties.
+ */
indoid = get_constraint_index(conoid);
if (OidIsValid(indoid))
+ {
RememberReplicaIdentityForRebuilding(indoid, tab);
+ RememberClusterOnForRebuilding(indoid, tab);
+ }
}
}
@@ -11652,7 +11677,14 @@ RememberIndexForRebuilding(Oid indoid, AlteredTableInfo *tab)
tab->changedIndexDefs = lappend(tab->changedIndexDefs,
defstring);
+ /*
+ * Remember if this index is used for the table's replica
+ * identity or if it is a clustered index, so that
+ * ATPostAlterTypeCleanup() can queue up commands necessary to
+ * restore those properties.
+ */
RememberReplicaIdentityForRebuilding(indoid, tab);
+ RememberClusterOnForRebuilding(indoid, tab);
}
}
}
@@ -11779,6 +11811,21 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
lappend(tab->subcmds[AT_PASS_OLD_CONSTR], cmd);
}
+ /*
+ * Queue up command to restore marking of index used for cluster.
+ */
+ 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 0a6db0d478..a7d63f107f 100644
--- a/src/backend/utils/cache/lsyscache.c
+++ b/src/backend/utils/cache/lsyscache.c
@@ -3334,3 +3334,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/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out
index fb6d86a269..81a8e5fc5a 100644
--- a/src/test/regress/expected/alter_table.out
+++ b/src/test/regress/expected/alter_table.out
@@ -4296,3 +4296,51 @@ create trigger xtrig
update bar1 set a = a + 1;
INFO: a=1, b=1
/* End test case for bug #16242 */
+-- Test that ALTER TABLE rewrite preserves a clustered index
+-- for normal indexes and indexes on constraints.
+create table alttype_cluster (a int);
+alter table alttype_cluster add primary key (a);
+create index alttype_cluster_ind on alttype_cluster (a);
+alter table alttype_cluster cluster on alttype_cluster_ind;
+-- Normal index remains clustered.
+select indexrelid::regclass, indisclustered from pg_index
+ where indrelid = 'alttype_cluster'::regclass
+ order by indexrelid::regclass::text;
+ indexrelid | indisclustered
+----------------------+----------------
+ alttype_cluster_ind | t
+ alttype_cluster_pkey | f
+(2 rows)
+
+alter table alttype_cluster alter a type bigint;
+select indexrelid::regclass, indisclustered from pg_index
+ where indrelid = 'alttype_cluster'::regclass
+ order by indexrelid::regclass::text;
+ indexrelid | indisclustered
+----------------------+----------------
+ alttype_cluster_ind | t
+ alttype_cluster_pkey | f
+(2 rows)
+
+-- Constraint index remains clustered.
+alter table alttype_cluster cluster on alttype_cluster_pkey;
+select indexrelid::regclass, indisclustered from pg_index
+ where indrelid = 'alttype_cluster'::regclass
+ order by indexrelid::regclass::text;
+ indexrelid | indisclustered
+----------------------+----------------
+ alttype_cluster_ind | f
+ alttype_cluster_pkey | t
+(2 rows)
+
+alter table alttype_cluster alter a type int;
+select indexrelid::regclass, indisclustered from pg_index
+ where indrelid = 'alttype_cluster'::regclass
+ order by indexrelid::regclass::text;
+ indexrelid | indisclustered
+----------------------+----------------
+ alttype_cluster_ind | f
+ alttype_cluster_pkey | t
+(2 rows)
+
+drop table alttype_cluster;
diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql
index 3801f19c58..5e6b74c54f 100644
--- a/src/test/regress/sql/alter_table.sql
+++ b/src/test/regress/sql/alter_table.sql
@@ -2840,3 +2840,28 @@ create trigger xtrig
update bar1 set a = a + 1;
/* End test case for bug #16242 */
+
+-- Test that ALTER TABLE rewrite preserves a clustered index
+-- for normal indexes and indexes on constraints.
+create table alttype_cluster (a int);
+alter table alttype_cluster add primary key (a);
+create index alttype_cluster_ind on alttype_cluster (a);
+alter table alttype_cluster cluster on alttype_cluster_ind;
+-- Normal index remains clustered.
+select indexrelid::regclass, indisclustered from pg_index
+ where indrelid = 'alttype_cluster'::regclass
+ order by indexrelid::regclass::text;
+alter table alttype_cluster alter a type bigint;
+select indexrelid::regclass, indisclustered from pg_index
+ where indrelid = 'alttype_cluster'::regclass
+ order by indexrelid::regclass::text;
+-- Constraint index remains clustered.
+alter table alttype_cluster cluster on alttype_cluster_pkey;
+select indexrelid::regclass, indisclustered from pg_index
+ where indrelid = 'alttype_cluster'::regclass
+ order by indexrelid::regclass::text;
+alter table alttype_cluster alter a type int;
+select indexrelid::regclass, indisclustered from pg_index
+ where indrelid = 'alttype_cluster'::regclass
+ order by indexrelid::regclass::text;
+drop table alttype_cluster;
--
2.26.0
[application/pgp-signature] signature.asc (832B, ../../20200402061421.GA112468@paquier.xyz/3-signature.asc)
download
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-04-02 06:52 Justin Pryzby <pryzby@telsasoft.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 33+ messages in thread
From: Justin Pryzby @ 2020-04-02 06:52 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Amit Langote <amitlangote09@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers
On Thu, Apr 02, 2020 at 03:14:21PM +0900, Michael Paquier wrote:
> Now, regarding patch 0002, note that you have a problem for this part:
> - 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);
>
> On an invalid tuple for pg_index, the new code would issue an error,
> while the old code would just return. And it seems to me that this
> can lead to problems because the parent relation is processed and
> locked at the beginning of cluster_rel(), *after* we know the index
> OID to work on.
> The refactoring is fine for the other two areas
> though, so I think that there is still value to apply
> get_index_isclustered() within mark_index_clustered() and cluster(),
> and I would suggest to keep 0002 to that.
>
> Justin, what do you think?
Sounds right. Or else get_index_isclustered() could be redefined to take a
boolean "do_elog" flag, and if syscache fails and that's false, then return
false instead of ERROR.
--
Justin
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-04-02 07:24 Michael Paquier <michael@paquier.xyz>
parent: Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 1 reply; 33+ messages in thread
From: Michael Paquier @ 2020-04-02 07:24 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Amit Langote <amitlangote09@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers
On Thu, Apr 02, 2020 at 01:52:09AM -0500, Justin Pryzby wrote:
> Sounds right. Or else get_index_isclustered() could be redefined to take a
> boolean "do_elog" flag, and if syscache fails and that's false, then return
> false instead of ERROR.
Not sure if that's completely right to do either. For one, it is not
consistent with the surroundings as of lsyscache.c.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200402072403.GD112468@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-04-02 07:38 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Michael Paquier <michael@paquier.xyz>
1 sibling, 1 reply; 33+ messages in thread
From: Alvaro Herrera @ 2020-04-02 07:38 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Amit Langote <amitlangote09@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers
On 2020-Apr-02, Michael Paquier wrote:
> Now, regarding patch 0002, note that you have a problem for this part:
> - 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);
>
> On an invalid tuple for pg_index, the new code would issue an error,
> while the old code would just return.
I don't think we need to worry about that problem, because we already
checked that the pg_class tuple for the index is there two lines above.
The pg_index tuple cannot have gone away on its own; and the index can't
be deleted either, because cluster_rel holds AEL on the table. There
isn't "probably" about the can't-happen condition, is there?
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-04-02 07:39 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 33+ messages in thread
From: Michael Paquier @ 2020-04-02 07:39 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: Alvaro Herrera <alvherre@2ndquadrant.com>; Amit Langote <amitlangote09@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers
On Thu, Apr 02, 2020 at 04:24:03PM +0900, Michael Paquier wrote:
> On Thu, Apr 02, 2020 at 01:52:09AM -0500, Justin Pryzby wrote:
>> Sounds right. Or else get_index_isclustered() could be redefined to take a
>> boolean "do_elog" flag, and if syscache fails and that's false, then return
>> false instead of ERROR.
>
> Not sure if that's completely right to do either. For one, it is not
> consistent with the surroundings as of lsyscache.c.
Actually, we do have some missing_ok flags lying around already in
lsyscache.c, so it would be much more consistent to use that name that
instead of the do_elog you are suggesting. Could you update the
patch to reflect that?
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200402073958.GE112468@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-04-03 06:54 Michael Paquier <michael@paquier.xyz>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 33+ messages in thread
From: Michael Paquier @ 2020-04-03 06:54 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Amit Langote <amitlangote09@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers
On Thu, Apr 02, 2020 at 04:38:36AM -0300, Alvaro Herrera wrote:
> I don't think we need to worry about that problem, because we already
> checked that the pg_class tuple for the index is there two lines above.
> The pg_index tuple cannot have gone away on its own; and the index can't
> be deleted either, because cluster_rel holds AEL on the table. There
> isn't "probably" about the can't-happen condition, is there?
Yes, you are right here. I was wondering about an interference with
the multi-relation cluster that would not lock the parent relation at
the upper level of cluster() but the check on the existence of the
index makes sure that we'll never see an invalid entry in pg_index, so
let's keep patch 0002 as originally presented. As the commit tree is
going to be rather crowded until feature freeze on Sunday, I'll wait
until Monday or Tuesday to finalize this patch set.
Now, would it be better to apply the refactoring patch for HEAD before
feature freeze, or are people fine if this is committed a bit after?
Patch 0002 is neither a new feature, nor an actual bug, and just some
code cleanup, but I am a bit worried about applying that cleanup on
HEAD just after the freeze.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200403065438.GA78887@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: ALTER tbl rewrite loses CLUSTER ON index
@ 2020-04-06 02:47 Michael Paquier <michael@paquier.xyz>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 0 replies; 33+ messages in thread
From: Michael Paquier @ 2020-04-06 02:47 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: Justin Pryzby <pryzby@telsasoft.com>; Amit Langote <amitlangote09@gmail.com>; Tom Lane <tgl@sss.pgh.pa.us>; pgsql-hackers
On Fri, Apr 03, 2020 at 03:54:38PM +0900, Michael Paquier wrote:
> Now, would it be better to apply the refactoring patch for HEAD before
> feature freeze, or are people fine if this is committed a bit after?
> Patch 0002 is neither a new feature, nor an actual bug, and just some
> code cleanup, but I am a bit worried about applying that cleanup on
> HEAD just after the freeze.
I have worked more on this one this morning and just applied the bug
fix down to 9.5, and the refactoring on HEAD. Thanks!
--
Michael
Attachments:
[application/pgp-signature] signature.asc (832B, ../../20200406024757.GA2066@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 33+ messages in thread
end of thread, other threads:[~2020-04-06 02:47 UTC | newest]
Thread overview: 33+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-02-02 16:17 ALTER tbl rewrite loses CLUSTER ON index Justin Pryzby <pryzby@telsasoft.com>
2020-02-05 06:53 ` Amit Langote <amitlangote09@gmail.com>
2020-02-05 08:32 ` Justin Pryzby <pryzby@telsasoft.com>
2020-02-06 09:14 ` Amit Langote <amitlangote09@gmail.com>
2020-02-06 14:44 ` Justin Pryzby <pryzby@telsasoft.com>
2020-02-06 17:24 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2020-02-07 08:42 ` Amit Langote <amitlangote09@gmail.com>
2020-02-07 14:39 ` Re: ALTER tbl rewrite loses CLUSTER ON index (consider moving indisclustered to pg_class) Justin Pryzby <pryzby@telsasoft.com>
2020-02-17 05:31 ` Re: ALTER tbl rewrite loses CLUSTER ON index (consider moving indisclustered to pg_class) Amit Langote <amitlangote09@gmail.com>
2020-02-17 05:49 ` Re: ALTER tbl rewrite loses CLUSTER ON index (consider moving indisclustered to pg_class) Justin Pryzby <pryzby@telsasoft.com>
2020-02-28 23:26 ` Re: ALTER tbl rewrite loses CLUSTER ON index (consider moving indisclustered to pg_class) Tom Lane <tgl@sss.pgh.pa.us>
2020-02-29 02:42 ` Justin Pryzby <pryzby@telsasoft.com>
2020-02-29 16:52 ` Justin Pryzby <pryzby@telsasoft.com>
2020-03-02 03:28 ` Michael Paquier <michael@paquier.xyz>
2020-03-03 01:14 ` Michael Paquier <michael@paquier.xyz>
2020-03-03 01:31 ` Justin Pryzby <pryzby@telsasoft.com>
2020-03-03 01:36 ` Justin Pryzby <pryzby@telsasoft.com>
2020-03-05 20:11 ` Ibrar Ahmed <ibrar.ahmad@gmail.com>
2020-03-12 17:19 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-03-16 07:01 ` Amit Langote <amitlangote09@gmail.com>
2020-03-16 13:27 ` Justin Pryzby <pryzby@telsasoft.com>
2020-03-16 14:25 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2020-03-17 05:33 ` Michael Paquier <michael@paquier.xyz>
2020-03-17 16:20 ` Justin Pryzby <pryzby@telsasoft.com>
2020-03-18 02:48 ` Michael Paquier <michael@paquier.xyz>
2020-04-02 06:14 ` Michael Paquier <michael@paquier.xyz>
2020-04-02 06:52 ` Justin Pryzby <pryzby@telsasoft.com>
2020-04-02 07:24 ` Michael Paquier <michael@paquier.xyz>
2020-04-02 07:39 ` Michael Paquier <michael@paquier.xyz>
2020-04-02 07:38 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2020-04-03 06:54 ` Michael Paquier <michael@paquier.xyz>
2020-04-06 02:47 ` Michael Paquier <michael@paquier.xyz>
2020-02-06 09:14 [PATCH v5] ALTER tbl rewrite loses CLUSTER ON index Amit Langote <amitlangote09@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox