Re: [PATCH] New command to monitor progression of long running queries

On 19.04.2017 17:13, Remi Colinet wrote:
> Maksim,
>
>
> 2017-04-18 20:31 GMT+02:00 Maksim Milyutin <m(dot)milyutin(at)postgrespro(dot)ru
> <mailto:m(dot)milyutin(at)postgrespro(dot)ru>>:
>
> On 18.04.2017 17:39, Remi Colinet wrote:
>
>
> Regarding the queryDesc state of SQL query upon receiving a
> request to
> report its execution progress, it does not bring any issue. The
> request
> is noted when the signal is received by the monitored backend.
> Then, the
> backend continues its execution code path. When interrupts are
> checked
> in the executor code, the request will be dealt.
>
>
> Yes, interrupts are checked in the CHECK_FOR_INTERRUPTS entries.
>
> When the request is being dealt, the monitored backend will stop its
> execution and report the progress of the SQL query. Whatever is the
> status of the SQL query, progress.c code checks the status and
> report
> either that the SQL query does not have a valid status, or
> otherwise the
> current execution state of the SQL query.
>
> SQL query status checking is about:
> - idle transaction
> - out of transaction status
> - null planned statement
> - utility command
> - self monitoring
>
> Other tests can be added if needed to exclude some SQL query
> state. Such
> checking is done in void HandleProgressRequest(void).
> I do not see why a SQL query progression would not be possible
> in this
> context. Even when the queryDescc is NULL, we can just report a
> <idle
> transaction> output. This is currently the case with the patch
> suggested.
>
>
> It's interesting question - how much the active query is in a usable
> state on the stage of execution. Tom Lane noticed that
> CHECK_FOR_INTERRUPTS doesn't give us 100% guarantee about full
> consistency [1].
>
>
> I wonder what you mean about usable state.
>

A usable query state is suitable for analysis, IOW we have consistent
QueryDesc object. This term was introduced by Tom Lane in [1]. I suppose
he meant the case when a query fails with error and before transaction
aborts we bump into *CHECK_FOR_INTERRUPTS* in the place where QueryDesc
may be inconsistent and further reading from it will give us invalid result.

> Currently, the code suggested tests the queryDesc pointer and all the
> sub nodes pointers in order to detect NULL pointers. When the progress
> report is collected by the backend, this backend does the collect and
> consequently does not run the query. So the query tree is not being
> modified. At this moment, whatever is the query state, we can manage to
> deal with its static state. It is only a tree which could also be just a
> NULL pointer in the most extreme case. Such case is dealt in the current
> code.
>

Perhaps the deep checking of QueryDesc would allow us to consider it as
consistent.

1. https://www.postgresql.org/message-id/24182.1472745492%40sss.pgh.pa.us

--
Maksim Milyutin
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company