Re: Adding error messages to a few slash commands - Mailing list pgsql-hackers
From | Robin Haberkorn |
---|---|
Subject | Re: Adding error messages to a few slash commands |
Date | |
Msg-id | DB941S35S38J.QYI17NLQPZ5B@b1-systems.de Whole thread Raw |
In response to | Re: Adding error messages to a few slash commands ("Robin Haberkorn" <haberkorn@b1-systems.de>) |
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
pgsql-hackers by date: