Skip to content
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

Interrupt handling #26

Merged
merged 1 commit into from
May 15, 2024
Merged

Interrupt handling #26

merged 1 commit into from
May 15, 2024

Conversation

mkaruza
Copy link
Collaborator

@mkaruza mkaruza commented May 8, 2024

  • Declare QuackNode to HOLD_INTERRUPTS for duration of scan. Once interrupt is observed we exit execution after heap pages are closed.

@mkaruza mkaruza requested a review from Tishj May 8, 2024 09:09
@mkaruza mkaruza force-pushed the safe-execution-interrupt branch 2 times, most recently from 583f4b4 to 9675c1c Compare May 8, 2024 09:21
@hlinnaka
Copy link
Collaborator

hlinnaka commented May 8, 2024

Note that most postgres internal functions can ereport an error for a wide range of reasons, so the code needs to be prepared for that anyway regardless of interrupts.

@mkaruza
Copy link
Collaborator Author

mkaruza commented May 8, 2024

Note that most postgres internal functions can ereport an error for a wide range of reasons, so the code needs to be prepared for that anyway regardless of interrupts.

@hlinnaka You are by right looking at PG code. This interrupt PR is mostly for safer cleaning and closing all pages during query execution. You think that this should be checking specific - QueryCancelPending variable and/or increasing QueryCancelHoldoffCount counter.

When custom scan node start we are increasing `QueryCancelHoldoffCount`
to block canceling query. In fetch page loop we are monitoring
`QueryCancelPending` signal for thread to finish execution.
`QueryCancelHoldoffCount` counter will be decreased when node exits.
@mkaruza mkaruza force-pushed the safe-execution-interrupt branch from 9675c1c to bf55f86 Compare May 13, 2024 07:14
@Mytherin
Copy link
Collaborator

Mytherin commented May 13, 2024

We can call Interrupt on the DuckDB Connection object to interrupt the running DuckDB query as well, that should allow us to interrupt during e.g. Parquet readers and other operations.

We could launch a background thread that polls periodically whether or not a cancellation has happened, when it has happened we call Interrupt.

@@ -47,14 +47,14 @@ void
Quack_BeginCustomScan(CustomScanState *cscanstate, EState *estate, int eflags) {
QuackScanState *quackScanState = (QuackScanState *)cscanstate;
quackScanState->css.ss.ps.ps_ResultTupleDesc = quackScanState->css.ss.ss_ScanTupleSlot->tts_tupleDescriptor;
HOLD_CANCEL_INTERRUPTS();
Copy link
Collaborator

@Mytherin Mytherin May 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could in the future e.g. have a background thread here:

void CheckForInterrupts() {
    while(!finished) {
        if (QueryCancelPending) {
               connection->Interrupt();
               break;
        }
        sleep(10ms);
    }
}

Base automatically changed from quack-node to main May 14, 2024 09:02
@mkaruza mkaruza merged commit f41dcd4 into main May 15, 2024
2 checks passed
@mkaruza mkaruza deleted the safe-execution-interrupt branch May 15, 2024 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants