On Mon, Aug 21, 2017 at 2:10 AM, Amit Langote <langote_amit...@lab.ntt.co.jp> wrote: > [ new patches ]
I am failing to understand the point of separating PartitionDispatch into PartitionDispatch and PartitionTableInfo. That seems like an unnecessary multiplication of entities, as does the introduction of PartitionKeyInfo. I also think that replacing reldesc with reloid is not really an improvement; any places that gets the relid has to go open the relation to get the reldesc, whereas without that it has a direct pointer to the information it needs. I suggest that this patch just focus on removing the following things from PartitionDispatchData: keystate, tupslot, tupmap. Those things are clearly executor-specific stuff that makes sense to move to a different structure, what you're calling PartitionTupleRoutingInfo (not sure that's the best name). The other stuff all seems fine. You're going to have to open the relation anyway, so keeping the reldesc around seems like an optimization, if anything. The PartitionKey and PartitionDesc pointers may not really be needed -- they're just pointers into reldesc -- but they're trivial to compute, so it doesn't hurt anything to have them either as a micro-optimization for performance or even just for readability. That just leaves indexes. In a world where keystate, tupslot, and tupmap are removed from the PartitionDispatchData, you must need indexes or there would be no point in constructing a PartitionDispatchData object in the first place; any application that needs neither indexes nor the executor-specific stuff could just use the Relation directly. Regarding your XXX comments, note that if you've got a lock on a relation, the pointers to the PartitionKey and PartitionDesc are stable. The PartitionKey can't change once it's established, and the PartitionDesc can't change while we've got a lock on the relation unless we change it ourselves (and any places that do should have CheckTableNotInUse checks). The keep_partkey and keep_partdesc handling in relcache.c exists exactly so that we can guarantee that the pointer won't go stale under us. Now, if we *don't* have a lock on the relation, then those pointers can easily be invalidated -- so you can't hang onto a PartitionDispatch for longer than you hang onto the lock on the Relation. But that shouldn't be a problem. I think you only need to hang onto PartitionDispatch pointers for the lifetime of a single query. One can imagine optimizations where we try to avoid rebuilding that for subsequent queries but I'm not sure there's any demonstrated need for such a system at present. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
