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/
From c1ce464684b1f475adcad14e934060dc20dabe15 Mon Sep 17 00:00:00 2001
From: Denis Rodionov <[email protected]>
Date: Thu, 4 Jun 2026 20:23:07 +0300
Subject: [PATCH v2] Remove obsolete tupDesc assignment in extended statistics
lookup_var_attr_stats() assigned tupDesc to VacAttrStats entries built for
expressions. That appears to be obsolete: make_build_data() uses tupDesc only
for regular columns, while expression values are evaluated separately.
Remove the assignment and add an assertion documenting that expression stats
do not need a tuple descriptor.
---
src/backend/statistics/extended_stats.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c
index 2b83355d26e..f812cea41e3 100644
--- a/src/backend/statistics/extended_stats.c
+++ b/src/backend/statistics/extended_stats.c
@@ -786,12 +786,10 @@ lookup_var_attr_stats(Bitmapset *attrs, List *exprs,
}
/*
- * 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);
i++;
}
--
2.34.1
