| From: | Michael Paquier <michael(at)paquier(dot)xyz> |
|---|---|
| To: | Corey Huinker <corey(dot)huinker(at)gmail(dot)com> |
| Cc: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>, Tender Wang <tndrwang(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Tomas Vondra <tomas(at)vondra(dot)me> |
| Subject: | Re: Add expressions to pg_restore_extended_stats() |
| Date: | 2026-02-04 11:30:19 |
| Message-ID: | aYMty6C9EN-CYoSJ@paquier.xyz |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Wed, Feb 04, 2026 at 05:32:46AM -0500, Corey Huinker wrote:
> Sigh. errsave_finish() only calls errfinish() on elevel >= ERROR, and
> that's where the context stack is walked. The failed work in progress is
> attached too, mostly as a cautionary tale.
v5-0001 still requires a lot of changes at quick glance, as of:
- Some diffs not required in some comment blocks.
- Regression test diffs show up for cases where WARNING messages are
changed, but should not. See for example the MCV checks.
- A couple of newly-introduced error strings use wordings that are not
project-style, as of the "malformed expressions", there is also a
"malformed expr expression".
- errhints need to be full sentences. For example, this should be a
full sentence:
Expression %d, element \"%s\", value \"%s\": invalid type \"%s\"
+ /*
+ * XXX:
+ *
+ * We may need to duplicate some steps from statatt_get_type() that we do
+ * not currently, those are:
+ *
+ * #include "catalog/pg_collation_d.h" if (typid == TSVECTOROID) stacoll =
+ * DEFAULT_COLLATION_OID;
+ *
+ * The multirange step-down may also need to happen here too.
+ */
Finally, this needs to be sorted out, and I am not sure to understand
fully yet how this interacts with the restore code in the context of
the expressions. If one or more test cases are required to cover such
cases, let's make it so. If more code changes are required to cope
with that, let's also make it so with some documentation explaining
the limits and assumptions of the backend code paths this refers to.
In this shape, this is not helpful.
Now that we have an agreement about the format of the input data and
how it maps with the assumptions of the stats computations in ANALYZE,
the hardest part is IMO done. However, could you clean up things?
This patch is not really in an acceptable shape: it has a bunch of
avoidable errors and it requires a lot of tweaks before even being
looked at more closely.
Thanks,
--
Michael
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Michael Paquier | 2026-02-04 11:37:24 | Re: Undefined behavior detected by new clang's ubsan |
| Previous Message | John Naylor | 2026-02-04 11:15:11 | Re: Undefined behavior detected by new clang's ubsan |