Thread: BUG #18134: ROW_COUNT do not set to 0 when psql's \gset command get no rows returned

The following bug has been logged on the website:

Bug reference:      18134
Logged by:          amutu
Email address:      amutu@amutu.com
PostgreSQL version: 15.4
Operating system:   FreeBSD 13.2-RELEASE amd64
Description:

reproduce steps：

$psql postgres
psql (15.4)
Type "help" for help.

postgres=# select * from cmdq  where status= 'new' \gset
no rows returned for \gset
postgres=# \echo :ROW_COUNT
:ROW_COUNT
postgres=# select * from cmdq;
   cmd   |  src_ip   | worker | status |              ts               | id

---------+-----------+--------+--------+-------------------------------+----
 ls /tmp | 127.0.0.1 |        | done   | 2023-09-23 14:39:59.047309+08 |
1
(1 row)

postgres=# \echo :ROW_COUNT
1
postgres=# select * from cmdq  where status= 'new' \gset
no rows returned for \gset
postgres=# \echo :ROW_COUNT
1
------------------------------------------------
from the psql doc:
ROW_COUNT
The number of rows returned or affected by the last SQL query, !!!!or 0 if
the query failed or did not report a row count.!!!!

On Tue, 26 Sep 2023 at 09:26, PG Bug reporting form <noreply@postgresql.org> wrote:
> The following bug has been logged on the website:
>
> Bug reference:      18134
> Logged by:          amutu
> Email address:      amutu@amutu.com
> PostgreSQL version: 15.4
> Operating system:   FreeBSD 13.2-RELEASE amd64
> Description:
>
> reproduce steps：
>
> $psql postgres
> psql (15.4)
> Type "help" for help.
>
> postgres=# select * from cmdq  where status= 'new' \gset
> no rows returned for \gset
> postgres=# \echo :ROW_COUNT
> :ROW_COUNT
> postgres=# select * from cmdq;
>    cmd   |  src_ip   | worker | status |              ts               | id
>
> ---------+-----------+--------+--------+-------------------------------+----
>  ls /tmp | 127.0.0.1 |        | done   | 2023-09-23 14:39:59.047309+08 |
> 1
> (1 row)
>
> postgres=# \echo :ROW_COUNT
> 1
> postgres=# select * from cmdq  where status= 'new' \gset
> no rows returned for \gset
> postgres=# \echo :ROW_COUNT
> 1
> ------------------------------------------------
> from the psql doc:
> ROW_COUNT
> The number of rows returned or affected by the last SQL query, !!!!or 0 if
> the query failed or did not report a row count.!!!!

It seems SaveResultVariables() was lost when executing failed. Attached fix it.

diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index ede197bebe..cf6c7778ee 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1660,8 +1660,8 @@ ExecQueryAndProcessResults(const char *query,
         }
 
         /* set variables on last result if all went well */
-        if (!is_watch && last && success)
-            SetResultVariables(result, true);
+        if (!is_watch && last)
+            SetResultVariables(result, success);
 
         ClearOrSaveResult(result);
         result = next_result;

-- 
Regrads,
Japin Li
ChengDu WenWu Information Technology Co., Ltd.

On Wed, Sep 27, 2023 at 06:00:58PM +0800, Japin Li wrote:
> It seems SaveResultVariables() was lost when executing failed. Attached fix it.

Yeah, I think that you're right here, the variables should be set.
SetResultVariables() ought to be called even if \gset failed to return
a result.  We have a bunch of paths in the psql code (for backend
errors as well as errors internal to psql) where things are done this
way, and the top of SetResultVariables() even documents that.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Sep 27, 2023 at 06:00:58PM +0800, Japin Li wrote:
>> It seems SaveResultVariables() was lost when executing failed. Attached fix it.

> Yeah, I think that you're right here, the variables should be set.
> SetResultVariables() ought to be called even if \gset failed to return
> a result.

Agreed.  The adjacent comment is now a lie though.  Maybe just

-         /* set variables on last result if all went well */
+         /* set variables from last result */

Bigger-picture question: should we back-patch this, or just fix it in
HEAD?  It's wrong, but I'm not sure it's so obviously wrong that there
could not be somebody depending on the existing behavior.

            regards, tom lane

On Sat, Sep 30, 2023 at 08:44:07PM -0400, Tom Lane wrote:
> Bigger-picture question: should we back-patch this, or just fix it in
> HEAD?  It's wrong, but I'm not sure it's so obviously wrong that there
> could not be somebody depending on the existing behavior.

FWIW, I am not excited about a backpatch, so I would keep this as a
HEAD-only change.  Sure, the existing behavior is incorrect, but it
would be more annoying to break somebody's script flow after a minor
release.  And that's not critical.

(If you want to fix that yourself, please feel free.  I can do that
tomorrow by myself but life keeps me busy today and I don't have any
room to keep an eye on the buildfarm.)
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> FWIW, I am not excited about a backpatch, so I would keep this as a
> HEAD-only change.  Sure, the existing behavior is incorrect, but it
> would be more annoying to break somebody's script flow after a minor
> release.  And that's not critical.

> (If you want to fix that yourself, please feel free.  I can do that
> tomorrow by myself but life keeps me busy today and I don't have any
> room to keep an eye on the buildfarm.)

I'm content to wait a day and see if anybody wants to make an
argument for back-patching.

            regards, tom lane

On Sat, Sep 30, 2023 at 08:58:12PM -0400, Tom Lane wrote:
> I'm content to wait a day and see if anybody wants to make an
> argument for back-patching.

Okay.  Seeing nothing, I've looked more at that this morning and
applied the change to HEAD, for now.
--
Michael