pg.ddx.io  pgsql-bugs@postgresql.org mailing list archive  
help / color / mirror / Atom feed
BUG #18885: ERROR: corrupt MVNDistinct entry - 2
15+ messages / 5 participants
[nested] [flat]

* BUG #18885: ERROR: corrupt MVNDistinct entry - 2
@ 2025-04-09 13:46 PG Bug reporting form <noreply@postgresql.org>
  2025-04-09 22:47 ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Tomas Vondra <tomas@vondra.me>
  2025-04-10 12:28 ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
  0 siblings, 2 replies; 15+ messages in thread

From: PG Bug reporting form @ 2025-04-09 13:46 UTC (permalink / raw)
  To: pgsql-bugs@lists.postgresql.org; +Cc: tharakan@gmail.com

The following bug has been logged on the website:

Bug reference:      18885
Logged by:          Robins Tharakan
Email address:      tharakan@gmail.com
PostgreSQL version: Unsupported/Unknown
Operating system:   Ubuntu
Description:        

The following SQL triggers "ERROR: corrupt MVNDistinct entry", however this
seems to be unrelated to a recent bugfix[1] for a similar issue (thus '2' in
the $SUBJECT).

The surfacing commit appears to be 6bb6a62f3cc45624c601d5270673a17447734629
[2].

SQL
===
$ cat ../sqith/repro.sql
CREATE TABLE a(b BIT VARYING, d int4range);
CREATE TABLE e(LIKE a);
CREATE TABLE f(LIKE a);
INSERT INTO f(d, b) VALUES('[0,0)', '');
CREATE STATISTICS ON d, b FROM f;
ANALYZE;
SELECT FROM f RIGHT JOIN e ON (e.d=f.d)AND(f.d=e.d);


Commit
======
Testing for start crashing
---
Checking (306dd6e727b~0) - 306dd6e727 - fail (1)
Checking (306dd6e727b~10) - 91f1fe90c7 - fail (1)
Checking (306dd6e727b~30) - 969ab9d4f5 - fail (1)
Checking (306dd6e727b~70) - 1495eff7bd - fail (1)
.
.
Checking (306dd6e727b~447) - fae535da0a - pass (0)
Checking (306dd6e727b~446) - 6bb6a62f3c - fail (1)
Surfacing Commit is 6bb6a62f3cc45624c601d5270673a17447734629



SQL Output
==========
$ psql -p 9999 postgres -f ../sqith/repro.sql
CREATE TABLE
CREATE TABLE
CREATE TABLE
INSERT 0 1
CREATE STATISTICS
ANALYZE
psql:../sqith/repro.sql:7: ERROR:  corrupt MVNDistinct entry

Found using SQLSmith / creduce.
-
robins
https://robins.in

Reference
1. Similar bugfix:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=e28033fe1af8037e0fec8bb3a32fabbe1...
2. Surfacing commit:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=6bb6a62f3cc45624c601d5270673a1744...



^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2
  2025-04-09 13:46 BUG #18885: ERROR: corrupt MVNDistinct entry - 2 PG Bug reporting form <noreply@postgresql.org>
@ 2025-04-09 22:47 ` Tomas Vondra <tomas@vondra.me>
  2025-04-13 23:26   ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 David Rowley <dgrowleyml@gmail.com>
  1 sibling, 1 reply; 15+ messages in thread

From: Tomas Vondra @ 2025-04-09 22:47 UTC (permalink / raw)
  To: tharakan@gmail.com, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>; +Cc: Alexander Korotkov <aekorotkov@gmail.com>

On 4/9/25 15:46, PG Bug reporting form wrote:
> The following bug has been logged on the website:
> 
> Bug reference:      18885
> Logged by:          Robins Tharakan
> Email address:      tharakan@gmail.com
> PostgreSQL version: Unsupported/Unknown
> Operating system:   Ubuntu
> Description:        
> 
> The following SQL triggers "ERROR: corrupt MVNDistinct entry", however this
> seems to be unrelated to a recent bugfix[1] for a similar issue (thus '2' in
> the $SUBJECT).
> 
> The surfacing commit appears to be 6bb6a62f3cc45624c601d5270673a17447734629
> [2].
> 

Thanks for the report. I only looked at this very briefly, but I agree
this seems to be a bug in 6bb6a62f3cc45624c601d5270673a17447734629,
because it simply ignores the possibility that multiple join clauses
could share the same Var. Which is the case in the reproducer, where the
join clause is simply

    ... ON (e.d=f.d) AND (e.d=f.d);

Which then leads to confusion when matching the MVDistinct entries.

I think estimate_multivariate_bucketsize() needs to be more careful
about building the GroupVarInfo list - in particular, it needs to do the
dance with examine_variable + add_unique_group_var + pull_var_clause,
similar to estimate_num_groups() at line ~3532.


regards

-- 
Tomas Vondra






^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2
  2025-04-09 13:46 BUG #18885: ERROR: corrupt MVNDistinct entry - 2 PG Bug reporting form <noreply@postgresql.org>
  2025-04-09 22:47 ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Tomas Vondra <tomas@vondra.me>
@ 2025-04-13 23:26   ` David Rowley <dgrowleyml@gmail.com>
  2025-04-15 09:41     ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
  0 siblings, 1 reply; 15+ messages in thread

From: David Rowley @ 2025-04-13 23:26 UTC (permalink / raw)
  To: Tomas Vondra <tomas@vondra.me>; +Cc: tharakan@gmail.com, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>; Alexander Korotkov <aekorotkov@gmail.com>

On Fri, 11 Apr 2025 at 01:31, Tomas Vondra <tomas@vondra.me> wrote:
> I think estimate_multivariate_bucketsize() needs to be more careful
> about building the GroupVarInfo list - in particular, it needs to do the
> dance with examine_variable + add_unique_group_var + pull_var_clause,
> similar to estimate_num_groups() at line ~3532.

This should be documented to prevent future callers of
estimate_multivariate_ndistinct() from falling for this.

The attached aims to do this.  I also couldn't resist a few other improvements.

There are a few strange goings-ons in the code itself that I didn't
adjust. For example, in the first "foreach(lc2, *varinfos)" loop after
the "if (stats)", there's a "found" variable that gets set and used
for no apparent reason. I don't see why the "found = true;" doesn't
just "continue;". The variable would only be needed if there was some
inner loop and we couldn't use "continue".  I also can't make sense of
the following comment:

/*
* XXX Maybe we should allow searching the expressions even if we
* found an attribute matching the expression? That would handle
* trivial expressions like "(a)" but it seems fairly useless.
*/

Maybe it meant "matching the Var"?

The final loop to build the newlist also looks more complex than it
needs to be. The prior loop over *varinfos could have recorded the
matching GroupVarInfos in the list in a Bitmapset and that final loop
could become:

foreach(lc, *varinfos)
{
   if (!bms_is_member(foreach_current_index(lc), matched_varinfos))
        newlist = lappend(newlist, lfirst(lc));
}

David

Attachments:

  [application/octet-stream] v1-0001-Improve-comments-for-estimate_multivariate_ndisti.patch (4.0K, ../../CAApHDvocZCUhM9W9mJ39d6oQz7ePKoqFnao_347mvC-A7QatcQ@mail.gmail.com/2-v1-0001-Improve-comments-for-estimate_multivariate_ndisti.patch)
  download | inline diff:
From 9fea491c0f230a34011874ccf00be1b82bca2bdd Mon Sep 17 00:00:00 2001
From: David Rowley <dgrowley@gmail.com>
Date: Mon, 14 Apr 2025 10:19:31 +1200
Subject: [PATCH v1] Improve comments for estimate_multivariate_ndistinct()

estimate_multivariate_ndistinct() is coded to assume the caller handles
passing it a list of GroupVarInfos with unique 'var' fields over the
entire list.  6bb6a62f3 added code which didn't ensure this and that
could result in estimate_multivariate_ndistinct() erroring out with:

ERROR:  corrupt MVNDistinct entry

This occurred because estimate_multivariate_ndistinct() first searches
for a set of stats that match to at least two of the given GroupVarInfos
and then later assumes that the MVNDistinctItem.items array of the
best matching stats will have an entry for those two columns.  If the
GroupVarInfos List contained a duplicate entry then the same column could
be matched to twice and that could trick the code into thinking we have
>= 2 columns matched in cases where only a single distinct column has been
matched.  This could result in a failure to find the correct
MVNDistinctItem in the stats as the array containing those never
contains an item for single columns.

