Hi,
Thanks for reading the patch.
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.
--
Amit Langote
EDB: http://www.enterprisedb.com