On 2017/11/06 12:53, David Rowley wrote: > On 3 November 2017 at 17:32, David Rowley <david.row...@2ndquadrant.com> > wrote: >> 2. This code is way more complex than it needs to be. >> >> if (num_parts > 0) >> { >> int j; >> >> all_indexes = (int *) palloc(num_parts * sizeof(int)); >> j = 0; >> if (min_part_idx >= 0 && max_part_idx >= 0) >> { >> for (i = min_part_idx; i <= max_part_idx; i++) >> all_indexes[j++] = i; >> } >> if (!bms_is_empty(other_parts)) >> while ((i = bms_first_member(other_parts)) >= 0) >> all_indexes[j++] = i; >> if (j > 1) >> qsort((void *) all_indexes, j, sizeof(int), intcmp); >> } >> >> It looks like the min/max partition stuff is just complicating things >> here. If you need to build this array of all_indexes[] anyway, I don't >> quite understand the point of the min/max. It seems like min/max would >> probably work a bit nicer if you didn't need the other_parts >> BitmapSet, so I recommend just getting rid of min/max completely and >> just have a BitmapSet with bit set for each partition's index you >> need, you'd not need to go to the trouble of performing a qsort on an >> array and you could get rid of quite a chunk of code too. >> >> The entire function would then not be much more complex than: >> >> partindexes = get_partitions_from_clauses(parent, partclauses); >> >> while ((i = bms_first_member(partindexes)) >= 0) >> { >> AppendRelInfo *appinfo = rel->part_appinfos[i]; >> result = lappend(result, appinfo); >> } >> >> Then you can also get rid of your intcmp() function too. > > I've read a bit more of the patch and I think even more now that the > min/max stuff should be removed.
Oops, I didn't catch this email before sending my earlier reply. Thanks for the bms range patch. Will reply to this shortly after reading your patch and thinking on it a bit. Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers