public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 2/4] Change-tmplinit-argument
15+ messages / 7 participants
[nested] [flat]
* [PATCH 2/4] Change-tmplinit-argument
@ 2019-01-17 12:05 Arthur Zakirov <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Arthur Zakirov @ 2019-01-17 12:05 UTC (permalink / raw)
---
contrib/dict_int/dict_int.c | 4 +-
contrib/dict_xsyn/dict_xsyn.c | 4 +-
contrib/unaccent/unaccent.c | 4 +-
src/backend/commands/tsearchcmds.c | 10 ++++-
src/backend/snowball/dict_snowball.c | 4 +-
src/backend/tsearch/dict_ispell.c | 4 +-
src/backend/tsearch/dict_simple.c | 4 +-
src/backend/tsearch/dict_synonym.c | 4 +-
src/backend/tsearch/dict_thesaurus.c | 4 +-
src/backend/utils/cache/ts_cache.c | 13 +++++-
src/include/tsearch/ts_cache.h | 4 ++
src/include/tsearch/ts_public.h | 67 ++++++++++++++++++++++++++--
12 files changed, 105 insertions(+), 21 deletions(-)
diff --git a/contrib/dict_int/dict_int.c b/contrib/dict_int/dict_int.c
index 628b9769c3..ddde55eee4 100644
--- a/contrib/dict_int/dict_int.c
+++ b/contrib/dict_int/dict_int.c
@@ -30,7 +30,7 @@ PG_FUNCTION_INFO_V1(dintdict_lexize);
Datum
dintdict_init(PG_FUNCTION_ARGS)
{
- List *dictoptions = (List *) PG_GETARG_POINTER(0);
+ DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictInt *d;
ListCell *l;
@@ -38,7 +38,7 @@ dintdict_init(PG_FUNCTION_ARGS)
d->maxlen = 6;
d->rejectlong = false;
- foreach(l, dictoptions)
+ foreach(l, init_data->dict_options)
{
DefElem *defel = (DefElem *) lfirst(l);
diff --git a/contrib/dict_xsyn/dict_xsyn.c b/contrib/dict_xsyn/dict_xsyn.c
index 509e14aee0..15b1a0033a 100644
--- a/contrib/dict_xsyn/dict_xsyn.c
+++ b/contrib/dict_xsyn/dict_xsyn.c
@@ -140,7 +140,7 @@ read_dictionary(DictSyn *d, const char *filename)
Datum
dxsyn_init(PG_FUNCTION_ARGS)
{
- List *dictoptions = (List *) PG_GETARG_POINTER(0);
+ DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictSyn *d;
ListCell *l;
char *filename = NULL;
@@ -153,7 +153,7 @@ dxsyn_init(PG_FUNCTION_ARGS)
d->matchsynonyms = false;
d->keepsynonyms = true;
- foreach(l, dictoptions)
+ foreach(l, init_data->dict_options)
{
DefElem *defel = (DefElem *) lfirst(l);
diff --git a/contrib/unaccent/unaccent.c b/contrib/unaccent/unaccent.c
index fc5176e338..f3663cefd0 100644
--- a/contrib/unaccent/unaccent.c
+++ b/contrib/unaccent/unaccent.c
@@ -270,12 +270,12 @@ PG_FUNCTION_INFO_V1(unaccent_init);
Datum
unaccent_init(PG_FUNCTION_ARGS)
{
- List *dictoptions = (List *) PG_GETARG_POINTER(0);
+ DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
TrieChar *rootTrie = NULL;
bool fileloaded = false;
ListCell *l;
- foreach(l, dictoptions)
+ foreach(l, init_data->dict_options)
{
DefElem *defel = (DefElem *) lfirst(l);
diff --git a/src/backend/commands/tsearchcmds.c b/src/backend/commands/tsearchcmds.c
index 8e5eec22b5..30c5eb72a2 100644
--- a/src/backend/commands/tsearchcmds.c
+++ b/src/backend/commands/tsearchcmds.c
@@ -389,17 +389,25 @@ verify_dictoptions(Oid tmplId, List *dictoptions)
}
else
{
+ DictInitData init_data;
+
/*
* Copy the options just in case init method thinks it can scribble on
* them ...
*/
dictoptions = copyObject(dictoptions);
+ init_data.dict_options = dictoptions;
+ init_data.dict.id = InvalidOid;
+ init_data.dict.xmin = InvalidTransactionId;
+ init_data.dict.xmax = InvalidTransactionId;
+ ItemPointerSetInvalid(&init_data.dict.tid);
+
/*
* Call the init method and see if it complains. We don't worry about
* it leaking memory, since our command will soon be over anyway.
*/
- (void) OidFunctionCall1(initmethod, PointerGetDatum(dictoptions));
+ (void) OidFunctionCall1(initmethod, PointerGetDatum(&init_data));
}
ReleaseSysCache(tup);
diff --git a/src/backend/snowball/dict_snowball.c b/src/backend/snowball/dict_snowball.c
index 5166738310..f30f29865c 100644
--- a/src/backend/snowball/dict_snowball.c
+++ b/src/backend/snowball/dict_snowball.c
@@ -201,14 +201,14 @@ locate_stem_module(DictSnowball *d, const char *lang)
Datum
dsnowball_init(PG_FUNCTION_ARGS)
{
- List *dictoptions = (List *) PG_GETARG_POINTER(0);
+ DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictSnowball *d;
bool stoploaded = false;
ListCell *l;
d = (DictSnowball *) palloc0(sizeof(DictSnowball));
- foreach(l, dictoptions)
+ foreach(l, init_data->dict_options)
{
DefElem *defel = (DefElem *) lfirst(l);
diff --git a/src/backend/tsearch/dict_ispell.c b/src/backend/tsearch/dict_ispell.c
index 8b05a477f1..fc9a96abca 100644
--- a/src/backend/tsearch/dict_ispell.c
+++ b/src/backend/tsearch/dict_ispell.c
@@ -29,7 +29,7 @@ typedef struct
Datum
dispell_init(PG_FUNCTION_ARGS)
{
- List *dictoptions = (List *) PG_GETARG_POINTER(0);
+ DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictISpell *d;
bool affloaded = false,
dictloaded = false,
@@ -40,7 +40,7 @@ dispell_init(PG_FUNCTION_ARGS)
NIStartBuild(&(d->obj));
- foreach(l, dictoptions)
+ foreach(l, init_data->dict_options)
{
DefElem *defel = (DefElem *) lfirst(l);
diff --git a/src/backend/tsearch/dict_simple.c b/src/backend/tsearch/dict_simple.c
index 2f62ef00c8..c92744641b 100644
--- a/src/backend/tsearch/dict_simple.c
+++ b/src/backend/tsearch/dict_simple.c
@@ -29,7 +29,7 @@ typedef struct
Datum
dsimple_init(PG_FUNCTION_ARGS)
{
- List *dictoptions = (List *) PG_GETARG_POINTER(0);
+ DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictSimple *d = (DictSimple *) palloc0(sizeof(DictSimple));
bool stoploaded = false,
acceptloaded = false;
@@ -37,7 +37,7 @@ dsimple_init(PG_FUNCTION_ARGS)
d->accept = true; /* default */
- foreach(l, dictoptions)
+ foreach(l, init_data->dict_options)
{
DefElem *defel = (DefElem *) lfirst(l);
diff --git a/src/backend/tsearch/dict_synonym.c b/src/backend/tsearch/dict_synonym.c
index b6226df940..d3f5f0da3f 100644
--- a/src/backend/tsearch/dict_synonym.c
+++ b/src/backend/tsearch/dict_synonym.c
@@ -91,7 +91,7 @@ compareSyn(const void *a, const void *b)
Datum
dsynonym_init(PG_FUNCTION_ARGS)
{
- List *dictoptions = (List *) PG_GETARG_POINTER(0);
+ DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictSyn *d;
ListCell *l;
char *filename = NULL;
@@ -104,7 +104,7 @@ dsynonym_init(PG_FUNCTION_ARGS)
char *line = NULL;
uint16 flags = 0;
- foreach(l, dictoptions)
+ foreach(l, init_data->dict_options)
{
DefElem *defel = (DefElem *) lfirst(l);
diff --git a/src/backend/tsearch/dict_thesaurus.c b/src/backend/tsearch/dict_thesaurus.c
index 75f8deef6a..8962e252e0 100644
--- a/src/backend/tsearch/dict_thesaurus.c
+++ b/src/backend/tsearch/dict_thesaurus.c
@@ -604,7 +604,7 @@ compileTheSubstitute(DictThesaurus *d)
Datum
thesaurus_init(PG_FUNCTION_ARGS)
{
- List *dictoptions = (List *) PG_GETARG_POINTER(0);
+ DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictThesaurus *d;
char *subdictname = NULL;
bool fileloaded = false;
@@ -612,7 +612,7 @@ thesaurus_init(PG_FUNCTION_ARGS)
d = (DictThesaurus *) palloc0(sizeof(DictThesaurus));
- foreach(l, dictoptions)
+ foreach(l, init_data->dict_options)
{
DefElem *defel = (DefElem *) lfirst(l);
diff --git a/src/backend/utils/cache/ts_cache.c b/src/backend/utils/cache/ts_cache.c
index 0545efc75b..8bc8d82c76 100644
--- a/src/backend/utils/cache/ts_cache.c
+++ b/src/backend/utils/cache/ts_cache.c
@@ -39,6 +39,7 @@
#include "catalog/pg_ts_template.h"
#include "commands/defrem.h"
#include "tsearch/ts_cache.h"
+#include "tsearch/ts_public.h"
#include "utils/builtins.h"
#include "utils/catcache.h"
#include "utils/fmgroids.h"
@@ -311,11 +312,15 @@ lookup_ts_dictionary_cache(Oid dictId)
MemSet(entry, 0, sizeof(TSDictionaryCacheEntry));
entry->dictId = dictId;
entry->dictCtx = saveCtx;
+ entry->dict_xmin = HeapTupleHeaderGetRawXmin(tpdict->t_data);
+ entry->dict_xmax = HeapTupleHeaderGetRawXmax(tpdict->t_data);
+ entry->dict_tid = tpdict->t_self;
entry->lexizeOid = template->tmpllexize;
if (OidIsValid(template->tmplinit))
{
+ DictInitData init_data;
List *dictoptions;
Datum opt;
bool isnull;
@@ -335,9 +340,15 @@ lookup_ts_dictionary_cache(Oid dictId)
else
dictoptions = deserialize_deflist(opt);
+ init_data.dict_options = dictoptions;
+ init_data.dict.id = dictId;
+ init_data.dict.xmin = entry->dict_xmin;
+ init_data.dict.xmax = entry->dict_xmax;
+ init_data.dict.tid = entry->dict_tid;
+
entry->dictData =
DatumGetPointer(OidFunctionCall1(template->tmplinit,
- PointerGetDatum(dictoptions)));
+ PointerGetDatum(&init_data)));
MemoryContextSwitchTo(oldcontext);
}
diff --git a/src/include/tsearch/ts_cache.h b/src/include/tsearch/ts_cache.h
index 77e325d101..2298e0a275 100644
--- a/src/include/tsearch/ts_cache.h
+++ b/src/include/tsearch/ts_cache.h
@@ -54,6 +54,10 @@ typedef struct TSDictionaryCacheEntry
Oid dictId;
bool isvalid;
+ TransactionId dict_xmin; /* XMIN of the dictionary's tuple */
+ TransactionId dict_xmax; /* XMAX of the dictionary's tuple */
+ ItemPointerData dict_tid; /* TID of the dictionary's tuple */
+
/* most frequent fmgr call */
Oid lexizeOid;
FmgrInfo lexize;
diff --git a/src/include/tsearch/ts_public.h b/src/include/tsearch/ts_public.h
index b325fa122c..db028ed6ad 100644
--- a/src/include/tsearch/ts_public.h
+++ b/src/include/tsearch/ts_public.h
@@ -13,6 +13,8 @@
#ifndef _PG_TS_PUBLIC_H_
#define _PG_TS_PUBLIC_H_
+#include "nodes/pg_list.h"
+#include "storage/itemptr.h"
#include "tsearch/ts_type.h"
/*
@@ -81,10 +83,69 @@ extern void readstoplist(const char *fname, StopList *s,
extern bool searchstoplist(StopList *s, char *key);
/*
- * Interface with dictionaries
+ * API for text search dictionaries.
+ *
+ * API functions to manage a text search dictionary are defined by a text search
+ * template. Currently an existing template cannot be altered in order to use
+ * different functions. API consists of the following functions:
+ *
+ * init function
+ * -------------
+ * - optional function which initializes internal structures of the dictionary
+ * - accepts DictInitData structure as an argument and must return a custom
+ * palloc'd structure which stores content of the processed dictionary and
+ * is used by lexize function
+ *
+ * lexize function
+ * ---------------
+ * - normalizes a single word (token) using specific dictionary
+ * - returns a palloc'd array of TSLexeme, with a terminating NULL entry
+ * - accepts the following arguments:
+ *
+ * - dictData - pointer to a structure returned by init function or NULL if
+ * init function wasn't defined by the template
+ * - token - string to normalize (not null-terminated)
+ * - length - length of the token
+ * - dictState - pointer to a DictSubState structure storing current
+ * state of a set of tokens processing and allows to normalize phrases
+ */
+
+/*
+ * A preprocessed dictionary can be stored in shared memory using DSM - this is
+ * decided in the init function. A DSM segment is released after altering or
+ * dropping the dictionary. The segment may still leak, when a backend uses the
+ * dictionary right before dropping - in that case the backend will hold the DSM
+ * untill it disconnects or calls lookup_ts_dictionary_cache().
+ *
+ * DictEntryData represents DSM segment with a preprocessed dictionary. We need
+ * to ensure the content of the DSM segment is still valid, which is what xmin,
+ * xmax and tid are for.
+ */
+typedef struct
+{
+ Oid id; /* OID of the dictionary */
+ TransactionId xmin; /* XMIN of the dictionary's tuple */
+ TransactionId xmax; /* XMAX of the dictionary's tuple */
+ ItemPointerData tid; /* TID of the dictionary's tuple */
+} DictEntryData;
+
+/*
+ * API structure for a dictionary initialization. It is passed as an argument
+ * to a template's init function.
*/
+typedef struct
+{
+ /* List of options for a template's init method */
+ List *dict_options;
+
+ /* Data used to allocate, search and release the DSM segment */
+ DictEntryData dict;
+} DictInitData;
-/* return struct for any lexize function */
+/*
+ * Return struct for any lexize function. They are combined into an array, the
+ * last entry is the terminating entry.
+ */
typedef struct
{
/*----------
@@ -108,7 +169,7 @@ typedef struct
uint16 flags; /* See flag bits below */
- char *lexeme; /* C string */
+ char *lexeme; /* C string (NULL for terminating entry) */
} TSLexeme;
/* Flag bits that can appear in TSLexeme.flags */
--
2.20.1
--------------9E285E8AF9A980CD1515771F
Content-Type: text/x-patch;
name="0003-Retrieve-shared-location-for-dict-v18.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="0003-Retrieve-shared-location-for-dict-v18.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH 2/4] Change-tmplinit-argument
@ 2019-01-17 12:05 Arthur Zakirov <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Arthur Zakirov @ 2019-01-17 12:05 UTC (permalink / raw)
Reviewed-by: Tomas Vondra, Ildus Kurbangaliev
---
contrib/dict_int/dict_int.c | 4 +-
contrib/dict_xsyn/dict_xsyn.c | 4 +-
contrib/unaccent/unaccent.c | 4 +-
src/backend/commands/tsearchcmds.c | 10 ++++-
src/backend/snowball/dict_snowball.c | 4 +-
src/backend/tsearch/dict_ispell.c | 4 +-
src/backend/tsearch/dict_simple.c | 4 +-
src/backend/tsearch/dict_synonym.c | 4 +-
src/backend/tsearch/dict_thesaurus.c | 4 +-
src/backend/utils/cache/ts_cache.c | 13 +++++-
src/include/tsearch/ts_cache.h | 4 ++
src/include/tsearch/ts_public.h | 67 ++++++++++++++++++++++++++--
12 files changed, 105 insertions(+), 21 deletions(-)
diff --git a/contrib/dict_int/dict_int.c b/contrib/dict_int/dict_int.c
index 628b9769c3..ddde55eee4 100644
--- a/contrib/dict_int/dict_int.c
+++ b/contrib/dict_int/dict_int.c
@@ -30,7 +30,7 @@ PG_FUNCTION_INFO_V1(dintdict_lexize);
Datum
dintdict_init(PG_FUNCTION_ARGS)
{
- List *dictoptions = (List *) PG_GETARG_POINTER(0);
+ DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictInt *d;
ListCell *l;
@@ -38,7 +38,7 @@ dintdict_init(PG_FUNCTION_ARGS)
d->maxlen = 6;
d->rejectlong = false;
- foreach(l, dictoptions)
+ foreach(l, init_data->dict_options)
{
DefElem *defel = (DefElem *) lfirst(l);
diff --git a/contrib/dict_xsyn/dict_xsyn.c b/contrib/dict_xsyn/dict_xsyn.c
index 509e14aee0..15b1a0033a 100644
--- a/contrib/dict_xsyn/dict_xsyn.c
+++ b/contrib/dict_xsyn/dict_xsyn.c
@@ -140,7 +140,7 @@ read_dictionary(DictSyn *d, const char *filename)
Datum
dxsyn_init(PG_FUNCTION_ARGS)
{
- List *dictoptions = (List *) PG_GETARG_POINTER(0);
+ DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictSyn *d;
ListCell *l;
char *filename = NULL;
@@ -153,7 +153,7 @@ dxsyn_init(PG_FUNCTION_ARGS)
d->matchsynonyms = false;
d->keepsynonyms = true;
- foreach(l, dictoptions)
+ foreach(l, init_data->dict_options)
{
DefElem *defel = (DefElem *) lfirst(l);
diff --git a/contrib/unaccent/unaccent.c b/contrib/unaccent/unaccent.c
index fc5176e338..f3663cefd0 100644
--- a/contrib/unaccent/unaccent.c
+++ b/contrib/unaccent/unaccent.c
@@ -270,12 +270,12 @@ PG_FUNCTION_INFO_V1(unaccent_init);
Datum
unaccent_init(PG_FUNCTION_ARGS)
{
- List *dictoptions = (List *) PG_GETARG_POINTER(0);
+ DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
TrieChar *rootTrie = NULL;
bool fileloaded = false;
ListCell *l;
- foreach(l, dictoptions)
+ foreach(l, init_data->dict_options)
{
DefElem *defel = (DefElem *) lfirst(l);
diff --git a/src/backend/commands/tsearchcmds.c b/src/backend/commands/tsearchcmds.c
index 8e5eec22b5..30c5eb72a2 100644
--- a/src/backend/commands/tsearchcmds.c
+++ b/src/backend/commands/tsearchcmds.c
@@ -389,17 +389,25 @@ verify_dictoptions(Oid tmplId, List *dictoptions)
}
else
{
+ DictInitData init_data;
+
/*
* Copy the options just in case init method thinks it can scribble on
* them ...
*/
dictoptions = copyObject(dictoptions);
+ init_data.dict_options = dictoptions;
+ init_data.dict.id = InvalidOid;
+ init_data.dict.xmin = InvalidTransactionId;
+ init_data.dict.xmax = InvalidTransactionId;
+ ItemPointerSetInvalid(&init_data.dict.tid);
+
/*
* Call the init method and see if it complains. We don't worry about
* it leaking memory, since our command will soon be over anyway.
*/
- (void) OidFunctionCall1(initmethod, PointerGetDatum(dictoptions));
+ (void) OidFunctionCall1(initmethod, PointerGetDatum(&init_data));
}
ReleaseSysCache(tup);
diff --git a/src/backend/snowball/dict_snowball.c b/src/backend/snowball/dict_snowball.c
index 5166738310..f30f29865c 100644
--- a/src/backend/snowball/dict_snowball.c
+++ b/src/backend/snowball/dict_snowball.c
@@ -201,14 +201,14 @@ locate_stem_module(DictSnowball *d, const char *lang)
Datum
dsnowball_init(PG_FUNCTION_ARGS)
{
- List *dictoptions = (List *) PG_GETARG_POINTER(0);
+ DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictSnowball *d;
bool stoploaded = false;
ListCell *l;
d = (DictSnowball *) palloc0(sizeof(DictSnowball));
- foreach(l, dictoptions)
+ foreach(l, init_data->dict_options)
{
DefElem *defel = (DefElem *) lfirst(l);
diff --git a/src/backend/tsearch/dict_ispell.c b/src/backend/tsearch/dict_ispell.c
index 8b05a477f1..fc9a96abca 100644
--- a/src/backend/tsearch/dict_ispell.c
+++ b/src/backend/tsearch/dict_ispell.c
@@ -29,7 +29,7 @@ typedef struct
Datum
dispell_init(PG_FUNCTION_ARGS)
{
- List *dictoptions = (List *) PG_GETARG_POINTER(0);
+ DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictISpell *d;
bool affloaded = false,
dictloaded = false,
@@ -40,7 +40,7 @@ dispell_init(PG_FUNCTION_ARGS)
NIStartBuild(&(d->obj));
- foreach(l, dictoptions)
+ foreach(l, init_data->dict_options)
{
DefElem *defel = (DefElem *) lfirst(l);
diff --git a/src/backend/tsearch/dict_simple.c b/src/backend/tsearch/dict_simple.c
index 2f62ef00c8..c92744641b 100644
--- a/src/backend/tsearch/dict_simple.c
+++ b/src/backend/tsearch/dict_simple.c
@@ -29,7 +29,7 @@ typedef struct
Datum
dsimple_init(PG_FUNCTION_ARGS)
{
- List *dictoptions = (List *) PG_GETARG_POINTER(0);
+ DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictSimple *d = (DictSimple *) palloc0(sizeof(DictSimple));
bool stoploaded = false,
acceptloaded = false;
@@ -37,7 +37,7 @@ dsimple_init(PG_FUNCTION_ARGS)
d->accept = true; /* default */
- foreach(l, dictoptions)
+ foreach(l, init_data->dict_options)
{
DefElem *defel = (DefElem *) lfirst(l);
diff --git a/src/backend/tsearch/dict_synonym.c b/src/backend/tsearch/dict_synonym.c
index b6226df940..d3f5f0da3f 100644
--- a/src/backend/tsearch/dict_synonym.c
+++ b/src/backend/tsearch/dict_synonym.c
@@ -91,7 +91,7 @@ compareSyn(const void *a, const void *b)
Datum
dsynonym_init(PG_FUNCTION_ARGS)
{
- List *dictoptions = (List *) PG_GETARG_POINTER(0);
+ DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictSyn *d;
ListCell *l;
char *filename = NULL;
@@ -104,7 +104,7 @@ dsynonym_init(PG_FUNCTION_ARGS)
char *line = NULL;
uint16 flags = 0;
- foreach(l, dictoptions)
+ foreach(l, init_data->dict_options)
{
DefElem *defel = (DefElem *) lfirst(l);
diff --git a/src/backend/tsearch/dict_thesaurus.c b/src/backend/tsearch/dict_thesaurus.c
index 75f8deef6a..8962e252e0 100644
--- a/src/backend/tsearch/dict_thesaurus.c
+++ b/src/backend/tsearch/dict_thesaurus.c
@@ -604,7 +604,7 @@ compileTheSubstitute(DictThesaurus *d)
Datum
thesaurus_init(PG_FUNCTION_ARGS)
{
- List *dictoptions = (List *) PG_GETARG_POINTER(0);
+ DictInitData *init_data = (DictInitData *) PG_GETARG_POINTER(0);
DictThesaurus *d;
char *subdictname = NULL;
bool fileloaded = false;
@@ -612,7 +612,7 @@ thesaurus_init(PG_FUNCTION_ARGS)
d = (DictThesaurus *) palloc0(sizeof(DictThesaurus));
- foreach(l, dictoptions)
+ foreach(l, init_data->dict_options)
{
DefElem *defel = (DefElem *) lfirst(l);
diff --git a/src/backend/utils/cache/ts_cache.c b/src/backend/utils/cache/ts_cache.c
index 0545efc75b..8bc8d82c76 100644
--- a/src/backend/utils/cache/ts_cache.c
+++ b/src/backend/utils/cache/ts_cache.c
@@ -39,6 +39,7 @@
#include "catalog/pg_ts_template.h"
#include "commands/defrem.h"
#include "tsearch/ts_cache.h"
+#include "tsearch/ts_public.h"
#include "utils/builtins.h"
#include "utils/catcache.h"
#include "utils/fmgroids.h"
@@ -311,11 +312,15 @@ lookup_ts_dictionary_cache(Oid dictId)
MemSet(entry, 0, sizeof(TSDictionaryCacheEntry));
entry->dictId = dictId;
entry->dictCtx = saveCtx;
+ entry->dict_xmin = HeapTupleHeaderGetRawXmin(tpdict->t_data);
+ entry->dict_xmax = HeapTupleHeaderGetRawXmax(tpdict->t_data);
+ entry->dict_tid = tpdict->t_self;
entry->lexizeOid = template->tmpllexize;
if (OidIsValid(template->tmplinit))
{
+ DictInitData init_data;
List *dictoptions;
Datum opt;
bool isnull;
@@ -335,9 +340,15 @@ lookup_ts_dictionary_cache(Oid dictId)
else
dictoptions = deserialize_deflist(opt);
+ init_data.dict_options = dictoptions;
+ init_data.dict.id = dictId;
+ init_data.dict.xmin = entry->dict_xmin;
+ init_data.dict.xmax = entry->dict_xmax;
+ init_data.dict.tid = entry->dict_tid;
+
entry->dictData =
DatumGetPointer(OidFunctionCall1(template->tmplinit,
- PointerGetDatum(dictoptions)));
+ PointerGetDatum(&init_data)));
MemoryContextSwitchTo(oldcontext);
}
diff --git a/src/include/tsearch/ts_cache.h b/src/include/tsearch/ts_cache.h
index 77e325d101..2298e0a275 100644
--- a/src/include/tsearch/ts_cache.h
+++ b/src/include/tsearch/ts_cache.h
@@ -54,6 +54,10 @@ typedef struct TSDictionaryCacheEntry
Oid dictId;
bool isvalid;
+ TransactionId dict_xmin; /* XMIN of the dictionary's tuple */
+ TransactionId dict_xmax; /* XMAX of the dictionary's tuple */
+ ItemPointerData dict_tid; /* TID of the dictionary's tuple */
+
/* most frequent fmgr call */
Oid lexizeOid;
FmgrInfo lexize;
diff --git a/src/include/tsearch/ts_public.h b/src/include/tsearch/ts_public.h
index b325fa122c..db028ed6ad 100644
--- a/src/include/tsearch/ts_public.h
+++ b/src/include/tsearch/ts_public.h
@@ -13,6 +13,8 @@
#ifndef _PG_TS_PUBLIC_H_
#define _PG_TS_PUBLIC_H_
+#include "nodes/pg_list.h"
+#include "storage/itemptr.h"
#include "tsearch/ts_type.h"
/*
@@ -81,10 +83,69 @@ extern void readstoplist(const char *fname, StopList *s,
extern bool searchstoplist(StopList *s, char *key);
/*
- * Interface with dictionaries
+ * API for text search dictionaries.
+ *
+ * API functions to manage a text search dictionary are defined by a text search
+ * template. Currently an existing template cannot be altered in order to use
+ * different functions. API consists of the following functions:
+ *
+ * init function
+ * -------------
+ * - optional function which initializes internal structures of the dictionary
+ * - accepts DictInitData structure as an argument and must return a custom
+ * palloc'd structure which stores content of the processed dictionary and
+ * is used by lexize function
+ *
+ * lexize function
+ * ---------------
+ * - normalizes a single word (token) using specific dictionary
+ * - returns a palloc'd array of TSLexeme, with a terminating NULL entry
+ * - accepts the following arguments:
+ *
+ * - dictData - pointer to a structure returned by init function or NULL if
+ * init function wasn't defined by the template
+ * - token - string to normalize (not null-terminated)
+ * - length - length of the token
+ * - dictState - pointer to a DictSubState structure storing current
+ * state of a set of tokens processing and allows to normalize phrases
+ */
+
+/*
+ * A preprocessed dictionary can be stored in shared memory using DSM - this is
+ * decided in the init function. A DSM segment is released after altering or
+ * dropping the dictionary. The segment may still leak, when a backend uses the
+ * dictionary right before dropping - in that case the backend will hold the DSM
+ * untill it disconnects or calls lookup_ts_dictionary_cache().
+ *
+ * DictEntryData represents DSM segment with a preprocessed dictionary. We need
+ * to ensure the content of the DSM segment is still valid, which is what xmin,
+ * xmax and tid are for.
+ */
+typedef struct
+{
+ Oid id; /* OID of the dictionary */
+ TransactionId xmin; /* XMIN of the dictionary's tuple */
+ TransactionId xmax; /* XMAX of the dictionary's tuple */
+ ItemPointerData tid; /* TID of the dictionary's tuple */
+} DictEntryData;
+
+/*
+ * API structure for a dictionary initialization. It is passed as an argument
+ * to a template's init function.
*/
+typedef struct
+{
+ /* List of options for a template's init method */
+ List *dict_options;
+
+ /* Data used to allocate, search and release the DSM segment */
+ DictEntryData dict;
+} DictInitData;
-/* return struct for any lexize function */
+/*
+ * Return struct for any lexize function. They are combined into an array, the
+ * last entry is the terminating entry.
+ */
typedef struct
{
/*----------
@@ -108,7 +169,7 @@ typedef struct
uint16 flags; /* See flag bits below */
- char *lexeme; /* C string */
+ char *lexeme; /* C string (NULL for terminating entry) */
} TSLexeme;
/* Flag bits that can appear in TSLexeme.flags */
--
2.21.0
--------------C3497FBBD55C428FF91561C0
Content-Type: text/x-patch;
name="0003-Retrieve-shared-location-for-dict-v19.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename="0003-Retrieve-shared-location-for-dict-v19.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* remap the .text segment into huge pages at run time
@ 2022-11-02 06:32 John Naylor <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: John Naylor @ 2022-11-02 06:32 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>; +Cc: Andres Freund <[email protected]>; Thomas Munro <[email protected]>
It's been known for a while that Postgres spends a lot of time translating
instruction addresses, and using huge pages in the text segment yields a
substantial performance boost in OLTP workloads [1][2]. The difficulty is,
this normally requires a lot of painstaking work (unless your OS does
superpage promotion, like FreeBSD).
I found an MIT-licensed library "iodlr" from Intel [3] that allows one to
remap the .text segment to huge pages at program start. Attached is a
hackish, Meson-only, "works on my machine" patchset to experiment with this
idea.
0001 adapts the library to our error logging and GUC system. The overview:
- read ELF info to get the start/end addresses of the .text segment
- calculate addresses therein aligned at huge page boundaries
- mmap a temporary region and memcpy the aligned portion of the .text
segment
- mmap aligned start address to a second region with huge pages and
MAP_FIXED
- memcpy over from the temp region and revoke the PROT_WRITE bit
The reason this doesn't "saw off the branch you're standing on" is that the
remapping is done in a function that's forced to live in a different
segment, and doesn't call any non-libc functions living elsewhere:
static void
__attribute__((__section__("lpstub")))
__attribute__((__noinline__))
MoveRegionToLargePages(const mem_range * r, int mmap_flags)
Debug messages show
2022-11-02 12:02:31.064 +07 [26955] DEBUG: .text start: 0x487540
2022-11-02 12:02:31.064 +07 [26955] DEBUG: .text end: 0x96cf12
2022-11-02 12:02:31.064 +07 [26955] DEBUG: aligned .text start: 0x600000
2022-11-02 12:02:31.064 +07 [26955] DEBUG: aligned .text end: 0x800000
2022-11-02 12:02:31.066 +07 [26955] DEBUG: binary mapped to huge pages
2022-11-02 12:02:31.066 +07 [26955] DEBUG: un-mmapping temporary code
region
Here, out of 5MB of Postgres text, only 1 huge page can be used, but that
still saves 512 entries in the TLB and might bring a small improvement. The
un-remapped region below 0x600000 contains the ~600kB of "cold" code, since
the linker puts the cold section first, at least recent versions of ld and
lld.
0002 is my attempt to force the linker's hand and get the entire text
segment mapped to huge pages. It's quite a finicky hack, and easily broken
(see below). That said, it still builds easily within our normal build
process, and maybe there is a better way to get the effect.
It does two things:
- Pass the linker -Wl,-zcommon-page-size=2097152
-Wl,-zmax-page-size=2097152 which aligns .init to a 2MB boundary. That's
done for predictability, but that means the next 2MB boundary is very
nearly 2MB away.
- Add a "cold" __asm__ filler function that just takes up space, enough to
push the end of the .text segment over the next aligned boundary, or to
~8MB in size.
In a non-assert build:
0001:
$ bloaty inst-perf/bin/postgres
FILE SIZE VM SIZE
-------------- --------------
53.7% 4.90Mi 58.7% 4.90Mi .text
...
100.0% 9.12Mi 100.0% 8.35Mi TOTAL
$ readelf -S --wide inst-perf/bin/postgres
[Nr] Name Type Address Off Size ES
Flg Lk Inf Al
...
[12] .init PROGBITS 0000000000486000 086000 00001b 00
AX 0 0 4
[13] .plt PROGBITS 0000000000486020 086020 001520 10
AX 0 0 16
[14] .text PROGBITS 0000000000487540 087540 4e59d2 00
AX 0 0 16
...
0002:
$ bloaty inst-perf/bin/postgres
FILE SIZE VM SIZE
-------------- --------------
46.9% 8.00Mi 69.9% 8.00Mi .text
...
100.0% 17.1Mi 100.0% 11.4Mi TOTAL
$ readelf -S --wide inst-perf/bin/postgres
[Nr] Name Type Address Off Size ES
Flg Lk Inf Al
...
[12] .init PROGBITS 0000000000600000 200000 00001b 00
AX 0 0 4
[13] .plt PROGBITS 0000000000600020 200020 001520 10
AX 0 0 16
[14] .text PROGBITS 0000000000601540 201540 7ff512 00
AX 0 0 16
...
Debug messages with 0002 shows 6MB mapped:
2022-11-02 12:35:28.482 +07 [28530] DEBUG: .text start: 0x601540
2022-11-02 12:35:28.482 +07 [28530] DEBUG: .text end: 0xe00a52
2022-11-02 12:35:28.482 +07 [28530] DEBUG: aligned .text start: 0x800000
2022-11-02 12:35:28.482 +07 [28530] DEBUG: aligned .text end: 0xe00000
2022-11-02 12:35:28.486 +07 [28530] DEBUG: binary mapped to huge pages
2022-11-02 12:35:28.486 +07 [28530] DEBUG: un-mmapping temporary code
region
Since the front is all-cold, and there is very little at the end,
practically all hot pages are now remapped. The biggest problem with the
hackish filler function (in addition to maintainability) is, if explicit
huge pages are turned off in the kernel, attempting mmap() with MAP_HUGETLB
causes complete startup failure if the .text segment is larger than 8MB. I
haven't looked into what's happening there yet, but I didn't want to get
too far in the weeds before getting feedback on whether the entire approach
in this thread is sound enough to justify working further on.
[1] https://www.cs.rochester.edu/u/sandhya/papers/ispass19.pdf
(paper: "On the Impact of Instruction Address Translation Overhead")
[2] https://twitter.com/AndresFreundTec/status/1214305610172289024
[3] https://github.com/intel/iodlr
--
John Naylor
EDB: http://www.enterprisedb.com
Attachments:
[application/x-patch] v1-0002-Put-all-non-cold-.text-in-huge-pages.patch (3.1K, ../../CAFBsxsHx9z45MfsAjELFiPv_kcgCcH_P5jNa=WaeGxO7HU3mag@mail.gmail.com/3-v1-0002-Put-all-non-cold-.text-in-huge-pages.patch)
download | inline diff:
From 9cde401f87937c1982f2355c8f81449514166376 Mon Sep 17 00:00:00 2001
From: John Naylor <[email protected]>
Date: Mon, 31 Oct 2022 13:59:30 +0700
Subject: [PATCH v1 2/2] Put all non-cold .text in huge pages
Tell linker to align addresses on 2MB boundaries. The .init
section will be so aligned, with the .text section soon after that.
Therefore, the start address of .text should always be align up to
nearly 2MB ahead of the actual start. The first nearly 2MB of .text
will not map to huge pages.
We count on cold sections linking to the front of the .text segment:
Since the cold sections total about 600kB in size, we need ~1.4MB of
additional padding to keep non-cold pages mappable to huge pages. Since
PG has about 5.0MB of .text, we also need an additional 1MB to push
the .text end just past an aligned boundary, so when we align the end
down, only a small number of pages will remain un-remapped at their
original 4kB size.
---
meson.build | 3 +++
src/backend/port/filler.c | 29 +++++++++++++++++++++++++++++
src/backend/port/meson.build | 3 +++
3 files changed, 35 insertions(+)
create mode 100644 src/backend/port/filler.c
diff --git a/meson.build b/meson.build
index bfacbdc0af..450946370c 100644
--- a/meson.build
+++ b/meson.build
@@ -239,6 +239,9 @@ elif host_system == 'freebsd'
elif host_system == 'linux'
sema_kind = 'unnamed_posix'
cppflags += '-D_GNU_SOURCE'
+ # WIP: debug builds are huge
+ # TODO: add portability check
+ ldflags += ['-Wl,-zcommon-page-size=2097152', '-Wl,-zmax-page-size=2097152']
elif host_system == 'netbsd'
# We must resolve all dynamic linking in the core server at program start.
diff --git a/src/backend/port/filler.c b/src/backend/port/filler.c
new file mode 100644
index 0000000000..de4e33bb05
--- /dev/null
+++ b/src/backend/port/filler.c
@@ -0,0 +1,29 @@
+/*
+ * Add enough padding to .text segment to bring the end just
+ * past a 2MB alignment boundary. In practice, this means .text needs
+ * to be at least 8MB. It shouldn't be much larger than this,
+ * because then more hot pages will remain in 4kB pages.
+ *
+ * FIXME: With this filler added, if explicit huge pages are turned off
+ * in the kernel, attempting mmap() with MAP_HUGETLB causes a crash
+ * instead of reporting failure if the .text segment is larger than 8MB.
+ *
+ * See MapStaticCodeToLargePages() in large_page.c
+ *
+ * XXX: The exact amount of filler must be determined experimentally
+ * on platforms of interest, in non-assert builds.
+ *
+ */
+static void
+__attribute__((used))
+__attribute__((cold))
+fill_function(int x)
+{
+ /* TODO: More architectures */
+#ifdef __x86_64__
+__asm__(
+ ".fill 3251000"
+);
+#endif
+ (void) x;
+}
\ No newline at end of file
diff --git a/src/backend/port/meson.build b/src/backend/port/meson.build
index 5ab65115e9..d876712e0c 100644
--- a/src/backend/port/meson.build
+++ b/src/backend/port/meson.build
@@ -16,6 +16,9 @@ if cdata.has('USE_WIN32_SEMAPHORES')
endif
if cdata.has('USE_SYSV_SHARED_MEMORY')
+ if host_system == 'linux'
+ backend_sources += files('filler.c')
+ endif
backend_sources += files('large_page.c')
backend_sources += files('sysv_shmem.c')
endif
--
2.37.3
[application/x-patch] v1-0001-Partly-remap-the-.text-segment-into-huge-pages-at.patch (12.7K, ../../CAFBsxsHx9z45MfsAjELFiPv_kcgCcH_P5jNa=WaeGxO7HU3mag@mail.gmail.com/4-v1-0001-Partly-remap-the-.text-segment-into-huge-pages-at.patch)
download | inline diff:
From 0012baab70779f5fc06c8717392dc76e8f156270 Mon Sep 17 00:00:00 2001
From: John Naylor <[email protected]>
Date: Mon, 31 Oct 2022 15:24:29 +0700
Subject: [PATCH v1 1/2] Partly remap the .text segment into huge pages at
postmaster start
Based on MIT licensed libary at https://github.com/intel/iodlr
The basic steps are:
- read ELF info to get the start/end addresses of the .text segment
- calculate addresses therein aligned at huge page boundaries
- mmap temporary region and memcpy aligned portion of .text segment
- mmap start address to new region with huge pages and MAP_FIXED
- memcpy over and revoke the PROT_WRITE bit
The Postgres .text segment is ~5.0MB in a non-assert build, so this
method can put 2-4MB into huge pages.
---
src/backend/port/large_page.c | 348 ++++++++++++++++++++++++++++
src/backend/port/meson.build | 1 +
src/backend/postmaster/postmaster.c | 7 +
src/include/port/large_page.h | 18 ++
4 files changed, 374 insertions(+)
create mode 100644 src/backend/port/large_page.c
create mode 100644 src/include/port/large_page.h
diff --git a/src/backend/port/large_page.c b/src/backend/port/large_page.c
new file mode 100644
index 0000000000..66a584f785
--- /dev/null
+++ b/src/backend/port/large_page.c
@@ -0,0 +1,348 @@
+/*-------------------------------------------------------------------------
+ *
+ * large_page.c
+ * Map .text segment of binary to huge pages
+ *
+ * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/backend/port/large_page.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+/*
+ * Based on Intel ioldr library:
+ * https://github.com/intel/iodlr.git
+ * MIT license and copyright notice follows
+ */
+
+/*
+ * Copyright (C) 2018 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom
+ * the Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included
+ * in all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
+ * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
+ * OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE
+ * OR OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * SPDX-License-Identifier: MIT
+ */
+
+#include "postgres.h"
+
+#include <link.h>
+#include <sys/mman.h>
+
+#include "port/large_page.h"
+#include "storage/pg_shmem.h"
+
+typedef struct
+{
+ char *from;
+ char *to;
+} mem_range;
+
+typedef struct
+{
+ uintptr_t start;
+ uintptr_t end;
+ bool found;
+} FindParams;
+
+static inline uintptr_t
+largepage_align_down(uintptr_t addr, size_t hugepagesize)
+{
+ return (addr & ~(hugepagesize - 1));
+}
+
+static inline uintptr_t
+largepage_align_up(uintptr_t addr, size_t hugepagesize)
+{
+ return largepage_align_down(addr + hugepagesize - 1, hugepagesize);
+}
+
+static bool
+FindTextSection(const char *fname, ElfW(Shdr) * text_section)
+{
+ ElfW(Ehdr) ehdr;
+ FILE *bin;
+
+ ElfW(Shdr) * shdrs = NULL;
+ ElfW(Shdr) * sh_strab;
+ char *section_names = NULL;
+
+ bin = fopen(fname, "r");
+ if (bin == NULL)
+ return false;
+
+ /* Read the header. */
+ if (fread(&ehdr, sizeof(ehdr), 1, bin) != 1)
+ return false;;
+
+ /* Read the section headers. */
+ shdrs = (ElfW(Shdr) *) palloc(ehdr.e_shnum * sizeof(ElfW(Shdr)));
+ if (fseek(bin, ehdr.e_shoff, SEEK_SET) != 0)
+ return false;;
+ if (fread(shdrs, sizeof(shdrs[0]), ehdr.e_shnum, bin) != ehdr.e_shnum)
+ return false;;
+
+ /* Read the string table. */
+ sh_strab = &shdrs[ehdr.e_shstrndx];
+ section_names = palloc(sh_strab->sh_size * sizeof(char));
+
+ if (fseek(bin, sh_strab->sh_offset, SEEK_SET) != 0)
+ return false;;
+ if (fread(section_names, sh_strab->sh_size, 1, bin) != 1)
+ return false;;
+
+ /* Find the ".text" section. */
+ for (uint32_t idx = 0; idx < ehdr.e_shnum; idx++)
+ {
+ ElfW(Shdr) * sh = &shdrs[idx];
+ if (!memcmp(§ion_names[sh->sh_name], ".text", 5))
+ {
+ *text_section = *sh;
+ fclose(bin);
+ return true;
+ }
+ }
+ return false;
+}
+
+/* Callback for dl_iterate_phdr to set the start and end of the .text segment */
+static int
+FindMapping(struct dl_phdr_info *hdr, size_t size, void *data)
+{
+ ElfW(Shdr) text_section;
+ FindParams *find_params = (FindParams *) data;
+
+ /*
+ * We are only interested in the mapping matching the main executable.
+ * This has the empty string for a name.
+ */
+ if (hdr->dlpi_name[0] != '\0')
+ return 0;
+
+ /*
+ * Open the info structure for the executable on disk to find the location
+ * of its .text section. We use the base address given to calculate the
+ * .text section offset in memory.
+ */
+ text_section.sh_size = 0;
+#ifdef __linux__
+ if (FindTextSection("/proc/self/exe", &text_section))
+ {
+ find_params->start = hdr->dlpi_addr + text_section.sh_addr;
+ find_params->end = find_params->start + text_section.sh_size;
+ find_params->found = true;
+ return 1;
+ }
+#endif
+ return 0;
+}
+
+/*
+ * Identify and return the text segment in the currently mapped memory region.
+ */
+static bool
+FindTextRegion(mem_range * region)
+{
+ FindParams find_params = {0, 0, false};
+
+ /*
+ * Note: the upstream source worked with shared libraries as well, hence
+ * the iteration over all ojects.
+ */
+ dl_iterate_phdr(FindMapping, &find_params);
+ if (find_params.found)
+ {
+ region->from = (char *) find_params.start;
+ region->to = (char *) find_params.end;
+ }
+
+ return find_params.found;
+}
+
+/*
+ * Move specified region to large pages.
+ *
+ * NB: We need to be very careful:
+ * 1. This function itself should not be moved. We use compiler attributes:
+ * WIP: if these aren't available, the function should do nothing
+ * (__section__) to put it outside the ".text" section
+ * (__noline__) to not inline this function
+ *
+ * 2. This function should not call any function(s) that might be moved.
+ */
+static void
+__attribute__((__section__("lpstub")))
+__attribute__((__noinline__))
+MoveRegionToLargePages(const mem_range * r, int mmap_flags)
+{
+ void *nmem = MAP_FAILED;
+ void *tmem = MAP_FAILED;
+ int ret = 0;
+ int mmap_errno = 0;
+ void *start = r->from;
+ size_t size = r->to - r->from;
+ bool success = false;
+
+ /* Allocate temporary region */
+ nmem = mmap(NULL, size,
+ PROT_READ | PROT_WRITE,
+ MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+ if (nmem == MAP_FAILED)
+ {
+ elog(DEBUG1, "failed to allocate temporary region");
+ return;
+ }
+
+ /* copy the original code */
+ memcpy(nmem, r->from, size);
+
+ /*
+ * mmap using the start address with MAP_FIXED so we get exactly the same
+ * virtual address. We already know the original page is r-xp (PROT_READ,
+ * PROT_EXEC, MAP_PRIVATE) We want PROT_WRITE because we are writing into
+ * it.
+ */
+ Assert(mmap_flags & MAP_HUGETLB);
+ tmem = mmap(start, size,
+ PROT_READ | PROT_WRITE | PROT_EXEC,
+ MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED | mmap_flags,
+ -1, 0);
+ mmap_errno = errno;
+
+ if (tmem == MAP_FAILED && huge_pages == HUGE_PAGES_ON)
+ {
+ /*
+ * WIP: need a way for the user to determine total huge pages needed,
+ * perhaps with shared_memory_size_in_huge_pages
+ */
+ errno = mmap_errno;
+ ereport(FATAL,
+ errmsg("mmap(%zu) with MAP_HUGETLB failed, huge pages disabled: %m", size),
+ (mmap_errno == ENOMEM) ?
+ errhint("This usually means not enough explicit huge pages were "
+ "configured in the kernel") : 0);
+ goto cleanup_tmp;
+ }
+ else if (tmem == MAP_FAILED)
+ {
+ Assert(huge_pages == HUGE_PAGES_TRY);
+
+ errno = mmap_errno;
+ elog(DEBUG1, "mmap(%zu) with MAP_HUGETLB failed, huge pages disabled: %m", size);
+
+ /*
+ * try remapping again with normal pages
+ *
+ * XXX we cannot just back out now
+ */
+ tmem = mmap(start, size,
+ PROT_READ | PROT_WRITE | PROT_EXEC,
+ MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED,
+ -1, 0);
+ mmap_errno = errno;
+
+ if (tmem == MAP_FAILED)
+ {
+ /*
+ * If we get here we cannot start the server. It's unlikely we
+ * will fail here after the postmaster successfully set up shared
+ * memory, but maybe we should have a GUC to turn off code
+ * remapping, hinted here.
+ */
+ errno = mmap_errno;
+ ereport(FATAL,
+ errmsg("mmap(%zu) failed for fallback code region: %m", size));
+ goto cleanup_tmp;
+ }
+ }
+ else
+ success = true;
+
+ /* copy the code to the newly mapped area and unset the write bit */
+ memcpy(start, nmem, size);
+ ret = mprotect(start, size, PROT_READ | PROT_EXEC);
+ if (ret < 0)
+ {
+ /* WIP: see note above about GUC and hint */
+ ereport(FATAL,
+ errmsg("failed to protect remapped code pages"));
+
+ /* Cannot start but at least try to clean up after ourselves */
+ munmap(tmem, size);
+ goto cleanup_tmp;
+ }
+
+ if (success)
+ elog(DEBUG1, "binary mapped to huge pages");
+
+cleanup_tmp:
+ /* Release the old/temporary mapped region */
+ elog(DEBUG3, "un-mmapping temporary code region");
+ ret = munmap(nmem, size);
+ if (ret < 0)
+ /* WIP: not sure of severity here */
+ ereport(LOG,
+ errmsg("failed to unmap temporary region"));
+
+ return;
+}
+
+/* Align the region to to be mapped to huge page boundaries. */
+static void
+AlignRegionToPageBoundary(mem_range * r, size_t hugepagesize)
+{
+ r->from = (char *) largepage_align_up((uintptr_t) r->from, hugepagesize);
+ r->to = (char *) largepage_align_down((uintptr_t) r->to, hugepagesize);
+}
+
+
+/* Map the postgres .text segment into huge pages. */
+void
+MapStaticCodeToLargePages(void)
+{
+ size_t hugepagesize;
+ int mmap_flags;
+ mem_range r = {0};
+
+ if (huge_pages == HUGE_PAGES_OFF)
+ return;
+
+ GetHugePageSize(&hugepagesize, &mmap_flags);
+ if (hugepagesize == 0)
+ return;
+
+ FindTextRegion(&r);
+ if (r.from == NULL || r.to == NULL)
+ return;
+
+ elog(DEBUG3, ".text start: %p", r.from);
+ elog(DEBUG3, ".text end: %p", r.to);
+
+ AlignRegionToPageBoundary(&r, hugepagesize);
+
+ elog(DEBUG3, "aligned .text start: %p", r.from);
+ elog(DEBUG3, "aligned .text end: %p", r.to);
+
+ /* check if aligned map region is large enough for huge pages */
+ if (r.to - r.from < hugepagesize || r.from > r.to)
+ return;
+
+ MoveRegionToLargePages(&r, mmap_flags);
+}
diff --git a/src/backend/port/meson.build b/src/backend/port/meson.build
index a22c25dd95..5ab65115e9 100644
--- a/src/backend/port/meson.build
+++ b/src/backend/port/meson.build
@@ -16,6 +16,7 @@ if cdata.has('USE_WIN32_SEMAPHORES')
endif
if cdata.has('USE_SYSV_SHARED_MEMORY')
+ backend_sources += files('large_page.c')
backend_sources += files('sysv_shmem.c')
endif
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 30fb576ac3..b30769c2b2 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -106,6 +106,7 @@
#include "pg_getopt.h"
#include "pgstat.h"
#include "port/pg_bswap.h"
+#include "port/large_page.h"
#include "postmaster/autovacuum.h"
#include "postmaster/auxprocess.h"
#include "postmaster/bgworker_internals.h"
@@ -1084,6 +1085,12 @@ PostmasterMain(int argc, char *argv[])
*/
CreateSharedMemoryAndSemaphores();
+ /*
+ * If enough huge pages are available after setting up shared memory, try
+ * to map the binary code to huge pages.
+ */
+ MapStaticCodeToLargePages();
+
/*
* Estimate number of openable files. This must happen after setting up
* semaphores, because on some platforms semaphores count as open files.
diff --git a/src/include/port/large_page.h b/src/include/port/large_page.h
new file mode 100644
index 0000000000..171819dd53
--- /dev/null
+++ b/src/include/port/large_page.h
@@ -0,0 +1,18 @@
+/*-------------------------------------------------------------------------
+ *
+ * large_page.h
+ * Map .text segment of binary to huge pages
+ *
+ * Portions Copyright (c) 1996-2022, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/port/large_page.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef LARGE_PAGE_H
+#define LARGE_PAGE_H
+
+extern void MapStaticCodeToLargePages(void);
+
+#endif /* LARGE_PAGE_H */
--
2.37.3
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: remap the .text segment into huge pages at run time
@ 2022-11-03 17:21 Andres Freund <[email protected]>
parent: John Naylor <[email protected]>
0 siblings, 3 replies; 15+ messages in thread
From: Andres Freund @ 2022-11-03 17:21 UTC (permalink / raw)
To: John Naylor <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]>
Hi,
On 2022-11-02 13:32:37 +0700, John Naylor wrote:
> It's been known for a while that Postgres spends a lot of time translating
> instruction addresses, and using huge pages in the text segment yields a
> substantial performance boost in OLTP workloads [1][2].
Indeed. Some of that we eventually should address by making our code less
"jumpy", but that's a large amount of work and only going to go so far.
> The difficulty is,
> this normally requires a lot of painstaking work (unless your OS does
> superpage promotion, like FreeBSD).
I still am confused by FreeBSD being able to do this without changing the
section alignment to be big enough. Or is the default alignment on FreeBSD
large enough already?
> I found an MIT-licensed library "iodlr" from Intel [3] that allows one to
> remap the .text segment to huge pages at program start. Attached is a
> hackish, Meson-only, "works on my machine" patchset to experiment with this
> idea.
I wonder how far we can get with just using the linker hints to align
sections. I know that the linux folks are working on promoting sufficiently
aligned executable pages to huge pages too, and might have succeeded already.
IOW, adding the linker flags might be a good first step.
> 0001 adapts the library to our error logging and GUC system. The overview:
>
> - read ELF info to get the start/end addresses of the .text segment
> - calculate addresses therein aligned at huge page boundaries
> - mmap a temporary region and memcpy the aligned portion of the .text
> segment
> - mmap aligned start address to a second region with huge pages and
> MAP_FIXED
> - memcpy over from the temp region and revoke the PROT_WRITE bit
Would mremap()'ing the temporary region also work? That might be simpler and
more robust (you'd see the MAP_HUGETLB failure before doing anything
irreversible). And you then might not even need this:
> The reason this doesn't "saw off the branch you're standing on" is that the
> remapping is done in a function that's forced to live in a different
> segment, and doesn't call any non-libc functions living elsewhere:
>
> static void
> __attribute__((__section__("lpstub")))
> __attribute__((__noinline__))
> MoveRegionToLargePages(const mem_range * r, int mmap_flags)
This would likely need a bunch more gating than the patch, understandably,
has. I think it'd faily horribly if there were .text relocations, for example?
I think there are some architectures that do that by default...
> 0002 is my attempt to force the linker's hand and get the entire text
> segment mapped to huge pages. It's quite a finicky hack, and easily broken
> (see below). That said, it still builds easily within our normal build
> process, and maybe there is a better way to get the effect.
>
> It does two things:
>
> - Pass the linker -Wl,-zcommon-page-size=2097152
> -Wl,-zmax-page-size=2097152 which aligns .init to a 2MB boundary. That's
> done for predictability, but that means the next 2MB boundary is very
> nearly 2MB away.
Yep. FWIW, my notes say
# align sections to 2MB boundaries for hugepage support
# bfd and gold linkers:
# -Wl,-zmax-page-size=0x200000 -Wl,-zcommon-page-size=0x200000
# lld:
# -Wl,-zmax-page-size=0x200000 -Wl,-z,separate-loadable-segments
# then copy binary to tmpfs mounted with -o huge=always
I.e. with lld you need slightly different flags -Wl,-z,separate-loadable-segments
The meson bit should probably just use
cc.get_supported_link_arguments([
'-Wl,-zmax-page-size=0x200000',
'-Wl,-zcommon-page-size=0x200000',
'-Wl,-zseparate-loadable-segments'])
Afaict there's really no reason to not do that by default, allowing kernels
that can promote to huge pages to do so.
My approach to forcing huge pages to be used was to then:
# copy binary to tmpfs mounted with -o huge=always
> - Add a "cold" __asm__ filler function that just takes up space, enough to
> push the end of the .text segment over the next aligned boundary, or to
> ~8MB in size.
I don't understand why this is needed - as long as the pages are aligned to
2MB, why do we need to fill things up on disk? The in-memory contents are the
relevant bit, no?
> Since the front is all-cold, and there is very little at the end,
> practically all hot pages are now remapped. The biggest problem with the
> hackish filler function (in addition to maintainability) is, if explicit
> huge pages are turned off in the kernel, attempting mmap() with MAP_HUGETLB
> causes complete startup failure if the .text segment is larger than 8MB.
I would expect MAP_HUGETLB to always fail if not enabled in the kernel,
independent of the .text segment size?
> +/* Callback for dl_iterate_phdr to set the start and end of the .text segment */
> +static int
> +FindMapping(struct dl_phdr_info *hdr, size_t size, void *data)
> +{
> + ElfW(Shdr) text_section;
> + FindParams *find_params = (FindParams *) data;
> +
> + /*
> + * We are only interested in the mapping matching the main executable.
> + * This has the empty string for a name.
> + */
> + if (hdr->dlpi_name[0] != '\0')
> + return 0;
> +
It's not entirely clear we'd only ever want to do this for the main
executable. E.g. plpgsql could also benefit.
> diff --git a/meson.build b/meson.build
> index bfacbdc0af..450946370c 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -239,6 +239,9 @@ elif host_system == 'freebsd'
> elif host_system == 'linux'
> sema_kind = 'unnamed_posix'
> cppflags += '-D_GNU_SOURCE'
> + # WIP: debug builds are huge
> + # TODO: add portability check
> + ldflags += ['-Wl,-zcommon-page-size=2097152', '-Wl,-zmax-page-size=2097152']
What's that WIP about?
> elif host_system == 'netbsd'
> # We must resolve all dynamic linking in the core server at program start.
> diff --git a/src/backend/port/filler.c b/src/backend/port/filler.c
> new file mode 100644
> index 0000000000..de4e33bb05
> --- /dev/null
> +++ b/src/backend/port/filler.c
> @@ -0,0 +1,29 @@
> +/*
> + * Add enough padding to .text segment to bring the end just
> + * past a 2MB alignment boundary. In practice, this means .text needs
> + * to be at least 8MB. It shouldn't be much larger than this,
> + * because then more hot pages will remain in 4kB pages.
> + *
> + * FIXME: With this filler added, if explicit huge pages are turned off
> + * in the kernel, attempting mmap() with MAP_HUGETLB causes a crash
> + * instead of reporting failure if the .text segment is larger than 8MB.
> + *
> + * See MapStaticCodeToLargePages() in large_page.c
> + *
> + * XXX: The exact amount of filler must be determined experimentally
> + * on platforms of interest, in non-assert builds.
> + *
> + */
> +static void
> +__attribute__((used))
> +__attribute__((cold))
> +fill_function(int x)
> +{
> + /* TODO: More architectures */
> +#ifdef __x86_64__
> +__asm__(
> + ".fill 3251000"
> +);
> +#endif
> + (void) x;
> +}
> \ No newline at end of file
> diff --git a/src/backend/port/meson.build b/src/backend/port/meson.build
> index 5ab65115e9..d876712e0c 100644
> --- a/src/backend/port/meson.build
> +++ b/src/backend/port/meson.build
> @@ -16,6 +16,9 @@ if cdata.has('USE_WIN32_SEMAPHORES')
> endif
>
> if cdata.has('USE_SYSV_SHARED_MEMORY')
> + if host_system == 'linux'
> + backend_sources += files('filler.c')
> + endif
> backend_sources += files('large_page.c')
> backend_sources += files('sysv_shmem.c')
> endif
> --
> 2.37.3
>
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: remap the .text segment into huge pages at run time
@ 2022-11-04 18:33 Andres Freund <[email protected]>
parent: Andres Freund <[email protected]>
2 siblings, 0 replies; 15+ messages in thread
From: Andres Freund @ 2022-11-04 18:33 UTC (permalink / raw)
To: John Naylor <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]>
Hi,
This nerd-sniped me badly :)
On 2022-11-03 10:21:23 -0700, Andres Freund wrote:
> On 2022-11-02 13:32:37 +0700, John Naylor wrote:
> > I found an MIT-licensed library "iodlr" from Intel [3] that allows one to
> > remap the .text segment to huge pages at program start. Attached is a
> > hackish, Meson-only, "works on my machine" patchset to experiment with this
> > idea.
>
> I wonder how far we can get with just using the linker hints to align
> sections. I know that the linux folks are working on promoting sufficiently
> aligned executable pages to huge pages too, and might have succeeded already.
>
> IOW, adding the linker flags might be a good first step.
Indeed, I did see that that works to some degree on the 5.19 kernel I was
running. However, it never seems to get around to using huge pages
sufficiently to compete with explicit use of huge pages.
More interestingly, a few days ago, a new madvise hint, MADV_COLLAPSE, was
added into linux 6.1. That explicitly remaps a region and uses huge pages for
it. Of course that's going to take a while to be widely available, but it
seems like a safer approach than the remapping approach from this thread.
I hacked in a MADV_COLLAPSE (with setarch -R, so that I could just hardcode
the address / length), and it seems to work nicely.
With the weird caveat that on fs one needs to make sure that the executable
doesn't reflinks to reuse parts of other files, and that the mold linker and
cp do... Not a concern on ext4, but on xfs. I took to copying the postgres
binary with cp --reflink=never
FWIW, you can see the state of the page mapping in more detail with the
kernel's page-types tool
sudo /home/andres/src/kernel/tools/vm/page-types -L -p 12297 -a 0x555555800,0x555556122
sudo /home/andres/src/kernel/tools/vm/page-types -f /srv/dev/build/m-opt/src/backend/postgres2
Perf results:
c=150;psql -f ~/tmp/prewarm.sql;perf stat -a -e cycles,iTLB-loads,iTLB-load-misses,itlb_misses.walk_active,itlb_misses.walk_completed_4k,itlb_misses.walk_completed_2m_4m,itlb_misses.walk_completed_1g pgbench -n -M prepared -S -P1 -c$c -j$c -T10
without MADV_COLLAPSE:
tps = 1038230.070771 (without initial connection time)
Performance counter stats for 'system wide':
1,184,344,476,152 cycles (71.41%)
2,846,146,710 iTLB-loads (71.43%)
2,021,885,782 iTLB-load-misses # 71.04% of all iTLB cache accesses (71.44%)
75,633,850,933 itlb_misses.walk_active (71.44%)
2,020,962,930 itlb_misses.walk_completed_4k (71.44%)
1,213,368 itlb_misses.walk_completed_2m_4m (57.12%)
2,293 itlb_misses.walk_completed_1g (57.11%)
10.064352587 seconds time elapsed
with MADV_COLLAPSE:
tps = 1113717.114278 (without initial connection time)
Performance counter stats for 'system wide':
1,173,049,140,611 cycles (71.42%)
1,059,224,678 iTLB-loads (71.44%)
653,603,712 iTLB-load-misses # 61.71% of all iTLB cache accesses (71.44%)
26,135,902,949 itlb_misses.walk_active (71.44%)
628,314,285 itlb_misses.walk_completed_4k (71.44%)
25,462,916 itlb_misses.walk_completed_2m_4m (57.13%)
2,228 itlb_misses.walk_completed_1g (57.13%)
Note that while the rate of itlb-misses stays roughly the same, the total
number of iTLB loads reduced substantially, and the number of cycles in which
an itlb miss was in progress is 1/3 of what it was before.
A lot of the remaining misses are from the context switches. The iTLB is
flushed on context switches, and of course pgbench -S is extremely context
switch heavy.
Comparing plain -S with 10 pipelined -S transactions (using -t 100000 / -t
10000 to compare the same amount of work) I get:
without MADV_COLLAPSE:
not pipelined:
tps = 1037732.722805 (without initial connection time)
Performance counter stats for 'system wide':
1,691,411,678,007 cycles (62.48%)
8,856,107 itlb.itlb_flush (62.48%)
4,600,041,062 iTLB-loads (62.48%)
2,598,218,236 iTLB-load-misses # 56.48% of all iTLB cache accesses (62.50%)
100,095,862,126 itlb_misses.walk_active (62.53%)
2,595,376,025 itlb_misses.walk_completed_4k (50.02%)
2,558,713 itlb_misses.walk_completed_2m_4m (50.00%)
2,146 itlb_misses.walk_completed_1g (49.98%)
14.582927646 seconds time elapsed
pipelined:
tps = 161947.008995 (without initial connection time)
Performance counter stats for 'system wide':
1,095,948,341,745 cycles (62.46%)
877,556 itlb.itlb_flush (62.46%)
4,576,237,561 iTLB-loads (62.48%)
307,971,166 iTLB-load-misses # 6.73% of all iTLB cache accesses (62.52%)
15,565,279,213 itlb_misses.walk_active (62.55%)
306,240,104 itlb_misses.walk_completed_4k (50.03%)
1,753,560 itlb_misses.walk_completed_2m_4m (50.00%)
2,189 itlb_misses.walk_completed_1g (49.96%)
9.374687885 seconds time elapsed
with MADV_COLLAPSE:
not pipelined:
tps = 1112040.859643 (without initial connection time)
Performance counter stats for 'system wide':
1,569,546,236,696 cycles (62.50%)
7,094,291 itlb.itlb_flush (62.51%)
1,599,845,097 iTLB-loads (62.51%)
692,042,864 iTLB-load-misses # 43.26% of all iTLB cache accesses (62.51%)
31,529,641,124 itlb_misses.walk_active (62.51%)
669,849,177 itlb_misses.walk_completed_4k (49.99%)
22,708,146 itlb_misses.walk_completed_2m_4m (49.99%)
2,752 itlb_misses.walk_completed_1g (49.99%)
13.611206182 seconds time elapsed
pipelined:
tps = 162484.443469 (without initial connection time)
Performance counter stats for 'system wide':
1,092,897,514,658 cycles (62.48%)
942,351 itlb.itlb_flush (62.48%)
233,996,092 iTLB-loads (62.48%)
102,155,575 iTLB-load-misses # 43.66% of all iTLB cache accesses (62.49%)
6,419,597,286 itlb_misses.walk_active (62.52%)
98,758,409 itlb_misses.walk_completed_4k (50.03%)
3,342,332 itlb_misses.walk_completed_2m_4m (50.02%)
2,190 itlb_misses.walk_completed_1g (49.98%)
9.355239897 seconds time elapsed
The difference in itlb.itlb_flush between pipelined / non-pipelined cases
unsurprisingly is stark.
While the pipelined case still sees a good bit reduced itlb traffic, the total
amount of cycles in which a walk is active is just not large enough to matter,
by the looks of it.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: remap the .text segment into huge pages at run time
@ 2022-11-04 21:21 Andres Freund <[email protected]>
parent: Andres Freund <[email protected]>
2 siblings, 0 replies; 15+ messages in thread
From: Andres Freund @ 2022-11-04 21:21 UTC (permalink / raw)
To: John Naylor <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]>
Hi,
On 2022-11-03 10:21:23 -0700, Andres Freund wrote:
> > - Add a "cold" __asm__ filler function that just takes up space, enough to
> > push the end of the .text segment over the next aligned boundary, or to
> > ~8MB in size.
>
> I don't understand why this is needed - as long as the pages are aligned to
> 2MB, why do we need to fill things up on disk? The in-memory contents are the
> relevant bit, no?
I now assume it's because you either observed the mappings set up by the
loader to not include the space between the segments?
With sufficient linker flags the segments are sufficiently aligned both on
disk and in memory to just map more:
bfd: -Wl,-zmax-page-size=0x200000,-zcommon-page-size=0x200000
Type Offset VirtAddr PhysAddr
FileSiz MemSiz Flags Align
...
LOAD 0x0000000000000000 0x0000000000000000 0x0000000000000000
0x00000000000c7f58 0x00000000000c7f58 R 0x200000
LOAD 0x0000000000200000 0x0000000000200000 0x0000000000200000
0x0000000000921d39 0x0000000000921d39 R E 0x200000
LOAD 0x0000000000c00000 0x0000000000c00000 0x0000000000c00000
0x00000000002626b8 0x00000000002626b8 R 0x200000
LOAD 0x0000000000fdf510 0x00000000011df510 0x00000000011df510
0x0000000000037fd6 0x000000000006a310 RW 0x200000
gold -Wl,-zmax-page-size=0x200000,-zcommon-page-size=0x200000,--rosegment
Type Offset VirtAddr PhysAddr
FileSiz MemSiz Flags Align
...
LOAD 0x0000000000000000 0x0000000000000000 0x0000000000000000
0x00000000009230f9 0x00000000009230f9 R E 0x200000
LOAD 0x0000000000a00000 0x0000000000a00000 0x0000000000a00000
0x000000000033a738 0x000000000033a738 R 0x200000
LOAD 0x0000000000ddf4e0 0x0000000000fdf4e0 0x0000000000fdf4e0
0x000000000003800a 0x000000000006a340 RW 0x200000
lld: -Wl,-zmax-page-size=0x200000,-zseparate-loadable-segments
LOAD 0x0000000000000000 0x0000000000000000 0x0000000000000000
0x000000000033710c 0x000000000033710c R 0x200000
LOAD 0x0000000000400000 0x0000000000400000 0x0000000000400000
0x0000000000921cb0 0x0000000000921cb0 R E 0x200000
LOAD 0x0000000000e00000 0x0000000000e00000 0x0000000000e00000
0x0000000000020ae0 0x0000000000020ae0 RW 0x200000
LOAD 0x0000000001000000 0x0000000001000000 0x0000000001000000
0x00000000000174ea 0x0000000000049820 RW 0x200000
mold -Wl,-zmax-page-size=0x200000,-zcommon-page-size=0x200000,-zseparate-loadable-segments
Type Offset VirtAddr PhysAddr
FileSiz MemSiz Flags Align
...
LOAD 0x0000000000000000 0x0000000000000000 0x0000000000000000
0x000000000032dde9 0x000000000032dde9 R 0x200000
LOAD 0x0000000000400000 0x0000000000400000 0x0000000000400000
0x0000000000921cbe 0x0000000000921cbe R E 0x200000
LOAD 0x0000000000e00000 0x0000000000e00000 0x0000000000e00000
0x00000000002174e8 0x0000000000249820 RW 0x200000
With these flags the "R E" segments all start on a 0x200000/2MiB boundary and
are padded to the next 2MiB boundary. However the OS / dynamic loader only
maps the necessary part, not all the zero padding.
This means that if we were to issue a MADV_COLLAPSE, we can before it do an
mremap() to increase the length of the mapping.
MADV_COLLAPSE without mremap:
tps = 1117335.766756 (without initial connection time)
Performance counter stats for 'system wide':
1,169,012,466,070 cycles (55.53%)
729,146,640,019 instructions # 0.62 insn per cycle (66.65%)
7,062,923 itlb.itlb_flush (66.65%)
1,041,825,587 iTLB-loads (66.65%)
634,272,420 iTLB-load-misses # 60.88% of all iTLB cache accesses (66.66%)
27,018,254,873 itlb_misses.walk_active (66.68%)
610,639,252 itlb_misses.walk_completed_4k (44.47%)
24,262,549 itlb_misses.walk_completed_2m_4m (44.46%)
2,948 itlb_misses.walk_completed_1g (44.43%)
10.039217004 seconds time elapsed
MADV_COLLAPSE with mremap:
tps = 1140869.853616 (without initial connection time)
Performance counter stats for 'system wide':
1,173,272,878,934 cycles (55.53%)
746,008,850,147 instructions # 0.64 insn per cycle (66.65%)
7,538,962 itlb.itlb_flush (66.65%)
799,861,088 iTLB-loads (66.65%)
254,347,048 iTLB-load-misses # 31.80% of all iTLB cache accesses (66.66%)
14,427,296,885 itlb_misses.walk_active (66.69%)
221,811,835 itlb_misses.walk_completed_4k (44.47%)
32,881,405 itlb_misses.walk_completed_2m_4m (44.46%)
3,043 itlb_misses.walk_completed_1g (44.43%)
10.038517778 seconds time elapsed
compared to a run without any huge pages (via THP or MADV_COLLAPSE):
tps = 1034960.102843 (without initial connection time)
Performance counter stats for 'system wide':
1,183,743,785,066 cycles (55.54%)
678,525,810,443 instructions # 0.57 insn per cycle (66.65%)
7,163,304 itlb.itlb_flush (66.65%)
2,952,660,798 iTLB-loads (66.65%)
2,105,431,590 iTLB-load-misses # 71.31% of all iTLB cache accesses (66.66%)
80,593,535,910 itlb_misses.walk_active (66.68%)
2,105,377,810 itlb_misses.walk_completed_4k (44.46%)
1,254,156 itlb_misses.walk_completed_2m_4m (44.46%)
3,366 itlb_misses.walk_completed_1g (44.44%)
10.039821650 seconds time elapsed
So a 7.96% win from no-huge-pages to MADV_COLLAPSE and a further 2.11% win
from there to also using mremap(), yielding a total of 10.23%. It's similar
across runs.
On my system the other libraries unfortunately aren't aligned properly. It'd
be nice to also remap at least libc. The majority of the remaining misses are
from the vdso (too small for a huge page), libc (not aligned properly),
returning from system calls (which flush the itlb) and pgbench / libpq (I
didn't add the mremap there, there's not enough code for a huge page without
it).
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: remap the .text segment into huge pages at run time
@ 2022-11-05 05:54 John Naylor <[email protected]>
parent: Andres Freund <[email protected]>
2 siblings, 1 reply; 15+ messages in thread
From: John Naylor @ 2022-11-05 05:54 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]>
On Sat, Nov 5, 2022 at 1:33 AM Andres Freund <[email protected]> wrote:
> > I wonder how far we can get with just using the linker hints to align
> > sections. I know that the linux folks are working on promoting
sufficiently
> > aligned executable pages to huge pages too, and might have succeeded
already.
> >
> > IOW, adding the linker flags might be a good first step.
>
> Indeed, I did see that that works to some degree on the 5.19 kernel I was
> running. However, it never seems to get around to using huge pages
> sufficiently to compete with explicit use of huge pages.
Oh nice, I didn't know that! There might be some threshold of pages mapped
before it does so. At least, that issue is mentioned in that paper linked
upthread for FreeBSD.
> More interestingly, a few days ago, a new madvise hint, MADV_COLLAPSE, was
> added into linux 6.1. That explicitly remaps a region and uses huge pages
for
> it. Of course that's going to take a while to be widely available, but it
> seems like a safer approach than the remapping approach from this thread.
I didn't know that either, funny timing.
> I hacked in a MADV_COLLAPSE (with setarch -R, so that I could just
hardcode
> the address / length), and it seems to work nicely.
>
> With the weird caveat that on fs one needs to make sure that the
executable
> doesn't reflinks to reuse parts of other files, and that the mold linker
and
> cp do... Not a concern on ext4, but on xfs. I took to copying the postgres
> binary with cp --reflink=never
What happens otherwise? That sounds like a difficult thing to guard against.
> The difference in itlb.itlb_flush between pipelined / non-pipelined cases
> unsurprisingly is stark.
>
> While the pipelined case still sees a good bit reduced itlb traffic, the
total
> amount of cycles in which a walk is active is just not large enough to
matter,
> by the looks of it.
Good to know, thanks for testing. Maybe the pipelined case is something
devs should consider when microbenchmarking, to reduce noise from context
switches.
On Sat, Nov 5, 2022 at 4:21 AM Andres Freund <[email protected]> wrote:
>
> Hi,
>
> On 2022-11-03 10:21:23 -0700, Andres Freund wrote:
> > > - Add a "cold" __asm__ filler function that just takes up space,
enough to
> > > push the end of the .text segment over the next aligned boundary, or
to
> > > ~8MB in size.
> >
> > I don't understand why this is needed - as long as the pages are
aligned to
> > 2MB, why do we need to fill things up on disk? The in-memory contents
are the
> > relevant bit, no?
>
> I now assume it's because you either observed the mappings set up by the
> loader to not include the space between the segments?
My knowledge is not quite that deep. The iodlr repo has an example "hello
world" program, which links with 8 filler objects, each with 32768
__attribute((used)) dummy functions. I just cargo-culted that idea and
simplified it. Interestingly enough, looking through the commit history,
they used to align the segments via linker flags, but took it out here:
https://github.com/intel/iodlr/pull/25#discussion_r397787559
...saying "I'm not sure why we added this". :/
I quickly tried to align the segments with the linker and then in my patch
have the address for mmap() rounded *down* from the .text start to the
beginning of that segment. It refused to start without logging an error.
BTW, that what I meant before, although I wasn't clear:
> > Since the front is all-cold, and there is very little at the end,
> > practically all hot pages are now remapped. The biggest problem with the
> > hackish filler function (in addition to maintainability) is, if explicit
> > huge pages are turned off in the kernel, attempting mmap() with
MAP_HUGETLB
> > causes complete startup failure if the .text segment is larger than 8MB.
>
> I would expect MAP_HUGETLB to always fail if not enabled in the kernel,
> independent of the .text segment size?
With the file-level hack, it would just fail without a trace with .text >
8MB (I have yet to enable core dumps on this new OS I have...), whereas
without it I did see the failures in the log, and successful fallback.
> With these flags the "R E" segments all start on a 0x200000/2MiB boundary
and
> are padded to the next 2MiB boundary. However the OS / dynamic loader only
> maps the necessary part, not all the zero padding.
>
> This means that if we were to issue a MADV_COLLAPSE, we can before it do
an
> mremap() to increase the length of the mapping.
I see, interesting. What location are you passing for madvise() and
mremap()? The beginning of the segment (for me has .init/.plt) or an
aligned boundary within .text?
--
John Naylor
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: remap the .text segment into huge pages at run time
@ 2022-11-05 08:27 Andres Freund <[email protected]>
parent: John Naylor <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Andres Freund @ 2022-11-05 08:27 UTC (permalink / raw)
To: John Naylor <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]>
Hi,
On 2022-11-05 12:54:18 +0700, John Naylor wrote:
> On Sat, Nov 5, 2022 at 1:33 AM Andres Freund <[email protected]> wrote:
> > I hacked in a MADV_COLLAPSE (with setarch -R, so that I could just
> hardcode
> > the address / length), and it seems to work nicely.
> >
> > With the weird caveat that on fs one needs to make sure that the
> executable
> > doesn't reflinks to reuse parts of other files, and that the mold linker
> and
> > cp do... Not a concern on ext4, but on xfs. I took to copying the postgres
> > binary with cp --reflink=never
>
> What happens otherwise? That sounds like a difficult thing to guard against.
MADV_COLLAPSE fails, but otherwise things continue on. I think it's mostly an
issue on dev systems, not on prod systems, because there the files will be be
unpacked from a package or such.
> > On 2022-11-03 10:21:23 -0700, Andres Freund wrote:
> > > > - Add a "cold" __asm__ filler function that just takes up space,
> enough to
> > > > push the end of the .text segment over the next aligned boundary, or
> to
> > > > ~8MB in size.
> > >
> > > I don't understand why this is needed - as long as the pages are
> aligned to
> > > 2MB, why do we need to fill things up on disk? The in-memory contents
> are the
> > > relevant bit, no?
> >
> > I now assume it's because you either observed the mappings set up by the
> > loader to not include the space between the segments?
>
> My knowledge is not quite that deep. The iodlr repo has an example "hello
> world" program, which links with 8 filler objects, each with 32768
> __attribute((used)) dummy functions. I just cargo-culted that idea and
> simplified it. Interestingly enough, looking through the commit history,
> they used to align the segments via linker flags, but took it out here:
>
> https://github.com/intel/iodlr/pull/25#discussion_r397787559
>
> ...saying "I'm not sure why we added this". :/
That was about using a linker script, not really linker flags though.
I don't think the dummy functions are a good approach, there were plenty
things after it when I played with them.
> I quickly tried to align the segments with the linker and then in my patch
> have the address for mmap() rounded *down* from the .text start to the
> beginning of that segment. It refused to start without logging an error.
Hm, what linker was that? I did note that you need some additional flags for
some of the linkers.
> > With these flags the "R E" segments all start on a 0x200000/2MiB boundary
> and
> > are padded to the next 2MiB boundary. However the OS / dynamic loader only
> > maps the necessary part, not all the zero padding.
> >
> > This means that if we were to issue a MADV_COLLAPSE, we can before it do
> an
> > mremap() to increase the length of the mapping.
>
> I see, interesting. What location are you passing for madvise() and
> mremap()? The beginning of the segment (for me has .init/.plt) or an
> aligned boundary within .text?
I started postgres with setarch -R, looked at /proc/$pid/[s]maps to see the
start/end of the r-xp mapped segment. Here's my hacky code, with a bunch of
comments added.
void *addr = (void*) 0x555555800000;
void *end = (void *) 0x555555e09000;
size_t advlen = (uintptr_t) end - (uintptr_t) addr;
const size_t bound = 1024*1024*2 - 1;
size_t advlen_up = (advlen + bound - 1) & ~(bound - 1);
void *r2;
/*
* Increase size of mapping to cover the tailing padding to the next
* segment. Otherwise all the code in that range can't be put into
* a huge page (access in the non-mapped range needs to cause a fault,
* hence can't be in the huge page).
* XXX: Should proably assert that that space is actually zeroes.
*/
r2 = mremap(addr, advlen, advlen_up, 0);
if (r2 == MAP_FAILED)
fprintf(stderr, "mremap failed: %m\n");
else if (r2 != addr)
fprintf(stderr, "mremap wrong addr: %m\n");
else
advlen = advlen_up;
/*
* The docs for MADV_COLLAPSE say there should be at least one page
* in the mapped space "for every eligible hugepage-aligned/sized
* region to be collapsed". I just forced that. But probably not
* necessary.
*/
r = madvise(addr, advlen, MADV_WILLNEED);
if (r != 0)
fprintf(stderr, "MADV_WILLNEED failed: %m\n");
r = madvise(addr, advlen, MADV_POPULATE_READ);
if (r != 0)
fprintf(stderr, "MADV_POPULATE_READ failed: %m\n");
/*
* Make huge pages out of it. Requires at least linux 6.1. We could
* fall back to MADV_HUGEPAGE if it fails, but it doesn't do all that
* much in older kernels.
*/
#define MADV_COLLAPSE 25
r = madvise(addr, advlen, MADV_COLLAPSE);
if (r != 0)
fprintf(stderr, "MADV_COLLAPSE failed: %m\n");
A real version would have to open /proc/self/maps and do this for at least
postgres' r-xp mapping. We could do it for libraries too, if they're suitably
aligned (both in memory and on-disk).
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: remap the .text segment into huge pages at run time
@ 2022-11-06 06:56 John Naylor <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: John Naylor @ 2022-11-06 06:56 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]>
On Sat, Nov 5, 2022 at 3:27 PM Andres Freund <[email protected]> wrote:
> > simplified it. Interestingly enough, looking through the commit history,
> > they used to align the segments via linker flags, but took it out here:
> >
> > https://github.com/intel/iodlr/pull/25#discussion_r397787559
> >
> > ...saying "I'm not sure why we added this". :/
>
> That was about using a linker script, not really linker flags though.
Oops, the commit I was referring to pointed to that discussion, but I
should have shown it instead:
--- a/large_page-c/example/Makefile
+++ b/large_page-c/example/Makefile
@@ -28,7 +28,6 @@ OBJFILES= \
filler16.o \
OBJS=$(addprefix $(OBJDIR)/,$(OBJFILES))
-LDFLAGS=-Wl,-z,max-page-size=2097152
But from what you're saying, this flag wouldn't have been enough anyway...
> I don't think the dummy functions are a good approach, there were plenty
> things after it when I played with them.
To be technical, the point wasn't to have no code after it, but to have no
*hot* code *before* it, since with the iodlr approach the first 1.99MB of
.text is below the first aligned boundary within that section. But yeah,
I'm happy to ditch that hack entirely.
> > > With these flags the "R E" segments all start on a 0x200000/2MiB
boundary
> > and
> > > are padded to the next 2MiB boundary. However the OS / dynamic loader
only
> > > maps the necessary part, not all the zero padding.
> > >
> > > This means that if we were to issue a MADV_COLLAPSE, we can before it
do
> > an
> > > mremap() to increase the length of the mapping.
> >
> > I see, interesting. What location are you passing for madvise() and
> > mremap()? The beginning of the segment (for me has .init/.plt) or an
> > aligned boundary within .text?
> /*
> * Make huge pages out of it. Requires at least linux 6.1. We
could
> * fall back to MADV_HUGEPAGE if it fails, but it doesn't do all
that
> * much in older kernels.
> */
About madvise(), I take it MADV_HUGEPAGE and MADV_COLLAPSE only work for
THP? The man page seems to indicate that.
In the support work I've done, the standard recommendation is to turn THP
off, especially if they report sudden performance problems. If explicit
HP's are used for shared mem, maybe THP is less of a risk? I need to look
back at the tests that led to that advice...
> A real version would have to open /proc/self/maps and do this for at least
I can try and generalize your above sketch into a v2 patch.
> postgres' r-xp mapping. We could do it for libraries too, if they're
suitably
> aligned (both in memory and on-disk).
It looks like plpgsql is only 27 standard pages in size...
Regarding glibc, we could try moving a couple of the hotter functions into
PG, using smaller and simpler coding, if that has better frontend cache
behavior. The paper "Understanding and Mitigating Front-End Stalls in
Warehouse-Scale Computers" talks about this, particularly section 4.4
regarding memcmp().
> > I quickly tried to align the segments with the linker and then in my
patch
> > have the address for mmap() rounded *down* from the .text start to the
> > beginning of that segment. It refused to start without logging an error.
>
> Hm, what linker was that? I did note that you need some additional flags
for
> some of the linkers.
BFD, but I wouldn't worry about that failure too much, since the
mremap()/madvise() strategy has a lot fewer moving parts.
On the subject of linkers, though, one thing that tripped me up was trying
to change the linker with Meson. First I tried
-Dc_args='-fuse-ld=lld'
but that led to warnings like this when :
/usr/bin/ld: warning: -z separate-loadable-segments ignored
When using this in the top level meson.build
elif host_system == 'linux'
sema_kind = 'unnamed_posix'
cppflags += '-D_GNU_SOURCE'
# Align the loadable segments to 2MB boundaries to support remapping to
# huge pages.
ldflags += cc.get_supported_link_arguments([
'-Wl,-zmax-page-size=0x200000',
'-Wl,-zcommon-page-size=0x200000',
'-Wl,-zseparate-loadable-segments'
])
According to
https://mesonbuild.com/howtox.html#set-linker
I need to add CC_LD=lld to the env vars before invoking, which got rid of
the warning. Then I wanted to verify that lld was actually used, and in
https://releases.llvm.org/14.0.0/tools/lld/docs/index.html
it says I can run this and it should show “Linker: LLD”, but that doesn't
appear for me:
$ readelf --string-dump .comment inst-perf/bin/postgres
String dump of section '.comment':
[ 0] GCC: (GNU) 12.2.1 20220819 (Red Hat 12.2.1-2)
--
John Naylor
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: remap the .text segment into huge pages at run time
@ 2022-11-06 18:16 Andres Freund <[email protected]>
parent: John Naylor <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Andres Freund @ 2022-11-06 18:16 UTC (permalink / raw)
To: John Naylor <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]>
Hi,
On 2022-11-06 13:56:10 +0700, John Naylor wrote:
> On Sat, Nov 5, 2022 at 3:27 PM Andres Freund <[email protected]> wrote:
> > I don't think the dummy functions are a good approach, there were plenty
> > things after it when I played with them.
>
> To be technical, the point wasn't to have no code after it, but to have no
> *hot* code *before* it, since with the iodlr approach the first 1.99MB of
> .text is below the first aligned boundary within that section. But yeah,
> I'm happy to ditch that hack entirely.
Just because code is colder than the alternative branch, doesn't necessary
mean it's entirely cold overall. I saw hits to things after the dummy function
to have a perf effect.
> > > > With these flags the "R E" segments all start on a 0x200000/2MiB
> boundary
> > > and
> > > > are padded to the next 2MiB boundary. However the OS / dynamic loader
> only
> > > > maps the necessary part, not all the zero padding.
> > > >
> > > > This means that if we were to issue a MADV_COLLAPSE, we can before it
> do
> > > an
> > > > mremap() to increase the length of the mapping.
> > >
> > > I see, interesting. What location are you passing for madvise() and
> > > mremap()? The beginning of the segment (for me has .init/.plt) or an
> > > aligned boundary within .text?
>
> > /*
> > * Make huge pages out of it. Requires at least linux 6.1. We
> could
> > * fall back to MADV_HUGEPAGE if it fails, but it doesn't do all
> that
> > * much in older kernels.
> > */
>
> About madvise(), I take it MADV_HUGEPAGE and MADV_COLLAPSE only work for
> THP? The man page seems to indicate that.
MADV_HUGEPAGE works as long as /sys/kernel/mm/transparent_hugepage/enabled is
to always or madvise. My understanding is that MADV_COLLAPSE will work even
if /sys/kernel/mm/transparent_hugepage/enabled is set to never.
> In the support work I've done, the standard recommendation is to turn THP
> off, especially if they report sudden performance problems.
I think that's pretty much an outdated suggestion FWIW. Largely caused by Red
Hat extremely aggressively backpatching transparent hugepages into RHEL 6
(IIRC). Lots of improvements have been made to THP since then. I've tried to
see negative effects maybe 2-3 years back, without success.
I really don't see a reason to ever set
/sys/kernel/mm/transparent_hugepage/enabled to 'never', rather than just 'madvise'.
> If explicit HP's are used for shared mem, maybe THP is less of a risk? I
> need to look back at the tests that led to that advice...
I wouldn't give that advice to customers anymore, unless they use extremely
old platforms or unless there's very concrete evidence.
> > A real version would have to open /proc/self/maps and do this for at least
>
> I can try and generalize your above sketch into a v2 patch.
Cool.
> > postgres' r-xp mapping. We could do it for libraries too, if they're
> suitably
> > aligned (both in memory and on-disk).
>
> It looks like plpgsql is only 27 standard pages in size...
>
> Regarding glibc, we could try moving a couple of the hotter functions into
> PG, using smaller and simpler coding, if that has better frontend cache
> behavior. The paper "Understanding and Mitigating Front-End Stalls in
> Warehouse-Scale Computers" talks about this, particularly section 4.4
> regarding memcmp().
I think the amount of work necessary for that is nontrivial and continual. So
I'm loathe to go there.
> > > I quickly tried to align the segments with the linker and then in my
> patch
> > > have the address for mmap() rounded *down* from the .text start to the
> > > beginning of that segment. It refused to start without logging an error.
> >
> > Hm, what linker was that? I did note that you need some additional flags
> for
> > some of the linkers.
>
> BFD, but I wouldn't worry about that failure too much, since the
> mremap()/madvise() strategy has a lot fewer moving parts.
>
> On the subject of linkers, though, one thing that tripped me up was trying
> to change the linker with Meson. First I tried
>
> -Dc_args='-fuse-ld=lld'
It's -Dc_link_args=...
> but that led to warnings like this when :
> /usr/bin/ld: warning: -z separate-loadable-segments ignored
>
> When using this in the top level meson.build
>
> elif host_system == 'linux'
> sema_kind = 'unnamed_posix'
> cppflags += '-D_GNU_SOURCE'
> # Align the loadable segments to 2MB boundaries to support remapping to
> # huge pages.
> ldflags += cc.get_supported_link_arguments([
> '-Wl,-zmax-page-size=0x200000',
> '-Wl,-zcommon-page-size=0x200000',
> '-Wl,-zseparate-loadable-segments'
> ])
>
>
> According to
>
> https://mesonbuild.com/howtox.html#set-linker
>
> I need to add CC_LD=lld to the env vars before invoking, which got rid of
> the warning. Then I wanted to verify that lld was actually used, and in
>
> https://releases.llvm.org/14.0.0/tools/lld/docs/index.html
You can just look at build.ninja, fwiw. Or use ninja -v (in postgres's cases
with -d keeprsp, because the commandline ends up being long enough for a
response file being used).
> it says I can run this and it should show “Linker: LLD”, but that doesn't
> appear for me:
>
> $ readelf --string-dump .comment inst-perf/bin/postgres
>
> String dump of section '.comment':
> [ 0] GCC: (GNU) 12.2.1 20220819 (Red Hat 12.2.1-2)
That's added by the compiler, not the linker. See e.g.:
$ readelf --string-dump .comment src/backend/postgres_lib.a.p/storage_ipc_procarray.c.o
String dump of section '.comment':
[ 1] GCC: (Debian 12.2.0-9) 12.2.0
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 15+ messages in thread
* [PATCH v1 2/7] Row pattern recognition patch (parse/analysis).
@ 2023-06-25 11:48 Tatsuo Ishii <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Tatsuo Ishii @ 2023-06-25 11:48 UTC (permalink / raw)
---
src/backend/parser/parse_agg.c | 7 ++
src/backend/parser/parse_clause.c | 196 +++++++++++++++++++++++++++++-
src/backend/parser/parse_expr.c | 4 +
src/backend/parser/parse_func.c | 3 +
4 files changed, 209 insertions(+), 1 deletion(-)
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 85cd47b7ae..aa7a1cee80 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -564,6 +564,10 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
errkind = true;
break;
+ case EXPR_KIND_RPR_DEFINE:
+ errkind = true;
+ break;
+
/*
* There is intentionally no default: case here, so that the
* compiler will warn if we add a new ParseExprKind without
@@ -953,6 +957,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
case EXPR_KIND_CYCLE_MARK:
errkind = true;
break;
+ case EXPR_KIND_RPR_DEFINE:
+ errkind = true;
+ break;
/*
* There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index f61f794755..5bb11add3c 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -100,7 +100,10 @@ static WindowClause *findWindowClause(List *wclist, const char *name);
static Node *transformFrameOffset(ParseState *pstate, int frameOptions,
Oid rangeopfamily, Oid rangeopcintype, Oid *inRangeFunc,
Node *clause);
-
+static void transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef);
+static List *transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef);
+static List *transformPatternClause(ParseState *pstate, WindowClause *wc, WindowDef *windef);
+static List *transformMeasureClause(ParseState *pstate, WindowClause *wc, WindowDef *windef);
/*
* transformFromClause -
@@ -2949,6 +2952,10 @@ transformWindowDefinitions(ParseState *pstate,
rangeopfamily, rangeopcintype,
&wc->endInRangeFunc,
windef->endOffset);
+
+ /* Process Row Pattern Recognition related clauses */
+ transformRPR(pstate, wc, windef);
+
wc->runCondition = NIL;
wc->winref = winref;
@@ -3814,3 +3821,190 @@ transformFrameOffset(ParseState *pstate, int frameOptions,
return node;
}
+
+/*
+ * transformRPR
+ * Process Row Pattern Recognition related clauses
+ */
+static void
+transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef)
+{
+ /* Check Frame option. Frame must start at current row */
+
+ /*
+ * Window definition exists?
+ */
+ if (windef == NULL)
+ return;
+
+ /*
+ * Row Pattern Common Syntax clause exists?
+ */
+ if (windef->rpCommonSyntax == NULL)
+ return;
+
+ if ((wc->frameOptions & FRAMEOPTION_START_CURRENT_ROW) == 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("FRAME must start at current row when row patttern recognition is used")));
+
+ /* Transform AFTER MACH SKIP TO clause */
+ wc->rpSkipTo = windef->rpCommonSyntax->rpSkipTo;
+ if (wc->rpSkipTo != ST_NEXT_ROW)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("only AFTER MATCH SKIP TO NEXT_ROW is supported")));
+
+ /* Transform SEEK or INITIAL clause */
+ wc->initial = windef->rpCommonSyntax->initial;
+
+ /* Transform DEFINE clause into list of TargetEntry's */
+ wc->defineClause = transformDefineClause(pstate, wc, windef);
+
+ /* Check PATTERN clause and copy to patternClause */
+ transformPatternClause(pstate, wc, windef);
+
+ /* Transform MEASURE clause */
+ transformMeasureClause(pstate, wc, windef);
+}
+
+/*
+ * transformDefineClause Process DEFINE clause and transform ResTarget into
+ * list of TargetEntry.
+ *
+ * XXX we only support column reference in row pattern definition search
+ * condition, e.g. "price". <row pattern definition variable name>.<column
+ * reference> is not supported, e.g. "A.price".
+ */
+static List *
+transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef)
+{
+ ListCell *lc;
+ ResTarget *restarget, *r;
+ List *restargets;
+
+
+ /*
+ * If Row Definition Common Syntax exists, DEFINE clause must exist.
+ * (the raw parser should have already checked it.)
+ */
+ Assert(windef->rpCommonSyntax->rpDefs != NULL);
+
+ /*
+ * Check for duplicate row pattern definition variables. The standard
+ * requires that no two row pattern definition variable names shall be
+ * equivalent.
+ */
+ restargets = NIL;
+ foreach(lc, windef->rpCommonSyntax->rpDefs)
+ {
+ char *name;
+ ListCell *l;
+
+ restarget = (ResTarget *)lfirst(lc);
+ name = restarget->name;
+
+ /*
+ * Make sure that row pattern definition search condition is a boolean
+ * expression.
+ */
+ transformWhereClause(pstate, restarget->val,
+ EXPR_KIND_RPR_DEFINE, "DEFINE");
+
+ foreach(l, restargets)
+ {
+ char *n;
+
+ r = (ResTarget *) lfirst(l);
+ n = r->name;
+
+ if (!strcmp(n, name))
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("row pattern definition variable name \"%s\" appears more than once in DEFINE clause",
+ name),
+ parser_errposition(pstate, exprLocation((Node *)r))));
+ }
+ restargets = lappend(restargets, restarget);
+ }
+ list_free(restargets);
+
+ return transformTargetList(pstate, windef->rpCommonSyntax->rpDefs,
+ EXPR_KIND_RPR_DEFINE);
+}
+
+/*
+ * transformPatternClause
+ * Process PATTERN clause and return PATTERN clause in the raw parse tree
+ */
+static List *
+transformPatternClause(ParseState *pstate, WindowClause *wc, WindowDef *windef)
+{
+ List *patterns;
+ ListCell *lc, *l;
+
+ /*
+ * Row Pattern Common Syntax clause exists?
+ */
+ if (windef->rpCommonSyntax == NULL)
+ return NULL;
+
+ /*
+ * Primary row pattern variable names in PATTERN clause must appear in
+ * DEFINE clause as row pattern definition variable names.
+ */
+ wc->patternVariable = NIL;
+ wc->patternRegexp = NIL;
+ foreach(lc, windef->rpCommonSyntax->rpPatterns)
+ {
+ A_Expr *a;
+ char *name;
+ char *regexp;
+ bool found = false;
+
+ if (!IsA(lfirst(lc), A_Expr))
+ ereport(ERROR,
+ errmsg("node type is not A_Expr"));
+
+ a = (A_Expr *)lfirst(lc);
+ name = strVal(a->lexpr);
+
+ foreach(l, windef->rpCommonSyntax->rpDefs)
+ {
+ ResTarget *restarget = (ResTarget *)lfirst(l);
+
+ if (!strcmp(restarget->name, name))
+ {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("primary row pattern variable name \"%s\" does not appear in DEFINE clause",
+ name),
+ parser_errposition(pstate, exprLocation((Node *)a))));
+ wc->patternVariable = lappend(wc->patternVariable, makeString(pstrdup(name)));
+ regexp = strVal(lfirst(list_head(a->name)));
+ wc->patternRegexp = lappend(wc->patternRegexp, makeString(pstrdup(regexp)));
+ }
+ return patterns;
+}
+
+/*
+ * transformMeasureClause
+ * Process MEASURE clause
+ */
+static List *
+transformMeasureClause(ParseState *pstate, WindowClause *wc, WindowDef *windef)
+{
+ if (windef->rowPatternMeasures == NIL)
+ return NIL;
+
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("%s","MEASURE clause is not supported yet"),
+ parser_errposition(pstate, exprLocation((Node *)windef->rowPatternMeasures))));
+}
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 346fd272b6..20231d9ec0 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -541,6 +541,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
case EXPR_KIND_COPY_WHERE:
case EXPR_KIND_GENERATED_COLUMN:
case EXPR_KIND_CYCLE_MARK:
+ case EXPR_KIND_RPR_DEFINE:
/* okay */
break;
@@ -1754,6 +1755,7 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
case EXPR_KIND_VALUES:
case EXPR_KIND_VALUES_SINGLE:
case EXPR_KIND_CYCLE_MARK:
+ case EXPR_KIND_RPR_DEFINE:
/* okay */
break;
case EXPR_KIND_CHECK_CONSTRAINT:
@@ -3133,6 +3135,8 @@ ParseExprKindName(ParseExprKind exprKind)
return "GENERATED AS";
case EXPR_KIND_CYCLE_MARK:
return "CYCLE";
+ case EXPR_KIND_RPR_DEFINE:
+ return "DEFINE";
/*
* There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index b3f0b6a137..2ff3699538 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2656,6 +2656,9 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location)
case EXPR_KIND_CYCLE_MARK:
errkind = true;
break;
+ case EXPR_KIND_RPR_DEFINE:
+ errkind = true;
+ break;
/*
* There is intentionally no default: case here, so that the
--
2.25.1
----Next_Part(Sun_Jun_25_21_05_09_2023_126)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v1-0003-Row-pattern-recognition-patch-planner.patch"
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [18] Policy on IMMUTABLE functions and Unicode updates
@ 2024-07-24 19:12 Jeff Davis <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Jeff Davis @ 2024-07-24 19:12 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Laurenz Albe <[email protected]>; pgsql-hackers; Daniel Verite <[email protected]>; Noah Misch <[email protected]>; Tom Lane <[email protected]>; Jeremy Schneider <[email protected]>
On Wed, 2024-07-24 at 14:47 -0400, Robert Haas wrote:
> On Wed, Jul 24, 2024 at 1:45 PM Jeff Davis <[email protected]> wrote:
> > There's a qualitative difference between a collation update which
> > can
> > break your PKs and FKs, and a ctype update which definitely will
> > not.
>
> I don't think that's true. All you need is a unique index on
> UPPER(somecol).
Primary keys are on plain column references, not expressions; and don't
support WHERE clauses, so I don't see how a ctype update would affect a
PK.
In any case, you are correct that Unicode updates could put some
constraints at risk, including unique indexes, CHECK, and partition
constraints. But someone has to actually use one of the affected
functions somewhere, and that's the main distinction that I'm trying to
draw.
The reason why collation is qualitatively a much bigger problem is
because there's no obvious indication that you are doing anything
related to collation at all. A very plain "CREATE TABLE x(t text
PRIMARY KEY)" is at risk.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [18] Policy on IMMUTABLE functions and Unicode updates
@ 2024-07-24 19:19 Robert Haas <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Robert Haas @ 2024-07-24 19:19 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Laurenz Albe <[email protected]>; pgsql-hackers; Daniel Verite <[email protected]>; Noah Misch <[email protected]>; Tom Lane <[email protected]>; Jeremy Schneider <[email protected]>
On Wed, Jul 24, 2024 at 3:12 PM Jeff Davis <[email protected]> wrote:
> In any case, you are correct that Unicode updates could put some
> constraints at risk, including unique indexes, CHECK, and partition
> constraints. But someone has to actually use one of the affected
> functions somewhere, and that's the main distinction that I'm trying to
> draw.
>
> The reason why collation is qualitatively a much bigger problem is
> because there's no obvious indication that you are doing anything
> related to collation at all. A very plain "CREATE TABLE x(t text
> PRIMARY KEY)" is at risk.
Well, I don't know. I agree that collation is a much bigger problem,
but not for that reason. I think a user who is familiar with the
problems in this area will see the danger either way, and one who
isn't, won't. For me, the only real difference is that a unique index
on a text column is a lot more common than one that involves UPPER.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [18] Policy on IMMUTABLE functions and Unicode updates
@ 2024-07-24 19:43 Jeremy Schneider <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 1 reply; 15+ messages in thread
From: Jeremy Schneider @ 2024-07-24 19:43 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Daniel Verite <[email protected]>; Jeff Davis <[email protected]>; Laurenz Albe <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers
On Wed, Jul 24, 2024 at 12:47 PM Robert Haas <[email protected]> wrote:
On Wed, Jul 24, 2024 at 1:45 PM Jeff Davis <[email protected]> wrote:
> There's a qualitative difference between a collation update which can
> break your PKs and FKs, and a ctype update which definitely will not.
I don't think that's true. All you need is a unique index on UPPER(somecol).
I doubt it’s common to have unique on upper()
But non-unique indexes for case insensitive searches will be more common.
Historically this is the most common way people did case insensitive on
oracle.
Changing ctype would mean these queries can return wrong results
The impact would be similar to the critical problem TripAdvisor hit in 2014
with their read replicas, in the Postgres email thread I linked above
-Jeremy
^ permalink raw reply [nested|flat] 15+ messages in thread
* Re: [18] Policy on IMMUTABLE functions and Unicode updates
@ 2024-07-24 19:47 Robert Haas <[email protected]>
parent: Jeremy Schneider <[email protected]>
0 siblings, 0 replies; 15+ messages in thread
From: Robert Haas @ 2024-07-24 19:47 UTC (permalink / raw)
To: Jeremy Schneider <[email protected]>; +Cc: Daniel Verite <[email protected]>; Jeff Davis <[email protected]>; Laurenz Albe <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Tom Lane <[email protected]>; pgsql-hackers
On Wed, Jul 24, 2024 at 3:43 PM Jeremy Schneider
<[email protected]> wrote:
> But non-unique indexes for case insensitive searches will be more common. Historically this is the most common way people did case insensitive on oracle.
>
> Changing ctype would mean these queries can return wrong results
Yeah. I mentioned earlier that I very recently saw a customer query
with UPPER() in the join condition. If someone is doing foo JOIN bar
ON upper(foo.x) = upper(bar.x), it is not unlikely that one or both of
those expressions are indexed. Not guaranteed, of course, but very
plausible.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 15+ messages in thread
end of thread, other threads:[~2024-07-24 19:47 UTC | newest]
Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2019-01-17 12:05 [PATCH 2/4] Change-tmplinit-argument Arthur Zakirov <[email protected]>
2019-01-17 12:05 [PATCH 2/4] Change-tmplinit-argument Arthur Zakirov <[email protected]>
2022-11-02 06:32 remap the .text segment into huge pages at run time John Naylor <[email protected]>
2022-11-03 17:21 ` Re: remap the .text segment into huge pages at run time Andres Freund <[email protected]>
2022-11-04 18:33 ` Re: remap the .text segment into huge pages at run time Andres Freund <[email protected]>
2022-11-04 21:21 ` Re: remap the .text segment into huge pages at run time Andres Freund <[email protected]>
2022-11-05 05:54 ` Re: remap the .text segment into huge pages at run time John Naylor <[email protected]>
2022-11-05 08:27 ` Re: remap the .text segment into huge pages at run time Andres Freund <[email protected]>
2022-11-06 06:56 ` Re: remap the .text segment into huge pages at run time John Naylor <[email protected]>
2022-11-06 18:16 ` Re: remap the .text segment into huge pages at run time Andres Freund <[email protected]>
2023-06-25 11:48 [PATCH v1 2/7] Row pattern recognition patch (parse/analysis). Tatsuo Ishii <[email protected]>
2024-07-24 19:12 Re: [18] Policy on IMMUTABLE functions and Unicode updates Jeff Davis <[email protected]>
2024-07-24 19:19 ` Re: [18] Policy on IMMUTABLE functions and Unicode updates Robert Haas <[email protected]>
2024-07-24 19:43 ` Re: [18] Policy on IMMUTABLE functions and Unicode updates Jeremy Schneider <[email protected]>
2024-07-24 19:47 ` Re: [18] Policy on IMMUTABLE functions and Unicode updates Robert Haas <[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