On 2017/08/26 3:28, Robert Haas wrote:
> 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 am worried about the open relcache reference in PartitionDispatch when
we start using it in the planner. Whereas there is a ExecEndModifyTable()
as a suitable place to close that reference, there doesn't seem to exist
one within the planner, but I guess we will have to figure something out.
For time being, the second patch closes the same in
expand_inherited_rtentry() right after picking up the OID using
RelationGetRelid(pd->reldesc).
> 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.
OK, done this way in the attached updated patch. Any suggestions about a
better name for what the patch calls PartitionTupleRoutingInfo?
> 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.
Agreed.
> 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.
Here too.
Attached are the updated patches.
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers