0002-review.patch

From adf40a833c14f3f80c7b3f40b5450e494cfb1561 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <tomas@2ndquadrant.com>
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