Re: Ought to use heap_multi_insert() for pg_attribute/dependinsertions? - Mailing list pgsql-hackers

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