From eeecbd232483057501d35e87bdf8c958035e0926 Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Fri, 9 Jul 2021 17:04:39 +0300 Subject: [PATCH 1/3] Introduce ColumnarSupportsIndexAM --- src/backend/columnar/columnar_tableam.c | 16 +++++++++++++--- src/include/columnar/columnar_tableam.h | 1 + 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/backend/columnar/columnar_tableam.c b/src/backend/columnar/columnar_tableam.c index db5e19013..480e38e8e 100644 --- a/src/backend/columnar/columnar_tableam.c +++ b/src/backend/columnar/columnar_tableam.c @@ -1863,9 +1863,7 @@ ColumnarProcessUtility(PlannedStmt *pstmt, GetCreateIndexRelationLockMode(indexStmt)); if (rel->rd_tableam == GetColumnarTableAmRoutine()) { - /* for now, we don't support index access methods other than btree & hash */ - if (strncmp(indexStmt->accessMethod, "btree", NAMEDATALEN) != 0 && - strncmp(indexStmt->accessMethod, "hash", NAMEDATALEN) != 0) + if (!ColumnarSupportsIndexAM(indexStmt->accessMethod)) { ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("only btree and hash indexes are supported on " @@ -1881,6 +1879,18 @@ ColumnarProcessUtility(PlannedStmt *pstmt, } +/* + * ColumnarSupportsIndexAM returns true if indexAM with given name is + * supported by columnar tables. + */ +bool +ColumnarSupportsIndexAM(char *indexAMName) +{ + return strncmp(indexAMName, "btree", NAMEDATALEN) == 0 || + strncmp(indexAMName, "hash", NAMEDATALEN) == 0; +} + + /* * IsColumnarTableAmTable returns true if relation has columnar_tableam * access method. This can be called before extension creation. diff --git a/src/include/columnar/columnar_tableam.h b/src/include/columnar/columnar_tableam.h index 2809c6439..adb819f1f 100644 --- a/src/include/columnar/columnar_tableam.h +++ b/src/include/columnar/columnar_tableam.h @@ -52,6 +52,7 @@ extern TableScanDesc columnar_beginscan_extended(Relation relation, Snapshot sna uint32 flags, Bitmapset *attr_needed, List *scanQual); extern int64 ColumnarScanChunkGroupsFiltered(TableScanDesc scanDesc); +extern bool ColumnarSupportsIndexAM(char *indexAMName); extern bool IsColumnarTableAmTable(Oid relationId); extern TableDDLCommand * ColumnarGetTableOptionsDDL(Oid relationId); extern char * GetShardedTableDDLCommandColumnar(uint64 shardId, void *context); From 90e856d6bcbbb170ae47540ebdbde1e54c249b2c Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Fri, 9 Jul 2021 17:04:53 +0300 Subject: [PATCH 2/3] Keep supported indexes when converting table to columnar --- .../distributed/commands/alter_table.c | 139 ++++++++++++++++-- .../alter_table_set_access_method.out | 92 +++++++++++- .../sql/alter_table_set_access_method.sql | 45 +++++- 3 files changed, 255 insertions(+), 21 deletions(-) diff --git a/src/backend/distributed/commands/alter_table.c b/src/backend/distributed/commands/alter_table.c index da2e5490c..052ca8b1b 100644 --- a/src/backend/distributed/commands/alter_table.c +++ b/src/backend/distributed/commands/alter_table.c @@ -181,6 +181,11 @@ static TableConversionReturn * AlterTableSetAccessMethod( static TableConversionReturn * ConvertTable(TableConversionState *con); static bool SwitchToSequentialAndLocalExecutionIfShardNameTooLong(char *relationName, char *longestShardName); +static void DropIndexesNotSupportedByColumnar(Oid relationId, + bool suppressNoticeMessages); +static char * GetIndexAccessMethodName(Oid indexId); +static void DropConstraintRestrict(Oid relationId, Oid constraintId); +static void DropIndexRestrict(Oid indexId); static void EnsureTableNotReferencing(Oid relationId, char conversionType); static void EnsureTableNotReferenced(Oid relationId, char conversionType); static void EnsureTableNotForeign(Oid relationId); @@ -537,26 +542,17 @@ ConvertTable(TableConversionState *con) List *preLoadCommands = GetPreLoadTableCreationCommands(con->relationId, true, newAccessMethod); - bool includeIndexes = true; if (con->accessMethod && strcmp(con->accessMethod, "columnar") == 0) { - if (!con->suppressNoticeMessages) - { - List *explicitIndexesOnTable = GetExplicitIndexOidList(con->relationId); - Oid indexOid = InvalidOid; - foreach_oid(indexOid, explicitIndexesOnTable) - { - ereport(NOTICE, (errmsg("the index %s on table %s will be dropped, " - "because columnar tables cannot have indexes", - get_rel_name(indexOid), - quote_qualified_identifier(con->schemaName, - con->relationName)))); - } - } - - includeIndexes = false; + DropIndexesNotSupportedByColumnar(con->relationId, + con->suppressNoticeMessages); } + /* + * Since we already dropped unsupported indexes, we can safely pass + * includeIndexes to be true. + */ + bool includeIndexes = true; bool includeReplicaIdentity = true; List *postLoadCommands = GetPostLoadTableCreationCommands(con->relationId, includeIndexes, @@ -825,6 +821,117 @@ ConvertTable(TableConversionState *con) } +/* + * DropIndexesNotSupportedByColumnar is a helper function used during accces + * method conversion to drop the indexes that are not supported by columnarAM. + */ +static void +DropIndexesNotSupportedByColumnar(Oid relationId, bool suppressNoticeMessages) +{ + Relation columnarRelation = RelationIdGetRelation(relationId); + List *indexIdList = RelationGetIndexList(columnarRelation); + + /* + * Immediately close the relation since we might execute ALTER TABLE + * for that relation. + */ + RelationClose(columnarRelation); + + Oid indexId = InvalidOid; + foreach_oid(indexId, indexIdList) + { + char *indexAmName = GetIndexAccessMethodName(indexId); + if (ColumnarSupportsIndexAM(indexAmName)) + { + continue; + } + + if (!suppressNoticeMessages) + { + ereport(NOTICE, (errmsg("unsupported access method for index %s " + "on columnar table %s, given index and " + "the constraint depending on the index " + "(if any) will be dropped", + get_rel_name(indexId), + generate_qualified_relation_name(relationId)))); + } + + Oid constraintId = get_index_constraint(indexId); + if (OidIsValid(constraintId)) + { + /* index is implied by a constraint, so drop the constraint itself */ + DropConstraintRestrict(relationId, constraintId); + } + else + { + DropIndexRestrict(indexId); + } + } +} + + +/* + * GetIndexAccessMethodName returns access method name of index with indexId. + * If there is no such index, then errors out. + */ +static char * +GetIndexAccessMethodName(Oid indexId) +{ + /* fetch pg_class tuple of the index relation */ + HeapTuple indexTuple = SearchSysCache1(RELOID, ObjectIdGetDatum(indexId)); + if (!HeapTupleIsValid(indexTuple)) + { + ereport(ERROR, (errmsg("index with oid %u does not exist", indexId))); + } + + Form_pg_class indexForm = (Form_pg_class) GETSTRUCT(indexTuple); + Oid indexAMId = indexForm->relam; + ReleaseSysCache(indexTuple); + + /* fetch pg_am tuple of index' access method */ + HeapTuple indexAMTuple = SearchSysCache1(AMOID, ObjectIdGetDatum(indexAMId)); + if (!HeapTupleIsValid(indexAMTuple)) + { + ereport(ERROR, (errmsg("access method with oid %u does not exist", indexAMId))); + } + + Form_pg_am indexAMForm = (Form_pg_am) GETSTRUCT(indexAMTuple); + char *indexAmName = pstrdup(indexAMForm->amname.data); + ReleaseSysCache(indexAMTuple); + + return indexAmName; +} + + +/* + * DropConstraintRestrict drops the constraint with constraintId by using spi. + */ +static void +DropConstraintRestrict(Oid relationId, Oid constraintId) +{ + char *qualifiedRelationName = generate_qualified_relation_name(relationId); + char *constraintName = get_constraint_name(constraintId); + const char *quotedConstraintName = quote_identifier(constraintName); + StringInfo dropConstraintCommand = makeStringInfo(); + appendStringInfo(dropConstraintCommand, "ALTER TABLE %s DROP CONSTRAINT %s RESTRICT;", + qualifiedRelationName, quotedConstraintName); + ExecuteQueryViaSPI(dropConstraintCommand->data, SPI_OK_UTILITY); +} + + +/* + * DropIndexRestrict drops the index with indexId by using spi. + */ +static void +DropIndexRestrict(Oid indexId) +{ + char *qualifiedIndexName = generate_qualified_relation_name(indexId); + StringInfo dropIndexCommand = makeStringInfo(); + appendStringInfo(dropIndexCommand, "DROP INDEX %s RESTRICT;", qualifiedIndexName); + ExecuteQueryViaSPI(dropIndexCommand->data, SPI_OK_UTILITY); +} + + /* * EnsureTableNotReferencing checks if the table has a reference to another * table and errors if it is. diff --git a/src/test/regress/expected/alter_table_set_access_method.out b/src/test/regress/expected/alter_table_set_access_method.out index c81578f5c..cf72940b1 100644 --- a/src/test/regress/expected/alter_table_set_access_method.out +++ b/src/test/regress/expected/alter_table_set_access_method.out @@ -301,7 +301,6 @@ SELECT event FROM time_partitioned ORDER BY 1; \set VERBOSITY default DROP TABLE time_partitioned; -- test altering a table with index to columnar --- the index will be dropped CREATE TABLE index_table (a INT) ; CREATE INDEX idx1 ON index_table (a); -- also create an index with statistics @@ -321,8 +320,6 @@ SELECT a.amname FROM pg_class c, pg_am a where c.relname = 'index_table' AND c.r (1 row) SELECT alter_table_set_access_method('index_table', 'columnar'); -NOTICE: the index idx1 on table alter_table_set_access_method.index_table will be dropped, because columnar tables cannot have indexes -NOTICE: the index idx2 on table alter_table_set_access_method.index_table will be dropped, because columnar tables cannot have indexes NOTICE: creating a new table for alter_table_set_access_method.index_table NOTICE: moving the data of alter_table_set_access_method.index_table NOTICE: dropping the old alter_table_set_access_method.index_table @@ -335,7 +332,9 @@ NOTICE: renaming the new table to alter_table_set_access_method.index_table SELECT indexname FROM pg_indexes WHERE schemaname = 'alter_table_set_access_method' AND tablename = 'index_table'; indexname --------------------------------------------------------------------- -(0 rows) + idx1 + idx2 +(2 rows) SELECT a.amname FROM pg_class c, pg_am a where c.relname = 'index_table' AND c.relnamespace = 'alter_table_set_access_method'::regnamespace AND c.relam = a.oid; amname @@ -343,6 +342,91 @@ SELECT a.amname FROM pg_class c, pg_am a where c.relname = 'index_table' AND c.r columnar (1 row) +CREATE TABLE "heap_\'tbl" ( + c1 CIRCLE, + c2 TEXT, + i int4[], + p point, + a int, + EXCLUDE USING gist + (c1 WITH &&, (c2::circle) WITH &&) + WHERE (circle_center(c1) <> '(0,0)'), + EXCLUDE USING btree + (a WITH =) + INCLUDE(p) + WHERE (c2 < 'astring') +); +CREATE INDEX heap_tbl_gin ON "heap_\'tbl" USING gin (i); +CREATE INDEX "heap_tbl_\'gist" ON "heap_\'tbl" USING gist(p); +CREATE INDEX heap_tbl_brin ON "heap_\'tbl" USING brin (a) WITH (pages_per_range = 1); +CREATE INDEX heap_tbl_hash ON "heap_\'tbl" USING hash (c2); +ALTER TABLE "heap_\'tbl" ADD CONSTRAINT heap_tbl_unique UNIQUE (c2); +CREATE UNIQUE INDEX "heap_tbl_\'btree" ON "heap_\'tbl" USING btree (a); +ALTER TABLE "heap_\'tbl" ADD CONSTRAINT "heap_tbl_\'pkey" PRIMARY KEY USING INDEX "heap_tbl_\'btree"; +NOTICE: ALTER TABLE / ADD CONSTRAINT USING INDEX will rename index "heap_tbl_\'btree" to "heap_tbl_\'pkey" +SELECT indexname FROM pg_indexes +WHERE schemaname = 'alter_table_set_access_method' AND + tablename = 'heap_\''tbl' +ORDER BY indexname; + indexname +--------------------------------------------------------------------- + heap_\'tbl_a_p_excl + heap_\'tbl_c1_c2_excl + heap_tbl_\'gist + heap_tbl_\'pkey + heap_tbl_brin + heap_tbl_gin + heap_tbl_hash + heap_tbl_unique +(8 rows) + +SELECT conname FROM pg_constraint +WHERE conrelid = 'heap_\''tbl'::regclass +ORDER BY conname; + conname +--------------------------------------------------------------------- + heap_\'tbl_a_p_excl + heap_\'tbl_c1_c2_excl + heap_tbl_\'pkey + heap_tbl_unique +(4 rows) + +SELECT alter_table_set_access_method('heap_\''tbl', 'columnar'); +NOTICE: unsupported access method for index heap_\'tbl_c1_c2_excl on columnar table alter_table_set_access_method."heap_\'tbl", given index and the constraint depending on the index (if any) will be dropped +NOTICE: unsupported access method for index heap_tbl_gin on columnar table alter_table_set_access_method."heap_\'tbl", given index and the constraint depending on the index (if any) will be dropped +NOTICE: unsupported access method for index heap_tbl_\'gist on columnar table alter_table_set_access_method."heap_\'tbl", given index and the constraint depending on the index (if any) will be dropped +NOTICE: unsupported access method for index heap_tbl_brin on columnar table alter_table_set_access_method."heap_\'tbl", given index and the constraint depending on the index (if any) will be dropped +NOTICE: creating a new table for alter_table_set_access_method."heap_\'tbl" +NOTICE: moving the data of alter_table_set_access_method."heap_\'tbl" +NOTICE: dropping the old alter_table_set_access_method."heap_\'tbl" +NOTICE: renaming the new table to alter_table_set_access_method."heap_\'tbl" + alter_table_set_access_method +--------------------------------------------------------------------- + +(1 row) + +SELECT indexname FROM pg_indexes +WHERE schemaname = 'alter_table_set_access_method' AND + tablename = 'heap_\''tbl' +ORDER BY indexname; + indexname +--------------------------------------------------------------------- + heap_\'tbl_a_p_excl + heap_tbl_\'pkey + heap_tbl_hash + heap_tbl_unique +(4 rows) + +SELECT conname FROM pg_constraint +WHERE conrelid = 'heap_\''tbl'::regclass +ORDER BY conname; + conname +--------------------------------------------------------------------- + heap_\'tbl_a_p_excl + heap_tbl_\'pkey + heap_tbl_unique +(3 rows) + -- test different table types SET client_min_messages to WARNING; SELECT 1 FROM master_add_node('localhost', :master_port, groupId := 0); diff --git a/src/test/regress/sql/alter_table_set_access_method.sql b/src/test/regress/sql/alter_table_set_access_method.sql index b041f8f65..4bb75d5c2 100644 --- a/src/test/regress/sql/alter_table_set_access_method.sql +++ b/src/test/regress/sql/alter_table_set_access_method.sql @@ -102,7 +102,6 @@ SELECT event FROM time_partitioned ORDER BY 1; DROP TABLE time_partitioned; -- test altering a table with index to columnar --- the index will be dropped CREATE TABLE index_table (a INT) USING heap; CREATE INDEX idx1 ON index_table (a); -- also create an index with statistics @@ -114,6 +113,50 @@ SELECT alter_table_set_access_method('index_table', 'columnar'); SELECT indexname FROM pg_indexes WHERE schemaname = 'alter_table_set_access_method' AND tablename = 'index_table'; SELECT a.amname FROM pg_class c, pg_am a where c.relname = 'index_table' AND c.relnamespace = 'alter_table_set_access_method'::regnamespace AND c.relam = a.oid; +CREATE TABLE "heap_\'tbl" ( + c1 CIRCLE, + c2 TEXT, + i int4[], + p point, + a int, + EXCLUDE USING gist + (c1 WITH &&, (c2::circle) WITH &&) + WHERE (circle_center(c1) <> '(0,0)'), + EXCLUDE USING btree + (a WITH =) + INCLUDE(p) + WHERE (c2 < 'astring') +); + +CREATE INDEX heap_tbl_gin ON "heap_\'tbl" USING gin (i); +CREATE INDEX "heap_tbl_\'gist" ON "heap_\'tbl" USING gist(p); +CREATE INDEX heap_tbl_brin ON "heap_\'tbl" USING brin (a) WITH (pages_per_range = 1); + +CREATE INDEX heap_tbl_hash ON "heap_\'tbl" USING hash (c2); +ALTER TABLE "heap_\'tbl" ADD CONSTRAINT heap_tbl_unique UNIQUE (c2); + +CREATE UNIQUE INDEX "heap_tbl_\'btree" ON "heap_\'tbl" USING btree (a); +ALTER TABLE "heap_\'tbl" ADD CONSTRAINT "heap_tbl_\'pkey" PRIMARY KEY USING INDEX "heap_tbl_\'btree"; + +SELECT indexname FROM pg_indexes +WHERE schemaname = 'alter_table_set_access_method' AND + tablename = 'heap_\''tbl' +ORDER BY indexname; + +SELECT conname FROM pg_constraint +WHERE conrelid = 'heap_\''tbl'::regclass +ORDER BY conname; + +SELECT alter_table_set_access_method('heap_\''tbl', 'columnar'); + +SELECT indexname FROM pg_indexes +WHERE schemaname = 'alter_table_set_access_method' AND + tablename = 'heap_\''tbl' +ORDER BY indexname; + +SELECT conname FROM pg_constraint +WHERE conrelid = 'heap_\''tbl'::regclass +ORDER BY conname; -- test different table types SET client_min_messages to WARNING; From 84a49cc221457405ad632eb190e1b4cc1f4dd297 Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Mon, 26 Jul 2021 11:50:35 +0300 Subject: [PATCH 3/3] Improve error message for indexAMs not supported by columnar --- src/backend/columnar/columnar_tableam.c | 5 +++-- src/test/regress/expected/columnar_alter.out | 2 +- src/test/regress/expected/columnar_indexes.out | 8 ++++---- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/backend/columnar/columnar_tableam.c b/src/backend/columnar/columnar_tableam.c index 480e38e8e..acf092cb9 100644 --- a/src/backend/columnar/columnar_tableam.c +++ b/src/backend/columnar/columnar_tableam.c @@ -1866,8 +1866,9 @@ ColumnarProcessUtility(PlannedStmt *pstmt, if (!ColumnarSupportsIndexAM(indexStmt->accessMethod)) { ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("only btree and hash indexes are supported on " - "columnar tables "))); + errmsg("unsupported access method for the " + "index on columnar table %s", + RelationGetRelationName(rel)))); } } diff --git a/src/test/regress/expected/columnar_alter.out b/src/test/regress/expected/columnar_alter.out index 4706fa386..707107568 100644 --- a/src/test/regress/expected/columnar_alter.out +++ b/src/test/regress/expected/columnar_alter.out @@ -412,7 +412,7 @@ CREATE TABLE circles ( c circle, EXCLUDE USING gist (c WITH &&) ) USING columnar; -ERROR: only btree and hash indexes are supported on columnar tables +ERROR: unsupported access method for the index on columnar table circles -- Row level security CREATE TABLE public.row_level_security_col (id int, pgUser CHARACTER VARYING) USING columnar; CREATE USER user1; diff --git a/src/test/regress/expected/columnar_indexes.out b/src/test/regress/expected/columnar_indexes.out index ef4a5d0de..7b28bce21 100644 --- a/src/test/regress/expected/columnar_indexes.out +++ b/src/test/regress/expected/columnar_indexes.out @@ -396,19 +396,19 @@ SELECT COUNT(*)=2 FROM pg_indexes WHERE tablename = 'p1'; CREATE TABLE testjsonb (j JSONB) USING columnar; INSERT INTO testjsonb SELECT CAST('{"f1" : ' ||'"'|| i*4 ||'", ' || '"f2" : '||'"'|| i*10 ||'"}' AS JSON) FROM generate_series(1,10) i; CREATE INDEX jidx ON testjsonb USING GIN (j); -ERROR: only btree and hash indexes are supported on columnar tables +ERROR: unsupported access method for the index on columnar table testjsonb INSERT INTO testjsonb SELECT CAST('{"f1" : ' ||'"'|| i*4 ||'", ' || '"f2" : '||'"'|| i*10 ||'"}' AS JSON) FROM generate_series(15,20) i; -- gist -- CREATE TABLE gist_point_tbl(id INT4, p POINT) USING columnar; INSERT INTO gist_point_tbl (id, p) SELECT g, point(g*10, g*10) FROM generate_series(1, 10) g; CREATE INDEX gist_pointidx ON gist_point_tbl USING gist(p); -ERROR: only btree and hash indexes are supported on columnar tables +ERROR: unsupported access method for the index on columnar table gist_point_tbl INSERT INTO gist_point_tbl (id, p) SELECT g, point(g*10, g*10) FROM generate_series(10, 20) g; -- sp gist -- CREATE TABLE box_temp (f1 box) USING columnar; INSERT INTO box_temp SELECT box(point(i, i), point(i * 2, i * 2)) FROM generate_series(1, 10) AS i; CREATE INDEX CONCURRENTLY box_spgist ON box_temp USING spgist (f1); -ERROR: only btree and hash indexes are supported on columnar tables +ERROR: unsupported access method for the index on columnar table box_temp -- CONCURRENTLY should not leave an invalid index behind SELECT COUNT(*)=0 FROM pg_index WHERE indrelid = 'box_temp'::regclass AND indisvalid = 'false'; ?column? @@ -420,7 +420,7 @@ INSERT INTO box_temp SELECT box(point(i, i), point(i * 2, i * 2)) FROM generate_ -- brin -- CREATE TABLE brin_summarize (value int) USING columnar; CREATE INDEX brin_summarize_idx ON brin_summarize USING brin (value) WITH (pages_per_range=2); -ERROR: only btree and hash indexes are supported on columnar tables +ERROR: unsupported access method for the index on columnar table brin_summarize -- Show that we safely fallback to serial index build. CREATE TABLE parallel_scan_test(a int) USING columnar WITH ( parallel_workers = 2 ); INSERT INTO parallel_scan_test SELECT i FROM generate_series(1,10) i;