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
