Re: Statistics Import and Export

Hi,

On Fri, Mar 08, 2024 at 01:35:40AM -0500, Corey Huinker wrote:
> Anyway, here's v7. Eagerly awaiting feedback.

Thanks!

A few random comments:

1 ===

+ The purpose of this function is to apply statistics values in an
+ upgrade situation that are "good enough" for system operation until

Worth to add a few words about "influencing" the planner use case?

2 ===

+#include "catalog/pg_type.h"
+#include "fmgr.h"

Are those 2 needed?

3 ===

+ if (!HeapTupleIsValid(ctup))
+ elog(ERROR, "pg_class entry for relid %u vanished during statistics import",

s/during statistics import/when setting statistics/?

4 ===

+Datum
+pg_set_relation_stats(PG_FUNCTION_ARGS)
+{
.
.
+ table_close(rel, ShareUpdateExclusiveLock);
+
+ PG_RETURN_BOOL(true);

Why returning a bool? (I mean we'd throw an error or return true).

5 ===

+ */
+Datum
+pg_set_attribute_stats(PG_FUNCTION_ARGS)
+{

This function is not that simple, worth to explain its logic in a comment above?

6 ===

+ if (!HeapTupleIsValid(tuple))
+ {
+ relation_close(rel, NoLock);
+ PG_RETURN_BOOL(false);
+ }
+
+ attr = (Form_pg_attribute) GETSTRUCT(tuple);
+ if (attr->attisdropped)
+ {
+ ReleaseSysCache(tuple);
+ relation_close(rel, NoLock);
+ PG_RETURN_BOOL(false);
+ }

Why is it returning "false" and not throwing an error? (if ok, then I think
we can get rid of returning a bool).

7 ===

+ * If this relation is an index and that index has expressions in
+ * it, and the attnum specified

s/is an index and that index has/is an index that has/?

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com