From adf40a833c14f3f80c7b3f40b5450e494cfb1561 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Mon, 25 Mar 2024 17:18:34 +0100 Subject: [PATCH 2/4] review --- src/backend/statistics/statistics.c | 11 +++++++++++ src/test/regress/expected/stats_export_import.out | 4 ++-- src/test/regress/sql/stats_export_import.sql | 4 ++-- 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/backend/statistics/statistics.c b/src/backend/statistics/statistics.c index 3987425ea4d..d1cd8342047 100644 --- a/src/backend/statistics/statistics.c +++ b/src/backend/statistics/statistics.c @@ -259,10 +259,16 @@ value_not_null_array_len(ExpandedArrayHeader *arr, const char *name) * values, which is a valid input string for an array of the type or basetype * of the attribute. Any error generated by the array_in() function will in * turn fail the function. + * + * XXX this is a very looooong function, maybe split that into smaller parts + * that do different parts (e.g. checks vs. actual update). */ Datum pg_set_attribute_stats(PG_FUNCTION_ARGS) { + /* XXX this would really deserve some comment that the order matters, + * and so on. It took me ages to realize it's also the index of the + * attribute in function arguments. */ enum { P_RELATION = 0, /* oid */ @@ -285,6 +291,7 @@ pg_set_attribute_stats(PG_FUNCTION_ARGS) }; /* names of columns that cannot be null */ + /* XXX surely many of those can be NULL? say, MCV or elem fields */ const char *param_names[] = { "relation", "attname", @@ -352,6 +359,8 @@ pg_set_attribute_stats(PG_FUNCTION_ARGS) /* * A null in a required parameter is an error. + * + * XXX How do we know P_N_DISTINCT is the last non-null argument? */ for (int i = P_RELATION; i <= P_N_DISTINCT; i++) if (PG_ARGISNULL(i)) @@ -372,6 +381,8 @@ pg_set_attribute_stats(PG_FUNCTION_ARGS) /* * If a caller specifies more stakind-stats than we have slots to store * them, raise an error. + * + * XXX is it a good idea to treat bool as 0/1 int? */ stakind_count = (int) has_mcv + (int) has_mc_elems + (int) has_rl_hist + (int) !PG_ARGISNULL(P_HIST_BOUNDS) + diff --git a/src/test/regress/expected/stats_export_import.out b/src/test/regress/expected/stats_export_import.out index 32b9e7ca113..c4072fa05f5 100644 --- a/src/test/regress/expected/stats_export_import.out +++ b/src/test/regress/expected/stats_export_import.out @@ -339,7 +339,7 @@ AND attname = 'id'; stats_export_import | test | id | f | 0.5 | 2 | -0.1 | | | | 0.5 | | | | | | (1 row) --- error: scalars can't have mcelem +-- error: scalars can't have mcelem SELECT pg_catalog.pg_set_attribute_stats( relation => 'stats_export_import.test'::regclass, attname => 'id'::name, @@ -483,7 +483,7 @@ SELECT pg_catalog.pg_set_attribute_stats( most_common_elem_freqs => '{0.3,0.4,0.2,0.3,0.0}'::real[] ); ERROR: most_common_elem_freqs frequency 0.400000 is out of range 0.200000 to 0.300000 --- ok: mcelem +-- ok: mcelem SELECT pg_catalog.pg_set_attribute_stats( relation => 'stats_export_import.test'::regclass, attname => 'tags'::name, diff --git a/src/test/regress/sql/stats_export_import.sql b/src/test/regress/sql/stats_export_import.sql index b3f17373d3e..ef4e74900b3 100644 --- a/src/test/regress/sql/stats_export_import.sql +++ b/src/test/regress/sql/stats_export_import.sql @@ -293,7 +293,7 @@ AND tablename = 'test' AND inherited = false AND attname = 'id'; --- error: scalars can't have mcelem +-- error: scalars can't have mcelem SELECT pg_catalog.pg_set_attribute_stats( relation => 'stats_export_import.test'::regclass, attname => 'id'::name, @@ -437,7 +437,7 @@ SELECT pg_catalog.pg_set_attribute_stats( most_common_elem_freqs => '{0.3,0.4,0.2,0.3,0.0}'::real[] ); --- ok: mcelem +-- ok: mcelem SELECT pg_catalog.pg_set_attribute_stats( relation => 'stats_export_import.test'::regclass, attname => 'tags'::name, -- 2.44.0