On Wed, Oct 18, 2023 at 2:09 AM torikoshia <torikos...@oss.nttdata.com> wrote: > > On 2023-10-16 18:46, Ashutosh Bapat wrote: > > On Thu, Oct 12, 2023 at 6:51 PM torikoshia <torikos...@oss.nttdata.com> > > wrote: > >> > >> On 2023-10-11 16:22, Ashutosh Bapat wrote: > >> > > >> > Considering the similarity with auto_explain I wondered whether this > >> > function should be part of auto_explain contrib module itself? If we > >> > do that users will need to load auto_explain extension and thus > >> > install executor hooks when this function doesn't need those. So may > >> > not be such a good idea. I didn't see any discussion on this. > >> > >> I once thought about adding this to auto_explain, but I left it asis > >> for > >> below reasons: > >> > >> - One of the typical use case of pg_log_query_plan() would be > >> analyzing > >> slow query on customer environments. On such environments, We cannot > >> always control what extensions to install. > > > > The same argument applies to auto_explain functionality as well. But > > it's not part of the core. > > Yeah, and when we have a situation where we want to run > pg_log_query_plan(), we can run it in any environment as long as it is > bundled with the core. > On the other hand, if it is built into auto_explain, we need to start by > installing auto_explain if we do not have auto_explain, which is often > difficult to do in production environments. > > >> Of course auto_explain is a major extension and it is quite > >> possible > >> that they installed auto_explain, but but it is also possible they do > >> not. > >> - It seems a bit counter-intuitive that pg_log_query_plan() is in an > >> extension called auto_explain, since it `manually`` logs plans > >> > > > > pg_log_query_plan() may not fit auto_explain but > > pg_explain_backend_query() does. What we are logging is more than just > > plan of the query, it might expand to be closer to explain output. > > While auto in auto_explain would refer to its automatically logging > > explain outputs, it can provide an additional function which provides > > similar functionality by manually triggering it. > > > > But we can defer this to a committer, if you want. > > > > I am more interested in avoiding the duplication of code, esp. the > > first comment in my reply > > If there are no objections, I will try porting it to auto_explain and > see its feasibility. > > >>> There is a lot of similarity between what this feature does and what > >>> auto explain does. I see the code is also duplicated. There is some > >>> merit in avoiding this duplication > >>> 1. we will get all the features of auto_explain automatically like > >>> choosing a format (this was expressed somebody earlier in this > >>> thread), setings etc. > >>> 2. avoid bugs. E.g your code switches context after ExplainState has > >>> been allocated. These states may leak depending upon when this > >>> function gets called. > >>> 3. Building features on top as James envisions will be easier.
In my view the fact that auto_explain is itself not part of core is a mistake, and I know there are more prominent hackers than myself who hold that view. While that decision as regards auto_explain has long since been made (and there would be work to undo it), I don't believe that we should repeat that choice here. I'm -10 on moving this into auto_explain. However perhaps there is still an opportunity for moving some of the auto_explain code into core so as to enable deduplicating the code. Regards, James Coleman
