On Thu, Jun 17, 2021 at 1:46 PM Zhihong Yu <zyu@yugabyte.com> wrote: > On Wed, Jun 16, 2021 at 9:29 PM Amit Langote <amitlangote09@gmail.com> wrote: >> Attached a slightly revised version of that patch, with a commit >> message this time. > > + int n_tups_inserted; > + int n_offset_changed; > > Since tuples appear in plural, maybe offset should be as well: offsets.
I was hoping one would read that as "the number of times the offset changed" while inserting "that many tuples", so the singular form makes more sense to me.
Actually, I even considered naming the variable n_offsets_seen, in which case the plural form makes sense, but I chose not to go with that name.
> + part_index = get_cached_list_partition(pd, boundinfo, key, > + values); > > nit:either put values on the same line, or align the 4 parameters on different lines.
Not sure pgindent requires us to follow that style, but I too prefer the way you suggest. It does make the patch a bit longer though.
> + if (part_index < 0) > + { > + bound_offset = partition_range_datum_bsearch(key->partsupfunc, > > Do we need to check the value of equal before computing part_index ?
Just in case you didn't notice, this is not new code, but appears as a diff hunk due to indenting.
As for whether the code should be checking 'equal', I don't think the logic at this particular site should do that. Requiring 'equal' to be true would mean that this code would only accept tuples that exactly match the bound that partition_range_datum_bsearch() returned.
Updated patch attached. Aside from addressing your 2nd point, I fixed a typo in a comment.