On Tue, Apr 02, 2024 at 04:23:45PM +0800, Andy Fan wrote:
>
> 0001 is your patch, I just rebase them against the current master. 0006
> is not much relevant with current patch, and I think it can be committed
> individually if you are OK with that.
Your 002 should also remove listidx to avoid warning
../src/backend/statistics/extended_stats.c:2879:8: error: variable 'listidx' set but not used
[-Werror,-Wunused-but-set-variable]
> Subject: [PATCH v1 2/8] Remove estimiatedcluases and varRelid arguments
> @@ -2939,15 +2939,11 @@ statext_try_join_estimates(PlannerInfo *root, List *clauses, int varRelid,
> /* needs to happen before skipping any clauses */
> listidx++;
>
> - /* Skip clauses that were already estimated. */
> - if (bms_is_member(listidx, estimatedclauses))
> - continue;
> -
Your 007 could instead test if relids == NULL:
> Subject: [PATCH v1 7/8] bms_is_empty is more effective than bms_num_members(b)
>- if (bms_num_members(relids) == 0)
>+ if (bms_is_empty(relids))
typos:
001: s/heuristict/heuristics/
002: s/grantee/guarantee/
002: s/estimiatedcluases/estimatedclauses/
It'd be nice to fix/silence these warnings from 001:
|../src/backend/statistics/extended_stats.c:3151:36: warning: ‘relid’ may be used uninitialized
[-Wmaybe-uninitialized]
| 3151 | if (var->varno != relid)
| | ^
|../src/backend/statistics/extended_stats.c:3104:33: note: ‘relid’ was declared here
| 3104 | int relid;
| | ^~~~~
|[1016/1893] Compiling C object src/backend/postgres_lib.a.p/statistics_mcv.c.o
|../src/backend/statistics/mcv.c: In function ‘mcv_combine_extended’:
|../src/backend/statistics/mcv.c:2431:49: warning: declaration of ‘idx’ shadows a previous local
[-Wshadow=compatible-local]
FYI, I also ran the patch with a $large number of reports without
observing any errors or crashes.
I'll try to look harder at the next patch revision.
--
Justin
