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

On 09/14/2015 10:23 AM, Shulgin, Oleksandr wrote:

On Sat, Sep 12, 2015 at 11:50 AM, Tomas Vondra
<tomas.vondra@2ndquadrant.com <mailto:tomas.vondra@2ndquadrant.com>> wrote:

...

    - 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.


Yes, I've hit the same problem after submitting the latest version of
the patch.  For now I've just added a check for queryDesc being not
NULL, but I guess the top of the current_query_stack might contains
something useful.  Something I need to try.

Well, the thing is we're able to do EXPLAIN on those queries, and IIRC auto_explain can log them too. So perhaps look into the hooks where they take the queryDesc in those cases - it has to be available somewhere.

    - 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.


I believe the main benefit of the less-locking approach is that if
something goes wrong when two backends tried to communicate it doesn't
prevent the rest of them from operating, because there is no shared (and
thus locked) communication channel.

Sure, but I think it really deserves a bit more detailed explanation of the protocol, and discussion of the expected behavior for some basic failure types.

For example - what happens when a backend requests info, but dies before receiving it, and the backed is reused for another connection? Doesn't this introduce a race condition? Perhaps not, I'm just asking.

    - 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 will add the code comments.  The user docs could wait before we decide
on the interface, I think.

Agreed, although I think having rudimentary user documentation would be useful for the reviewers - a summary of the goals that are a bit scattered over the e-mail thread.

    - 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?


I can think of something like:

EXPLAIN [ ( option [, ...] ) ] PROCESS <PID>;

where option is extended with:

   QUERY
   PROGRESS
   BACKTRACE

in addition to the usual ANALYZE, VERBOSE, FORMAT, etc.

Seems OK. I'm not quite sure what PROGRESS is - I assume it's the same thing as ANALYZE? Why not to use the keyword, then?

    - 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?


If we extend the interface to a more general one, there still might be
some space for querying status of checkpointer of bgwriter.

I don't think we should mix this with monitoring of auxiliary processes. This interface is designed at monitoring SQL queries running in other backends, effectively "remote" EXPLAIN. But those auxiliary processes are not processing SQL queries at all, they're not even using regular executor ...

OTOH the ability to request this info (e.g. auxiliary process looking at plans running in backends) seems useful, so I'm ok with tuple slots for auxiliary processes.

    - 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?


Good point.

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


I think it's being only used for error reporting and could help in
debugging, but for now that's it.

I recommend getting rid of it, unless we have a clear idea of how to use it. Otherwise I envision we won't really keep it updated properly (because it's "just for debugging"), and then one day someone actually starts using it and get bitten by it.

    - 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.


I was able to make this work with a simple change to InstrEndLoop and
the callers.  Basically, adding a bool parameter in_explain and passing
an appropriate value.  I guess that's not the best approach, but it
appears to work.

Adding a GUC to enable instrumentation sounds reasonable.

Nice!

Do you believe it makes sense to add instrumentation support in this
same patch or better focus on making the simplest thing work first?

Let's make it a "patch series" with two patches.

    - 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.


Sure, that was in my plans (and see above for possible syntax).  What
would be really neat is retrieving the complete backtrace.  Not sure
what the good interface would look like, but using JSON format for the
output sounds promising.

I don't quite see the reason to encode everything as JSON, because that makes it a bit difficult for clients that would prefer YAML, for example. Why not to just use the requested format? For example in YAML, we can do something like this:

            QUERY PLAN
----------------------------------
 - Plan[0]:                      +
     Node Type: "Index Only Scan"+
     Scan Direction: "Forward"   +
     ...                         +
                                 +
 - Plan[1]:                      +
     Node Type: "Index Only Scan"+
     Scan Direction: "Forward"   +
     ...                         +
                                 +
 - Plan[2]:                      +
     Node Type: "Index Only Scan"+
     Scan Direction: "Forward"   +
     ...                         +

and similarly for other formats. We don't really use nesting.