public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 4/4] Implement vacuum full/cluster (INDEX_TABLESPACE <tablespace>)
21+ messages / 6 participants
[nested] [flat]
* [PATCH 4/4] Implement vacuum full/cluster (INDEX_TABLESPACE <tablespace>)
@ 2020-04-01 01:35 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 21+ messages in thread
From: Justin Pryzby @ 2020-04-01 01:35 UTC (permalink / raw)
---
doc/src/sgml/ref/cluster.sgml | 12 ++++-
doc/src/sgml/ref/vacuum.sgml | 12 ++++-
src/backend/commands/cluster.c | 56 ++++++++++++++---------
src/backend/commands/matview.c | 3 +-
src/backend/commands/tablecmds.c | 2 +-
src/backend/commands/vacuum.c | 40 ++++++++--------
src/backend/postmaster/autovacuum.c | 1 +
src/include/commands/cluster.h | 4 +-
src/include/commands/vacuum.h | 5 +-
src/test/regress/input/tablespace.source | 13 ++++++
src/test/regress/output/tablespace.source | 20 ++++++++
11 files changed, 117 insertions(+), 51 deletions(-)
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 6758ef97a6..9c0f5add59 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -29,6 +29,7 @@ CLUSTER [VERBOSE]
VERBOSE [ <replaceable class="parameter">boolean</replaceable> ]
TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
+ INDEX_TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
</synopsis>
</refsynopsisdiv>
@@ -106,6 +107,15 @@ CLUSTER [VERBOSE]
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>INDEX_TABLESPACE</literal></term>
+ <listitem>
+ <para>
+ Specifies that the table's indexes will be rebuilt on a new tablespace.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><literal>TABLESPACE</literal></term>
<listitem>
@@ -128,7 +138,7 @@ CLUSTER [VERBOSE]
<term><replaceable class="parameter">new_tablespace</replaceable></term>
<listitem>
<para>
- The tablespace where the table will be rebuilt.
+ The tablespace where the table or its indexes will be rebuilt.
</para>
</listitem>
</varlistentry>
diff --git a/doc/src/sgml/ref/vacuum.sgml b/doc/src/sgml/ref/vacuum.sgml
index 5261a7c727..28cab119b6 100644
--- a/doc/src/sgml/ref/vacuum.sgml
+++ b/doc/src/sgml/ref/vacuum.sgml
@@ -36,6 +36,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
TRUNCATE [ <replaceable class="parameter">boolean</replaceable> ]
PARALLEL <replaceable class="parameter">integer</replaceable>
TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
+ INDEX_TABLESPACE <replaceable class="parameter">new_tablespace</replaceable>
<phrase>and <replaceable class="parameter">table_and_columns</replaceable> is:</phrase>
@@ -265,6 +266,15 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>INDEX_TABLESPACE</literal></term>
+ <listitem>
+ <para>
+ Specifies that the relation's indexes will be rebuilt on a new tablespace.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><replaceable class="parameter">boolean</replaceable></term>
<listitem>
@@ -314,7 +324,7 @@ VACUUM [ FULL ] [ FREEZE ] [ VERBOSE ] [ ANALYZE ] [ <replaceable class="paramet
<term><replaceable class="parameter">new_tablespace</replaceable></term>
<listitem>
<para>
- The tablespace where the relation will be rebuilt.
+ The tablespace where the relation or its indexes will be rebuilt.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
index 626321e3ac..7fc6b69e95 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -71,7 +71,7 @@ typedef struct
static void rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose,
- Oid NewTableSpaceOid);
+ Oid NewTableSpaceOid, Oid NewIdxTableSpaceOid);
static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
bool verbose, bool *pSwapToastByContent,
TransactionId *pFreezeXid, MultiXactId *pCutoffMulti);
@@ -108,8 +108,9 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
ListCell *lc;
ClusterParams params = {0};
bool verbose = false;
- /* Name of tablespace to use for clustered relation. */
- char *tablespaceName = NULL;
+ /* Names of tablespaces to use for clustered relations. */
+ char *tablespaceName = NULL,
+ *idxtablespaceName = NULL;
/* Parse option list */
foreach(lc, stmt->params)
@@ -120,6 +121,8 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
verbose = defGetBoolean(opt);
else if (strcmp(opt->defname, "tablespace") == 0)
tablespaceName = defGetString(opt);
+ else if (strcmp(opt->defname, "index_tablespace") == 0)
+ idxtablespaceName = defGetString(opt);
else
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
@@ -130,18 +133,11 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
params.options = (verbose ? CLUOPT_VERBOSE : 0);
- /* Select tablespace Oid to use. */
- if (tablespaceName)
- {
- params.tablespaceOid = get_tablespace_oid(tablespaceName, false);
-
- /* Can't move a non-shared relation into pg_global */
- if (params.tablespaceOid == GLOBALTABLESPACE_OID)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot move non-shared relation to tablespace \"%s\"",
- tablespaceName)));
- }
+ /* Get tablespaces to use. */
+ params.tablespaceOid = tablespaceName ?
+ get_tablespace_oid(tablespaceName, false) : InvalidOid;
+ params.idxtablespaceOid = idxtablespaceName ?
+ get_tablespace_oid(idxtablespaceName, false) : InvalidOid;
if (stmt->relation != NULL)
{
@@ -293,8 +289,9 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
* instead of index order. This is the new implementation of VACUUM FULL,
* and error messages should refer to the operation as VACUUM not CLUSTER.
*
- * "tablespaceOid" is the tablespace where the relation will be rebuilt, or
- * InvalidOid to use its current tablespace.
+ * "tablespaceOid" and "idxtablespaceOid" are the tablespaces where the relation
+ * and its indexes will be rebuilt, or InvalidOid to use their current
+ * tablespaces.
*/
void
cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
@@ -464,7 +461,8 @@ cluster_rel(Oid tableOid, Oid indexOid, ClusterParams *params)
TransferPredicateLocksToHeapRelation(OldHeap);
/* rebuild_relation does all the dirty work */
- rebuild_relation(OldHeap, indexOid, verbose, params->tablespaceOid);
+ rebuild_relation(OldHeap, indexOid, verbose, params->tablespaceOid,
+ params->idxtablespaceOid);
/* NB: rebuild_relation does table_close() on OldHeap */
@@ -613,10 +611,11 @@ mark_index_clustered(Relation rel, Oid indexOid, bool is_internal)
* NB: this routine closes OldHeap at the right time; caller should not.
*/
static void
-rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespaceOid)
+rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespaceOid, Oid NewIdxTablespaceOid)
{
Oid tableOid = RelationGetRelid(OldHeap);
Oid tableSpace = OldHeap->rd_rel->reltablespace;
+ Oid idxtableSpace;
Oid OIDNewHeap;
char relpersistence;
bool is_system_catalog;
@@ -626,7 +625,20 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespace
/* Use new tablespace if passed. */
if (OidIsValid(NewTablespaceOid))
+ {
tableSpace = NewTablespaceOid;
+ /* It's not a shared catalog, so refuse to move it to shared tablespace */
+ if (tableSpace == GLOBALTABLESPACE_OID)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot move non-shared relation to tablespace \"%s\"",
+ get_tablespace_name(tableSpace))));
+ }
+
+ if (OidIsValid(NewIdxTablespaceOid))
+ idxtableSpace = NewIdxTablespaceOid;
+ else
+ idxtableSpace = get_rel_tablespace(indexOid);
/* Mark the correct index as clustered */
if (OidIsValid(indexOid))
@@ -655,7 +667,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, bool verbose, Oid NewTablespace
finish_heap_swap(tableOid, OIDNewHeap, is_system_catalog,
swap_toast_by_content, false, true,
frozenXid, cutoffMulti,
- relpersistence);
+ relpersistence, idxtableSpace);
}
@@ -1403,12 +1415,12 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
bool is_internal,
TransactionId frozenXid,
MultiXactId cutoffMulti,
- char newrelpersistence)
+ char newrelpersistence, Oid idxtableSpace)
{
ObjectAddress object;
Oid mapped_tables[4];
int reindex_flags;
- ReindexParams reindex_params = {0};
+ ReindexParams reindex_params = { .tablespaceOid = idxtableSpace };
int i;
/* Report that we are now swapping relation files */
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index c5c25ce11d..ad1bfdc0d2 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -855,7 +855,8 @@ static void
refresh_by_heap_swap(Oid matviewOid, Oid OIDNewHeap, char relpersistence)
{
finish_heap_swap(matviewOid, OIDNewHeap, false, false, true, true,
- RecentXmin, ReadNextMultiXactId(), relpersistence);
+ RecentXmin, ReadNextMultiXactId(), relpersistence,
+ InvalidOid);
}
/*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 75b04eb161..eba8478f91 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5077,7 +5077,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode,
!OidIsValid(tab->newTableSpace),
RecentXmin,
ReadNextMultiXactId(),
- persistence);
+ persistence, InvalidOid);
}
else
{
diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 61565c3f14..0eb9786bea 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -109,9 +109,9 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
bool disable_page_skipping = false;
ListCell *lc;
- /* Name and Oid of tablespace to use for relations after VACUUM FULL. */
- char *tablespacename = NULL;
- Oid tablespaceOid = InvalidOid;
+ /* Tablespace to use for relations after VACUUM FULL. */
+ char *tablespacename = NULL,
+ *idxtablespacename = NULL;
/* Set default value */
params.index_cleanup = VACOPT_TERNARY_DEFAULT;
@@ -151,6 +151,8 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
params.truncate = get_vacopt_ternary_value(opt);
else if (strcmp(opt->defname, "tablespace") == 0)
tablespacename = defGetString(opt);
+ else if (strcmp(opt->defname, "index_tablespace") == 0)
+ idxtablespacename = defGetString(opt);
else if (strcmp(opt->defname, "parallel") == 0)
{
if (opt->arg == NULL)
@@ -211,26 +213,18 @@ ExecVacuum(ParseState *pstate, VacuumStmt *vacstmt, bool isTopLevel)
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("VACUUM FULL cannot be performed in parallel")));
- /* Get tablespace Oid to use. */
- if (tablespacename)
- {
- if ((params.options & VACOPT_FULL) == 0)
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("incompatible TABLESPACE option"),
- errdetail("TABLESPACE can only be used with VACUUM FULL.")));
-
- tablespaceOid = get_tablespace_oid(tablespacename, false);
-
- /* Can't move a non-shared relation into pg_global */
- if (tablespaceOid == GLOBALTABLESPACE_OID)
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("cannot move non-shared relation to tablespace \"%s\"",
- tablespacename)));
+ if ((params.options & VACOPT_FULL) == 0 &&
+ (tablespacename || idxtablespacename))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("incompatible TABLESPACE option"),
+ errdetail("TABLESPACE can only be used with VACUUM FULL.")));
- }
- params.tablespace_oid = tablespaceOid;
+ /* Get tablespace Oids to use. */
+ params.tablespace_oid = tablespacename ?
+ get_tablespace_oid(tablespacename, false) : InvalidOid;
+ params.idxtablespace_oid = idxtablespacename ?
+ get_tablespace_oid(idxtablespacename, false) : InvalidOid;
/*
* Make sure VACOPT_ANALYZE is specified if any column lists are present.
@@ -1964,6 +1958,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
{
ClusterParams cluster_params = {
.tablespaceOid = params->tablespace_oid,
+ .idxtablespaceOid = params->idxtablespace_oid,
+ /* Other params initialized to 0/false/NULL */
};
/* close relation before vacuuming, but hold lock until commit */
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index a5d98c8c95..fd9eb75e11 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2933,6 +2933,7 @@ table_recheck_autovac(Oid relid, HTAB *table_toast_map,
tab->at_params.is_wraparound = wraparound;
tab->at_params.log_min_duration = log_min_duration;
tab->at_params.tablespace_oid = InvalidOid;
+ tab->at_params.idxtablespace_oid = InvalidOid;
tab->at_vacuum_cost_limit = vac_cost_limit;
tab->at_vacuum_cost_delay = vac_cost_delay;
tab->at_relname = NULL;
diff --git a/src/include/commands/cluster.h b/src/include/commands/cluster.h
index 868dea5ecc..c04a2ce20b 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -28,6 +28,7 @@ typedef struct ClusterParams
{
bits32 options; /* bitmask of CLUOPT_* */
Oid tablespaceOid; /* tablespace to rebuild relation, or InvalidOid */
+ Oid idxtablespaceOid; /* tablespace to rebuild relation's indexes */
} ClusterParams;
extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel);
@@ -45,6 +46,7 @@ extern void finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
bool is_internal,
TransactionId frozenXid,
MultiXactId minMulti,
- char newrelpersistence);
+ char newrelpersistence,
+ Oid idxtablespaceOid);
#endif /* CLUSTER_H */
diff --git a/src/include/commands/vacuum.h b/src/include/commands/vacuum.h
index 0934487d4b..6c4e085a45 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -227,8 +227,9 @@ typedef struct VacuumParams
* disabled.
*/
int nworkers;
- Oid tablespace_oid; /* tablespace to use for relations
- * rebuilt by VACUUM FULL */
+ /* tablespaces to use for relations rebuilt by VACUUM FULL */
+ Oid tablespace_oid;
+ Oid idxtablespace_oid;
} VacuumParams;
/* GUC parameters */
diff --git a/src/test/regress/input/tablespace.source b/src/test/regress/input/tablespace.source
index 2244c1d51d..9a13112605 100644
--- a/src/test/regress/input/tablespace.source
+++ b/src/test/regress/input/tablespace.source
@@ -80,6 +80,19 @@ REINDEX (TABLESPACE pg_default) TABLE CONCURRENTLY regress_tblspace_test_tbl; --
SELECT relname FROM pg_class
WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace');
+-- check CLUSTER with INDEX_TABLESPACE change to non-default location
+CLUSTER (INDEX_TABLESPACE regress_tblspace) regress_tblspace_test_tbl USING regress_tblspace_test_tbl_idx; -- ok
+-- check relations moved to new tablespace
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
+ORDER BY relname;
+-- check VACUUM with INDEX_TABLESPACE change
+VACUUM (FULL, ANALYSE, FREEZE, INDEX_TABLESPACE pg_default) regress_tblspace_test_tbl; -- ok
+-- check relations moved back to pg_default
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace');
+
+
-- create a schema we can use
CREATE SCHEMA testschema;
diff --git a/src/test/regress/output/tablespace.source b/src/test/regress/output/tablespace.source
index 889933a9c8..2f8ecf514d 100644
--- a/src/test/regress/output/tablespace.source
+++ b/src/test/regress/output/tablespace.source
@@ -114,6 +114,26 @@ WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspa
---------
(0 rows)
+-- check CLUSTER with INDEX_TABLESPACE change to non-default location
+CLUSTER (INDEX_TABLESPACE regress_tblspace) regress_tblspace_test_tbl USING regress_tblspace_test_tbl_idx; -- ok
+-- check relations moved to new tablespace
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace')
+ORDER BY relname;
+ relname
+-------------------------------
+ regress_tblspace_test_tbl_idx
+(1 row)
+
+-- check VACUUM with INDEX_TABLESPACE change
+VACUUM (FULL, ANALYSE, FREEZE, INDEX_TABLESPACE pg_default) regress_tblspace_test_tbl; -- ok
+-- check relations moved back to pg_default
+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace');
+ relname
+---------
+(0 rows)
+
-- create a schema we can use
CREATE SCHEMA testschema;
-- try a table
--
2.17.0
--tsOsTdHNUZQcU9Ye--
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
@ 2023-10-11 09:31 Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
0 siblings, 1 reply; 21+ messages in thread
From: Ronan Dunklau @ 2023-10-11 09:31 UTC (permalink / raw)
To: Devrim Gündüz <[email protected]>; Thomas Munro <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Andres Freund <[email protected]>; Dmitry Dolgov <[email protected]>
Le mercredi 11 octobre 2023, 10:59:50 CEST Thomas Munro a écrit :
> The back-patch to 12 was a little trickier than anticipated, but after
> taking a break and trying again I now have PG 12...17 patches that
> I've tested against LLVM 10...18 (that's 54 combinations), in every
> case only with the clang corresponding to LLVM.
Thank you Thomas for those patches, and the extensive testing, I will run my
own and let you know.
> I've attached only the patches for master, but the 12-16 versions are
> available at https://github.com/macdice/postgres/tree/llvm16-$N in
> case anyone has comments on those.
For PG13 and PG12, it looks like the ExecEvalBoolSubroutineTemplate is not
used anywhere, as ExecEvalBoolSubroutine was introduced in PG14 if I'm not
mistaken.
Best regards,
--
Ronan Dunklau
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
@ 2023-10-13 20:32 ` Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
0 siblings, 1 reply; 21+ messages in thread
From: Thomas Munro @ 2023-10-13 20:32 UTC (permalink / raw)
To: Ronan Dunklau <[email protected]>; +Cc: Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Andres Freund <[email protected]>; Dmitry Dolgov <[email protected]>
On Wed, Oct 11, 2023 at 10:31 PM Ronan Dunklau <[email protected]> wrote:
> Le mercredi 11 octobre 2023, 10:59:50 CEST Thomas Munro a écrit :
> > The back-patch to 12 was a little trickier than anticipated, but after
> > taking a break and trying again I now have PG 12...17 patches that
> > I've tested against LLVM 10...18 (that's 54 combinations), in every
> > case only with the clang corresponding to LLVM.
>
> Thank you Thomas for those patches, and the extensive testing, I will run my
> own and let you know.
Thanks! No news is good news, I hope? I'm hoping to commit this today.
> > I've attached only the patches for master, but the 12-16 versions are
> > available at https://github.com/macdice/postgres/tree/llvm16-$N in
> > case anyone has comments on those.
>
> For PG13 and PG12, it looks like the ExecEvalBoolSubroutineTemplate is not
> used anywhere, as ExecEvalBoolSubroutine was introduced in PG14 if I'm not
> mistaken.
Right, looks like I can remove that in those branches.
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
@ 2023-10-16 08:31 ` Ronan Dunklau <[email protected]>
2023-10-18 11:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
0 siblings, 1 reply; 21+ messages in thread
From: Ronan Dunklau @ 2023-10-16 08:31 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Andres Freund <[email protected]>; Dmitry Dolgov <[email protected]>
Le vendredi 13 octobre 2023, 22:32:13 CEST Thomas Munro a écrit :
> On Wed, Oct 11, 2023 at 10:31 PM Ronan Dunklau <[email protected]>
wrote:
> > Le mercredi 11 octobre 2023, 10:59:50 CEST Thomas Munro a écrit :
> > > The back-patch to 12 was a little trickier than anticipated, but after
> > > taking a break and trying again I now have PG 12...17 patches that
> > > I've tested against LLVM 10...18 (that's 54 combinations), in every
> > > case only with the clang corresponding to LLVM.
> >
> > Thank you Thomas for those patches, and the extensive testing, I will run
> > my own and let you know.
>
> Thanks! No news is good news, I hope? I'm hoping to commit this today.
>
> > > I've attached only the patches for master, but the 12-16 versions are
> > > available at https://github.com/macdice/postgres/tree/llvm16-$N in
> > > case anyone has comments on those.
> >
> > For PG13 and PG12, it looks like the ExecEvalBoolSubroutineTemplate is not
> > used anywhere, as ExecEvalBoolSubroutine was introduced in PG14 if I'm not
> > mistaken.
>
> Right, looks like I can remove that in those branches.
Oh sorry I thought I followed up. I ran the same stress testing involving
several hours of sqlsmith with all jit costs set to zero and didn't notice
anything with LLVM16.
Thank you !
--
Ronan Dunklau
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
@ 2023-10-18 11:02 ` Thomas Munro <[email protected]>
2023-10-18 12:06 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
0 siblings, 1 reply; 21+ messages in thread
From: Thomas Munro @ 2023-10-18 11:02 UTC (permalink / raw)
To: Ronan Dunklau <[email protected]>; +Cc: Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Andres Freund <[email protected]>; Dmitry Dolgov <[email protected]>
I pushed the first patch, for LLVM 16, and the build farm told me that
some old LLVM versions don't like it. The problem seems to be the
function LLVMGlobalGetValueType(). I can see that that was only added
to the C API in 2018, so it looks like I may need to back-port that
(trivial) wrapper into our own llvmjit_wrap.cpp for LLVM < 8.
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-18 11:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
@ 2023-10-18 12:06 ` Thomas Munro <[email protected]>
2023-10-18 17:20 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
0 siblings, 1 reply; 21+ messages in thread
From: Thomas Munro @ 2023-10-18 12:06 UTC (permalink / raw)
To: Ronan Dunklau <[email protected]>; +Cc: Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Andres Freund <[email protected]>; Dmitry Dolgov <[email protected]>
On Thu, Oct 19, 2023 at 12:02 AM Thomas Munro <[email protected]> wrote:
> I pushed the first patch, for LLVM 16, and the build farm told me that
> some old LLVM versions don't like it. The problem seems to be the
> function LLVMGlobalGetValueType(). I can see that that was only added
> to the C API in 2018, so it looks like I may need to back-port that
> (trivial) wrapper into our own llvmjit_wrap.cpp for LLVM < 8.
Concretely something like the attached should probably fix it, but
it'll take me a little while to confirm that...
Attachments:
[application/octet-stream] 0001-Supply-missing-LLVMGlobalGetValueType-for-LLVM-8.patch (1.7K, ../../CA+hUKGKtUB=-BQZd=u4BFZ-UJWi47fekwiYMSDXDZMyV+z1G-g@mail.gmail.com/2-0001-Supply-missing-LLVMGlobalGetValueType-for-LLVM-8.patch)
download | inline diff:
From 69effaf33a733ebf330281837679cb82e45fc1db Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email protected]>
Date: Thu, 19 Oct 2023 00:24:02 +1300
Subject: [PATCH] Supply missing LLVMGlobalGetValueType() for LLVM < 8.
Commit 37d5babb started using this C function while adding support for
LLVM 16, but failed to notice that it was not available in old LLVM
versions. We can put the wrapper into our own llvmjit_wrap.cpp file.
Discussion: https://postgr.es/m/CA%2BhUKGKnLnJnWrkr%3D4mSGhE5FuTK55FY15uULR7%3Dzzc%3DwX4Nqw%40mail.gmail.com
---
src/backend/jit/llvm/llvmjit_wrap.cpp | 8 ++++++++
src/include/jit/llvmjit.h | 4 ++++
2 files changed, 12 insertions(+)
diff --git a/src/backend/jit/llvm/llvmjit_wrap.cpp b/src/backend/jit/llvm/llvmjit_wrap.cpp
index 997a2c0278..cb896e2b6a 100644
--- a/src/backend/jit/llvm/llvmjit_wrap.cpp
+++ b/src/backend/jit/llvm/llvmjit_wrap.cpp
@@ -88,3 +88,11 @@ LLVMGetFunctionType(LLVMValueRef r)
{
return llvm::wrap(llvm::unwrap<llvm::Function>(r)->getFunctionType());
}
+
+#if LLVM_VERSION_MAJOR < 8
+LLVMTypeRef
+LLVMGlobalGetValueType(LLVMValueRef g)
+{
+ return llvm::wrap(llvm::unwrap<llvm::GlobalValue>(g)->getValueType());
+}
+#endif
diff --git a/src/include/jit/llvmjit.h b/src/include/jit/llvmjit.h
index 8eed61373b..3ab86de3ac 100644
--- a/src/include/jit/llvmjit.h
+++ b/src/include/jit/llvmjit.h
@@ -146,6 +146,10 @@ extern unsigned LLVMGetAttributeCountAtIndexPG(LLVMValueRef F, uint32 Idx);
extern LLVMTypeRef LLVMGetFunctionReturnType(LLVMValueRef r);
extern LLVMTypeRef LLVMGetFunctionType(LLVMValueRef r);
+#if LLVM_MAJOR_VERSION < 8
+extern LLVMTypeRef LLVMGlobalGetValueType(LLVMValueRef g);
+#endif
+
#ifdef __cplusplus
} /* extern "C" */
#endif
--
2.42.0
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-18 11:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 12:06 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
@ 2023-10-18 17:20 ` Thomas Munro <[email protected]>
2023-10-18 18:11 ` Re: LLVM 16 (opaque pointers) Andres Freund <[email protected]>
2023-10-20 21:48 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-21 06:08 ` Re: LLVM 16 (opaque pointers) Andres Freund <[email protected]>
0 siblings, 3 replies; 21+ messages in thread
From: Thomas Munro @ 2023-10-18 17:20 UTC (permalink / raw)
To: Ronan Dunklau <[email protected]>; +Cc: Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Andres Freund <[email protected]>; Dmitry Dolgov <[email protected]>
On Thu, Oct 19, 2023 at 1:06 AM Thomas Munro <[email protected]> wrote:
> On Thu, Oct 19, 2023 at 12:02 AM Thomas Munro <[email protected]> wrote:
> > I pushed the first patch, for LLVM 16, and the build farm told me that
> > some old LLVM versions don't like it. The problem seems to be the
> > function LLVMGlobalGetValueType(). I can see that that was only added
> > to the C API in 2018, so it looks like I may need to back-port that
> > (trivial) wrapper into our own llvmjit_wrap.cpp for LLVM < 8.
>
> Concretely something like the attached should probably fix it, but
> it'll take me a little while to confirm that...
Pushed, after digging up some old LLVM skeletons to test, and those
"old LLVM" animals are turning green now. I went ahead and pushed the
much smaller and simpler patch for LLVM 17.
Interestingly, a new problem just showed up on the the RHEL9 s390x
machine "lora", where a previously reported problem [1] apparently
re-appeared. It complains about incompatible layout, previously
blamed on mismatch between clang and LLVM versions. I can see that
its clang is v15 from clues in the conflig log, but I don't know which
version of LLVM is being used. However, I see now that --with-llvm
was literally just turned on, so there is no reason to think that this
would have worked before or this work is relevant. Strange though --
we must be able to JIT further than that on s390x because we have
crash reports in other threads (ie we made it past this and into other
more advanced brokenness).
[1] https://www.postgresql.org/message-id/flat/20210319190047.7o4bwhbp5dzkqif3%40alap3.anarazel.de#ec51b...
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-18 11:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 12:06 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 17:20 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
@ 2023-10-18 18:11 ` Andres Freund <[email protected]>
2 siblings, 0 replies; 21+ messages in thread
From: Andres Freund @ 2023-10-18 18:11 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Ronan Dunklau <[email protected]>; Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Dmitry Dolgov <[email protected]>
Hi,
On 2023-10-19 06:20:26 +1300, Thomas Munro wrote:
> On Thu, Oct 19, 2023 at 1:06 AM Thomas Munro <[email protected]> wrote:
> > On Thu, Oct 19, 2023 at 12:02 AM Thomas Munro <[email protected]> wrote:
> > > I pushed the first patch, for LLVM 16, and the build farm told me that
> > > some old LLVM versions don't like it. The problem seems to be the
> > > function LLVMGlobalGetValueType(). I can see that that was only added
> > > to the C API in 2018, so it looks like I may need to back-port that
> > > (trivial) wrapper into our own llvmjit_wrap.cpp for LLVM < 8.
> >
> > Concretely something like the attached should probably fix it, but
> > it'll take me a little while to confirm that...
>
> Pushed, after digging up some old LLVM skeletons to test, and those
> "old LLVM" animals are turning green now. I went ahead and pushed the
> much smaller and simpler patch for LLVM 17.
I enabled a new set of buildfarm animals to test LLVM 16 and 17. I initially
forgot to disable them for 11, which means we'll have those failed build on 11
until they age out :/.
Particularly for the LLVM debug builds it'll take a fair bit to run on all
branches. Each branch takes about 3h.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-18 11:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 12:06 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 17:20 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
@ 2023-10-20 21:48 ` Thomas Munro <[email protected]>
2023-10-20 22:00 ` Re: LLVM 16 (opaque pointers) Mark Wong <[email protected]>
2023-10-20 22:03 ` Re: LLVM 16 (opaque pointers) Tom Lane <[email protected]>
2023-10-20 22:12 ` Re: LLVM 16 (opaque pointers) Andres Freund <[email protected]>
2 siblings, 3 replies; 21+ messages in thread
From: Thomas Munro @ 2023-10-20 21:48 UTC (permalink / raw)
To: Ronan Dunklau <[email protected]>; +Cc: Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Andres Freund <[email protected]>; Dmitry Dolgov <[email protected]>
On Thu, Oct 19, 2023 at 6:20 AM Thomas Munro <[email protected]> wrote:
> Interestingly, a new problem just showed up on the the RHEL9 s390x
> machine "lora", where a previously reported problem [1] apparently
> re-appeared. It complains about incompatible layout, previously
> blamed on mismatch between clang and LLVM versions. I can see that
> its clang is v15 from clues in the conflig log, but I don't know which
> version of LLVM is being used. However, I see now that --with-llvm
> was literally just turned on, so there is no reason to think that this
> would have worked before or this work is relevant. Strange though --
> we must be able to JIT further than that on s390x because we have
> crash reports in other threads (ie we made it past this and into other
> more advanced brokenness).
I see that Mark has also just enabled --with-llvm on some POWER Linux
animals, and they have failed in various ways. The failures are
strangely lacking in detail. It seems we didn't have coverage before,
and I recall that there were definitely versions of LLVM that *didn't*
work for our usage in the past, which I'll need to dredge out of the
archives. I will try to get onto a cfarm POWER machine and see if I
can reproduce that, before and after these commits, and whose bug is
it etc.
I doubt I can get anywhere near an s390x though, and we definitely had
pre-existing problems on that arch.
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-18 11:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 12:06 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 17:20 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 21:48 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
@ 2023-10-20 22:00 ` Mark Wong <[email protected]>
2 siblings, 0 replies; 21+ messages in thread
From: Mark Wong @ 2023-10-20 22:00 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Ronan Dunklau <[email protected]>; Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Andres Freund <[email protected]>; Dmitry Dolgov <[email protected]>
On Sat, Oct 21, 2023 at 10:48:47AM +1300, Thomas Munro wrote:
> On Thu, Oct 19, 2023 at 6:20 AM Thomas Munro <[email protected]> wrote:
> > Interestingly, a new problem just showed up on the the RHEL9 s390x
> > machine "lora", where a previously reported problem [1] apparently
> > re-appeared. It complains about incompatible layout, previously
> > blamed on mismatch between clang and LLVM versions. I can see that
> > its clang is v15 from clues in the conflig log, but I don't know which
> > version of LLVM is being used. However, I see now that --with-llvm
> > was literally just turned on, so there is no reason to think that this
> > would have worked before or this work is relevant. Strange though --
> > we must be able to JIT further than that on s390x because we have
> > crash reports in other threads (ie we made it past this and into other
> > more advanced brokenness).
>
> I see that Mark has also just enabled --with-llvm on some POWER Linux
> animals, and they have failed in various ways. The failures are
> strangely lacking in detail. It seems we didn't have coverage before,
> and I recall that there were definitely versions of LLVM that *didn't*
> work for our usage in the past, which I'll need to dredge out of the
> archives. I will try to get onto a cfarm POWER machine and see if I
> can reproduce that, before and after these commits, and whose bug is
> it etc.
Yeah, I'm slowing enabling --with-llvm on POWER, s390x, and aarch64 (but
none here yet as I write this)...
> I doubt I can get anywhere near an s390x though, and we definitely had
> pre-existing problems on that arch.
If you want to send me your ssh key, I have access to these systems
through OSUOSL and LinuxFoundation programs.
Regards,
Mark
--
Mark Wong
EDB https://enterprisedb.com
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-18 11:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 12:06 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 17:20 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 21:48 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
@ 2023-10-20 22:03 ` Tom Lane <[email protected]>
2 siblings, 0 replies; 21+ messages in thread
From: Tom Lane @ 2023-10-20 22:03 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Ronan Dunklau <[email protected]>; Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Andres Freund <[email protected]>; Dmitry Dolgov <[email protected]>; [email protected]
Thomas Munro <[email protected]> writes:
> I doubt I can get anywhere near an s390x though, and we definitely had
> pre-existing problems on that arch.
Yeah. Too bad there's no s390x in the gcc compile farm.
(I'm wondering how straight a line to draw between that fact
and llvm's evident shortcomings on s390x.)
I'm missing my old access to Red Hat's dev machines. But in
the meantime, Mark's clearly got beaucoup access to s390
machines, so I wonder if he can let you into any of them?
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-18 11:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 12:06 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 17:20 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 21:48 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
@ 2023-10-20 22:12 ` Andres Freund <[email protected]>
2023-10-20 23:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2 siblings, 1 reply; 21+ messages in thread
From: Andres Freund @ 2023-10-20 22:12 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Ronan Dunklau <[email protected]>; Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Dmitry Dolgov <[email protected]>
Hi,
On 2023-10-21 10:48:47 +1300, Thomas Munro wrote:
> On Thu, Oct 19, 2023 at 6:20 AM Thomas Munro <[email protected]> wrote:
> > Interestingly, a new problem just showed up on the the RHEL9 s390x
> > machine "lora", where a previously reported problem [1] apparently
> > re-appeared. It complains about incompatible layout, previously
> > blamed on mismatch between clang and LLVM versions. I can see that
> > its clang is v15 from clues in the conflig log, but I don't know which
> > version of LLVM is being used. However, I see now that --with-llvm
> > was literally just turned on, so there is no reason to think that this
> > would have worked before or this work is relevant. Strange though --
> > we must be able to JIT further than that on s390x because we have
> > crash reports in other threads (ie we made it past this and into other
> > more advanced brokenness).
>
> I see that Mark has also just enabled --with-llvm on some POWER Linux
> animals, and they have failed in various ways. The failures are
> strangely lacking in detail. It seems we didn't have coverage before,
> and I recall that there were definitely versions of LLVM that *didn't*
> work for our usage in the past, which I'll need to dredge out of the
> archives. I will try to get onto a cfarm POWER machine and see if I
> can reproduce that, before and after these commits, and whose bug is
> it etc.
I'm quite sure that jiting did pass on ppc64 at some point.
> I doubt I can get anywhere near an s390x though, and we definitely had
> pre-existing problems on that arch.
IMO an LLVM bug, rather than a postgres bug, but I guess it's all a matter of
perspective.
https://github.com/llvm/llvm-project/issues/53009#issuecomment-1042748553
I had made another bug report about this issue at some point, but I can't
refind it right now.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-18 11:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 12:06 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 17:20 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 21:48 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 22:12 ` Re: LLVM 16 (opaque pointers) Andres Freund <[email protected]>
@ 2023-10-20 23:02 ` Thomas Munro <[email protected]>
2023-10-20 23:07 ` Re: LLVM 16 (opaque pointers) Andres Freund <[email protected]>
2023-10-21 00:45 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
0 siblings, 2 replies; 21+ messages in thread
From: Thomas Munro @ 2023-10-20 23:02 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Ronan Dunklau <[email protected]>; Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Dmitry Dolgov <[email protected]>
On Sat, Oct 21, 2023 at 11:12 AM Andres Freund <[email protected]> wrote:
> On 2023-10-21 10:48:47 +1300, Thomas Munro wrote:
> > On Thu, Oct 19, 2023 at 6:20 AM Thomas Munro <[email protected]> wrote:
> > I see that Mark has also just enabled --with-llvm on some POWER Linux
> > animals, and they have failed in various ways. The failures are
> > strangely lacking in detail. It seems we didn't have coverage before,
> > and I recall that there were definitely versions of LLVM that *didn't*
> > work for our usage in the past, which I'll need to dredge out of the
> > archives. I will try to get onto a cfarm POWER machine and see if I
> > can reproduce that, before and after these commits, and whose bug is
> > it etc.
>
> I'm quite sure that jiting did pass on ppc64 at some point.
Yeah, I remember debugging it on EDB's POWER machine. First off, we
know that LLVM < 7 doesn't work for us on POWER, because:
https://www.postgresql.org/message-id/CAEepm%3D39F_B3Ou8S3OrUw%2BhJEUP3p%3DwCu0ug-TTW67qKN53g3w%40ma...
That was fixed:
https://github.com/llvm/llvm-project/commit/a95b0df5eddbe7fa1e9f8fe0b1ff62427e1c0318
So I think that means that we'd first have to go through those animals
and figure out which ones have older LLVM, and ignore those results --
they just can't use --with-llvm. Unfortunately there doesn't seem to
be any clue on the version from the paths used by OpenSUSE. Mark, do
you know?
> > I doubt I can get anywhere near an s390x though, and we definitely had
> > pre-existing problems on that arch.
>
> IMO an LLVM bug, rather than a postgres bug, but I guess it's all a matter of
> perspective.
> https://github.com/llvm/llvm-project/issues/53009#issuecomment-1042748553
Ah, good to know about that. But there are also reports of crashes in
released versions that manage to get passed that ABI-wobble business:
https://www.postgresql.org/message-id/flat/CAF1DzPXjpPxnsgySz2Zjm8d2dx7%3DJ070C%2BMQBFh%2B9NBNcBKCAg...
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-18 11:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 12:06 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 17:20 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 21:48 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 22:12 ` Re: LLVM 16 (opaque pointers) Andres Freund <[email protected]>
2023-10-20 23:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
@ 2023-10-20 23:07 ` Andres Freund <[email protected]>
1 sibling, 0 replies; 21+ messages in thread
From: Andres Freund @ 2023-10-20 23:07 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Ronan Dunklau <[email protected]>; Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Dmitry Dolgov <[email protected]>
Hi,
On 2023-10-21 12:02:51 +1300, Thomas Munro wrote:
> On Sat, Oct 21, 2023 at 11:12 AM Andres Freund <[email protected]> wrote:
> > > I doubt I can get anywhere near an s390x though, and we definitely had
> > > pre-existing problems on that arch.
> >
> > IMO an LLVM bug, rather than a postgres bug, but I guess it's all a matter of
> > perspective.
> > https://github.com/llvm/llvm-project/issues/53009#issuecomment-1042748553
>
> Ah, good to know about that. But there are also reports of crashes in
> released versions that manage to get passed that ABI-wobble business:
>
> https://www.postgresql.org/message-id/flat/CAF1DzPXjpPxnsgySz2Zjm8d2dx7%3DJ070C%2BMQBFh%2B9NBNcBKCAg...
Trying to debug that now, using access to an s390x box provided by Mark...
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-18 11:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 12:06 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 17:20 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 21:48 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 22:12 ` Re: LLVM 16 (opaque pointers) Andres Freund <[email protected]>
2023-10-20 23:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
@ 2023-10-21 00:45 ` Thomas Munro <[email protected]>
2023-10-21 01:45 ` Re: LLVM 16 (opaque pointers) Tom Lane <[email protected]>
1 sibling, 1 reply; 21+ messages in thread
From: Thomas Munro @ 2023-10-21 00:45 UTC (permalink / raw)
To: Mark Wong <[email protected]>; +Cc: Ronan Dunklau <[email protected]>; Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Dmitry Dolgov <[email protected]>; Andres Freund <[email protected]>
On Sat, Oct 21, 2023 at 12:02 PM Thomas Munro <[email protected]> wrote:
> On Sat, Oct 21, 2023 at 11:12 AM Andres Freund <[email protected]> wrote:
> > I'm quite sure that jiting did pass on ppc64 at some point.
>
> Yeah, I remember debugging it on EDB's POWER machine. First off, we
> know that LLVM < 7 doesn't work for us on POWER, because:
>
> https://www.postgresql.org/message-id/CAEepm%3D39F_B3Ou8S3OrUw%2BhJEUP3p%3DwCu0ug-TTW67qKN53g3w%40ma...
>
> That was fixed:
>
> https://github.com/llvm/llvm-project/commit/a95b0df5eddbe7fa1e9f8fe0b1ff62427e1c0318
>
> So I think that means that we'd first have to go through those animals
> and figure out which ones have older LLVM, and ignore those results --
> they just can't use --with-llvm. Unfortunately there doesn't seem to
> be any clue on the version from the paths used by OpenSUSE. Mark, do
> you know?
Adding Mark to this subthread. Concretely, could you please disable
--with-llvm on any of those machines running LLVM < 7? And report
what version any remaining animals are running? (It'd be nice if the
build farm logged "$LLVM_CONFIG --version" somewhere.) One of them
seems to have clang 5 which is a clue -- if the LLVM is also 5 it's
just not going to work, as LLVM is one of those forwards-only projects
that doesn't back-patch fixes like that.
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-18 11:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 12:06 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 17:20 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 21:48 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 22:12 ` Re: LLVM 16 (opaque pointers) Andres Freund <[email protected]>
2023-10-20 23:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-21 00:45 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
@ 2023-10-21 01:45 ` Tom Lane <[email protected]>
2023-10-22 01:44 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
0 siblings, 1 reply; 21+ messages in thread
From: Tom Lane @ 2023-10-21 01:45 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Mark Wong <[email protected]>; Ronan Dunklau <[email protected]>; Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Dmitry Dolgov <[email protected]>; Andres Freund <[email protected]>
Thomas Munro <[email protected]> writes:
> (It'd be nice if the
> build farm logged "$LLVM_CONFIG --version" somewhere.)
It's not really the buildfarm script's responsibility to do that,
but feel free to make configure do so.
regards, tom lane
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-18 11:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 12:06 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 17:20 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 21:48 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 22:12 ` Re: LLVM 16 (opaque pointers) Andres Freund <[email protected]>
2023-10-20 23:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-21 00:45 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-21 01:45 ` Re: LLVM 16 (opaque pointers) Tom Lane <[email protected]>
@ 2023-10-22 01:44 ` Thomas Munro <[email protected]>
2023-10-23 00:15 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
0 siblings, 1 reply; 21+ messages in thread
From: Thomas Munro @ 2023-10-22 01:44 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Mark Wong <[email protected]>; Ronan Dunklau <[email protected]>; Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Dmitry Dolgov <[email protected]>; Andres Freund <[email protected]>
On Sat, Oct 21, 2023 at 2:45 PM Tom Lane <[email protected]> wrote:
> Thomas Munro <[email protected]> writes:
> > (It'd be nice if the
> > build farm logged "$LLVM_CONFIG --version" somewhere.)
>
> It's not really the buildfarm script's responsibility to do that,
> but feel free to make configure do so.
Done, copying the example of how we do it for perl and various other things.
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-18 11:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 12:06 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 17:20 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 21:48 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 22:12 ` Re: LLVM 16 (opaque pointers) Andres Freund <[email protected]>
2023-10-20 23:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-21 00:45 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-21 01:45 ` Re: LLVM 16 (opaque pointers) Tom Lane <[email protected]>
2023-10-22 01:44 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
@ 2023-10-23 00:15 ` Thomas Munro <[email protected]>
2023-10-23 15:27 ` Re: LLVM 16 (opaque pointers) Mark Wong <[email protected]>
0 siblings, 1 reply; 21+ messages in thread
From: Thomas Munro @ 2023-10-23 00:15 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Mark Wong <[email protected]>; Ronan Dunklau <[email protected]>; Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Dmitry Dolgov <[email protected]>; Andres Freund <[email protected]>
On Sun, Oct 22, 2023 at 2:44 PM Thomas Munro <[email protected]> wrote:
> On Sat, Oct 21, 2023 at 2:45 PM Tom Lane <[email protected]> wrote:
> > Thomas Munro <[email protected]> writes:
> > > (It'd be nice if the
> > > build farm logged "$LLVM_CONFIG --version" somewhere.)
> >
> > It's not really the buildfarm script's responsibility to do that,
> > but feel free to make configure do so.
>
> Done, copying the example of how we do it for perl and various other things.
Build farm measles update:
With that we can see that nicator (LLVM 15 on POWER) is green. We can
see that cavefish (LLVM 6 on POWER) is red as expected. We can also
see that bonito (LLVM 7 on POWER) is red, so my earlier theory that
this might be due to the known bug we got fixed in LLVM 7 is not
enough. Maybe there are other things fixed on POWER somewhere between
those LLVM versions? I suspect it'll be hard to figure out without
debug builds and backtraces.
One thing is definitely our fault, though. xenodermus shows failures
on REL_12_STABLE and REL_13_STABLE, using debug LLVM 6 on x86. I
couldn't reproduce this locally on (newer) debug LLVM, so I bugged
Andres for access to the host/libraries and chased it down. There is
some type punning for a function parameter REL_13_STABLE and earlier,
removed by Andres in REL_14_STABLE, and when I back-patched my
refactoring I effectively back-patched a few changes from his commit
df99ddc70b97 that removed the type punning, but I should have brought
one more line from that commit to remove another trace of it. See
attached.
Attachments:
[application/x-patch] 0001-jit-Fix-LLVM-back-patching-bug-in-12-and-13.patch (1.2K, ../../CA+hUKGLQ38rgZ3bvNHXPRjsWFAg3pa=tnpeq0osa+=miFD5jAw@mail.gmail.com/2-0001-jit-Fix-LLVM-back-patching-bug-in-12-and-13.patch)
download | inline diff:
From 52397f8c70641080f2487ee5f019f143dd35957c Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email protected]>
Date: Sun, 22 Oct 2023 23:20:56 +0000
Subject: [PATCH] jit: Fix LLVM back-patching bug in 12 and 13.
While back-patching f90b4a84, I missed that branches before 14 did some
type punning in a function parameter. That didn't cause any problem for
release builds of LLVM, but debug builds of some older versions would
fail a type cross-check assertion. To fix this, we need to back-patch a
line of df99ddc7.
Per build farm animal xenodermus, which runs a debug build of LLVM 6
with jit_above_cost=0.
diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index d84fbba7cc..c2e367f00d 100644
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -1126,7 +1126,7 @@ llvm_compile_expr(ExprState *state)
llvm_pg_var_type("TypeExecEvalSubroutine"));
v_params[0] = v_state;
- v_params[1] = l_ptr_const(op, l_ptr(TypeSizeT));
+ v_params[1] = l_ptr_const(op, l_ptr(StructExprEvalStep));
v_params[2] = v_econtext;
l_call(b,
LLVMGetFunctionType(ExecEvalSubroutineTemplate),
--
2.42.0
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-18 11:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 12:06 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 17:20 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 21:48 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 22:12 ` Re: LLVM 16 (opaque pointers) Andres Freund <[email protected]>
2023-10-20 23:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-21 00:45 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-21 01:45 ` Re: LLVM 16 (opaque pointers) Tom Lane <[email protected]>
2023-10-22 01:44 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-23 00:15 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
@ 2023-10-23 15:27 ` Mark Wong <[email protected]>
0 siblings, 0 replies; 21+ messages in thread
From: Mark Wong @ 2023-10-23 15:27 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Tom Lane <[email protected]>; Ronan Dunklau <[email protected]>; Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Dmitry Dolgov <[email protected]>; Andres Freund <[email protected]>
On Mon, Oct 23, 2023 at 01:15:04PM +1300, Thomas Munro wrote:
> On Sun, Oct 22, 2023 at 2:44 PM Thomas Munro <[email protected]> wrote:
> > On Sat, Oct 21, 2023 at 2:45 PM Tom Lane <[email protected]> wrote:
> > > Thomas Munro <[email protected]> writes:
> > > > (It'd be nice if the
> > > > build farm logged "$LLVM_CONFIG --version" somewhere.)
> > >
> > > It's not really the buildfarm script's responsibility to do that,
> > > but feel free to make configure do so.
> >
> > Done, copying the example of how we do it for perl and various other things.
>
> Build farm measles update:
>
> With that we can see that nicator (LLVM 15 on POWER) is green. We can
> see that cavefish (LLVM 6 on POWER) is red as expected. We can also
> see that bonito (LLVM 7 on POWER) is red, so my earlier theory that
> this might be due to the known bug we got fixed in LLVM 7 is not
> enough. Maybe there are other things fixed on POWER somewhere between
> those LLVM versions? I suspect it'll be hard to figure out without
> debug builds and backtraces.
I haven't gotten around to disabling llvm on any of my animals with llvm
< 7 yet. Do you still want to hold on that?
> One thing is definitely our fault, though. xenodermus shows failures
> on REL_12_STABLE and REL_13_STABLE, using debug LLVM 6 on x86. I
> couldn't reproduce this locally on (newer) debug LLVM, so I bugged
> Andres for access to the host/libraries and chased it down. There is
> some type punning for a function parameter REL_13_STABLE and earlier,
> removed by Andres in REL_14_STABLE, and when I back-patched my
> refactoring I effectively back-patched a few changes from his commit
> df99ddc70b97 that removed the type punning, but I should have brought
> one more line from that commit to remove another trace of it. See
> attached.
Here are my list of llvm-config versions and distros for s390x and POWER
(didn't see any issues on aarch64 but I grabbed all the info at the same
time.)
s390x:
branta: 10.0.0 Ubuntu 20.04.4 LTS
cotinga: 6.0.0 Ubuntu 18.04.6 LTS
perch: 6.0.0 Ubuntu 18.04.6 LTS
sarus: 14.0.0 Ubuntu 22.04.1 LTS
aracari: 15.0.7 Red Hat Enterprise Linux 8.6
pipit: 15.0.7 Red Hat Enterprise Linux 8.6
lora: 15.0.7 Red Hat Enterprise Linux 9.2
mamushi: 15.0.7 Red Hat Enterprise Linux 9.2
pike: 11.0.1 Debian GNU/Linux 11
rinkhals: 11.0.1 Debian GNU/Linux 11
POWER:
bonito: 7.0.1 Fedora 29
cavefish: 6.0.0 Ubuntu 18.04.6 LTS
demoiselle: 5.0.1 openSUSE Leap 15.0
elasmobranch: 5.0.1 openSUSE Leap 15.0
babbler: 15.0.7 AlmaLinux 8.8
pytilia: 15.0.7 AlmaLinux 8.8
nicator: 15.0.7 AlmaLinux 9.2
twinspot: 15.0.7 AlmaLinux 9.2
cascabel: 11.0.1 Debian GNU/Linux 11
habu: 16.0.6 Fedora Linux 38
kingsnake: 16.0.6 Fedora Linux 38
krait: CentOS 16.0.6 Stream 8
lancehead: CentOS 16.0.6 Stream 8
aarch64:
boiga: 14.0.5 Fedora Linux 36
corzo: 14.0.5 Fedora Linux 36
desman: 16.0.6 Fedora Linux 38
motmot: 16.0.6 Fedora Linux 38
whinchat: 11.0.1 Debian GNU/Linux 11
jackdaw: 11.0.1 Debian GNU/Linux 11
blackneck: 7.0.1 Debian GNU/Linux 10
alimoche: 7.0.1 Debian GNU/Linux 10
bulbul: 15.0.7 AlmaLinux 8.8
broadbill: 15.0.7 AlmaLinux 8.8
oystercatcher: 15.0.7 AlmaLinux 9.2
potoo: 15.0.7 AlmaLinux 9.2
whiting: 6.0.0 Ubuntu 18.04.5 LTS
vimba: 6.0.0 Ubuntu 18.04.5 LTS
splitfin: 10.0.0 Ubuntu 20.04.6 LTS
rudd: 10.0.0 Ubuntu 20.04.6 LTS
turbot: 14.0.0 Ubuntu 22.04.3 LTS
shiner: 14.0.0 Ubuntu 22.04.3 LTS
ziege: 16.0.6 CentOS Stream 8
chevrotain: 11.0.1 Debian GNU/Linux 11
Regards,
Mark
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-18 11:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 12:06 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 17:20 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
@ 2023-10-21 06:08 ` Andres Freund <[email protected]>
2023-10-21 23:16 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2 siblings, 1 reply; 21+ messages in thread
From: Andres Freund @ 2023-10-21 06:08 UTC (permalink / raw)
To: Thomas Munro <[email protected]>; +Cc: Ronan Dunklau <[email protected]>; Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Dmitry Dolgov <[email protected]>; Tom Stellard <[email protected]>; Mark Wong <[email protected]>
Hi,
On 2023-10-19 06:20:26 +1300, Thomas Munro wrote:
> Interestingly, a new problem just showed up on the the RHEL9 s390x
> machine "lora", where a previously reported problem [1] apparently
> re-appeared. It complains about incompatible layout, previously
> blamed on mismatch between clang and LLVM versions.
I've attached a patch revision that I spent the last couple hours working
on. It's very very roughly based on a patch Tom Stellard had written (which I
think a few rpm packages use). But instead of encoding details about specific
layout details, I made the code check if the data layout works and fall back
to the cpu / features used for llvmjit_types.bc. This way it's not s390x
specific, future odd architecture behaviour would "automatically" be handled
the same.
With that at least the main regression tests pass on s390x, even with
jit_above_cost=0.
> I can see that its clang is v15 from clues in the conflig log, but I don't
> know which version of LLVM is being used. However, I see now that
> --with-llvm was literally just turned on, so there is no reason to think
> that this would have worked before or this work is relevant. Strange though
> -- we must be able to JIT further than that on s390x because we have crash
> reports in other threads (ie we made it past this and into other more
> advanced brokenness).
You can avoid the borkedness by a) running on an older cpu b) adding
compilation flags to change the code generation target
(e.g. -march=native). And some RPM packages have applied the patch by Tom
Stellard.
> [1] https://www.postgresql.org/message-id/flat/20210319190047.7o4bwhbp5dzkqif3%40alap3.anarazel.de#ec51b...
Greetings,
Andres Freund
Attachments:
[text/x-diff] v2-0001-jit-Add-fallback-in-case-of-runtime-compile-time-.patch (9.0K, ../../[email protected]/2-v2-0001-jit-Add-fallback-in-case-of-runtime-compile-time-.patch)
download | inline diff:
From e7ab9eb11576ef85e4d2f6e1ade0a6028279634d Mon Sep 17 00:00:00 2001
From: Andres Freund <[email protected]>
Date: Fri, 20 Oct 2023 23:03:53 -0700
Subject: [PATCH v2] jit: Add fallback in case of runtime/compile time ABI
mismatch
LLVM's s390x target uses a different ABI (called data layout in LLVM) for z13
and newer processors. If IR files (like llvmjit_types.bc) are compiled to
target a processor older than z13, which is the default, and JIT occurs on a
z13 or newer processor, the ABI mismatch will cause JIT to fail at runtime.
To deal with that, check if data layouts match during JIT initialization. If
the runtime detected cpu / features result in a different layout, try if the
cpu/features recorded in in llvmjit_types.bc work.
Author: Andres Freund <[email protected]>
Author: Tom Stellard <[email protected]> (in an older version)
Discussion: [email protected]
Discussion: CA+hUKG+hopeRBGVwS4JE3uy1UO3TRnhF08HMiSpMrPODgtiw9Q@mail.gmail.com
---
src/backend/jit/llvm/llvmjit.c | 191 ++++++++++++++++++++++++++++-----
1 file changed, 166 insertions(+), 25 deletions(-)
diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c
index 58f638859a4..fa2a982991d 100644
--- a/src/backend/jit/llvm/llvmjit.c
+++ b/src/backend/jit/llvm/llvmjit.c
@@ -101,6 +101,8 @@ static size_t llvm_jit_context_in_use_count = 0;
static size_t llvm_llvm_context_reuse_count = 0;
static const char *llvm_triple = NULL;
static const char *llvm_layout = NULL;
+static char *llvm_cpu = NULL;
+static char *llvm_cpu_features = NULL;
static LLVMContextRef llvm_context;
@@ -123,6 +125,7 @@ static void llvm_optimize_module(LLVMJitContext *context, LLVMModuleRef module);
static void llvm_create_types(void);
static void llvm_set_target(void);
+static void llvm_set_cpu_and_features(void);
static void llvm_recreate_llvm_context(void);
static uint64_t llvm_resolve_symbol(const char *name, void *ctx);
@@ -884,9 +887,6 @@ static void
llvm_session_initialize(void)
{
MemoryContext oldcontext;
- char *error = NULL;
- char *cpu = NULL;
- char *features = NULL;
LLVMTargetMachineRef opt0_tm;
LLVMTargetMachineRef opt3_tm;
@@ -931,38 +931,21 @@ llvm_session_initialize(void)
*/
llvm_set_target();
- if (LLVMGetTargetFromTriple(llvm_triple, &llvm_targetref, &error) != 0)
- {
- elog(FATAL, "failed to query triple %s", error);
- }
-
- /*
- * We want the generated code to use all available features. Therefore
- * grab the host CPU string and detect features of the current CPU. The
- * latter is needed because some CPU architectures default to enabling
- * features not all CPUs have (weird, huh).
- */
- cpu = LLVMGetHostCPUName();
- features = LLVMGetHostCPUFeatures();
- elog(DEBUG2, "LLVMJIT detected CPU \"%s\", with features \"%s\"",
- cpu, features);
+ llvm_set_cpu_and_features();
opt0_tm =
- LLVMCreateTargetMachine(llvm_targetref, llvm_triple, cpu, features,
+ LLVMCreateTargetMachine(llvm_targetref, llvm_triple,
+ llvm_cpu, llvm_cpu_features,
LLVMCodeGenLevelNone,
LLVMRelocDefault,
LLVMCodeModelJITDefault);
opt3_tm =
- LLVMCreateTargetMachine(llvm_targetref, llvm_triple, cpu, features,
+ LLVMCreateTargetMachine(llvm_targetref, llvm_triple,
+ llvm_cpu, llvm_cpu_features,
LLVMCodeGenLevelAggressive,
LLVMRelocDefault,
LLVMCodeModelJITDefault);
- LLVMDisposeMessage(cpu);
- cpu = NULL;
- LLVMDisposeMessage(features);
- features = NULL;
-
/* force symbols in main binary to be loaded */
LLVMLoadLibraryPermanently(NULL);
@@ -1092,6 +1075,73 @@ load_return_type(LLVMModuleRef mod, const char *name)
return typ;
}
+/*
+ * Copies a string that needs to be freed with LLVMDisposeMessage() and then
+ * frees the source string.
+ */
+static char *
+llvm_to_pg_str(char *str)
+{
+ char *ret = pstrdup(str);
+
+ LLVMDisposeMessage(str);
+
+ return ret;
+}
+
+/*
+ * Return data layout for a target machine created with cpu and features
+ *
+ * The return value is a palloc'd string.
+ */
+static char *
+determine_data_layout(const char *cpu, const char *features)
+{
+ LLVMTargetMachineRef tm;
+ LLVMTargetDataRef layout;
+ char *layout_str;
+
+ tm = LLVMCreateTargetMachine(llvm_targetref, llvm_triple, cpu, features,
+ LLVMCodeGenLevelNone,
+ LLVMRelocDefault,
+ LLVMCodeModelJITDefault);
+ layout = LLVMCreateTargetDataLayout(tm);
+ layout_str = LLVMCopyStringRepOfTargetData(layout);
+
+ LLVMDisposeTargetData(layout);
+ LLVMDisposeTargetMachine(tm);
+
+ return llvm_to_pg_str(layout_str);
+}
+
+/*
+ * Convenience wrapper around LLVMGetStringAttributeAtIndex &
+ * LLVMGetStringAttributeValue.
+ *
+ * The return value is a zero-terminated, palloc'd string.
+ */
+static char *
+get_string_attribute_value(LLVMValueRef v, uint32 index,
+ const char *name, const char *fallback)
+{
+ LLVMAttributeRef attr;
+ const char *val;
+ unsigned len;
+
+ attr = LLVMGetStringAttributeAtIndex(v, index, name, strlen(name));
+ if (!attr)
+ return fallback ? pstrdup(fallback) : NULL;
+
+ val = LLVMGetStringAttributeValue(attr, &len);
+
+ /*
+ * LLVMGetStringAttributeValue() returns values not zero terminated, which
+ * inconvenient to work with. Also has the advantage that the return value
+ * is freed by memory context cleanup etc.
+ */
+ return psprintf("%.*s", len, val);
+}
+
/*
* Load triple & layout from clang emitted file so we're guaranteed to be
* compatible.
@@ -1099,14 +1149,105 @@ load_return_type(LLVMModuleRef mod, const char *name)
static void
llvm_set_target(void)
{
+ char *error = NULL;
+
if (!llvm_types_module)
elog(ERROR, "failed to extract target information, llvmjit_types.c not loaded");
+ /* can get called again after partial initialization */
+
if (llvm_triple == NULL)
llvm_triple = pstrdup(LLVMGetTarget(llvm_types_module));
if (llvm_layout == NULL)
llvm_layout = pstrdup(LLVMGetDataLayoutStr(llvm_types_module));
+
+ if (llvm_targetref == NULL)
+ {
+ if (LLVMGetTargetFromTriple(llvm_triple, &llvm_targetref, &error) != 0)
+ elog(FATAL, "failed to query triple %s", error);
+ }
+}
+
+/*
+ * Determine CPU and features to use for JIT.
+ *
+ * We want the generated code to use all available features. Therefore
+ * grab the host CPU string and detect features of the current CPU. The
+ * latter is needed because some CPU architectures default to enabling
+ * features not all CPUs have (weird, huh).
+ *
+ * Unfortunately there is at least one architecture on which LLVM doesn't play
+ * fair - on s390, LLVM will use a different ABI for the same triple,
+ * depending on host CPU (IMO not a sane decision, but ...). To work around
+ * that, if the layout of llvmjit_types.bc does not match what we get using
+ * the host cpu / features, try target-cpu/target-features that clang recorded
+ * in llvmjit_types.bc at compile time.
+ */
+static void
+llvm_set_cpu_and_features(void)
+{
+ char *host_cpu;
+ char *host_cpu_features;
+ char *host_layout_str;
+
+ /* can get called again after partial initialization */
+ if (llvm_cpu != NULL)
+ return;
+
+ /* determine runtime CPU / feature */
+ host_cpu = llvm_to_pg_str(LLVMGetHostCPUName());
+ host_cpu_features = llvm_to_pg_str(LLVMGetHostCPUFeatures());
+ host_layout_str = determine_data_layout(host_cpu, host_cpu_features);
+
+ elog(DEBUG2, "detected CPU \"%s\", with features \"%s\", resulting in layout \"%s\"",
+ host_cpu, host_cpu_features, host_layout_str);
+
+ /* check if we can use detected values or if we need to fall back */
+ if (strcmp(host_layout_str, llvm_layout) == 0)
+ {
+ llvm_cpu = host_cpu;
+ llvm_cpu_features = host_cpu_features;
+ pfree(host_layout_str);
+ }
+ else
+ {
+ char *module_cpu;
+ char *module_cpu_features;
+ char *module_layout_str;
+
+ /* incompatible, try to fall back to module cpu / features */
+
+ module_cpu = get_string_attribute_value(AttributeTemplate,
+ LLVMAttributeFunctionIndex,
+ "target-cpu", "generic");
+ module_cpu_features = get_string_attribute_value(AttributeTemplate,
+ LLVMAttributeFunctionIndex,
+ "target-features", "");
+ module_layout_str = determine_data_layout(module_cpu, module_cpu_features);
+
+ if (strcmp(module_layout_str, llvm_layout) != 0)
+ {
+ /* leaking a few strings, this isn't expected to ever be hit */
+ ereport(ERROR,
+ errmsg_internal("could not determine working CPU / feature comination for JIT compilation"),
+ errdetail_internal("compile time data layout: \"%s\", host layout \"%s\", fallback layout \"%s\"",
+ llvm_layout, host_layout_str, module_layout_str));
+ }
+
+ llvm_cpu = module_cpu;
+ llvm_cpu_features = module_cpu_features;
+
+ ereport(DEBUG2,
+ errmsg_internal("detected CPU / features yield incompatible data layout, using values from module instead"),
+ errdetail_internal("module CPU \"%s\", features \"%s\", resulting in layout \"%s\"",
+ module_cpu, module_cpu_features, module_layout_str));
+
+ pfree(host_cpu);
+ pfree(host_cpu_features);
+ pfree(host_layout_str);
+ pfree(module_layout_str);
+ }
}
/*
--
2.38.0
^ permalink raw reply [nested|flat] 21+ messages in thread
* Re: LLVM 16 (opaque pointers)
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-18 11:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 12:06 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 17:20 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-21 06:08 ` Re: LLVM 16 (opaque pointers) Andres Freund <[email protected]>
@ 2023-10-21 23:16 ` Thomas Munro <[email protected]>
0 siblings, 0 replies; 21+ messages in thread
From: Thomas Munro @ 2023-10-21 23:16 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Ronan Dunklau <[email protected]>; Devrim Gündüz <[email protected]>; PostgreSQL Hackers <[email protected]>; Fabien COELHO <[email protected]>; Dmitry Dolgov <[email protected]>; Tom Stellard <[email protected]>; Mark Wong <[email protected]>
On Sat, Oct 21, 2023 at 7:08 PM Andres Freund <[email protected]> wrote:
> I've attached a patch revision that I spent the last couple hours working
> on. It's very very roughly based on a patch Tom Stellard had written (which I
> think a few rpm packages use). But instead of encoding details about specific
> layout details, I made the code check if the data layout works and fall back
> to the cpu / features used for llvmjit_types.bc. This way it's not s390x
> specific, future odd architecture behaviour would "automatically" be handled
> the same
The explanation makes sense and this seems like a solid plan to deal
with it. I didn't try on a s390x, but I tested locally on our master
branch with LLVM 7, 10, 17, 18, and then I hacked your patch to take
the fallback path as if a layout mismatch had been detected, and it
worked fine:
2023-10-22 11:49:55.663 NZDT [12000] DEBUG: detected CPU "skylake",
with features "...", resulting in layout
"e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
2023-10-22 11:49:55.664 NZDT [12000] DEBUG: detected CPU / features
yield incompatible data layout, using values from module instead
2023-10-22 11:49:55.664 NZDT [12000] DETAIL: module CPU "x86-64",
features "...", resulting in layout
"e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+ To deal with that, check if data layouts match during JIT
initialization. If
+ the runtime detected cpu / features result in a different layout,
try if the
+ cpu/features recorded in in llvmjit_types.bc work.
s|try |check |
s| in in | in |
+ errmsg_internal("could not
determine working CPU / feature comination for JIT compilation"),
s|comination|combination|
s| / |/|g
^ permalink raw reply [nested|flat] 21+ messages in thread
end of thread, other threads:[~2023-10-23 15:27 UTC | newest]
Thread overview: 21+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-04-01 01:35 [PATCH 4/4] Implement vacuum full/cluster (INDEX_TABLESPACE <tablespace>) Justin Pryzby <[email protected]>
2023-10-11 09:31 Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-13 20:32 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-16 08:31 ` Re: LLVM 16 (opaque pointers) Ronan Dunklau <[email protected]>
2023-10-18 11:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 12:06 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 17:20 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-18 18:11 ` Re: LLVM 16 (opaque pointers) Andres Freund <[email protected]>
2023-10-20 21:48 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 22:00 ` Re: LLVM 16 (opaque pointers) Mark Wong <[email protected]>
2023-10-20 22:03 ` Re: LLVM 16 (opaque pointers) Tom Lane <[email protected]>
2023-10-20 22:12 ` Re: LLVM 16 (opaque pointers) Andres Freund <[email protected]>
2023-10-20 23:02 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-20 23:07 ` Re: LLVM 16 (opaque pointers) Andres Freund <[email protected]>
2023-10-21 00:45 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-21 01:45 ` Re: LLVM 16 (opaque pointers) Tom Lane <[email protected]>
2023-10-22 01:44 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-23 00:15 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[email protected]>
2023-10-23 15:27 ` Re: LLVM 16 (opaque pointers) Mark Wong <[email protected]>
2023-10-21 06:08 ` Re: LLVM 16 (opaque pointers) Andres Freund <[email protected]>
2023-10-21 23:16 ` Re: LLVM 16 (opaque pointers) Thomas Munro <[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