Hi. On 2018/04/05 0:45, Jesper Pedersen wrote: > Hi, > > On 04/04/2018 09:29 AM, David Rowley wrote: >> Thanks for updating. I've made a pass over v49 and I didn't find very >> much wrong with it. >> >> The only real bug I found was a missing IsA(rinfo->clause, Const) in >> the pseudoconstant check inside >> generate_partition_pruning_steps_internal.
Fixed. >> Most of the changes are comment fixes with a few stylistic changes >> thrown which are pretty much all there just to try to shrink the code >> a line or two or reduce indentation. >> >> I feel pretty familiar with this code now and assuming the attached is >> included I'm happy for someone else, hopefully, a committer to take a >> look at it. Thank you, your changes look good to me. >> I'll leave the following notes: >> >> 1. Still not sure about RelOptInfo->has_default_part. This flag is >> only looked at in generate_partition_pruning_steps. The RelOptInfo and >> the boundinfo is available to look at, it's just that the >> partition_bound_has_default macro is defined in partition.c rather >> than partition.h. Hmm, it might not be such a bad idea to bring out the PartitionBoundInfoData into partition.h. If we do that, we won't need the has_default_part that the patch adds to RelOptInfo. In the Attached v50 set, 0002 does that. >> 2. Don't really like the new isopne variable name. It's not very >> simple to decode, perhaps something like is_not_eq is better? isopne does sound a bit unintelligible. I propose op_is_ne so that it sounds consistent with the preceding member of the struct that's called opno. I want to keep "ne" and not start calling it not_eq, as a few other places use the string "ne" to refer to a similar thing, like: /* inequality */ Datum range_ne(PG_FUNCTION_ARGS) Datum timestamptz_ne_date(PG_FUNCTION_ARGS) Since the field is local to partprune.c, I guess that it's fine as the comment where it's defined tells what it is. >> 3. The part of the code I'm least familiar with is >> get_steps_using_prefix_recurse(). I admit to not having had time to >> fully understand that and consider ways to break it. The purpose of that code is to generate *all* needed steps to be combined using COMBINE_INTERSECT such that the pruning will occur using the most restrictive set of clauses in cases where the same key is referenced in multiple restriction clauses containing non-equality operators. So, for a range partitioned table on (a, b): For a query like explain select * from foo a <= 1 and a <= 3 and b < 5 and b <= 10 Pruning steps generated to be combined with an enclosing INTERSECT step will be as follows: <= (1, 10) < (1, 5) <= (3, 10) < (3, 5) >> Marking as ready for committer. Thank you! > Passes check-world, and CommitFest app has been updated to reflect the > current patch set. Trivial changes attached. Merged these changes. Thanks again Jesper. Attached v50. Thanks, Amit