On Thu, Apr 12, 2018 at 12:40 AM, David Rowley
<david.rowley@2ndquadrant.com> wrote:
> I guess the problem there would be there's nothing to say that parse
> analysis will shortly be followed by a call to the planner, and a call
> to the planner does not mean the plan is about to be executed. So I
> don't think it would be possible to keep pointers to relcache entries
> between these modules, and it would be hard to determine whose
> responsibility it would be to call relation_close().
Yeah, that's definitely a non-starter.
> It might be possible to do something better in each module by keeping
> an array indexed by RTI which have each entry NULL initially then on
> first relation_open set the element in the array to that pointer.
I'm not sure that makes a lot of sense in the planner, but in the
executor it might be a good idea. See also
https://www.postgresql.org/message-id/CA%2BTgmoYKToP4-adCFFRNrO21OGuH%3Dphx-fiB1dYoqksNYX6YHQ%40mail.gmail.com
for related discussion. I think that a coding pattern where we rely
on relation_open(..., NoLock) is inherently dangerous -- it's too easy
to be wrong about whether the lock is sure to have been taken. It
would be much better to open the relation once and hold onto the
pointer, not just for performance reasons, but for robustness.
BTW, looking at ExecSetupPartitionPruneState:
/*
* Create a sub memory context which we'll use when making calls to the
* query planner's function to determine which partitions will
match. The
* planner is not too careful about freeing memory, so we'll ensure we
* call the function in this context to avoid any memory leaking in the
* executor's memory context.
*/
This is a sloppy cut-and-paste job, not only because somebody changed
one copy of the word "planner" to "executor" and left the others
untouched, but also because the rationale isn't really correct for the
executor anyway, which has memory contexts all over the place and
frees them all the time. I don't know whether the context is not
needed at all or whether the context is needed but the rationale is
different, but I don't buy that explanation.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
