Re: Skip partition tuple routing with constant partition key

From: David Rowley <dgrowleyml(at)gmail(dot)com>
To: Amit Langote <amitlangote09(at)gmail(dot)com>
Cc: Zhihong Yu <zyu(at)yugabyte(dot)com>, Greg Stark <stark(at)mit(dot)edu>, "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "tsunakawa(dot)takay(at)fujitsu(dot)com" <tsunakawa(dot)takay(at)fujitsu(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Subject: Re: Skip partition tuple routing with constant partition key
Date: 2022-07-28 02:59:21
Message-ID: CAApHDvop6r1MC9Adht7O_oc8ENS2DD2YcUL70re_dKFhG4+8cQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Thu, 28 Jul 2022 at 00:50, Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> So, in a way the caching scheme works for
> LIST partitioning only if the same value appears consecutively in the
> input set, whereas it does not for *a set of* values belonging to the
> same partition appearing consecutively. Maybe that's a reasonable
> restriction for now.

I'm not really seeing another cheap enough way of doing that. Any LIST
partition could allow any number of values. We've only space to record
1 of those values by way of recording which element in the
PartitionBound that it was located.

> if (part_index < 0)
> - part_index = boundinfo->default_index;
> + {
> + /*
> + * Since we don't do caching for the default partition or failed
> + * lookups, we'll just wipe the cache fields back to their initial
> + * values. The count becomes 0 rather than 1 as 1 means it's the
> + * first time we've found a partition we're recording for the cache.
> + */
> + partdesc->last_found_datum_index = -1;
> + partdesc->last_found_part_index = -1;
> + partdesc->last_found_count = 0;
> +
> + return boundinfo->default_index;
> + }
>
> I wonder why not to leave the cache untouched in this case? It's
> possible that erratic rows only rarely occur in the input sets.

I looked into that and I ended up just removing the code to reset the
cache. It now works similarly to a LIST partitioned table's NULL
partition.

> Should the comment update above get_partition_for_tuple() mention
> something like the cached path is basically O(1) and the non-cache
> path O (log N) as I can see in comments in some other modules, like
> pairingheap.c?

I adjusted for the other things you mentioned but I didn't add the big
O stuff. I thought the comment was clear enough.

I'd quite like to push this patch early next week, so if anyone else
is following along that might have any objections, could they do so
before then?

David

Attachment Content-Type Size
v13_cache_last_partition.patch text/plain 12.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2022-07-28 03:21:28 Re: [BUG] Logical replication failure "ERROR: could not map filenode "base/13237/442428" to relation OID" with catalog modifying txns
Previous Message Julien Rouhaud 2022-07-28 02:58:45 Re: Official Windows Installer and Documentation