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
