New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add user interrupt check while waiting for query results to be ready. #193

Merged
merged 6 commits into from Oct 4, 2018

Conversation

Projects
None yet
2 participants
@zozlak
Copy link
Contributor

zozlak commented Aug 25, 2018

Sometimes user wants to terminate a query before a first row of results is available (e.g. it turned out that the query is too complicated and he wants to rewrite it instead of waiting for its completion). So far user interrupts where not checked until PQgetResult() completion so the only option to achieve that was to terminate whole R session or to terminate the query on the database server.

This pull requests adds user interrupts check between PQsendQueryPrepared() and PQgetResult().

It is done by waiting for data on the database connection socket with a timeout (1 second) using glibc's select() and running checkUserInterrupt() if the timeout is hit.

Add user interrupt check while waiting for query results to be ready.…
… Allows safe interruption of long-running queries.
@krlmlr
Copy link
Member

krlmlr left a comment

Thanks! The new code contains some libpq function calls I'm not familiar with, could we make this optional for now?

DESCRIPTION Outdated
Authors@R: c(
person("Hadley", "Wickham", role = "aut"),
person("Jeroen", "Ooms", role = "aut"),
person("Kirill", "M\u00fcller", role = c("aut", "cre"), email = "krlmlr+r@mailbox.org", comment = c(ORCID = "0000-0002-1416-3412")),
person("Mateusz", "\u017b\u00f3\u0142tak", role = c("ctb")),

This comment has been minimized.

@krlmlr

krlmlr Aug 26, 2018

Member

In this project, we acknowledge in the NEWS.md file. I'll add an acknowledgment for all contributors to r-dbi repos to the upcoming DBI blog post too.

This comment has been minimized.

@zozlak

zozlak Aug 27, 2018

Contributor

Moved to NEWS.md in d6dcba9

Show resolved Hide resolved src/PqResultImpl.cpp
@zozlak

This comment has been minimized.

Copy link
Contributor

zozlak commented Aug 27, 2018

The new code contains some libpq function calls I'm not familiar with, could we make this optional for now?

Of course we can. How would you prefer it to be done?

By the way I added a link to the PG documentation in the code and an example of using PQconsumeInput() can be found at https://www.postgresql.org/docs/current/static/libpq-example.html (they don't use PQisBusy() in the example because they are waiting for notifications and not data but the idea is the same)

@krlmlr

This comment has been minimized.

Copy link
Member

krlmlr commented Aug 28, 2018

Thanks. I think an argument to dbConnect() would be a good way.

Can you please also resolve the conflicts? I can take care of the NEWS entry.

@zozlak

This comment has been minimized.

Copy link
Contributor

zozlak commented Aug 28, 2018

Conflicts removed, adding dbConnect() parameter requires a little more time :)

@zozlak

This comment has been minimized.

Copy link
Contributor

zozlak commented Aug 30, 2018

I've been quite busy last few days, I'll provide the dbConnect() switch during the weekend.

@zozlak

This comment has been minimized.

Copy link
Contributor

zozlak commented Sep 1, 2018

Any idea how to name the parameter?

DbConnect() extended with a `checkInterrupts` parameter controlling i…
…f user interrupts should be checked while waiting for the first row of data to be available.
@zozlak

This comment has been minimized.

Copy link
Contributor

zozlak commented Sep 1, 2018

I added it as checkInterrupts but I'm not attached to the name :)

@krlmlr
Copy link
Member

krlmlr left a comment

Thanks for your PR. I'll tweak myself.

Show resolved Hide resolved src/PqResultImpl.cpp
Show resolved Hide resolved R/PqConnection.R
Show resolved Hide resolved NEWS.md
Show resolved Hide resolved src/PqResultImpl.cpp
Show resolved Hide resolved src/PqResultImpl.cpp
Show resolved Hide resolved src/PqResultImpl.cpp

@krlmlr krlmlr merged commit 06dde3a into r-dbi:master Oct 4, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
deploy/netlify Deploy preview ready!
Details
@krlmlr

This comment has been minimized.

Copy link
Member

krlmlr commented Oct 4, 2018

Could you please double-check if the code still works as intended after my tweaks? In master now.

@zozlak

This comment has been minimized.

Copy link
Contributor

zozlak commented Oct 4, 2018

Seems to work fine :)

Just a very minor remark - as you moved the timeval timeout declaration inside the loop in the src/PqResultImpl.cpp:485 it can be written as

timeval timeout = {1, 0};

instead of

timeval timeout = {0, 0};
timeout.tv_sec = 1;
@krlmlr

This comment has been minimized.

Copy link
Member

krlmlr commented Oct 5, 2018

Thanks. The code is slightly more expressive in the current state, I'm keeping it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment