>>> 0004: >>> I think this needs to introduce a field at the top level of >>> RelationData that indicates the variant of Relation that we're dealing >>> with. Continuously dereferencing rel->rd_rel->relkind seems rather >>> expensive compared to "just" keeping track of another boolean (or >>> enum, char, or whatever type) at the front of the struct that >>> distinguishes the exact type of data contained, if we can afford to. >>> >>> (It'll also make it easier to verify we're dealing with the right type >>> of Relation, as we won't have to Assert() two layers of indirection >>> deep) >> >> I would prefer to not do that. I like that now we have a single source >> of truth and adding such a field would be a bunch of code churn. > > What specifically would need a bunch of code churn? The paths we're > dealing with shouldn't need much (if any) changes in that regard. One > reason to do this is to make it simpler to add another variant to the > union, if/when we figure out how to split the larger of [table-like, > index-like] relations further. > >> Performance-wise the extra indirection doesn't matter. It's not accessed >> anywhere in way that it's expensive relative to all the other stuff >> happening. The pgbench I ran support that claim. > > Sure, but for debugging and for interpretability I do think that it > makes more sense to have an indicator which of the two variants > should/can be accessed that is stored in the Relation itself. It'll > also simplify the condition (from two OR-ed options to just one) and > will make adding future variants (if any) simpler by allowing using > switch() blocks when we need to handle generic Relations.
Took a stab at it in a new top commit so we can discuss before integrating it throughout all existing commits. >>> I think this'll be better if it splits out the Table and Index parts> from >>> RelationBuildDesc. >> >> I'm not sure what you're exactly after here. Can you share some more >> details please? > > I meant that the code of RelationBuildDesc would be easier to read and > understand if we split the code that fills in the shared fields from > the code that fills in the index variant of the union and from the > code that fills in the non-index parts of the union, instead of mixing > those. That doesn't need to be in a different function per se, but > could also just be a better vertical grouping with a singular if/else > block. Done. Now, first comes all shared initialization code, then if (RelationIsIndex()) with the two legs for index and non-index specific RelationData members. I'll send out a new patch set once I've addressed the review comments in your latest mail. -- David Geier
