Re: Adding error messages to a few slash commands - Mailing list pgsql-hackers

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