Thread: [HACKERS] [PATCH v2] Progress command to monitor progression of long runningSQL queries

On Wed, May 10, 2017 at 06:40:31PM +0200, Remi Colinet wrote:
> Hello,
> 
> This is version 2 of the new command name PROGRESS which I wrote in
> order to monitor long running SQL queries in a Postgres backend
> process.

Perhaps I missed something important in the discussion, but was there
a good reason that this isn't a function callable from SQL, i.e. not
restricted to the psql client?

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Thu, May 11, 2017 at 1:40 AM, Remi Colinet <remi.colinet@gmail.com> wrote:
> This is version 2 of the new command name PROGRESS which I wrote in order to
> monitor long running SQL queries in a Postgres backend process.

It would be a good idea to add this patch to the next commit fest if
you are expecting some feedback:
https://commitfest.postgresql.org/14/
But please don't expect immediate feedback, the primary focus these
days is to stabilize the upcoming release.

> New command justification
> ====================
>
> [root@rco pg]# git diff --stat master..
> [...]
>  58 files changed, 5972 insertions(+), 2619 deletions(-)

For your first patch, you may want something less... Challenging.
Please note as well that it would be nice if you review other patches
to get more familiar with the community process, it is expected that
for each patch submitted, an equivalent amount of review is done.
That's hard to measure but for a patch as large as that you will need
to pick up at least one patch equivalent in difficulty.

Regarding the patch, this is way to close to the progress facility
already in place. So why don't you extend it for the executor? We can
likely come up with something that can help, though I see the part
where the plan run by a backend is shared among all processes as a
major bottleneck in performance. You have at least three concepts
different here:
- Store plans run in shared memory to allow access to any other processes.
- Allow a process to look at the plan run by another one with a SQL interface.
- Track the progress of a running plan, via pg_stat_activity.
All in all, I think that a new command is not adapted, and that you
had better split each concept before implementing something
over-engineered like the patch attached.
-- 
Michael

On Thu, May 11, 2017 at 05:24:16PM +0200, Remi Colinet wrote:
> 2017-05-10 21:52 GMT+02:00 David Fetter <david@fetter.org>:
> 
> > On Wed, May 10, 2017 at 06:40:31PM +0200, Remi Colinet wrote:
> > > Hello,
> > >
> > > This is version 2 of the new command name PROGRESS which I wrote in
> > > order to monitor long running SQL queries in a Postgres backend
> > > process.
> >
> > Perhaps I missed something important in the discussion, but was there
> > a good reason that this isn't a function callable from SQL, i.e. not
> > restricted to the psql client?

Please don't top post. http://www.caliburn.nl/topposting.html

> That's good point.
> 
> I will probably convert the new command to a SQL function.

Great!

> I was fumbling between a table or a SQL function. Oracle uses
> v$session_longops to track progression of long running SQL queries
> which have run for more than 6 seconds.  But a function is much
> better to provide parameters such as the backend pid and a format
> specification for the output.

Once it's a function, it can very easily be called in a system (or
user-defined) view.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter      XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Wed, May 10, 2017 at 10:05 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Regarding the patch, this is way to close to the progress facility
> already in place. So why don't you extend it for the executor?

I don't think that's a good idea.  The existing progress facility
advertises a fixed number of counters with a command-type-specific
interpretation, but for *query* progress reporting, we really need a
richer model.  A query plan can contain an unbounded number of
executor nodes and Remi's idea is, quite reasonably, to report
something about each one.

From a design point of view, I think a patch like this has to clear a
number of hurdles.  It needs to:

1. Decide what data to expose.  The sample output looks reasonable in
this case, although the amount of code churn looks really high.

2. Find a way to advertise the data that it exposes.  This patch looks
like it allocates a half-MB of shared memory per backend and gives up
if that's not enough.

3. Find a way to synchronize the access to the data that it exposes.
This patch ignores that problem completely, so multiple progress
report commands running at the same time will behave unpredictably.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, May 10, 2017 at 10:10 PM, Remi Colinet <remi.colinet@gmail.com> wrote:
>
> Parallel queries can also be monitored. The same mecanism is used to monitor
> child workers with a slight difference: the main worker requests the child
> progression directly in order to dump the whole progress tree in shared
> memory.
>

