Re: On-demand running query plans using auto_explain and signals - Mailing list pgsql-hackers

Hi,

I did a quick initial review of this patch today, so here are my 
comments so far:

- ipcs.c should include utils/cmdstatus.h (the compiler complains  about implicit declaration of two functions)

- Attempts to get plan for simple insert queries like this
      INSERT INTO x SELECT * FROM x;
  end with a segfault, because ActivePortal->queryDesc is 0x0 for this  query. Needs investigation.

- The lockless approach seems fine to me, although I think the fear  of performance issues is a bit moot (I don't think
weexpect large  number of processes calling pg_cmdstatus at the same time). But  it's not significantly more complex,
sowhy not.
 

- The patch contains pretty much no documentation, both comments  at the code level and user docs. The lack of user
docsis not that  a big deal at this point (although the patch seems to be mature  enough, although the user-level API
willlikely change).
 
  The lack of code comments is more serious, as it makes the review  somewhat more difficult. For example it'd be very
niceto document  the contract for the lock-less interface.
 

- I agree that pg_cmdstatus() is not the best API. Having something  like EXPLAIN PID would be nice, but it does not
reallywork for  all the request types (just CMD_STATUS_REQUEST_EXPLAIN). Maybe  there's not a single API for all cases,
i.e.we should use EXPLAIN  PID for one case and invent something different for the other?
 

- Is there a particular reason why we allocate slots for auxiliary  processes and not just for backends (NumBackends)?
Dowe expect those  auxiliary processes to ever use this API?
 

- CleanupCmdStatusSlot seems needlessly complicated. I don't quite see  the need for the second argument, or the
internalslot variable. Why  not to simply use the MyCmdStatusSlot directly?
 

- I also don't quite understand why we need to track css_pid for the  slot? In what scenario will this actually
matter?

- While being able to get EXPLAIN from the running process is nice,  I'm especially interested in getting EXPLAIN
ANALYZEto get insight  into the progress of the execution. The are other ways to get the  EXPLAIN, e.g. by opening a
differentconnection and actually running  it (sure, the plan might have changed since then), but currently  there's no
wayto get insight into the progress.
 
  From the thread I get the impression that Oleksandr also finds this  useful - correct? What are the plans in this
direction?
  ISTM we need at least two things for that to work:
  (a) Ability to enable instrumentation on all queries (effectively      what auto_explain allows), otherwise we can't
getEXPLAIN ANALYZE      on the queries later. But auto_explain is an extension, so that      does not seem as a good
matchif this is supposed to be in core.      In that case a separate GUC seems appropriate.
 
  (b) Being able to run the InstrEnd* methods repeatedly - the initial      message in this thread mentions issues with
InstrEndLoopfor      example. So perhaps this is non-trivial.
 

- And finally, I think we should really support all existing EXPLAIN  formats, not just text. We need to support the
otherformats (yaml,  json, xml) if we want to use the EXPLAIN PID approach, and it also  makes the plans easier to
processby additional tools.
 


regards

-- 
Tomas Vondra                   http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services