| From: | Michael Paquier <michael(at)paquier(dot)xyz> |
|---|---|
| To: | Denis Rodionov <denis(dot)rodionov(at)tantorlabs(dot)com> |
| Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
| Subject: | Re: [PATCH] Remove obsolete tupDesc assignment in extended statistics |
| Date: | 2026-06-01 05:17:16 |
| Message-ID: | ah0V3EPMrHJs5IKs@paquier.xyz |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Chao Li | 2026-06-01 05:33:40 | pg_stat_statements: Fix normalization of + signs for FETCH and MOVE |
| Previous Message | Kyotaro Horiguchi | 2026-06-01 04:58:58 | Re: Improve pg_stat_statements scalability |