Обсуждение: Adding error messages to a few slash commands

Hello hackers,

Currently, some slash commands in psql return an error saying "Did not
find any XXXX named YYYY" while some return an empty table. This patch
changes a few of the slash commands to return a similar error message.
I did not change all of the slash commands in this initial patch to
wait for feedback, happy to do that if this patch is acceptable. I did
not add any tests yet because the existing ones did not seem to have
any, happy to add tests if needed. Also, I know that we are in a
feature freeze, is such a change acceptable now?

Thanks

-- 
Thanks and regards
Abhishek Chanda

Sorry, I forgot to attach example usage. Here is how these currently behave:

postgres=> \dT foo
     List of data types
 Schema | Name | Description
--------+------+-------------
(0 rows)

postgres => \du foo
     List of roles
 Role name | Attributes
-----------+------------

postgres => \df foo
                       List of functions
 Schema | Name | Result data type | Argument data types | Type
--------+------+------------------+---------------------+------
(0 rows)

Here is how these look like after this change:

postgres=> \dT foo
Did not find any data type named "foo".
postgres => \df foo
Did not find any function named "foo".
postgres => \du foo
Did not find any role named "foo".

Thanks

On Sat, Apr 12, 2025 at 10:43 PM Abhishek Chanda
<abhishek.becs@gmail.com> wrote:
>
> Hello hackers,
>
> Currently, some slash commands in psql return an error saying "Did not
> find any XXXX named YYYY" while some return an empty table. This patch
> changes a few of the slash commands to return a similar error message.
> I did not change all of the slash commands in this initial patch to
> wait for feedback, happy to do that if this patch is acceptable. I did
> not add any tests yet because the existing ones did not seem to have
> any, happy to add tests if needed. Also, I know that we are in a
> feature freeze, is such a change acceptable now?
>
> Thanks
>
> --
> Thanks and regards
> Abhishek Chanda



--
Thanks and regards
Abhishek Chanda

Abhishek Chanda <abhishek.becs@gmail.com> writes:
> Currently, some slash commands in psql return an error saying "Did not
> find any XXXX named YYYY" while some return an empty table. This patch
> changes a few of the slash commands to return a similar error message.

Personally, if I were trying to make these things consistent, I'd have
gone in the other direction (ie return empty tables).  We don't make
psql throw an error when an ordinary user query returns zero rows;
why should \d commands do that?

> Also, I know that we are in a
> feature freeze, is such a change acceptable now?

Whether changing this is a good idea or not, it's surely hard
to claim that it's a bug fix.  So I'd say it's out of scope for
post-feature-freeze.

            regards, tom lane

Abhishek Chanda <abhishek.becs@gmail.com> writes:

Currently, some slash commands in psql return an error saying "Did not
find any XXXX named YYYY" while some return an empty table. This patch
changes a few of the slash commands to return a similar error message.

Personally, if I were trying to make these things consistent, I'd have
gone in the other direction (ie return empty tables).

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

Thanks for the feedback, attached an updated patch that changes most
of those "Did not find" messages to empty tables. I did not change two
sets, listDbRoleSettings and listTables both have comments describing
that these are deliberately this way.

I wanted to post this update to close this loop for now. I will follow
up once the merge window opens again.

Thanks

On Sun, Apr 13, 2025 at 1:47 AM Pavel Luzanov <p.luzanov@postgrespro.ru> wrote:
>
> On 13.04.2025 08:29, Tom Lane wrote:
>
> Abhishek Chanda <abhishek.becs@gmail.com> writes:
>
> Currently, some slash commands in psql return an error saying "Did not
> find any XXXX named YYYY" while some return an empty table. This patch
> changes a few of the slash commands to return a similar error message.
>
>
> +1 for this patch.
>
> Personally, if I were trying to make these things consistent, I'd have
> gone in the other direction (ie return empty tables).
>
>
> +1
> Returning empty tables is a more appropriate behavior.
>
> --
> Pavel Luzanov
> Postgres Professional: https://postgrespro.com



--
Thanks and regards
Abhishek Chanda