Here we make it more clear that the function needs a distinct set of
GroupVarInfos and also tidy up a few other comments to make things a bit
easier to follow.
---
 src/backend/utils/adt/selfuncs.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index 588d991fa57..2585db7d692 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -4146,14 +4146,18 @@ estimate_hashagg_tablesize(PlannerInfo *root, Path *path,
  */
 
 /*
- * Find applicable ndistinct statistics for the given list of VarInfos (which
- * must all belong to the given rel), and update *ndistinct to the estimate of
- * the MVNDistinctItem that best matches.  If a match it found, *varinfos is
- * updated to remove the list of matched varinfos.
+ * Find the best matching ndistinct extended statistics for the given list of
+ * GroupVarInfos.
  *
- * Varinfos that aren't for simple Vars are ignored.
+ * Callers must ensure that the given GroupVarInfos all belong to 'rel' and
+ * the GroupVarInfos list does not contain any duplicate Vars or expressions.
  *
- * Return true if we're able to find a match, false otherwise.
+ * When statistics are found that match > 1 of the given GroupVarInfo, the
+ * *ndistinct parameter is set according to the ndistinct estimate and a new
+ * list is built with the matching GroupVarInfos removed which is output via
+ * the *varinfos parameter before returning true.  When no matching stats are
+ * found, false is returned and the *varinfos and *ndistinct parameters are
+ * left untouched.
  */
 static bool
 estimate_multivariate_ndistinct(PlannerInfo *root, RelOptInfo *rel,
@@ -4234,15 +4238,22 @@ estimate_multivariate_ndistinct(PlannerInfo *root, RelOptInfo *rel,
 			}
 		}
 
+		/*
+		 * The ndistinct extended statistics contain estimates for a minimum
+		 * of pairs of columns which the statistics are defined on and
+		 * certainly not single columns.  Here we skip unless we managed to
+		 * match to at least two columns.
+		 */
 		if (nshared_vars + nshared_exprs < 2)
 			continue;
 
 		/*
-		 * Does this statistics object match more columns than the currently
-		 * best object?  If so, use this one instead.
+		 * Check if these statistics are a better match than the previous best
+		 * match and if so, take note of the StatisticExtInfo.
 		 *
-		 * XXX This should break ties using name of the object, or something
-		 * like that, to make the outcome stable.
+		 * The statslist is sorted by statOid, so the StatisticExtInfo we
+		 * select as the best match is deterministic even when multiple sets
+		 * of statistics match equally as well.
 		 */
 		if ((nshared_exprs > nmatches_exprs) ||
 			(((nshared_exprs == nmatches_exprs)) && (nshared_vars > nmatches_vars)))
-- 
2.43.0



^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2
  2025-04-09 13:46 BUG #18885: ERROR: corrupt MVNDistinct entry - 2 PG Bug reporting form <noreply@postgresql.org>
  2025-04-09 22:47 ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Tomas Vondra <tomas@vondra.me>
  2025-04-13 23:26   ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 David Rowley <dgrowleyml@gmail.com>
@ 2025-04-15 09:41     ` Andrei Lepikhov <lepihov@gmail.com>
  2025-04-15 13:16       ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Tomas Vondra <tomas@vondra.me>
  0 siblings, 1 reply; 15+ messages in thread

From: Andrei Lepikhov @ 2025-04-15 09:41 UTC (permalink / raw)
  To: David Rowley <dgrowleyml@gmail.com>; +Cc: tharakan@gmail.com, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>; Alexander Korotkov <aekorotkov@gmail.com>; Tomas Vondra <tomas@vondra.me>

On 4/14/25 01:26, David Rowley wrote:
> just "continue;". The variable would only be needed if there was some
> inner loop and we couldn't use "continue".
+1
> * XXX Maybe we should allow searching the expressions even if we
> * found an attribute matching the expression? That would handle
> * trivial expressions like "(a)" but it seems fairly useless.
> */
> Maybe it meant "matching the Var"?
+1

I have read your modification of comments to 
estimate_multivariate_ndistinct. It suggested the idea of letting the 
GroupVarInfo struct be exported. For now, only add_unique_group_var or 
another internal selfuncs.c routine may build this list.
I'm not sure that the GroupVarInfo abstraction is shaped ideally at the 
moment, but it seems it may perform duties similar to RestrictInfo but 
for grouping keys: store the key, cache data about this key, cache 
statistics and estimations.
It would be the first step in letting modules use 
estimate_multivariate_ndistinct. Likewise, we can use 
statext_clauselist_selectivity and dependencies_clauselist_selectivity.

What do you think about that?

P.S. statext_mcv_clauselist_selectivity deserves to be exported, too.

-- 
regards, Andrei Lepikhov





^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2
  2025-04-09 13:46 BUG #18885: ERROR: corrupt MVNDistinct entry - 2 PG Bug reporting form <noreply@postgresql.org>
  2025-04-09 22:47 ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Tomas Vondra <tomas@vondra.me>
  2025-04-13 23:26   ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 David Rowley <dgrowleyml@gmail.com>
  2025-04-15 09:41     ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
@ 2025-04-15 13:16       ` Tomas Vondra <tomas@vondra.me>
  2025-04-16 04:11         ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 David Rowley <dgrowleyml@gmail.com>
  2025-04-16 06:57         ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
  0 siblings, 2 replies; 15+ messages in thread

From: Tomas Vondra @ 2025-04-15 13:16 UTC (permalink / raw)
  To: Andrei Lepikhov <lepihov@gmail.com>; David Rowley <dgrowleyml@gmail.com>; +Cc: tharakan@gmail.com, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>; Alexander Korotkov <aekorotkov@gmail.com>



On 4/15/25 11:41, Andrei Lepikhov wrote:
> On 4/14/25 01:26, David Rowley wrote:
>> just "continue;". The variable would only be needed if there was some
>> inner loop and we couldn't use "continue".
> +1
>> * XXX Maybe we should allow searching the expressions even if we
>> * found an attribute matching the expression? That would handle
>> * trivial expressions like "(a)" but it seems fairly useless.
>> */
>> Maybe it meant "matching the Var"?
> +1
> 
> I have read your modification of comments to
> estimate_multivariate_ndistinct. It suggested the idea of letting the
> GroupVarInfo struct be exported. For now, only add_unique_group_var or
> another internal selfuncs.c routine may build this list.
> I'm not sure that the GroupVarInfo abstraction is shaped ideally at the
> moment, but it seems it may perform duties similar to RestrictInfo but
> for grouping keys: store the key, cache data about this key, cache
> statistics and estimations.
> It would be the first step in letting modules use
> estimate_multivariate_ndistinct. Likewise, we can use
> statext_clauselist_selectivity and dependencies_clauselist_selectivity.
> 
> What do you think about that?
> 
> P.S. statext_mcv_clauselist_selectivity deserves to be exported, too.
> 

Not sure we'd want to export GroupVarInfo in the current shape. It was
meant for a short-lived local state (within a single function), which
seems quite different from what RestrictInfo does.

In any case, such rework seems out of scope for PG18 - it's much more
than a fix for the bug.


regards

-- 
Tomas Vondra






^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2
  2025-04-09 13:46 BUG #18885: ERROR: corrupt MVNDistinct entry - 2 PG Bug reporting form <noreply@postgresql.org>
  2025-04-09 22:47 ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Tomas Vondra <tomas@vondra.me>
  2025-04-13 23:26   ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 David Rowley <dgrowleyml@gmail.com>
  2025-04-15 09:41     ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
  2025-04-15 13:16       ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Tomas Vondra <tomas@vondra.me>
@ 2025-04-16 04:11         ` David Rowley <dgrowleyml@gmail.com>
  2025-04-21 09:41           ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Tomas Vondra <tomas@vondra.me>
  1 sibling, 1 reply; 15+ messages in thread

From: David Rowley @ 2025-04-16 04:11 UTC (permalink / raw)
  To: Tomas Vondra <tomas@vondra.me>; +Cc: Andrei Lepikhov <lepihov@gmail.com>; tharakan@gmail.com, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>; Alexander Korotkov <aekorotkov@gmail.com>

On Wed, 16 Apr 2025 at 01:16, Tomas Vondra <tomas@vondra.me> wrote:
> In any case, such rework seems out of scope for PG18 - it's much more
> than a fix for the bug.

This is the reason I stopped short of adjusting the code in the patch
I proposed in [1].  Speaking of which, do you have any views on that?
It seems much more likely that someone would have caught the issue
being reported here when writing the patch or during review if there
was some sort of documentation to mention the function can't handle
duplicate GroupVarInfos.

I'll look at that patch again in the next few days with aims to push
it unless someone has something to say about it.

David

[1] https://postgr.es/m/CAApHDvocZCUhM9W9mJ39d6oQz7ePKoqFnao_347mvC-A7QatcQ@mail.gmail.com





^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2
  2025-04-09 13:46 BUG #18885: ERROR: corrupt MVNDistinct entry - 2 PG Bug reporting form <noreply@postgresql.org>
  2025-04-09 22:47 ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Tomas Vondra <tomas@vondra.me>
  2025-04-13 23:26   ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 David Rowley <dgrowleyml@gmail.com>
  2025-04-15 09:41     ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
  2025-04-15 13:16       ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Tomas Vondra <tomas@vondra.me>
  2025-04-16 04:11         ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 David Rowley <dgrowleyml@gmail.com>
@ 2025-04-21 09:41           ` Tomas Vondra <tomas@vondra.me>
  2025-04-21 12:11             ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 David Rowley <dgrowleyml@gmail.com>
  0 siblings, 1 reply; 15+ messages in thread

From: Tomas Vondra @ 2025-04-21 09:41 UTC (permalink / raw)
  To: David Rowley <dgrowleyml@gmail.com>; +Cc: Andrei Lepikhov <lepihov@gmail.com>; tharakan@gmail.com, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>; Alexander Korotkov <aekorotkov@gmail.com>



On 4/16/25 06:11, David Rowley wrote:
> On Wed, 16 Apr 2025 at 01:16, Tomas Vondra <tomas@vondra.me> wrote:
>> In any case, such rework seems out of scope for PG18 - it's much more
>> than a fix for the bug.
> 
> This is the reason I stopped short of adjusting the code in the patch
> I proposed in [1].  Speaking of which, do you have any views on that?
> It seems much more likely that someone would have caught the issue
> being reported here when writing the patch or during review if there
> was some sort of documentation to mention the function can't handle
> duplicate GroupVarInfos.
> 
> I'll look at that patch again in the next few days with aims to push
> it unless someone has something to say about it.
> 

Yeah, you're right the estimate_multivariate_ndistinct() comments should
have mentioned that the GroupVarInfos are expected to be unique ... and
perhaps there could have been some assert to check that.


regards

-- 
Tomas Vondra






^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2
  2025-04-09 13:46 BUG #18885: ERROR: corrupt MVNDistinct entry - 2 PG Bug reporting form <noreply@postgresql.org>
  2025-04-09 22:47 ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Tomas Vondra <tomas@vondra.me>
  2025-04-13 23:26   ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 David Rowley <dgrowleyml@gmail.com>
  2025-04-15 09:41     ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
  2025-04-15 13:16       ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Tomas Vondra <tomas@vondra.me>
  2025-04-16 04:11         ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 David Rowley <dgrowleyml@gmail.com>
  2025-04-21 09:41           ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Tomas Vondra <tomas@vondra.me>
@ 2025-04-21 12:11             ` David Rowley <dgrowleyml@gmail.com>
  0 siblings, 0 replies; 15+ messages in thread

From: David Rowley @ 2025-04-21 12:11 UTC (permalink / raw)
  To: Tomas Vondra <tomas@vondra.me>; +Cc: Andrei Lepikhov <lepihov@gmail.com>; tharakan@gmail.com, pgsql-bugs@lists.postgresql.org, Alexander Korotkov <aekorotkov@gmail.com>

On Mon, 21 Apr 2025 at 21:41, Tomas Vondra <tomas@vondra.me> wrote:
> Yeah, you're right the estimate_multivariate_ndistinct() comments should
> have mentioned that the GroupVarInfos are expected to be unique ... and
> perhaps there could have been some assert to check that.

I did the comments in f3281f9f9.

David





^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2
  2025-04-09 13:46 BUG #18885: ERROR: corrupt MVNDistinct entry - 2 PG Bug reporting form <noreply@postgresql.org>
  2025-04-09 22:47 ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Tomas Vondra <tomas@vondra.me>
  2025-04-13 23:26   ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 David Rowley <dgrowleyml@gmail.com>
  2025-04-15 09:41     ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
  2025-04-15 13:16       ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Tomas Vondra <tomas@vondra.me>
@ 2025-04-16 06:57         ` Andrei Lepikhov <lepihov@gmail.com>
  1 sibling, 0 replies; 15+ messages in thread

From: Andrei Lepikhov @ 2025-04-16 06:57 UTC (permalink / raw)
  To: Tomas Vondra <tomas@vondra.me>; David Rowley <dgrowleyml@gmail.com>; +Cc: tharakan@gmail.com, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>; Alexander Korotkov <aekorotkov@gmail.com>

On 4/15/25 15:16, Tomas Vondra wrote:
> On 4/15/25 11:41, Andrei Lepikhov wrote:
>> What do you think about that? 
> Not sure we'd want to export GroupVarInfo in the current shape. It was
> meant for a short-lived local state (within a single function), which
> seems quite different from what RestrictInfo does.
> 
> In any case, such rework seems out of scope for PG18 - it's much more
> than a fix for the bug.
Of course, it is not for PG 18. I'm working on further GROUP-BY 
optimisation where columns are re-arranged in a way to put at the first 
position column with maximum ndistinct estimation. Additional work is 
needed that may be compensated by distinct caching.
One more ongoing project - correcting ndistinct estimation by passing 
through the EquivalenceClass and looking for a minimum ndistinct. It 
also needs a kinda cache. That's why I am pondering how to redefine 
GroupVarInfo to reduce estimation efforts.

-- 
regards, Andrei Lepikhov





^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2
  2025-04-09 13:46 BUG #18885: ERROR: corrupt MVNDistinct entry - 2 PG Bug reporting form <noreply@postgresql.org>
@ 2025-04-10 12:28 ` Andrei Lepikhov <lepihov@gmail.com>
  2025-04-11 22:30   ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Alexander Korotkov <aekorotkov@gmail.com>
  1 sibling, 1 reply; 15+ messages in thread

From: Andrei Lepikhov @ 2025-04-10 12:28 UTC (permalink / raw)
  To: tharakan@gmail.com, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>; Alexander Korotkov <aekorotkov@gmail.com>

On 4/9/25 15:46, PG Bug reporting form wrote:
> The following SQL triggers "ERROR: corrupt MVNDistinct entry", however this
> seems to be unrelated to a recent bugfix[1] for a similar issue (thus '2' in
> the $SUBJECT).
That's my fault.
We wanted to avoid using of the add_unique_group_var, but forgot it does 
some necessary stuff.
Here, I attempt to use this routine in the hash join bucket size 
estimation. I transformed it a little, made it more general. Not sure it 
is the best design, but it is debatable.

-- 
regards, Andrei Lepikhov

Attachments:

  [text/x-patch] 0001-Be-more-conventional-estimating-hash-join-bucket-siz.patch (5.6K, ../../ed14891b-d2ec-471a-a91f-2ea258c31795@gmail.com/2-0001-Be-more-conventional-estimating-hash-join-bucket-siz.patch)
  download | inline diff:
From c824159493b5caf27db9fe0ecb0f69a5e25f7713 Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" <lepihov@gmail.com>
Date: Thu, 10 Apr 2025 14:16:44 +0200
Subject: [PATCH] Be more conventional estimating hash join bucket size.

Implementing estimation of hash join bucket size by extended statistics
we wanted to avoid use of add_unique_group_var because its logic is too
specific for the groups number estimation routine. But we forgot that this
routine also does over stuff, like duplicates removal from the list
of estimated clauses.

With this commit we make add_unique_group_var a little more general that allows
to use it during the bucket estimation.
---
 src/backend/utils/adt/selfuncs.c        | 35 +++++++++++++------------
 src/test/regress/expected/stats_ext.out | 16 +++++++++++
 src/test/regress/sql/stats_ext.sql      |  8 ++++++
 3 files changed, 42 insertions(+), 17 deletions(-)

diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index 588d991fa57..8016c4dd3b0 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -3313,16 +3313,12 @@ typedef struct
 } GroupVarInfo;
 
 static List *
-add_unique_group_var(PlannerInfo *root, List *varinfos,
-					 Node *var, VariableStatData *vardata)
+add_unique_group_var(PlannerInfo *root, List *varinfos, Node *var,
+					 RelOptInfo *rel, double ndistinct, bool isdefault)
 {
 	GroupVarInfo *varinfo;
-	double		ndistinct;
-	bool		isdefault;
 	ListCell   *lc;
 
-	ndistinct = get_variable_numdistinct(vardata, &isdefault);
-
 	/*
 	 * The nullingrels bits within the var could cause the same var to be
 	 * counted multiple times if it's marked with different nullingrels.  They
@@ -3345,7 +3341,7 @@ add_unique_group_var(PlannerInfo *root, List *varinfos,
 		 * relations (see comments for estimate_num_groups).  We aren't too
 		 * fussy about the semantics of "equal" here.
 		 */
-		if (vardata->rel != varinfo->rel &&
+		if (rel != varinfo->rel &&
 			exprs_known_equal(root, var, varinfo->var, InvalidOid))
 		{
 			if (varinfo->ndistinct <= ndistinct)
@@ -3364,7 +3360,7 @@ add_unique_group_var(PlannerInfo *root, List *varinfos,
 	varinfo = (GroupVarInfo *) palloc(sizeof(GroupVarInfo));
 
 	varinfo->var = var;
-	varinfo->rel = vardata->rel;
+	varinfo->rel = rel;
 	varinfo->ndistinct = ndistinct;
 	varinfo->isdefault = isdefault;
 	varinfos = lappend(varinfos, varinfo);
@@ -3532,8 +3528,12 @@ estimate_num_groups(PlannerInfo *root, List *groupExprs, double input_rows,
 		examine_variable(root, groupexpr, 0, &vardata);
 		if (HeapTupleIsValid(vardata.statsTuple) || vardata.isunique)
 		{
-			varinfos = add_unique_group_var(root, varinfos,
-											groupexpr, &vardata);
+			double	ndistinct;
+			bool	isdefault;
+
+			ndistinct = get_variable_numdistinct(&vardata, &isdefault);
+			varinfos = add_unique_group_var(root, varinfos, groupexpr,
+											vardata.rel, ndistinct, isdefault);
 			ReleaseVariableStats(vardata);
 			continue;
 		}
@@ -3569,9 +3569,13 @@ estimate_num_groups(PlannerInfo *root, List *groupExprs, double input_rows,
 		foreach(l2, varshere)
 		{
 			Node	   *var = (Node *) lfirst(l2);
+			double		ndistinct;
+			bool		isdefault;
 
 			examine_variable(root, var, 0, &vardata);
-			varinfos = add_unique_group_var(root, varinfos, var, &vardata);
+			ndistinct = get_variable_numdistinct(&vardata, &isdefault);
+			varinfos = add_unique_group_var(root, varinfos, var, vardata.rel,
+											ndistinct, isdefault);
 			ReleaseVariableStats(vardata);
 		}
 	}
@@ -3880,12 +3884,9 @@ estimate_multivariate_bucketsize(PlannerInfo *root, RelOptInfo *inner,
 					 */
 					continue;
 
-				varinfo = (GroupVarInfo *) palloc(sizeof(GroupVarInfo));
-				varinfo->var = expr;
-				varinfo->rel = root->simple_rel_array[relid];
-				varinfo->ndistinct = 0.0;
-				varinfo->isdefault = false;
-				varinfos = lappend(varinfos, varinfo);
+				varinfos = add_unique_group_var(root, varinfos, expr,
+												root->simple_rel_array[relid],
+												0.0, false);
 
 				/*
 				 * Remember the link to RestrictInfo for the case the clause
diff --git a/src/test/regress/expected/stats_ext.out b/src/test/regress/expected/stats_ext.out
index 686d8c93aa8..472ef0e44ae 100644
--- a/src/test/regress/expected/stats_ext.out
+++ b/src/test/regress/expected/stats_ext.out
@@ -3427,4 +3427,20 @@ SELECT * FROM sb_1 a, sb_2 b WHERE a.x = b.x AND a.y = b.y AND a.z = b.z;
          ->  Seq Scan on sb_2 b
 (5 rows)
 
+-- Check that the Hash Join bucket size estimator detects equal clauses correctly.
+SET enable_nestloop = 'off';
+SET enable_mergejoin = 'off';
+EXPLAIN (COSTS OFF)
+SELECT FROM sb_1 LEFT JOIN sb_2 ON (sb_2.x=sb_1.x) AND (sb_1.x=sb_2.x);
+                       QUERY PLAN                       
+--------------------------------------------------------
+ Hash Left Join
+   Hash Cond: ((sb_1.x = sb_2.x) AND (sb_1.x = sb_2.x))
+   ->  Seq Scan on sb_1
+   ->  Hash
+         ->  Seq Scan on sb_2
+(5 rows)
+
+RESET enable_nestloop;
+RESET enable_mergejoin;
 DROP TABLE sb_1, sb_2 CASCADE;
diff --git a/src/test/regress/sql/stats_ext.sql b/src/test/regress/sql/stats_ext.sql
index b71a6cd089f..001ec72026a 100644
--- a/src/test/regress/sql/stats_ext.sql
+++ b/src/test/regress/sql/stats_ext.sql
@@ -1747,4 +1747,12 @@ ANALYZE sb_2;
 EXPLAIN (COSTS OFF) -- Choose hash join
 SELECT * FROM sb_1 a, sb_2 b WHERE a.x = b.x AND a.y = b.y AND a.z = b.z;
 
+-- Check that the Hash Join bucket size estimator detects equal clauses correctly.
+SET enable_nestloop = 'off';
+SET enable_mergejoin = 'off';
+EXPLAIN (COSTS OFF)
+SELECT FROM sb_1 LEFT JOIN sb_2 ON (sb_2.x=sb_1.x) AND (sb_1.x=sb_2.x);
+RESET enable_nestloop;
+RESET enable_mergejoin;
+
 DROP TABLE sb_1, sb_2 CASCADE;
-- 
2.39.5



^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2
  2025-04-09 13:46 BUG #18885: ERROR: corrupt MVNDistinct entry - 2 PG Bug reporting form <noreply@postgresql.org>
  2025-04-10 12:28 ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
@ 2025-04-11 22:30   ` Alexander Korotkov <aekorotkov@gmail.com>
  2025-04-14 14:41     ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
  0 siblings, 1 reply; 15+ messages in thread

From: Alexander Korotkov @ 2025-04-11 22:30 UTC (permalink / raw)
  To: Andrei Lepikhov <lepihov@gmail.com>; +Cc: tharakan@gmail.com, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>

Hi, Andrei!

On Thu, Apr 10, 2025 at 3:28 PM Andrei Lepikhov <lepihov@gmail.com> wrote:
>
> On 4/9/25 15:46, PG Bug reporting form wrote:
> > The following SQL triggers "ERROR: corrupt MVNDistinct entry", however this
> > seems to be unrelated to a recent bugfix[1] for a similar issue (thus '2' in
> > the $SUBJECT).
> That's my fault.
> We wanted to avoid using of the add_unique_group_var, but forgot it does
> some necessary stuff.
> Here, I attempt to use this routine in the hash join bucket size
> estimation. I transformed it a little, made it more general. Not sure it
> is the best design, but it is debatable.

Thank you for the fix, but it's not enough.

If you would replace a new regression tests query with this, it would
fail with an assertion.

SELECT FROM sb_1 LEFT JOIN sb_2 ON (sb_2.x=sb_1.x) AND (sb_1.x=sb_2.x)
AND (sb_1.y=sb_2.y);

When you use add_unique_group_var() which keeps varinfos unique then
you can no longer expect that varinfos have the same order as
origin_rinfos.

------
Regards,
Alexander Korotkov
Supabase





^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2
  2025-04-09 13:46 BUG #18885: ERROR: corrupt MVNDistinct entry - 2 PG Bug reporting form <noreply@postgresql.org>
  2025-04-10 12:28 ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
  2025-04-11 22:30   ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Alexander Korotkov <aekorotkov@gmail.com>
@ 2025-04-14 14:41     ` Andrei Lepikhov <lepihov@gmail.com>
  2025-04-16 10:00       ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
  0 siblings, 1 reply; 15+ messages in thread

From: Andrei Lepikhov @ 2025-04-14 14:41 UTC (permalink / raw)
  To: Alexander Korotkov <aekorotkov@gmail.com>; Tomas Vondra <tomas@vondra.me>; +Cc: tharakan@gmail.com, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>

On 4/12/25 00:30, Alexander Korotkov wrote:
> On Thu, Apr 10, 2025 at 3:28 PM Andrei Lepikhov <lepihov@gmail.com> wrote:
>> ...
>> Here, I attempt to use this routine in the hash join bucket size
>> estimation. I transformed it a little, made it more general. Not sure it
>> is the best design, but it is debatable.
> ...
> SELECT FROM sb_1 LEFT JOIN sb_2 ON (sb_2.x=sb_1.x) AND (sb_1.x=sb_2.x)
> AND (sb_1.y=sb_2.y);
> 
> When you use add_unique_group_var() which keeps varinfos unique then
> you can no longer expect that varinfos have the same order as
> origin_rinfos.
Ok, here is a patch that considers this issue. Now GroupVarInfo tracks 
source RestrictInfo. Not sure it is an ideal approach, but we don't need 
to synchronise the restrictions and corresponding varinfos.

On Fri, 11 Apr 2025 at 01:31, Tomas Vondra <tomas@vondra.me> wrote:
 > I think estimate_multivariate_bucketsize() needs to be more careful
 > about building the GroupVarInfo list - in particular, it needs to do
 > dance with examine_variable + add_unique_group_var + pull_var_clause,
 > similar to estimate_num_groups() at line ~3532.
Yeah, estimate_num_groups and bucket size estimation have a lot in 
common. It would be better to invent some common GroupVarInfo 
preparation/estimation code for them, but specifics of HashJoin bucket 
estimation need mcv_freq and result caching that limits intersection of 
these estimators.

-- 
regards, Andrei Lepikhov

Attachments:

  [text/x-patch] v0-0001-Be-more-conventional-in-estimating-hash-join-buck.patch (9.3K, ../../c9225640-e2dc-4d9c-a598-0d1d1673b11f@gmail.com/2-v0-0001-Be-more-conventional-in-estimating-hash-join-buck.patch)
  download | inline diff:
From 2df9c47f188ed1a8ae8b70865c6a5ddff6dbe766 Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" <lepihov@gmail.com>
Date: Thu, 10 Apr 2025 14:16:44 +0200
Subject: [PATCH v0] Be more conventional in estimating hash join bucket size.

Implementing estimation of hash join bucket size by extended statistics, we
wanted to avoid the use of add_unique_group_var because its logic greatly
depends on the group's number estimation code. But we forgot that this routine
also does over stuff, like duplicate removal from the list of estimated
clauses.
With this commit, we make add_unique_group_var a little more general, which
allows us to use it during the bucket estimation. At first, it doesn't need
vardata receiving RelOptInfo reference, ndistinct and isdefault values
directly. Also, GroupVarInfo tracks original restriction - it is a palliative
that simplifies the detection of clauses that have not yet been estimated.
---
 src/backend/utils/adt/selfuncs.c        | 77 +++++++++++--------------
 src/test/regress/expected/stats_ext.out | 28 +++++++++
 src/test/regress/sql/stats_ext.sql      | 11 ++++
 3 files changed, 72 insertions(+), 44 deletions(-)

diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index 588d991fa57..b88a6bf5822 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -3310,19 +3310,17 @@ typedef struct
 	RelOptInfo *rel;			/* relation it belongs to */
 	double		ndistinct;		/* # distinct values */
 	bool		isdefault;		/* true if DEFAULT_NUM_DISTINCT was used */
+	RestrictInfo *source;		/* Source restriction to surely identification */
 } GroupVarInfo;
 
 static List *
-add_unique_group_var(PlannerInfo *root, List *varinfos,
-					 Node *var, VariableStatData *vardata)
+add_unique_group_var(PlannerInfo *root, List *varinfos, Node *var,
+					 RelOptInfo *rel, double ndistinct, bool isdefault,
+					 RestrictInfo *source)
 {
 	GroupVarInfo *varinfo;
-	double		ndistinct;
-	bool		isdefault;
 	ListCell   *lc;
 
-	ndistinct = get_variable_numdistinct(vardata, &isdefault);
-
 	/*
 	 * The nullingrels bits within the var could cause the same var to be
 	 * counted multiple times if it's marked with different nullingrels.  They
@@ -3345,7 +3343,7 @@ add_unique_group_var(PlannerInfo *root, List *varinfos,
 		 * relations (see comments for estimate_num_groups).  We aren't too
 		 * fussy about the semantics of "equal" here.
 		 */
-		if (vardata->rel != varinfo->rel &&
+		if (rel != varinfo->rel &&
 			exprs_known_equal(root, var, varinfo->var, InvalidOid))
 		{
 			if (varinfo->ndistinct <= ndistinct)
@@ -3364,9 +3362,10 @@ add_unique_group_var(PlannerInfo *root, List *varinfos,
 	varinfo = (GroupVarInfo *) palloc(sizeof(GroupVarInfo));
 
 	varinfo->var = var;
-	varinfo->rel = vardata->rel;
+	varinfo->rel = rel;
 	varinfo->ndistinct = ndistinct;
 	varinfo->isdefault = isdefault;
+	varinfo->source = source;
 	varinfos = lappend(varinfos, varinfo);
 	return varinfos;
 }
@@ -3532,8 +3531,13 @@ estimate_num_groups(PlannerInfo *root, List *groupExprs, double input_rows,
 		examine_variable(root, groupexpr, 0, &vardata);
 		if (HeapTupleIsValid(vardata.statsTuple) || vardata.isunique)
 		{
-			varinfos = add_unique_group_var(root, varinfos,
-											groupexpr, &vardata);
+			double	ndistinct;
+			bool	isdefault;
+
+			ndistinct = get_variable_numdistinct(&vardata, &isdefault);
+			varinfos = add_unique_group_var(root, varinfos, groupexpr,
+											vardata.rel, ndistinct, isdefault,
+											NULL);
 			ReleaseVariableStats(vardata);
 			continue;
 		}
@@ -3569,9 +3573,13 @@ estimate_num_groups(PlannerInfo *root, List *groupExprs, double input_rows,
 		foreach(l2, varshere)
 		{
 			Node	   *var = (Node *) lfirst(l2);
+			double		ndistinct;
+			bool		isdefault;
 
 			examine_variable(root, var, 0, &vardata);
-			varinfos = add_unique_group_var(root, varinfos, var, &vardata);
+			ndistinct = get_variable_numdistinct(&vardata, &isdefault);
+			varinfos = add_unique_group_var(root, varinfos, var, vardata.rel,
+											ndistinct, isdefault, NULL);
 			ReleaseVariableStats(vardata);
 		}
 	}
@@ -3816,13 +3824,9 @@ estimate_multivariate_bucketsize(PlannerInfo *root, RelOptInfo *inner,
 		ListCell   *lc;
 		int			relid = -1;
 		List	   *varinfos = NIL;
-		List	   *origin_rinfos = NIL;
 		double		mvndistinct;
-		List	   *origin_varinfos;
 		int			group_relid = -1;
 		RelOptInfo *group_rel = NULL;
-		ListCell   *lc1,
-				   *lc2;
 
 		/*
 		 * Find clauses, referencing the same single base relation and try to
@@ -3835,7 +3839,6 @@ estimate_multivariate_bucketsize(PlannerInfo *root, RelOptInfo *inner,
 			RestrictInfo *rinfo = lfirst_node(RestrictInfo, lc);
 			Node	   *expr;
 			Relids		relids;
-			GroupVarInfo *varinfo;
 
 			/*
 			 * Find the inner side of the join, which we need to estimate the
@@ -3880,18 +3883,9 @@ estimate_multivariate_bucketsize(PlannerInfo *root, RelOptInfo *inner,
 					 */
 					continue;
 
-				varinfo = (GroupVarInfo *) palloc(sizeof(GroupVarInfo));
-				varinfo->var = expr;
-				varinfo->rel = root->simple_rel_array[relid];
-				varinfo->ndistinct = 0.0;
-				varinfo->isdefault = false;
-				varinfos = lappend(varinfos, varinfo);
-
-				/*
-				 * Remember the link to RestrictInfo for the case the clause
-				 * is failed to be estimated.
-				 */
-				origin_rinfos = lappend(origin_rinfos, rinfo);
+				varinfos = add_unique_group_var(root, varinfos, expr,
+												root->simple_rel_array[relid],
+												0.0, false, rinfo);
 			}
 			else
 				/* This clause can't be estimated with extended statistics */
@@ -3906,22 +3900,23 @@ estimate_multivariate_bucketsize(PlannerInfo *root, RelOptInfo *inner,
 			 * Multivariate statistics doesn't apply to single columns except
 			 * for expressions, but it has not been implemented yet.
 			 */
-			otherclauses = list_concat(otherclauses, origin_rinfos);
+
+			if (varinfos != NULL)
+				otherclauses = lappend(otherclauses,
+								((GroupVarInfo *) linitial(varinfos))->source);
 			list_free_deep(varinfos);
-			list_free(origin_rinfos);
 			continue;
 		}
 
 		Assert(group_rel != NULL);
 
 		/* Employ the extended statistics. */
-		origin_varinfos = varinfos;
 		for (;;)
 		{
-			bool		estimated = estimate_multivariate_ndistinct(root,
-																	group_rel,
-																	&varinfos,
-																	&mvndistinct);
+			bool	estimated = estimate_multivariate_ndistinct(root,
+																group_rel,
+																&varinfos,
+																&mvndistinct);
 
 			if (!estimated)
 				break;
@@ -3936,19 +3931,13 @@ estimate_multivariate_bucketsize(PlannerInfo *root, RelOptInfo *inner,
 			Assert(ndistinct >= 1.0);
 		}
 
-		Assert(list_length(origin_varinfos) == list_length(origin_rinfos));
-
 		/* Collect unmatched clauses as otherclauses. */
-		forboth(lc1, origin_varinfos, lc2, origin_rinfos)
+		foreach(lc, varinfos)
 		{
-			GroupVarInfo *vinfo = lfirst(lc1);
-
-			if (!list_member_ptr(varinfos, vinfo))
-				/* Already estimated */
-				continue;
+			GroupVarInfo *varinfo = lfirst(lc);
 
 			/* Can't be estimated here - push to the returning list */
-			otherclauses = lappend(otherclauses, lfirst(lc2));
+			otherclauses = lappend(otherclauses, varinfo->source);
 		}
 	}
 
diff --git a/src/test/regress/expected/stats_ext.out b/src/test/regress/expected/stats_ext.out
index 686d8c93aa8..6359e5fb689 100644
--- a/src/test/regress/expected/stats_ext.out
+++ b/src/test/regress/expected/stats_ext.out
@@ -3427,4 +3427,32 @@ SELECT * FROM sb_1 a, sb_2 b WHERE a.x = b.x AND a.y = b.y AND a.z = b.z;
          ->  Seq Scan on sb_2 b
 (5 rows)
 
+-- Check that the Hash Join bucket size estimator detects equal clauses correctly.
+SET enable_nestloop = 'off';
+SET enable_mergejoin = 'off';
+EXPLAIN (COSTS OFF)
+SELECT FROM sb_1 LEFT JOIN sb_2 ON (sb_2.x=sb_1.x) AND (sb_1.x=sb_2.x);
+                       QUERY PLAN                       
+--------------------------------------------------------
+ Hash Left Join
+   Hash Cond: ((sb_1.x = sb_2.x) AND (sb_1.x = sb_2.x))
+   ->  Seq Scan on sb_1
+   ->  Hash
+         ->  Seq Scan on sb_2
+(5 rows)
+
+EXPLAIN (COSTS OFF)
+SELECT FROM sb_1 LEFT JOIN sb_2
+   ON (sb_2.x=sb_1.x) AND (sb_1.x=sb_2.x) AND (sb_1.y=sb_2.y);
+                                  QUERY PLAN                                  
+------------------------------------------------------------------------------
+ Hash Left Join
+   Hash Cond: ((sb_1.x = sb_2.x) AND (sb_1.y = sb_2.y) AND (sb_1.x = sb_2.x))
+   ->  Seq Scan on sb_1
+   ->  Hash
+         ->  Seq Scan on sb_2
+(5 rows)
+
+RESET enable_nestloop;
+RESET enable_mergejoin;
 DROP TABLE sb_1, sb_2 CASCADE;
diff --git a/src/test/regress/sql/stats_ext.sql b/src/test/regress/sql/stats_ext.sql
index b71a6cd089f..da4f2fe9c93 100644
--- a/src/test/regress/sql/stats_ext.sql
+++ b/src/test/regress/sql/stats_ext.sql
@@ -1747,4 +1747,15 @@ ANALYZE sb_2;
 EXPLAIN (COSTS OFF) -- Choose hash join
 SELECT * FROM sb_1 a, sb_2 b WHERE a.x = b.x AND a.y = b.y AND a.z = b.z;
 
+-- Check that the Hash Join bucket size estimator detects equal clauses correctly.
+SET enable_nestloop = 'off';
+SET enable_mergejoin = 'off';
+EXPLAIN (COSTS OFF)
+SELECT FROM sb_1 LEFT JOIN sb_2 ON (sb_2.x=sb_1.x) AND (sb_1.x=sb_2.x);
+EXPLAIN (COSTS OFF)
+SELECT FROM sb_1 LEFT JOIN sb_2
+   ON (sb_2.x=sb_1.x) AND (sb_1.x=sb_2.x) AND (sb_1.y=sb_2.y);
+RESET enable_nestloop;
+RESET enable_mergejoin;
+
 DROP TABLE sb_1, sb_2 CASCADE;
-- 
2.39.5



^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2
  2025-04-09 13:46 BUG #18885: ERROR: corrupt MVNDistinct entry - 2 PG Bug reporting form <noreply@postgresql.org>
  2025-04-10 12:28 ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
  2025-04-11 22:30   ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Alexander Korotkov <aekorotkov@gmail.com>
  2025-04-14 14:41     ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
@ 2025-04-16 10:00       ` Andrei Lepikhov <lepihov@gmail.com>
  2025-04-21 20:23         ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Alexander Korotkov <aekorotkov@gmail.com>
  0 siblings, 1 reply; 15+ messages in thread

From: Andrei Lepikhov @ 2025-04-16 10:00 UTC (permalink / raw)
  To: Alexander Korotkov <aekorotkov@gmail.com>; +Cc: tharakan@gmail.com, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>; Tomas Vondra <tomas@vondra.me>

On 4/14/25 16:41, Andrei Lepikhov wrote:
> On 4/12/25 00:30, Alexander Korotkov wrote:
>> When you use add_unique_group_var() which keeps varinfos unique then
>> you can no longer expect that varinfos have the same order as
>> origin_rinfos.
> Ok, here is a patch that considers this issue. Now GroupVarInfo tracks 
> source RestrictInfo. Not sure it is an ideal approach, but we don't need 
> to synchronise the restrictions and corresponding varinfos.
This is the new version of the patch.
I don't like the previous version because it was too invasive. Also, 
add_unique_group_var checks "known equal" keys. It is not applicable in 
the current case, of course, but it still seems suspicious: equality 
expressions may be applied at an upper level of the join tree, and 
equality doesn't exist at this specific place yet. Being correct in the 
case of GROUP-BY operator estimation, such conjecture may distort 
estimation in the middle of the join tree.
With the current patch, we just stay away from that doubtful code.

-- 
regards, Andrei Lepikhov

Attachments:

  [text/x-patch] v1-0001-Make-the-necessary-preparations-before-estimating.patch (4.2K, ../../7cab95a3-0358-4102-96d1-8c94ff3c6fdb@gmail.com/2-v1-0001-Make-the-necessary-preparations-before-estimating.patch)
  download | inline diff:
From b55a84c5b7d6f86409acb87291986bf6b7714f0e Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" <lepihov@gmail.com>
Date: Thu, 10 Apr 2025 14:16:44 +0200
Subject: [PATCH v1] Make the necessary preparations before estimating the hash
 join bucket size.

To estimate with extended statistics, it is necessary to clear
the varnullingrels field in the expression, and duplicates are not allowed
in the GroupVarInfo list.
---
 src/backend/utils/adt/selfuncs.c        | 26 +++++++++++++++++++++++
 src/test/regress/expected/stats_ext.out | 28 +++++++++++++++++++++++++
 src/test/regress/sql/stats_ext.sql      | 11 ++++++++++
 3 files changed, 65 insertions(+)

diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index 588d991fa57..2fd1e177c20 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -3850,6 +3850,8 @@ estimate_multivariate_bucketsize(PlannerInfo *root, RelOptInfo *inner,
 			if (bms_get_singleton_member(relids, &relid) &&
 				root->simple_rel_array[relid]->statlist != NIL)
 			{
+				bool is_duplicate = false;
+
 				/*
 				 * This inner-side expression references only one relation.
 				 * Extended statistics on this clause can exist.
@@ -3880,6 +3882,30 @@ estimate_multivariate_bucketsize(PlannerInfo *root, RelOptInfo *inner,
 					 */
 					continue;
 
+				/* Clear nullingrels to correctly match hash keys */
+				expr = remove_nulling_relids(expr, root->outer_join_rels, NULL);
+
+				/*
+				 * Detect and exclude exact duplicates from the list of
+				 * hash keys (like add_unique_group_var does).
+				 */
+				foreach(lc1, varinfos)
+				{
+					varinfo = (GroupVarInfo *) lfirst(lc1);
+
+					if (!equal(expr, varinfo->var))
+						continue;
+
+					is_duplicate = true;
+					break;
+				}
+				if (is_duplicate)
+					/*
+					 * Skip exact duplicates. Adding them to the otherclauses
+					 * list also doesn't make sense.
+					 */
+					continue;
+
 				varinfo = (GroupVarInfo *) palloc(sizeof(GroupVarInfo));
 				varinfo->var = expr;
 				varinfo->rel = root->simple_rel_array[relid];
diff --git a/src/test/regress/expected/stats_ext.out b/src/test/regress/expected/stats_ext.out
index 686d8c93aa8..6359e5fb689 100644
--- a/src/test/regress/expected/stats_ext.out
+++ b/src/test/regress/expected/stats_ext.out
@@ -3427,4 +3427,32 @@ SELECT * FROM sb_1 a, sb_2 b WHERE a.x = b.x AND a.y = b.y AND a.z = b.z;
          ->  Seq Scan on sb_2 b
 (5 rows)
 
+-- Check that the Hash Join bucket size estimator detects equal clauses correctly.
+SET enable_nestloop = 'off';
+SET enable_mergejoin = 'off';
+EXPLAIN (COSTS OFF)
+SELECT FROM sb_1 LEFT JOIN sb_2 ON (sb_2.x=sb_1.x) AND (sb_1.x=sb_2.x);
+                       QUERY PLAN                       
+--------------------------------------------------------
+ Hash Left Join
+   Hash Cond: ((sb_1.x = sb_2.x) AND (sb_1.x = sb_2.x))
+   ->  Seq Scan on sb_1
+   ->  Hash
+         ->  Seq Scan on sb_2
+(5 rows)
+
+EXPLAIN (COSTS OFF)
+SELECT FROM sb_1 LEFT JOIN sb_2
+   ON (sb_2.x=sb_1.x) AND (sb_1.x=sb_2.x) AND (sb_1.y=sb_2.y);
+                                  QUERY PLAN                                  
+------------------------------------------------------------------------------
+ Hash Left Join
+   Hash Cond: ((sb_1.x = sb_2.x) AND (sb_1.y = sb_2.y) AND (sb_1.x = sb_2.x))
+   ->  Seq Scan on sb_1
+   ->  Hash
+         ->  Seq Scan on sb_2
+(5 rows)
+
+RESET enable_nestloop;
+RESET enable_mergejoin;
 DROP TABLE sb_1, sb_2 CASCADE;
diff --git a/src/test/regress/sql/stats_ext.sql b/src/test/regress/sql/stats_ext.sql
index b71a6cd089f..da4f2fe9c93 100644
--- a/src/test/regress/sql/stats_ext.sql
+++ b/src/test/regress/sql/stats_ext.sql
@@ -1747,4 +1747,15 @@ ANALYZE sb_2;
 EXPLAIN (COSTS OFF) -- Choose hash join
 SELECT * FROM sb_1 a, sb_2 b WHERE a.x = b.x AND a.y = b.y AND a.z = b.z;
 
+-- Check that the Hash Join bucket size estimator detects equal clauses correctly.
+SET enable_nestloop = 'off';
+SET enable_mergejoin = 'off';
+EXPLAIN (COSTS OFF)
+SELECT FROM sb_1 LEFT JOIN sb_2 ON (sb_2.x=sb_1.x) AND (sb_1.x=sb_2.x);
+EXPLAIN (COSTS OFF)
+SELECT FROM sb_1 LEFT JOIN sb_2
+   ON (sb_2.x=sb_1.x) AND (sb_1.x=sb_2.x) AND (sb_1.y=sb_2.y);
+RESET enable_nestloop;
+RESET enable_mergejoin;
+
 DROP TABLE sb_1, sb_2 CASCADE;
-- 
2.39.5



^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2
  2025-04-09 13:46 BUG #18885: ERROR: corrupt MVNDistinct entry - 2 PG Bug reporting form <noreply@postgresql.org>
  2025-04-10 12:28 ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
  2025-04-11 22:30   ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Alexander Korotkov <aekorotkov@gmail.com>
  2025-04-14 14:41     ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
  2025-04-16 10:00       ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
@ 2025-04-21 20:23         ` Alexander Korotkov <aekorotkov@gmail.com>
  2025-04-23 06:52           ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
  0 siblings, 1 reply; 15+ messages in thread

From: Alexander Korotkov @ 2025-04-21 20:23 UTC (permalink / raw)
  To: Andrei Lepikhov <lepihov@gmail.com>; +Cc: tharakan@gmail.com, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>; Tomas Vondra <tomas@vondra.me>

On Wed, Apr 16, 2025 at 1:00 PM Andrei Lepikhov <lepihov@gmail.com> wrote:
> On 4/14/25 16:41, Andrei Lepikhov wrote:
> > On 4/12/25 00:30, Alexander Korotkov wrote:
> >> When you use add_unique_group_var() which keeps varinfos unique then
> >> you can no longer expect that varinfos have the same order as
> >> origin_rinfos.
> > Ok, here is a patch that considers this issue. Now GroupVarInfo tracks
> > source RestrictInfo. Not sure it is an ideal approach, but we don't need
> > to synchronise the restrictions and corresponding varinfos.
> This is the new version of the patch.
> I don't like the previous version because it was too invasive. Also,
> add_unique_group_var checks "known equal" keys. It is not applicable in
> the current case, of course, but it still seems suspicious: equality
> expressions may be applied at an upper level of the join tree, and
> equality doesn't exist at this specific place yet. Being correct in the
> case of GROUP-BY operator estimation, such conjecture may distort
> estimation in the middle of the join tree.
> With the current patch, we just stay away from that doubtful code.

This patch looks good to me.  I've added to comments to clarify the
things and revised the commit message.  I'm going to push this if no
objections.

------
Regards,
Alexander Korotkov
Supabase

Attachments:

  [application/octet-stream] v2-0001-Properly-prepare-varinfos-in-estimate_multivariat.patch (6.2K, ../../CAPpHfdsTQ1DAEaoGyDA8NZn-ywK4AQ0brDyiac8t=LCft0voAQ@mail.gmail.com/2-v2-0001-Properly-prepare-varinfos-in-estimate_multivariat.patch)
  download | inline diff:
From bf0b78db8292a16b8e7bd587ef86102e56b426f0 Mon Sep 17 00:00:00 2001
From: "Andrei V. Lepikhov" <lepihov@gmail.com>
Date: Thu, 10 Apr 2025 14:16:44 +0200
Subject: [PATCH v2] Properly prepare varinfos in
 estimate_multivariate_bucketsize()

To estimate with extended statistics, we need to clear the varnullingrels
field in the expression, and duplicates are not allowed in the GroupVarInfo
list.  We might re-use add_unique_group_var(), but we don't do so for two
reasons.

  1) We must keep the origin_rinfos list ordered exactly the same way as
     varinfos.
  2) add_unique_group_var() is designed for estimate_num_groups(), where a
     larger number of groups is worse.   While estimating the number of hash
     buckets, we have the opposite: a lesser number of groups is worse.
     Therefore, we don't have to remove "known equal" vars: the removed var
     may valuably contribute to the multivariate statistics to grow the number
     of groups.

Reported-by: Robins Tharakan <tharakan@gmail.com>
Discussion: https://postgr.es/m/18885-da51324078588253%40postgresql.org
Author: Andrei Lepikhov <lepihov@gmail.com>
Reviewed-by: David Rowley <dgrowleyml@gmail.com>
Reviewed-by: Tomas Vondra <tomas@vondra.me>
Reviewed-by: Alexander Korotkov <aekorotkov@gmail.com>
---
 src/backend/utils/adt/selfuncs.c        | 51 +++++++++++++++++++++++++
 src/test/regress/expected/stats_ext.out | 28 ++++++++++++++
 src/test/regress/sql/stats_ext.sql      | 11 ++++++
 3 files changed, 90 insertions(+)

diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index 987f2154459..0241984da5b 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -3850,6 +3850,8 @@ estimate_multivariate_bucketsize(PlannerInfo *root, RelOptInfo *inner,
 			if (bms_get_singleton_member(relids, &relid) &&
 				root->simple_rel_array[relid]->statlist != NIL)
 			{
+				bool		is_duplicate = false;
+
 				/*
 				 * This inner-side expression references only one relation.
 				 * Extended statistics on this clause can exist.
@@ -3880,6 +3882,53 @@ estimate_multivariate_bucketsize(PlannerInfo *root, RelOptInfo *inner,
 					 */
 					continue;
 
+				/*
+				 * We're going to add the new clause to the varinfos list.  We
+				 * might re-use add_unique_group_var(), but we don't do so for
+				 * two reasons.
+				 *
+				 * 1) We must keep the origin_rinfos list ordered exactly the
+				 * same way as varinfos.
+				 *
+				 * 2) add_unique_group_var() is designed for
+				 * estimate_num_groups(), where a larger number of groups is
+				 * worse.   While estimating the number of hash buckets, we
+				 * have the opposite: a lesser number of groups is worse.
+				 * Therefore, we don't have to remove "known equal" vars: the
+				 * removed var may valuably contribute to the multivariate
+				 * statistics to grow the number of groups.
+				 */
+
+				/*
+				 * Clear nullingrels to correctly match hash keys.  See
+				 * add_unique_group_var()'s comment for details.
+				 */
+				expr = remove_nulling_relids(expr, root->outer_join_rels, NULL);
+
+				/*
+				 * Detect and exclude exact duplicates from the list of hash
+				 * keys (like add_unique_group_var does).
+				 */
+				foreach(lc1, varinfos)
+				{
+					varinfo = (GroupVarInfo *) lfirst(lc1);
+
+					if (!equal(expr, varinfo->var))
+						continue;
+
+					is_duplicate = true;
+					break;
+				}
+
+				if (is_duplicate)
+				{
+					/*
+					 * Skip exact duplicates. Adding them to the otherclauses
+					 * list also doesn't make sense.
+					 */
+					continue;
+				}
+
 				varinfo = (GroupVarInfo *) palloc(sizeof(GroupVarInfo));
 				varinfo->var = expr;
 				varinfo->rel = root->simple_rel_array[relid];
@@ -3894,8 +3943,10 @@ estimate_multivariate_bucketsize(PlannerInfo *root, RelOptInfo *inner,
 				origin_rinfos = lappend(origin_rinfos, rinfo);
 			}
 			else
+			{
 				/* This clause can't be estimated with extended statistics */
 				otherclauses = lappend(otherclauses, rinfo);
+			}
 
 			clauses = foreach_delete_current(clauses, lc);
 		}
diff --git a/src/test/regress/expected/stats_ext.out b/src/test/regress/expected/stats_ext.out
index 686d8c93aa8..6359e5fb689 100644
--- a/src/test/regress/expected/stats_ext.out
+++ b/src/test/regress/expected/stats_ext.out
@@ -3427,4 +3427,32 @@ SELECT * FROM sb_1 a, sb_2 b WHERE a.x = b.x AND a.y = b.y AND a.z = b.z;
          ->  Seq Scan on sb_2 b
 (5 rows)
 
+-- Check that the Hash Join bucket size estimator detects equal clauses correctly.
+SET enable_nestloop = 'off';
+SET enable_mergejoin = 'off';
+EXPLAIN (COSTS OFF)
+SELECT FROM sb_1 LEFT JOIN sb_2 ON (sb_2.x=sb_1.x) AND (sb_1.x=sb_2.x);
+                       QUERY PLAN                       
+--------------------------------------------------------
+ Hash Left Join
+   Hash Cond: ((sb_1.x = sb_2.x) AND (sb_1.x = sb_2.x))
+   ->  Seq Scan on sb_1
+   ->  Hash
+         ->  Seq Scan on sb_2
+(5 rows)
+
+EXPLAIN (COSTS OFF)
+SELECT FROM sb_1 LEFT JOIN sb_2
+   ON (sb_2.x=sb_1.x) AND (sb_1.x=sb_2.x) AND (sb_1.y=sb_2.y);
+                                  QUERY PLAN                                  
+------------------------------------------------------------------------------
+ Hash Left Join
+   Hash Cond: ((sb_1.x = sb_2.x) AND (sb_1.y = sb_2.y) AND (sb_1.x = sb_2.x))
+   ->  Seq Scan on sb_1
+   ->  Hash
+         ->  Seq Scan on sb_2
+(5 rows)
+
+RESET enable_nestloop;
+RESET enable_mergejoin;
 DROP TABLE sb_1, sb_2 CASCADE;
diff --git a/src/test/regress/sql/stats_ext.sql b/src/test/regress/sql/stats_ext.sql
index b71a6cd089f..da4f2fe9c93 100644
--- a/src/test/regress/sql/stats_ext.sql
+++ b/src/test/regress/sql/stats_ext.sql
@@ -1747,4 +1747,15 @@ ANALYZE sb_2;
 EXPLAIN (COSTS OFF) -- Choose hash join
 SELECT * FROM sb_1 a, sb_2 b WHERE a.x = b.x AND a.y = b.y AND a.z = b.z;
 
+-- Check that the Hash Join bucket size estimator detects equal clauses correctly.
+SET enable_nestloop = 'off';
+SET enable_mergejoin = 'off';
+EXPLAIN (COSTS OFF)
+SELECT FROM sb_1 LEFT JOIN sb_2 ON (sb_2.x=sb_1.x) AND (sb_1.x=sb_2.x);
+EXPLAIN (COSTS OFF)
+SELECT FROM sb_1 LEFT JOIN sb_2
+   ON (sb_2.x=sb_1.x) AND (sb_1.x=sb_2.x) AND (sb_1.y=sb_2.y);
+RESET enable_nestloop;
+RESET enable_mergejoin;
+
 DROP TABLE sb_1, sb_2 CASCADE;
-- 
2.39.5 (Apple Git-154)



^ permalink  raw  reply  [nested|flat] 15+ messages in thread

* Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2
  2025-04-09 13:46 BUG #18885: ERROR: corrupt MVNDistinct entry - 2 PG Bug reporting form <noreply@postgresql.org>
  2025-04-10 12:28 ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
  2025-04-11 22:30   ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Alexander Korotkov <aekorotkov@gmail.com>
  2025-04-14 14:41     ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
  2025-04-16 10:00       ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Andrei Lepikhov <lepihov@gmail.com>
  2025-04-21 20:23         ` Re: BUG #18885: ERROR: corrupt MVNDistinct entry - 2 Alexander Korotkov <aekorotkov@gmail.com>
@ 2025-04-23 06:52           ` Andrei Lepikhov <lepihov@gmail.com>
  0 siblings, 0 replies; 15+ messages in thread

From: Andrei Lepikhov @ 2025-04-23 06:52 UTC (permalink / raw)
  To: Alexander Korotkov <aekorotkov@gmail.com>; +Cc: tharakan@gmail.com, pgsql-bugs@lists.postgresql.org, PG Bug reporting form <noreply@postgresql.org>; Tomas Vondra <tomas@vondra.me>

On 4/21/25 22:23, Alexander Korotkov wrote:
> On Wed, Apr 16, 2025 at 1:00 PM Andrei Lepikhov <lepihov@gmail.com> wrote:
>> On 4/14/25 16:41, Andrei Lepikhov wrote:
>>> On 4/12/25 00:30, Alexander Korotkov wrote:
>>>> When you use add_unique_group_var() which keeps varinfos unique then
>>>> you can no longer expect that varinfos have the same order as
>>>> origin_rinfos.
>>> Ok, here is a patch that considers this issue. Now GroupVarInfo tracks
>>> source RestrictInfo. Not sure it is an ideal approach, but we don't need
>>> to synchronise the restrictions and corresponding varinfos.
>> This is the new version of the patch.
>> I don't like the previous version because it was too invasive. Also,
>> add_unique_group_var checks "known equal" keys. It is not applicable in
>> the current case, of course, but it still seems suspicious: equality
>> expressions may be applied at an upper level of the join tree, and
>> equality doesn't exist at this specific place yet. Being correct in the
>> case of GROUP-BY operator estimation, such conjecture may distort
>> estimation in the middle of the join tree.
>> With the current patch, we just stay away from that doubtful code.
> 
> This patch looks good to me.  I've added to comments to clarify the
> things and revised the commit message.  I'm going to push this if no
> objections.
Looks better than my version. No objections.

Maybe only a few words could be added - why do we initialise ndistinct 
and isdefault with these specific values: an extended statistic doesn't 
employ per-column values, and we avoid spending cycles initialising them 
with 'undefined' semantics.

-- 
regards, Andrei Lepikhov





^ permalink  raw  reply  [nested|flat] 15+ messages in thread


end of thread, other threads:[~2025-04-23 06:52 UTC | newest]

Thread overview: 15+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-04-09 13:46 BUG #18885: ERROR: corrupt MVNDistinct entry - 2 PG Bug reporting form <noreply@postgresql.org>
2025-04-09 22:47 ` Tomas Vondra <tomas@vondra.me>
2025-04-13 23:26   ` David Rowley <dgrowleyml@gmail.com>
2025-04-15 09:41     ` Andrei Lepikhov <lepihov@gmail.com>
2025-04-15 13:16       ` Tomas Vondra <tomas@vondra.me>
2025-04-16 04:11         ` David Rowley <dgrowleyml@gmail.com>
2025-04-21 09:41           ` Tomas Vondra <tomas@vondra.me>
2025-04-21 12:11             ` David Rowley <dgrowleyml@gmail.com>
2025-04-16 06:57         ` Andrei Lepikhov <lepihov@gmail.com>
2025-04-10 12:28 ` Andrei Lepikhov <lepihov@gmail.com>
2025-04-11 22:30   ` Alexander Korotkov <aekorotkov@gmail.com>
2025-04-14 14:41     ` Andrei Lepikhov <lepihov@gmail.com>
2025-04-16 10:00       ` Andrei Lepikhov <lepihov@gmail.com>
2025-04-21 20:23         ` Alexander Korotkov <aekorotkov@gmail.com>
2025-04-23 06:52           ` Andrei Lepikhov <lepihov@gmail.com>

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox