On Mon, Apr 11, 2016 at 9:45 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Mon, Apr 11, 2016 at 5:16 PM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp> wrote:
>> On 2016/04/11 12:30, Michael Paquier wrote:
>>>
>>> + if ((cancel = PQgetCancel(entry->conn)))
>>> + {
>>> + PQcancel(cancel, errbuf, sizeof(errbuf));
>>> + PQfreeCancel(cancel);
>>> + }
>>> Wouldn't it be better to issue a WARNING here if PQcancel does not
>>> succeed?
>>
>> Seems like a good idea. Attached is an updated version of the patch.
>
> Thanks for the new version. The patch looks good to me.
I'm wondering why we are fixing this specific case and not any of the
other calls to PQexec() or PQexecParams() in postgres_fdw.c.
I mean, many of those instances are cases where the query isn't likely
to run for very long, but certainly "FETCH %d FROM c%u" is in theory
just as bad as the new code introduced in 9.6. In practice, it
probably isn't, because we're probably only fetching 50 rows at a time
rather than potentially a lot more, but if we're fixing this code up
to be interrupt-safe, maybe we should fix it all at the same time.
Even for the short-running queries like CLOSE and DEALLOCATE, it seems
possible that there could be a network-related hang which you might
want to interrupt.
How about we encapsulate the while (PQisBusy(...)) loop into a new
function pgfdw_get_result(), which can be called after first calling
PQsendQueryParams()? So then this code will say dmstate->result =
pgfdw_get_result(dmstate->conn). And we can do something similar for
the other call to PQexecParams() in create_cursor(). Then let's also
add something like pgfdw_exec_query() which calls PQsendQuery() and
then pgfdw_get_result, and use that to replace all of the existing
calls to PQexec().
Then all the SQL postgres_fdw executes would be interruptible, not
just the new stuff.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company