RE: Skip partition tuple routing with constant partition key

Hi Amit-san

From: Amit Langote <amitlangote09(at)gmail(dot)com>
> On Fri, Jun 4, 2021 at 6:05 PM Amit Langote <amitlangote09(at)gmail(dot)com>
> wrote:
> > On Fri, Jun 4, 2021 at 4:38 PM Amit Langote <amitlangote09(at)gmail(dot)com>
> wrote:
> > > On Thu, Jun 3, 2021 at 8:48 PM Amit Langote <amitlangote09(at)gmail(dot)com>
> wrote:
> > > > On Tue, Jun 1, 2021 at 5:43 PM houzj(dot)fnst(at)fujitsu(dot)com
> > > > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > > > > So, If we want to share the cached partition between statements,
> > > > > we seems cannot use ExecPartitionCheck. Instead, I tried
> > > > > directly invoke the partition support
> > > > > function(partsupfunc) to check If the cached info is correct. In
> > > > > this approach I tried cache the *bound offset* in
> > > > > PartitionDescData, and we can use the bound offset to get the
> > > > > bound datum from PartitionBoundInfoData and invoke the
> partsupfunc to do the CHECK.
> > > > >
> > > > > Attach a POC patch about it. Just to share an idea about sharing
> > > > > cached partition info between statements.
> > > >
> > > > I have not looked at your patch yet, but yeah that's what I would
> > > > imagine doing it.
> > >
> > > Just read it and think it looks promising.
> > >
> > > On code, I wonder why not add the rechecking-cached-offset code
> > > directly in get_partiiton_for_tuple(), instead of adding a whole new
> > > function for that. Can you please check the attached revised version?
>
> I should have clarified a bit more on why I think a new function looked
> unnecessary to me. The thing about that function that bothered me was that
> it appeared to duplicate a lot of code fragments of get_partition_for_tuple().
> That kind of duplication often leads to bugs of omission later if something from
> either function needs to change.

Thanks for the patch and explanation, I think you are right that it’s better add
the rechecking-cached-offset code directly in get_partiiton_for_tuple().

And now, I think maybe it's time to try to optimize the performance.
Currently, if every row to be inserted in a statement belongs to different
partition, then the cache check code will bring a slight performance
degradation(AFAICS: 2% ~ 4%).

So, If we want to solve this, then we may need 1) a reloption to let user control whether use the cache.
Or, 2) introduce some simple strategy to control whether use cache automatically.

I have not write a patch about 1) reloption, because I think it will be nice if we can
enable this cache feature by default. So, I attached a WIP patch about approach 2).

The rough idea is to check the average batch number every 1000 rows.
If the average batch num is greater than 1, then we enable the cache check,
if not, disable cache check. This is similar to what 0d5f05cde0 did.

Thoughts ?

Best regards,
houzj