Thanks for the review.
On Wed, Dec 7, 2022 at 4:00 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> I find the API of GetCachedPlans a little weird after this patch. I
> think it may be better to have it return a pointer of a new struct --
> one that contains both the CachedPlan pointer and the list of pruning
> results. (As I understand, the sole caller that isn't interested in the
> pruning results, SPI_plan_get_cached_plan, can be explained by the fact
> that it knows there won't be any. So I don't think we need to worry
> about this case?)
David, in his Apr 7 reply on this thread, also sounded to suggest
something similar.
Hmm, I was / am not so sure if GetCachedPlan() should return something
that is not CachedPlan. An idea I had today was to replace the
part_prune_results_list output List parameter with, say,
QueryInitPruningResult, or something like that and put the current
list into that struct. Was looking at QueryEnvironment to come up
with *that* name. Any thoughts?
> And I think you should make that struct also be the last argument of
> PortalDefineQuery, so you don't need the separate
> PortalStorePartitionPruneResults function -- because as far as I can
> tell, the callers that pass a non-NULL pointer there are the exactly
> same that later call PortalStorePartitionPruneResults.
Yes, it would be better to not need PortalStorePartitionPruneResults.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