Thanks for the feedback, attached an updated patch that changes most
of those "Did not find" messages to empty tables. I did not change two
sets, listDbRoleSettings and listTables both have comments describing
that these are deliberately this way.

I wanted to post this update to close this loop for now. I will follow
up once the merge window opens again.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com

Thanks Pavel, done now https://commitfest.postgresql.org/patch/5699/

Thanks

On Mon, Apr 14, 2025 at 4:27 AM Pavel Luzanov <p.luzanov@postgrespro.ru> wrote:
>
> On 13.04.2025 19:40, Abhishek Chanda wrote:
>
> Thanks for the feedback, attached an updated patch that changes most
> of those "Did not find" messages to empty tables. I did not change two
> sets, listDbRoleSettings and listTables both have comments describing
> that these are deliberately this way.
>
>
> Thanks.
>
> I wanted to post this update to close this loop for now. I will follow
> up once the merge window opens again.
>
>
> I recommend to create a new entry for this patch in the open July commitfest.
> I will be able to review this patch in a couple months later in June,
> if no one wants to review it earlier.
>
> --
> Pavel Luzanov
> Postgres Professional: https://postgrespro.com



--
Thanks and regards
Abhishek Chanda

On Mon Apr 14, 2025 at 12:27:53 GMT +03, Pavel Luzanov wrote:
> I recommend to create a new entry for this patch in the open July
> commitfest <https://commitfest.postgresql.org/53/>.
> I will be able to review this patch in a couple months later in June,
> if no one wants to review it earlier.

I could review it right now, i.e. do a pre-commit review according to [1].
Still have to "pay off" my own Commitfest entry. This would be my first PG
review. But is it even desirable to write reviews before the beginning of the
Commitfest? An important part -- especially in simple patches like this one
-- would be to apply it to HEAD. And shouldn't that be better done as late as
possible?

[1] https://wiki.postgresql.org/wiki/Reviewing_a_Patch

--
Robin Haberkorn
Senior Software Engineer

B1 Systems GmbH
Osterfeldstraße 7 / 85088 Vohburg / https://www.b1-systems.de
GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt, HRB 3537

On Sun, 13 Apr 2025 at 17:40, Abhishek Chanda <abhishek.becs@gmail.com> wrote:
>
> Thanks for the feedback, attached an updated patch that changes most
> of those "Did not find" messages to empty tables. I did not change two
> sets, listDbRoleSettings and listTables both have comments describing
> that these are deliberately this way.
>

+1 for always displaying an empty table if no objects are found.

The reason given for listDbRoleSettings() being an exception could be
solved by making the title depend on the number of arguments, e.g.,
"List of settings for role \"%s\" and database \"%s\".".

It's not clear what the "historical reasons" are for listTables()
being an exception, but if we're changing any of these, I'd say we
should make them all consistent.

Regards,
Dean

Hello Abhishek,

I have reviewed your patch after there was no response to my initial proposal
and nobody else wrote a review yet.

The patch is in the correct unified-diff format (apparently generated via git
format-patch). It applies cleanly to the current master branch. The patch does
not however add any tests (e.g. to src/bin/psql/t/001_basic.pl). On the other
hand, there weren't any tests for the affected slash-commands, so the patch
doesn't break the test suite.

The patch tries to remove special "Did not find any XXXX named YYYY" error
messages for `\d` commands (`\d`, `\dx+`, `\dRp+`, `\dFp` and `\dF`) in psql,
printing empty tables instead. This would make the behavior of the `\d`
commands more consistent with the the behavior of ordinary user queries. On the
other hand, the change in question could well break existing scripts calling
`psql -c '\d ...'`, especially since the process return code changes. For
instance, before the proposed change, psql would fail with return code 1:

# psql -c '\d foo'; echo $?
Did not find any relation named "foo".
1

After applying the patch, the return code will be 0 (success) instead:

# psql -c '\d foo'; echo $?
0

