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
signature.asc
Description: PGP signature
