On 6/1/26 08:17, Michael Paquier wrote:
> On Thu, May 28, 2026 at 06:58:39PM +0300, Denis Rodionov wrote:
>> lookup_var_attr_stats() assigns tupDesc to VacAttrStats entries created for
>> expressions. That appears to be obsolete: make_build_data() uses tupDesc
>> only when fetching values for regular columns, while expressions are handled
>> separately using VacAttrStats entries created by examine_expression().
>>
>> The old comment also points at statext_mcv_build(), but this does not seem
>> to match the current code anymore. If some future code needs a tuple
>> descriptor for expression entries here, it should probably set up that
>> dependency explicitly rather than rely on copying vacatts[0]->tupDesc.
>>
>> The patch removes the assignment and the outdated XXX comment.
>
> I got this code under my eyes a couple of weeks ago for a bug fix, and
> wondered the same thing as you here, but I never got down to analyze
> where the TupleDesc is used. Could that be just an oversight of
> a4d75c86bf15 due to rebases of what has been committed?
>
> Perhaps we should enforce somewhere that a VacAttrStats.tupDesc is
> never set for an expression, in the shape of an assertion? I am not
> sure where I would plant that, or if it's required, but that would
> feel better than just removing these lines.
> --
> Michael
Thanks for looking at this.
I checked a4d75c86bf15 and current master. The only use of
VacAttrStats.tupDesc in extended_stats.c seems to be the heap_getattr()
call in make_build_data(). That loop only iterates over stat->columns,
so it only uses the VacAttrStats entries for regular columns. Expression
entries are handled separately and make_build_data() creates their
VacAttrStats entries with examine_expression().
I agree that an assertion makes this clearer. In v2 I removed the
obsolete assignment and added an assertion in lookup_var_attr_stats() to
document that expression stats do not need a tuple descriptor.
Best regards,
Denis Rodionov
Tantor Labs LLC,
https://tantorlabs.com/
