Re: [HACKERS] [PATCH] New command to monitor progression of longrunning queries - Mailing list pgsql-hackers

On 19.04.2017 17:13, Remi Colinet wrote:

Maksim,


2017-04-18 20:31 GMT+02:00 Maksim Milyutin <m.milyutin@postgrespro.ru
<mailto:m.milyutin@postgrespro.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