public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v24 1/5] Remove pg_collation.collversion.
6+ messages / 4 participants
[nested] [flat]
* [PATCH v24 1/5] Remove pg_collation.collversion.
@ 2019-05-28 18:15 Thomas Munro <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Munro @ 2019-05-28 18:15 UTC (permalink / raw)
While this allowed the system to detect changes in collation versions,
it didn't help us track what had to be done about it. A later patch
will add version tracking for individual database objects that depend
on collations.
Author: Thomas Munro
Reviewed-by: Julien Rouhaud, Peter Eisentraut and Michael Paquier
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 | 85 -------------------
src/backend/nodes/copyfuncs.c | 13 ---
src/backend/nodes/equalfuncs.c | 11 ---
src/backend/parser/gram.y | 18 +---
src/backend/tcop/utility.c | 12 ---
src/backend/utils/adt/pg_locale.c | 37 --------
src/bin/pg_dump/pg_dump.c | 8 +-
src/include/catalog/pg_collation.dat | 7 +-
src/include/catalog/pg_collation.h | 5 --
src/include/catalog/toasting.h | 1 -
src/include/commands/collationcmds.h | 1 -
src/include/nodes/parsenodes.h | 11 ---
.../regress/expected/collate.icu.utf8.out | 3 -
.../regress/expected/collate.linux.utf8.out | 3 -
src/test/regress/sql/collate.icu.utf8.sql | 5 --
src/test/regress/sql/collate.linux.utf8.sql | 5 --
21 files changed, 11 insertions(+), 320 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 49a881b262..57f3dece43 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -2352,17 +2352,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 f065856535..69fe242761 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -25419,11 +25419,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..78eceda848 100644
--- a/src/backend/commands/collationcmds.c
+++ b/src/backend/commands/collationcmds.c
@@ -68,7 +68,6 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e
bool collisdeterministic = true;
int collencoding = 0;
char collprovider = 0;
- char *collversion = NULL;
Oid newoid;
ObjectAddress address;
@@ -166,9 +165,6 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e
if (deterministicEl)
collisdeterministic = defGetBoolean(deterministicEl);
- if (versionEl)
- collversion = defGetString(versionEl);
-
if (collproviderstr)
{
if (pg_strcasecmp(collproviderstr, "icu") == 0)
@@ -215,9 +211,6 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e
}
}
- if (!collversion)
- collversion = get_collation_actual_version(collprovider, collcollate);
-
newoid = CollationCreate(collName,
collNamespace,
GetUserId(),
@@ -226,7 +219,6 @@ DefineCollation(ParseState *pstate, List *names, List *parameters, bool if_not_e
collencoding,
collcollate,
collctype,
- collversion,
if_not_exists,
false); /* not quiet */
@@ -277,80 +269,6 @@ IsThereCollationInNamespace(const char *collname, Oid nspOid)
collname, get_namespace_name(nspOid))));
}
-/*
- * ALTER COLLATION
- */
-ObjectAddress
-AlterCollation(AlterCollationStmt *stmt)
-{
- Relation rel;
- Oid collOid;
- HeapTuple tup;
- Form_pg_collation collForm;
- Datum collversion;
- bool isnull;
- char *oldversion;
- char *newversion;
- ObjectAddress address;
-
- rel = table_open(CollationRelationId, RowExclusiveLock);
- collOid = get_collation_oid(stmt->collname, false);
-
- if (!pg_collation_ownercheck(collOid, GetUserId()))
- aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_COLLATION,
- NameListToString(stmt->collname));
-
- tup = SearchSysCacheCopy1(COLLOID, ObjectIdGetDatum(collOid));
- if (!HeapTupleIsValid(tup))
- elog(ERROR, "cache lookup failed for collation %u", collOid);
-
- collForm = (Form_pg_collation) GETSTRUCT(tup);
- collversion = SysCacheGetAttr(COLLOID, tup, Anum_pg_collation_collversion,
- &isnull);
- oldversion = isnull ? NULL : TextDatumGetCString(collversion);
-
- newversion = get_collation_actual_version(collForm->collprovider, NameStr(collForm->collcollate));
-
- /* cannot change from NULL to non-NULL or vice versa */
- if ((!oldversion && newversion) || (oldversion && !newversion))
- elog(ERROR, "invalid collation version change");
- else if (oldversion && newversion && strcmp(newversion, oldversion) != 0)
- {
- bool nulls[Natts_pg_collation];
- bool replaces[Natts_pg_collation];
- Datum values[Natts_pg_collation];
-
- ereport(NOTICE,
- (errmsg("changing version from %s to %s",
- oldversion, newversion)));
-
- memset(values, 0, sizeof(values));
- memset(nulls, false, sizeof(nulls));
- memset(replaces, false, sizeof(replaces));
-
- values[Anum_pg_collation_collversion - 1] = CStringGetTextDatum(newversion);
- replaces[Anum_pg_collation_collversion - 1] = true;
-
- tup = heap_modify_tuple(tup, RelationGetDescr(rel),
- values, nulls, replaces);
- }
- else
- ereport(NOTICE,
- (errmsg("version has not changed")));
-
- CatalogTupleUpdate(rel, &tup->t_self, tup);
-
- InvokeObjectPostAlterHook(CollationRelationId, collOid, 0);
-
- ObjectAddressSet(address, CollationRelationId, collOid);
-
- heap_freetuple(tup);
- table_close(rel, NoLock);
-
- return address;
-}
-
-
Datum
pg_collation_actual_version(PG_FUNCTION_ARGS)
{
@@ -608,7 +526,6 @@ pg_import_system_collations(PG_FUNCTION_ARGS)
collid = CollationCreate(localebuf, nspid, GetUserId(),
COLLPROVIDER_LIBC, true, enc,
localebuf, localebuf,
- get_collation_actual_version(COLLPROVIDER_LIBC, localebuf),
true, true);
if (OidIsValid(collid))
{
@@ -669,7 +586,6 @@ pg_import_system_collations(PG_FUNCTION_ARGS)
collid = CollationCreate(alias, nspid, GetUserId(),
COLLPROVIDER_LIBC, true, enc,
locale, locale,
- get_collation_actual_version(COLLPROVIDER_LIBC, locale),
true, true);
if (OidIsValid(collid))
{
@@ -731,7 +647,6 @@ pg_import_system_collations(PG_FUNCTION_ARGS)
nspid, GetUserId(),
COLLPROVIDER_ICU, true, -1,
collcollate, collcollate,
- get_collation_actual_version(COLLPROVIDER_ICU, collcollate),
true, true);
if (OidIsValid(collid))
{
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index d8cf87e6d0..5cbbe3ba2e 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 627b026b19..73a9507b80 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 4ff35095b8..c43b16037e 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 97cbaa3072..7453802fce 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 11d05c73ac..2c5f5bfa37 100644
--- a/src/backend/utils/adt/pg_locale.c
+++ b/src/backend/utils/adt/pg_locale.c
@@ -1522,8 +1522,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))
@@ -1625,41 +1623,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 a41a3db876..2f6bfede8e 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -13672,7 +13672,11 @@ dumpCollation(Archive *fout, CollInfo *collinfo)
/* Get collation-specific details */
appendPQExpBufferStr(query, "SELECT ");
- if (fout->remoteVersion >= 100000)
+ if (fout->remoteVersion >= 130000)
+ appendPQExpBufferStr(query,
+ "collprovider, "
+ "NULL AS collversion, ");
+ else if (fout->remoteVersion >= 100000)
appendPQExpBufferStr(query,
"collprovider, "
"collversion, ");
@@ -13744,7 +13748,7 @@ dumpCollation(Archive *fout, CollInfo *collinfo)
* For binary upgrade, carry over the collation version. For normal
* dump/restore, omit the version, so that it is computed upon restore.
*/
- if (dopt->binary_upgrade)
+ if (dopt->binary_upgrade && fout->remoteVersion < 130000)
{
int i_collversion;
diff --git a/src/include/catalog/pg_collation.dat b/src/include/catalog/pg_collation.dat
index ba1b3e201b..45301ccdd7 100644
--- a/src/include/catalog/pg_collation.dat
+++ b/src/include/catalog/pg_collation.dat
@@ -15,17 +15,16 @@
{ oid => '100', oid_symbol => 'DEFAULT_COLLATION_OID',
descr => 'database\'s default collation',
collname => 'default', collnamespace => 'PGNSP', collowner => 'PGUID',
- collprovider => 'd', collencoding => '-1', collcollate => '', collctype => '',
- collversion => '_null_' },
+ collprovider => 'd', collencoding => '-1', collcollate => '', collctype => '' },
{ oid => '950', oid_symbol => 'C_COLLATION_OID',
descr => 'standard C collation',
collname => 'C', collnamespace => 'PGNSP', collowner => 'PGUID',
collprovider => 'c', collencoding => '-1', collcollate => 'C',
- collctype => 'C', collversion => '_null_' },
+ collctype => 'C' },
{ oid => '951', oid_symbol => 'POSIX_COLLATION_OID',
descr => 'standard POSIX collation',
collname => 'POSIX', collnamespace => 'PGNSP', collowner => 'PGUID',
collprovider => 'c', collencoding => '-1', collcollate => 'POSIX',
- collctype => 'POSIX', collversion => '_null_' },
+ collctype => 'POSIX' },
]
diff --git a/src/include/catalog/pg_collation.h b/src/include/catalog/pg_collation.h
index 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 5e1ffafb91..7a23fb7529 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
--wac7ysb48OaltWcw
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
filename="v24-0002-Add-pg_depend.refobjversion.patch"
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [RFC] Lock-free XLog Reservation from WAL
@ 2025-01-03 13:01 wenhui qiu <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: wenhui qiu @ 2025-01-03 13:01 UTC (permalink / raw)
To: Yura Sokolov <[email protected]>; +Cc: Zhou, Zhiguo <[email protected]>; pgsql-hackers
Hi
Thank you for your path，NUM_XLOGINSERT_LOCKS increase to 128，I think it
will be challenged，do we make it guc ？
On Fri, 3 Jan 2025 at 20:36, Yura Sokolov <[email protected]> wrote:
> 02.01.2025 10:05, Zhou, Zhiguo wrote:
> > Hi all,
> >
> > I am reaching out to solicit your insights and comments on a recent
> proposal regarding the "Lock-free XLog Reservation from WAL." We have
> identified some challenges with the current WAL insertions, which require
> space reservations in the WAL buffer which involve updating two
> shared-memory statuses in XLogCtlInsert: CurrBytePos (the start position of
> the current XLog) and PrevBytePos (the prev-link to the previous XLog).
> Currently, the use of XLogCtlInsert.insertpos_lck ensures consistency but
> introduces lock contention, hindering the parallelism of XLog insertions.
> >
> > To address this issue, we propose the following changes:
> >
> > 1. Removal of PrevBytePos: This will allow increments of the CurrBytePos
> (a single uint64 field) to be implemented with an atomic operation
> (fetch_add).
> > 2. Updating Prev-Link of next XLog: Based on the fact that the prev-link
> of the next XLog always points to the head of the current Xlog,we will
> slightly exceed the reserved memory range of the current XLog to update the
> prev-link of the next XLog, regardless of which backend acquires the next
> memory space. The next XLog inserter will wait until its prev-link is
> updated for CRC calculation before starting its own XLog copy into the WAL.
> > 3. Breaking Sequential Write Convention: Each backend will update the
> prev-link of its next XLog first, then return to the header position for
> the current log insertion. This change will reduce the dependency of XLog
> writes on previous ones (compared with the sequential writes).
> > 4. Revised GetXLogBuffer: To support #3, we need update this function to
> separate the LSN it intends to access from the LSN it expects to update in
> the insertingAt field.
> > 5. Increase NUM_XLOGINSERT_LOCKS: With the above changes, increasing
> NUM_XLOGINSERT_LOCKS, for example to 128, could effectively enhance the
> parallelism.
> >
> > The attached patch could pass the regression tests (make check, make
> check-world), and in the performance test of this POC on SPR (480 vCPU)
> shows that this optimization could help the TPCC benchmark better scale
> with the core count and as a result the performance with full cores enabled
> could be improved by 2.04x.
> >
> > Before we proceed with further patch validation and refinement work, we
> are eager to hear the community's thoughts and comments on this
> optimization so that we can confirm our current work aligns with
> expectations.
>
> Good day, Zhiguo.
>
> Idea looks great.
>
> Minor issue:
> - you didn't remove use of `insertpos_lck` from `ReserveXLogSwitch`.
>
> I initially thought it became un-synchronized against
> `ReserveXLogInsertLocation`, but looking closer I found it is
> synchronized with `WALInsertLockAcquireExclusive`.
> Since there are no other `insertpos_lck` usages after your patch, I
> don't see why it should exists and be used in `ReserveXLogSwitch`.
>
> Still I'd prefer to see CAS loop in this place to be consistent with
> other non-locking access. And it will allow to get rid of
> `WALInsertLockAcquireExclusive`, (though probably it is not a big issue).
>
> Major issue:
> - `SetPrevRecPtr` and `GetPrevRecPtr` do non-atomic write/read with on
> platforms where MAXALIGN != 8 or without native 64 load/store. Branch
> with 'memcpy` is rather obvious, but even pointer de-referencing on
> "lucky case" is not safe either.
>
> I have no idea how to fix it at the moment.
>
> Readability issue:
> - It would be good to add `Assert(ptr >= upto)` into `GetXLogBuffer`.
> I had hard time to recognize `upto` is strictly not in the future.
> - Certainly, final version have to have fixed and improved comments.
> Many patch's ideas are strictly non-obvious. I had hard time to
> recognize patch is not a piece of ... (excuse me for the swear sentence).
>
> Indeed, patch is much better than it looks on first sight.
> I came with alternative idea yesterday, but looking closer to your patch
> today I see it is superior to mine (if atomic access will be fixed).
>
> ----
>
> regards,
> Yura Sokolov aka funny-falcon
>
>
>
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [RFC] Lock-free XLog Reservation from WAL
@ 2025-01-06 06:46 Zhou, Zhiguo <[email protected]>
parent: wenhui qiu <[email protected]>
0 siblings, 2 replies; 6+ messages in thread
From: Zhou, Zhiguo @ 2025-01-06 06:46 UTC (permalink / raw)
To: wenhui qiu <[email protected]>; Yura Sokolov <[email protected]>; +Cc: pgsql-hackers
Hi Yura and Wenhui,
Thanks for kindly reviewing this work!
On 1/3/2025 9:01 PM, wenhui qiu wrote:
> Hi
> Thank you for your path，NUM_XLOGINSERT_LOCKS increase to 128，I
> think it will be challenged，do we make it guc ？
>
I noticed there have been some discussions (for example, [1] and its
responses) about making NUM_XLOGINSERT_LOCKS a GUC, which seems to be a
controversial proposal. Given that, we may first focus on the lock-free
XLog reservation implementation, and leave the increase of
NUM_XLOGINSERT_LOCKS for a future patch, where we would provide more
quantitative evidence for the various implementations. WDYT?
> On Fri, 3 Jan 2025 at 20:36, Yura Sokolov <[email protected]
> <mailto:[email protected]>> wrote:
>
> Good day, Zhiguo.
>
> Idea looks great.
>
> Minor issue:
> - you didn't remove use of `insertpos_lck` from `ReserveXLogSwitch`.
>
> I initially thought it became un-synchronized against
> `ReserveXLogInsertLocation`, but looking closer I found it is
> synchronized with `WALInsertLockAcquireExclusive`.
> Since there are no other `insertpos_lck` usages after your patch, I
> don't see why it should exists and be used in `ReserveXLogSwitch`.
>
> Still I'd prefer to see CAS loop in this place to be consistent with
> other non-locking access. And it will allow to get rid of
> `WALInsertLockAcquireExclusive`, (though probably it is not a big
> issue).
>
Exactly, it should be safe to remove `insertpos_lck`. And I agree with
you on getting rid of `WALInsertLockAcquireExclusive` with CAS loop
which should significantly reduce the synchronization cost here
especially when we intend to increase NUM_XLOGINSERT_LOCKS. I will try
it in the next version of patch.
> Major issue:
> - `SetPrevRecPtr` and `GetPrevRecPtr` do non-atomic write/read with on
> platforms where MAXALIGN != 8 or without native 64 load/store. Branch
> with 'memcpy` is rather obvious, but even pointer de-referencing on
> "lucky case" is not safe either.
>
> I have no idea how to fix it at the moment.
>
Indeed, non-atomic write/read operations can lead to safety issues in
some situations. My initial thought is to define a bit near the
prev-link to flag the completion of the update. In this way, we could
allow non-atomic or even discontinuous write/read operations on the
prev-link, while simultaneously guaranteeing its atomicity through
atomic operations (as well as memory barriers) on the flag bit. What do
you think of this as a viable solution?
> Readability issue:
> - It would be good to add `Assert(ptr >= upto)` into `GetXLogBuffer`.
> I had hard time to recognize `upto` is strictly not in the future.
> - Certainly, final version have to have fixed and improved comments.
> Many patch's ideas are strictly non-obvious. I had hard time to
> recognize patch is not a piece of ... (excuse me for the swear
> sentence).
Thanks for the suggestion and patience. It's really more readable after
inserting the assertion, I will fix it and improve other comments in the
following patches.
> Indeed, patch is much better than it looks on first sight.
> I came with alternative idea yesterday, but looking closer to your
> patch
> today I see it is superior to mine (if atomic access will be fixed).
>
> ----
>
> regards,
> Yura Sokolov aka funny-falcon
>
>
Regards,
Zhiguo
[1] https://www.postgresql.org/message-id/2266698.1704854297%40sss.pgh.pa.us
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [RFC] Lock-free XLog Reservation from WAL
@ 2025-01-06 08:35 wenhui qiu <[email protected]>
parent: Zhou, Zhiguo <[email protected]>
1 sibling, 0 replies; 6+ messages in thread
From: wenhui qiu @ 2025-01-06 08:35 UTC (permalink / raw)
To: Zhou, Zhiguo <[email protected]>; +Cc: Yura Sokolov <[email protected]>; pgsql-hackers
HI Zhiguo
Thank you for your reply ,Then you'll have to prove that 128 is the
optimal value, otherwise they'll have a hard time agreeing with you on this
patch.
Thanks
On Mon, Jan 6, 2025 at 2:46 PM Zhou, Zhiguo <[email protected]> wrote:
> Hi Yura and Wenhui,
>
> Thanks for kindly reviewing this work!
>
> On 1/3/2025 9:01 PM, wenhui qiu wrote:
> > Hi
> > Thank you for your path，NUM_XLOGINSERT_LOCKS increase to 128，I
> > think it will be challenged，do we make it guc ？
> >
>
> I noticed there have been some discussions (for example, [1] and its
> responses) about making NUM_XLOGINSERT_LOCKS a GUC, which seems to be a
> controversial proposal. Given that, we may first focus on the lock-free
> XLog reservation implementation, and leave the increase of
> NUM_XLOGINSERT_LOCKS for a future patch, where we would provide more
> quantitative evidence for the various implementations. WDYT?
>
>
> > On Fri, 3 Jan 2025 at 20:36, Yura Sokolov <[email protected]
> > <mailto:[email protected]>> wrote:
> >
> > Good day, Zhiguo.
> >
> > Idea looks great.
> >
> > Minor issue:
> > - you didn't remove use of `insertpos_lck` from `ReserveXLogSwitch`.
> >
> > I initially thought it became un-synchronized against
> > `ReserveXLogInsertLocation`, but looking closer I found it is
> > synchronized with `WALInsertLockAcquireExclusive`.
> > Since there are no other `insertpos_lck` usages after your patch, I
> > don't see why it should exists and be used in `ReserveXLogSwitch`.
> >
> > Still I'd prefer to see CAS loop in this place to be consistent with
> > other non-locking access. And it will allow to get rid of
> > `WALInsertLockAcquireExclusive`, (though probably it is not a big
> > issue).
> >
>
> Exactly, it should be safe to remove `insertpos_lck`. And I agree with
> you on getting rid of `WALInsertLockAcquireExclusive` with CAS loop
> which should significantly reduce the synchronization cost here
> especially when we intend to increase NUM_XLOGINSERT_LOCKS. I will try
> it in the next version of patch.
>
>
> > Major issue:
> > - `SetPrevRecPtr` and `GetPrevRecPtr` do non-atomic write/read with
> on
> > platforms where MAXALIGN != 8 or without native 64 load/store. Branch
> > with 'memcpy` is rather obvious, but even pointer de-referencing on
> > "lucky case" is not safe either.
> >
> > I have no idea how to fix it at the moment.
> >
>
> Indeed, non-atomic write/read operations can lead to safety issues in
> some situations. My initial thought is to define a bit near the
> prev-link to flag the completion of the update. In this way, we could
> allow non-atomic or even discontinuous write/read operations on the
> prev-link, while simultaneously guaranteeing its atomicity through
> atomic operations (as well as memory barriers) on the flag bit. What do
> you think of this as a viable solution?
>
>
> > Readability issue:
> > - It would be good to add `Assert(ptr >= upto)` into `GetXLogBuffer`.
> > I had hard time to recognize `upto` is strictly not in the future.
> > - Certainly, final version have to have fixed and improved comments.
> > Many patch's ideas are strictly non-obvious. I had hard time to
> > recognize patch is not a piece of ... (excuse me for the swear
> > sentence).
>
> Thanks for the suggestion and patience. It's really more readable after
> inserting the assertion, I will fix it and improve other comments in the
> following patches.
>
>
> > Indeed, patch is much better than it looks on first sight.
> > I came with alternative idea yesterday, but looking closer to your
> > patch
> > today I see it is superior to mine (if atomic access will be fixed).
> >
> > ----
> >
> > regards,
> > Yura Sokolov aka funny-falcon
> >
> >
>
> Regards,
> Zhiguo
>
>
> [1]
> https://www.postgresql.org/message-id/2266698.1704854297%40sss.pgh.pa.us
>
>
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [RFC] Lock-free XLog Reservation from WAL
@ 2025-01-07 02:49 Юрий Соколов <[email protected]>
parent: Zhou, Zhiguo <[email protected]>
1 sibling, 1 reply; 6+ messages in thread
From: Юрий Соколов @ 2025-01-07 02:49 UTC (permalink / raw)
To: Zhou, Zhiguo <[email protected]>; +Cc: wenhui qiu <[email protected]>; pgsql-hackers
> On 6 Jan 2025, at 09:46, Zhou, Zhiguo <[email protected]> wrote:
>
> Hi Yura and Wenhui,
>
> Thanks for kindly reviewing this work!
>
> On 1/3/2025 9:01 PM, wenhui qiu wrote:
>> Hi
>> Thank you for your path，NUM_XLOGINSERT_LOCKS increase to 128，I think it will be challenged，do we make it guc ？
>
> I noticed there have been some discussions (for example, [1] and its responses) about making NUM_XLOGINSERT_LOCKS a GUC, which seems to be a controversial proposal. Given that, we may first focus on the lock-free XLog reservation implementation, and leave the increase of NUM_XLOGINSERT_LOCKS for a future patch, where we would provide more quantitative evidence for the various implementations. WDYT?
>
>
>> On Fri, 3 Jan 2025 at 20:36, Yura Sokolov <[email protected] <mailto:[email protected]><mailto:[email protected]>> wrote:
>> Good day, Zhiguo.
>> Idea looks great.
>> Minor issue:
>> - you didn't remove use of `insertpos_lck` from `ReserveXLogSwitch`.
>> I initially thought it became un-synchronized against
>> `ReserveXLogInsertLocation`, but looking closer I found it is
>> synchronized with `WALInsertLockAcquireExclusive`.
>> Since there are no other `insertpos_lck` usages after your patch, I
>> don't see why it should exists and be used in `ReserveXLogSwitch`.
>> Still I'd prefer to see CAS loop in this place to be consistent with
>> other non-locking access. And it will allow to get rid of
>> `WALInsertLockAcquireExclusive`, (though probably it is not a big
>> issue).
>
> Exactly, it should be safe to remove `insertpos_lck`. And I agree with you on getting rid of `WALInsertLockAcquireExclusive` with CAS loop which should significantly reduce the synchronization cost here especially when we intend to increase NUM_XLOGINSERT_LOCKS. I will try it in the next version of patch.
>
>
>> Major issue:
>> - `SetPrevRecPtr` and `GetPrevRecPtr` do non-atomic write/read with on
>> platforms where MAXALIGN != 8 or without native 64 load/store. Branch
>> with 'memcpy` is rather obvious, but even pointer de-referencing on
>> "lucky case" is not safe either.
>> I have no idea how to fix it at the moment.
>
> Indeed, non-atomic write/read operations can lead to safety issues in some situations. My initial thought is to define a bit near the prev-link to flag the completion of the update. In this way, we could allow non-atomic or even discontinuous write/read operations on the prev-link, while simultaneously guaranteeing its atomicity through atomic operations (as well as memory barriers) on the flag bit. What do you think of this as a viable solution?
>
>
>> Readability issue:
>> - It would be good to add `Assert(ptr >= upto)` into `GetXLogBuffer`.
>> I had hard time to recognize `upto` is strictly not in the future.
>> - Certainly, final version have to have fixed and improved comments.
>> Many patch's ideas are strictly non-obvious. I had hard time to
>> recognize patch is not a piece of ... (excuse me for the swear
>> sentence).
>
> Thanks for the suggestion and patience. It's really more readable after inserting the assertion, I will fix it and improve other comments in the following patches.
>
>
>> Indeed, patch is much better than it looks on first sight.
>> I came with alternative idea yesterday, but looking closer to your
>> patch
>> today I see it is superior to mine (if atomic access will be fixed).
>
> [1] https://www.postgresql.org/message-id/2266698.1704854297%40sss.pgh.pa.us
Good day, Zhiguo.
Here’s my attempt to organise link to previous record without messing with xlog buffers:
- link is stored in lock-free hash table instead.
I don’t claim it is any better than using xlog buffers.
It is just alternative vision.
Some tricks in implementation:
- Relying on byte-position nature, it could be converted to 32 bit unique
value with `(uint32)(pos ^ (pos>>32))`. Certainly it is not totally unique,
but it is certainly unique among 32GB consecutive log.
- PrevBytePos could be calculated as a difference between positions, and
this difference is certainly less than 4GB, so it also could be stored as 32
bit value (PrevSize).
- Since xlog records are aligned we could use lowest bit of PrevSize as a lock.
- While Cuckoo Hashing could suffer from un-solvable cycle conflicts, this implementation relies on concurrent deleters which will eventually break such cycles if any.
I have a version without 32bit conversion trick, and it is a bit lighter on atomic instructions count, but it performs badly in absence of native 64bit atomics.
——
regards
Yura Sokolov aka funny-falcon

