Re: Ought to use heap_multi_insert() for pg_attribute/depend insertions? - Mailing list pgsql-hackers

> On 29 Jun 2020, at 08:57, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Jun 26, 2020 at 02:26:50PM +0200, Daniel Gustafsson wrote:
>> On 26 Jun 2020, at 10:11, Michael Paquier <michael@paquier.xyz> wrote:
>>> +   /* TODO is nreferenced a reasonable allocation of slots? */
>>> +   slot = palloc(sizeof(TupleTableSlot *) * nreferenced);
>>> It seems to me that we could just apply the same rule as for
>>> pg_attribute and pg_shdepend, no?
>>
>> I think so, I see no reason not to.
>
> I was actually looking a patch to potentially support REINDEX for
> partitioned table, and the CONCURRENTLY case may need this patch,
> still if a lot of dependencies are switched at once it may be a
> problem, so it is better to cap it.

Agreed.

> One thing though is that we may finish by allocating more slots than what's
> necessary if some dependencies are pinned, but using multi-inserts would be a
> gain anyway, and the patch does not insert in more slots than needed.

I was playing around with a few approaches around this when I initially wrote
this, but I was unable to find any way to calculate the correct number of slots
which wasn't significantly more expensive than the extra allocations.

> Attached is a rebased set, with more splitting work done after a new
> round of review.  0001 is more refactoring to use more
> ObjectAddress[Sub]Set() where we can, leading to some more cleanup:
> 5 files changed, 43 insertions(+), 120 deletions(-)

This patch seems quite straightforward, and in the case of the loops in for
example CreateConstraintEntry() makes the code a lot more readable.  +1 for
applying 0001.

> In this round, I got confused with the way ObjectAddress items are
> assigned, assuming that we have to use the same dependency type for a
> bunch of dependencies to attach.  Using add_exact_object_address() is
> fine for this purpose, but this also makes me wonder if we should try
> to think harder about this interface so as we would be able to insert
> a bunch of dependency tuples with multiple types of dependencies
> handled.  So this has made me remove reset_object_addresses() from the
> patch series, as it was used when dependency types were mixed up.  We
> could also discuss that separately, but that's not strongly mandatory
> here.

Ok, once the final state of this patchset is known I can take a stab at
recording multiple dependencies with different behaviors as a separate
patchset.

> There are however cases where it is straight-forward to gather and insert
> multiple records, like in InsertExtensionTuple() (as does already
> tsearchcmds.c), which is what 0002 does.


+1 on 0002 as well.

> An opposite example is StorePartitionKey(), where there is a mix of normal and
> internal dependencies, so I have removed it for now.


I don't think it makes the code any worse off to handle the NORMAL dependencies
separate here, but MVV.

> ExecDropSingleTupleTableSlot() was also called for an incorrect number
> of slots when it came to pg_shdepend.

Hmm, don't you mean in heap.c:InsertPgAttributeTuples()?

> I was thinking if it could be possible to do more consolidation between the
> three places where we calculate the number of slots to use, but that would also
> mean to have more tuple slot dependency moving around, which is not great.


If we do, we need to keep the cap consistent across all callers, else we'll end
up with an API without an abstraction to make it worth more than saving a few
lines of quite simple to read code.  Currently this is the case, but that might
not always hold, so not sure it if it's worth it.

0001 + 0002 + 0003 pass tests on my machine and nothing sticks out.

cheers ./daniel