Message-ID: <55F3F56C.4000604@2ndquadrant.com>
Date: Sat, 12 Sep 2015 11:50:36 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64;
 rv:31.0) Gecko/20100101 Thunderbird/31.7.0
MIME-Version: 1.0
To: pgsql-hackers@postgresql.org
Subject: Re: On-demand running query plans using auto_explain and
 signals
References: 
 <CACACo5Sz7G0MFauC082iM=XX_hQ7qQ5ndR4JPo+H-O5vp6iCcQ@mail.gmail.com>
 <CACACo5R8BN+=q8r+Bz8Bm6072HiiFbbTrWeRyHzJXLnZ8W+3yQ@mail.gmail.com>
 <CACACo5SsSBOwdj_A1+=vA+r8yrtjnKAjyXt2ev-8F=3Ja-n-VA@mail.gmail.com>
 <CAFj8pRBWZbYpT-wRksbxcCUAHZ4+15JJDtq88heaxbXa-sPLiQ@mail.gmail.com>
 <CAFj8pRAbk-dMBYMrDJ3aUROCz4+zgt6gO9=rnWtV32h1jzPaHg@mail.gmail.com>
 <CACACo5QSXC=sQoj6QJ901C_vFjGnhDrN0My58O=LC9A0dYB_aw@mail.gmail.com>
 <CAFj8pRAiQj+un5YL-mGpfd1_owTp_3UP2v0kr08KPm95iiUweg@mail.gmail.com>
 <CACACo5Rmob7aGP0y9zn8ZUcRWuNnV7swk2ugY9pPcEVziP_3yQ@mail.gmail.com>
 <CAFj8pRCv4h3XvDGm0FMfDj9DhM=sDAXLj_RNSd9WZJ91=cAiGA@mail.gmail.com>
 <CACACo5Sqbe3wCa7nXb4v8n2nvksTQymNME+s_ipNKk=iJPPN9Q@mail.gmail.com>
 <CACACo5T8u+=XrDBbtbM4MkXJJ2StxmtpovO5bM3NmrzQiWQsQw@mail.gmail.com>
 <CAFj8pRBg8Zo9E0Cno8HgDtvu+Z-drGTJ8JzUO6mo9C2JSFrJdA@mail.gmail.com>
 <CACACo5S63L4O7dNjFxbFPLGHKZouCzU13Pg=+K3TdZxEbWCmSQ@mail.gmail.com>
 <CACACo5RowbRffGTn=JazPmBkMkmDoCwH7EJgO9F1gCCLPEW3eg@mail.gmail.com>
In-Reply-To: 
 <CACACo5RowbRffGTn=JazPmBkMkmDoCwH7EJgO9F1gCCLPEW3eg@mail.gmail.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

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