Re: Multivariate MCV list vs. statistics target

Hello.

At Thu, 1 Aug 2019 00:15:48 +0000, "Jamison, Kirk" <k.jamison@jp.fujitsu.com> wrote in
<D09B13F772D2274BB348A310EE3027C6518F94@g01jpexmbkw24>
> The patch does not apply anymore.
> In addition to the whitespace detected,
> kindly rebase the patch as there were changes from recent commits
> in the following files:
>        src/backend/commands/analyze.c
>        src/bin/pg_dump/pg_dump.c
>        src/bin/pg_dump/t/002_pg_dump.pl
>        src/test/regress/expected/stats_ext.out
>        src/test/regress/sql/stats_ext.sql

The patch finally failed only for stats_ext.out, where 14ef15a222
is hitting. (for b2a3d706b8)

I looked through this patch and have some comments.



+++ b/src/backend/commands/statscmds.c
+#include "access/heapam.h"
..
+#include "utils/fmgroids.h"

These don't seem needed.


+    Assert(stmt->missing_ok);

Perhaps we shouldn't Assert on this condition. Isn't it better we
just "elog(ERROR" here?


+    DeconstructQualifiedName(stmt->defnames, &schemaname, &statname);

Maybe we don't need detailed analysis that the function emits on
error. Couldn't we use NameListToString() instead? That reduces
the number of ereport()s and considerably simplify the code
around.

+  oldtup = SearchSysCache1(STATEXTOID, ObjectIdGetDatum(stxoid));
+
+  /* Must be owner of the existing statistics object */
+  if (!pg_statistics_object_ownercheck(stxoid, GetUserId()))

This repeats the SearchSysCache twice in a quite short
duration. I suppose it'd be better that ACL (and validity) checks
done directly using oldtup.


+  /* replace the stxstattarget column */
+  repl_repl[Anum_pg_statistic_ext_stxstattarget - 1] = true;
+  repl_val[Anum_pg_statistic_ext_stxstattarget - 1] = Int32GetDatum(newtarget)

We usually do this kind of work using SearchSysCacheCopyN(),
which simplifies the code around, too.


+++ b/src/backend/statistics/mcv.c
>    * Maximum number of MCV items to store, based on the attribute with the
>    * largest stats target (and the number of groups we have available).
>    */
-  nitems = stats[0]->attr->attstattarget;
-  for (i = 1; i < numattrs; i++)
-  {
-    if (stats[i]->attr->attstattarget > nitems)
-      nitems = stats[i]->attr->attstattarget;
-  }
+  nitems = stattarget;

Maybe you forgot to modify the comment.


check_xact_readonly() returns false for this command. As the
result it emits a somewhat pointless error message.

=# alter statistics s1 set statistics 0;
ERROR:  cannot assign TransactionIds during recovery


+++ b/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.h
>   i_stxname = PQfnumber(res, "stxname");
>   i_stxnamespace = PQfnumber(res, "stxnamespace");
>   i_rolname = PQfnumber(res, "rolname");
+   i_stattarget = PQfnumber(res, "stxstattarget");

I'm not sure whether it is the convention here, but variable name
is different from column name only for the added line.


+++ b/src/bin/psql/tab-complete.c
-        COMPLETE_WITH("OWNER TO", "RENAME TO", "SET SCHEMA");
+        COMPLETE_WITH("OWNER TO", "RENAME TO", "SET SCHEMA", "SET STATISTICS");

ALTER STATISTICS s2 SET STATISTICS<tab> is suggested with
ext-stats names,  but it's the place for target value.


+++ b/src/include/nodes/nodes.h
   T_CallStmt,
+  T_AlterStatsStmt,
 
I think it should be immediately below T_CreateStatsStmt.


+++ b/src/include/nodes/parsenodes.h
+  bool    missing_ok;    /* skip error if statistics object is missing */

Should be very trivial, but many bool members especially
missing_ok have a comment having "?" at the end.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center