Re: Speeding up INSERTs and UPDATEs to partitioned tables

On 2018/11/04 19:07, David Rowley wrote:
> On 1 November 2018 at 22:39, Amit Langote <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)jp> wrote:
> I've attached v13 which hopefully addresses these.

Thank you for updating the patch.

>> The macro naming discussion got me thinking today about the macro itself.
>> It encapsulates access to the various PartitionTupleRouting arrays
>> containing the maps, but maybe we've got the interface of tuple routing a
>> bit (maybe a lot given this thread!) wrong to begin with. Instead of
>> ExecFindPartition returning indexes into various PartitionTupleRouting
>> arrays and its callers then using those indexes to fetch various objects
>> from those arrays, why doesn't it return those objects itself? Although
>> we made sure that the callers don't need to worry about the meaning of
>> these indexes changing with this patch, it still seems a bit odd for them
>> to have to go back to those arrays to get various objects.
>>
>> How about we change ExecFindPartition's interface so that it returns the
>> ResultRelInfo, the two maps, and the partition slot? So, the arrays
>> simply become a cache for ExecFindPartition et al and are no longer
>> accessed outside execPartition.c. Although that makes the interface of
>> ExecFindPartition longer, I think it reduces overall complexity.
>
> I don't really think stuffing values into a bunch of output parameters
> is much of an improvement. I'd rather allow callers to fetch what they
> need using the index we return. Most callers don't need to know about
> the child to parent maps, so it seems nicer for those places not to
> have to invent a dummy variable to pass along to ExecFindPartition()
> so it can needlessly populate it for them.

Well, if a caller finds a partition using ExecFindPartition, it's going to
need to fetch those other objects anyway. Both of its callers that exist
today, CopyFrom and ExecPrepareTupleRouting, fetch both maps and the slot
in the same code block as ExecFindPartition.

> Perhaps a better design would be to instead of having random special
> partitioned-table-only fields in ResultRelInfo, just have an extra
> struct there that contains the extra partition information which would
> include the translation maps and then just return the ResultRelInfo
> and allow callers to lookup any extra details they require.

IIUC, you're saying that we could introduce a new struct that contains
auxiliary information needed by partition pruning (maps, slot, etc. for
tuple conversion) and add a new ResultRelInfo member of that struct type.
That way, there is no need to return them separately or return an index to
access them from their arrays. I guess we won't even need the arrays we
have now. I think that might be a good idea and simplifies things
significantly.

It reminds me of how ResultRelInfo grew a ri_onConflict member of type
OnConflictSetState [1]. We decided to go that way, as opposed to the
earlier approach of having arrays of num_partitions length in
ModifyTableState or PartitionTupleRouting that contained ON CONFLICT
related objects for individual partitions.

> I've not
> looked into this in detail, but I think the committer work that's
> required for the patch as it is today is already quite significant.
> I'm not keen on warding any willing one off by making the commit job
> any harder. I agree that it would be good to stabilise the API for
> all this partitioning code sometime, but I don't believe it needs to
> be done all in one commit. My intent here is to improve performance or
> INSERT and UPDATE on partitioned tables. Perhaps we can shape some API
> redesign later in the release cycle. What do you think?

I do suspect that simplifying ExecFindPartition's interface as part of
patch will make a committer's life easier, as the resulting interface is
simpler, especially if we revise it like you suggest above.

Thanks,
Amit

[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=555ee77a9