From 90874e0fd9a6e46dc6717b2ab3fe27aa9fe918d2 Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Mon, 17 Jun 2024 16:25:27 +0200 Subject: [PATCH v20240617 31/56] review --- src/backend/statistics/mcv.c | 12 ++++++++++++ src/test/regress/sql/ext_sort_mcv_proc.sql | 3 +++ 2 files changed, 15 insertions(+) diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c index 96f4c1b94f6..367efdbb391 100644 --- a/src/backend/statistics/mcv.c +++ b/src/backend/statistics/mcv.c @@ -74,6 +74,13 @@ // #define DEBUG_MCV 1 /* should be removed after review. */ +/* + * XXX The patch says "optimize the order of mcv equal function evaluation" + * but how is this new struct related to that? + * + * XXX Anyway, if we want to do this, surely this is not the only opportunity + * to replace a couple separate variables with a struct wrapping them logically? + */ typedef struct { FmgrInfo fmgrinfo; @@ -84,6 +91,11 @@ typedef struct #endif } McvProc; +/* + * XXX What's the reasoning behind reordering the functions like this? Doesn't it + * have the same issues with unpredictable behavior like the GROUP BY patch, which + * got eventually reverted and reworked? + */ static int cmp_mcv_proc(const void *a, const void *b) { diff --git a/src/test/regress/sql/ext_sort_mcv_proc.sql b/src/test/regress/sql/ext_sort_mcv_proc.sql index 09360d5b23e..14677dd02f7 100644 --- a/src/test/regress/sql/ext_sort_mcv_proc.sql +++ b/src/test/regress/sql/ext_sort_mcv_proc.sql @@ -1,3 +1,6 @@ +-- FIXME would be nice to explain what's the point of the test, what it tries to verify +-- FIXME maybe use "costs off"? +-- FIXME where's the expected output for the test? also, not added to the schedule create table t(level_1 text, level_2 text, level_3 text); insert into t -- 2.45.2