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 we expect large
  number of processes calling pg_cmdstatus at the same time). But
  it's not significantly more complex, so why not.

- The patch contains pretty much no documentation, both comments
  at the code level and user docs. The lack of user docs is not that
  a big deal at this point (although the patch seems to be mature
  enough, although the user-level API will likely change).

  The lack of code comments is more serious, as it makes the review
  somewhat more difficult. For example it'd be very nice to 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 really work 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)? Do we 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 internal slot 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 ANALYZE to get insight
  into the progress of the execution. The are other ways to get the
  EXPLAIN, e.g. by opening a different connection and actually running
  it (sure, the plan might have changed since then), but currently
  there's no way to 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 get EXPLAIN ANALYZE
      on the queries later. But auto_explain is an extension, so that
      does not seem as a good match if 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 InstrEndLoop for
      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 other formats (yaml,
  json, xml) if we want to use the EXPLAIN PID approach, and it also
  makes the plans easier to process by additional tools.

regards

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers