On 2019/03/16 6:41, Robert Haas wrote: > On Fri, Mar 15, 2019 at 3:45 PM Tom Lane <t...@sss.pgh.pa.us> wrote: >> I agree that copying data isn't great. What I don't agree is that this >> solution is better. In particular, copying data out of the relcache >> rather than expecting the relcache to hold still over long periods >> is the way we've done things everywhere else, cf RelationGetIndexList, >> RelationGetStatExtList, RelationGetIndexExpressions, >> RelationGetIndexPredicate, RelationGetIndexAttrBitmap, >> RelationGetExclusionInfo, GetRelationPublicationActions. I don't care >> for a patch randomly deciding to do things differently on the basis of an >> unsupported-by-evidence argument that it might cost too much to copy the >> data. If we're going to make a push to reduce the amount of copying of >> that sort that we do, it should be a separately (and carefully) designed >> thing that applies to all the relcache substructures that have the issue, >> not one-off hacks that haven't been reviewed thoroughly. > > That's not an unreasonable argument. On the other hand, if you never > try new stuff, you lose opportunities to explore things that might > turn out to be better and worth adopting more widely. > > I am not very convinced that it makes sense to lump something like > RelationGetIndexAttrBitmap in with something like > RelationGetPartitionDesc. RelationGetIndexAttrBitmap is returning a > single Bitmapset, whereas the data structure RelationGetPartitionDesc > is vastly larger and more complex. You can't say "well, if it's best > to copy 32 bytes of data out of the relcache every time we need it, it > must also be right to copy 10k or 100k of data out of the relcache > every time we need it." > > There is another difference as well: there's a good chance that > somebody is going to want to mutate a Bitmapset, whereas they had > BETTER NOT think that they can mutate the PartitionDesc. So returning > an uncopied Bitmapset is kinda risky in a way that returning an > uncopied PartitionDesc is not. > > If we want an at-least-somewhat unified solution here, I think we need > to bite the bullet and make the planner hold a reference to the > relcache throughout planning. (The executor already keeps it open, I > believe.) Otherwise, how is the relcache supposed to know when it can > throw stuff away and when it can't? The only alternative seems to be > to have each subsystem hold its own reference count, as I did with the > PartitionDirectory stuff, which is not something we'd want to scale > out.
Fwiw, I'd like to vote for planner holding the relcache reference open throughout planning. The planner could then reference the various substructures directly (using a non-copying accessor), except those that something in the planner might want to modify, in which case use the copying accessor. Thanks, Amit
