Re: Switch to multi-inserts for pg_depend

Hi,

On 2020-08-07 15:16:19 +0900, Michael Paquier wrote:
> From cd117fa88938c89ac953a5e3c036828337150b07 Mon Sep 17 00:00:00 2001
> From: Michael Paquier <michael(at)paquier(dot)xyz>
> Date: Fri, 7 Aug 2020 10:57:40 +0900
> Subject: [PATCH 1/2] Use multi-inserts for pg_depend
>
> This is a follow-up of the work done in e3931d01. This case is a bit
> different than pg_attribute and pg_shdepend: the maximum number of items
> to insert is known in advance, but there is no need to handle pinned
> dependencies. Hence, the base allocation for slots is done based on the
> number of items and the maximum allowed with a cap at 64kB, and items
> are initialized once used to minimize the overhead of the operation.
>
> Author: Daniel Gustafsson, Michael Paquier
> Discussion: https://postgr.es/m/XXX
> ---
> src/backend/catalog/pg_depend.c | 95 ++++++++++++++++++++++++---------
> 1 file changed, 69 insertions(+), 26 deletions(-)
>
> diff --git a/src/backend/catalog/pg_depend.c b/src/backend/catalog/pg_depend.c
> index 70baf03178..596f0c5e29 100644
> --- a/src/backend/catalog/pg_depend.c
> +++ b/src/backend/catalog/pg_depend.c
> @@ -47,6 +47,12 @@ recordDependencyOn(const ObjectAddress *depender,
> recordMultipleDependencies(depender, referenced, 1, behavior);
> }
>
> +/*
> + * Cap the maximum amount of bytes allocated for recordMultipleDependencies()
> + * slots.
> + */
> +#define MAX_PGDEPEND_INSERT_BYTES 65535
> +

Do we really want to end up with several separate defines for different
type of catalog batch inserts? That doesn't seem like a good
thing. Think there should be a single define for all catalog bulk
inserts.

> /*
> * Record multiple dependencies (of the same kind) for a single dependent
> * object. This has a little less overhead than recording each separately.
> @@ -59,10 +65,10 @@ recordMultipleDependencies(const ObjectAddress *depender,
> {
> Relation dependDesc;
> CatalogIndexState indstate;
> - HeapTuple tup;
> - int i;
> - bool nulls[Natts_pg_depend];
> - Datum values[Natts_pg_depend];
> + int slotCount, i;
> + TupleTableSlot **slot;
> + int nslots, max_slots;
> + bool slot_init = true;
>
> if (nreferenced <= 0)
> return; /* nothing to do */
> @@ -76,11 +82,18 @@ recordMultipleDependencies(const ObjectAddress *depender,
>
> dependDesc = table_open(DependRelationId, RowExclusiveLock);
>
> + /*
> + * Allocate the slots to use, but delay initialization until we know that
> + * they will be used.
> + */
> + max_slots = Min(nreferenced,
> + MAX_PGDEPEND_INSERT_BYTES / sizeof(FormData_pg_depend));
> + slot = palloc(sizeof(TupleTableSlot *) * max_slots);
> +
> /* Don't open indexes unless we need to make an update */
> indstate = NULL;
>
> - memset(nulls, false, sizeof(nulls));
> -
> + slotCount = 0;
> for (i = 0; i < nreferenced; i++, referenced++)
> {
> /*
> @@ -88,38 +101,68 @@ recordMultipleDependencies(const ObjectAddress *depender,
> * need to record dependencies on it. This saves lots of space in
> * pg_depend, so it's worth the time taken to check.
> */
> - if (!isObjectPinned(referenced, dependDesc))
> + if (isObjectPinned(referenced, dependDesc))
> + continue;
> +

Hm, would it be better to first iterate over the dependencies, compute
the number of dependencies to be inserted, and then go ahead and create
the right number of slots?

> From fcc0a11e9fc94d2fedc71dd10ba2a23713225963 Mon Sep 17 00:00:00 2001
> From: Michael Paquier <michael(at)paquier(dot)xyz>
> Date: Fri, 7 Aug 2020 15:14:51 +0900
> Subject: [PATCH 2/2] Switch to multi-insert dependencies for many code paths
>
> This makes use of the new APIs to insert dependencies in groups, instead
> of doing the operation one-by-one.

Seems several places have been modified to new APIs despite only
covering a single dependency. Perhaps worth mentioning?

Greetings,

Andres Freund