From 17266e3301dd7c39fa8a41ae79fc6c4f9fea7398 Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Thu, 11 Jan 2018 18:05:51 +0100 Subject: [PATCH 1/3] Implement ALTER INDEX ... RENAME TO ... The command is now distributed among the shards when the table is distributed. To that effect, we fill in the DDLJob's targetRelationId with the OID of the table for which the index is defined, rather than the OID of the index itself. --- .../distributed/executor/multi_utility.c | 89 ++++++++++++++++--- .../input/multi_alter_table_statements.source | 15 ++++ .../multi_alter_table_statements.source | 34 +++++++ 3 files changed, 126 insertions(+), 12 deletions(-) diff --git a/src/backend/distributed/executor/multi_utility.c b/src/backend/distributed/executor/multi_utility.c index 519954eea..51f2d89a6 100644 --- a/src/backend/distributed/executor/multi_utility.c +++ b/src/backend/distributed/executor/multi_utility.c @@ -155,6 +155,7 @@ static void ErrorIfUnsupportedForeignConstraint(Relation relation, static char * ExtractNewExtensionVersion(Node *parsetree); static void CreateLocalTable(RangeVar *relation, char *nodeName, int32 nodePort); static bool IsAlterTableRenameStmt(RenameStmt *renameStmt); +static bool IsIndexRenameStmt(RenameStmt *renameStmt); static bool AlterInvolvesPartitionColumn(AlterTableStmt *alterTableStatement, AlterTableCmd *command); static void ExecuteDistributedDDLJob(DDLJob *ddlJob); @@ -1371,11 +1372,17 @@ PlanAlterTableStmt(AlterTableStmt *alterTableStatement, const char *alterTableCo static List * PlanRenameStmt(RenameStmt *renameStmt, const char *renameCommand) { - Oid relationId = InvalidOid; + Oid objectRelationId = InvalidOid; /* SQL Object OID */ + Oid tableRelationId = InvalidOid; /* Relation OID, maybe not the same. */ bool isDistributedRelation = false; DDLJob *ddlJob = NULL; - if (!IsAlterTableRenameStmt(renameStmt)) + /* + * We only support some of the PostgreSQL supported RENAME statements, and + * our list include only renaming table and index (related) objects. + */ + if (!IsAlterTableRenameStmt(renameStmt) && + !IsIndexRenameStmt(renameStmt)) { return NIL; } @@ -1385,32 +1392,70 @@ PlanRenameStmt(RenameStmt *renameStmt, const char *renameCommand) * RenameRelation(), renameatt() and RenameConstraint(). However, since all * three statements have identical lock levels, we just use a single statement. */ - relationId = RangeVarGetRelid(renameStmt->relation, AccessExclusiveLock, - renameStmt->missing_ok); + objectRelationId = RangeVarGetRelid(renameStmt->relation, + AccessExclusiveLock, + renameStmt->missing_ok); /* * If the table does not exist, don't do anything here to allow PostgreSQL * to throw the appropriate error or notice message later. */ - if (!OidIsValid(relationId)) + if (!OidIsValid(objectRelationId)) { return NIL; } /* we have no planning to do unless the table is distributed */ - isDistributedRelation = IsDistributedTable(relationId); + switch (renameStmt->renameType) + { + case OBJECT_TABLE: + case OBJECT_COLUMN: + case OBJECT_TABCONSTRAINT: + { + /* the target object is our tableRelationId. */ + tableRelationId = objectRelationId; + break; + } + + case OBJECT_INDEX: + { + /* + * here, objRelationId points to the index relation entry, and we + * are interested into the entry of the table on which the index is + * defined. + */ + tableRelationId = IndexGetRelation(objectRelationId, false); + break; + } + + default: + + /* + * Nodes that are not supported by Citus: we pass-through to the + * main PostgreSQL executor. Any Citus-supported RenameStmt + * renameType must appear above in the switch, explicitly. + */ + return NIL; + } + + isDistributedRelation = IsDistributedTable(tableRelationId); if (!isDistributedRelation) { return NIL; } + /* + * We might ERROR out on some commands, but only for Citus tables where + * isDistributedRelation is true. That's why this test comes this late in + * the function. + */ ErrorIfUnsupportedRenameStmt(renameStmt); ddlJob = palloc0(sizeof(DDLJob)); - ddlJob->targetRelationId = relationId; + ddlJob->targetRelationId = tableRelationId; ddlJob->concurrentIndexCmd = false; ddlJob->commandString = renameCommand; - ddlJob->taskList = DDLTaskList(relationId, renameCommand); + ddlJob->taskList = DDLTaskList(tableRelationId, renameCommand); return list_make1(ddlJob); } @@ -2733,14 +2778,14 @@ OptionsSpecifyOwnedBy(List *optionList, Oid *ownedByTableId) * ErrorIfDistributedRenameStmt errors out if the corresponding rename statement * operates on any part of a distributed table other than a column. * - * Note: This function handles only those rename statements which operate on tables. + * Note: This function handles RenameStmt applied to relations handed by Citus. + * At the moment of writing this comment, it could be either tables or indexes. */ static void ErrorIfUnsupportedRenameStmt(RenameStmt *renameStmt) { - Assert(IsAlterTableRenameStmt(renameStmt)); - - if (renameStmt->renameType == OBJECT_TABCONSTRAINT) + if (IsAlterTableRenameStmt(renameStmt) && + renameStmt->renameType == OBJECT_TABCONSTRAINT) { ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("renaming constraints belonging to distributed tables is " @@ -2860,6 +2905,26 @@ IsAlterTableRenameStmt(RenameStmt *renameStmt) } +/* + * IsIndexRenameStmt returns whether the passed-in RenameStmt is the following + * form: + * + * - ALTER INDEX RENAME + */ +static bool +IsIndexRenameStmt(RenameStmt *renameStmt) +{ + bool isIndexRenameStmt = false; + + if (renameStmt->renameType == OBJECT_INDEX) + { + isIndexRenameStmt = true; + } + + return isIndexRenameStmt; +} + + /* * AlterInvolvesPartitionColumn checks if the given alter table command * involves relation's partition column. diff --git a/src/test/regress/input/multi_alter_table_statements.source b/src/test/regress/input/multi_alter_table_statements.source index 2cefeaf88..ababf5119 100644 --- a/src/test/regress/input/multi_alter_table_statements.source +++ b/src/test/regress/input/multi_alter_table_statements.source @@ -375,6 +375,21 @@ ALTER TABLE lineitem_renamed RENAME TO lineitem_alter; SELECT relname FROM pg_class WHERE relname LIKE 'lineitem_alter%' ORDER BY relname; \c - - - :master_port +-- verify that we can rename indexes on distributed tables +CREATE INDEX temp_index_1 ON lineitem_alter(l_linenumber); +ALTER INDEX temp_index_1 RENAME TO idx_lineitem_linenumber; + +-- verify rename is performed +SELECT relname FROM pg_class WHERE relname = 'idx_lineitem_linenumber'; + +-- show rename worked on one worker, too +\c - - - :worker_1_port +SELECT relname FROM pg_class WHERE relname LIKE 'idx_lineitem_linenumber%%' ORDER BY relname; +\c - - - :master_port + +-- now get rid of the index +DROP INDEX idx_lineitem_linenumber; + -- verify that we don't intercept DDL commands if propagation is turned off SET citus.enable_ddl_propagation to false; diff --git a/src/test/regress/output/multi_alter_table_statements.source b/src/test/regress/output/multi_alter_table_statements.source index 49017ff2e..b00f53ca0 100644 --- a/src/test/regress/output/multi_alter_table_statements.source +++ b/src/test/regress/output/multi_alter_table_statements.source @@ -844,6 +844,40 @@ SELECT relname FROM pg_class WHERE relname LIKE 'lineitem_alter%' ORDER BY relna (15 rows) \c - - - :master_port +-- verify that we can rename indexes on distributed tables +CREATE INDEX temp_index_1 ON lineitem_alter(l_linenumber); +ALTER INDEX temp_index_1 RENAME TO idx_lineitem_linenumber; +-- verify rename is performed +SELECT relname FROM pg_class WHERE relname = 'idx_lineitem_linenumber'; + relname +------------------------- + idx_lineitem_linenumber +(1 row) + +-- show rename worked on one worker, too +\c - - - :worker_1_port +SELECT relname FROM pg_class WHERE relname LIKE 'idx_lineitem_linenumber%%' ORDER BY relname; + relname +-------------------------------- + idx_lineitem_linenumber_220000 + idx_lineitem_linenumber_220001 + idx_lineitem_linenumber_220002 + idx_lineitem_linenumber_220003 + idx_lineitem_linenumber_220004 + idx_lineitem_linenumber_220005 + idx_lineitem_linenumber_220006 + idx_lineitem_linenumber_220007 + idx_lineitem_linenumber_220008 + idx_lineitem_linenumber_220010 + idx_lineitem_linenumber_220011 + idx_lineitem_linenumber_220012 + idx_lineitem_linenumber_220013 + idx_lineitem_linenumber_220014 +(14 rows) + +\c - - - :master_port +-- now get rid of the index +DROP INDEX idx_lineitem_linenumber; -- verify that we don't intercept DDL commands if propagation is turned off SET citus.enable_ddl_propagation to false; -- table rename statement can be performed on the coordinator only now From 952da72c55d6919d72efb93001aaa4fa29aa8758 Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Wed, 17 Jan 2018 20:50:17 +0100 Subject: [PATCH 2/3] Implement ALTER TABLE|INDEX ... SET|RESET (). PostgreSQL implements support for several relation kinds in a single statement, such as in the AlterTableStmt case, which supports both tables and indexes and more (see ATExecSetRelOptions in PostgreSQL source code file src/backend/commands/tablecmds.c for an example of that). As a consequence, this patch implements support for setting and resetting storage parameters on both relation kinds. --- .../distributed/executor/multi_utility.c | 109 ++++++++++-- .../distributed/utils/citus_ruleutils.c | 143 +++++++++++++++- .../expected/multi_reference_table.out | 2 +- .../input/multi_alter_table_statements.source | 58 ++++++- .../multi_alter_table_statements.source | 158 +++++++++++++++++- 5 files changed, 446 insertions(+), 24 deletions(-) diff --git a/src/backend/distributed/executor/multi_utility.c b/src/backend/distributed/executor/multi_utility.c index 51f2d89a6..363418372 100644 --- a/src/backend/distributed/executor/multi_utility.c +++ b/src/backend/distributed/executor/multi_utility.c @@ -139,6 +139,7 @@ static void ErrorIfUnstableCreateOrAlterExtensionStmt(Node *parsetree); static void ErrorIfUnsupportedIndexStmt(IndexStmt *createIndexStatement); static void ErrorIfUnsupportedDropIndexStmt(DropStmt *dropIndexStatement); static void ErrorIfUnsupportedAlterTableStmt(AlterTableStmt *alterTableStatement); +static void ErrorIfUnsupportedAlterIndexStmt(AlterTableStmt *alterTableStatement); static void ErrorIfAlterDropsPartitionColumn(AlterTableStmt *alterTableStatement); static void ErrorIfUnsupportedSeqStmt(CreateSeqStmt *createSeqStmt); static void ErrorIfDistributedAlterSeqOwnedBy(AlterSeqStmt *alterSeqStmt); @@ -366,7 +367,8 @@ multi_ProcessUtility(PlannedStmt *pstmt, if (IsA(parsetree, AlterTableStmt)) { AlterTableStmt *alterTableStmt = (AlterTableStmt *) parsetree; - if (alterTableStmt->relkind == OBJECT_TABLE) + if (alterTableStmt->relkind == OBJECT_TABLE || + alterTableStmt->relkind == OBJECT_INDEX) { ddlJobs = PlanAlterTableStmt(alterTableStmt, queryString); } @@ -1238,6 +1240,7 @@ PlanAlterTableStmt(AlterTableStmt *alterTableStatement, const char *alterTableCo LOCKMODE lockmode = 0; Oid leftRelationId = InvalidOid; Oid rightRelationId = InvalidOid; + char leftRelationKind; bool isDistributedRelation = false; List *commandList = NIL; ListCell *commandCell = NULL; @@ -1255,13 +1258,38 @@ PlanAlterTableStmt(AlterTableStmt *alterTableStatement, const char *alterTableCo return NIL; } + /* + * AlterTableStmt applies also to INDEX relations, and we have support for + * SET/SET storage parameters in Citus, so we might have to check for + * another relation here. + */ + leftRelationKind = get_rel_relkind(leftRelationId); + if (leftRelationKind == RELKIND_INDEX) + { + leftRelationId = IndexGetRelation(leftRelationId, false); + } + isDistributedRelation = IsDistributedTable(leftRelationId); if (!isDistributedRelation) { return NIL; } - ErrorIfUnsupportedAlterTableStmt(alterTableStatement); + /* + * The PostgreSQL parser dispatches several commands into the node type + * AlterTableStmt, from ALTER INDEX to ALTER SEQUENCE or ALTER VIEW. Here + * we have a special implementation for ALTER INDEX, and a specific error + * message in case of unsupported sub-command. + */ + if (leftRelationKind == RELKIND_INDEX) + { + ErrorIfUnsupportedAlterIndexStmt(alterTableStatement); + } + else + { + /* this function also accepts more than just RELKIND_RELATION... */ + ErrorIfUnsupportedAlterTableStmt(alterTableStatement); + } /* * We check if there is a ADD FOREIGN CONSTRAINT command in sub commands list. @@ -2011,9 +2039,9 @@ ErrorIfUnsupportedDropIndexStmt(DropStmt *dropIndexStatement) /* - * ErrorIfUnsupportedAlterTableStmt checks if the corresponding alter table statement - * is supported for distributed tables and errors out if it is not. Currently, - * only the following commands are supported. + * ErrorIfUnsupportedAlterTableStmt checks if the corresponding alter table + * statement is supported for distributed tables and errors out if it is not. + * Currently, only the following commands are supported. * * ALTER TABLE ADD|DROP COLUMN * ALTER TABLE ALTER COLUMN SET DATA TYPE @@ -2021,6 +2049,8 @@ ErrorIfUnsupportedDropIndexStmt(DropStmt *dropIndexStatement) * ALTER TABLE SET|DROP DEFAULT * ALTER TABLE ADD|DROP CONSTRAINT * ALTER TABLE REPLICA IDENTITY + * ALTER TABLE SET () + * ALTER TABLE RESET () */ static void ErrorIfUnsupportedAlterTableStmt(AlterTableStmt *alterTableStatement) @@ -2170,14 +2200,71 @@ ErrorIfUnsupportedAlterTableStmt(AlterTableStmt *alterTableStatement) break; } + case AT_SetRelOptions: /* SET (...) */ + case AT_ResetRelOptions: /* RESET (...) */ + case AT_ReplaceRelOptions: /* replace entire option list */ + { + /* this command is supported by Citus */ + break; + } + default: { - ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), - errmsg("alter table command is currently unsupported"), - errdetail("Only ADD|DROP COLUMN, SET|DROP NOT NULL, " - "SET|DROP DEFAULT, ADD|DROP CONSTRAINT, " - "ATTACH|DETACH PARTITION and TYPE subcommands " - "are supported."))); + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("alter table command is currently unsupported"), + errdetail("Only ADD|DROP COLUMN, SET|DROP NOT NULL, " + "SET|DROP DEFAULT, ADD|DROP CONSTRAINT, " + "SET (), RESET (), " + "ATTACH|DETACH PARTITION and TYPE subcommands " + "are supported."))); + } + } + } +} + + +/* + * ErrorIfUnsupportedAlterIndexStmt checks if the corresponding alter index + * statement is supported for distributed tables and errors out if it is not. + * Currently, only the following commands are supported. + * + * ALTER INDEX SET () + * ALTER INDEX RESET () + */ +static void +ErrorIfUnsupportedAlterIndexStmt(AlterTableStmt *alterTableStatement) +{ + List *commandList = alterTableStatement->cmds; + ListCell *commandCell = NULL; + + /* error out if any of the subcommands are unsupported */ + foreach(commandCell, commandList) + { + AlterTableCmd *command = (AlterTableCmd *) lfirst(commandCell); + AlterTableType alterTableType = command->subtype; + + switch (alterTableType) + { + case AT_SetRelOptions: /* SET (...) */ + case AT_ResetRelOptions: /* RESET (...) */ + case AT_ReplaceRelOptions: /* replace entire option list */ + { + /* this command is supported by Citus */ + break; + } + + /* unsupported create index statements */ + case AT_SetTableSpace: + default: + { + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("alter index ... set tablespace ... " + "is currently unsupported"), + errdetail("Only RENAME TO, SET (), and RESET () " + "are supported."))); + return; /* keep compiler happy */ } } } diff --git a/src/backend/distributed/utils/citus_ruleutils.c b/src/backend/distributed/utils/citus_ruleutils.c index 3dbe27801..f4526207a 100644 --- a/src/backend/distributed/utils/citus_ruleutils.c +++ b/src/backend/distributed/utils/citus_ruleutils.c @@ -31,6 +31,7 @@ #include "catalog/pg_extension.h" #include "catalog/pg_foreign_data_wrapper.h" #include "catalog/pg_index.h" +#include "catalog/pg_type.h" #include "commands/defrem.h" #include "commands/extension.h" #include "distributed/citus_ruleutils.h" @@ -45,6 +46,7 @@ #include "nodes/parsenodes.h" #include "nodes/pg_list.h" #include "parser/parse_utilcmd.h" +#include "parser/parser.h" #include "storage/lock.h" #include "utils/acl.h" #include "utils/array.h" @@ -62,7 +64,8 @@ static void AppendOptionListToString(StringInfo stringData, List *options); static const char * convert_aclright_to_string(int aclright); - +static void simple_quote_literal(StringInfo buf, const char *val); +static char * flatten_reloptions(Oid relid); /* * pg_get_extensiondef_string finds the foreign data wrapper that corresponds to @@ -474,6 +477,20 @@ pg_get_tableschemadef_string(Oid tableRelationId, bool includeSequenceDefaults) appendStringInfo(&buffer, " PARTITION BY %s ", partitioningInformation); } #endif + + /* + * Add any reloptions (storage parameters) defined on the table in a WITH + * clause. + */ + { + char *reloptions = flatten_reloptions(tableRelationId); + if (reloptions) + { + appendStringInfo(&buffer, " WITH (%s)", reloptions); + pfree(reloptions); + } + } + relation_close(relation, AccessShareLock); return (buffer.data); @@ -1112,3 +1129,127 @@ pg_get_replica_identity_command(Oid tableRelationId) return (buf->len > 0) ? buf->data : NULL; } + + +/* + * Generate a C string representing a relation's reloptions, or NULL if none. + * + * This function comes from PostgreSQL source code in + * src/backend/utils/adt/ruleutils.c + */ +static char * +flatten_reloptions(Oid relid) +{ + char *result = NULL; + HeapTuple tuple; + Datum reloptions; + bool isnull; + + tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(relid)); + if (!HeapTupleIsValid(tuple)) + { + elog(ERROR, "cache lookup failed for relation %u", relid); + } + + reloptions = SysCacheGetAttr(RELOID, tuple, + Anum_pg_class_reloptions, &isnull); + if (!isnull) + { + StringInfoData buf; + Datum *options; + int noptions; + int i; + + initStringInfo(&buf); + + deconstruct_array(DatumGetArrayTypeP(reloptions), + TEXTOID, -1, false, 'i', + &options, NULL, &noptions); + + for (i = 0; i < noptions; i++) + { + char *option = TextDatumGetCString(options[i]); + char *name; + char *separator; + char *value; + + /* + * Each array element should have the form name=value. If the "=" + * is missing for some reason, treat it like an empty value. + */ + name = option; + separator = strchr(option, '='); + if (separator) + { + *separator = '\0'; + value = separator + 1; + } + else + { + value = ""; + } + + if (i > 0) + { + appendStringInfoString(&buf, ", "); + } + appendStringInfo(&buf, "%s=", quote_identifier(name)); + + /* + * In general we need to quote the value; but to avoid unnecessary + * clutter, do not quote if it is an identifier that would not + * need quoting. (We could also allow numbers, but that is a bit + * trickier than it looks --- for example, are leading zeroes + * significant? We don't want to assume very much here about what + * custom reloptions might mean.) + */ + if (quote_identifier(value) == value) + { + appendStringInfoString(&buf, value); + } + else + { + simple_quote_literal(&buf, value); + } + + pfree(option); + } + + result = buf.data; + } + + ReleaseSysCache(tuple); + + return result; +} + + +/* + * simple_quote_literal - Format a string as a SQL literal, append to buf + * + * This function comes from PostgreSQL source code in + * src/backend/utils/adt/ruleutils.c + */ +static void +simple_quote_literal(StringInfo buf, const char *val) +{ + const char *valptr; + + /* + * We form the string literal according to the prevailing setting of + * standard_conforming_strings; we never use E''. User is responsible for + * making sure result is used correctly. + */ + appendStringInfoChar(buf, '\''); + for (valptr = val; *valptr; valptr++) + { + char ch = *valptr; + + if (SQL_STR_DOUBLE(ch, !standard_conforming_strings)) + { + appendStringInfoChar(buf, ch); + } + appendStringInfoChar(buf, ch); + } + appendStringInfoChar(buf, '\''); +} diff --git a/src/test/regress/expected/multi_reference_table.out b/src/test/regress/expected/multi_reference_table.out index 74ca5cd2f..f6ec7cc13 100644 --- a/src/test/regress/expected/multi_reference_table.out +++ b/src/test/regress/expected/multi_reference_table.out @@ -1385,7 +1385,7 @@ SELECT "Column", "Type", "Modifiers" FROM table_desc WHERE relid='reference_sche -- as we expect, setting WITH OIDS does not work for reference tables ALTER TABLE reference_schema.reference_table_ddl SET WITH OIDS; ERROR: alter table command is currently unsupported -DETAIL: Only ADD|DROP COLUMN, SET|DROP NOT NULL, SET|DROP DEFAULT, ADD|DROP CONSTRAINT, ATTACH|DETACH PARTITION and TYPE subcommands are supported. +DETAIL: Only ADD|DROP COLUMN, SET|DROP NOT NULL, SET|DROP DEFAULT, ADD|DROP CONSTRAINT, SET (), RESET (), ATTACH|DETACH PARTITION and TYPE subcommands are supported. -- now test the renaming of the table, and back to the expected name ALTER TABLE reference_schema.reference_table_ddl RENAME TO reference_table_ddl_test; ALTER TABLE reference_schema.reference_table_ddl_test RENAME TO reference_table_ddl; diff --git a/src/test/regress/input/multi_alter_table_statements.source b/src/test/regress/input/multi_alter_table_statements.source index ababf5119..bba59420d 100644 --- a/src/test/regress/input/multi_alter_table_statements.source +++ b/src/test/regress/input/multi_alter_table_statements.source @@ -26,10 +26,18 @@ CREATE TABLE lineitem_alter ( l_shipinstruct char(25) not null, l_shipmode char(10) not null, l_comment varchar(44) not null - ); + ) + WITH ( fillfactor = 80 ); SELECT master_create_distributed_table('lineitem_alter', 'l_orderkey', 'append'); \copy lineitem_alter FROM '@abs_srcdir@/data/lineitem.1.data' with delimiter '|' +-- verify that the storage options made it to the table definitions +SELECT relname, reloptions FROM pg_class WHERE relname = 'lineitem_alter'; + +\c - - - :worker_1_port +SELECT relname, reloptions FROM pg_class WHERE relname LIKE 'lineitem_alter%' ORDER BY relname; +\c - - - :master_port + -- Verify that we can add columns ALTER TABLE lineitem_alter ADD COLUMN float_column FLOAT; @@ -372,7 +380,22 @@ ALTER TABLE lineitem_renamed RENAME TO lineitem_alter; -- show rename worked on one worker, too \c - - - :worker_1_port -SELECT relname FROM pg_class WHERE relname LIKE 'lineitem_alter%' ORDER BY relname; +SELECT relname FROM pg_class WHERE relname LIKE 'lineitem_alter%' AND relname <> 'lineitem_alter_220009' /* failed copy trails */ ORDER BY relname; +\c - - - :master_port + +-- verify that we can set and reset storage parameters +ALTER TABLE lineitem_alter SET(fillfactor=40); +SELECT relname, reloptions FROM pg_class WHERE relname = 'lineitem_alter'; + +\c - - - :worker_1_port +SELECT relname, reloptions FROM pg_class WHERE relname LIKE 'lineitem_alter%' AND relname <> 'lineitem_alter_220009' /* failed copy trails */ ORDER BY relname; +\c - - - :master_port + +ALTER TABLE lineitem_alter RESET(fillfactor); +SELECT relname, reloptions FROM pg_class WHERE relname = 'lineitem_alter'; + +\c - - - :worker_1_port +SELECT relname, reloptions FROM pg_class WHERE relname LIKE 'lineitem_alter%' AND relname <> 'lineitem_alter_220009' /* failed copy trails */ ORDER BY relname; \c - - - :master_port -- verify that we can rename indexes on distributed tables @@ -384,7 +407,7 @@ SELECT relname FROM pg_class WHERE relname = 'idx_lineitem_linenumber'; -- show rename worked on one worker, too \c - - - :worker_1_port -SELECT relname FROM pg_class WHERE relname LIKE 'idx_lineitem_linenumber%%' ORDER BY relname; +SELECT relname FROM pg_class WHERE relname LIKE 'idx_lineitem_linenumber%' ORDER BY relname; \c - - - :master_port -- now get rid of the index @@ -517,3 +540,32 @@ END; \c - - - :worker_1_port SELECT relname FROM pg_class WHERE relname LIKE 'test_table_1%'; \c - - - :master_port + +-- Test WITH options on a normal simple hash-distributed table +CREATE TABLE hash_dist(id bigint primary key, f1 text) WITH (fillfactor=40); +SELECT create_distributed_table('hash_dist','id'); + +-- verify that the storage options made it to the table definitions +SELECT relname, reloptions FROM pg_class WHERE relname = 'hash_dist'; + +\c - - - :worker_1_port +SELECT relname, reloptions FROM pg_class WHERE relkind = 'r' AND relname LIKE 'hash_dist%' ORDER BY relname; +\c - - - :master_port + +-- verify that we can set and reset index storage parameters +ALTER INDEX hash_dist_pkey SET(fillfactor=40); +SELECT relname, reloptions FROM pg_class WHERE relname = 'hash_dist_pkey'; + +\c - - - :worker_1_port +SELECT relname, reloptions FROM pg_class WHERE relname LIKE 'hash_dist_pkey%' ORDER BY relname; +\c - - - :master_port + +ALTER INDEX hash_dist_pkey RESET(fillfactor); +SELECT relname, reloptions FROM pg_class WHERE relname = 'hash_dist_pkey'; + +\c - - - :worker_1_port +SELECT relname, reloptions FROM pg_class WHERE relname LIKE 'hash_dist_pkey%' ORDER BY relname; +\c - - - :master_port + +-- verify error message on ALTER INDEX, SET TABLESPACE is unsupported +ALTER INDEX hash_dist_pkey SET TABLESPACE foo; diff --git a/src/test/regress/output/multi_alter_table_statements.source b/src/test/regress/output/multi_alter_table_statements.source index b00f53ca0..12c57939f 100644 --- a/src/test/regress/output/multi_alter_table_statements.source +++ b/src/test/regress/output/multi_alter_table_statements.source @@ -22,7 +22,8 @@ CREATE TABLE lineitem_alter ( l_shipinstruct char(25) not null, l_shipmode char(10) not null, l_comment varchar(44) not null - ); + ) + WITH ( fillfactor = 80 ); SELECT master_create_distributed_table('lineitem_alter', 'l_orderkey', 'append'); master_create_distributed_table --------------------------------- @@ -30,6 +31,24 @@ SELECT master_create_distributed_table('lineitem_alter', 'l_orderkey', 'append') (1 row) \copy lineitem_alter FROM '@abs_srcdir@/data/lineitem.1.data' with delimiter '|' +-- verify that the storage options made it to the table definitions +SELECT relname, reloptions FROM pg_class WHERE relname = 'lineitem_alter'; + relname | reloptions +----------------+----------------- + lineitem_alter | {fillfactor=80} +(1 row) + +\c - - - :worker_1_port +SELECT relname, reloptions FROM pg_class WHERE relname LIKE 'lineitem_alter%' ORDER BY relname; + relname | reloptions +-----------------------+----------------- + lineitem_alter_220000 | {fillfactor=80} + lineitem_alter_220001 | {fillfactor=80} + lineitem_alter_220002 | {fillfactor=80} + lineitem_alter_220003 | {fillfactor=80} +(4 rows) + +\c - - - :master_port -- Verify that we can add columns ALTER TABLE lineitem_alter ADD COLUMN float_column FLOAT; ALTER TABLE lineitem_alter ADD COLUMN date_column DATE; @@ -328,7 +347,7 @@ SELECT "Column", "Type", "Modifiers" FROM table_desc WHERE relid='public.lineite ALTER TABLE lineitem_alter ADD COLUMN int_column3 INTEGER, ALTER COLUMN int_column1 SET STATISTICS 10; ERROR: alter table command is currently unsupported -DETAIL: Only ADD|DROP COLUMN, SET|DROP NOT NULL, SET|DROP DEFAULT, ADD|DROP CONSTRAINT, ATTACH|DETACH PARTITION and TYPE subcommands are supported. +DETAIL: Only ADD|DROP COLUMN, SET|DROP NOT NULL, SET|DROP DEFAULT, ADD|DROP CONSTRAINT, SET (), RESET (), ATTACH|DETACH PARTITION and TYPE subcommands are supported. ALTER TABLE lineitem_alter DROP COLUMN int_column1, DROP COLUMN int_column2; SELECT "Column", "Type", "Modifiers" FROM table_desc WHERE relid='public.lineitem_alter'::regclass; Column | Type | Modifiers @@ -360,12 +379,12 @@ ERROR: cannot execute ALTER TABLE command involving partition column -- Verify that we error out on unsupported statement types ALTER TABLE lineitem_alter ALTER COLUMN l_orderkey SET STATISTICS 100; ERROR: alter table command is currently unsupported -DETAIL: Only ADD|DROP COLUMN, SET|DROP NOT NULL, SET|DROP DEFAULT, ADD|DROP CONSTRAINT, ATTACH|DETACH PARTITION and TYPE subcommands are supported. +DETAIL: Only ADD|DROP COLUMN, SET|DROP NOT NULL, SET|DROP DEFAULT, ADD|DROP CONSTRAINT, SET (), RESET (), ATTACH|DETACH PARTITION and TYPE subcommands are supported. ALTER TABLE lineitem_alter DROP CONSTRAINT IF EXISTS non_existent_contraint; NOTICE: constraint "non_existent_contraint" of relation "lineitem_alter" does not exist, skipping ALTER TABLE lineitem_alter SET WITHOUT OIDS; ERROR: alter table command is currently unsupported -DETAIL: Only ADD|DROP COLUMN, SET|DROP NOT NULL, SET|DROP DEFAULT, ADD|DROP CONSTRAINT, ATTACH|DETACH PARTITION and TYPE subcommands are supported. +DETAIL: Only ADD|DROP COLUMN, SET|DROP NOT NULL, SET|DROP DEFAULT, ADD|DROP CONSTRAINT, SET (), RESET (), ATTACH|DETACH PARTITION and TYPE subcommands are supported. -- Verify that we error out in case of postgres errors on supported statement -- types ALTER TABLE lineitem_alter ADD COLUMN new_column non_existent_type; @@ -823,7 +842,7 @@ SELECT relname FROM pg_class WHERE relname LIKE 'lineitem_renamed%' ORDER BY re ALTER TABLE lineitem_renamed RENAME TO lineitem_alter; -- show rename worked on one worker, too \c - - - :worker_1_port -SELECT relname FROM pg_class WHERE relname LIKE 'lineitem_alter%' ORDER BY relname; +SELECT relname FROM pg_class WHERE relname LIKE 'lineitem_alter%' AND relname <> 'lineitem_alter_220009' /* failed copy trails */ ORDER BY relname; relname ----------------------- lineitem_alter_220000 @@ -835,13 +854,69 @@ SELECT relname FROM pg_class WHERE relname LIKE 'lineitem_alter%' ORDER BY relna lineitem_alter_220006 lineitem_alter_220007 lineitem_alter_220008 - lineitem_alter_220009 lineitem_alter_220010 lineitem_alter_220011 lineitem_alter_220012 lineitem_alter_220013 lineitem_alter_220014 -(15 rows) +(14 rows) + +\c - - - :master_port +-- verify that we can set and reset storage parameters +ALTER TABLE lineitem_alter SET(fillfactor=40); +SELECT relname, reloptions FROM pg_class WHERE relname = 'lineitem_alter'; + relname | reloptions +----------------+----------------- + lineitem_alter | {fillfactor=40} +(1 row) + +\c - - - :worker_1_port +SELECT relname, reloptions FROM pg_class WHERE relname LIKE 'lineitem_alter%' AND relname <> 'lineitem_alter_220009' /* failed copy trails */ ORDER BY relname; + relname | reloptions +-----------------------+----------------- + lineitem_alter_220000 | {fillfactor=40} + lineitem_alter_220001 | {fillfactor=40} + lineitem_alter_220002 | {fillfactor=40} + lineitem_alter_220003 | {fillfactor=40} + lineitem_alter_220004 | {fillfactor=40} + lineitem_alter_220005 | {fillfactor=40} + lineitem_alter_220006 | {fillfactor=40} + lineitem_alter_220007 | {fillfactor=40} + lineitem_alter_220008 | {fillfactor=40} + lineitem_alter_220010 | {fillfactor=40} + lineitem_alter_220011 | {fillfactor=40} + lineitem_alter_220012 | {fillfactor=40} + lineitem_alter_220013 | {fillfactor=40} + lineitem_alter_220014 | {fillfactor=40} +(14 rows) + +\c - - - :master_port +ALTER TABLE lineitem_alter RESET(fillfactor); +SELECT relname, reloptions FROM pg_class WHERE relname = 'lineitem_alter'; + relname | reloptions +----------------+------------ + lineitem_alter | +(1 row) + +\c - - - :worker_1_port +SELECT relname, reloptions FROM pg_class WHERE relname LIKE 'lineitem_alter%' AND relname <> 'lineitem_alter_220009' /* failed copy trails */ ORDER BY relname; + relname | reloptions +-----------------------+------------ + lineitem_alter_220000 | + lineitem_alter_220001 | + lineitem_alter_220002 | + lineitem_alter_220003 | + lineitem_alter_220004 | + lineitem_alter_220005 | + lineitem_alter_220006 | + lineitem_alter_220007 | + lineitem_alter_220008 | + lineitem_alter_220010 | + lineitem_alter_220011 | + lineitem_alter_220012 | + lineitem_alter_220013 | + lineitem_alter_220014 | +(14 rows) \c - - - :master_port -- verify that we can rename indexes on distributed tables @@ -856,7 +931,7 @@ SELECT relname FROM pg_class WHERE relname = 'idx_lineitem_linenumber'; -- show rename worked on one worker, too \c - - - :worker_1_port -SELECT relname FROM pg_class WHERE relname LIKE 'idx_lineitem_linenumber%%' ORDER BY relname; +SELECT relname FROM pg_class WHERE relname LIKE 'idx_lineitem_linenumber%' ORDER BY relname; relname -------------------------------- idx_lineitem_linenumber_220000 @@ -1055,3 +1130,70 @@ SELECT relname FROM pg_class WHERE relname LIKE 'test_table_1%'; (0 rows) \c - - - :master_port +-- Test WITH options on a normal simple hash-distributed table +CREATE TABLE hash_dist(id bigint primary key, f1 text) WITH (fillfactor=40); +SELECT create_distributed_table('hash_dist','id'); + create_distributed_table +-------------------------- + +(1 row) + +-- verify that the storage options made it to the table definitions +SELECT relname, reloptions FROM pg_class WHERE relname = 'hash_dist'; + relname | reloptions +-----------+----------------- + hash_dist | {fillfactor=40} +(1 row) + +\c - - - :worker_1_port +SELECT relname, reloptions FROM pg_class WHERE relkind = 'r' AND relname LIKE 'hash_dist%' ORDER BY relname; + relname | reloptions +------------------+----------------- + hash_dist_220033 | {fillfactor=40} + hash_dist_220034 | {fillfactor=40} + hash_dist_220035 | {fillfactor=40} + hash_dist_220036 | {fillfactor=40} +(4 rows) + +\c - - - :master_port +-- verify that we can set and reset index storage parameters +ALTER INDEX hash_dist_pkey SET(fillfactor=40); +SELECT relname, reloptions FROM pg_class WHERE relname = 'hash_dist_pkey'; + relname | reloptions +----------------+----------------- + hash_dist_pkey | {fillfactor=40} +(1 row) + +\c - - - :worker_1_port +SELECT relname, reloptions FROM pg_class WHERE relname LIKE 'hash_dist_pkey%' ORDER BY relname; + relname | reloptions +-----------------------+----------------- + hash_dist_pkey_220033 | {fillfactor=40} + hash_dist_pkey_220034 | {fillfactor=40} + hash_dist_pkey_220035 | {fillfactor=40} + hash_dist_pkey_220036 | {fillfactor=40} +(4 rows) + +\c - - - :master_port +ALTER INDEX hash_dist_pkey RESET(fillfactor); +SELECT relname, reloptions FROM pg_class WHERE relname = 'hash_dist_pkey'; + relname | reloptions +----------------+------------ + hash_dist_pkey | +(1 row) + +\c - - - :worker_1_port +SELECT relname, reloptions FROM pg_class WHERE relname LIKE 'hash_dist_pkey%' ORDER BY relname; + relname | reloptions +-----------------------+------------ + hash_dist_pkey_220033 | + hash_dist_pkey_220034 | + hash_dist_pkey_220035 | + hash_dist_pkey_220036 | +(4 rows) + +\c - - - :master_port +-- verify error message on ALTER INDEX, SET TABLESPACE is unsupported +ALTER INDEX hash_dist_pkey SET TABLESPACE foo; +ERROR: alter index ... set tablespace ... is currently unsupported +DETAIL: Only RENAME TO, SET (), and RESET () are supported. From c9760fbb64e5c08913b682c9d25822721f0693fc Mon Sep 17 00:00:00 2001 From: Dimitri Fontaine Date: Fri, 12 Jan 2018 22:27:40 +0100 Subject: [PATCH 3/3] Fix CREATE INDEX with storage options on distributed tables. By sharing the implementation of the function AppendOptionListToString on three call sites, we would expand an extra OPTIONS keyword in a create index statement, and omit other bits of the specific syntax here. This patch introduces an AppendStorageParametersToString() function that is very similar to AppendOptionListToString() but handles WITH(a="foo",...) syntax that is used in reloptions (aka Storage Parameters). Fixes #1747. --- .../distributed/utils/citus_ruleutils.c | 46 +++++++++++++++++-- .../input/multi_alter_table_statements.source | 13 ++++++ .../multi_alter_table_statements.source | 22 +++++++++ 3 files changed, 76 insertions(+), 5 deletions(-) diff --git a/src/backend/distributed/utils/citus_ruleutils.c b/src/backend/distributed/utils/citus_ruleutils.c index f4526207a..d8adea88b 100644 --- a/src/backend/distributed/utils/citus_ruleutils.c +++ b/src/backend/distributed/utils/citus_ruleutils.c @@ -63,6 +63,8 @@ static void AppendOptionListToString(StringInfo stringData, List *options); +static void AppendStorageParametersToString(StringInfo stringBuffer, + List *optionList); static const char * convert_aclright_to_string(int aclright); static void simple_quote_literal(StringInfo buf, const char *val); static char * flatten_reloptions(Oid relid); @@ -755,11 +757,7 @@ deparse_shard_index_statement(IndexStmt *origStmt, Oid distrelid, int64 shardid, appendStringInfoString(buffer, ") "); - if (indexStmt->options != NIL) - { - appendStringInfoString(buffer, "WITH "); - AppendOptionListToString(buffer, indexStmt->options); - } + AppendStorageParametersToString(buffer, indexStmt->options); if (indexStmt->whereClause != NULL) { @@ -1021,6 +1019,44 @@ AppendOptionListToString(StringInfo stringBuffer, List *optionList) } +/* + * AppendStorageParametersToString converts the storage parameter list to its + * textual format, and appends this text to the given string buffer. + */ +static void +AppendStorageParametersToString(StringInfo stringBuffer, List *optionList) +{ + ListCell *optionCell = NULL; + bool firstOptionPrinted = false; + + if (optionList == NIL) + { + return; + } + + appendStringInfo(stringBuffer, " WITH ("); + + foreach(optionCell, optionList) + { + DefElem *option = (DefElem *) lfirst(optionCell); + char *optionName = option->defname; + char *optionValue = defGetString(option); + + if (firstOptionPrinted) + { + appendStringInfo(stringBuffer, ", "); + } + firstOptionPrinted = true; + + appendStringInfo(stringBuffer, "%s = %s ", + quote_identifier(optionName), + quote_literal_cstr(optionValue)); + } + + appendStringInfo(stringBuffer, ")"); +} + + /* copy of postgresql's function, which is static as well */ static const char * convert_aclright_to_string(int aclright) diff --git a/src/test/regress/input/multi_alter_table_statements.source b/src/test/regress/input/multi_alter_table_statements.source index bba59420d..d7da24df4 100644 --- a/src/test/regress/input/multi_alter_table_statements.source +++ b/src/test/regress/input/multi_alter_table_statements.source @@ -569,3 +569,16 @@ SELECT relname, reloptions FROM pg_class WHERE relname LIKE 'hash_dist_pkey%' OR -- verify error message on ALTER INDEX, SET TABLESPACE is unsupported ALTER INDEX hash_dist_pkey SET TABLESPACE foo; + +-- verify that we can add indexes with new storage options +CREATE UNIQUE INDEX another_index ON hash_dist(id) WITH (fillfactor=50); + +-- show the index and its storage options on coordinator, then workers +SELECT relname, reloptions FROM pg_class WHERE relname = 'another_index'; + +\c - - - :worker_1_port +SELECT relname, reloptions FROM pg_class WHERE relname LIKE 'another_index%' ORDER BY relname; +\c - - - :master_port + +-- get rid of the index +DROP INDEX another_index; diff --git a/src/test/regress/output/multi_alter_table_statements.source b/src/test/regress/output/multi_alter_table_statements.source index 12c57939f..790cd2faa 100644 --- a/src/test/regress/output/multi_alter_table_statements.source +++ b/src/test/regress/output/multi_alter_table_statements.source @@ -1197,3 +1197,25 @@ SELECT relname, reloptions FROM pg_class WHERE relname LIKE 'hash_dist_pkey%' OR ALTER INDEX hash_dist_pkey SET TABLESPACE foo; ERROR: alter index ... set tablespace ... is currently unsupported DETAIL: Only RENAME TO, SET (), and RESET () are supported. +-- verify that we can add indexes with new storage options +CREATE UNIQUE INDEX another_index ON hash_dist(id) WITH (fillfactor=50); +-- show the index and its storage options on coordinator, then workers +SELECT relname, reloptions FROM pg_class WHERE relname = 'another_index'; + relname | reloptions +---------------+----------------- + another_index | {fillfactor=50} +(1 row) + +\c - - - :worker_1_port +SELECT relname, reloptions FROM pg_class WHERE relname LIKE 'another_index%' ORDER BY relname; + relname | reloptions +----------------------+----------------- + another_index_220033 | {fillfactor=50} + another_index_220034 | {fillfactor=50} + another_index_220035 | {fillfactor=50} + another_index_220036 | {fillfactor=50} +(4 rows) + +\c - - - :master_port +-- get rid of the index +DROP INDEX another_index;