Re: [pgAdmin][RM-2341]: Add menu option for starting PSQL

Hi Dave/Team,

On Mon, May 17, 2021 at 6:47 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Hi
>
> On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <
> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>
>> Hi Akshay/ Team,
>>
>> Please find the attached updated patch for the psql tool.
>>
>
> Hmm, this version is also broken. There's a typo in editor_template.html
> on line 138 - it splits a string across two lines which throws an error.
> Having fixed that...
>
> I also note there's a lot of Javascript in that HTML file. That should be
> pushed into the webpacked bundle I think, and not included inline in HTML.
>
I have moved most of the code in the js file, few things are still in HTML.

>
> A couple of other things I noticed:
>
> - The button is enabled if the treeview has a Server selected. It could be
> argued that the query tool should do the same (defaulting to the
> maintenance database), however, that would be a separate change, and psql
> should be consistent with the query tool.
>
It is now consistent with the query tool.

>
> - If I do a "select * from pg_class;" I still get:
>
> postgres=# select * from pg_class;
> WARNING: terminal is not fully functional
>
I am not able to reproduce the warning for the terminal (I am working on
Catalina 10.15.7), I checked on browsers (chrome, firefox, Safari) and also
checked on local nwjs runtime but still not able to reproduce the warning.
but found one limitation: if we try to load data from the table containing
millions of records, UI gets very slow.

>
> - I'm sure using \q in the previous version displayed a message saying the
> session exited (the one on line 138 of editor_template.html). It no longer
> seems to do so.
>

