Re: Ought to use heap_multi_insert() for pg_attribute/depend insertions?

From: Michael Paquier <michael(at)paquier(dot)xyz>
To: Daniel Gustafsson <daniel(at)yesql(dot)se>
Cc: Andres Freund <andres(at)anarazel(dot)de>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Ought to use heap_multi_insert() for pg_attribute/depend insertions?
Date: 2020-06-26 08:11:13
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Thu, Jun 25, 2020 at 09:38:23AM +0200, Daniel Gustafsson wrote:
> Attached is a rebased version which was updated to handle the changes for op
> class parameters introduced in 911e70207703799605.

Thanks for the updated version.

While re-reading the code, I got cold feet with the changes done in
recordDependencyOn(). Sure, we could do as you propose, but that does
not have to be part of this patch I think, aimed at switching more
catalogs to use multi inserts, and it just feels a duplicate of
recordMultipleDependencies(), with the same comments copied all over
the place, etc.

MAX_TEMPLATE_BYTES in pg_shdepend.c needs a comment to explain that
this is to cap the number of slots used in
copyTemplateDependencies() for pg_shdepend.

Not much a fan of the changes in GenerateTypeDependencies(),
particularly the use of refobjs[8], capped to the number of items from
typeForm. If we add new members I think that this would break
easily without us actually noticing that it broke. The use of
ObjectAddressSet() is a good idea though, but it does not feel
consistent if you don't the same coding rule to typbasetype,
typcollation or typelem. I am also thinking to split this part of the
cleanup in a first independent patch.

pg_constraint.c, pg_operator.c, extension.c and pg_aggregate.c were
using ObjectAddressSubSet() with subset set to 0 when registering a
dependency. It is simpler to just use ObjectAddressSet(). As this
updates the way dependencies are tracked and recorded, that's better
if kept in the main patch.

+ /* 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?

CatalogTupleInsertWithInfo() becomes mostly unused with this patch,
its only caller being now LOs. Just noticing, I'd rather not remove
it for now.

The attached includes a bunch of modifications I have done while going
through the patch (I indend to split and apply the changes of
pg_type.c separately first, just lacked of time now to send a proper
split), and there is the number of slots for pg_depend insertions that
still needs to be addressed. On top of that pgindent has not been run
yet. That's all I have for today, overall the patch is taking a
committable shape :)

Attachment Content-Type Size
catalog_multi_insert-v11.patch text/x-diff 54.1 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Masahiko Sawada 2020-06-26 08:24:16 Re: PG 13 release notes, first draft
Previous Message Kyotaro Horiguchi 2020-06-26 07:26:16 Re: [patch] demote