Re: Adding error messages to a few slash commands

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(at)b1-systems(dot)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