public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v24 4/5] Add ALTER INDEX ... ALTER COLLATION ... REFRESH VERSION. 2+ messages / 2 participants [nested] [flat]
* [PATCH v24 4/5] Add ALTER INDEX ... ALTER COLLATION ... REFRESH VERSION. @ 2019-12-11 12:54 Julien Rouhaud <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Julien Rouhaud @ 2019-12-11 12:54 UTC (permalink / raw) This command allows privileged users to specify that the currently installed collation version, for a specific collation, is binary compatible with the one that was installed when the specified index was built. This provides a way to clear warnings about potentially corrupted indexes without having to use REINDEX. Author: Julien Rouhaud Reviewed-by: Laurenz Albe, Thomas Munro and Peter Eisentraut Discussion: https://postgr.es/m/CAEepm%3D0uEQCpfq_%2BLYFBdArCe4Ot98t1aR4eYiYTe%3DyavQygiQ%40mail.gmail.com --- doc/src/sgml/ref/alter_index.sgml | 17 +++++++ src/backend/catalog/index.c | 2 +- src/backend/commands/tablecmds.c | 46 +++++++++++++++++++ src/backend/nodes/copyfuncs.c | 1 + src/backend/parser/gram.y | 8 ++++ src/bin/psql/tab-complete.c | 27 ++++++++++- src/include/catalog/index.h | 3 ++ src/include/nodes/parsenodes.h | 4 +- .../regress/expected/collate.icu.utf8.out | 20 ++++++++ src/test/regress/sql/collate.icu.utf8.sql | 11 +++++ 10 files changed, 136 insertions(+), 3 deletions(-) diff --git a/doc/src/sgml/ref/alter_index.sgml b/doc/src/sgml/ref/alter_index.sgml index a5e3b06ee4..03355164b3 100644 --- a/doc/src/sgml/ref/alter_index.sgml +++ b/doc/src/sgml/ref/alter_index.sgml @@ -25,6 +25,7 @@ ALTER INDEX [ IF EXISTS ] <replaceable class="parameter">name</replaceable> RENA ALTER INDEX [ IF EXISTS ] <replaceable class="parameter">name</replaceable> SET TABLESPACE <replaceable class="parameter">tablespace_name</replaceable> ALTER INDEX <replaceable class="parameter">name</replaceable> ATTACH PARTITION <replaceable class="parameter">index_name</replaceable> ALTER INDEX <replaceable class="parameter">name</replaceable> DEPENDS ON EXTENSION <replaceable class="parameter">extension_name</replaceable> +ALTER INDEX <replaceable class="parameter">name</replaceable> ALTER COLLATION <replaceable class="parameter">collation_name</replaceable> REFRESH VERSION ALTER INDEX [ IF EXISTS ] <replaceable class="parameter">name</replaceable> SET ( <replaceable class="parameter">storage_parameter</replaceable> [= <replaceable class="parameter">value</replaceable>] [, ... ] ) ALTER INDEX [ IF EXISTS ] <replaceable class="parameter">name</replaceable> RESET ( <replaceable class="parameter">storage_parameter</replaceable> [, ... ] ) ALTER INDEX [ IF EXISTS ] <replaceable class="parameter">name</replaceable> ALTER [ COLUMN ] <replaceable class="parameter">column_number</replaceable> @@ -112,6 +113,22 @@ ALTER INDEX ALL IN TABLESPACE <replaceable class="parameter">name</replaceable> </listitem> </varlistentry> + <varlistentry> + <term><literal>ALTER COLLATION <replaceable class="parameter">collation_name</replaceable> REFRESH VERSION</literal></term> + <listitem> + <para> + This command declares that the index is compatible with the currently + installed version of a collation that determines its order. It is used + to silence warnings caused by collation version incompatibilities and + should be issued only if the collation ordering is known not to have + changed since the index was last built. Be aware that incorrect use of + this command can hide index corruption. If you don't know whether a + collation's definition has changed, using <xref linkend="sql-reindex"/> + is a safe alternative. + </para> + </listitem> + </varlistentry> + <varlistentry> <term><literal>SET ( <replaceable class="parameter">storage_parameter</replaceable> [= <replaceable class="parameter">value</replaceable>] [, ... ] )</literal></term> <listitem> diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 210b25d147..34dbae05a2 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -3171,7 +3171,7 @@ index_build(Relation heapRelation, SetUserIdAndSecContext(save_userid, save_sec_context); } -static char * +char * index_force_collation_version(const ObjectAddress *otherObject, const char *version, void *userdata) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index f79044f39f..cdf3cbf5b1 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -93,6 +93,7 @@ #include "utils/lsyscache.h" #include "utils/memutils.h" #include "utils/partcache.h" +#include "utils/pg_locale.h" #include "utils/relcache.h" #include "utils/ruleutils.h" #include "utils/snapmgr.h" @@ -556,6 +557,7 @@ static void refuseDupeIndexAttach(Relation parentIdx, Relation partIdx, Relation partitionTbl); static List *GetParentedForeignKeyRefs(Relation partition); static void ATDetachCheckNoForeignKeyRefs(Relation partition); +static void ATExecAlterCollationRefreshVersion(Relation rel, List *coll); /* ---------------------------------------------------------------- @@ -3926,6 +3928,10 @@ AlterTableGetLockLevel(List *cmds) cmd_lockmode = AccessShareLock; break; + case AT_AlterCollationRefreshVersion: + cmd_lockmode = AccessExclusiveLock; + break; + default: /* oops */ elog(ERROR, "unrecognized alter table type: %d", (int) cmd->subtype); @@ -4093,6 +4099,12 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, /* This command never recurses */ pass = AT_PASS_MISC; break; + case AT_AlterCollationRefreshVersion: /* ALTER COLLATION ... REFRESH + * VERSION */ + ATSimplePermissions(rel, ATT_INDEX); + /* This command never recurses */ + pass = AT_PASS_MISC; + break; case AT_SetStorage: /* ALTER COLUMN SET STORAGE */ ATSimplePermissions(rel, ATT_TABLE | ATT_MATVIEW | ATT_FOREIGN_TABLE); ATSimpleRecursion(wqueue, rel, cmd, recurse, lockmode, context); @@ -4659,6 +4671,11 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation rel, Assert(rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE); ATExecDetachPartition(rel, ((PartitionCmd *) cmd->def)->name); break; + case AT_AlterCollationRefreshVersion: + /* ATPrepCmd ensured it must be an index */ + Assert(rel->rd_rel->relkind == RELKIND_INDEX); + ATExecAlterCollationRefreshVersion(rel, cmd->object); + break; default: /* oops */ elog(ERROR, "unrecognized alter table type: %d", (int) cmd->subtype); @@ -17477,3 +17494,32 @@ ATDetachCheckNoForeignKeyRefs(Relation partition) table_close(rel, NoLock); } } + +/* Execute an ALTER INDEX ... ALTER COLLATION ... REFRESH VERSION + * + * This override an existing dependency on a specific collation for a specific + * index to depend on the current collation version. + */ +static void +ATExecAlterCollationRefreshVersion(Relation rel, List *coll) +{ + ObjectAddress object; + NewCollationVersionDependency forced_dependency; + + Assert(coll != NIL); + forced_dependency.oid = get_collation_oid(coll, false); + + /* Retrieve the current version for the CURRENT VERSION case. */ + Assert(OidIsValid(forced_dependency.oid)); + forced_dependency.version = + get_collation_version_for_oid(forced_dependency.oid); + + object.classId = RelationRelationId; + object.objectId = rel->rd_id; + object.objectSubId = 0; + visitDependentObjects(&object, &index_force_collation_version, + &forced_dependency); + + /* Invalidate the index relcache */ + CacheInvalidateRelcache(rel); +} diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 5cbbe3ba2e..ebf9118f4f 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -3217,6 +3217,7 @@ _copyAlterTableCmd(const AlterTableCmd *from) COPY_SCALAR_FIELD(subtype); COPY_STRING_FIELD(name); + COPY_NODE_FIELD(object); COPY_SCALAR_FIELD(num); COPY_NODE_FIELD(newowner); COPY_NODE_FIELD(def); diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index c43b16037e..68934bfeb1 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -2594,6 +2594,14 @@ alter_table_cmd: n->subtype = AT_NoForceRowSecurity; $$ = (Node *)n; } + /* ALTER INDEX <name> ALTER COLLATION ... REFRESH VERSION */ + | ALTER COLLATION any_name REFRESH VERSION_P + { + AlterTableCmd *n = makeNode(AlterTableCmd); + n->subtype = AT_AlterCollationRefreshVersion; + n->object = $3; + $$ = (Node *)n; + } | alter_generic_options { AlterTableCmd *n = makeNode(AlterTableCmd); diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c index eb018854a5..ec0ef1feae 100644 --- a/src/bin/psql/tab-complete.c +++ b/src/bin/psql/tab-complete.c @@ -45,6 +45,7 @@ #include "catalog/pg_am_d.h" #include "catalog/pg_class_d.h" +#include "catalog/pg_collation_d.h" #include "common.h" #include "libpq-fe.h" #include "pqexpbuffer.h" @@ -814,6 +815,20 @@ static const SchemaQuery Query_for_list_of_statistics = { " (SELECT tgrelid FROM pg_catalog.pg_trigger "\ " WHERE pg_catalog.quote_ident(tgname)='%s')" +/* the silly-looking length condition is just to eat up the current word */ +#define Query_for_list_of_colls_for_one_index \ +" SELECT DISTINCT pg_catalog.quote_ident(coll.collname) " \ +" FROM pg_catalog.pg_depend d, pg_catalog.pg_collation coll, " \ +" pg_catalog.pg_class c" \ +" WHERE (%d = pg_catalog.length('%s'))" \ +" AND d.refclassid = " CppAsString2(CollationRelationId) \ +" AND d.refobjid = coll.oid " \ +" AND d.classid = " CppAsString2(RelationRelationId) \ +" AND d.objid = c.oid " \ +" AND c.relkind = " CppAsString2(RELKIND_INDEX) \ +" AND pg_catalog.pg_table_is_visible(c.oid) " \ +" AND c.relname = '%s'" + #define Query_for_list_of_ts_configurations \ "SELECT pg_catalog.quote_ident(cfgname) FROM pg_catalog.pg_ts_config "\ " WHERE substring(pg_catalog.quote_ident(cfgname),1,%d)='%s'" @@ -1709,7 +1724,8 @@ psql_completion(const char *text, int start, int end) /* ALTER INDEX <name> */ else if (Matches("ALTER", "INDEX", MatchAny)) COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME TO", "SET", - "RESET", "ATTACH PARTITION", "DEPENDS", "NO DEPENDS"); + "RESET", "ATTACH PARTITION", "DEPENDS", "NO DEPENDS", + "ALTER COLLATION"); else if (Matches("ALTER", "INDEX", MatchAny, "ATTACH")) COMPLETE_WITH("PARTITION"); else if (Matches("ALTER", "INDEX", MatchAny, "ATTACH", "PARTITION")) @@ -1759,6 +1775,15 @@ psql_completion(const char *text, int start, int end) COMPLETE_WITH("ON EXTENSION"); else if (Matches("ALTER", "INDEX", MatchAny, "DEPENDS")) COMPLETE_WITH("ON EXTENSION"); + /* ALTER INDEX <name> ALTER COLLATION */ + else if (Matches("ALTER", "INDEX", MatchAny, "ALTER", "COLLATION")) + { + completion_info_charp = prev4_wd; + COMPLETE_WITH_QUERY(Query_for_list_of_colls_for_one_index); + } + /* ALTER INDEX <name> ALTER COLLATION <name> */ + else if (Matches("ALTER", "INDEX", MatchAny, "ALTER", "COLLATION", MatchAny)) + COMPLETE_WITH("REFRESH VERSION"); /* ALTER LANGUAGE <name> */ else if (Matches("ALTER", "LANGUAGE", MatchAny)) diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h index 9b4de26514..46d5df1613 100644 --- a/src/include/catalog/index.h +++ b/src/include/catalog/index.h @@ -123,6 +123,9 @@ extern void FormIndexDatum(IndexInfo *indexInfo, extern void index_check_collation_versions(Oid relid); +extern char *index_force_collation_version(const ObjectAddress *otherObject, + const char *version, + void *userdata); extern void index_force_collation_versions(Oid indexid, Oid coll, char *version); diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 7a23fb7529..423f33b72c 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -1847,7 +1847,8 @@ typedef enum AlterTableType AT_DetachPartition, /* DETACH PARTITION */ AT_AddIdentity, /* ADD IDENTITY */ AT_SetIdentity, /* SET identity column options */ - AT_DropIdentity /* DROP IDENTITY */ + AT_DropIdentity, /* DROP IDENTITY */ + AT_AlterCollationRefreshVersion /* ALTER COLLATION ... REFRESH VERSION */ } AlterTableType; typedef struct ReplicaIdentityStmt @@ -1863,6 +1864,7 @@ typedef struct AlterTableCmd /* one subcommand of an ALTER TABLE */ AlterTableType subtype; /* Type of table alteration to apply */ char *name; /* column, constraint, or trigger to act on, * or tablespace */ + List *object; /* collation to act on if it's a collation */ int16 num; /* attribute number for columns referenced by * number */ RoleSpec *newowner; diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out index db386c1b09..adc1dddda7 100644 --- a/src/test/regress/expected/collate.icu.utf8.out +++ b/src/test/regress/expected/collate.icu.utf8.out @@ -2054,6 +2054,26 @@ SELECT objid::regclass FROM pg_depend WHERE refobjversion = 'not a version'; ------- (0 rows) +-- Test ALTER INDEX name ALTER COLLATION name REFRESH VERSION +UPDATE pg_depend SET refobjversion = 'not a version' +WHERE refclassid = 'pg_collation'::regclass +AND objid::regclass::text = 'icuidx17_part' +AND refobjversion IS NOT NULL; +SELECT objid::regclass FROM pg_depend WHERE refobjversion = 'not a version'; + objid +--------------- + icuidx17_part +(1 row) + +ALTER INDEX icuidx17_part ALTER COLLATION "en-x-icu" REFRESH VERSION; +SELECT objid::regclass, refobjversion = 'not a version' AS ver FROM pg_depend +WHERE refclassid = 'pg_collation'::regclass +AND objid::regclass::text = 'icuidx17_part'; + objid | ver +---------------+----- + icuidx17_part | f +(1 row) + -- cleanup RESET search_path; SET client_min_messages TO warning; diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql index e93530af55..b3a75b29e6 100644 --- a/src/test/regress/sql/collate.icu.utf8.sql +++ b/src/test/regress/sql/collate.icu.utf8.sql @@ -823,6 +823,17 @@ VACUUM FULL collate_part_1; SELECT objid::regclass FROM pg_depend WHERE refobjversion = 'not a version'; +-- Test ALTER INDEX name ALTER COLLATION name REFRESH VERSION +UPDATE pg_depend SET refobjversion = 'not a version' +WHERE refclassid = 'pg_collation'::regclass +AND objid::regclass::text = 'icuidx17_part' +AND refobjversion IS NOT NULL; +SELECT objid::regclass FROM pg_depend WHERE refobjversion = 'not a version'; +ALTER INDEX icuidx17_part ALTER COLLATION "en-x-icu" REFRESH VERSION; +SELECT objid::regclass, refobjversion = 'not a version' AS ver FROM pg_depend +WHERE refclassid = 'pg_collation'::regclass +AND objid::regclass::text = 'icuidx17_part'; + -- cleanup RESET search_path; SET client_min_messages TO warning; -- 2.20.1 --wac7ysb48OaltWcw Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="v24-0005-doc-Add-Collation-Versions-section.patch" ^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher @ 2023-01-07 18:50 Tom Lane <[email protected]> 0 siblings, 0 replies; 2+ messages in thread From: Tom Lane @ 2023-01-07 18:50 UTC (permalink / raw) To: Önder Kalacı <[email protected]>; +Cc: Andres Freund <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; Amit Kapila <[email protected]>; PostgreSQL Hackers <[email protected]> =?UTF-8?B?w5ZuZGVyIEthbGFjxLE=?= <[email protected]> writes: > Attached v22. I took a very brief look through this. I'm not too pleased with this whole line of development TBH. It seems to me that the core design of execReplication.c and related code is "let's build our own half-baked executor and much-less-than-half-baked planner, because XXX". (I'm not too sure what XXX was, really, but apparently somebody managed to convince people that that is a sane and maintainable design.) Now this patch has decided that it *will* use the real planner, or at least portions of it in some cases. If we're going to do that ISTM we ought to replace all the existing not-really-a-planner logic, but this has not done that; instead we have a large net addition to the already very duplicative replication code, with weird restrictions because it doesn't want to make changes to the half-baked executor. I think we should either live within the constraints set by this overarching design, or else nuke execReplication.c from orbit and start using the real planner and executor. Perhaps the foreign key enforcement mechanisms could be a model --- although if you don't want to buy into using SPI as well, you probably should look at Amit L's work at [1]. Also ... maybe I am missing something, but is REPLICA IDENTITY FULL sanely defined in the first place? It looks to me that RelationFindReplTupleSeq assumes without proof that there is a unique full-tuple match, but that is only reasonable to assume if there is at least one unique index (and maybe not even then, if nulls are involved). If there is a unique index, why can't that be chosen as replica identity? If there isn't, what semantics are we actually providing? What I'm thinking about is that maybe REPLICA IDENTITY FULL should be defined as "the subscriber can pick any unique index to match on, and is allowed to fail if the table has none". Or if "fail" is a bridge too far for you, we could fall back to the existing seqscan logic. But thumbing through the existing indexes to find a non-expression unique index wouldn't require invoking the full planner. Any candidate index would result in a plan estimated to fetch just one row, so there aren't likely to be serious cost differences. regards, tom lane [1] https://www.postgresql.org/message-id/flat/CA+HiwqG5e8pk8s7+7zhr1Nc_PGyhEdM5f=pHkMOdK1RYWXfJsg@mail.... ^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2023-01-07 18:50 UTC | newest] Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-12-11 12:54 [PATCH v24 4/5] Add ALTER INDEX ... ALTER COLLATION ... REFRESH VERSION. Julien Rouhaud <[email protected]> 2023-01-07 18:50 Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox