[PATCH] Remove obsolete tupDesc assignment in extended statistics

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

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/

On Thu, Jun 04, 2026 at 09:27:05PM +0300, Denis Rodionov wrote:

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.

         /*
-         * XXX We need tuple descriptor later, and we just grab it from
-         * stats[0]->tupDesc (see e.g. statext_mcv_build). But as coded
-         * examine_attribute does not set that, so just grab it from the first
-         * vacatts element.
+         * Expression stats are not tied to a heap attribute, so they do not
+         * need a tuple descriptor.
          */
-        stats[i]->tupDesc = vacatts[0]->tupDesc;
+        Assert(stats[i]->tupDesc == NULL);

Planting an assertion at this location feels OK here. (I need to
remember to look at that again once v20 opens for business, noting
that down now..)
--
Michael

On 6/10/26 08:21, Michael Paquier wrote:

Planting an assertion at this location feels OK here. (I need to
remember to look at that again once v20 opens for business, noting
that down now..)
--
Michael

Hi Michael,

Thanks for the previous review.
I have attached two small patches:
- v3 removes the obsolete tupDesc assignment in extended statistics
and retains the assertion suggested in your review. It has been rebased
on the current master.
- v1 removes a stray semicolon after import_expressions(), which I
noticed while working in the same area.

The second change is independent, but small enough that I have included
it here rather than starting a separate thread.

Could you please take a look and commit them if they look good?

Best regards,
Denis Rodionov,
Tantor Labs LLC,
https://tantorlabs.com/