Re: Ought to use heap_multi_insert() for pg_attribute/dependinsertions? - Mailing list pgsql-hackers
From | Michael Paquier |
---|---|
Subject | Re: Ought to use heap_multi_insert() for pg_attribute/dependinsertions? |
Date | |
Msg-id | 20200302020600.GA32059@paquier.xyz Whole thread Raw |
In response to | Re: Ought to use heap_multi_insert() for pg_attribute/dependinsertions? (Michael Paquier <michael@paquier.xyz>) |
Responses |
Re: Ought to use heap_multi_insert() for pg_attribute/dependinsertions?
|
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
Attachment
pgsql-hackers by date: