public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v27 1/5] Remove pg_collation.collversion.
14+ 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; 14+ 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] 14+ messages in thread
* Re: Parameter for planner estimate of recursive queries
@ 2022-01-25 09:44 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]>
0 siblings, 2 replies; 14+ 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] 14+ messages in thread
* Re: Parameter for planner estimate of recursive queries
2022-01-25 09:44 Re: Parameter for planner estimate of recursive queries Peter Eisentraut <[email protected]>
@ 2022-01-28 13:40 ` Hamid Akhtar <[email protected]>
1 sibling, 0 replies; 14+ 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] 14+ messages in thread
* Re: Parameter for planner estimate of recursive queries
2022-01-25 09:44 Re: Parameter for planner estimate of recursive queries Peter Eisentraut <[email protected]>
@ 2022-01-28 14:07 ` Robert Haas <[email protected]>
2022-03-10 17:42 ` 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]>
1 sibling, 2 replies; 14+ 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] 14+ messages in thread
* Re: Parameter for planner estimate of recursive queries
2022-01-25 09:44 Re: Parameter for planner estimate of recursive queries Peter Eisentraut <[email protected]>
2022-01-28 14:07 ` Re: Parameter for planner estimate of recursive queries Robert Haas <[email protected]>
@ 2022-03-10 17:42 ` Simon Riggs <[email protected]>
2022-03-22 00:04 ` Re: Parameter for planner estimate of recursive queries Andres Freund <[email protected]>
1 sibling, 1 reply; 14+ 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] 14+ messages in thread
* Re: Parameter for planner estimate of recursive queries
2022-01-25 09:44 Re: Parameter for planner estimate of recursive queries Peter Eisentraut <[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 ` Andres Freund <[email protected]>
2022-03-23 14:53 ` Re: Parameter for planner estimate of recursive queries Simon Riggs <[email protected]>
0 siblings, 1 reply; 14+ 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] 14+ messages in thread
* Re: Parameter for planner estimate of recursive queries
2022-01-25 09:44 Re: Parameter for planner estimate of recursive queries Peter Eisentraut <[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 ` Simon Riggs <[email protected]>
0 siblings, 0 replies; 14+ 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] 14+ messages in thread
* Re: Parameter for planner estimate of recursive queries
2022-01-25 09:44 Re: Parameter for planner estimate of recursive queries Peter Eisentraut <[email protected]>
2022-01-28 14:07 ` Re: Parameter for planner estimate of recursive queries Robert Haas <[email protected]>
@ 2022-03-23 17:36 ` Tom Lane <[email protected]>
2022-03-23 18:20 ` Re: Parameter for planner estimate of recursive queries Simon Riggs <[email protected]>
1 sibling, 1 reply; 14+ 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] 14+ messages in thread
* Re: Parameter for planner estimate of recursive queries
2022-01-25 09:44 Re: Parameter for planner estimate of recursive queries Peter Eisentraut <[email protected]>
2022-01-28 14:07 ` Re: Parameter for planner estimate of recursive queries Robert Haas <[email protected]>
2022-03-23 17:36 ` Re: Parameter for planner estimate of recursive queries Tom Lane <[email protected]>
@ 2022-03-23 18:20 ` Simon Riggs <[email protected]>
2022-03-23 19:37 ` Re: Parameter for planner estimate of recursive queries Simon Riggs <[email protected]>
0 siblings, 1 reply; 14+ 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] 14+ messages in thread
* Re: Parameter for planner estimate of recursive queries
2022-01-25 09:44 Re: Parameter for planner estimate of recursive queries Peter Eisentraut <[email protected]>
2022-01-28 14:07 ` Re: Parameter for planner estimate of recursive queries Robert Haas <[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 ` Simon Riggs <[email protected]>
2022-03-23 20:25 ` Re: Parameter for planner estimate of recursive queries Tom Lane <[email protected]>
0 siblings, 1 reply; 14+ 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] 14+ messages in thread
* Re: Parameter for planner estimate of recursive queries
2022-01-25 09:44 Re: Parameter for planner estimate of recursive queries Peter Eisentraut <[email protected]>
2022-01-28 14:07 ` Re: Parameter for planner estimate of recursive queries Robert Haas <[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 ` Tom Lane <[email protected]>
2022-03-24 10:32 ` Re: Parameter for planner estimate of recursive queries Simon Riggs <[email protected]>
0 siblings, 1 reply; 14+ 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] 14+ messages in thread
* Re: Parameter for planner estimate of recursive queries
2022-01-25 09:44 Re: Parameter for planner estimate of recursive queries Peter Eisentraut <[email protected]>
2022-01-28 14:07 ` Re: Parameter for planner estimate of recursive queries Robert Haas <[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 ` Simon Riggs <[email protected]>
2022-03-24 15:48 ` Re: Parameter for planner estimate of recursive queries Tom Lane <[email protected]>
0 siblings, 1 reply; 14+ 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] 14+ messages in thread
* Re: Parameter for planner estimate of recursive queries
2022-01-25 09:44 Re: Parameter for planner estimate of recursive queries Peter Eisentraut <[email protected]>
2022-01-28 14:07 ` Re: Parameter for planner estimate of recursive queries Robert Haas <[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 ` Tom Lane <[email protected]>
2022-03-24 16:04 ` Re: Parameter for planner estimate of recursive queries Simon Riggs <[email protected]>
0 siblings, 1 reply; 14+ 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] 14+ messages in thread
* Re: Parameter for planner estimate of recursive queries
2022-01-25 09:44 Re: Parameter for planner estimate of recursive queries Peter Eisentraut <[email protected]>
2022-01-28 14:07 ` Re: Parameter for planner estimate of recursive queries Robert Haas <[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 ` Simon Riggs <[email protected]>
0 siblings, 0 replies; 14+ 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] 14+ messages in thread
end of thread, other threads:[~2022-03-24 16:04 UTC | newest]
Thread overview: 14+ 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]>
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