public inbox for [email protected]  

help / color / mirror / Atom feed

From: Yugo NAGATA <[email protected]>
To: [email protected]
Subject: Fix cancellation check in ExecQueryAndProcessResults
Date: Fri, 8 Mar 2024 14:24:12 +0900
Message-ID: <[email protected]> (raw)

Hi,

While looking ExecQueryAndProcessResults, I found the following code.

    /*   
     * If SIGINT is sent while the query is processing, the interrupt will be
     * consumed.  The user's intention, though, is to cancel the entire watch
     * process, so detect a sent cancellation request and exit in this case.
     */
    if (is_watch && cancel_pressed)
    {    
        ClearOrSaveAllResults();
        return 0;
    }

I guess the intention is that when the query is cancelled during \watch process,
we want to exit early before handling the error. However, we cannot detect the
cancel at this timing because currently we use PQsendQuery which is asynchronous
and does not wait  the result. We have to check cancel_pressed after PQgetResult
call. I'm also attached a patch for this, with some comments fix. 

Regards,
Yugo Nagata

-- 
Yugo NAGATA <[email protected]>


Attachments:

  [text/x-diff] fix_query_cancellation_error_in_psql_watch.patch (1.3K, ../[email protected]/2-fix_query_cancellation_error_in_psql_watch.patch)
  download | inline diff:
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 76e01b02a3..7b83e221fd 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1406,9 +1406,9 @@ DescribeQuery(const char *query, double *elapsed_msec)
  * For other commands, the results are processed normally, depending on their
  * status.
  *
- * Returns 1 on complete success, 0 on interrupt and -1 or errors.  Possible
- * failure modes include purely client-side problems; check the transaction
- * status for the server-side opinion.
+ * Returns 1 on complete success, 0 on interrupt or results less than min_rows
+ * rows, and -1 on errors.  Possible failure modes include purely client-side
+ * problems; check the transaction status for the server-side opinion.
  *
  * Note that on a combined query, failure does not mean that nothing was
  * committed.
@@ -1450,6 +1450,9 @@ ExecQueryAndProcessResults(const char *query,
 		return -1;
 	}
 
+	/* first result */
+	result = PQgetResult(pset.db);
+
 	/*
 	 * If SIGINT is sent while the query is processing, the interrupt will be
 	 * consumed.  The user's intention, though, is to cancel the entire watch
@@ -1461,8 +1464,6 @@ ExecQueryAndProcessResults(const char *query,
 		return 0;
 	}
 
-	/* first result */
-	result = PQgetResult(pset.db);
 	if (min_rows > 0 && PQntuples(result) < min_rows)
 	{
 		return_early = true;

view thread (2+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected]
  Subject: Re: Fix cancellation check in ExecQueryAndProcessResults
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox