Fix small issues of pg_restore_extended_stats()

From: Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com>
To: PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Cc: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Corey Huinker <corey(dot)huinker(at)gmail(dot)com>
Subject: Fix small issues of pg_restore_extended_stats()
Date: 2026-05-18 02:25:16
Message-ID: A7C11B83-7534-4A09-9071-FBD09175CFC8@gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

While testing pg_restore_extended_stats(), I noticed several small issues.

1. Inconsistent expression key verification behavior

In this example, “b” is an invalid key, so it raises a warning and rejects the input:
```
evantest=# select pg_restore_extended_stats(
'schemaname', 'repro’,
'relname', 't’,
'statistics_schemaname', 'repro’,
'statistics_name', 's’,
'inherited', false,
'exprs', '[{"b": "1"}]'::jsonb);
WARNING: could not import element in expression -1: invalid key name
pg_restore_extended_stats
---------------------------
f
(1 row)
```

However, when I tried “a”, which is also an invalid key, it is silently ignored:
```
evantest=# select pg_restore_extended_stats(
'schemaname', 'repro’,
'relname', 't’,
'statistics_schemaname', 'repro’,
'statistics_name', 's’,
'inherited', false,
'exprs', '[{"a": "1"}]'::jsonb);
pg_restore_extended_stats
---------------------------
t
(1 row)
```

After debugging, I think the problem is here:
```
/*
* Check if key is found in the list of expression argnames.
*/
static bool
key_in_expr_argnames(JsonbValue *key)
{
Assert(key->type == jbvString);
for (int i = 0; i < NUM_ATTRIBUTE_STATS_ELEMS; i++)
{
if (strncmp(extexprargname[i], key->val.string.val, key->val.string.len) == 0)
return true;
}
return false;
}
```

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. For example:
```
evantest=# select pg_restore_extended_stats(
'schemaname', 'repro’,
'relname', 't’,
'statistics_schemaname', 'repro’,
'statistics_name', 's’,
'inherited', false,
'exprs', '[{"n_distinct": "5", "correlatio": "-0.6"}]'::jsonb);
pg_restore_extended_stats
---------------------------
t
(1 row)

evantest=# select n_distinct, correlation from pg_stats_ext_exprs where statistics_schemaname = 'repro' and statistics_name = 's';
n_distinct | correlation
------------+-------------
5 |
(1 row)
```

Here, the user may think correlation has been set, but it has not, and there is no warning. I think this behavior should be fixed.

The fix is straightforward. If we keep using strncmp() for safety, we should also compare the lengths of both strings.

2. Wrong number in a warning message

I have only one expression in this example:
```
evantest=# select pg_restore_extended_stats(
evantest(# 'schemaname', 'repro',
evantest(# 'relname', 't',
evantest(# 'statistics_schemaname', 'repro',
evantest(# 'statistics_name', 's',
evantest(# 'inherited', false,
evantest(# 'exprs', '[{"n_distinct": "1"}, {"n_distinct": "2"}, {"n_distinct": "3"}]'::jsonb
evantest(# );
WARNING: could not parse "exprs": incorrect number of elements (3 required)
pg_restore_extended_stats
---------------------------
f
(1 row)
```

The warning message says “3 required”, but it should be “1 required”.

The root cause is here:
```
num_root_elements = JsonContainerSize(root);
if (numexprs != num_root_elements)
{
ereport(WARNING,
errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("could not parse \"%s\": incorrect number of elements (%d required)",
argname, num_root_elements));
goto exprs_error;
}
```

In the ereport, we should use numexprs instead.

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).

But from my previous experience, I know this kind of issue is usually not considered as serious, so I only point it out here without making a fix for now.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Attachment Content-Type Size
v1-0001-Fix-prefix-matching-of-expression-stats-keys.patch application/octet-stream 3.6 KB
v1-0002-Fix-required-expression-count-in-stats-import-war.patch application/octet-stream 4.0 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2026-05-18 03:15:36 Re: [PATCH] Fix ProcKill lock-group vs procLatch recycle race
Previous Message Junwang Zhao 2026-05-18 02:07:02 Re: (SQL/PGQ) cache lookup failed for label