public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v14 1/7] Remove pg_collation.collversion. 4+ messages / 3 participants [nested] [flat]
* [PATCH v14 1/7] Remove pg_collation.collversion. @ 2019-05-28 18:15 Thomas Munro <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Thomas Munro @ 2019-05-28 18:15 UTC (permalink / raw) While this allowed the system to detect changes in collation versions, it didn't help us track what had to be done about it. A later patch will add version tracking for individual database objects that depend on collations. Author: Thomas Munro Reviewed-by: Julien Rouhaud and Peter Eisentraut Discussion: https://postgr.es/m/CAEepm%3D0uEQCpfq_%2BLYFBdArCe4Ot98t1aR4eYiYTe%3DyavQygiQ%40mail.gmail.com --- doc/src/sgml/catalogs.sgml | 11 --- doc/src/sgml/func.sgml | 5 +- doc/src/sgml/ref/alter_collation.sgml | 65 -------------- doc/src/sgml/ref/create_collation.sgml | 21 ----- src/backend/catalog/pg_collation.c | 5 -- src/backend/commands/collationcmds.c | 85 ------------------- src/backend/nodes/copyfuncs.c | 13 --- src/backend/nodes/equalfuncs.c | 11 --- src/backend/parser/gram.y | 18 +--- src/backend/tcop/utility.c | 12 --- src/backend/utils/adt/pg_locale.c | 37 -------- src/bin/pg_dump/pg_dump.c | 8 +- src/include/catalog/pg_collation.dat | 7 +- src/include/catalog/pg_collation.h | 5 -- src/include/catalog/toasting.h | 1 - src/include/commands/collationcmds.h | 1 - src/include/nodes/parsenodes.h | 11 --- .../regress/expected/collate.icu.utf8.out | 3 - .../regress/expected/collate.linux.utf8.out | 3 - src/test/regress/sql/collate.icu.utf8.sql | 5 -- src/test/regress/sql/collate.linux.utf8.sql | 5 -- 21 files changed, 11 insertions(+), 321 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index c6f95fa688..c2d33c76e0 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -2122,17 +2122,6 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l <entry></entry> <entry><symbol>LC_CTYPE</symbol> for this collation object</entry> </row> - - <row> - <entry><structfield>collversion</structfield></entry> - <entry><type>text</type></entry> - <entry></entry> - <entry> - Provider-specific version of the collation. This is recorded when the - collation is created and then checked when it is used, to detect - changes in the collation definition that could lead to data corruption. - </entry> - </row> </tbody> </tgroup> </table> diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index 323366feb6..b2d991ac7f 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -21115,10 +21115,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); <para> <function>pg_collation_actual_version</function> returns the actual version of the collation object as it is currently installed in the - operating system. If this is different from the value - in <literal>pg_collation.collversion</literal>, then objects depending on - the collation might need to be rebuilt. See also - <xref linkend="sql-altercollation"/>. + operating system. </para> <para> diff --git a/doc/src/sgml/ref/alter_collation.sgml b/doc/src/sgml/ref/alter_collation.sgml index 4cfcb42251..c985b0de56 100644 --- a/doc/src/sgml/ref/alter_collation.sgml +++ b/doc/src/sgml/ref/alter_collation.sgml @@ -21,8 +21,6 @@ PostgreSQL documentation <refsynopsisdiv> <synopsis> -ALTER COLLATION <replaceable>name</replaceable> REFRESH VERSION - ALTER COLLATION <replaceable>name</replaceable> RENAME TO <replaceable>new_name</replaceable> ALTER COLLATION <replaceable>name</replaceable> OWNER TO { <replaceable>new_owner</replaceable> | CURRENT_USER | SESSION_USER } ALTER COLLATION <replaceable>name</replaceable> SET SCHEMA <replaceable>new_schema</replaceable> @@ -88,72 +86,9 @@ ALTER COLLATION <replaceable>name</replaceable> SET SCHEMA <replaceable>new_sche </listitem> </varlistentry> - <varlistentry> - <term><literal>REFRESH VERSION</literal></term> - <listitem> - <para> - Update the collation's version. - See <xref linkend="sql-altercollation-notes" - endterm="sql-altercollation-notes-title"/> below. - </para> - </listitem> - </varlistentry> </variablelist> </refsect1> - <refsect1 id="sql-altercollation-notes"> - <title id="sql-altercollation-notes-title">Notes</title> - - <para> - When using collations provided by the ICU library, the ICU-specific version - of the collator is recorded in the system catalog when the collation object - is created. When the collation is used, the current version is - checked against the recorded version, and a warning is issued when there is - a mismatch, for example: -<screen> -WARNING: collation "xx-x-icu" has version mismatch -DETAIL: The collation in the database was created using version 1.2.3.4, but the operating system provides version 2.3.4.5. -HINT: Rebuild all objects affected by this collation and run ALTER COLLATION pg_catalog."xx-x-icu" REFRESH VERSION, or build PostgreSQL with the right library version. -</screen> - A change in collation definitions can lead to corrupt indexes and other - problems because the database system relies on stored objects having a - certain sort order. Generally, this should be avoided, but it can happen - in legitimate circumstances, such as when - using <command>pg_upgrade</command> to upgrade to server binaries linked - with a newer version of ICU. When this happens, all objects depending on - the collation should be rebuilt, for example, - using <command>REINDEX</command>. When that is done, the collation version - can be refreshed using the command <literal>ALTER COLLATION ... REFRESH - VERSION</literal>. This will update the system catalog to record the - current collator version and will make the warning go away. Note that this - does not actually check whether all affected objects have been rebuilt - correctly. - </para> - <para> - When using collations provided by <literal>libc</literal> and - <productname>PostgreSQL</productname> was built with the GNU C library, the - C library's version is used as a collation version. Since collation - definitions typically change only with GNU C library releases, this provides - some defense against corruption, but it is not completely reliable. - </para> - <para> - Currently, there is no version tracking for the database default collation. - </para> - - <para> - The following query can be used to identify all collations in the current - database that need to be refreshed and the objects that depend on them: -<programlisting><![CDATA[ -SELECT pg_describe_object(refclassid, refobjid, refobjsubid) AS "Collation", - pg_describe_object(classid, objid, objsubid) AS "Object" - FROM pg_depend d JOIN pg_collation c - ON refclassid = 'pg_collation'::regclass AND refobjid = c.oid - WHERE c.collversion <> pg_collation_actual_version(c.oid) - ORDER BY 1, 2; -]]></programlisting> - </para> - </refsect1> - <refsect1> <title>Examples</title> diff --git a/doc/src/sgml/ref/create_collation.sgml b/doc/src/sgml/ref/create_collation.sgml index def4dda6e8..36120385d1 100644 --- a/doc/src/sgml/ref/create_collation.sgml +++ b/doc/src/sgml/ref/create_collation.sgml @@ -24,7 +24,6 @@ CREATE COLLATION [ IF NOT EXISTS ] <replaceable>name</replaceable> ( [ LC_CTYPE = <replaceable>lc_ctype</replaceable>, ] [ PROVIDER = <replaceable>provider</replaceable>, ] [ DETERMINISTIC = <replaceable>boolean</replaceable>, ] - [ VERSION = <replaceable>version</replaceable> ] ) CREATE COLLATION [ IF NOT EXISTS ] <replaceable>name</replaceable> FROM <replaceable>existing_collation</replaceable> </synopsis> @@ -146,26 +145,6 @@ CREATE COLLATION [ IF NOT EXISTS ] <replaceable>name</replaceable> FROM <replace </listitem> </varlistentry> - <varlistentry> - <term><replaceable>version</replaceable></term> - - <listitem> - <para> - Specifies the version string to store with the collation. Normally, - this should be omitted, which will cause the version to be computed - from the actual version of the collation as provided by the operating - system. This option is intended to be used - by <command>pg_upgrade</command> for copying the version from an - existing installation. - </para> - - <para> - See also <xref linkend="sql-altercollation"/> for how to handle - collation version mismatches. - </para> - </listitem> - </varlistentry> - <varlistentry> <term><replaceable>existing_collation</replaceable></term> diff --git a/src/backend/catalog/pg_collation.c b/src/backend/catalog/pg_collation.c index 8559779a4f..c78192e34b 100644 --- a/src/backend/catalog/pg_collation.c +++ b/src/backend/catalog/pg_collation.c @@ -49,7 +49,6 @@ CollationCreate(const char *collname, Oid collnamespace, bool collisdeterministic, int32 collencoding, const char *collcollate, const char *collctype, - const char *collversion, bool if_not_exists, bool quiet) { @@ -167,10 +166,6 @@ CollationCreate(const char *collname, Oid collnamespace, values[Anum_pg_collation_collcollate - 1] = NameGetDatum(&name_collate); namestrcpy(&name_ctype, collctype); values[Anum_pg_collation_collctype - 1] = NameGetDatum(&name_ctype); - if (collversion) - values[Anum_pg_collation_collversion - 1] = CStringGetTextDatum(collversion); - else - nulls[Anum_pg_collation_collversion - 1] = true; tup = heap_form_tuple(tupDesc, values, nulls); diff --git a/src/backend/commands/collationcmds.c b/src/backend/commands/collationcmds.c index 9f6582c530..78eceda848 100644 --- a/src/backend/commands/collationcmds.c +++ b/src/backend/commands/collationcmds.c @@ -68,7 +68,6 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e bool collisdeterministic = true; int collencoding = 0; char collprovider = 0; - char *collversion = NULL; Oid newoid; ObjectAddress address; @@ -166,9 +165,6 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e if (deterministicEl) collisdeterministic = defGetBoolean(deterministicEl); - if (versionEl) - collversion = defGetString(versionEl); - if (collproviderstr) { if (pg_strcasecmp(collproviderstr, "icu") == 0) @@ -215,9 +211,6 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e } } - if (!collversion) - collversion = get_collation_actual_version(collprovider, collcollate); - newoid = CollationCreate(collName, collNamespace, GetUserId(), @@ -226,7 +219,6 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e collencoding, collcollate, collctype, - collversion, if_not_exists, false); /* not quiet */ @@ -277,80 +269,6 @@ IsThereCollationInNamespace(const char *collname, Oid nspOid) collname, get_namespace_name(nspOid)))); } -/* - * ALTER COLLATION - */ -ObjectAddress -AlterCollation(AlterCollationStmt *stmt) -{ - Relation rel; - Oid collOid; - HeapTuple tup; - Form_pg_collation collForm; - Datum collversion; - bool isnull; - char *oldversion; - char *newversion; - ObjectAddress address; - - rel = table_open(CollationRelationId, RowExclusiveLock); - collOid = get_collation_oid(stmt->collname, false); - - if (!pg_collation_ownercheck(collOid, GetUserId())) - aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_COLLATION, - NameListToString(stmt->collname)); - - tup = SearchSysCacheCopy1(COLLOID, ObjectIdGetDatum(collOid)); - if (!HeapTupleIsValid(tup)) - elog(ERROR, "cache lookup failed for collation %u", collOid); - - collForm = (Form_pg_collation) GETSTRUCT(tup); - collversion = SysCacheGetAttr(COLLOID, tup, Anum_pg_collation_collversion, - &isnull); - oldversion = isnull ? NULL : TextDatumGetCString(collversion); - - newversion = get_collation_actual_version(collForm->collprovider, NameStr(collForm->collcollate)); - - /* cannot change from NULL to non-NULL or vice versa */ - if ((!oldversion && newversion) || (oldversion && !newversion)) - elog(ERROR, "invalid collation version change"); - else if (oldversion && newversion && strcmp(newversion, oldversion) != 0) - { - bool nulls[Natts_pg_collation]; - bool replaces[Natts_pg_collation]; - Datum values[Natts_pg_collation]; - - ereport(NOTICE, - (errmsg("changing version from %s to %s", - oldversion, newversion))); - - memset(values, 0, sizeof(values)); - memset(nulls, false, sizeof(nulls)); - memset(replaces, false, sizeof(replaces)); - - values[Anum_pg_collation_collversion - 1] = CStringGetTextDatum(newversion); - replaces[Anum_pg_collation_collversion - 1] = true; - - tup = heap_modify_tuple(tup, RelationGetDescr(rel), - values, nulls, replaces); - } - else - ereport(NOTICE, - (errmsg("version has not changed"))); - - CatalogTupleUpdate(rel, &tup->t_self, tup); - - InvokeObjectPostAlterHook(CollationRelationId, collOid, 0); - - ObjectAddressSet(address, CollationRelationId, collOid); - - heap_freetuple(tup); - table_close(rel, NoLock); - - return address; -} - - Datum pg_collation_actual_version(PG_FUNCTION_ARGS) { @@ -608,7 +526,6 @@ pg_import_system_collations(PG_FUNCTION_ARGS) collid = CollationCreate(localebuf, nspid, GetUserId(), COLLPROVIDER_LIBC, true, enc, localebuf, localebuf, - get_collation_actual_version(COLLPROVIDER_LIBC, localebuf), true, true); if (OidIsValid(collid)) { @@ -669,7 +586,6 @@ pg_import_system_collations(PG_FUNCTION_ARGS) collid = CollationCreate(alias, nspid, GetUserId(), COLLPROVIDER_LIBC, true, enc, locale, locale, - get_collation_actual_version(COLLPROVIDER_LIBC, locale), true, true); if (OidIsValid(collid)) { @@ -731,7 +647,6 @@ pg_import_system_collations(PG_FUNCTION_ARGS) nspid, GetUserId(), COLLPROVIDER_ICU, true, -1, collcollate, collcollate, - get_collation_actual_version(COLLPROVIDER_ICU, collcollate), true, true); if (OidIsValid(collid)) { diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index eaab97f753..7caf0f2f53 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -3184,16 +3184,6 @@ _copyAlterTableCmd(const AlterTableCmd *from) return newnode; } -static AlterCollationStmt * -_copyAlterCollationStmt(const AlterCollationStmt *from) -{ - AlterCollationStmt *newnode = makeNode(AlterCollationStmt); - - COPY_NODE_FIELD(collname); - - return newnode; -} - static AlterDomainStmt * _copyAlterDomainStmt(const AlterDomainStmt *from) { @@ -5184,9 +5174,6 @@ copyObjectImpl(const void *from) case T_AlterTableCmd: retval = _copyAlterTableCmd(from); break; - case T_AlterCollationStmt: - retval = _copyAlterCollationStmt(from); - break; case T_AlterDomainStmt: retval = _copyAlterDomainStmt(from); break; diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c index 88b912977e..05f694929c 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -1105,14 +1105,6 @@ _equalAlterTableCmd(const AlterTableCmd *a, const AlterTableCmd *b) return true; } -static bool -_equalAlterCollationStmt(const AlterCollationStmt *a, const AlterCollationStmt *b) -{ - COMPARE_NODE_FIELD(collname); - - return true; -} - static bool _equalAlterDomainStmt(const AlterDomainStmt *a, const AlterDomainStmt *b) { @@ -3278,9 +3270,6 @@ equal(const void *a, const void *b) case T_AlterTableCmd: retval = _equalAlterTableCmd(a, b); break; - case T_AlterCollationStmt: - retval = _equalAlterCollationStmt(a, b); - break; case T_AlterDomainStmt: retval = _equalAlterDomainStmt(a, b); break; diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y index 7e384f956c..804cbafda4 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -245,7 +245,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query); } %type <node> stmt schema_stmt - AlterEventTrigStmt AlterCollationStmt + AlterEventTrigStmt AlterDatabaseStmt AlterDatabaseSetStmt AlterDomainStmt AlterEnumStmt AlterFdwStmt AlterForeignServerStmt AlterGroupStmt AlterObjectDependsStmt AlterObjectSchemaStmt AlterOwnerStmt @@ -830,7 +830,6 @@ stmtmulti: stmtmulti ';' stmt stmt : AlterEventTrigStmt - | AlterCollationStmt | AlterDatabaseStmt | AlterDatabaseSetStmt | AlterDefaultPrivilegesStmt @@ -10343,21 +10342,6 @@ drop_option: } ; -/***************************************************************************** - * - * ALTER COLLATION - * - *****************************************************************************/ - -AlterCollationStmt: ALTER COLLATION any_name REFRESH VERSION_P - { - AlterCollationStmt *n = makeNode(AlterCollationStmt); - n->collname = $3; - $$ = (Node *)n; - } - ; - - /***************************************************************************** * * ALTER SYSTEM diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c index b1f7f6e2d0..9cecf409a4 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -1793,10 +1793,6 @@ ProcessUtilitySlow(ParseState *pstate, address = AlterStatistics((AlterStatsStmt *) parsetree); break; - case T_AlterCollationStmt: - address = AlterCollation((AlterCollationStmt *) parsetree); - break; - default: elog(ERROR, "unrecognized node type: %d", (int) nodeTag(parsetree)); @@ -2944,10 +2940,6 @@ CreateCommandTag(Node *parsetree) tag = CMDTAG_DROP_SUBSCRIPTION; break; - case T_AlterCollationStmt: - tag = CMDTAG_ALTER_COLLATION; - break; - case T_PrepareStmt: tag = CMDTAG_PREPARE; break; @@ -3560,10 +3552,6 @@ GetCommandLogLevel(Node *parsetree) lev = LOGSTMT_DDL; break; - case T_AlterCollationStmt: - lev = LOGSTMT_DDL; - break; - /* already-planned queries */ case T_PlannedStmt: { diff --git a/src/backend/utils/adt/pg_locale.c b/src/backend/utils/adt/pg_locale.c index 64fd3ae18a..60dab33fcb 100644 --- a/src/backend/utils/adt/pg_locale.c +++ b/src/backend/utils/adt/pg_locale.c @@ -1352,8 +1352,6 @@ pg_newlocale_from_collation(Oid collid) const char *collctype pg_attribute_unused(); struct pg_locale_struct result; pg_locale_t resultp; - Datum collversion; - bool isnull; tp = SearchSysCache1(COLLOID, ObjectIdGetDatum(collid)); if (!HeapTupleIsValid(tp)) @@ -1455,41 +1453,6 @@ pg_newlocale_from_collation(Oid collid) #endif /* not USE_ICU */ } - collversion = SysCacheGetAttr(COLLOID, tp, Anum_pg_collation_collversion, - &isnull); - if (!isnull) - { - char *actual_versionstr; - char *collversionstr; - - actual_versionstr = get_collation_actual_version(collform->collprovider, collcollate); - if (!actual_versionstr) - { - /* - * This could happen when specifying a version in CREATE - * COLLATION for a libc locale, or manually creating a mess in - * the catalogs. - */ - ereport(ERROR, - (errmsg("collation \"%s\" has no actual version, but a version was specified", - NameStr(collform->collname)))); - } - collversionstr = TextDatumGetCString(collversion); - - if (strcmp(actual_versionstr, collversionstr) != 0) - ereport(WARNING, - (errmsg("collation \"%s\" has version mismatch", - NameStr(collform->collname)), - errdetail("The collation in the database was created using version %s, " - "but the operating system provides version %s.", - collversionstr, actual_versionstr), - errhint("Rebuild all objects affected by this collation and run " - "ALTER COLLATION %s REFRESH VERSION, " - "or build PostgreSQL with the right library version.", - quote_qualified_identifier(get_namespace_name(collform->collnamespace), - NameStr(collform->collname))))); - } - ReleaseSysCache(tp); /* We'll keep the pg_locale_t structures in TopMemoryContext */ diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index ad039e97a5..febe582d27 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -13501,7 +13501,11 @@ dumpCollation(Archive *fout, CollInfo *collinfo) /* Get collation-specific details */ appendPQExpBufferStr(query, "SELECT "); - if (fout->remoteVersion >= 100000) + if (fout->remoteVersion >= 130000) + appendPQExpBufferStr(query, + "collprovider, " + "NULL AS collversion, "); + else if (fout->remoteVersion >= 100000) appendPQExpBufferStr(query, "collprovider, " "collversion, "); @@ -13573,7 +13577,7 @@ dumpCollation(Archive *fout, CollInfo *collinfo) * For binary upgrade, carry over the collation version. For normal * dump/restore, omit the version, so that it is computed upon restore. */ - if (dopt->binary_upgrade) + if (dopt->binary_upgrade && fout->remoteVersion < 130000) { int i_collversion; diff --git a/src/include/catalog/pg_collation.dat b/src/include/catalog/pg_collation.dat index ba1b3e201b..45301ccdd7 100644 --- a/src/include/catalog/pg_collation.dat +++ b/src/include/catalog/pg_collation.dat @@ -15,17 +15,16 @@ { oid => '100', oid_symbol => 'DEFAULT_COLLATION_OID', descr => 'database\'s default collation', collname => 'default', collnamespace => 'PGNSP', collowner => 'PGUID', - collprovider => 'd', collencoding => '-1', collcollate => '', collctype => '', - collversion => '_null_' }, + collprovider => 'd', collencoding => '-1', collcollate => '', collctype => '' }, { oid => '950', oid_symbol => 'C_COLLATION_OID', descr => 'standard C collation', collname => 'C', collnamespace => 'PGNSP', collowner => 'PGUID', collprovider => 'c', collencoding => '-1', collcollate => 'C', - collctype => 'C', collversion => '_null_' }, + collctype => 'C' }, { oid => '951', oid_symbol => 'POSIX_COLLATION_OID', descr => 'standard POSIX collation', collname => 'POSIX', collnamespace => 'PGNSP', collowner => 'PGUID', collprovider => 'c', collencoding => '-1', collcollate => 'POSIX', - collctype => 'POSIX', collversion => '_null_' }, + collctype => 'POSIX' }, ] diff --git a/src/include/catalog/pg_collation.h b/src/include/catalog/pg_collation.h index 6955bb1273..cfde555366 100644 --- a/src/include/catalog/pg_collation.h +++ b/src/include/catalog/pg_collation.h @@ -37,10 +37,6 @@ CATALOG(pg_collation,3456,CollationRelationId) int32 collencoding; /* encoding for this collation; -1 = "all" */ NameData collcollate; /* LC_COLLATE setting */ NameData collctype; /* LC_CTYPE setting */ -#ifdef CATALOG_VARLEN /* variable-length fields start here */ - text collversion; /* provider-dependent version of collation - * data */ -#endif } FormData_pg_collation; /* ---------------- @@ -65,7 +61,6 @@ extern Oid CollationCreate(const char *collname, Oid collnamespace, bool collisdeterministic, int32 collencoding, const char *collcollate, const char *collctype, - const char *collversion, bool if_not_exists, bool quiet); extern void RemoveCollationById(Oid collationOid); diff --git a/src/include/catalog/toasting.h b/src/include/catalog/toasting.h index 51491c4513..8f131893dc 100644 --- a/src/include/catalog/toasting.h +++ b/src/include/catalog/toasting.h @@ -51,7 +51,6 @@ extern void BootstrapToastTable(char *relName, /* normal catalogs */ DECLARE_TOAST(pg_aggregate, 4159, 4160); DECLARE_TOAST(pg_attrdef, 2830, 2831); -DECLARE_TOAST(pg_collation, 4161, 4162); DECLARE_TOAST(pg_constraint, 2832, 2833); DECLARE_TOAST(pg_default_acl, 4143, 4144); DECLARE_TOAST(pg_description, 2834, 2835); diff --git a/src/include/commands/collationcmds.h b/src/include/commands/collationcmds.h index 373b85374c..3e1c16ac7f 100644 --- a/src/include/commands/collationcmds.h +++ b/src/include/commands/collationcmds.h @@ -20,6 +20,5 @@ extern ObjectAddress DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_exists); extern void IsThereCollationInNamespace(const char *collname, Oid nspOid); -extern ObjectAddress AlterCollation(AlterCollationStmt *stmt); #endif /* COLLATIONCMDS_H */ diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 2039b42449..079fe1a5f3 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -1870,17 +1870,6 @@ typedef struct AlterTableCmd /* one subcommand of an ALTER TABLE */ } AlterTableCmd; -/* ---------------------- - * Alter Collation - * ---------------------- - */ -typedef struct AlterCollationStmt -{ - NodeTag type; - List *collname; -} AlterCollationStmt; - - /* ---------------------- * Alter Domain * diff --git a/src/test/regress/expected/collate.icu.utf8.out b/src/test/regress/expected/collate.icu.utf8.out index 2b86ce9028..60d9263a2f 100644 --- a/src/test/regress/expected/collate.icu.utf8.out +++ b/src/test/regress/expected/collate.icu.utf8.out @@ -1082,9 +1082,6 @@ SELECT collname FROM pg_collation WHERE collname LIKE 'test%'; DROP SCHEMA test_schema; DROP ROLE regress_test_role; --- ALTER -ALTER COLLATION "en-x-icu" REFRESH VERSION; -NOTICE: version has not changed -- dependencies CREATE COLLATION test0 FROM "C"; CREATE TABLE collate_dep_test1 (a int, b text COLLATE test0); diff --git a/src/test/regress/expected/collate.linux.utf8.out b/src/test/regress/expected/collate.linux.utf8.out index f06ae543e4..580b00eea7 100644 --- a/src/test/regress/expected/collate.linux.utf8.out +++ b/src/test/regress/expected/collate.linux.utf8.out @@ -1093,9 +1093,6 @@ SELECT collname FROM pg_collation WHERE collname LIKE 'test%'; DROP SCHEMA test_schema; DROP ROLE regress_test_role; --- ALTER -ALTER COLLATION "en_US" REFRESH VERSION; -NOTICE: version has not changed -- dependencies CREATE COLLATION test0 FROM "C"; CREATE TABLE collate_dep_test1 (a int, b text COLLATE test0); diff --git a/src/test/regress/sql/collate.icu.utf8.sql b/src/test/regress/sql/collate.icu.utf8.sql index 67de7d9794..35acf91fbf 100644 --- a/src/test/regress/sql/collate.icu.utf8.sql +++ b/src/test/regress/sql/collate.icu.utf8.sql @@ -405,11 +405,6 @@ DROP SCHEMA test_schema; DROP ROLE regress_test_role; --- ALTER - -ALTER COLLATION "en-x-icu" REFRESH VERSION; - - -- dependencies CREATE COLLATION test0 FROM "C"; diff --git a/src/test/regress/sql/collate.linux.utf8.sql b/src/test/regress/sql/collate.linux.utf8.sql index cbbd2203e4..c697c99488 100644 --- a/src/test/regress/sql/collate.linux.utf8.sql +++ b/src/test/regress/sql/collate.linux.utf8.sql @@ -406,11 +406,6 @@ DROP SCHEMA test_schema; DROP ROLE regress_test_role; --- ALTER - -ALTER COLLATION "en_US" REFRESH VERSION; - - -- dependencies CREATE COLLATION test0 FROM "C"; -- 2.20.1 --wRRV7LY7NUeQGEoC Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="v14-0002-Add-pg_depend.refobjversion.patch" ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Assert failure of the cross-check for nullingrels @ 2023-05-20 15:24 Tom Lane <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Tom Lane @ 2023-05-20 15:24 UTC (permalink / raw) To: Richard Guo <[email protected]>; +Cc: pgsql-hackers Richard Guo <[email protected]> writes: > I tried with v4 patch and find that, as you predicted, it might reject > all the clones in some cases. Check the query below > ... > So the qual 't3.a = t4.a' is missing in this plan shape. I poked into that more closely and realized that the reason that clause_is_computable_at() misbehaves is that both clones of the "t3.a = t4.a" qual have the same clause_relids: (4 5 6) which is t3, the left join to t3, and t4. This is unsurprising because the difference in these clones is whether they are expected to be evaluated above or below outer join 3 (the left join to t2), and t2 doesn't appear in the qual. (It does appear in "t2.b = t4.b", which is why there's no similar misbehavior for that qual.) If they have the same clause_relids, then clearly in its current form clause_is_computable_at() cannot distinguish them. So what I now think we should do is have clause_is_computable_at() examine their required_relids instead. Those will be different, by construction in deconstruct_distribute_oj_quals(), ensuring that we select only one of the group of clones. BTW, while I've not tried it, I suspect your v2 patch also fails on this example for the same reason: it cannot distinguish the clones of this qual. regards, tom lane Attachments: [text/x-diff] v5-0001-fix-clause_is_computable_at.patch (5.0K, ../../[email protected]/2-v5-0001-fix-clause_is_computable_at.patch) download | inline diff: diff --git a/src/backend/optimizer/util/restrictinfo.c b/src/backend/optimizer/util/restrictinfo.c index d2bc096e1c..760d24bebf 100644 --- a/src/backend/optimizer/util/restrictinfo.c +++ b/src/backend/optimizer/util/restrictinfo.c @@ -544,13 +544,24 @@ clause_is_computable_at(PlannerInfo *root, RestrictInfo *rinfo, Relids eval_relids) { - Relids clause_relids = rinfo->clause_relids; + Relids clause_relids; ListCell *lc; /* Nothing to do if no outer joins have been performed yet. */ if (!bms_overlap(eval_relids, root->outer_join_rels)) return true; + /* + * For an ordinary qual clause, we consider the actual clause_relids as + * explained above. However, it's possible for multiple members of a + * group of clone quals to have the same clause_relids, so for clones use + * the required_relids instead to ensure we select just one of them. + */ + if (rinfo->has_clone || rinfo->is_clone) + clause_relids = rinfo->required_relids; + else + clause_relids = rinfo->clause_relids; + foreach(lc, root->join_info_list) { SpecialJoinInfo *sjinfo = (SpecialJoinInfo *) lfirst(lc); diff --git a/src/test/regress/expected/join.out b/src/test/regress/expected/join.out index 8fa2b376f3..1fd75c8f58 100644 --- a/src/test/regress/expected/join.out +++ b/src/test/regress/expected/join.out @@ -2500,6 +2500,74 @@ select * from int4_tbl t1 -> Seq Scan on int4_tbl t4 (12 rows) +explain (costs off) +select * from int4_tbl t1 + left join (int4_tbl t2 left join int4_tbl t3 on t2.f1 > 0) on t2.f1 > 1 + left join int4_tbl t4 on t2.f1 > 2 and t3.f1 > 3 +where t1.f1 = coalesce(t2.f1, 1); + QUERY PLAN +---------------------------------------------------- + Nested Loop Left Join + Join Filter: ((t2.f1 > 2) AND (t3.f1 > 3)) + -> Nested Loop Left Join + Join Filter: (t2.f1 > 0) + -> Nested Loop Left Join + Filter: (t1.f1 = COALESCE(t2.f1, 1)) + -> Seq Scan on int4_tbl t1 + -> Materialize + -> Seq Scan on int4_tbl t2 + Filter: (f1 > 1) + -> Seq Scan on int4_tbl t3 + -> Materialize + -> Seq Scan on int4_tbl t4 +(13 rows) + +explain (costs off) +select * from int4_tbl t1 + left join ((select t2.f1 from int4_tbl t2 + left join int4_tbl t3 on t2.f1 > 0 + where t3.f1 is null) s + left join tenk1 t4 on s.f1 > 1) + on s.f1 = t1.f1; + QUERY PLAN +------------------------------------------------- + Nested Loop Left Join + Join Filter: (t2.f1 > 1) + -> Hash Right Join + Hash Cond: (t2.f1 = t1.f1) + -> Nested Loop Left Join + Join Filter: (t2.f1 > 0) + Filter: (t3.f1 IS NULL) + -> Seq Scan on int4_tbl t2 + -> Materialize + -> Seq Scan on int4_tbl t3 + -> Hash + -> Seq Scan on int4_tbl t1 + -> Seq Scan on tenk1 t4 +(13 rows) + +explain (costs off) +select * from onek t1 + left join onek t2 on t1.unique1 = t2.unique1 + left join onek t3 on t2.unique1 = t3.unique1 + left join onek t4 on t3.unique1 = t4.unique1 and t2.unique2 = t4.unique2; + QUERY PLAN +------------------------------------------------------------------------ + Hash Left Join + Hash Cond: ((t3.unique1 = t4.unique1) AND (t2.unique2 = t4.unique2)) + -> Hash Left Join + Hash Cond: (t2.unique1 = t3.unique1) + -> Hash Left Join + Hash Cond: (t1.unique1 = t2.unique1) + -> Seq Scan on onek t1 + -> Hash + -> Seq Scan on onek t2 + -> Hash + -> Seq Scan on onek t3 + -> Hash + -> Seq Scan on onek t4 +(13 rows) + -- -- check a case where we formerly got confused by conflicting sort orders -- in redundant merge join path keys diff --git a/src/test/regress/sql/join.sql b/src/test/regress/sql/join.sql index 641fd1a21b..84547c7dff 100644 --- a/src/test/regress/sql/join.sql +++ b/src/test/regress/sql/join.sql @@ -488,6 +488,26 @@ select * from int4_tbl t1 left join int4_tbl t3 on t2.f1 = t3.f1 left join int4_tbl t4 on t3.f1 != t4.f1; +explain (costs off) +select * from int4_tbl t1 + left join (int4_tbl t2 left join int4_tbl t3 on t2.f1 > 0) on t2.f1 > 1 + left join int4_tbl t4 on t2.f1 > 2 and t3.f1 > 3 +where t1.f1 = coalesce(t2.f1, 1); + +explain (costs off) +select * from int4_tbl t1 + left join ((select t2.f1 from int4_tbl t2 + left join int4_tbl t3 on t2.f1 > 0 + where t3.f1 is null) s + left join tenk1 t4 on s.f1 > 1) + on s.f1 = t1.f1; + +explain (costs off) +select * from onek t1 + left join onek t2 on t1.unique1 = t2.unique1 + left join onek t3 on t2.unique1 = t3.unique1 + left join onek t4 on t3.unique1 = t4.unique1 and t2.unique2 = t4.unique2; + -- -- check a case where we formerly got confused by conflicting sort orders -- in redundant merge join path keys ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Assert failure of the cross-check for nullingrels @ 2023-05-21 19:44 Tom Lane <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Tom Lane @ 2023-05-21 19:44 UTC (permalink / raw) To: Richard Guo <[email protected]>; +Cc: pgsql-hackers I wrote: > If they have the same clause_relids, then clearly in its current > form clause_is_computable_at() cannot distinguish them. So what > I now think we should do is have clause_is_computable_at() examine > their required_relids instead. Those will be different, by > construction in deconstruct_distribute_oj_quals(), ensuring that > we select only one of the group of clones. Since we're hard up against the beta1 wrap deadline, I went ahead and pushed the v5 patch. I doubt that it's perfect yet, but it's a small change and demonstrably fixes the cases we know about. As I said in the commit message, the main knock I'd lay on v5 is "why not use required_relids all the time?". I tried to do that and soon found that the answer is that we're not maintaining required_relids very accurately. I found two causes so far: 1. equivclass.c sometimes generates placeholder constant-true join clauses, and it's being sloppy about that. It copies the required_relids of the original clause, but fails to copy is_pushed_down, making the clause look like it's been assigned to the wrong side of the join-clause-vs-filter-clause divide. I found that we need to copy has_clone/is_clone as well. The attached quick-hack patch avoids the bugs, but now I feel like it was a mistake to not add has_clone/is_clone as full-fledged arguments of make_restrictinfo. I'm inclined to change that, but not right before beta1 when we have no evidence of a reachable bug. (Mind you, there might *be* a reachable bug, but ...) 2. When distribute_qual_to_rels forces a qual up to a particular syntactic level, it applies a relid set that very possibly refers to rels the clause doesn't actually depend on. This is problematic because if the clause gets postponed to above some outer join that nulls those rels, then it looks like it's being evaluated in an unsafe location. I think that when we detect commutability of two outer joins, we need to adjust the relevant min_xxxhand sets more thoroughly than we do now. I've not managed to write a patch for that yet. One problem is that if we insist on removing all unreferenced rels from required_relids, we might end up with a set that mentions *none* of the RHS and therefore fails to keep the clause from dropping into the LHS where it must not go. Not sure about a nice way to handle that. regards, tom lane Attachments: [text/x-diff] clean-up-const-true-clauses-wip.patch (3.9K, ../../[email protected]/2-clean-up-const-true-clauses-wip.patch) download | inline diff: diff --git a/src/backend/optimizer/path/equivclass.c b/src/backend/optimizer/path/equivclass.c index 2db1bf6448..e0077aa1e4 100644 --- a/src/backend/optimizer/path/equivclass.c +++ b/src/backend/optimizer/path/equivclass.c @@ -1993,20 +1993,24 @@ reconsider_outer_join_clauses(PlannerInfo *root) if (reconsider_outer_join_clause(root, ojcinfo, true)) { RestrictInfo *rinfo = ojcinfo->rinfo; + RestrictInfo *ninfo; found = true; /* remove it from the list */ root->left_join_clauses = foreach_delete_current(root->left_join_clauses, cell); /* throw back a dummy replacement clause (see notes above) */ - rinfo = make_restrictinfo(root, + ninfo = make_restrictinfo(root, (Expr *) makeBoolConst(true, false), - true, /* is_pushed_down */ - false, /* pseudoconstant */ + rinfo->is_pushed_down, + false, /* !pseudoconstant */ 0, /* security_level */ rinfo->required_relids, rinfo->outer_relids); - distribute_restrictinfo_to_rels(root, rinfo); + /* copy clone marking, too */ + ninfo->has_clone = rinfo->has_clone; + ninfo->is_clone = rinfo->is_clone; + distribute_restrictinfo_to_rels(root, ninfo); } } @@ -2018,20 +2022,24 @@ reconsider_outer_join_clauses(PlannerInfo *root) if (reconsider_outer_join_clause(root, ojcinfo, false)) { RestrictInfo *rinfo = ojcinfo->rinfo; + RestrictInfo *ninfo; found = true; /* remove it from the list */ root->right_join_clauses = foreach_delete_current(root->right_join_clauses, cell); /* throw back a dummy replacement clause (see notes above) */ - rinfo = make_restrictinfo(root, + ninfo = make_restrictinfo(root, (Expr *) makeBoolConst(true, false), - true, /* is_pushed_down */ - false, /* pseudoconstant */ + rinfo->is_pushed_down, + false, /* !pseudoconstant */ 0, /* security_level */ rinfo->required_relids, rinfo->outer_relids); - distribute_restrictinfo_to_rels(root, rinfo); + /* copy clone marking, too */ + ninfo->has_clone = rinfo->has_clone; + ninfo->is_clone = rinfo->is_clone; + distribute_restrictinfo_to_rels(root, ninfo); } } @@ -2043,20 +2051,24 @@ reconsider_outer_join_clauses(PlannerInfo *root) if (reconsider_full_join_clause(root, ojcinfo)) { RestrictInfo *rinfo = ojcinfo->rinfo; + RestrictInfo *ninfo; found = true; /* remove it from the list */ root->full_join_clauses = foreach_delete_current(root->full_join_clauses, cell); /* throw back a dummy replacement clause (see notes above) */ - rinfo = make_restrictinfo(root, + ninfo = make_restrictinfo(root, (Expr *) makeBoolConst(true, false), - true, /* is_pushed_down */ - false, /* pseudoconstant */ + rinfo->is_pushed_down, + false, /* !pseudoconstant */ 0, /* security_level */ rinfo->required_relids, rinfo->outer_relids); - distribute_restrictinfo_to_rels(root, rinfo); + /* copy clone marking, too */ + ninfo->has_clone = rinfo->has_clone; + ninfo->is_clone = rinfo->is_clone; + distribute_restrictinfo_to_rels(root, ninfo); } } } while (found); diff --git a/src/backend/optimizer/util/restrictinfo.c b/src/backend/optimizer/util/restrictinfo.c index 760d24bebf..8558c8acd4 100644 --- a/src/backend/optimizer/util/restrictinfo.c +++ b/src/backend/optimizer/util/restrictinfo.c @@ -513,8 +513,8 @@ extract_actual_join_clauses(List *restrictinfo_list, { /* joinquals shouldn't have been marked pseudoconstant */ Assert(!rinfo->pseudoconstant); - Assert(!rinfo_is_constant_true(rinfo)); - *joinquals = lappend(*joinquals, rinfo->clause); + if (!rinfo_is_constant_true(rinfo)) + *joinquals = lappend(*joinquals, rinfo->clause); } } } ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Assert failure of the cross-check for nullingrels @ 2023-06-06 20:11 Alvaro Herrera <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Alvaro Herrera @ 2023-06-06 20:11 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Richard Guo <[email protected]>; pgsql-hackers On 2023-May-21, Tom Lane wrote: > Since we're hard up against the beta1 wrap deadline, I went ahead > and pushed the v5 patch. I doubt that it's perfect yet, but it's > a small change and demonstrably fixes the cases we know about. > > As I said in the commit message, the main knock I'd lay on v5 > is "why not use required_relids all the time?". So, is this done? I see that you made other commits fixing related code several days after this email, but none seems to match the changes you posted in this patch; and also it's not clear to me that there's any test case where this patch is expected to change behavior. (So there's also a question of whether this is a bug fix or rather some icying on cake.) -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2023-06-06 20:11 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-05-28 18:15 [PATCH v14 1/7] Remove pg_collation.collversion. Thomas Munro <[email protected]> 2023-05-20 15:24 Re: Assert failure of the cross-check for nullingrels Tom Lane <[email protected]> 2023-05-21 19:44 ` Re: Assert failure of the cross-check for nullingrels Tom Lane <[email protected]> 2023-06-06 20:11 ` Re: Assert failure of the cross-check for nullingrels Alvaro Herrera <[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