From: | Michael Paquier <michael(at)paquier(dot)xyz> |
---|---|
To: | Daniel Gustafsson <daniel(at)yesql(dot)se> |
Cc: | Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Postgres hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: Switch to multi-inserts for pg_depend |
Date: | 2020-09-03 05:35:06 |
Message-ID: | 20200903053506.GC14963@paquier.xyz |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Sep 01, 2020 at 11:53:36AM +0200, Daniel Gustafsson wrote:
> On 14 Aug 2020, at 20:23, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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?
Good idea, removing slot_init looks like a good thing for readability.
And the same can be done for pg_shdepend.
> On 31 Aug 2020, at 09:56, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> +#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.
Makes sense, I have switched to MAX_CATALOG_MULTI_INSERT_BYTES.
> 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).
Okay, I have updated the comments where this formulation is used.
Does that look adapted to you?
> + 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.
Yeah, agreed. I am not sure yet how to design those APIs. One option
is to use a set of an array with DependencyType elements, each one
storing a list of dependencies of the same type.
> + /* 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?
Okay, done.
--
Michael
Attachment | Content-Type | Size |
---|---|---|
v3-0001-Use-multi-inserts-for-pg_depend.patch | text/x-diff | 12.0 KB |
v3-0002-Switch-to-multi-insert-dependencies-for-many-code.patch | text/x-diff | 40.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Tom Lane | 2020-09-03 05:36:16 | Re: A micro-optimisation for walkdir() |
Previous Message | Andrey V. Lepikhov | 2020-09-03 05:14:41 | Re: Ideas about a better API for postgres_fdw remote estimates |