Re: hyrax vs. RelationBuildPartitionDesc

On 2019/03/16 6:41, Robert Haas wrote:
> On Fri, Mar 15, 2019 at 3:45 PM Tom Lane <tgl@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