| From: | Michael Paquier <michael(at)paquier(dot)xyz> |
|---|---|
| To: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
| Cc: | PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Corey Huinker <corey(dot)huinker(at)gmail(dot)com> |
| Subject: | Re: Fix small issues of pg_restore_extended_stats() |
| Date: | 2026-05-18 04:19:02 |
| Message-ID: | agqTNkgZqim5J9gV@paquier.xyz |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Mon, May 18, 2026 at 10:25:16AM +0800, Chao Li wrote:
> 1. Inconsistent expression key verification behavior
>
> This function is actually doing a prefix comparison using the input
> key length, so as long as an input key matches a prefix of a valid
> key name, the function returns true.
>
> At runtime, it does not lead to an incorrect value being imported,
> because the invalid key will still be filtered out later. But one
> bad scenario I can imagine is that a user is trying to set
> “correlation”, and accidentally omits the last “n”. In that case,
> the key is silently discarded and the user is not aware of it, which
> can lead to a surprising result.
It's a silent data loss, with data getting ignored. The call should
report a failure because the key does not match with a name we'd
expect. pg_dump would not generate that, but stats injection is a
supported use-case so that would be confusing.
Nice investigation.
> 2. Wrong number in a warning message
>
> The warning message says “3 required”, but it should be “1
> required”.
Harmless, still wrong.
> 3. Inconsistent heap_freetuple()
>
> In pg_clear_extended_stats(), heap_freetuple(tup) is only called
> before the final return. However, in two earlier paths, the function
> only emits warning messages and returns. It seems those paths should
> also call heap_freetuple(tup).
In the !IsValid() case, there is nothing to free, but you are right
about the second case where stxrelid does not match. It's minor, but
I don't have an argument against doing that either, and the free at
the bottom is a trace that we want to be consistent.
I'll go fix all that. Thanks for the report!
--
Michael
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Peter Smith | 2026-05-18 04:49:08 | Re: Logical Replication - revisit `is_table_publication` function implementation |
| Previous Message | Fujii Masao | 2026-05-18 04:17:13 | Re: [PATCH] Fix psql tab completion for REPACK boolean options |