MIME-Version: 1.0
References: <aaDrJsE1I5mrE-QF@paquier.xyz>
In-Reply-To: <aaDrJsE1I5mrE-QF@paquier.xyz>
From: Corey Huinker <corey.huinker@gmail.com>
Date: Thu, 26 Feb 2026 22:52:48 -0500
Message-ID: <CADkLM=c_3mPHr0JGP_72csQkpAt-v2fRN830RO8or1_R1qX2Wg@mail.gmail.com>
Subject: Re: Defects with invalid stats data for expressions in extended stats
To: Michael Paquier <michael@paquier.xyz>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>, Tomas Vondra <tomas@vondra.me>
Content-Type: multipart/alternative; boundary="000000000000576043064bc62c1e"
Archived-At: <https://www.postgresql.org/message-id/CADkLM%3Dc_3mPHr0JGP_72csQkpAt-v2fRN830RO8or1_R1qX2Wg%40mail.gmail.com>
Precedence: bulk

--000000000000576043064bc62c1e
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Thu, Feb 26, 2026 at 7:54=E2=80=AFPM Michael Paquier <michael@paquier.xy=
z> wrote:

> Hi all,
> (Adding Tomas in CC., as primary author of the feature dealt with
> here, and Corey, due to his work for the restore of extstats.)
>
> While looking at the proposed patch for the restore of extended
> statistics on expressions, I have bumped into two defects that exist
> since this feature has been introduced in v15.  First, I have thought
> that this was a problem only related to the proposed patch, but after
> more analysis, I have found that this issue is independent, and can be
> triggered without restoring any stats.
>
> First, one thing that is important to know is that when defining an
> extstat object with N expressions, what we finish by storing in the
> catalogs is an array of N pg_statistics tuples.  Some expressions can
> have invalid data, symbolized by this code in extended_stats.c
> if (!stats->stats_valid)
> {
>     astate =3D accumArrayResult(astate,
>                               (Datum) 0,
>                               true,
>                               typOid,
>                               CurrentMemoryContext);
>     continue;
> }
>
> There is nothing wrong with that.  Having N elements in the array of
> pg_statistics tuples is a requirement, and the code clearly intends
> that.  There should be no more and no less elements, and this is used
> as a marker to let the code that loads this catalog data that nothing
> could be computed.  This data is inserted when we run ANALYZE.
>
> Some code paths are unfortunately not water-proof with this NULL-ness
> handling, and I have found two of them as fixed by 0001.
>
> 1) When building statistics, lookup_var_attr_stats() has missed the
> fact that computing stats for an expression could lead to invalid
> stats being generated.  examine_attribute() deals with this case by
> returning NULL if either:
> 1-1) the typanalyze callback returns false,
> 1-2) The number of rows returned is negative.
> 1-3) For whatever reason in a custom type implementation, the
> compute_stats callback is not set.
> lookup_var_attr_stats() has been dealing with the case of invalid
> stats for attributes, but it has missed the mark after calling
> examine_attribute() for each expression.  Note that
> examine_attribute() exists in both extended_stats.c and analyze.c,
> they are very close in shape, and need to rely on the same assumptions
> in terms of what the typanalyze callback can return.
> lookup_var_attr_stats() has two callers, both are able to deal with
> NULL data (aka invalid stats).  A consequence of this issue is a set
> of NULL pointer dereferences for MCV, ndistinct and dependencies, as
> all these code paths expect something to exist for each expression.
> As there is no stats data, each of them would crash.  At least one
> needs to be specified when creating an extstat object.
>
> 2) When loading statistics, statext_expressions_load() missed that
> some elements in the pg_statistics array could be NULL.  This issue
> can only be triggered if we have some invalid data stored in the
> catalogs.  This issue can be triggered on any branches with a
> typanalyze callback that returns true, where stats_valid is set to
> false when computing the stats (all the in-core callbacks set this
> flag to true, nobody in their right mind would do that except me here,
> I suspect).  The restore of extended stats for expressions makes
> this defect more easily reachable by *bypassing* the build, but as the
> previous sentence describes it is *not* a mandatory requirement
> depending on what a typanalyze callback does.  Hence, we have patch it
> in all the stable branches anyway.  The code allows NULL data to exist
> for some expressions, but the load does not cope with it.  This is
> reflected by fixing two code paths:
> 2-1) statext_expressions_load() needs to be able to load NULL data.
> 2-2) examine_variable() in selfuncs.c needs to lead with this possible
> consequence.
> All the callers of examine_variable() have an exit path in selfuncs.c
> if there is no stats data available, assuming some defaults, in the
> event where statsTuple is NULL (aka invalid).  Note that it is
> possible to reach this path with a typanalyze that returns true,
> meaning that some data is available and that we store NULL data to be
> stored, but the compute_stats callback has the idea to set stats_valid
> to false.  We never set stats_valid to false in any of the in-core
> callbacks.
>
> In order to demonstrate these two bugs, I have implemented a test
> module called test_custom_types, as of 0002 (btree operator bloats the
> module, it is required for extended stats), that includes a simple
> custom type with two buggy typanalyze callbacks (one for the build
> defect, and one for the load defect).  I think that it would be a good
> thing to backpatch this module with the main fix, so as we can cover
> these fancy scenarios for all released branches.  This could be
> enlarged for more cases if we have more defects detected in the future
> in this area of the code.  This affects versions down to v15.
>
> In order to finish the business with the restore of extended stats for
> this release, these defects have to be addressed first.  It does not
> impact what has been already committed for the restore of extended
> stats, fortunately: we have not touched the expressions yet and the
> patch is still floating around in this CF.
>
> I am registering this item in the final CF, as a bug fix.  The
> typanalyze requirements may sound it like something worth only fixing
> on HEAD, but I don't really see a reason why back-branches should not
> be fixed as well.
>
> So, thoughts or comments?
>

