> On 14 Aug 2020, at 20:23, Alvaro Herrera <alvhe...@2ndquadrant.com> wrote:
> The logic to keep track number of used slots used is baroque, though -- that > could use a lot of simplification. What if slot_init was an integer which increments together with the loop variable until max_slots is reached? If so, maybe it should be renamed slot_init_count and slotCount renamed slot_stored_count to make the their use clearer? > On 31 Aug 2020, at 09:56, Michael Paquier <mich...@paquier.xyz> wrote: > It has been a couple of weeks, and I am not really sure what is the > suggestion here. So I would like to move on with this patch set as > the changes are straight-forward using the existing routines for > object addresses by grouping all insert dependencies of the same type. > Are there any objections? I'm obviously biased but I think this patchset is a net win. There are more things we can do in this space, but it's a good start. > Attached is a rebased set, where I have added in indexing.h a unique > definition for the hard limit of 64kB for the amount of data that can > be inserted at once, based on the suggestion from Alvaro and Andres. +#define MAX_CATALOG_INSERT_BYTES 65535 This name, and inclusion in a headerfile, implies that the definition is somewhat generic as opposed to its actual use. Using MULTIINSERT rather than INSERT in the name would clarify I reckon. A few other comments: + /* + * Allocate the slots to use, but delay initialization until we know that + * they will be used. + */ I think this comment warrants a longer explanation on why they wont all be used, or perhaps none of them (which is the real optimization win here). + ObjectAddresses *addrs_auto; + ObjectAddresses *addrs_normal; It's not for this patch, but it seems a logical next step would be to be able to record the DependencyType as well when collecting deps rather than having to create multiple buckets. + /* Normal dependency from a domain to its collation. */ + /* We know the default collation is pinned, so don't bother recording it */ It's just moved and not introduced in this patch, but shouldn't these two lines be joined into a normal block comment? cheers ./daniel
