|From:||"Jamison, Kirk" <k(dot)jamison(at)jp(dot)fujitsu(dot)com>|
|To:||'Tomas Vondra' <tomas(dot)vondra(at)2ndquadrant(dot)com>, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>|
|Cc:||PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>|
|Subject:||RE: Multivariate MCV list vs. statistics target|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On Tuesday, July 9, 2019, Tomas Vondra wrote:
> >apparently v1 of the ALTER STATISTICS patch was a bit confused about
> >length of the VacAttrStats array passed to statext_compute_stattarget,
> >causing segfaults. Attached v2 patch fixes that, and it also makes sure
> >we print warnings about ignored statistics only once.
> v3 of the patch, adding pg_dump support - it works just like when you tweak
> statistics target for a column, for example. When the value stored in the
> catalog is not -1, pg_dump emits a separate ALTER STATISTICS command setting
> it (for the already created statistics object).
Hi Tomas, I stumbled upon your patch.
According to the CF bot, your patch applies cleanly, builds successfully, and
passes make world. Meaning, the pg_dump tap test passed, but there was no
test for the new SET STATISTICS yet. So you might want to add a regression
test for that and integrate it in the existing alter_generic file.
Upon quick read-through, the syntax and docs are correct because it's similar
to the format of ALTER TABLE/INDEX... SET STATISTICS... :
ALTER [ COLUMN ] column_name SET STATISTICS integer
+ /* XXX What if the target is set to 0? Reset the statistic? */
This also makes me wonder. I haven't looked deeply into the code, but since 0 is
a valid value, I believe it should reset the stats.
After lookup though, this is how it's tested in ALTER TABLE:
/test/regress/sql/stats_ext.sql:-- Ensure things work sanely with SET STATISTICS 0
> I've considered making it part of CREATE STATISTICS itself, but it seems a
> bit cumbersome and we don't do it for columns either. I'm not against adding
> it in the future, but at this point I don't see a need.
I agree. Perhaps that's for another patch should you decide to add it in the future.
|Next Message||Julien Rouhaud||2019-07-19 06:29:27||Re: Add parallelism and glibc dependent only options to reindexdb|
|Previous Message||Amit Khandekar||2019-07-19 05:45:49||Re: Minimal logical decoding on standbys|