Re: PoC/WIP: Extended statistics on expressions - Mailing list pgsql-hackers

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