Great detective work.

Patch applies for me, but there seems to be some user-specific stuff in the
test, which causes it to fail:

diff -U3
/home/corey/src/postgres/src/test/modules/test_custom_types/expected/test_c=
ustom_types.out
/home/corey/src/postgres/build/testrun/test_custom_types/regress/results/te=
st_custom_types.out
---
/home/corey/src/postgres/src/test/modules/test_custom_types/expected/test_c=
ustom_types.out
2026-02-26 22:34:30.928378989 -0500
+++
/home/corey/src/postgres/build/testrun/test_custom_types/regress/results/te=
st_custom_types.out
2026-02-26 22:35:53.005346284 -0500
@@ -127,7 +127,7 @@
 tablename              | test_table
 statistics_schemaname  | public
 statistics_name        | test_stats
-statistics_owner       | ioltas
+statistics_owner       | corey
 expr                   | func_int_custom(data)
 inherited              | f
 null_frac              |

A nitpick about the test - it uses a plpgsql function when we've been
moving such trivial functions to SQL standard function bodies for a while
now, and they were introduced back in v14 so there's no backporting
concern. Blah blah, eat our own dogfood blah.

--000000000000576043064bc62c1e
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote g=
mail_quote_container"><div dir=3D"ltr" class=3D"gmail_attr">On Thu, Feb 26,=
 2026 at 7:54=E2=80=AFPM Michael Paquier &lt;<a href=3D"mailto:michael@paqu=
