public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v27 1/5] Remove pg_collation.collversion. 16+ messages / 7 participants [nested] [flat]
* [PATCH v27 1/5] Remove pg_collation.collversion. @ 2019-05-28 18:15 Thomas Munro <[email protected]> 0 siblings, 0 replies; 16+ 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 <[email protected]> Reviewed-by: Julien Rouhaud <[email protected]> Reviewed-by: Peter Eisentraut <[email protected]> Reviewed-by: Michael Paquier <[email protected]> Discussion: https://postgr.es/m/CAEepm%3D0uEQCpfq_%2BLYFBdArCe4Ot98t1aR4eYiYTe%3DyavQygiQ%40mail.gmail.com --- doc/src/sgml/catalogs.sgml | 11 --- doc/src/sgml/func.sgml | 6 +- doc/src/sgml/ref/alter_collation.sgml | 63 ------------- doc/src/sgml/ref/create_collation.sgml | 21 ----- src/backend/catalog/pg_collation.c | 5 -- src/backend/commands/collationcmds.c | 88 ------------------- 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 | 24 +---- 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, 7 insertions(+), 343 deletions(-) diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml index 26fda20d19..a28ae89e13 100644 --- a/doc/src/sgml/catalogs.sgml +++ b/doc/src/sgml/catalogs.sgml @@ -2353,17 +2353,6 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l <symbol>LC_CTYPE</symbol> for this collation object </para></entry> </row> - - <row> - <entry role="catalog_table_entry"><para role="column_definition"> - <structfield>collversion</structfield> <type>text</type> - </para> - <para> - 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. - </para></entry> - </row> </tbody> </tgroup> </table> diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml index f766c1bc67..8432e790f2 100644 --- a/doc/src/sgml/func.sgml +++ b/doc/src/sgml/func.sgml @@ -25438,11 +25438,7 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup()); </para> <para> 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 - <structname>pg_collation</structname>.<structfield>collversion</structfield>, - then objects depending on the collation might need to be rebuilt. See - also <xref linkend="sql-altercollation"/>. + installed in the operating system. </para></entry> </row> diff --git a/doc/src/sgml/ref/alter_collation.sgml b/doc/src/sgml/ref/alter_collation.sgml index bee6f0dd3c..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,70 +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"/> below. - </para> - </listitem> - </varlistentry> </variablelist> </refsect1> - <refsect1 id="sql-altercollation-notes" xreflabel="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 58f5f0cd63..b97842071f 100644 --- a/doc/src/sgml/ref/create_collation.sgml +++ b/doc/src/sgml/ref/create_collation.sgml @@ -27,7 +27,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> @@ -149,26 +148,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 5fdf1acb7e..3c84378d02 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..5ad8886e60 100644 --- a/src/backend/commands/collationcmds.c +++ b/src/backend/commands/collationcmds.c @@ -61,14 +61,12 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e DefElem *lcctypeEl = NULL; DefElem *providerEl = NULL; DefElem *deterministicEl = NULL; - DefElem *versionEl = NULL; char *collcollate = NULL; char *collctype = NULL; char *collproviderstr = NULL; bool collisdeterministic = true; int collencoding = 0; char collprovider = 0; - char *collversion = NULL; Oid newoid; ObjectAddress address; @@ -96,8 +94,6 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e defelp = &providerEl; else if (strcmp(defel->defname, "deterministic") == 0) defelp = &deterministicEl; - else if (strcmp(defel->defname, "version") == 0) - defelp = &versionEl; else { ereport(ERROR, @@ -166,9 +162,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 +208,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 +216,6 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e collencoding, collcollate, collctype, - collversion, if_not_exists, false); /* not quiet */ @@ -277,80 +266,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 +523,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 +583,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 +644,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 89c409de66..2218f6e3db 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -3226,16 +3226,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) { @@ -5232,9 +5222,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 e3f33c40be..40eb879176 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -1107,14 +1107,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) { @@ -3284,9 +3276,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 dbb47d4982..e432049f8e 100644 --- a/src/backend/parser/gram.y +++ b/src/backend/parser/gram.y @@ -254,7 +254,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 @@ -838,7 +838,6 @@ stmtmulti: stmtmulti ';' stmt stmt : AlterEventTrigStmt - | AlterCollationStmt | AlterDatabaseStmt | AlterDatabaseSetStmt | AlterDefaultPrivilegesStmt @@ -10137,21 +10136,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 9b0c376c8c..fb8a964cc4 100644 --- a/src/backend/tcop/utility.c +++ b/src/backend/tcop/utility.c @@ -1795,10 +1795,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)); @@ -2946,10 +2942,6 @@ CreateCommandTag(Node *parsetree) tag = CMDTAG_DROP_SUBSCRIPTION; break; - case T_AlterCollationStmt: - tag = CMDTAG_ALTER_COLLATION; - break; - case T_PrepareStmt: tag = CMDTAG_PREPARE; break; @@ -3562,10 +3554,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 07299dbc09..514e0fa0af 100644 --- a/src/backend/utils/adt/pg_locale.c +++ b/src/backend/utils/adt/pg_locale.c @@ -1513,8 +1513,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)) @@ -1616,41 +1614,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 9c8436dde6..af8d38488e 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -13548,12 +13548,10 @@ dumpCollation(Archive *fout, CollInfo *collinfo) if (fout->remoteVersion >= 100000) appendPQExpBufferStr(query, - "collprovider, " - "collversion, "); + "collprovider, "); else appendPQExpBufferStr(query, - "'c' AS collprovider, " - "NULL AS collversion, "); + "'c' AS collprovider, "); if (fout->remoteVersion >= 120000) appendPQExpBufferStr(query, @@ -13614,24 +13612,6 @@ dumpCollation(Archive *fout, CollInfo *collinfo) appendStringLiteralAH(q, collctype, fout); } - /* - * 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) - { - int i_collversion; - - i_collversion = PQfnumber(res, "collversion"); - if (!PQgetisnull(res, 0, i_collversion)) - { - appendPQExpBufferStr(q, ", version = "); - appendStringLiteralAH(q, - PQgetvalue(res, 0, i_collversion), - fout); - } - } - appendPQExpBufferStr(q, ");\n"); if (dopt->binary_upgrade) 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 27618b324d..e7e958b808 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); 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 151bcdb7ef..a8da438d22 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -1873,17 +1873,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 --o56keubw4jpehgac Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="v27-0002-Add-pg_depend.refobjversion.patch" ^ permalink raw reply [nested|flat] 16+ messages in thread
* Parameter for planner estimate of recursive queries @ 2021-10-27 14:58 Simon Riggs <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Simon Riggs @ 2021-10-27 14:58 UTC (permalink / raw) To: pgsql-hackers I've been investigating the poor performance of a WITH RECURSIVE query, which I've recreated with test data. The first thing was to re-write the query, which helped improve performance by about 30%, but the plan was still very bad. With a small patch I've been able to improve performance by about x100. The poor performance is traced to the planner cost estimates for recursive queries. Specifically, the cost of the recursive arm of the query is evaluated based upon both of these hardcoded assumptions: 1. The recursion will last for 10 loops 2. The average size of the worktable will be 10x the size of the initial query (non-recursive term). Taken together these assumptions lead to a very poor estimate of the worktable activity (in this case), which leads to the plan changing as a result. The factor 10 is a reasonably safe assumption and helps avoid worst case behavior in bigger graph queries. However, the factor 10 is way too large for many types of graph query, such as where the path through the data is tight, and/or the query is written to prune bushy graphs, e.g. shortest path queries. The factor 10 should not be hardcoded in the planner, but should be settable, just as cursor_tuple_fraction is. I've written a short patch to make the estimate of the avg size of the worktable configurable: recursive_worktable_estimate = N (default 10) Using this parameter with the test query results in a consistently repeatable ~100x gain in performance, using recursive_worktable_estimate = 1 for a shortest path query: Unpatched: 1775ms Patched: 17.2ms This is because the estimated size of the worktable is closer to the truth and so leads naturally to a more sensible plan. EXPLAINs attached - please look at the estimated rows for the WorkTable Scan. There are various options for setting the two estimates: just one, or other, or both values separately, or both together. Note that I haven't touched the estimate that recursion will last for 10 loops. I figured that people would object to two knobs. Thoughts? There are 2 other ways to speed up the query. One is to set enable_seqscan = off, which helps about 20%, but may have other consequences. Two is to set work_mem = 512MB, so that the poor plan (hash) works as fast as possible, but that is far from optimal either. Getting the right plan is x20 faster than either of those alternatives. -- Simon Riggs http://www.EnterpriseDB.com/ Attachments: [application/octet-stream] recursive_worktable_estimate.v1.patch (2.9K, ../../CANbhV-EuaLm4H3g0+BSTYHEGxJj3Kht0R+rJ8vT57Dejnh=_nA@mail.gmail.com/2-recursive_worktable_estimate.v1.patch) download | inline diff: diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c index 1e4d404f02..9b189de91d 100644 --- a/src/backend/optimizer/path/costsize.c +++ b/src/backend/optimizer/path/costsize.c @@ -123,6 +123,7 @@ double cpu_index_tuple_cost = DEFAULT_CPU_INDEX_TUPLE_COST; double cpu_operator_cost = DEFAULT_CPU_OPERATOR_COST; double parallel_tuple_cost = DEFAULT_PARALLEL_TUPLE_COST; double parallel_setup_cost = DEFAULT_PARALLEL_SETUP_COST; +double recursive_worktable_estimate = DEFAULT_RECURSIVE_WORKTABLE_ESTIMATE; int effective_cache_size = DEFAULT_EFFECTIVE_CACHE_SIZE; @@ -5659,10 +5660,11 @@ set_cte_size_estimates(PlannerInfo *root, RelOptInfo *rel, double cte_rows) if (rte->self_reference) { /* - * In a self-reference, arbitrarily assume the average worktable size - * is about 10 times the nonrecursive term's size. + * Use recursive_worktable_estimate to get average size of worktable, + * across all iterations. This will vary depending upon how bushy the + * data is, so allow the user to set based upon their data. */ - rel->tuples = 10 * cte_rows; + rel->tuples = recursive_worktable_estimate * cte_rows; } else { diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index d2ce4a8450..c84643bc0d 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -3702,6 +3702,19 @@ static struct config_real ConfigureNamesReal[] = NULL, NULL, NULL }, + { + {"recursive_worktable_estimate", PGC_USERSET, QUERY_TUNING_OTHER, + gettext_noop("Sets the planner's estimate of the size of " + "the recursive_worktable as a multiple of the " + "initial non-recursive query size."), + NULL, + GUC_EXPLAIN + }, + &recursive_worktable_estimate, + DEFAULT_RECURSIVE_WORKTABLE_ESTIMATE, 0.0, 1000000.0, + NULL, NULL, NULL + }, + { {"geqo_selection_bias", PGC_USERSET, QUERY_TUNING_GEQO, gettext_noop("GEQO: selective pressure within the population."), diff --git a/src/include/optimizer/cost.h b/src/include/optimizer/cost.h index 2113bc82de..9af7a6eea3 100644 --- a/src/include/optimizer/cost.h +++ b/src/include/optimizer/cost.h @@ -29,6 +29,8 @@ #define DEFAULT_PARALLEL_TUPLE_COST 0.1 #define DEFAULT_PARALLEL_SETUP_COST 1000.0 +#define DEFAULT_RECURSIVE_WORKTABLE_ESTIMATE 10.0 + #define DEFAULT_EFFECTIVE_CACHE_SIZE 524288 /* measured in pages */ typedef enum diff --git a/src/include/optimizer/optimizer.h b/src/include/optimizer/optimizer.h index 41b49b2662..eccac5039f 100644 --- a/src/include/optimizer/optimizer.h +++ b/src/include/optimizer/optimizer.h @@ -92,6 +92,7 @@ extern PGDLLIMPORT double cpu_operator_cost; extern PGDLLIMPORT double parallel_tuple_cost; extern PGDLLIMPORT double parallel_setup_cost; extern PGDLLIMPORT int effective_cache_size; +extern PGDLLIMPORT double recursive_worktable_estimate; extern double clamp_row_est(double nrows); [application/octet-stream] graph_query_explains.out (4.9K, ../../CANbhV-EuaLm4H3g0+BSTYHEGxJj3Kht0R+rJ8vT57Dejnh=_nA@mail.gmail.com/3-graph_query_explains.out) download ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Parameter for planner estimate of recursive queries @ 2021-12-31 14:10 Simon Riggs <[email protected]> parent: Simon Riggs <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Simon Riggs @ 2021-12-31 14:10 UTC (permalink / raw) To: pgsql-hackers On Wed, 27 Oct 2021 at 15:58, Simon Riggs <[email protected]> wrote: > The poor performance is traced to the planner cost estimates for > recursive queries. Specifically, the cost of the recursive arm of the > query is evaluated based upon both of these hardcoded assumptions: > > 1. The recursion will last for 10 loops > 2. The average size of the worktable will be 10x the size of the > initial query (non-recursive term). > > Taken together these assumptions lead to a very poor estimate of the > worktable activity (in this case), which leads to the plan changing as > a result. > > The factor 10 is a reasonably safe assumption and helps avoid worst > case behavior in bigger graph queries. However, the factor 10 is way > too large for many types of graph query, such as where the path > through the data is tight, and/or the query is written to prune bushy > graphs, e.g. shortest path queries. The factor 10 should not be > hardcoded in the planner, but should be settable, just as > cursor_tuple_fraction is. If you think this should be derived without parameters, then we would want a function that starts at 1 for 1 input row and gets much larger for larger input. The thinking here is that Graph OLTP is often a shortest path between two nodes, whereas Graph Analytics and so the worktable will get much bigger. So I'm, thinking we use rel->tuples = min(1, cte_rows * cte_rows/2); -- Simon Riggs http://www.EnterpriseDB.com/ ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Parameter for planner estimate of recursive queries @ 2022-01-25 09:44 Peter Eisentraut <[email protected]> parent: Simon Riggs <[email protected]> 0 siblings, 2 replies; 16+ messages in thread From: Peter Eisentraut @ 2022-01-25 09:44 UTC (permalink / raw) To: Simon Riggs <[email protected]>; pgsql-hackers On 31.12.21 15:10, Simon Riggs wrote: >> The factor 10 is a reasonably safe assumption and helps avoid worst >> case behavior in bigger graph queries. However, the factor 10 is way >> too large for many types of graph query, such as where the path >> through the data is tight, and/or the query is written to prune bushy >> graphs, e.g. shortest path queries. The factor 10 should not be >> hardcoded in the planner, but should be settable, just as >> cursor_tuple_fraction is. > If you think this should be derived without parameters, then we would > want a function that starts at 1 for 1 input row and gets much larger > for larger input. The thinking here is that Graph OLTP is often a > shortest path between two nodes, whereas Graph Analytics and so the > worktable will get much bigger. On the one hand, this smells like a planner hint. But on the other hand, it doesn't look like we will come up with proper graph-aware selectivity estimation system any time soon, so just having all graph OLTP queries suck until then because the planner hint is hardcoded doesn't seem like a better solution. So I think this setting can be ok. I think the way you have characterized it makes sense, too: for graph OLAP, you want a larger value, for graph OLTP, you want a smaller value. ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Parameter for planner estimate of recursive queries @ 2022-01-28 13:40 Hamid Akhtar <[email protected]> parent: Peter Eisentraut <[email protected]> 1 sibling, 0 replies; 16+ messages in thread From: Hamid Akhtar @ 2022-01-28 13:40 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Simon Riggs <[email protected]>; pgsql-hackers On Tue, 25 Jan 2022 at 14:44, Peter Eisentraut < [email protected]> wrote: > On 31.12.21 15:10, Simon Riggs wrote: > >> The factor 10 is a reasonably safe assumption and helps avoid worst > >> case behavior in bigger graph queries. However, the factor 10 is way > >> too large for many types of graph query, such as where the path > >> through the data is tight, and/or the query is written to prune bushy > >> graphs, e.g. shortest path queries. The factor 10 should not be > >> hardcoded in the planner, but should be settable, just as > >> cursor_tuple_fraction is. > > If you think this should be derived without parameters, then we would > > want a function that starts at 1 for 1 input row and gets much larger > > for larger input. The thinking here is that Graph OLTP is often a > > shortest path between two nodes, whereas Graph Analytics and so the > > worktable will get much bigger. > > On the one hand, this smells like a planner hint. But on the other > hand, it doesn't look like we will come up with proper graph-aware > selectivity estimation system any time soon, so just having all graph > OLTP queries suck until then because the planner hint is hardcoded > doesn't seem like a better solution. So I think this setting can be ok. > I think the way you have characterized it makes sense, too: for graph > OLAP, you want a larger value, for graph OLTP, you want a smaller value. > Do you think there is a case to replace the 10x multiplier with "recursive_worktable_estimate" for total_rows calculation in the cost_recursive_union function too? ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Parameter for planner estimate of recursive queries @ 2022-01-28 14:07 Robert Haas <[email protected]> parent: Peter Eisentraut <[email protected]> 1 sibling, 2 replies; 16+ messages in thread From: Robert Haas @ 2022-01-28 14:07 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: Simon Riggs <[email protected]>; pgsql-hackers On Tue, Jan 25, 2022 at 4:44 AM Peter Eisentraut <[email protected]> wrote: > On the one hand, this smells like a planner hint. But on the other > hand, it doesn't look like we will come up with proper graph-aware > selectivity estimation system any time soon, so just having all graph > OLTP queries suck until then because the planner hint is hardcoded > doesn't seem like a better solution. So I think this setting can be ok. I agree. It's a bit lame, but seems pretty harmless, and I can't see us realistically doing a lot better with any reasonable amount of work. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Parameter for planner estimate of recursive queries @ 2022-03-10 17:42 Simon Riggs <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 1 reply; 16+ messages in thread From: Simon Riggs @ 2022-03-10 17:42 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; pgsql-hackers On Fri, 28 Jan 2022 at 14:07, Robert Haas <[email protected]> wrote: > > On Tue, Jan 25, 2022 at 4:44 AM Peter Eisentraut > <[email protected]> wrote: > > On the one hand, this smells like a planner hint. But on the other > > hand, it doesn't look like we will come up with proper graph-aware > > selectivity estimation system any time soon, so just having all graph > > OLTP queries suck until then because the planner hint is hardcoded > > doesn't seem like a better solution. So I think this setting can be ok. > > I agree. It's a bit lame, but seems pretty harmless, and I can't see > us realistically doing a lot better with any reasonable amount of > work. Shall I set this as Ready For Committer? -- Simon Riggs http://www.EnterpriseDB.com/ ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Parameter for planner estimate of recursive queries @ 2022-03-22 00:04 Andres Freund <[email protected]> parent: Simon Riggs <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Andres Freund @ 2022-03-22 00:04 UTC (permalink / raw) To: Simon Riggs <[email protected]>; +Cc: Robert Haas <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers Hi, On 2022-03-10 17:42:14 +0000, Simon Riggs wrote: > Shall I set this as Ready For Committer? Currently this CF entry fails on cfbot: https://cirrus-ci.com/task/4531771134967808?logs=test_world#L1158 [16:27:35.772] # Failed test 'no parameters missing from postgresql.conf.sample' [16:27:35.772] # at t/003_check_guc.pl line 82. [16:27:35.772] # got: '1' [16:27:35.772] # expected: '0' [16:27:35.772] # Looks like you failed 1 test of 3. [16:27:35.772] [16:27:35] t/003_check_guc.pl .............. Marked as waiting on author. - Andres ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Parameter for planner estimate of recursive queries @ 2022-03-23 14:53 Simon Riggs <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 0 replies; 16+ messages in thread From: Simon Riggs @ 2022-03-23 14:53 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Robert Haas <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers On Tue, 22 Mar 2022 at 00:04, Andres Freund <[email protected]> wrote: > On 2022-03-10 17:42:14 +0000, Simon Riggs wrote: > > Shall I set this as Ready For Committer? > > Currently this CF entry fails on cfbot: https://cirrus-ci.com/task/4531771134967808?logs=test_world#L1158 > > [16:27:35.772] # Failed test 'no parameters missing from postgresql.conf.sample' > [16:27:35.772] # at t/003_check_guc.pl line 82. > [16:27:35.772] # got: '1' > [16:27:35.772] # expected: '0' > [16:27:35.772] # Looks like you failed 1 test of 3. > [16:27:35.772] [16:27:35] t/003_check_guc.pl .............. > > Marked as waiting on author. Thanks. I've corrected that by adding a line to postgresql.conf.sample, as well as adding docs. -- Simon Riggs http://www.EnterpriseDB.com/ Attachments: [application/octet-stream] recursive_worktable_estimate.v2.patch (4.7K, ../../CANbhV-EPrq9Z1gSHTeXVrxRHZq91=5VWDTBrbG=DaPn5oHe08A@mail.gmail.com/2-recursive_worktable_estimate.v2.patch) download | inline diff: diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 7a48973b3c..86673b4fba 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -5919,6 +5919,25 @@ SELECT * FROM parent WHERE key = 2400; </listitem> </varlistentry> + <varlistentry id="guc-recursive-worktable-estimate" xreflabel="recursive_worktable_estimate"> + <term><varname>recursive_worktable_estimate</varname> (<type>floating point</type>) + <indexterm> + <primary><varname>recursive_worktable_estimate</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + Sets the planner's estimate of the average size of the worktable, if + a recursive query is requested, as a multiple of the number of rows in the + initial non-recursive term. The default is 10.0. Smaller values of this + setting bias the planner towards using <quote>fast start</quote> plans + for recursive queries, which will retrieve the first few rows quickly while + perhaps taking a long time to fetch all rows. A setting of 1.0 is appropriate + for shortest path queries; larger values may be better for graph analytics. + </para> + </listitem> + </varlistentry> + </variablelist> </sect2> </sect1> diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c index 4d9f3b4bb6..31e8de4bf3 100644 --- a/src/backend/optimizer/path/costsize.c +++ b/src/backend/optimizer/path/costsize.c @@ -123,6 +123,7 @@ double cpu_index_tuple_cost = DEFAULT_CPU_INDEX_TUPLE_COST; double cpu_operator_cost = DEFAULT_CPU_OPERATOR_COST; double parallel_tuple_cost = DEFAULT_PARALLEL_TUPLE_COST; double parallel_setup_cost = DEFAULT_PARALLEL_SETUP_COST; +double recursive_worktable_estimate = DEFAULT_RECURSIVE_WORKTABLE_ESTIMATE; int effective_cache_size = DEFAULT_EFFECTIVE_CACHE_SIZE; @@ -5665,10 +5666,11 @@ set_cte_size_estimates(PlannerInfo *root, RelOptInfo *rel, double cte_rows) if (rte->self_reference) { /* - * In a self-reference, arbitrarily assume the average worktable size - * is about 10 times the nonrecursive term's size. + * Use recursive_worktable_estimate to get average size of worktable, + * across all iterations. This will vary depending upon how bushy the + * data is, so allow the user to set based upon their data. */ - rel->tuples = 10 * cte_rows; + rel->tuples = recursive_worktable_estimate * cte_rows; } else { diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 932aefc777..60dfa66b28 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -3740,6 +3740,19 @@ static struct config_real ConfigureNamesReal[] = NULL, NULL, NULL }, + { + {"recursive_worktable_estimate", PGC_USERSET, QUERY_TUNING_OTHER, + gettext_noop("Sets the planner's estimate of the size of " + "the recursive_worktable as a multiple of the " + "initial non-recursive query size."), + NULL, + GUC_EXPLAIN + }, + &recursive_worktable_estimate, + DEFAULT_RECURSIVE_WORKTABLE_ESTIMATE, 0.0, 1000000.0, + NULL, NULL, NULL + }, + { {"geqo_selection_bias", PGC_USERSET, QUERY_TUNING_GEQO, gettext_noop("GEQO: selective pressure within the population."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index 4cf5b26a36..c7eeb11a17 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -426,6 +426,7 @@ # JOIN clauses #plan_cache_mode = auto # auto, force_generic_plan or # force_custom_plan +#recursive_worktable_estimate = 10.0 # Set to 1.0 for shortest path queries #------------------------------------------------------------------------------ diff --git a/src/include/optimizer/cost.h b/src/include/optimizer/cost.h index 356a51f370..1afaae5e61 100644 --- a/src/include/optimizer/cost.h +++ b/src/include/optimizer/cost.h @@ -29,6 +29,8 @@ #define DEFAULT_PARALLEL_TUPLE_COST 0.1 #define DEFAULT_PARALLEL_SETUP_COST 1000.0 +#define DEFAULT_RECURSIVE_WORKTABLE_ESTIMATE 10.0 + #define DEFAULT_EFFECTIVE_CACHE_SIZE 524288 /* measured in pages */ typedef enum diff --git a/src/include/optimizer/optimizer.h b/src/include/optimizer/optimizer.h index 6b8ee0c69f..5e60f0bb27 100644 --- a/src/include/optimizer/optimizer.h +++ b/src/include/optimizer/optimizer.h @@ -92,6 +92,7 @@ extern PGDLLIMPORT double cpu_operator_cost; extern PGDLLIMPORT double parallel_tuple_cost; extern PGDLLIMPORT double parallel_setup_cost; extern PGDLLIMPORT int effective_cache_size; +extern PGDLLIMPORT double recursive_worktable_estimate; extern double clamp_row_est(double nrows); ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Parameter for planner estimate of recursive queries @ 2022-03-23 17:36 Tom Lane <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 1 reply; 16+ messages in thread From: Tom Lane @ 2022-03-23 17:36 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Simon Riggs <[email protected]>; pgsql-hackers Robert Haas <[email protected]> writes: > On Tue, Jan 25, 2022 at 4:44 AM Peter Eisentraut > <[email protected]> wrote: >> On the one hand, this smells like a planner hint. But on the other >> hand, it doesn't look like we will come up with proper graph-aware >> selectivity estimation system any time soon, so just having all graph >> OLTP queries suck until then because the planner hint is hardcoded >> doesn't seem like a better solution. So I think this setting can be ok. > I agree. It's a bit lame, but seems pretty harmless, and I can't see > us realistically doing a lot better with any reasonable amount of > work. Yeah, agreed on all counts. The thing that makes it lame is that there's no reason to expect that the same multiplier is good for every recursive query done in an installation, or even in a session. One could imagine dealing with that by adding custom syntax to WITH, as we have already done once: WITH RECURSIVE cte1 AS SCALE 1.0 (SELECT ... But I *really* hesitate to go there, mainly because once we do something like that we can't ever undo it. I think Simon's proposal is a reasonable low-effort compromise. Some nitpicks: * The new calculation needs clamp_row_est(), since the float GUC could be fractional or even zero. * Do we want to prevent the GUC value from being zero? It's not very sensible, plus I think we might want to reserve that value to mean "use the built-in calculation", in case we ever do put in some smarter logic here. But I'm not sure what a reasonable non-zero lower bound would be. * The proposed docs claim that a smaller setting works by biasing the planner towards fast-start plans, but I don't think I believe that explanation. I'd venture that we want text more along the lines of "This may help the planner choose the most appropriate method for joining the work table to the query's other tables". regards, tom lane ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Parameter for planner estimate of recursive queries @ 2022-03-23 18:20 Simon Riggs <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Simon Riggs @ 2022-03-23 18:20 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers On Wed, 23 Mar 2022 at 17:36, Tom Lane <[email protected]> wrote: > > Robert Haas <[email protected]> writes: > > On Tue, Jan 25, 2022 at 4:44 AM Peter Eisentraut > > <[email protected]> wrote: > >> On the one hand, this smells like a planner hint. But on the other > >> hand, it doesn't look like we will come up with proper graph-aware > >> selectivity estimation system any time soon, so just having all graph > >> OLTP queries suck until then because the planner hint is hardcoded > >> doesn't seem like a better solution. So I think this setting can be ok. > > > I agree. It's a bit lame, but seems pretty harmless, and I can't see > > us realistically doing a lot better with any reasonable amount of > > work. > > Yeah, agreed on all counts. The thing that makes it lame is that > there's no reason to expect that the same multiplier is good for > every recursive query done in an installation, or even in a session. > > One could imagine dealing with that by adding custom syntax to WITH, > as we have already done once: > > WITH RECURSIVE cte1 AS SCALE 1.0 (SELECT ... > > But I *really* hesitate to go there, mainly because once we do > something like that we can't ever undo it. I think Simon's > proposal is a reasonable low-effort compromise. > > Some nitpicks: > > * The new calculation needs clamp_row_est(), since the float > GUC could be fractional or even zero. True, will do. > * Do we want to prevent the GUC value from being zero? It's not > very sensible, plus I think we might want to reserve that value > to mean "use the built-in calculation", in case we ever do put > in some smarter logic here. But I'm not sure what a reasonable > non-zero lower bound would be. Agreed, makes sense. > * The proposed docs claim that a smaller setting works by biasing > the planner towards fast-start plans, but I don't think I believe > that explanation. I'd venture that we want text more along the > lines of "This may help the planner choose the most appropriate > method for joining the work table to the query's other tables". OK, will improve. [New patch version pending] -- Simon Riggs http://www.EnterpriseDB.com/ ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Parameter for planner estimate of recursive queries @ 2022-03-23 19:37 Simon Riggs <[email protected]> parent: Simon Riggs <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Simon Riggs @ 2022-03-23 19:37 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers On Wed, 23 Mar 2022 at 18:20, Simon Riggs <[email protected]> wrote: > [New patch version pending] -- Simon Riggs http://www.EnterpriseDB.com/ Attachments: [application/octet-stream] recursive_worktable_estimate.v3.patch (4.8K, ../../CANbhV-FGxKuPNo2QPnzVnb=S=u4zq7DvmxV7LaAvtSTxVn5kFQ@mail.gmail.com/2-recursive_worktable_estimate.v3.patch) download | inline diff: diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 7a48973b3c..d440429a2c 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -5919,6 +5919,23 @@ SELECT * FROM parent WHERE key = 2400; </listitem> </varlistentry> + <varlistentry id="guc-recursive-worktable-estimate" xreflabel="recursive_worktable_estimate"> + <term><varname>recursive_worktable_estimate</varname> (<type>floating point</type>) + <indexterm> + <primary><varname>recursive_worktable_estimate</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + Sets the planner's estimate of the average size of the worktable as a multiple of + the initial non-recursive term of the query. The default is 10.0. + This helps the planner choose the most appropriate method for joining the worktable + to the query's other tables. A setting of 1.0 is appropriate for shortest path + queries; graph analytics queries may benefit from larger values. + </para> + </listitem> + </varlistentry> + </variablelist> </sect2> </sect1> diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c index 4d9f3b4bb6..4015efb314 100644 --- a/src/backend/optimizer/path/costsize.c +++ b/src/backend/optimizer/path/costsize.c @@ -123,6 +123,7 @@ double cpu_index_tuple_cost = DEFAULT_CPU_INDEX_TUPLE_COST; double cpu_operator_cost = DEFAULT_CPU_OPERATOR_COST; double parallel_tuple_cost = DEFAULT_PARALLEL_TUPLE_COST; double parallel_setup_cost = DEFAULT_PARALLEL_SETUP_COST; +double recursive_worktable_estimate = DEFAULT_RECURSIVE_WORKTABLE_ESTIMATE; int effective_cache_size = DEFAULT_EFFECTIVE_CACHE_SIZE; @@ -5664,11 +5665,20 @@ set_cte_size_estimates(PlannerInfo *root, RelOptInfo *rel, double cte_rows) if (rte->self_reference) { + double multiplier = recursive_worktable_estimate; + + /* + * Reserve zero as a special case meaning use-the-built-in-calculation. + */ + if (recursive_worktable_estimate == 0.0) + multiplier = DEFAULT_RECURSIVE_WORKTABLE_ESTIMATE; + /* - * In a self-reference, arbitrarily assume the average worktable size - * is about 10 times the nonrecursive term's size. + * Use recursive_worktable_estimate to get average size of worktable, + * across all iterations. This will vary depending upon how bushy the + * data is, so allow the user to set based upon their data. */ - rel->tuples = 10 * cte_rows; + rel->tuples = clamp_row_est(multiplier * cte_rows); } else { diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 932aefc777..677fdcc46f 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -3740,6 +3740,19 @@ static struct config_real ConfigureNamesReal[] = NULL, NULL, NULL }, + { + {"recursive_worktable_estimate", PGC_USERSET, QUERY_TUNING_OTHER, + gettext_noop("Sets the planner's estimate of the average size " + "of the worktable as a multiple of the initial " + "non-recursive term of the query."), + NULL, + GUC_EXPLAIN + }, + &recursive_worktable_estimate, + DEFAULT_RECURSIVE_WORKTABLE_ESTIMATE, 0.0, 1000000.0, + NULL, NULL, NULL + }, + { {"geqo_selection_bias", PGC_USERSET, QUERY_TUNING_GEQO, gettext_noop("GEQO: selective pressure within the population."), diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample index 4cf5b26a36..c7eeb11a17 100644 --- a/src/backend/utils/misc/postgresql.conf.sample +++ b/src/backend/utils/misc/postgresql.conf.sample @@ -426,6 +426,7 @@ # JOIN clauses #plan_cache_mode = auto # auto, force_generic_plan or # force_custom_plan +#recursive_worktable_estimate = 10.0 # Set to 1.0 for shortest path queries #------------------------------------------------------------------------------ diff --git a/src/include/optimizer/cost.h b/src/include/optimizer/cost.h index 356a51f370..1afaae5e61 100644 --- a/src/include/optimizer/cost.h +++ b/src/include/optimizer/cost.h @@ -29,6 +29,8 @@ #define DEFAULT_PARALLEL_TUPLE_COST 0.1 #define DEFAULT_PARALLEL_SETUP_COST 1000.0 +#define DEFAULT_RECURSIVE_WORKTABLE_ESTIMATE 10.0 + #define DEFAULT_EFFECTIVE_CACHE_SIZE 524288 /* measured in pages */ typedef enum diff --git a/src/include/optimizer/optimizer.h b/src/include/optimizer/optimizer.h index 6b8ee0c69f..5e60f0bb27 100644 --- a/src/include/optimizer/optimizer.h +++ b/src/include/optimizer/optimizer.h @@ -92,6 +92,7 @@ extern PGDLLIMPORT double cpu_operator_cost; extern PGDLLIMPORT double parallel_tuple_cost; extern PGDLLIMPORT double parallel_setup_cost; extern PGDLLIMPORT int effective_cache_size; +extern PGDLLIMPORT double recursive_worktable_estimate; extern double clamp_row_est(double nrows); ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Parameter for planner estimate of recursive queries @ 2022-03-23 20:25 Tom Lane <[email protected]> parent: Simon Riggs <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Tom Lane @ 2022-03-23 20:25 UTC (permalink / raw) To: Simon Riggs <[email protected]>; +Cc: Robert Haas <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers Simon Riggs <[email protected]> writes: >> [New patch version pending] Do you have any objection if I rename the GUC to recursive_worktable_factor? That seems a bit clearer as to what it does, and it leaves more room for other knobs in the same area if we decide we need any. regards, tom lane ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Parameter for planner estimate of recursive queries @ 2022-03-24 10:32 Simon Riggs <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Simon Riggs @ 2022-03-24 10:32 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers On Wed, 23 Mar 2022 at 20:25, Tom Lane <[email protected]> wrote: > > Simon Riggs <[email protected]> writes: > >> [New patch version pending] > > Do you have any objection if I rename the GUC to > recursive_worktable_factor? That seems a bit clearer as to what > it does, and it leaves more room for other knobs in the same area > if we decide we need any. None, I think your proposal is a better name. -- Simon Riggs http://www.EnterpriseDB.com/ ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Parameter for planner estimate of recursive queries @ 2022-03-24 15:48 Tom Lane <[email protected]> parent: Simon Riggs <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Tom Lane @ 2022-03-24 15:48 UTC (permalink / raw) To: Simon Riggs <[email protected]>; +Cc: Robert Haas <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers Simon Riggs <[email protected]> writes: > On Wed, 23 Mar 2022 at 20:25, Tom Lane <[email protected]> wrote: >> Do you have any objection if I rename the GUC to >> recursive_worktable_factor? That seems a bit clearer as to what >> it does, and it leaves more room for other knobs in the same area >> if we decide we need any. > None, I think your proposal is a better name. Pushed that way. regards, tom lane ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Parameter for planner estimate of recursive queries @ 2022-03-24 16:04 Simon Riggs <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 16+ messages in thread From: Simon Riggs @ 2022-03-24 16:04 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Peter Eisentraut <[email protected]>; pgsql-hackers On Thu, 24 Mar 2022 at 15:48, Tom Lane <[email protected]> wrote: > > Simon Riggs <[email protected]> writes: > > On Wed, 23 Mar 2022 at 20:25, Tom Lane <[email protected]> wrote: > >> Do you have any objection if I rename the GUC to > >> recursive_worktable_factor? That seems a bit clearer as to what > >> it does, and it leaves more room for other knobs in the same area > >> if we decide we need any. > > > None, I think your proposal is a better name. > > Pushed that way. Ok, thanks. -- Simon Riggs http://www.EnterpriseDB.com/ ^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2022-03-24 16:04 UTC | newest] Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2019-05-28 18:15 [PATCH v27 1/5] Remove pg_collation.collversion. Thomas Munro <[email protected]> 2021-10-27 14:58 Parameter for planner estimate of recursive queries Simon Riggs <[email protected]> 2021-12-31 14:10 ` Re: Parameter for planner estimate of recursive queries Simon Riggs <[email protected]> 2022-01-25 09:44 ` Re: Parameter for planner estimate of recursive queries Peter Eisentraut <[email protected]> 2022-01-28 13:40 ` Re: Parameter for planner estimate of recursive queries Hamid Akhtar <[email protected]> 2022-01-28 14:07 ` Re: Parameter for planner estimate of recursive queries Robert Haas <[email protected]> 2022-03-10 17:42 ` Re: Parameter for planner estimate of recursive queries Simon Riggs <[email protected]> 2022-03-22 00:04 ` Re: Parameter for planner estimate of recursive queries Andres Freund <[email protected]> 2022-03-23 14:53 ` Re: Parameter for planner estimate of recursive queries Simon Riggs <[email protected]> 2022-03-23 17:36 ` Re: Parameter for planner estimate of recursive queries Tom Lane <[email protected]> 2022-03-23 18:20 ` Re: Parameter for planner estimate of recursive queries Simon Riggs <[email protected]> 2022-03-23 19:37 ` Re: Parameter for planner estimate of recursive queries Simon Riggs <[email protected]> 2022-03-23 20:25 ` Re: Parameter for planner estimate of recursive queries Tom Lane <[email protected]> 2022-03-24 10:32 ` Re: Parameter for planner estimate of recursive queries Simon Riggs <[email protected]> 2022-03-24 15:48 ` Re: Parameter for planner estimate of recursive queries Tom Lane <[email protected]> 2022-03-24 16:04 ` Re: Parameter for planner estimate of recursive queries Simon Riggs <[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