I would suggest to rework the patch, so that 1 is still returned. This is also
in line with how UNIX tools like `grep` behave in case they report an "empty"
response (i.e. if `grep` does not produce any match in the given files). On the
other hand returning 1 without printing any error message might create new
inconsistencies in psql. More feedback is required from the community on that
question. If the return code is considered important by the community, it would
be a good reason for adding a test case, so that psql behavior doesn't change
unexpectedly in the future.

It is noteworthy, that both before and after the patch, a plain `\d` does
output an error message while the psql process still returns with code 0:

# psql -c '\d'; echo $?
Did not find any relations.
0

You clarified you didn't remove the messages because of code comments in
listTables() and listDbRoleSettings(). A failure return code however should
probably still be returned (if we decide that this is what all the other \d
commands should do).

The examples above also raise another possible issue. The commands `\d`, `\dx+`
and `\dRp+` actually do not print empty tables (instead of "Did not find any
XXXX named YYYY") when invoked with a pattern, but produce no output at all.
This probably makes sense considering that they could also print output for
multiple items (e.g. tables). The remaining affected commands (`\dFp` and
`\dF`) will actually print empty tables now.

Another point of criticism is that the patch needlessly adds an empty line in
describeRoles() - moreover a line with trailing whitespace. I would suggest to
remove that change before committing.

To summarize, in my opinion you should:

* Get feedback on the desired return codes of psql in case of
  empty responses.
* Fix the return codes if others agree that psql should return failure codes
  and add a test case.
* Remove the unnecessary change in describeRoles().

Best regards,
Robin Haberkorn

On Tue May 13, 2025 at 00:00:17 GMT +03, Robin Haberkorn wrote:
> On Mon Apr 14, 2025 at 12:27:53 GMT +03, Pavel Luzanov wrote:
>> I recommend to create a new entry for this patch in the open July
>> commitfest <https://commitfest.postgresql.org/53/>.
>> I will be able to review this patch in a couple months later in June,
>> if no one wants to review it earlier.
>
> I could review it right now, i.e. do a pre-commit review according to [1].
> Still have to "pay off" my own Commitfest entry. This would be my first PG
> review. But is it even desirable to write reviews before the beginning of the
> Commitfest? An important part -- especially in simple patches like this one
> -- would be to apply it to HEAD. And shouldn't that be better done as late as
> possible?
>
> [1] https://wiki.postgresql.org/wiki/Reviewing_a_Patch

--
Robin Haberkorn
Software Engineer

B1 Systems GmbH
Osterfeldstraße 7 / 85088 Vohburg / https://www.b1-systems.de
GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt, HRB 3537

Hi all,

Thanks a lot for the review, Robin. And I am terribly sorry for the
slow reply, it just took me a while to get to this. But I think I have
addressed all your feedback, I have changed everything to set an error
code of 1 if anything is not empty. Also added tests for the return
code as requested, and cleaned up the change in describeRoles().
Please let me know if I missed anything.

Attached an updated and rebased version of the patch.

Thanks

On Fri, Jul 11, 2025 at 4:15 AM Robin Haberkorn <haberkorn@b1-systems.de> wrote:
>
> Hello Abhishek,
>
> I have reviewed your patch after there was no response to my initial proposal
> and nobody else wrote a review yet.
>
> The patch is in the correct unified-diff format (apparently generated via git
> format-patch). It applies cleanly to the current master branch. The patch does
> not however add any tests (e.g. to src/bin/psql/t/001_basic.pl). On the other
> hand, there weren't any tests for the affected slash-commands, so the patch
> doesn't break the test suite.
>
> The patch tries to remove special "Did not find any XXXX named YYYY" error
> messages for `\d` commands (`\d`, `\dx+`, `\dRp+`, `\dFp` and `\dF`) in psql,
> printing empty tables instead. This would make the behavior of the `\d`
> commands more consistent with the the behavior of ordinary user queries. On the
> other hand, the change in question could well break existing scripts calling
> `psql -c '\d ...'`, especially since the process return code changes. For
> instance, before the proposed change, psql would fail with return code 1:
>
> # psql -c '\d foo'; echo $?
> Did not find any relation named "foo".
> 1
>
> After applying the patch, the return code will be 0 (success) instead:
>
> # psql -c '\d foo'; echo $?
> 0
>
> I would suggest to rework the patch, so that 1 is still returned. This is also
> in line with how UNIX tools like `grep` behave in case they report an "empty"
> response (i.e. if `grep` does not produce any match in the given files). On the
> other hand returning 1 without printing any error message might create new
> inconsistencies in psql. More feedback is required from the community on that
> question. If the return code is considered important by the community, it would
> be a good reason for adding a test case, so that psql behavior doesn't change
> unexpectedly in the future.
>
> It is noteworthy, that both before and after the patch, a plain `\d` does
> output an error message while the psql process still returns with code 0:
>
> # psql -c '\d'; echo $?
> Did not find any relations.
> 0
>
> You clarified you didn't remove the messages because of code comments in
> listTables() and listDbRoleSettings(). A failure return code however should
> probably still be returned (if we decide that this is what all the other \d
> commands should do).
>
> The examples above also raise another possible issue. The commands `\d`, `\dx+`
> and `\dRp+` actually do not print empty tables (instead of "Did not find any
> XXXX named YYYY") when invoked with a pattern, but produce no output at all.
> This probably makes sense considering that they could also print output for
> multiple items (e.g. tables). The remaining affected commands (`\dFp` and
> `\dF`) will actually print empty tables now.
>
> Another point of criticism is that the patch needlessly adds an empty line in
> describeRoles() - moreover a line with trailing whitespace. I would suggest to
> remove that change before committing.
>
> To summarize, in my opinion you should:
>
> * Get feedback on the desired return codes of psql in case of
>   empty responses.
> * Fix the return codes if others agree that psql should return failure codes
>   and add a test case.
> * Remove the unnecessary change in describeRoles().
>
> Best regards,
> Robin Haberkorn
>
> On Tue May 13, 2025 at 00:00:17 GMT +03, Robin Haberkorn wrote:
> > On Mon Apr 14, 2025 at 12:27:53 GMT +03, Pavel Luzanov wrote:
> >> I recommend to create a new entry for this patch in the open July
> >> commitfest <https://commitfest.postgresql.org/53/>.
> >> I will be able to review this patch in a couple months later in June,
> >> if no one wants to review it earlier.
> >
> > I could review it right now, i.e. do a pre-commit review according to [1].
> > Still have to "pay off" my own Commitfest entry. This would be my first PG
> > review. But is it even desirable to write reviews before the beginning of the
> > Commitfest? An important part -- especially in simple patches like this one
> > -- would be to apply it to HEAD. And shouldn't that be better done as late as
> > possible?
> >
> > [1] https://wiki.postgresql.org/wiki/Reviewing_a_Patch
>
> --
> Robin Haberkorn
> Software Engineer
>
> B1 Systems GmbH
> Osterfeldstraße 7 / 85088 Vohburg / https://www.b1-systems.de
> GF: Ralph Dehner / Unternehmenssitz: Vohburg / AG: Ingolstadt, HRB 3537



--
Thanks and regards
Abhishek Chanda

On Wed, Oct 15, 2025 at 10:56:47PM -0500, Abhishek Chanda wrote:
> Thanks a lot for the review, Robin. And I am terribly sorry for the
> slow reply, it just took me a while to get to this. But I think I have
> addressed all your feedback, I have changed everything to set an error
> code of 1 if anything is not empty. Also added tests for the return
> code as requested, and cleaned up the change in describeRoles().
> Please let me know if I missed anything.
>
> Attached an updated and rebased version of the patch.

There is something that I am not following here.  The consensus of the
thread seems to be that folks are OK to show the results as empty
tables instead of an error, backtracking on the "historical" reasons
documented in the code.  That's OK by me.  Why not.

However, your patch does the opposite.  For example:
=# \dg "no.such.role"
Did not find any role named ""no.such.role"".

On HEAD, this returns:
=# \dg "no.such.role"
     List of roles
 Role name | Attributes
-----------+------------

So your patch is doing the opposite of the consensus I am reading.
Note that `make check` complains immediately with that.  You also may
want to be careful that all the code paths you are patching are
covered in the regression tests.
--
Michael