Hello.
At Thu, 1 Aug 2019 00:15:48 +0000, "Jamison, Kirk" <k.jami...@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
