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

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.

One comment removal is missed in heapam.c (my fault, then).

+ * TODO this is defined in copy.c, if we want to use this to limit the number
+ * of slots in this patch, we need to figure out where to put it.
+ */
+#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?

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

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.

+/*
+ * InsertPgAttributeTuples
+ * Construct and insert multiple tuples in pg_attribute.
*
- * Caller has already opened and locked pg_attribute. new_attribute is the
- * attribute to insert. attcacheoff is always initialized to -1, attacl and
- * attoptions are always initialized to NULL.
- *
- * indstate is the index state for CatalogTupleInsertWithInfo. It can be
- * passed as NULL, in which case we'll fetch the necessary info. (Don't do
- * this when inserting multiple attributes, because it's a tad more
- * expensive.)
+ * This is a variant of InsertPgAttributeTuple() which dynamically allocates
+ * space for multiple tuples. Having two so similar functions is a kludge, but
+ * for now it's a TODO to make it less terrible.
And those comments largely need to remain around.

- attr = TupleDescAttr(tupdesc, i);
- /* Fill in the correct relation OID */
- attr->attrelid = new_rel_oid;
- /* Make sure this is OK, too */
- attr->attstattarget = -1;
-
- InsertPgAttributeTuple(rel, attr, indstate);
attstattarget handling is inconsistent here, no?
InsertPgAttributeTuples() does not touch this part, though your code
updates the attribute's attrelid.

- referenced.classId = RelationRelationId;
- referenced.objectId = heapRelationId;
- referenced.objectSubId = indexInfo->ii_IndexAttrNumbers[i];
-
- recordDependencyOn(&myself, &referenced, DEPENDENCY_AUTO);
-
+ add_object_address(OCLASS_CLASS, heapRelationId,
+ indexInfo->ii_IndexAttrNumbers[i],
+ refobjs);
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.

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

+ /* TODO is nreferenced a reasonable allocation of slots? */
+ slot = palloc(sizeof(TupleTableSlot *) * nreferenced);
I guess so.

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