> > I know Michael opposed the idea of carrying these structures, > > at least CachedPlan, to Executor hooks ( or maybe just not QueryDesc?? ). > > It will be good to see what he think, or if others an opinion about this, > > about > > adding a pointer to CachedPlanSource in PlannedStmt vs setting a flag in > > PlannedStmt to track plan cache type for the current execution? The > former > > does provide more capability for extensions, as Andrei has pointed out > > earlier. > > My line is pretty simple here: we should never include in > executor-specific or plan-specific areas structures that are handled > by entirely different memory contexts or portions of the code, all > parts (executor, query description and or [cached] plan) relying on > entirely different assumptions and contexts, because I suspect that it > would bite us back hard in the shape of corrupted stacks depending on > the timing where these resources are freed. One of the upthread > proposals where a reference of the cached plan pointer was added to an > executor state structure crossed this line. > > Simple fields are no-brainers, they are just carried in the same > context as the caller, independently of other states. When it comes > to stats and monitoring, we don't have to be exact, we can sometimes > even be a bit lossy in the reports. > > A simple "plan type" field in a PlannedStmt should be more than OK, as > it would be carried across the contexts where we want to know about it > and handle it, aka PGSS entry storing. >
Thanks! You make valid point about memory contexts and opening ourselves to bugs/crashes. [0] implements a single field “plan type” in PlannedStmt. [0] https://www.postgresql.org/message-id/CAA5RZ0tA43qf1HF1g6oYUGNBGFr23F29pf4F9Hymp2z8PyTsPg%40mail.gmail.com -- Sami