>
>>
>>
>> On Tue, May 11, 2021 at 3:40 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>
>>> Hi
>>>
>>> On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <
>>> akshay(dot)joshi(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Nikhil
>>>>
>>>> Following are the review comments:
>>>>
>>>> *GUI specific*:
>>>>
>>>> - We need a panel icon for PSQL like query tool, we can also add
>>>> that on the browser tree toolbar.
>>>> - PSQL Tool menu should be visible for all the child nodes of the
>>>> database node. Follow the same as Query Tool.
>>>> - PSQL tab title should be only database server name as the user
>>>> can change the database/user from PSQL command, so it's been difficult to
>>>> update the tab title.
>>>> - PSQL connection is still open even if we disconnect the database
>>>> server from the browser tree.
>>>>
>>>> *Code specific:*
>>>>
>>>> - Remove an extra space from requirements.txt and package.json
>>>> - Documentation needs to be updated to let the user know from where
>>>> the PSQL tool will open and on which node it is applicable.
>>>> - psql/__init__.py check there are so many unused imports please
>>>> remove them.
>>>> - We are not using cheroot so it should be removed from
>>>> requirements.txt and also remove the import statement from pgAdmin4.py
>>>> - Test cases are showing successful but actually, there are some
>>>> routing errors please check.
>>>>
>>>> A few other things I noticed:
>>>
>>> - I was prompted to enter a password. This should be passed in the
>>> environment to psql as it is for pg_dump etc.
>>> - There seems to be an issue with terminal compatibility (which I didn't
>>> have on my prototype):
>>>
>>> ml=# select * from pg_class;
>>> WARNING: terminal is not fully functional
>>> -[ RECORD 1 ]-------+----------------------------------------------
>>> oid | 79354
>>> relname | housing
>>> ...
>>>
>>> - The panel should honour the styleguide. I'm running in dark mode, and
>>> see a jet black background. I would expect to see the same
>>> background/foreground colours as the treeview.
>>> - I spotted at least one print() statement that shouldn't be there
>>> (debug output should go through the logger) - psql/__init__.py:235
>>> - This seems suspect - why would there be a password in a connection
>>> string we've built? And why would it be xxx?
>>>
>>> if 'password=xxx' in conn_attr:
>>> conn_attr = conn_attr.replace('password=xxx', '')
>>>
>>> - There's a thick white line at the bottom of the panel, where a
>>> horizontal scrollbar might be if there was one.
>>> - The trailing semi-colon should be removed from: "ERROR: Shell commands
>>> are disabled in psql for security;"
>>>
>>> Once we're happy with the patch in general, I'll do a string review
>>> before committing. In particular, I want to be sure the text in config.py
>>> is appropriately worded.
>>>
>>> This is shaping up nicely! Good work.
>>>
>>>
>>>>
>>>> On Mon, May 10, 2021 at 7:32 PM Nikhil Mohite <
>>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi Dave/ Team,
>>>>>
>>>>> PFA updated patch, sorry for the inconvenience, while cleanup I
>>>>> removed the unwanted libraries but forgot to remove the code related to
>>>>> them.
>>>>>
>>>>> On Mon, May 10, 2021 at 7:10 PM Dave Page <dpage(at)pgadmin(dot)org> wrote:
>>>>>
>>>>>> Hi
>>>>>>
>>>>>> On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <
>>>>>> nikhil(dot)mohite(at)enterprisedb(dot)com> wrote:
>>>>>>
>>>>>>> Hi Hackers,
>>>>>>>
>>>>>>> Please find the attached patch for RM-2341
>>>>>>> <https://redmine.postgresql.org/issues/2341>: Add Menu option for
>>>>>>> starting PSQL.
>>>>>>> 1. Added new Option PSQL Tool in Tools menu.
>>>>>>> 2. Added the same option for Server and Database nodes from the tree
>>>>>>> view.
>>>>>>>
>>>>>>
>>>>>> Unfortunately there's a trailing comma in package.json that makes it
>>>>>> invalid. If I fix that, then I get the error below, so I'm guessing the
>>>>>> intention was to actually include another package there?
>>>>>>
>>>>>> ERROR in ./pgadmin/tools/psql/static/js/psql_module.js 23:50-82
>>>>>> Module not found: Error: Can't resolve 'local-echo-controller' in
>>>>>> '/Users/dpage/git/pgadmin4/web/pgadmin/tools/psql/static/js'
>>>>>> resolve 'local-echo-controller' in
>>>>>> '/Users/dpage/git/pgadmin4/web/pgadmin/tools/psql/static/js'
>>>>>> Parsed request is a module
>>>>>> using description file: /Users/dpage/git/pgadmin4/web/package.json
>>>>>> (relative path: ./pgadmin/tools/psql/static/js)
>>>>>> aliased with mapping 'local-echo-controller':
>>>>>> '/Users/dpage/git/pgadmin4/web/node_modules/local-echo' to
>>>>>> '/Users/dpage/git/pgadmin4/web/node_modules/local-echo'
>>>>>> using description file:
>>>>>> /Users/dpage/git/pgadmin4/web/package.json (relative path:
>>>>>> ./pgadmin/tools/psql/static/js)
>>>>>> Field 'browser' doesn't contain a valid alias configuration
>>>>>> root path /Users/dpage/git/pgadmin4/web
>>>>>> using description file:
>>>>>> /Users/dpage/git/pgadmin4/web/package.json (relative path:
>>>>>> ./Users/dpage/git/pgadmin4/web/node_modules/local-echo)
>>>>>> no extension
>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>> configuration
>>>>>>
>>>>>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo
>>>>>> doesn't exist
>>>>>> .js
>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>> configuration
>>>>>>
>>>>>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo.js
>>>>>> doesn't exist
>>>>>> .jsx
>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>> configuration
>>>>>>
>>>>>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo.jsx
>>>>>> doesn't exist
>>>>>> as directory
>>>>>>
>>>>>> /Users/dpage/git/pgadmin4/web/Users/dpage/git/pgadmin4/web/node_modules/local-echo
>>>>>> doesn't exist
>>>>>> using description file:
>>>>>> /Users/dpage/git/pgadmin4/web/package.json (relative path:
>>>>>> ./node_modules/local-echo)
>>>>>> no extension
>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>> configuration
>>>>>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo
>>>>>> doesn't exist
>>>>>> .js
>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>> configuration
>>>>>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo.js
>>>>>> doesn't exist
>>>>>> .jsx
>>>>>> Field 'browser' doesn't contain a valid alias
>>>>>> configuration
>>>>>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo.jsx
>>>>>> doesn't exist
>>>>>> as directory
>>>>>> /Users/dpage/git/pgadmin4/web/node_modules/local-echo
>>>>>> doesn't exist
>>>>>> @ ./pgadmin/tools/psql/static/js/index.js 17:19-43
>>>>>>
>>>>>> 2021-05-10 14:38:37: webpack 5.21.2 compiled with 1 error in 60041 ms
>>>>>>
>>>>>> --
>>>>>> Dave Page
>>>>>> Blog: https://pgsnake.blogspot.com
>>>>>> Twitter: @pgsnake
>>>>>>
>>>>>> EDB: https://www.enterprisedb.com
>>>>>>
>>>>>
>>>>> Regards,
>>>>> Nikhil Mohite
>>>>>
>>>>
>>>>
>>>> --
>>>> *Thanks & Regards*
>>>> *Akshay Joshi*
>>>> *pgAdmin Hacker | Principal Software Architect*
>>>> *EDB Postgres <http://edbpostgres.com>*
>>>>
>>>> *Mobile: +91 976-788-8246*
>>>>
>>>
>>>
>>> --
>>> Dave Page
>>> Blog: https://pgsnake.blogspot.com
>>> Twitter: @pgsnake
>>>
>>> EDB: https://www.enterprisedb.com
>>>
>>> Regards,
>> Nikhil Mohite
>>
>
>
> --
> Dave Page
> Blog: https://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EDB: https://www.enterprisedb.com
>

Regards,
Nikhil Mohite