On Wed, Jun 16, 2021 at 10:37 PM Amit Langote <amitlangot...@gmail.com>
wrote:
> Hi,
>
> Thanks for reading the patch.
>
> On Thu, Jun 17, 2021 at 1:46 PM Zhihong Yu <z...@yugabyte.com> wrote:
> > On Wed, Jun 16, 2021 at 9:29 PM Amit Langote <amitlangot...@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
Hi, Amit:
Thanks for the quick response.
w.r.t. the last point, since variable equal is defined within the case of
PARTITION_STRATEGY_RANGE,
I wonder if it can be named don_t_care or something like that.
That way, it would be clearer to the reader that its value is purposefully
not checked.
It is fine to leave the variable as is since this was existing code.
Cheers
