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

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.

I conformed this patch fix the problem, thanks!

Japin Li <japinli@hotmail.com> 于2023年9月28日周四 10:51写道：

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