Re: Skip partition tuple routing with constant partition key - Mailing list pgsql-hackers

From Zhihong Yu
Subject Re: Skip partition tuple routing with constant partition key
Date
Msg-id CALNJ-vQnG+EDEdnehzY2NeuSyDpPyMeAi29Vw6iK6e_91xsNUA@mail.gmail.com
Whole thread Raw
In response to Re: Skip partition tuple routing with constant partition key  (Amit Langote <amitlangote09@gmail.com>)
Responses Re: Skip partition tuple routing with constant partition key
List pgsql-hackers


On Wed, Jun 16, 2021 at 10:37 PM Amit Langote <amitlangote09@gmail.com> wrote:
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
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

pgsql-hackers by date:

Previous
From: Amit Langote
Date:
Subject: Re: Decoding speculative insert with toast leaks memory
Next
From: Fabien COELHO
Date:
Subject: Re: pgbench logging broken by time logic changes