Re: Adding error messages to a few slash commands - Mailing list pgsql-hackers
From | Abhishek Chanda |
---|---|
Subject | Re: Adding error messages to a few slash commands |
Date | |
Msg-id | CAKiP-K-MNi9h4xwaykp7XFv+xuG7=Eq7wV_cRY8aBPd0Pa_sdQ@mail.gmail.com Whole thread Raw |
In response to | Re: Adding error messages to a few slash commands ("Robin Haberkorn" <haberkorn@b1-systems.de>) |
Responses |
Re: Adding error messages to a few slash commands
|
List | pgsql-hackers |
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
Attachment
pgsql-hackers by date: