On 1/17/21 3:55 AM, Zhihong Yu wrote:
Hi,
+ * Check that only the base rel is mentioned. (This should be dead code
+ * now that add_missing_from is history.)
+ */
+ if (list_length(pstate->p_rtable) != 1)
If it is dead code, it can be removed, right ?
Maybe. The question is whether it really is dead code. It's copied from
transformIndexStmt so I kept it for now.
For statext_mcv_clauselist_selectivity:
+ // bms_free(list_attnums[listidx]);
> The commented line can be removed.
Actually, this should probably do list_free on the list_exprs.
+bool
+examine_clause_args2(List *args, Node **exprp, Const **cstp, bool
*expronleftp)
Better add some comment for examine_clause_args2 since there
is examine_clause_args() already.
Yeah, this probably needs a better name. I have a feeling this may need
a refactoring to reuse more of the existing code for the expressions.
+ if (!ok || stats->compute_stats == NULL || stats->minrows <= 0)
When would stats->minrows have negative value ?
The typanalyze function (e.g. std_typanalyze) can set it to negative
value. This is the same check as in examine_attribute, and we need the
same protections I think.
For serialize_expr_stats():
+ sd = table_open(StatisticRelationId, RowExclusiveLock);
+
+ /* lookup OID of composite type for pg_statistic */
+ typOid = get_rel_type_id(StatisticRelationId);
+ if (!OidIsValid(typOid))
+ ereport(ERROR,
Looks like the table_open() call can be made after the typOid check.
Probably, but this failure is unlikely (should never happen) so I don't
think this makes any real difference.
+ Datum values[Natts_pg_statistic];
+ bool nulls[Natts_pg_statistic];
+ HeapTuple stup;
+
+ if (!stats->stats_valid)
It seems the local arrays can be declared after the validity check.
Nope, that would be against C99.
+ if (enabled[i] == STATS_EXT_NDISTINCT)
+ ndistinct_enabled = true;
+ if (enabled[i] == STATS_EXT_DEPENDENCIES)
+ dependencies_enabled = true;
+ if (enabled[i] == STATS_EXT_MCV)
the second and third if should be preceded with 'else'
Yeah, although this just moves existing code. But you're right it could
use else.
+ReleaseDummy(HeapTuple tuple)
+{
+ pfree(tuple);
Since ReleaseDummy() is just a single pfree call, maybe you don't need
this method - call pfree in its place.
No, that's not possible because the freefunc callback expects signature
void (*)(HeapTupleData *)
and pfree() does not match that.
thanks
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
