Re: Collecting statistics about contents of JSONB columns

On 1/1/22 16:33, Zhihong Yu wrote:
> Hi,
> For patch 1:
> 
> +   List       *statisticsName = NIL;   /* optional stats estimat. 
> procedure */
> 
> I think if the variable is named estimatorName (or something similar), 
> it would be easier for people to grasp its purpose.
> 

I agree "statisticsName" might be too generic or confusing, but I'm not 
sure "estimator" is an improvement. Because this is not an "estimator" 
(in the sense of estimating selectivity). It "transforms" statistics to 
match the expression.

> +       /* XXX perhaps full "statistics" wording would be better */
> +       else if (strcmp(defel->defname, "stats") == 0)
> 
> I would recommend (stats sounds too general):
> 
> +       else if (strcmp(defel->defname, "statsestimator") == 0)
> 
> +       statisticsOid = ValidateStatisticsEstimator(statisticsName);
> 
> statisticsOid -> statsEstimatorOid
> 

Same issue with the "estimator" bit :-(

> For get_oprstat():
> 
> +   }
> +   else
> +       return (RegProcedure) InvalidOid;
> 
> keyword else is not needed (considering the return statement in if block).
> 

True, but this is how the other get_ functions in lsyscache.c do it. 
Like get_oprjoin().

> For patch 06:
> 
> +   /* FIXME Could be before the memset, I guess? Checking 
> vardata->statsTuple. */
> +   if (!data->statsTuple)
> +       return false;
> 
> I would agree the check can be lifted above the memset call.
> 

OK.

> + * XXX This does not really extract any stats, it merely allocates the 
> struct?
> + */
> +static JsonPathStats
> +jsonPathStatsGetSpecialStats(JsonPathStats pstats, JsonPathStatsType type)
> 
> As comments says, I think allocJsonPathStats() would be better name for 
> the func.
> 
> + * XXX Why doesn't this do jsonPathStatsGetTypeFreq check similar to what
> + * jsonPathStatsGetLengthStats does?
> 
> I think `jsonPathStatsGetTypeFreq(pstats, jbvArray, 0.0) <= 0.0` check 
> should be added for jsonPathStatsGetArrayLengthStats().
> 
> To be continued ...

OK. I'll see if Nikita has some ideas about the naming changes.

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company