What if there is any error in the worker (like "out of memory") while
gathering the statistics?  It seems both for workers as well as for
the main backend it will just error out.  I am not sure if it is a
good idea to error out the backend or parallel worker as it will just
end the query execution.  Also, even if it is okay, there doesn't seem
to be a way by which a parallel worker can communicate the error back
to master backend, rather it will just exit silently which is not
right.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sat, May 13, 2017 at 10:53 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, May 10, 2017 at 10:05 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> Regarding the patch, this is way to close to the progress facility
>> already in place. So why don't you extend it for the executor?
>
> I don't think that's a good idea.  The existing progress facility
> advertises a fixed number of counters with a command-type-specific
> interpretation, but for *query* progress reporting, we really need a
> richer model.  A query plan can contain an unbounded number of
> executor nodes and Remi's idea is, quite reasonably, to report
> something about each one.

I see what you're coming at. As st_progress_param is upper-bounded,
what should be actually used as report structure is a tree made of
nodes with one planner node attached to each node of the report tree.
Then the reporting facility should need to make persistent the data
reported.which is in this case a set of planner nodes, being able to also make
persistent the changes on the tree reported to the user. Then when a
backend reports its execution status, what it does is just updating a
portion of the tree in shared memory. Lock contention may be a problem
though, perhaps things could be made atomic?

> From a design point of view, I think a patch like this has to clear a
> number of hurdles.  It needs to:
>
> 1. Decide what data to expose.  The sample output looks reasonable in
> this case, although the amount of code churn looks really high.
>
> 2. Find a way to advertise the data that it exposes.  This patch looks
> like it allocates a half-MB of shared memory per backend and gives up
> if that's not enough.

Perhaps DSM? It is not user-friendly to fail sporadically...

> 3. Find a way to synchronize the access to the data that it exposes.
> This patch ignores that problem completely, so multiple progress
> report commands running at the same time will behave unpredictably.

Yeah..
-- 
Michael

On Tue, May 16, 2017 at 2:17 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Perhaps DSM? It is not user-friendly to fail sporadically...

Yeah.  I've been thinking we might want to give each backend a
backend-lifetime DSA that is created on first use.  That could be
useful for some parallel query stuff and maybe for this as well.
Other backends could attach to it to read data from it, and it would
go away on last detach (which would normally be the detach by the
creating backend, but not if somebody else happens to be reading at
the moment the backend exits).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, May 17, 2017 at 9:43 PM, Remi Colinet <remi.colinet@gmail.com> wrote:
>
> 2017-05-13 14:38 GMT+02:00 Amit Kapila <amit.kapila16@gmail.com>:
>>
>> On Wed, May 10, 2017 at 10:10 PM, Remi Colinet <remi.colinet@gmail.com>
>> wrote:
>> >
>> > Parallel queries can also be monitored. The same mecanism is used to
>> > monitor
>> > child workers with a slight difference: the main worker requests the
>> > child
>> > progression directly in order to dump the whole progress tree in shared
>> > memory.
>> >
>>
>> What if there is any error in the worker (like "out of memory") while
>> gathering the statistics?  It seems both for workers as well as for
>> the main backend it will just error out.  I am not sure if it is a
>> good idea to error out the backend or parallel worker as it will just
>> end the query execution.
>
>
> The handling of progress report starts by the creation of a MemoryContext
> attached to CurrentMemoryContext. Then, few memory (few KB) is allocated.
> Meanwhile, the handling of progress report could indeed exhaust memory and
> fail the backend request. But, in such situation, the backend could also
> have fail even without any progress request.
>
>>
>> Also, even if it is okay, there doesn't seem
>> to be a way by which a parallel worker can communicate the error back
>> to master backend, rather it will just exit silently which is not
>> right.
>
>
> If a child worker fails, it will not respond to the main backend request.
> The main backend will follow up it execution after a 5 seconds timeout (GUC
> param to be added may be). In which case, the report would be partially
> filled.
>
> If the main backend fails, the requesting backend will have a response such
> as:
>
> test=# PROGRESS 14611;
>  PLAN PROGRESS
> ----------------
>  <backend timeout>
> (1 row)
>
> test=#
>
> and the child workers will log their response to the shared memory. This
> response will not be collected by the main backend which has failed.
>

If the worker errors out due to any reason, we should end the main
query as well, otherwise, it can produce wrong results.  See the error
handling of workers in HandleParallelMessage


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com