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

> On 2 Mar 2020, at 03:06, Michael Paquier <michael@paquier.xyz> wrote:

Thanks a lot for another round of review, much appreciated!

> On Fri, Feb 28, 2020 at 05:24:29PM +0900, Michael Paquier wrote:
>> +   /*
>> +    * CONTAINS_NEW_TUPLE will always be set unless the multi_insert was
>> +    * performed for a catalog.  If it is a catalog, return immediately as
>> +    * there is nothing to logically decode.
>> +    */
>> +   if (!(xlrec->flags & XLH_INSERT_CONTAINS_NEW_TUPLE))
>> +       return;
>> Hmm, OK.  Consistency with DecodeInsert() is a good idea here, so
>> count me in regarding the way your patch handles the problem.  I would
>> be fine to apply that part but, Andres, perhaps you would prefer
>> taking care of it yourself?
>
> Okay, I have applied this part.

Thanks!

> +#define MAX_BUFFERED_BYTES 65535
> The use-case is different than copy.c, so aren't you looking at a
> separate variable to handle that?

Renamed to indicate current usecase.

> +reset_object_addresses(ObjectAddresses *addrs)
> +{
> +   if (addrs->numrefs == 0)
> +       return;
> Or just use an assert?

I don't see why we should prohibit calling reset_object_addresses so strongly,
it's nonsensical but is it wrong enough to Assert on?  I can change it if you
feel it should be an assertion, but have left it for now.

> src/backend/commands/tablecmds.c:   /* attribute.attacl is handled by
> InsertPgAttributeTuple */
> src/backend/catalog/heap.c: * This is a variant of
> InsertPgAttributeTuple() which dynamically allocates
> Those two references are not true anymore as your patch removes
> InsertPgAttributeTuple() and replaces it by the plural flavor.

Fixed.

> +/*
> + * InsertPgAttributeTuples
> ...
> And those comments largely need to remain around.

Fixed.

> attstattarget handling is inconsistent here, no?

Indeed it was, fixed.

> Not convinced that we have to publish add_object_address() in the
> headers, because we have already add_exact_object_address which is
> able to do the same job.  All code paths touched by your patch involve
> already ObjectAddress objects, so switching to _exact_ creates less
> code diffs.

Good point, using _exact will make the changes smaller at the cost of additions
being larger.  I do prefer the add_object_address API since it makes code more
readable IMO, but there is clear value in not exposing more functions.  I've
changed to using add_exact_object_address in the attached version.

> +       if (ntuples == DEPEND_TUPLE_BUF)
> +       {
> +           CatalogTuplesMultiInsertWithInfo(sdepRel, slot, ntuples, indstate);
> +           ntuples = 0;
> +       }
> Maybe this is a sufficient argument that we had better consider the
> template copy as part of a separate patch, handled once the basics is
> in place.  It is not really obvious if 32 is a good thing, or not.

Fixed by instead avoiding the copying altogether and creating virtual tuples.
Andres commented on this upthread but I had missed fixing the code to do that,
so better late than never.

>    /* construct new attribute's pg_attribute entry */
> +   oldcontext = MemoryContextSwitchTo(CurTransactionContext);
> This needs a comment as this change is not obvious for the reader.
> And actually, why?

Thats a good question, this was a leftover from a different version the code
which I abandoned, but I missed removing the context handling.  Sorry about the
sloppyness there. Removed.

The v9 patch attached addresses, I believe, all comments made to date.  I tried
to read through earlier reviews too to ensure I hadn't missed anything so I
hope I've covered all findings so far.

cheers ./daniel