ier.xyz">michael@paquier.xyz</a>&gt; wrote:<br></div><blockquote class=3D"g=
mail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204=
,204,204);padding-left:1ex">Hi all,<br>
(Adding Tomas in CC., as primary author of the feature dealt with<br>
here, and Corey, due to his work for the restore of extstats.)<br>
<br>
While looking at the proposed patch for the restore of extended<br>
statistics on expressions, I have bumped into two defects that exist<br>
since this feature has been introduced in v15.=C2=A0 First, I have thought<=
br>
that this was a problem only related to the proposed patch, but after<br>
more analysis, I have found that this issue is independent, and can be<br>
triggered without restoring any stats.<br>
<br>
First, one thing that is important to know is that when defining an<br>
extstat object with N expressions, what we finish by storing in the<br>
catalogs is an array of N pg_statistics tuples.=C2=A0 Some expressions can<=
br>
have invalid data, symbolized by this code in extended_stats.c <br>
if (!stats-&gt;stats_valid)<br>
{<br>
=C2=A0 =C2=A0 astate =3D accumArrayResult(astate,<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (Datum) 0,<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 true,<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 typOid,<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 CurrentMemoryContext);<br>
=C2=A0 =C2=A0 continue;<br>
}<br>
<br>
There is nothing wrong with that.=C2=A0 Having N elements in the array of<b=
r>
pg_statistics tuples is a requirement, and the code clearly intends<br>
that.=C2=A0 There should be no more and no less elements, and this is used<=
br>
as a marker to let the code that loads this catalog data that nothing<br>
could be computed.=C2=A0 This data is inserted when we run ANALYZE.<br>
<br>
Some code paths are unfortunately not water-proof with this NULL-ness<br>
handling, and I have found two of them as fixed by 0001.<br>
<br>
1) When building statistics, lookup_var_attr_stats() has missed the<br>
fact that computing stats for an expression could lead to invalid<br>
stats being generated.=C2=A0 examine_attribute() deals with this case by<br=
>
returning NULL if either:<br>
1-1) the typanalyze callback returns false,<br>
1-2) The number of rows returned is negative.<br>
1-3) For whatever reason in a custom type implementation, the<br>
compute_stats callback is not set.<br>
lookup_var_attr_stats() has been dealing with the case of invalid<br>
stats for attributes, but it has missed the mark after calling<br>
examine_attribute() for each expression.=C2=A0 Note that<br>
examine_attribute() exists in both extended_stats.c and analyze.c,<br>
they are very close in shape, and need to rely on the same assumptions<br>
in terms of what the typanalyze callback can return.<br>
lookup_var_attr_stats() has two callers, both are able to deal with<br>
NULL data (aka invalid stats).=C2=A0 A consequence of this issue is a set<b=
r>
of NULL pointer dereferences for MCV, ndistinct and dependencies, as<br>
all these code paths expect something to exist for each expression.<br>
As there is no stats data, each of them would crash.=C2=A0 At least one<br>
needs to be specified when creating an extstat object.<br>
<br>
2) When loading statistics, statext_expressions_load() missed that<br>
some elements in the pg_statistics array could be NULL.=C2=A0 This issue<br=
>
can only be triggered if we have some invalid data stored in the<br>
catalogs.=C2=A0 This issue can be triggered on any branches with a<br>
typanalyze callback that returns true, where stats_valid is set to<br>
false when computing the stats (all the in-core callbacks set this<br>
flag to true, nobody in their right mind would do that except me here,<br>
I suspect).=C2=A0 The restore of extended stats for expressions makes<br>
this defect more easily reachable by *bypassing* the build, but as the<br>
previous sentence describes it is *not* a mandatory requirement<br>
depending on what a typanalyze callback does.=C2=A0 Hence, we have patch it=
<br>
in all the stable branches anyway.=C2=A0 The code allows NULL data to exist=
<br>
for some expressions, but the load does not cope with it.=C2=A0 This is<br>
reflected by fixing two code paths: <br>
2-1) statext_expressions_load() needs to be able to load NULL data.<br>
2-2) examine_variable() in selfuncs.c needs to lead with this possible<br>
consequence.<br>
All the callers of examine_variable() have an exit path in selfuncs.c<br>
if there is no stats data available, assuming some defaults, in the<br>
event where statsTuple is NULL (aka invalid).=C2=A0 Note that it is<br>
possible to reach this path with a typanalyze that returns true,<br>
meaning that some data is available and that we store NULL data to be<br>
stored, but the compute_stats callback has the idea to set stats_valid<br>
to false.=C2=A0 We never set stats_valid to false in any of the in-core<br>
callbacks.<br>
<br>
In order to demonstrate these two bugs, I have implemented a test<br>
module called test_custom_types, as of 0002 (btree operator bloats the<br>
module, it is required for extended stats), that includes a simple<br>
custom type with two buggy typanalyze callbacks (one for the build<br>
defect, and one for the load defect).=C2=A0 I think that it would be a good=
<br>
thing to backpatch this module with the main fix, so as we can cover<br>
these fancy scenarios for all released branches.=C2=A0 This could be<br>
enlarged for more cases if we have more defects detected in the future<br>
in this area of the code.=C2=A0 This affects versions down to v15.<br>
<br>
In order to finish the business with the restore of extended stats for<br>
this release, these defects have to be addressed first.=C2=A0 It does not<b=
r>
impact what has been already committed for the restore of extended<br>
stats, fortunately: we have not touched the expressions yet and the<br>
patch is still floating around in this CF.<br>
<br>
I am registering this item in the final CF, as a bug fix.=C2=A0 The<br>
typanalyze requirements may sound it like something worth only fixing<br>
on HEAD, but I don&#39;t really see a reason why back-branches should not<b=
r>
be fixed as well. <br>
<br>
So, thoughts or comments? <br></blockquote><div><br></div><div>Great detect=
ive work.</div><div><br></div><div>Patch applies for me, but there seems to=
 be some user-specific stuff in the test, which causes it to fail:<br><br><=
div style=3D"margin-left:40px">diff -U3 /home/corey/src/postgres/src/test/m=
odules/test_custom_types/expected/test_custom_types.out /home/corey/src/pos=
tgres/build/testrun/test_custom_types/regress/results/test_custom_types.out=
<br>--- /home/corey/src/postgres/src/test/modules/test_custom_types/expecte=
d/test_custom_types.out	2026-02-26 22:34:30.928378989 -0500<br>+++ /home/co=
rey/src/postgres/build/testrun/test_custom_types/regress/results/test_custo=
m_types.out	2026-02-26 22:35:53.005346284 -0500<br>@@ -127,7 +127,7 @@<br>=
=C2=A0tablename =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0| test_tabl=
e<br>=C2=A0statistics_schemaname =C2=A0| public<br>=C2=A0statistics_name =
=C2=A0 =C2=A0 =C2=A0 =C2=A0| test_stats<br>-statistics_owner =C2=A0 =C2=A0 =
=C2=A0 | ioltas<br>+statistics_owner =C2=A0 =C2=A0 =C2=A0 | corey<br>=C2=A0=
expr =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 | func_=
int_custom(data)<br>=C2=A0inherited =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 =C2=A0| f<br>=C2=A0null_frac =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0| <br></div><br></div><div>A nitpick about the test - it uses a plpgs=
ql function when we&#39;ve been moving such trivial functions to SQL standa=
rd function bodies for a while now, and they were introduced back in v14 so=
 there&#39;s no backporting concern. Blah blah, eat our own dogfood blah.</=
div><br></div></div>

--000000000000576043064bc62c1e--