| From: | Corey Huinker <corey(dot)huinker(at)gmail(dot)com> |
|---|---|
| To: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
| Cc: | Michael Paquier <michael(at)paquier(dot)xyz>, Tender Wang <tndrwang(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Extended Statistics set/restore/clear functions. |
| Date: | 2026-01-15 20:21:23 |
| Message-ID: | CADkLM=eDs0s0MVnjnybSrAxOYE+ksqyOAt5hJTQCwTyPWpBkUQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
>
>
> Here are some comments for v24:
>
> 1.
> ```
> + inherited = PG_GETARG_NAME(INHERITED_ARG);
> ```
>
> Should this use PG_GETARG_BOOL()?
>
Yes. I'm curious why the compiler didn't catch that.
>
> 2
> ```
> proparallel => 'u', prorettype => 'void', proargtypes => 'text text text
> text bool’,
> ```
>
> Should we define proargtypes as “name name name name bool”? As the doc
> change mentions type “name” for the first 4 parameters. Maybe you want to
> avoid that because Name is a structure. Anyway, I don’t have a strong
> opinion here.
>
The existing pg_restore_relation_stats() and pg_restore_attribute_stats()
use the text type for both parameters. This was done mostly to avoid
typecasts that could potentially fail in a pg_restore script, and partly to
make things easier on someone writing the function call. The -clear
variants of those functions followed the same convention of text type for
those paramers, and so this is just doing the same.
>
> 3 - extended_stats_funcs.c
> ```
> * Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group
> ```
>
> This is a brand new file, so the copyright year should be just 2026. If
> you search for “2025-2026”, you will get a bunch of files, they should be
> created in 2025.
>
That may have been done because some of the code that will end up here is
being moved from other files rather than being purely new. My revised patch
is leaving it as-is for now.
>
> 4 - Given comment 1, no test fails. I think that’s because all test cases
> use inherited = false. Maybe we need a test case for inherited = true.
>
Added.
In addition to issues 1 and 4 I added some inheritance-specific tests per
Paquier's off-list request, and the result is attached.
| Attachment | Content-Type | Size |
|---|---|---|
| v25-0001-Add-pg_clear_extended_stats.patch | text/x-patch | 25.1 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Dharin Shah | 2026-01-15 20:50:54 | Re: [Patch] Add WHERE clause support to REFRESH MATERIALIZED VIEW |
| Previous Message | Andreas Karlsson | 2026-01-15 20:03:43 | Re: Add IS_INDEX macro to brin and gist index |