On Wed, 2024-11-27 at 00:08 -0500, Corey Huinker wrote:
>
> 0003 - Re-enabling in-place updates because catalog bloat bad.
Attached is my version of this patch, which I intend to commit soon.
I added docs and tests, and I refactored a bit to check the arguments
first.
Also, I separated the mvcc and in-place paths, so that it was easier to
review that each one is following the right protocol.
Regards,
Jeff Davis
From a41503b82469297797db78163426c40a7a86317f Mon Sep 17 00:00:00 2001
From: Jeff Davis <j...@j-davis.com>
Date: Fri, 6 Dec 2024 12:32:29 -0800
Subject: [PATCH v33j] Use in-place updates for pg_restore_relation_stats().
This matches the behavior of vac_update_relstats(), which is important
to avoid bloating pg_class.
Author: Corey Huinker
Discussion: https://postgr.es/m/CADkLM=fc3je+ufv3gshqjjssf+t8674rxpuxw62el55mueq...@mail.gmail.com
---
doc/src/sgml/func.sgml | 8 +
src/backend/statistics/relation_stats.c | 198 +++++++++++++--------
src/test/regress/expected/stats_import.out | 61 +++++++
src/test/regress/sql/stats_import.sql | 37 ++++
4 files changed, 233 insertions(+), 71 deletions(-)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 8b81106fa23..2c35252dc06 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -30175,6 +30175,14 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
function is to maintain a consistent function signature to avoid
errors when restoring statistics from previous versions.
</para>
+ <para>
+ To match the behavior of <xref linkend="sql-vacuum"/> and <xref
+ linkend="sql-analyze"/> when updating relation statistics,
+ <function>pg_restore_relation_stats()</function> does not follow MVCC
+ transactional semantics (see <xref linkend="mvcc"/>). New relation
+ statistics may be durable even if the transaction aborts, and the
+ changes are not isolated from other transactions.
+ </para>
<para>
Arguments are passed as pairs of <replaceable>argname</replaceable>
and <replaceable>argvalue</replaceable>, where
diff --git a/src/backend/statistics/relation_stats.c b/src/backend/statistics/relation_stats.c
index e619d5cf5b1..f84157b16ab 100644
--- a/src/backend/statistics/relation_stats.c
+++ b/src/backend/statistics/relation_stats.c
@@ -20,6 +20,7 @@
#include "access/heapam.h"
#include "catalog/indexing.h"
#include "statistics/stat_utils.h"
+#include "utils/fmgroids.h"
#include "utils/fmgrprotos.h"
#include "utils/syscache.h"
@@ -50,59 +51,28 @@ static struct StatsArgInfo relarginfo[] =
[NUM_RELATION_STATS_ARGS] = {0}
};
-static bool relation_statistics_update(FunctionCallInfo fcinfo, int elevel);
+static bool relation_statistics_update(FunctionCallInfo fcinfo, int elevel,
+ bool inplace);
/*
* Internal function for modifying statistics for a relation.
*/
static bool
-relation_statistics_update(FunctionCallInfo fcinfo, int elevel)
+relation_statistics_update(FunctionCallInfo fcinfo, int elevel, bool inplace)
{
Oid reloid;
Relation crel;
- HeapTuple ctup;
- Form_pg_class pgcform;
- int replaces[3] = {0};
- Datum values[3] = {0};
- bool nulls[3] = {0};
- int ncols = 0;
- TupleDesc tupdesc;
+ int32 relpages = DEFAULT_RELPAGES;
+ bool update_relpages = false;
+ float reltuples = DEFAULT_RELTUPLES;
+ bool update_reltuples = false;
+ int32 relallvisible = DEFAULT_RELALLVISIBLE;
+ bool update_relallvisible = false;
bool result = true;
- stats_check_required_arg(fcinfo, relarginfo, RELATION_ARG);
- reloid = PG_GETARG_OID(RELATION_ARG);
-
- if (RecoveryInProgress())
- ereport(ERROR,
- (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("recovery is in progress"),
- errhint("Statistics cannot be modified during recovery.")));
-
- stats_lock_check_privileges(reloid);
-
- /*
- * Take RowExclusiveLock on pg_class, consistent with
- * vac_update_relstats().
- */
- crel = table_open(RelationRelationId, RowExclusiveLock);
-
- tupdesc = RelationGetDescr(crel);
- ctup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid));
- if (!HeapTupleIsValid(ctup))
- {
- ereport(elevel,
- (errcode(ERRCODE_OBJECT_IN_USE),
- errmsg("pg_class entry for relid %u not found", reloid)));
- table_close(crel, RowExclusiveLock);
- return false;
- }
-
- pgcform = (Form_pg_class) GETSTRUCT(ctup);
-
- /* relpages */
if (!PG_ARGISNULL(RELPAGES_ARG))
{
- int32 relpages = PG_GETARG_INT32(RELPAGES_ARG);
+ relpages = PG_GETARG_INT32(RELPAGES_ARG);
/*
* Partitioned tables may have relpages=-1. Note: for relations with
@@ -116,17 +86,13 @@ relation_statistics_update(FunctionCallInfo fcinfo, int elevel)
errmsg("relpages cannot be < -1")));
result = false;
}
- else if (relpages != pgcform->relpages)
- {
- replaces[ncols] = Anum_pg_class_relpages;
- values[ncols] = Int32GetDatum(relpages);
- ncols++;
- }
+ else
+ update_relpages = true;
}
if (!PG_ARGISNULL(RELTUPLES_ARG))
{
- float reltuples = PG_GETARG_FLOAT4(RELTUPLES_ARG);
+ reltuples = PG_GETARG_FLOAT4(RELTUPLES_ARG);
if (reltuples < -1.0)
{
@@ -135,18 +101,13 @@ relation_statistics_update(FunctionCallInfo fcinfo, int elevel)
errmsg("reltuples cannot be < -1.0")));
result = false;
}
- else if (reltuples != pgcform->reltuples)
- {
- replaces[ncols] = Anum_pg_class_reltuples;
- values[ncols] = Float4GetDatum(reltuples);
- ncols++;
- }
-
+ else
+ update_reltuples = true;
}
if (!PG_ARGISNULL(RELALLVISIBLE_ARG))
{
- int32 relallvisible = PG_GETARG_INT32(RELALLVISIBLE_ARG);
+ relallvisible = PG_GETARG_INT32(RELALLVISIBLE_ARG);
if (relallvisible < 0)
{
@@ -155,23 +116,118 @@ relation_statistics_update(FunctionCallInfo fcinfo, int elevel)
errmsg("relallvisible cannot be < 0")));
result = false;
}
- else if (relallvisible != pgcform->relallvisible)
+ else
+ update_relallvisible = true;
+ }
+
+ stats_check_required_arg(fcinfo, relarginfo, RELATION_ARG);
+ reloid = PG_GETARG_OID(RELATION_ARG);
+
+ if (RecoveryInProgress())
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("recovery is in progress"),
+ errhint("Statistics cannot be modified during recovery.")));
+
+ stats_lock_check_privileges(reloid);
+
+ /*
+ * Take RowExclusiveLock on pg_class, consistent with
+ * vac_update_relstats().
+ */
+ crel = table_open(RelationRelationId, RowExclusiveLock);
+
+ if (inplace)
+ {
+ HeapTuple ctup = NULL;
+ ScanKeyData key[1];
+ Form_pg_class pgcform;
+ void *inplace_state = NULL;
+ bool dirty = false;
+
+ ScanKeyInit(&key[0], Anum_pg_class_oid, BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(reloid));
+ systable_inplace_update_begin(crel, ClassOidIndexId, true, NULL, 1, key,
+ &ctup, &inplace_state);
+ if (!HeapTupleIsValid(ctup))
+ elog(ERROR, "pg_class entry for relid %u vanished while updating statistics",
+ reloid);
+ pgcform = (Form_pg_class) GETSTRUCT(ctup);
+
+ if (update_relpages && relpages != pgcform->relpages)
+ {
+ pgcform->relpages = relpages;
+ dirty = true;
+ }
+ if (update_reltuples && pgcform->reltuples != reltuples)
{
- replaces[ncols] = Anum_pg_class_relallvisible;
- values[ncols] = Int32GetDatum(relallvisible);
- ncols++;
+ pgcform->reltuples = reltuples;
+ dirty = true;
+ }
+ if (update_relallvisible && pgcform->relallvisible != relallvisible)
+ {
+ pgcform->relallvisible = relallvisible;
+ dirty = true;
}
- }
- /* only update pg_class if there is a meaningful change */
- if (ncols > 0)
+ if (dirty)
+ systable_inplace_update_finish(inplace_state, ctup);
+ else
+ systable_inplace_update_cancel(inplace_state);
+ }
+ else
{
- HeapTuple newtup;
+ TupleDesc tupdesc = RelationGetDescr(crel);
+ HeapTuple ctup;
+ Form_pg_class pgcform;
+ int replaces[3] = {0};
+ Datum values[3] = {0};
+ bool nulls[3] = {0};
+ int nreplaces = 0;
+
+ ctup = SearchSysCache1(RELOID, ObjectIdGetDatum(reloid));
+ if (!HeapTupleIsValid(ctup))
+ {
+ ereport(elevel,
+ (errcode(ERRCODE_OBJECT_IN_USE),
+ errmsg("pg_class entry for relid %u not found", reloid)));
+ table_close(crel, RowExclusiveLock);
+ return false;
+ }
+ pgcform = (Form_pg_class) GETSTRUCT(ctup);
+
+ if (update_relpages && relpages != pgcform->relpages)
+ {
+ replaces[nreplaces] = Anum_pg_class_relpages;
+ values[nreplaces] = Int32GetDatum(relpages);
+ nreplaces++;
+ }
+
+ if (update_reltuples && reltuples != pgcform->reltuples)
+ {
+ replaces[nreplaces] = Anum_pg_class_reltuples;
+ values[nreplaces] = Float4GetDatum(reltuples);
+ nreplaces++;
+ }
+
+ if (update_relallvisible && relallvisible != pgcform->relallvisible)
+ {
+ replaces[nreplaces] = Anum_pg_class_relallvisible;
+ values[nreplaces] = Int32GetDatum(relallvisible);
+ nreplaces++;
+ }
+
+ if (nreplaces > 0)
+ {
+ HeapTuple newtup;
+
+ newtup = heap_modify_tuple_by_cols(ctup, tupdesc, nreplaces,
+ replaces, values, nulls);
+ CatalogTupleUpdate(crel, &newtup->t_self, newtup);
+ heap_freetuple(newtup);
+ }
- newtup = heap_modify_tuple_by_cols(ctup, tupdesc, ncols, replaces, values,
- nulls);
- CatalogTupleUpdate(crel, &newtup->t_self, newtup);
- heap_freetuple(newtup);
+ ReleaseSysCache(ctup);
}
/* release the lock, consistent with vac_update_relstats() */
@@ -188,7 +244,7 @@ relation_statistics_update(FunctionCallInfo fcinfo, int elevel)
Datum
pg_set_relation_stats(PG_FUNCTION_ARGS)
{
- relation_statistics_update(fcinfo, ERROR);
+ relation_statistics_update(fcinfo, ERROR, false);
PG_RETURN_VOID();
}
@@ -212,7 +268,7 @@ pg_clear_relation_stats(PG_FUNCTION_ARGS)
newfcinfo->args[3].value = DEFAULT_RELALLVISIBLE;
newfcinfo->args[3].isnull = false;
- relation_statistics_update(newfcinfo, ERROR);
+ relation_statistics_update(newfcinfo, ERROR, false);
PG_RETURN_VOID();
}
@@ -230,7 +286,7 @@ pg_restore_relation_stats(PG_FUNCTION_ARGS)
relarginfo, WARNING))
result = false;
- if (!relation_statistics_update(positional_fcinfo, WARNING))
+ if (!relation_statistics_update(positional_fcinfo, WARNING, true))
result = false;
PG_RETURN_BOOL(result);
diff --git a/src/test/regress/expected/stats_import.out b/src/test/regress/expected/stats_import.out
index aab862c97c7..fb50da1cd83 100644
--- a/src/test/regress/expected/stats_import.out
+++ b/src/test/regress/expected/stats_import.out
@@ -105,6 +105,47 @@ WHERE oid = 'stats_import.test'::regclass;
18 | 401 | 5
(1 row)
+-- test MVCC behavior: changes do not persist after abort (in contrast
+-- to pg_restore_relation_stats(), which uses in-place updates).
+BEGIN;
+SELECT
+ pg_catalog.pg_set_relation_stats(
+ relation => 'stats_import.test'::regclass,
+ relpages => NULL::integer,
+ reltuples => 4000.0::real,
+ relallvisible => 4::integer);
+ pg_set_relation_stats
+-----------------------
+
+(1 row)
+
+ABORT;
+SELECT relpages, reltuples, relallvisible
+FROM pg_class
+WHERE oid = 'stats_import.test'::regclass;
+ relpages | reltuples | relallvisible
+----------+-----------+---------------
+ 18 | 401 | 5
+(1 row)
+
+BEGIN;
+SELECT
+ pg_catalog.pg_clear_relation_stats(
+ 'stats_import.test'::regclass);
+ pg_clear_relation_stats
+-------------------------
+
+(1 row)
+
+ABORT;
+SELECT relpages, reltuples, relallvisible
+FROM pg_class
+WHERE oid = 'stats_import.test'::regclass;
+ relpages | reltuples | relallvisible
+----------+-----------+---------------
+ 18 | 401 | 5
+(1 row)
+
-- clear
SELECT
pg_catalog.pg_clear_relation_stats(
@@ -705,6 +746,25 @@ WHERE oid = 'stats_import.test'::regclass;
(1 row)
-- ok: just relpages
+SELECT pg_restore_relation_stats(
+ 'relation', 'stats_import.test'::regclass,
+ 'version', 150000::integer,
+ 'relpages', '15'::integer);
+ pg_restore_relation_stats
+---------------------------
+ t
+(1 row)
+
+SELECT relpages, reltuples, relallvisible
+FROM pg_class
+WHERE oid = 'stats_import.test'::regclass;
+ relpages | reltuples | relallvisible
+----------+-----------+---------------
+ 15 | 400 | 4
+(1 row)
+
+-- test non-MVCC behavior: new value should persist after abort
+BEGIN;
SELECT pg_restore_relation_stats(
'relation', 'stats_import.test'::regclass,
'version', 150000::integer,
@@ -714,6 +774,7 @@ SELECT pg_restore_relation_stats(
t
(1 row)
+ABORT;
SELECT relpages, reltuples, relallvisible
FROM pg_class
WHERE oid = 'stats_import.test'::regclass;
diff --git a/src/test/regress/sql/stats_import.sql b/src/test/regress/sql/stats_import.sql
index 31455b58c1d..d3058bf8f6b 100644
--- a/src/test/regress/sql/stats_import.sql
+++ b/src/test/regress/sql/stats_import.sql
@@ -76,6 +76,31 @@ SELECT relpages, reltuples, relallvisible
FROM pg_class
WHERE oid = 'stats_import.test'::regclass;
+-- test MVCC behavior: changes do not persist after abort (in contrast
+-- to pg_restore_relation_stats(), which uses in-place updates).
+BEGIN;
+SELECT
+ pg_catalog.pg_set_relation_stats(
+ relation => 'stats_import.test'::regclass,
+ relpages => NULL::integer,
+ reltuples => 4000.0::real,
+ relallvisible => 4::integer);
+ABORT;
+
+SELECT relpages, reltuples, relallvisible
+FROM pg_class
+WHERE oid = 'stats_import.test'::regclass;
+
+BEGIN;
+SELECT
+ pg_catalog.pg_clear_relation_stats(
+ 'stats_import.test'::regclass);
+ABORT;
+
+SELECT relpages, reltuples, relallvisible
+FROM pg_class
+WHERE oid = 'stats_import.test'::regclass;
+
-- clear
SELECT
pg_catalog.pg_clear_relation_stats(
@@ -565,10 +590,22 @@ FROM pg_class
WHERE oid = 'stats_import.test'::regclass;
-- ok: just relpages
+SELECT pg_restore_relation_stats(
+ 'relation', 'stats_import.test'::regclass,
+ 'version', 150000::integer,
+ 'relpages', '15'::integer);
+
+SELECT relpages, reltuples, relallvisible
+FROM pg_class
+WHERE oid = 'stats_import.test'::regclass;
+
+-- test non-MVCC behavior: new value should persist after abort
+BEGIN;
SELECT pg_restore_relation_stats(
'relation', 'stats_import.test'::regclass,
'version', 150000::integer,
'relpages', '16'::integer);
+ABORT;
SELECT relpages, reltuples, relallvisible
FROM pg_class
--
2.34.1