Attachments:
[application/octet-stream] v1-0001-Lock-free-XLog-Reservation-using-lock-free-hash-t.patch (16.3K, ../../[email protected]/3-v1-0001-Lock-free-XLog-Reservation-using-lock-free-hash-t.patch)
download | inline diff:
From c41f7b72d57b4fa4211079f7637f0a8470ec9348 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=D0=AE=D1=80=D0=B8=D0=B9=20=D0=A1=D0=BE=D0=BA=D0=BE=D0=BB?=
=?UTF-8?q?=D0=BE=D0=B2?= <[email protected]>
Date: Mon, 6 Jan 2025 21:54:06 +0300
Subject: [PATCH v1] Lock-free XLog Reservation using lock-free hash-table
Removed PrevBytePos to eliminate lock contention, allowing atomic updates
to CurrBytePos. Use lock-free hash-table based on 4-way Cuckoo Hashing
to store link to PrevBytePos.
---
src/backend/access/transam/xlog.c | 346 ++++++++++++++++++++++++------
1 file changed, 285 insertions(+), 61 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index b9ea92a542..2f35e1645a 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -68,6 +68,8 @@
#include "catalog/pg_database.h"
#include "common/controldata_utils.h"
#include "common/file_utils.h"
+#include "common/hashfn.h"
+#include "common/pg_prng.h"
#include "executor/instrument.h"
#include "miscadmin.h"
#include "pg_trace.h"
@@ -147,7 +149,7 @@ int wal_segment_size = DEFAULT_XLOG_SEG_SIZE;
* to happen concurrently, but adds some CPU overhead to flushing the WAL,
* which needs to iterate all the locks.
*/
-#define NUM_XLOGINSERT_LOCKS 8
+#define NUM_XLOGINSERT_LOCKS 128
/*
* Max distance from last checkpoint, before triggering a new xlog-based
@@ -384,6 +386,36 @@ typedef union WALInsertLockPadded
char pad[PG_CACHE_LINE_SIZE];
} WALInsertLockPadded;
+/*
+ * It links current position with previous one.
+ * - CurrPosId is (CurrBytePos ^ (CurrBytePos>>32))
+ * Since CurrBytePos grows monotonically and it is aligned to MAXALIGN,
+ * CurrPosId correctly identifies CurrBytePos for at least 4*2^32 = 32GB of
+ * WAL logs.
+ * - PrevSize is difference between CurrBytePos and PrevBytePos
+ */
+typedef struct
+{
+ uint32 CurrPosId;
+ uint32 PrevSize;
+} WALPrevPosLinkVal;
+
+/*
+ * This is an element of lock-free hash-table.
+ * PrevSize's lowest bit is used as a lock, relying on fact it is MAXALIGN-ed.
+ */
+typedef struct
+{
+ pg_atomic_uint32 CurrPosId;
+ pg_atomic_uint32 PrevSize;
+} WALPrevPosLink;
+
+#define PREV_LINKS_HASH_CAPA 256
+StaticAssertDecl(!(PREV_LINKS_HASH_CAPA & (PREV_LINKS_HASH_CAPA - 1)),
+ "PREV_LINKS_HASH_CAPA should be power of two");
+
+#define SWAP_ONCE_IN 128
+
/*
* Session status of running backup, used for sanity checks in SQL-callable
* functions to start and stop backups.
@@ -395,26 +427,31 @@ static SessionBackupState sessionBackupState = SESSION_BACKUP_NONE;
*/
typedef struct XLogCtlInsert
{
- slock_t insertpos_lck; /* protects CurrBytePos and PrevBytePos */
-
/*
* CurrBytePos is the end of reserved WAL. The next record will be
- * inserted at that position. PrevBytePos is the start position of the
- * previously inserted (or rather, reserved) record - it is copied to the
- * prev-link of the next record. These are stored as "usable byte
- * positions" rather than XLogRecPtrs (see XLogBytePosToRecPtr()).
+ * inserted at that position.
+ *
+ * The start position of the previously inserted (or rather, reserved)
+ * record (it is copied to the prev-link of the next record) will be
+ * stored in PrevLinksHash.
+ *
+ * These are stored as "usable byte positions" rather than XLogRecPtrs
+ * (see XLogBytePosToRecPtr()).
*/
- uint64 CurrBytePos;
- uint64 PrevBytePos;
+ pg_atomic_uint64 CurrBytePos pg_attribute_aligned(PG_CACHE_LINE_SIZE);
/*
- * Make sure the above heavily-contended spinlock and byte positions are
- * on their own cache line. In particular, the RedoRecPtr and full page
- * write variables below should be on a different cache line. They are
- * read on every WAL insertion, but updated rarely, and we don't want
- * those reads to steal the cache line containing Curr/PrevBytePos.
+ * PrevLinksHash is a lock-free hash table based on Cuckoo algorith. It is
+ * mostly 4 way: for every element computed two positions h1, h2, and
+ * neighbour h1+1 and h2+2 are used as well. This way even on collision we
+ * have 3 distinct position, which provide us ~75% fill rate without
+ * unsolvable cycles (due to Cuckoo's theory).
+ *
+ * Certainly, we rely on the fact we will delete elements with same speed
+ * as we add them, and even unsolvable cycles will be destroyed soon by
+ * concurrent deletions.
*/
- char pad[PG_CACHE_LINE_SIZE];
+ WALPrevPosLink PrevLinksHash[PREV_LINKS_HASH_CAPA];
/*
* fullPageWrites is the authoritative value used by all backends to
@@ -700,6 +737,15 @@ static void CopyXLogRecordToWAL(int write_len, bool isLogSwitch,
XLogRecData *rdata,
XLogRecPtr StartPos, XLogRecPtr EndPos,
TimeLineID tli);
+
+static bool WALPrevPosLinkInsert(WALPrevPosLink * link, WALPrevPosLinkVal val);
+static bool WALPrevPosLinkConsume(WALPrevPosLink * link, WALPrevPosLinkVal * val);
+static bool WALPrevPosLinkSwap(WALPrevPosLink * link, WALPrevPosLinkVal * val);
+static void LinkAndFindPrevPos(XLogRecPtr StartPos, XLogRecPtr EndPos,
+ XLogRecPtr *PrevPtr);
+static void LinkStartPrevPos(XLogRecPtr EndOfLog, XLogRecPtr LastRec);
+static XLogRecPtr ReadInsertCurrBytePos(void);
+
static void ReserveXLogInsertLocation(int size, XLogRecPtr *StartPos,
XLogRecPtr *EndPos, XLogRecPtr *PrevPtr);
static bool ReserveXLogSwitch(XLogRecPtr *StartPos, XLogRecPtr *EndPos,
@@ -1086,6 +1132,192 @@ XLogInsertRecord(XLogRecData *rdata,
return EndPos;
}
+/*
+ * Attempt to write into empty link.
+ */
+static pg_attribute_always_inline bool
+WALPrevPosLinkInsert(WALPrevPosLink * link, WALPrevPosLinkVal val)
+{
+ uint32 empty = 0;
+
+ /* first check it read-only */
+ if (pg_atomic_read_u32(&link->PrevSize) != 0)
+ return false;
+ if (!pg_atomic_compare_exchange_u32(&link->PrevSize, &empty, 1))
+ /* someone else occupied the entry */
+ return false;
+
+ pg_atomic_write_u32(&link->CurrPosId, val.CurrPosId);
+ /* This write acts as unlock as well. */
+ pg_atomic_write_membarrier_u32(&link->PrevSize, val.PrevSize);
+ return true;
+}
+
+/*
+ * Attempt to consume matched link.
+ */
+static pg_attribute_always_inline bool
+WALPrevPosLinkConsume(WALPrevPosLink * link, WALPrevPosLinkVal * val)
+{
+
+ if (pg_atomic_read_u32(&link->CurrPosId) != val->CurrPosId)
+ return false;
+
+ /* Try lock */
+ val->PrevSize = pg_atomic_fetch_or_u32(&link->PrevSize, 1);
+ if (val->PrevSize & 1)
+ /* Lock failed */
+ return false;
+
+ if (pg_atomic_read_u32(&link->CurrPosId) != val->CurrPosId)
+ {
+ /* unlock with old value */
+ pg_atomic_write_u32(&link->PrevSize, val->PrevSize);
+ return false;
+ }
+
+ pg_atomic_write_u32(&link->CurrPosId, 0);
+ /* This write acts as unlock as well. */
+ pg_atomic_write_membarrier_u32(&link->PrevSize, 0);
+ return true;
+}
+
+/*
+ * Attempt to swap entry: remember existing link and write our.
+ * It could happen we consume empty entry. Caller will detect it by checking
+ * remembered value.
+ */
+static pg_attribute_always_inline bool
+WALPrevPosLinkSwap(WALPrevPosLink * link, WALPrevPosLinkVal * val)
+{
+ uint32 oldCur;
+ uint32 oldPrev;
+
+ /* Attempt to lock entry against concurrent consumer or swapper */
+ oldPrev = pg_atomic_fetch_or_u32(&link->PrevSize, 1);
+ if (oldPrev & 1)
+ /* Lock failed */
+ return false;
+
+ oldCur = pg_atomic_read_u32(&link->CurrPosId);
+ pg_atomic_write_u32(&link->CurrPosId, val->CurrPosId);
+ /* This write acts as unlock as well. */
+ pg_atomic_write_membarrier_u32(&link->PrevSize, val->PrevSize);
+
+ val->CurrPosId = oldCur;
+ val->PrevSize = oldPrev;
+ return true;
+}
+
+static pg_attribute_always_inline void
+CalcCuckooPositions(uint32 ptr, uint32 pos[4])
+{
+ uint32 hash = murmurhash32(ptr);
+
+ pos[0] = hash % PREV_LINKS_HASH_CAPA;
+ pos[1] = (pos[0] + 1) % PREV_LINKS_HASH_CAPA;
+ pos[2] = (hash >> 16) % PREV_LINKS_HASH_CAPA;
+ pos[3] = (pos[2] + 2) % PREV_LINKS_HASH_CAPA;
+}
+
+/*
+ * Write new link (EndPos, StartPos) and find PrevPtr for StartPos.
+ *
+ * Links are stored in lock-free Cuckoo based hash-table.
+ * We use mostly-4 way Cuckoo hashing which provides high fill rate without
+ * hard cycle collisions. Also we rely on concurrent consumers of existing
+ * entry, so cycles will be broken in mean time.
+ *
+ * Cuckoo hashing relies on re-insertion for balancing, so we occasionally
+ * swaps entry and try to insert swapped instead of our.
+ */
+static void
+LinkAndFindPrevPos(XLogRecPtr StartPos, XLogRecPtr EndPos, XLogRecPtr *PrevPtr)
+{
+ WALPrevPosLink *hashtable = XLogCtl->Insert.PrevLinksHash;
+ WALPrevPosLinkVal lookup = {
+ .CurrPosId = StartPos ^ (StartPos >> 32),
+ };
+ WALPrevPosLinkVal insert = {
+ .CurrPosId = EndPos ^ (EndPos >> 32),
+ .PrevSize = EndPos - StartPos
+ };
+ pg_prng_state prng;
+ uint32 lookup_pos[4];
+ uint32 insert_pos[4];
+ uint32 i;
+ bool inserted = false;
+ bool found = false;
+
+ CalcCuckooPositions(lookup.CurrPosId, lookup_pos);
+ CalcCuckooPositions(insert.CurrPosId, insert_pos);
+ pg_prng_seed(&prng, StartPos);
+
+ while (!inserted || !found)
+ {
+ for (i = 0; !found && i < 4; i++)
+ found = WALPrevPosLinkConsume(&hashtable[lookup_pos[i]], &lookup);
+
+ if (inserted)
+ goto next;
+
+ for (i = 0; !inserted && i < 4; i++)
+ inserted = WALPrevPosLinkInsert(&hashtable[insert_pos[i]], insert);
+
+ if (inserted)
+ goto next;
+
+ if (pg_prng_uint32(&prng) % SWAP_ONCE_IN != 0)
+ goto next;
+
+ i = pg_prng_uint32(&prng) % 4;
+ if (!WALPrevPosLinkSwap(&hashtable[insert_pos[i]], &insert))
+ goto next;
+
+ if (insert.PrevSize == 0)
+ /* Lucky case: entry become empty and we inserted into */
+ inserted = true;
+ else if (insert.CurrPosId == lookup.CurrPosId)
+ {
+ /*
+ * We occasionally replaced entry we looked for. No need to insert
+ * it again.
+ */
+ inserted = true;
+ Assert(!found);
+ found = true;
+ lookup.PrevSize = insert.PrevSize;
+ break;
+ }
+ else
+ CalcCuckooPositions(insert.CurrPosId, insert_pos);
+
+next:
+ pg_spin_delay();
+ }
+
+ *PrevPtr = StartPos - lookup.PrevSize;
+}
+
+static pg_attribute_always_inline void
+LinkStartPrevPos(XLogRecPtr EndOfLog, XLogRecPtr LastRec)
+{
+ WALPrevPosLink *hashtable = XLogCtl->Insert.PrevLinksHash;
+ uint32 insert_pos[4];
+
+ CalcCuckooPositions(EndOfLog ^ (EndOfLog >> 32), insert_pos);
+ pg_atomic_write_u32(&hashtable[insert_pos[0]].CurrPosId,
+ EndOfLog ^ (EndOfLog >> 32));
+ pg_atomic_write_u32(&hashtable[insert_pos[0]].PrevSize,
+ EndOfLog - LastRec);
+}
+
+static pg_attribute_always_inline XLogRecPtr
+ReadInsertCurrBytePos(void)
+{
+ return pg_atomic_read_u64(&XLogCtl->Insert.CurrBytePos);
+}
+
/*
* Reserves the right amount of space for a record of given size from the WAL.
* *StartPos is set to the beginning of the reserved section, *EndPos to
@@ -1118,25 +1350,9 @@ ReserveXLogInsertLocation(int size, XLogRecPtr *StartPos, XLogRecPtr *EndPos,
/* All (non xlog-switch) records should contain data. */
Assert(size > SizeOfXLogRecord);
- /*
- * The duration the spinlock needs to be held is minimized by minimizing
- * the calculations that have to be done while holding the lock. The
- * current tip of reserved WAL is kept in CurrBytePos, as a byte position
- * that only counts "usable" bytes in WAL, that is, it excludes all WAL
- * page headers. The mapping between "usable" byte positions and physical
- * positions (XLogRecPtrs) can be done outside the locked region, and
- * because the usable byte position doesn't include any headers, reserving
- * X bytes from WAL is almost as simple as "CurrBytePos += X".
- */
- SpinLockAcquire(&Insert->insertpos_lck);
-
- startbytepos = Insert->CurrBytePos;
+ startbytepos = pg_atomic_fetch_add_u64(&Insert->CurrBytePos, size);
endbytepos = startbytepos + size;
- prevbytepos = Insert->PrevBytePos;
- Insert->CurrBytePos = endbytepos;
- Insert->PrevBytePos = startbytepos;
-
- SpinLockRelease(&Insert->insertpos_lck);
+ LinkAndFindPrevPos(startbytepos, endbytepos, &prevbytepos);
*StartPos = XLogBytePosToRecPtr(startbytepos);
*EndPos = XLogBytePosToEndRecPtr(endbytepos);
@@ -1172,26 +1388,24 @@ ReserveXLogSwitch(XLogRecPtr *StartPos, XLogRecPtr *EndPos, XLogRecPtr *PrevPtr)
uint32 segleft;
/*
- * These calculations are a bit heavy-weight to be done while holding a
- * spinlock, but since we're holding all the WAL insertion locks, there
- * are no other inserters competing for it. GetXLogInsertRecPtr() does
- * compete for it, but that's not called very frequently.
+ * Currently ReserveXLogInsertLocation is protected with exclusive
+ * insertion lock, so there is no contention against CurrBytePos, But we
+ * still do CAS loop for being uniform.
+ *
+ * Probably we'll get rid of exclusive lock in a future.
*/
- SpinLockAcquire(&Insert->insertpos_lck);
- startbytepos = Insert->CurrBytePos;
+repeat:
+ startbytepos = pg_atomic_read_u64(&Insert->CurrBytePos);
ptr = XLogBytePosToEndRecPtr(startbytepos);
if (XLogSegmentOffset(ptr, wal_segment_size) == 0)
{
- SpinLockRelease(&Insert->insertpos_lck);
*EndPos = *StartPos = ptr;
return false;
}
endbytepos = startbytepos + size;
- prevbytepos = Insert->PrevBytePos;
-
*StartPos = XLogBytePosToRecPtr(startbytepos);
*EndPos = XLogBytePosToEndRecPtr(endbytepos);
@@ -1202,10 +1416,19 @@ ReserveXLogSwitch(XLogRecPtr *StartPos, XLogRecPtr *EndPos, XLogRecPtr *PrevPtr)
*EndPos += segleft;
endbytepos = XLogRecPtrToBytePos(*EndPos);
}
- Insert->CurrBytePos = endbytepos;
- Insert->PrevBytePos = startbytepos;
- SpinLockRelease(&Insert->insertpos_lck);
+ if (!pg_atomic_compare_exchange_u64(&Insert->CurrBytePos,
+ &startbytepos,
+ endbytepos))
+ {
+ /*
+ * Don't use spin delay here: perform_spin_delay primary case is for
+ * solving single core contention. But on single core we will succeed
+ * on the next attempt.
+ */
+ goto repeat;
+ }
+ LinkAndFindPrevPos(startbytepos, endbytepos, &prevbytepos);
*PrevPtr = XLogBytePosToRecPtr(prevbytepos);
@@ -1507,7 +1730,6 @@ WaitXLogInsertionsToFinish(XLogRecPtr upto)
XLogRecPtr inserted;
XLogRecPtr reservedUpto;
XLogRecPtr finishedUpto;
- XLogCtlInsert *Insert = &XLogCtl->Insert;
int i;
if (MyProc == NULL)
@@ -1522,9 +1744,7 @@ WaitXLogInsertionsToFinish(XLogRecPtr upto)
return inserted;
/* Read the current insert position */
- SpinLockAcquire(&Insert->insertpos_lck);
- bytepos = Insert->CurrBytePos;
- SpinLockRelease(&Insert->insertpos_lck);
+ bytepos = ReadInsertCurrBytePos();
reservedUpto = XLogBytePosToEndRecPtr(bytepos);
/*
@@ -5017,12 +5237,18 @@ XLOGShmemInit(void)
XLogCtl->InstallXLogFileSegmentActive = false;
XLogCtl->WalWriterSleeping = false;
- SpinLockInit(&XLogCtl->Insert.insertpos_lck);
SpinLockInit(&XLogCtl->info_lck);
+ pg_atomic_init_u64(&XLogCtl->Insert.CurrBytePos, 0);
pg_atomic_init_u64(&XLogCtl->logInsertResult, InvalidXLogRecPtr);
pg_atomic_init_u64(&XLogCtl->logWriteResult, InvalidXLogRecPtr);
pg_atomic_init_u64(&XLogCtl->logFlushResult, InvalidXLogRecPtr);
pg_atomic_init_u64(&XLogCtl->unloggedLSN, InvalidXLogRecPtr);
+
+ for (i = 0; i < PREV_LINKS_HASH_CAPA; i++)
+ {
+ pg_atomic_init_u32(&XLogCtl->Insert.PrevLinksHash[i].CurrPosId, 0);
+ pg_atomic_init_u32(&XLogCtl->Insert.PrevLinksHash[i].PrevSize, 0);
+ }
}
/*
@@ -6018,8 +6244,13 @@ StartupXLOG(void)
* previous incarnation.
*/
Insert = &XLogCtl->Insert;
- Insert->PrevBytePos = XLogRecPtrToBytePos(endOfRecoveryInfo->lastRec);
- Insert->CurrBytePos = XLogRecPtrToBytePos(EndOfLog);
+ {
+ XLogRecPtr endOfLog = XLogRecPtrToBytePos(EndOfLog);
+ XLogRecPtr lastRec = XLogRecPtrToBytePos(endOfRecoveryInfo->lastRec);
+
+ pg_atomic_write_u64(&Insert->CurrBytePos, endOfLog);
+ LinkStartPrevPos(endOfLog, lastRec);
+ }
/*
* Tricky point here: lastPage contains the *last* block that the LastRec
@@ -7005,7 +7236,7 @@ CreateCheckPoint(int flags)
if (shutdown)
{
- XLogRecPtr curInsert = XLogBytePosToRecPtr(Insert->CurrBytePos);
+ XLogRecPtr curInsert = XLogBytePosToRecPtr(ReadInsertCurrBytePos());
/*
* Compute new REDO record ptr = location of next XLOG record.
@@ -9434,14 +9665,7 @@ register_persistent_abort_backup_handler(void)
XLogRecPtr
GetXLogInsertRecPtr(void)
{
- XLogCtlInsert *Insert = &XLogCtl->Insert;
- uint64 current_bytepos;
-
- SpinLockAcquire(&Insert->insertpos_lck);
- current_bytepos = Insert->CurrBytePos;
- SpinLockRelease(&Insert->insertpos_lck);
-
- return XLogBytePosToRecPtr(current_bytepos);
+ return XLogBytePosToRecPtr(ReadInsertCurrBytePos());
}
/*
--
2.39.3 (Apple Git-146)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [RFC] Lock-free XLog Reservation from WAL
@ 2025-01-09 16:03 Zhou, Zhiguo <[email protected]>
parent: Юрий Соколов <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Zhou, Zhiguo @ 2025-01-09 16:03 UTC (permalink / raw)
To: Юрий Соколов <[email protected]>; +Cc: wenhui qiu <[email protected]>; pgsql-hackers
On 1/7/2025 10:49 AM, Юрий Соколов wrote:
>
>> On 6 Jan 2025, at 09:46, Zhou, Zhiguo <[email protected]> wrote:
>>
>> Hi Yura and Wenhui,
>>
>> Thanks for kindly reviewing this work!
>>
>> On 1/3/2025 9:01 PM, wenhui qiu wrote:
>>> Hi
>>> Thank you for your path，NUM_XLOGINSERT_LOCKS increase to 128，I
>>> think it will be challenged，do we make it guc ？
>>
>> I noticed there have been some discussions (for example, [1] and its
>> responses) about making NUM_XLOGINSERT_LOCKS a GUC, which seems to be
>> a controversial proposal. Given that, we may first focus on the lock-
>> free XLog reservation implementation, and leave the increase of
>> NUM_XLOGINSERT_LOCKS for a future patch, where we would provide more
>> quantitative evidence for the various implementations. WDYT?
>>
>>
>>> On Fri, 3 Jan 2025 at 20:36, Yura Sokolov <[email protected]
>>> <mailto:[email protected]><mailto:[email protected]
>>> <mailto:[email protected]>>> wrote:
>>> Good day, Zhiguo.
>>> Idea looks great.
>>> Minor issue:
>>> - you didn't remove use of `insertpos_lck` from `ReserveXLogSwitch`.
>>> I initially thought it became un-synchronized against
>>> `ReserveXLogInsertLocation`, but looking closer I found it is
>>> synchronized with `WALInsertLockAcquireExclusive`.
>>> Since there are no other `insertpos_lck` usages after your patch, I
>>> don't see why it should exists and be used in `ReserveXLogSwitch`.
>>> Still I'd prefer to see CAS loop in this place to be consistent with
>>> other non-locking access. And it will allow to get rid of
>>> `WALInsertLockAcquireExclusive`, (though probably it is not a big
>>> issue).
>>
>> Exactly, it should be safe to remove `insertpos_lck`. And I agree with
>> you on getting rid of `WALInsertLockAcquireExclusive` with CAS loop
>> which should significantly reduce the synchronization cost here
>> especially when we intend to increase NUM_XLOGINSERT_LOCKS. I will try
>> it in the next version of patch.
>>
>>
>>> Major issue:
>>> - `SetPrevRecPtr` and `GetPrevRecPtr` do non-atomic write/read with on
>>> platforms where MAXALIGN != 8 or without native 64 load/store. Branch
>>> with 'memcpy` is rather obvious, but even pointer de-referencing on
>>> "lucky case" is not safe either.
>>> I have no idea how to fix it at the moment.
>>
>> Indeed, non-atomic write/read operations can lead to safety issues in
>> some situations. My initial thought is to define a bit near the prev-
>> link to flag the completion of the update. In this way, we could allow
>> non-atomic or even discontinuous write/read operations on the prev-
>> link, while simultaneously guaranteeing its atomicity through atomic
>> operations (as well as memory barriers) on the flag bit. What do you
>> think of this as a viable solution?
>>
>>
>>> Readability issue:
>>> - It would be good to add `Assert(ptr >= upto)` into `GetXLogBuffer`.
>>> I had hard time to recognize `upto` is strictly not in the future.
>>> - Certainly, final version have to have fixed and improved comments.
>>> Many patch's ideas are strictly non-obvious. I had hard time to
>>> recognize patch is not a piece of ... (excuse me for the swear
>>> sentence).
>>
>> Thanks for the suggestion and patience. It's really more readable
>> after inserting the assertion, I will fix it and improve other
>> comments in the following patches.
>>
>>
>>> Indeed, patch is much better than it looks on first sight.
>>> I came with alternative idea yesterday, but looking closer to your
>>> patch
>>> today I see it is superior to mine (if atomic access will be fixed).
>>
>> [1]https://www.postgresql.org/message-
>> id/2266698.1704854297%40sss.pgh.pa.us <https://www.postgresql.org/
>> message-id/2266698.1704854297%40sss.pgh.pa.us>
>
> Good day, Zhiguo.
>
> Here’s my attempt to organise link to previous record without messing
> with xlog buffers:
> - link is stored in lock-free hash table instead.
>
> I don’t claim it is any better than using xlog buffers.
> It is just alternative vision.
>
> Some tricks in implementation:
> - Relying on byte-position nature, it could be converted to 32 bit unique
> value with `(uint32)(pos ^ (pos>>32))`. Certainly it is not totally
> unique,
> but it is certainly unique among 32GB consecutive log.
> - PrevBytePos could be calculated as a difference between positions, and
> this difference is certainly less than 4GB, so it also could be
> stored as 32
> bit value (PrevSize).
> - Since xlog records are aligned we could use lowest bit of PrevSize as
> a lock.
> - While Cuckoo Hashing could suffer from un-solvable cycle conflicts,
> this implementation relies on concurrent deleters which will eventually
> break such cycles if any.
>
> I have a version without 32bit conversion trick, and it is a bit lighter
> on atomic instructions count, but it performs badly in absence of native
> 64bit atomics.
>
> ——
> regards
> Yura Sokolov aka funny-falcon
Good day, Yura!
Your implementation based on the lock-free hash table is truly
impressive! One of the aspects I particularly admire is how your
solution doesn't require breaking the current convention of XLog
insertion, whose revision is quite error-prone and ungraceful. My minor
concern is that the limited number of entries (256) in the hash table
would be a bottleneck for parallel memory reservation, but I believe
this is not a critical issue.
I will soon try to evaluate the performance impact of your patch on my
device with the TPCC benchmark and also profile it to see if there are
any changes that could be made to further improve it.
BTW, do you have a plan to merge this patch to the master branch? Thanks!
Regards,
Zhiguo
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2025-01-09 16:03 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-05-28 18:15 [PATCH v24 1/5] Remove pg_collation.collversion. Thomas Munro <[email protected]>
2025-01-03 13:01 Re: [RFC] Lock-free XLog Reservation from WAL wenhui qiu <[email protected]>
2025-01-06 06:46 ` Re: [RFC] Lock-free XLog Reservation from WAL Zhou, Zhiguo <[email protected]>
2025-01-06 08:35 ` Re: [RFC] Lock-free XLog Reservation from WAL wenhui qiu <[email protected]>
2025-01-07 02:49 ` Re: [RFC] Lock-free XLog Reservation from WAL Юрий Соколов <[email protected]>
2025-01-09 16:03 ` Re: [RFC] Lock-free XLog Reservation from WAL Zhou, Zhiguo <[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