public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v23 5/5] Implement vacuum full/cluster (INDEX_TABLESPACE <tablespace>)
4+ messages / 3 participants
[nested] [flat]
* [PATCH v23 5/5] Implement vacuum full/cluster (INDEX_TABLESPACE <tablespace>)
@ 2020-04-01 01:35 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 4+ 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 | 65 ++++++++++++++---------
src/backend/commands/matview.c | 3 +-
src/backend/commands/tablecmds.c | 2 +-
src/backend/commands/vacuum.c | 47 +++++++---------
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, 124 insertions(+), 60 deletions(-)
diff --git a/doc/src/sgml/ref/cluster.sgml b/doc/src/sgml/ref/cluster.sgml
index 5274b1af7e..076849a76a 100644
--- a/doc/src/sgml/ref/cluster.sgml
+++ b/doc/src/sgml/ref/cluster.sgml
@@ -28,6 +28,7 @@ CLUSTER [VERBOSE] [ ( <replaceable class="parameter">option</replaceable> [, ...
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] [ ( <replaceable class="parameter">option</replaceable> [, ...
</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><replaceable class="parameter">table_name</replaceable></term>
<listitem>
@@ -142,7 +152,7 @@ CLUSTER [VERBOSE] [ ( <replaceable class="parameter">option</replaceable> [, ...
<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 2408b59bb2..6461746968 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 e94f73414f..602e3cf7f5 100644
--- a/src/backend/commands/cluster.c
+++ b/src/backend/commands/cluster.c
@@ -71,7 +71,8 @@ typedef struct
static void rebuild_relation(Relation OldHeap, Oid indexOid,
- Oid NewTableSpaceOid, bool isTopLevel, bool verbose);
+ Oid NewTableSpaceOid, Oid NewIdxTableSpaceOid,
+ bool isTopLevel, bool verbose);
static void copy_table_data(Oid OIDNewHeap, Oid OIDOldHeap, Oid OIDOldIndex,
bool isTopLevel, bool verbose,
bool *pSwapToastByContent,
@@ -109,9 +110,11 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
{
ListCell *lc;
int options = 0;
- /* Name and Oid of tablespace to use for clustered relation. */
- char *tablespaceName = NULL;
- Oid tablespaceOid = InvalidOid;
+ /* Name and Oid of tablespaces to use for clustered relations. */
+ char *tablespaceName = NULL,
+ *idxtablespaceName = NULL;
+ Oid tablespaceOid,
+ idxtablespaceOid;
/* Parse list of generic parameters not handled by the parser */
foreach(lc, stmt->params)
@@ -125,6 +128,8 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
options &= ~CLUOPT_VERBOSE;
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),
@@ -133,18 +138,11 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
parser_errposition(pstate, opt->location)));
}
- /* Select tablespace Oid to use. */
- if (tablespaceName)
- {
- 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)));
- }
+ /* Get tablespaces to use. */
+ tablespaceOid = tablespaceName ?
+ get_tablespace_oid(tablespaceName, false) : InvalidOid;
+ idxtablespaceOid = idxtablespaceName ?
+ get_tablespace_oid(idxtablespaceName, false) : InvalidOid;
if (stmt->relation != NULL)
{
@@ -215,7 +213,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
table_close(rel, NoLock);
/* Do the job. */
- cluster_rel(tableOid, indexOid, tablespaceOid, options, isTopLevel);
+ cluster_rel(tableOid, indexOid, tablespaceOid, idxtablespaceOid, options, isTopLevel);
}
else
{
@@ -264,7 +262,7 @@ cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel)
PushActiveSnapshot(GetTransactionSnapshot());
/* Do the job. */
cluster_rel(rvtc->tableOid, rvtc->indexOid, tablespaceOid,
- options | CLUOPT_RECHECK,
+ idxtablespaceOid, options | CLUOPT_RECHECK,
isTopLevel);
PopActiveSnapshot();
CommitTransactionCommand();
@@ -295,11 +293,12 @@ 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, Oid tablespaceOid, int options, bool isTopLevel)
+cluster_rel(Oid tableOid, Oid indexOid, Oid tablespaceOid, Oid idxtablespaceOid, int options, bool isTopLevel)
{
Relation OldHeap;
bool verbose = ((options & CLUOPT_VERBOSE) != 0);
@@ -466,7 +465,7 @@ cluster_rel(Oid tableOid, Oid indexOid, Oid tablespaceOid, int options, bool isT
TransferPredicateLocksToHeapRelation(OldHeap);
/* rebuild_relation does all the dirty work */
- rebuild_relation(OldHeap, indexOid, tablespaceOid, isTopLevel, verbose);
+ rebuild_relation(OldHeap, indexOid, tablespaceOid, idxtablespaceOid, isTopLevel, verbose);
/* NB: rebuild_relation does table_close() on OldHeap */
@@ -616,10 +615,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, Oid NewTablespaceOid, bool isTopLevel, bool verbose)
+rebuild_relation(Relation OldHeap, Oid indexOid, Oid NewTablespaceOid, Oid NewIdxTablespaceOid, bool isTopLevel, bool verbose)
{
Oid tableOid = RelationGetRelid(OldHeap);
Oid tableSpace = OldHeap->rd_rel->reltablespace;
+ Oid idxtableSpace;
Oid OIDNewHeap;
char relpersistence;
bool is_system_catalog;
@@ -629,7 +629,20 @@ rebuild_relation(Relation OldHeap, Oid indexOid, Oid NewTablespaceOid, bool isTo
/* 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))
@@ -658,7 +671,7 @@ rebuild_relation(Relation OldHeap, Oid indexOid, Oid NewTablespaceOid, bool isTo
finish_heap_swap(tableOid, OIDNewHeap, is_system_catalog,
swap_toast_by_content, false, true,
frozenXid, cutoffMulti,
- relpersistence);
+ relpersistence, idxtableSpace);
}
@@ -1407,7 +1420,7 @@ 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];
@@ -1469,7 +1482,7 @@ finish_heap_swap(Oid OIDOldHeap, Oid OIDNewHeap,
pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
PROGRESS_CLUSTER_PHASE_REBUILD_INDEX);
- reindex_relation(OIDOldHeap, InvalidOid, reindex_flags, 0);
+ reindex_relation(OIDOldHeap, idxtableSpace, reindex_flags, 0);
/* Report that we are now doing clean up */
pgstat_progress_update_param(PROGRESS_CLUSTER_PHASE,
diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c
index f80a9e96a9..68ea07cae5 100644
--- a/src/backend/commands/matview.c
+++ b/src/backend/commands/matview.c
@@ -844,7 +844,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 c1f5054042..d81a5536e8 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5053,7 +5053,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 07d50bea0d..5e8433600f 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_FEATURE_NOT_SUPPORTED),
- errmsg("incompatible TABLESPACE option"),
- errdetail("You can only use TABLESPACE 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_FEATURE_NOT_SUPPORTED),
+ errmsg("incompatible TABLESPACE option"),
+ errdetail("You can only use TABLESPACE 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.
@@ -1755,8 +1749,7 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
Relation onerel;
LockRelId onerelid;
Oid toast_relid,
- save_userid,
- tablespaceOid = InvalidOid;
+ save_userid;
int save_sec_context;
int save_nestlevel;
@@ -1900,14 +1893,13 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
OidIsValid(params->tablespace_oid) &&
IsSystemRelation(onerel) && !allowSystemTableMods)
{
+ params->tablespace_oid = InvalidOid;
ereport(WARNING,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("skipping tablespace change of \"%s\"",
RelationGetRelationName(onerel)),
errdetail("Cannot move system relation, only VACUUM is performed.")));
}
- else
- tablespaceOid = params->tablespace_oid;
/*
* Get a session-level lock too. This will protect our access to the
@@ -1978,7 +1970,8 @@ vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params)
cluster_options |= CLUOPT_VERBOSE;
/* VACUUM FULL is now a variant of CLUSTER; see cluster.c */
- cluster_rel(relid, InvalidOid, tablespaceOid, cluster_options, true);
+ cluster_rel(relid, InvalidOid, params->tablespace_oid,
+ params->idxtablespace_oid, cluster_options, true);
}
else
table_relation_vacuum(onerel, params, vac_strategy);
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index e231ce81ec..cc12c59ca4 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -2901,6 +2901,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 a64b7e84da..68b831af21 100644
--- a/src/include/commands/cluster.h
+++ b/src/include/commands/cluster.h
@@ -21,6 +21,7 @@
extern void cluster(ParseState *pstate, ClusterStmt *stmt, bool isTopLevel);
extern void cluster_rel(Oid tableOid, Oid indexOid, Oid tablespaceOid,
+ Oid indextablespaceOid,
int options, bool isTopLevel);
extern void check_index_is_clusterable(Relation OldHeap, Oid indexOid,
bool recheck, LOCKMODE lockmode);
@@ -35,6 +36,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 b6e944d3a5..a5d0b31ab2 100644
--- a/src/include/commands/vacuum.h
+++ b/src/include/commands/vacuum.h
@@ -229,8 +229,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 eb9dfcc0f4..4cf79ab4bf 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 789a0e56cc..bc72406b8e 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
--mP3DRpeJDSE+ciuQ--
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: benchmark results comparing versions 15.2 and 16
@ 2023-05-08 19:11 Andres Freund <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Andres Freund @ 2023-05-08 19:11 UTC (permalink / raw)
To: Alexander Lakhin <[email protected]>; Michael Paquier <[email protected]>; +Cc: MARK CALLAGHAN <[email protected]>; [email protected]
Hi,
On 2023-05-08 16:00:01 +0300, Alexander Lakhin wrote:
> This difference is confirmed by multiple test runs. `git bisect` for this
> regression pointed at f193883fc.
I can reproduce a significant regression due to f193883fc of a workload just
running
SELECT CURRENT_TIMESTAMP;
A single session running it on my workstation via pgbench -Mprepared gets
before:
tps = 89359.128359 (without initial connection time)
after:
tps = 83843.585152 (without initial connection time)
Obviously this is an extreme workload, but that nevertheless seems too large
to just accept...
Michael, the commit message notes that there were no measured performance
regression - yet I see one in a trivial test. What were you measuring?
I'm a bit surprised by the magnitude of the regression, but it's not
surprising that there is a performance effect. You're replacing something that
doesn't go through the whole generic function rigamarole, and replace it with
something that does...
Looking at two perf profiles, the biggest noticable difference is
Before:
- 5.51% 0.13% postgres postgres [.] ExecInitResult
- 5.38% ExecInitResult
+ 2.29% ExecInitResultTupleSlotTL
- 2.22% ExecAssignProjectionInfo
- 2.19% ExecBuildProjectionInfo
0.47% ExecReadyInterpretedExpr
- 0.43% ExecInitExprRec
- 0.10% palloc
AllocSetAlloc.localalias (inlined)
+ 0.32% expression_tree_walker_impl.localalias (inlined)
+ 0.28% get_typlen
0.09% ExecPushExprSlots
+ 0.06% MemoryContextAllocZeroAligned
+ 0.04% MemoryContextAllocZeroAligned
0.02% exprType.localalias (inlined)
+ 0.41% ExecAssignExprContext
+ 0.35% MemoryContextAllocZeroAligned
0.11% ExecInitQual.localalias (inlined)
+ 0.11% _start
+ 0.02% 0x55b89c764d7f
After:
- 6.57% 0.17% postgres postgres [.] ExecInitResult
- 6.40% ExecInitResult
- 3.00% ExecAssignProjectionInfo
- ExecBuildProjectionInfo
- 0.91% ExecInitExprRec
- 0.65% ExecInitFunc
0.23% fmgr_info_cxt_security
+ 0.18% palloc0
+ 0.07% object_aclcheck
0.04% fmgr_info
0.05% check_stack_depth
+ 0.05% palloc
+ 0.58% expression_tree_walker_impl.localalias (inlined)
+ 0.55% get_typlen
0.37% ExecReadyInterpretedExpr
+ 0.11% MemoryContextAllocZeroAligned
0.09% ExecPushExprSlots
0.04% exprType.localalias (inlined)
+ 2.77% ExecInitResultTupleSlotTL
+ 0.50% ExecAssignExprContext
+ 0.09% MemoryContextAllocZeroAligned
0.05% ExecInitQual.localalias (inlined)
+ 0.10% _start
I.e. we spend more time building the expression state for expression
evaluation, because we now go through the generic ExecInitFunc(), instead of
something dedicated. We also now need to do permission checking etc.
I don't think that's the entirety of the regression...
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: benchmark results comparing versions 15.2 and 16
@ 2023-05-09 16:48 Andres Freund <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Andres Freund @ 2023-05-09 16:48 UTC (permalink / raw)
To: Alexander Lakhin <[email protected]>; Michael Paquier <[email protected]>; +Cc: MARK CALLAGHAN <[email protected]>; [email protected]
Hi,
On 2023-05-08 12:11:17 -0700, Andres Freund wrote:
> Hi,
>
> On 2023-05-08 16:00:01 +0300, Alexander Lakhin wrote:
> > This difference is confirmed by multiple test runs. `git bisect` for this
> > regression pointed at f193883fc.
>
> I can reproduce a significant regression due to f193883fc of a workload just
> running
> SELECT CURRENT_TIMESTAMP;
>
> A single session running it on my workstation via pgbench -Mprepared gets
> before:
> tps = 89359.128359 (without initial connection time)
> after:
> tps = 83843.585152 (without initial connection time)
>
> Obviously this is an extreme workload, but that nevertheless seems too large
> to just accept...
Added an open item for this.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: benchmark results comparing versions 15.2 and 16
@ 2023-05-11 04:28 Michael Paquier <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Michael Paquier @ 2023-05-11 04:28 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; Michael Paquier <[email protected]>; MARK CALLAGHAN <[email protected]>; [email protected]
On Tue, May 09, 2023 at 09:48:24AM -0700, Andres Freund wrote:
> On 2023-05-08 12:11:17 -0700, Andres Freund wrote:
>> I can reproduce a significant regression due to f193883fc of a workload just
>> running
>> SELECT CURRENT_TIMESTAMP;
>>
>> A single session running it on my workstation via pgbench -Mprepared gets
>> before:
>> tps = 89359.128359 (without initial connection time)
>> after:
>> tps = 83843.585152 (without initial connection time)
>>
>> Obviously this is an extreme workload, but that nevertheless seems too large
>> to just accept...
>
> Added an open item for this.
Thanks for the report, I'll come back to it and look at it at the
beginning of next week. In the worst case, that would mean a revert
of this refactoring, I assume.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2023-05-11 04:28 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-04-01 01:35 [PATCH v23 5/5] Implement vacuum full/cluster (INDEX_TABLESPACE <tablespace>) Justin Pryzby <[email protected]>
2023-05-08 19:11 Re: benchmark results comparing versions 15.2 and 16 Andres Freund <[email protected]>
2023-05-09 16:48 ` Re: benchmark results comparing versions 15.2 and 16 Andres Freund <[email protected]>
2023-05-11 04:28 ` Re: benchmark results comparing versions 15.2 and 16 Michael Paquier <[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