> On 26 Jun 2020, at 10:11, Michael Paquier <mich...@paquier.xyz> wrote: > > On Thu, Jun 25, 2020 at 09:38:23AM +0200, Daniel Gustafsson wrote: >> Attached is a rebased version which was updated to handle the changes for op >> class parameters introduced in 911e70207703799605. > > Thanks for the updated version.
Thanks for reviewing! > While re-reading the code, I got cold feet with the changes done in > recordDependencyOn(). Sure, we could do as you propose, but that does > not have to be part of this patch I think, aimed at switching more > catalogs to use multi inserts, and it just feels a duplicate of > recordMultipleDependencies(), with the same comments copied all over > the place, etc. Fair enough, I can take that to another patch later in the cycle. > MAX_TEMPLATE_BYTES in pg_shdepend.c needs a comment to explain that > this is to cap the number of slots used in > copyTemplateDependencies() for pg_shdepend. Agreed, +1 on the proposed wording. > Not much a fan of the changes in GenerateTypeDependencies(), > particularly the use of refobjs[8], capped to the number of items from > typeForm. If we add new members I think that this would break > easily without us actually noticing that it broke. Yeah, thats not good, it's better to leave that out. > The use of > ObjectAddressSet() is a good idea though, but it does not feel > consistent if you don't the same coding rule to typbasetype, > typcollation or typelem. I am also thinking to split this part of the > cleanup in a first independent patch. +1 on splitting into a separate patch. > pg_constraint.c, pg_operator.c, extension.c and pg_aggregate.c were > using ObjectAddressSubSet() with subset set to 0 when registering a > dependency. It is simpler to just use ObjectAddressSet(). Fair enough, either way, I don't have strong opinions. > As this > updates the way dependencies are tracked and recorded, that's better > if kept in the main patch. Agreed. > + /* TODO is nreferenced a reasonable allocation of slots? */ > + slot = palloc(sizeof(TupleTableSlot *) * nreferenced); > It seems to me that we could just apply the same rule as for > pg_attribute and pg_shdepend, no? I think so, I see no reason not to. > CatalogTupleInsertWithInfo() becomes mostly unused with this patch, > its only caller being now LOs. Just noticing, I'd rather not remove > it for now. Agreed, let's not bite off that too here, there's enough to chew on. > The attached includes a bunch of modifications I have done while going > through the patch (I indend to split and apply the changes of > pg_type.c separately first, just lacked of time now to send a proper > split), and there is the number of slots for pg_depend insertions that > still needs to be addressed. On top of that pgindent has not been run > yet. That's all I have for today, overall the patch is taking a > committable shape :) I like it, thanks for hacking on it. I will take another look at it later today when back at my laptop. cheers ./daniel
