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

Поиск
Список
Период
Сортировка
От Daniel Gustafsson
Тема Re: Ought to use heap_multi_insert() for pg_attribute/dependinsertions?
Дата
Msg-id 6EB631B7-0024-4CD9-9046-B2ACC1A4E7DA@yesql.se
обсуждение исходный текст
Ответ на Re: Ought to use heap_multi_insert() for pg_attribute/dependinsertions?  (Michael Paquier <michael@paquier.xyz>)
Ответы Re: Ought to use heap_multi_insert() for pg_attribute/dependinsertions?  (Michael Paquier <michael@paquier.xyz>)
Список pgsql-hackers
> On 26 Jun 2020, at 10:11, Michael Paquier <michael@paquier.xyz> wrote:
>
> 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.

Thanks for reviewing!

> 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.

Fair enough, I can take that to another patch later in the cycle.

> 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.

Agreed, +1 on the proposed wording.

> 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.

Yeah, thats not good, it's better to leave that out.

> 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.

+1 on splitting into a separate 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().

Fair enough, either way, I don't have strong opinions.

> As this
> updates the way dependencies are tracked and recorded, that's better
> if kept in the main patch.

Agreed.

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

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

Agreed, let's not bite off that too here, there's enough to chew on.

> 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 :)

I like it, thanks for hacking on it.  I will take another look at it later
today when back at my laptop.

cheers ./daniel




В списке pgsql-hackers по дате отправления:

Предыдущее
От: Amit Kapila
Дата:
Сообщение: Re: pgsql: Enable Unix-domain sockets support on Windows
Следующее
От: Daniel Gustafsson
Дата:
Сообщение: Re: should libpq also require TLSv1.2 by default?