SQL errors are discarded when using the pspg pager

[lelit]

Description

I use the following settings in my pgclirc file:

pager = pspg --csv --rr=2 --quit-if-one-screen --ignore-case --csv-header on -s 4 -X
table_format = csv

and I'm quite happy, except for one notable defect: when I execute a query containing an error, pgcli does not properly handle the case and still emit the output coming from PG thru the pager, that simply discards it not being a CSV stream.

For example, not using the pager I get:

$ pgcli -h localhost -u user -d test
Server: PostgreSQL 11.8 (Debian 11.8-1.pgdg90+1)
Version: 3.0.0
Chat: https://gitter.im/dbcli/pgcli
Home: http://pgcli.com
user@localhost:test> select foo;
column "foo" does not exist
LINE 1: select foo
               ^

Time: 0.001s

while going thru pspg this is what I get:

$ pgcli -h localhost -u user -d test --pgclirc my.pgclirc
Server: PostgreSQL 11.8 (Debian 11.8-1.pgdg90+1)
Version: 3.0.0
Chat: https://gitter.im/dbcli/pgcli
Home: http://pgcli.com
user@localhost:test> select foo;
Time: 0.002s

Maybe the _evaluate_command() function should return that success flag to the caller and the latter emit the output without the pager?

Environment

$ pip freeze
cli-helpers==2.1.0
click==7.1.2
configobj==5.0.6
pendulum==2.1.2
-e git+https://github.com/dbcli/pgcli.git@8f7e31450835bca5d9a8bb4de252efba6f4b7b10#egg=pgcli
pgspecial==1.11.10
prompt-toolkit==3.0.5
psycopg2==2.8.4
pudb==2019.2
Pygments==2.6.1
python-dateutil==2.8.1
pytzdata==2020.1
setproctitle==1.1.10
six==1.15.0
sqlparse==0.3.1
tabulate==0.8.7
terminaltables==3.1.0
urwid==2.1.1
wcwidth==0.2.5

$ pspg --version
pspg-2.6.6
with readline (version: 0x0603)
with integrated menu
ncurses version: 6.1, patch: 20190112
ncurses with wide char support
ncurses widechar num: 1
wchar_t width: 4, max: 2147483647
with postgres client integration

The same happens with latest released version of pspg, 3.1.2.

[lelit]

With the following patch:

diff --git a/pgcli/main.py b/pgcli/main.py
index b146898..1f4c947 100644
--- a/pgcli/main.py
+++ b/pgcli/main.py
@@ -903,6 +903,9 @@ def _evaluate_command(self, text):
             logger.debug("rows: %r", cur)
             logger.debug("status: %r", status)
 
+            if not success and self.table_format == 'csv':
+                self.echo_via_pager(status)
+
             if self._should_limit_output(sql, cur):
                 cur, status = self._limit_output(cur)

the error above comes out as follows:

user@localhost:test> select foo;                                                                                                                                                                                                                          
┌────────────────────────────────────┐
│ ERRORE:  la colonna foo non esiste │
├────────────────────────────────────┤
│ LINE 1: select foo                 │
│ ^                                  │
└────────────────────────────────────┘
(2 rows)
user@localhost:test>                                                                                                                                                                                                                                      

Is this reasonable?

[lelit]

Apparently nobody is using pspg... 🤔

[j-bennet]

Hah. Apparently so! _evaluate_command returning success would be reasonable. Tabulating error output is not a great solution, even though it's better than completely losing error information.

[lelit]

I agree, but I could not figure out a non invasive alternative 🤷‍♂️
Do you think I should open a PR with the above?

[j-bennet]

Looking at _evaluate_command, it seems like it already is supposed to return a class with success flag set:

pgcli/pgcli/main.py

Line 947 in baea769

all_success,

so do things break/not handled in pgexecute.run then? We seem to be catching DatabaseError there, but does SQL error fit into that category?

pgcli/pgcli/pgexecute.py

Line 410 in baea769

except psycopg2.DatabaseError as e:

[lelit]

No, run() catches the error and signals it, in particular with a success (the second last item of the tuple) set to False, but then nothing in _evaluate_command() checks it.

[j-bennet]

@lelit Would you like to attempt fixing that?

[lelit]

In some other way than the simplistic way above you mean?

[j-bennet]

Yes. So we can have output consistent with non-pspg pagers.

[okbob]

#1230 is related to this issue too. Unfortunately, CSV is too simple format, and it is impossible clean garbage in format on reader side.

[jerzyk]

I've just hit this issue, plus there is a missing output from e.g. @delete from@ - no info on how many records were removed when output is set to csv

[okbob]

note: pgcli fixed issue with slow print in table format. So workaround with csv format is not necessary now.

[lelit]

@okbob I'm not sure to understand what you mean: are you saying that I can leave table_format to the default and remove --csv option from the pgpg pager, or what?

[okbob]

pá 28. 5. 2021 v 17:40 odesílatel Lele Gaifax ***@***.***> napsal:

@okbob <https://github.com/okbob> I'm not sure to understand what you mean: are you saying that I can leave table_format to the default and remove --csv option from the pgpg pager, or what?

yes —

…

You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1200 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEFO45Q6NB44GC4FQPKFWLTP62QHANCNFSM4PP3VZLA> .

[lelit]

Thanks, I will try and report back. Maybe you could rectify the suggested configuration here?

[okbob]

yes, I'll fix it

[j-bennet]

@lelit Was your problem fixed?

[lelit]

I haven't tried okbob's solution yet, so I'm still using my hacked fork. I'll do my best to try the easy way and report back soon.

[lelit]

Yes, I confirm that what @okbob suggested fixes this issue!

To be clear, now my pgclirc contains

pager = pspg --rr=2 --quit-if-one-screen --ignore-case -s 4 -X
table_format = psql

and everything seems working as expected 🎉.

As a side note for @okbob: after looking up the meaning for the --pgcli-fix option you suggest in the pgcli section, it seems redundant without --csv.

Thanks a lot!

[okbob]

so 12. 6. 2021 v 11:56 odesílatel Lele Gaifax ***@***.***> napsal:

Yes, I confirm that what @okbob <https://github.com/okbob> suggested fixes this issue! To be clear, now my pgclirc contains pager = pspg --rr=2 --quit-if-one-screen --ignore-case -s 4 -X table_format = psql and everything seems working as expected 🎉. As a side note for @okbob <https://github.com/okbob>: after looking up the meaning for the --pgcli-fix option you suggest in the pgcli section, it seems redundant without --csv.

I modified pspg readme Thank you for check Pavel

…

Thanks a lot! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1200 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEFO4YR246K2XNDZRE4IU3TSMVK7ANCNFSM4PP3VZLA> .