On Mon, Aug 21, 2017 at 2:10 AM, Amit Langote
<Langote_Amit_f8@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
