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

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
54 messages Options
123
Reply | Threaded
Open this post in threaded view
|

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

nikhil.mohite
Hi Hackers,

Please find the attached patch for RM-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.





--
Thanks & Regards,
Nikhil Mohite
Software Engineer.
Mob.No: +91-7798364578.

RM_2341.patch (153K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Dave Page-7
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--
Reply | Threaded
Open this post in threaded view
|

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

nikhil.mohite
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 <[hidden email]> wrote:
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--

Regards,
Nikhil Mohite 

RM_2341_V2.patch (152K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Akshay Joshi
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.

On Mon, May 10, 2021 at 7:32 PM Nikhil Mohite <[hidden email]> 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 <[hidden email]> wrote:
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--

Regards,
Nikhil Mohite 


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246

Reply | Threaded
Open this post in threaded view
|

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

Dave Page-7
Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--

Regards,
Nikhil Mohite 


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Reply | Threaded
Open this post in threaded view
|

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

nikhil.mohite
Hi Akshay/ Team,

Please find the attached updated patch for the psql tool.


On Tue, May 11, 2021 at 3:40 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--

Regards,
Nikhil Mohite 


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Regards,
Nikhil Mohite 

RM_2341_V3.patch (480K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Dave Page-7
Hi

On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <[hidden email]> 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.

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.

- If I do a "select * from pg_class;" I still get:

postgres=# select * from pg_class;
WARNING: terminal is not fully functional

- 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 <[hidden email]> wrote:
Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--

Regards,
Nikhil Mohite 


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Regards,
Nikhil Mohite 


--
Reply | Threaded
Open this post in threaded view
|

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

nikhil.mohite
Hi Dave/Team,


On Mon, May 17, 2021 at 6:47 PM Dave Page <[hidden email]> wrote:
Hi

On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <[hidden email]> 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 <[hidden email]> wrote:
Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--

Regards,
Nikhil Mohite 


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Regards,
Nikhil Mohite 


--

Regards,
Nikhil Mohite 

RM_2341_V4.patch (480K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Dave Page-7
Hi

On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/Team,


On Mon, May 17, 2021 at 6:47 PM Dave Page <[hidden email]> wrote:
Hi

On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <[hidden email]> 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.

Hmm, yes - in particular, colours for the different themes. Please move them into the css for the themes. You have a mix of style, layout and code in this file which needs to be cleaned up.

Speaking of themes, the background colour for selected text doesn't seem right (it's barely visible) in the dark theme. Can you fix that to match the colouring in the SQL text boxes please?
 

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:

It looks like that can be fixed by adding:

env={'TERM': 'xterm'}

to the subprocess.Popen() call.

I noticed while I was playing with that, that you are passing the password as part of the connection string. As I've mentioned in the past, that is absolutely not acceptable; it will expose the password to all manner of tools (such as ps -ef). You *must* pass the password to psql using the PGPASSWORD environment variable.
 
if we try to load data from the table containing millions of records, UI gets very slow.

Is xtermjs discarding the older buffer contents when it fills up? Can you tell where the memory usage is?

 

- 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.  

In addition to the issue above, it looks like the \! blocking may have lost it's ability to ignore quoted strings:

pgweb=# select '\!';
ERROR: Shell commands are disabled in psql for security
 
 


On Tue, May 11, 2021 at 3:40 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--

Regards,
Nikhil Mohite 


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Regards,
Nikhil Mohite 


--

Regards,
Nikhil Mohite 


--
Reply | Threaded
Open this post in threaded view
|

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

nikhil.mohite
Hi Dave/ Team,

On Tue, May 18, 2021 at 8:41 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/Team,


On Mon, May 17, 2021 at 6:47 PM Dave Page <[hidden email]> wrote:
Hi

On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <[hidden email]> 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.

Hmm, yes - in particular, colours for the different themes. Please move them into the css for the themes. You have a mix of style, layout and code in this file which needs to be cleaned up.
xterm V3 onwards they have provided the API to set the theme and other settings, earlier I tried with CSS to override the theme but couldn’t able to apply the theme properly as some style get applied as in-line style for the HTML, so used the API to set the theme.


Speaking of themes, the background colour for selected text doesn't seem right (it's barely visible) in the dark theme. Can you fix that to match the colouring in the SQL text boxes please? 
 

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:

It looks like that can be fixed by adding:

env={'TERM': 'xterm'}

to the subprocess.Popen() call.

I noticed while I was playing with that, that you are passing the password as part of the connection string. As I've mentioned in the past, that is absolutely not acceptable; it will expose the password to all manner of tools (such as ps -ef). You *must* pass the password to psql using the PGPASSWORD environment variable.
 
if we try to load data from the table containing millions of records, UI gets very slow.

Is xtermjs discarding the older buffer contents when it fills up? Can you tell where the memory usage is?
I checked the psql memory consumption in terminal and pgAdmin psql tool memory consumption is the similar. Also tested the performance and query execution timing is also  similar.

 

- 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.  

In addition to the issue above, it looks like the \! blocking may have lost it's ability to ignore quoted strings:

pgweb=# select '\!';
ERROR: Shell commands are disabled in psql for security 
 
 


On Tue, May 11, 2021 at 3:40 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--

Regards,
Nikhil Mohite 


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Regards,
Nikhil Mohite 


--

Regards,
Nikhil Mohite 


--


Regards,
Nikhil Mohite


--
Thanks & Regards,
Nikhil Mohite
Software Engineer.
Mob.No: +91-7798364578.
Reply | Threaded
Open this post in threaded view
|

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

Dave Page-7
Hi

On Wed, May 19, 2021 at 8:58 AM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/ Team,

On Tue, May 18, 2021 at 8:41 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/Team,


On Mon, May 17, 2021 at 6:47 PM Dave Page <[hidden email]> wrote:
Hi

On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <[hidden email]> 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.

Hmm, yes - in particular, colours for the different themes. Please move them into the css for the themes. You have a mix of style, layout and code in this file which needs to be cleaned up.
xterm V3 onwards they have provided the API to set the theme and other settings, earlier I tried with CSS to override the theme but couldn’t able to apply the theme properly as some style get applied as in-line style for the HTML, so used the API to set the theme.

OK, but either way we can't hard-code styles from themes in HTML templates for individual features; that way leads to madness.

Perhaps Aditya or one of the other team members can give some assistance?
 


Speaking of themes, the background colour for selected text doesn't seem right (it's barely visible) in the dark theme. Can you fix that to match the colouring in the SQL text boxes please? 
 

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:

It looks like that can be fixed by adding:

env={'TERM': 'xterm'}

to the subprocess.Popen() call.

I noticed while I was playing with that, that you are passing the password as part of the connection string. As I've mentioned in the past, that is absolutely not acceptable; it will expose the password to all manner of tools (such as ps -ef). You *must* pass the password to psql using the PGPASSWORD environment variable.
 
if we try to load data from the table containing millions of records, UI gets very slow.

Is xtermjs discarding the older buffer contents when it fills up? Can you tell where the memory usage is?
I checked the psql memory consumption in terminal and pgAdmin psql tool memory consumption is the similar. Also tested the performance and query execution timing is also  similar.

OK, so there's probably not much we can do here.
 

 

- 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.  

In addition to the issue above, it looks like the \! blocking may have lost it's ability to ignore quoted strings:

pgweb=# select '\!';
ERROR: Shell commands are disabled in psql for security 
 
 


On Tue, May 11, 2021 at 3:40 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--

Regards,
Nikhil Mohite 


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Regards,
Nikhil Mohite 


--

Regards,
Nikhil Mohite 


--


Regards,
Nikhil Mohite


--
Thanks & Regards,
Nikhil Mohite
Software Engineer.
Mob.No: +91-7798364578.


--
Reply | Threaded
Open this post in threaded view
|

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

nikhil.mohite
Hi Dave/ Team,

On Wed, May 19, 2021 at 1:43 PM Dave Page <[hidden email]> wrote:
Hi

On Wed, May 19, 2021 at 8:58 AM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/ Team,

On Tue, May 18, 2021 at 8:41 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/Team,


On Mon, May 17, 2021 at 6:47 PM Dave Page <[hidden email]> wrote:
Hi

On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <[hidden email]> 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.

Hmm, yes - in particular, colours for the different themes. Please move them into the css for the themes. You have a mix of style, layout and code in this file which needs to be cleaned up.
xterm V3 onwards they have provided the API to set the theme and other settings, earlier I tried with CSS to override the theme but couldn’t able to apply the theme properly as some style get applied as in-line style for the HTML, so used the API to set the theme.

OK, but either way we can't hard-code styles from themes in HTML templates for individual features; that way leads to madness.

Perhaps Aditya or one of the other team members can give some assistance?
I have moved the color settings to the respective theme files. Aditya helped in this.
 


Speaking of themes, the background colour for selected text doesn't seem right (it's barely visible) in the dark theme. Can you fix that to match the colouring in the SQL text boxes please? 
I tried the default selection color from SQL for the dark and standard themes but still, it was not readable so just updated the color code with another color as follows.
1. Dark Theme:
Screenshot 2021-05-19 at 6.29.43 PM.png
2. High Contrast: (using default SQL selection color)
Screenshot 2021-05-19 at 6.59.52 PM.png
3. Standard:
image.png
can we go with the colors or should we update it?
 

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:

It looks like that can be fixed by adding:

env={'TERM': 'xterm'}
Added this in the environment when opening the psql panel.

to the subprocess.Popen() call.

I noticed while I was playing with that, that you are passing the password as part of the connection string. As I've mentioned in the past, that is absolutely not acceptable; it will expose the password to all manner of tools (such as ps -ef). You *must* pass the password to psql using the PGPASSWORD environment variable.
 
if we try to load data from the table containing millions of records, UI gets very slow.
Removed the password from the connection string and added 'PGPASSWORD' in the environment. 

Is xtermjs discarding the older buffer contents when it fills up? Can you tell where the memory usage is?
I checked the psql memory consumption in terminal and pgAdmin psql tool memory consumption is the similar. Also tested the performance and query execution timing is also  similar.

OK, so there's probably not much we can do here.
 

 

- 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.  

In addition to the issue above, it looks like the \! blocking may have lost it's ability to ignore quoted strings:

pgweb=# select '\!';
ERROR: Shell commands are disabled in psql for security 
Fixed the issue now it is consistent with the psql terminal. 
 
 


On Tue, May 11, 2021 at 3:40 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--

Regards,
Nikhil Mohite 


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Regards,
Nikhil Mohite 


--

Regards,
Nikhil Mohite 


--


Regards,
Nikhil Mohite


--
Thanks & Regards,
Nikhil Mohite
Software Engineer.
Mob.No: +91-7798364578.


--
Regards,
Nikhil Mohite 

RM_2341_V5.patch (484K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Dave Page-7
Thanks Nikhil. Can someone else review this version please?

On Wed, May 19, 2021 at 2:42 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/ Team,

On Wed, May 19, 2021 at 1:43 PM Dave Page <[hidden email]> wrote:
Hi

On Wed, May 19, 2021 at 8:58 AM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/ Team,

On Tue, May 18, 2021 at 8:41 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/Team,


On Mon, May 17, 2021 at 6:47 PM Dave Page <[hidden email]> wrote:
Hi

On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <[hidden email]> 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.

Hmm, yes - in particular, colours for the different themes. Please move them into the css for the themes. You have a mix of style, layout and code in this file which needs to be cleaned up.
xterm V3 onwards they have provided the API to set the theme and other settings, earlier I tried with CSS to override the theme but couldn’t able to apply the theme properly as some style get applied as in-line style for the HTML, so used the API to set the theme.

OK, but either way we can't hard-code styles from themes in HTML templates for individual features; that way leads to madness.

Perhaps Aditya or one of the other team members can give some assistance?
I have moved the color settings to the respective theme files. Aditya helped in this.
 


Speaking of themes, the background colour for selected text doesn't seem right (it's barely visible) in the dark theme. Can you fix that to match the colouring in the SQL text boxes please? 
I tried the default selection color from SQL for the dark and standard themes but still, it was not readable so just updated the color code with another color as follows.
1. Dark Theme:
Screenshot 2021-05-19 at 6.29.43 PM.png
2. High Contrast: (using default SQL selection color)
Screenshot 2021-05-19 at 6.59.52 PM.png
3. Standard:
image.png
can we go with the colors or should we update it?
 

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:

It looks like that can be fixed by adding:

env={'TERM': 'xterm'}
Added this in the environment when opening the psql panel.

to the subprocess.Popen() call.

I noticed while I was playing with that, that you are passing the password as part of the connection string. As I've mentioned in the past, that is absolutely not acceptable; it will expose the password to all manner of tools (such as ps -ef). You *must* pass the password to psql using the PGPASSWORD environment variable.
 
if we try to load data from the table containing millions of records, UI gets very slow.
Removed the password from the connection string and added 'PGPASSWORD' in the environment. 

Is xtermjs discarding the older buffer contents when it fills up? Can you tell where the memory usage is?
I checked the psql memory consumption in terminal and pgAdmin psql tool memory consumption is the similar. Also tested the performance and query execution timing is also  similar.

OK, so there's probably not much we can do here.
 

 

- 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.  

In addition to the issue above, it looks like the \! blocking may have lost it's ability to ignore quoted strings:

pgweb=# select '\!';
ERROR: Shell commands are disabled in psql for security 
Fixed the issue now it is consistent with the psql terminal. 
 
 


On Tue, May 11, 2021 at 3:40 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--

Regards,
Nikhil Mohite 


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Regards,
Nikhil Mohite 


--

Regards,
Nikhil Mohite 


--


Regards,
Nikhil Mohite


--
Thanks & Regards,
Nikhil Mohite
Software Engineer.
Mob.No: +91-7798364578.


--
Regards,
Nikhil Mohite 


--
Reply | Threaded
Open this post in threaded view
|

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

Akshay Joshi
Hi Nikhil 

Following are the review comments:
  • Set "ENABLE_PSQL = False", PSQL button from browser tree and context menu option should not be visible.
  • Documentation screenshot should be in standard theme for consistency, and check the size it's very large. Take the screenshot with the new PSQL button on the browser tree.
  • Update 'menu_bar.rst' and 'toolbar.rst' with new changes.
  • Remove commented code (if any)
  • Check SonarQube (I haven't run)

On Thu, May 20, 2021 at 2:52 PM Dave Page <[hidden email]> wrote:
Thanks Nikhil. Can someone else review this version please?

On Wed, May 19, 2021 at 2:42 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/ Team,

On Wed, May 19, 2021 at 1:43 PM Dave Page <[hidden email]> wrote:
Hi

On Wed, May 19, 2021 at 8:58 AM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/ Team,

On Tue, May 18, 2021 at 8:41 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/Team,


On Mon, May 17, 2021 at 6:47 PM Dave Page <[hidden email]> wrote:
Hi

On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <[hidden email]> 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.

Hmm, yes - in particular, colours for the different themes. Please move them into the css for the themes. You have a mix of style, layout and code in this file which needs to be cleaned up.
xterm V3 onwards they have provided the API to set the theme and other settings, earlier I tried with CSS to override the theme but couldn’t able to apply the theme properly as some style get applied as in-line style for the HTML, so used the API to set the theme.

OK, but either way we can't hard-code styles from themes in HTML templates for individual features; that way leads to madness.

Perhaps Aditya or one of the other team members can give some assistance?
I have moved the color settings to the respective theme files. Aditya helped in this.
 


Speaking of themes, the background colour for selected text doesn't seem right (it's barely visible) in the dark theme. Can you fix that to match the colouring in the SQL text boxes please? 
I tried the default selection color from SQL for the dark and standard themes but still, it was not readable so just updated the color code with another color as follows.
1. Dark Theme:
Screenshot 2021-05-19 at 6.29.43 PM.png
2. High Contrast: (using default SQL selection color)
Screenshot 2021-05-19 at 6.59.52 PM.png
3. Standard:
image.png
can we go with the colors or should we update it?
 

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:

It looks like that can be fixed by adding:

env={'TERM': 'xterm'}
Added this in the environment when opening the psql panel.

to the subprocess.Popen() call.

I noticed while I was playing with that, that you are passing the password as part of the connection string. As I've mentioned in the past, that is absolutely not acceptable; it will expose the password to all manner of tools (such as ps -ef). You *must* pass the password to psql using the PGPASSWORD environment variable.
 
if we try to load data from the table containing millions of records, UI gets very slow.
Removed the password from the connection string and added 'PGPASSWORD' in the environment. 

Is xtermjs discarding the older buffer contents when it fills up? Can you tell where the memory usage is?
I checked the psql memory consumption in terminal and pgAdmin psql tool memory consumption is the similar. Also tested the performance and query execution timing is also  similar.

OK, so there's probably not much we can do here.
 

 

- 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.  

In addition to the issue above, it looks like the \! blocking may have lost it's ability to ignore quoted strings:

pgweb=# select '\!';
ERROR: Shell commands are disabled in psql for security 
Fixed the issue now it is consistent with the psql terminal. 
 
 


On Tue, May 11, 2021 at 3:40 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--

Regards,
Nikhil Mohite 


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Regards,
Nikhil Mohite 


--

Regards,
Nikhil Mohite 


--


Regards,
Nikhil Mohite


--
Thanks & Regards,
Nikhil Mohite
Software Engineer.
Mob.No: +91-7798364578.


--
Regards,
Nikhil Mohite 


--


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246

Reply | Threaded
Open this post in threaded view
|

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

nikhil.mohite
Hi Akshay/ Team

On Mon, May 24, 2021 at 9:19 PM Akshay Joshi <[hidden email]> wrote:
Hi Nikhil 

Following are the review comments:
  • Set "ENABLE_PSQL = False", PSQL button from browser tree and context menu option should not be visible.
  • Documentation screenshot should be in standard theme for consistency, and check the size it's very large. Take the screenshot with the new PSQL button on the browser tree.
  • Update 'menu_bar.rst' and 'toolbar.rst' with new changes.
  • Remove commented code (if any)
  • Check SonarQube (I haven't run)
Please find the updated patch, resolve all the review comments, and update the code to resolve the SonarQube issues.

On Thu, May 20, 2021 at 2:52 PM Dave Page <[hidden email]> wrote:
Thanks Nikhil. Can someone else review this version please?

On Wed, May 19, 2021 at 2:42 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/ Team,

On Wed, May 19, 2021 at 1:43 PM Dave Page <[hidden email]> wrote:
Hi

On Wed, May 19, 2021 at 8:58 AM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/ Team,

On Tue, May 18, 2021 at 8:41 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/Team,


On Mon, May 17, 2021 at 6:47 PM Dave Page <[hidden email]> wrote:
Hi

On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <[hidden email]> 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.

Hmm, yes - in particular, colours for the different themes. Please move them into the css for the themes. You have a mix of style, layout and code in this file which needs to be cleaned up.
xterm V3 onwards they have provided the API to set the theme and other settings, earlier I tried with CSS to override the theme but couldn’t able to apply the theme properly as some style get applied as in-line style for the HTML, so used the API to set the theme.

OK, but either way we can't hard-code styles from themes in HTML templates for individual features; that way leads to madness.

Perhaps Aditya or one of the other team members can give some assistance?
I have moved the color settings to the respective theme files. Aditya helped in this.
 


Speaking of themes, the background colour for selected text doesn't seem right (it's barely visible) in the dark theme. Can you fix that to match the colouring in the SQL text boxes please? 
I tried the default selection color from SQL for the dark and standard themes but still, it was not readable so just updated the color code with another color as follows.
1. Dark Theme:
Screenshot 2021-05-19 at 6.29.43 PM.png
2. High Contrast: (using default SQL selection color)
Screenshot 2021-05-19 at 6.59.52 PM.png
3. Standard:
image.png
can we go with the colors or should we update it?
 

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:

It looks like that can be fixed by adding:

env={'TERM': 'xterm'}
Added this in the environment when opening the psql panel.

to the subprocess.Popen() call.

I noticed while I was playing with that, that you are passing the password as part of the connection string. As I've mentioned in the past, that is absolutely not acceptable; it will expose the password to all manner of tools (such as ps -ef). You *must* pass the password to psql using the PGPASSWORD environment variable.
 
if we try to load data from the table containing millions of records, UI gets very slow.
Removed the password from the connection string and added 'PGPASSWORD' in the environment. 

Is xtermjs discarding the older buffer contents when it fills up? Can you tell where the memory usage is?
I checked the psql memory consumption in terminal and pgAdmin psql tool memory consumption is the similar. Also tested the performance and query execution timing is also  similar.

OK, so there's probably not much we can do here.
 

 

- 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.  

In addition to the issue above, it looks like the \! blocking may have lost it's ability to ignore quoted strings:

pgweb=# select '\!';
ERROR: Shell commands are disabled in psql for security 
Fixed the issue now it is consistent with the psql terminal. 
 
 


On Tue, May 11, 2021 at 3:40 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--

Regards,
Nikhil Mohite 


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Regards,
Nikhil Mohite 


--

Regards,
Nikhil Mohite 


--


Regards,
Nikhil Mohite


--
Thanks & Regards,
Nikhil Mohite
Software Engineer.
Mob.No: +91-7798364578.


--
Regards,
Nikhil Mohite 


--


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246


RM_2341_V6.patch (700K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Akshay Joshi
Hi Nikhil

Please rebase and send the patch again.

On Tue, May 25, 2021 at 2:52 PM Nikhil Mohite <[hidden email]> wrote:
Hi Akshay/ Team

On Mon, May 24, 2021 at 9:19 PM Akshay Joshi <[hidden email]> wrote:
Hi Nikhil 

Following are the review comments:
  • Set "ENABLE_PSQL = False", PSQL button from browser tree and context menu option should not be visible.
  • Documentation screenshot should be in standard theme for consistency, and check the size it's very large. Take the screenshot with the new PSQL button on the browser tree.
  • Update 'menu_bar.rst' and 'toolbar.rst' with new changes.
  • Remove commented code (if any)
  • Check SonarQube (I haven't run)
Please find the updated patch, resolve all the review comments, and update the code to resolve the SonarQube issues.

On Thu, May 20, 2021 at 2:52 PM Dave Page <[hidden email]> wrote:
Thanks Nikhil. Can someone else review this version please?

On Wed, May 19, 2021 at 2:42 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/ Team,

On Wed, May 19, 2021 at 1:43 PM Dave Page <[hidden email]> wrote:
Hi

On Wed, May 19, 2021 at 8:58 AM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/ Team,

On Tue, May 18, 2021 at 8:41 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/Team,


On Mon, May 17, 2021 at 6:47 PM Dave Page <[hidden email]> wrote:
Hi

On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <[hidden email]> 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.

Hmm, yes - in particular, colours for the different themes. Please move them into the css for the themes. You have a mix of style, layout and code in this file which needs to be cleaned up.
xterm V3 onwards they have provided the API to set the theme and other settings, earlier I tried with CSS to override the theme but couldn’t able to apply the theme properly as some style get applied as in-line style for the HTML, so used the API to set the theme.

OK, but either way we can't hard-code styles from themes in HTML templates for individual features; that way leads to madness.

Perhaps Aditya or one of the other team members can give some assistance?
I have moved the color settings to the respective theme files. Aditya helped in this.
 


Speaking of themes, the background colour for selected text doesn't seem right (it's barely visible) in the dark theme. Can you fix that to match the colouring in the SQL text boxes please? 
I tried the default selection color from SQL for the dark and standard themes but still, it was not readable so just updated the color code with another color as follows.
1. Dark Theme:
Screenshot 2021-05-19 at 6.29.43 PM.png
2. High Contrast: (using default SQL selection color)
Screenshot 2021-05-19 at 6.59.52 PM.png
3. Standard:
image.png
can we go with the colors or should we update it?
 

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:

It looks like that can be fixed by adding:

env={'TERM': 'xterm'}
Added this in the environment when opening the psql panel.

to the subprocess.Popen() call.

I noticed while I was playing with that, that you are passing the password as part of the connection string. As I've mentioned in the past, that is absolutely not acceptable; it will expose the password to all manner of tools (such as ps -ef). You *must* pass the password to psql using the PGPASSWORD environment variable.
 
if we try to load data from the table containing millions of records, UI gets very slow.
Removed the password from the connection string and added 'PGPASSWORD' in the environment. 

Is xtermjs discarding the older buffer contents when it fills up? Can you tell where the memory usage is?
I checked the psql memory consumption in terminal and pgAdmin psql tool memory consumption is the similar. Also tested the performance and query execution timing is also  similar.

OK, so there's probably not much we can do here.
 

 

- 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.  

In addition to the issue above, it looks like the \! blocking may have lost it's ability to ignore quoted strings:

pgweb=# select '\!';
ERROR: Shell commands are disabled in psql for security 
Fixed the issue now it is consistent with the psql terminal. 
 
 


On Tue, May 11, 2021 at 3:40 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--

Regards,
Nikhil Mohite 


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Regards,
Nikhil Mohite 


--

Regards,
Nikhil Mohite 


--


Regards,
Nikhil Mohite


--
Thanks & Regards,
Nikhil Mohite
Software Engineer.
Mob.No: +91-7798364578.


--
Regards,
Nikhil Mohite 


--


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246

Reply | Threaded
Open this post in threaded view
|

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

nikhil.mohite
Hi Akshay,


Please find the updated patch. (V6)
On Tue, May 25, 2021 at 2:55 PM Akshay Joshi <[hidden email]> wrote:
Hi Nikhil

Please rebase and send the patch again.

On Tue, May 25, 2021 at 2:52 PM Nikhil Mohite <[hidden email]> wrote:
Hi Akshay/ Team

On Mon, May 24, 2021 at 9:19 PM Akshay Joshi <[hidden email]> wrote:
Hi Nikhil 

Following are the review comments:
  • Set "ENABLE_PSQL = False", PSQL button from browser tree and context menu option should not be visible.
  • Documentation screenshot should be in standard theme for consistency, and check the size it's very large. Take the screenshot with the new PSQL button on the browser tree.
  • Update 'menu_bar.rst' and 'toolbar.rst' with new changes.
  • Remove commented code (if any)
  • Check SonarQube (I haven't run)
Please find the updated patch, resolve all the review comments, and update the code to resolve the SonarQube issues.

On Thu, May 20, 2021 at 2:52 PM Dave Page <[hidden email]> wrote:
Thanks Nikhil. Can someone else review this version please?

On Wed, May 19, 2021 at 2:42 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/ Team,

On Wed, May 19, 2021 at 1:43 PM Dave Page <[hidden email]> wrote:
Hi

On Wed, May 19, 2021 at 8:58 AM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/ Team,

On Tue, May 18, 2021 at 8:41 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/Team,


On Mon, May 17, 2021 at 6:47 PM Dave Page <[hidden email]> wrote:
Hi

On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <[hidden email]> 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.

Hmm, yes - in particular, colours for the different themes. Please move them into the css for the themes. You have a mix of style, layout and code in this file which needs to be cleaned up.
xterm V3 onwards they have provided the API to set the theme and other settings, earlier I tried with CSS to override the theme but couldn’t able to apply the theme properly as some style get applied as in-line style for the HTML, so used the API to set the theme.

OK, but either way we can't hard-code styles from themes in HTML templates for individual features; that way leads to madness.

Perhaps Aditya or one of the other team members can give some assistance?
I have moved the color settings to the respective theme files. Aditya helped in this.
 


Speaking of themes, the background colour for selected text doesn't seem right (it's barely visible) in the dark theme. Can you fix that to match the colouring in the SQL text boxes please? 
I tried the default selection color from SQL for the dark and standard themes but still, it was not readable so just updated the color code with another color as follows.
1. Dark Theme:
Screenshot 2021-05-19 at 6.29.43 PM.png
2. High Contrast: (using default SQL selection color)
Screenshot 2021-05-19 at 6.59.52 PM.png
3. Standard:
image.png
can we go with the colors or should we update it?
 

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:

It looks like that can be fixed by adding:

env={'TERM': 'xterm'}
Added this in the environment when opening the psql panel.

to the subprocess.Popen() call.

I noticed while I was playing with that, that you are passing the password as part of the connection string. As I've mentioned in the past, that is absolutely not acceptable; it will expose the password to all manner of tools (such as ps -ef). You *must* pass the password to psql using the PGPASSWORD environment variable.
 
if we try to load data from the table containing millions of records, UI gets very slow.
Removed the password from the connection string and added 'PGPASSWORD' in the environment. 

Is xtermjs discarding the older buffer contents when it fills up? Can you tell where the memory usage is?
I checked the psql memory consumption in terminal and pgAdmin psql tool memory consumption is the similar. Also tested the performance and query execution timing is also  similar.

OK, so there's probably not much we can do here.
 

 

- 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.  

In addition to the issue above, it looks like the \! blocking may have lost it's ability to ignore quoted strings:

pgweb=# select '\!';
ERROR: Shell commands are disabled in psql for security 
Fixed the issue now it is consistent with the psql terminal. 
 
 


On Tue, May 11, 2021 at 3:40 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--

Regards,
Nikhil Mohite 


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Regards,
Nikhil Mohite 


--

Regards,
Nikhil Mohite 


--


Regards,
Nikhil Mohite


--
Thanks & Regards,
Nikhil Mohite
Software Engineer.
Mob.No: +91-7798364578.


--
Regards,
Nikhil Mohite 


--


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246


RM_2341_V6.patch (698K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Akshay Joshi
Thanks, patch applied.

I have updated the screenshot and some documentation stuff.

On Tue, May 25, 2021 at 3:08 PM Nikhil Mohite <[hidden email]> wrote:
Hi Akshay,


Please find the updated patch. (V6)
On Tue, May 25, 2021 at 2:55 PM Akshay Joshi <[hidden email]> wrote:
Hi Nikhil

Please rebase and send the patch again.

On Tue, May 25, 2021 at 2:52 PM Nikhil Mohite <[hidden email]> wrote:
Hi Akshay/ Team

On Mon, May 24, 2021 at 9:19 PM Akshay Joshi <[hidden email]> wrote:
Hi Nikhil 

Following are the review comments:
  • Set "ENABLE_PSQL = False", PSQL button from browser tree and context menu option should not be visible.
  • Documentation screenshot should be in standard theme for consistency, and check the size it's very large. Take the screenshot with the new PSQL button on the browser tree.
  • Update 'menu_bar.rst' and 'toolbar.rst' with new changes.
  • Remove commented code (if any)
  • Check SonarQube (I haven't run)
Please find the updated patch, resolve all the review comments, and update the code to resolve the SonarQube issues.

On Thu, May 20, 2021 at 2:52 PM Dave Page <[hidden email]> wrote:
Thanks Nikhil. Can someone else review this version please?

On Wed, May 19, 2021 at 2:42 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/ Team,

On Wed, May 19, 2021 at 1:43 PM Dave Page <[hidden email]> wrote:
Hi

On Wed, May 19, 2021 at 8:58 AM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/ Team,

On Tue, May 18, 2021 at 8:41 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/Team,


On Mon, May 17, 2021 at 6:47 PM Dave Page <[hidden email]> wrote:
Hi

On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <[hidden email]> 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.

Hmm, yes - in particular, colours for the different themes. Please move them into the css for the themes. You have a mix of style, layout and code in this file which needs to be cleaned up.
xterm V3 onwards they have provided the API to set the theme and other settings, earlier I tried with CSS to override the theme but couldn’t able to apply the theme properly as some style get applied as in-line style for the HTML, so used the API to set the theme.

OK, but either way we can't hard-code styles from themes in HTML templates for individual features; that way leads to madness.

Perhaps Aditya or one of the other team members can give some assistance?
I have moved the color settings to the respective theme files. Aditya helped in this.
 


Speaking of themes, the background colour for selected text doesn't seem right (it's barely visible) in the dark theme. Can you fix that to match the colouring in the SQL text boxes please? 
I tried the default selection color from SQL for the dark and standard themes but still, it was not readable so just updated the color code with another color as follows.
1. Dark Theme:
Screenshot 2021-05-19 at 6.29.43 PM.png
2. High Contrast: (using default SQL selection color)
Screenshot 2021-05-19 at 6.59.52 PM.png
3. Standard:
image.png
can we go with the colors or should we update it?
 

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:

It looks like that can be fixed by adding:

env={'TERM': 'xterm'}
Added this in the environment when opening the psql panel.

to the subprocess.Popen() call.

I noticed while I was playing with that, that you are passing the password as part of the connection string. As I've mentioned in the past, that is absolutely not acceptable; it will expose the password to all manner of tools (such as ps -ef). You *must* pass the password to psql using the PGPASSWORD environment variable.
 
if we try to load data from the table containing millions of records, UI gets very slow.
Removed the password from the connection string and added 'PGPASSWORD' in the environment. 

Is xtermjs discarding the older buffer contents when it fills up? Can you tell where the memory usage is?
I checked the psql memory consumption in terminal and pgAdmin psql tool memory consumption is the similar. Also tested the performance and query execution timing is also  similar.

OK, so there's probably not much we can do here.
 

 

- 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.  

In addition to the issue above, it looks like the \! blocking may have lost it's ability to ignore quoted strings:

pgweb=# select '\!';
ERROR: Shell commands are disabled in psql for security 
Fixed the issue now it is consistent with the psql terminal. 
 
 


On Tue, May 11, 2021 at 3:40 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--

Regards,
Nikhil Mohite 


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Regards,
Nikhil Mohite 


--

Regards,
Nikhil Mohite 


--


Regards,
Nikhil Mohite


--
Thanks & Regards,
Nikhil Mohite
Software Engineer.
Mob.No: +91-7798364578.


--
Regards,
Nikhil Mohite 


--


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246

Reply | Threaded
Open this post in threaded view
|

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

nikhil.mohite
Hi Team,

Following are few points related to PSQL tool on windows:
  1.  Currently using the pywinpty library on windows to create pty process and execute the  psql.exe.
  2. To read the stderr (errors) currently using '2>>&1' arguments to psql.exe command. (It will redirect stderr to stdout )
  3. Windows conPTY is available on Windows 10 only (released after 2018).
  4. Windows conPTY does not support the Asynchronous I/O, so to get the terminal output, need to add the read function after every command execution. (something like select() is not available)
  5. Also found some performance issues with psql on windows.
    1. To read the output from the terminal need to add some sleep time as it will take time to return the output. 
    2. Resize the terminal is also not consistent and causing the issue if we resize the window faster or multiple times very quickly. 
    3. Loading large dataset sometimes cause system to non-responsive state.(In this state restart requires)
Please find the patch for disable the psql tool for windows platform.(Windows builds are falling due to this sending patch for disable psql on windows.)

Reference  links:

If any suggestions or questions please let me know.

Regards,
Nikhil Mohite.

On Tue, May 25, 2021 at 8:20 PM Akshay Joshi <[hidden email]> wrote:
Thanks, patch applied.

I have updated the screenshot and some documentation stuff.

On Tue, May 25, 2021 at 3:08 PM Nikhil Mohite <[hidden email]> wrote:
Hi Akshay,


Please find the updated patch. (V6)
On Tue, May 25, 2021 at 2:55 PM Akshay Joshi <[hidden email]> wrote:
Hi Nikhil

Please rebase and send the patch again.

On Tue, May 25, 2021 at 2:52 PM Nikhil Mohite <[hidden email]> wrote:
Hi Akshay/ Team

On Mon, May 24, 2021 at 9:19 PM Akshay Joshi <[hidden email]> wrote:
Hi Nikhil 

Following are the review comments:
  • Set "ENABLE_PSQL = False", PSQL button from browser tree and context menu option should not be visible.
  • Documentation screenshot should be in standard theme for consistency, and check the size it's very large. Take the screenshot with the new PSQL button on the browser tree.
  • Update 'menu_bar.rst' and 'toolbar.rst' with new changes.
  • Remove commented code (if any)
  • Check SonarQube (I haven't run)
Please find the updated patch, resolve all the review comments, and update the code to resolve the SonarQube issues.

On Thu, May 20, 2021 at 2:52 PM Dave Page <[hidden email]> wrote:
Thanks Nikhil. Can someone else review this version please?

On Wed, May 19, 2021 at 2:42 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/ Team,

On Wed, May 19, 2021 at 1:43 PM Dave Page <[hidden email]> wrote:
Hi

On Wed, May 19, 2021 at 8:58 AM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/ Team,

On Tue, May 18, 2021 at 8:41 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/Team,


On Mon, May 17, 2021 at 6:47 PM Dave Page <[hidden email]> wrote:
Hi

On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <[hidden email]> 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.

Hmm, yes - in particular, colours for the different themes. Please move them into the css for the themes. You have a mix of style, layout and code in this file which needs to be cleaned up.
xterm V3 onwards they have provided the API to set the theme and other settings, earlier I tried with CSS to override the theme but couldn’t able to apply the theme properly as some style get applied as in-line style for the HTML, so used the API to set the theme.

OK, but either way we can't hard-code styles from themes in HTML templates for individual features; that way leads to madness.

Perhaps Aditya or one of the other team members can give some assistance?
I have moved the color settings to the respective theme files. Aditya helped in this.
 


Speaking of themes, the background colour for selected text doesn't seem right (it's barely visible) in the dark theme. Can you fix that to match the colouring in the SQL text boxes please? 
I tried the default selection color from SQL for the dark and standard themes but still, it was not readable so just updated the color code with another color as follows.
1. Dark Theme:
Screenshot 2021-05-19 at 6.29.43 PM.png
2. High Contrast: (using default SQL selection color)
Screenshot 2021-05-19 at 6.59.52 PM.png
3. Standard:
image.png
can we go with the colors or should we update it?
 

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:

It looks like that can be fixed by adding:

env={'TERM': 'xterm'}
Added this in the environment when opening the psql panel.

to the subprocess.Popen() call.

I noticed while I was playing with that, that you are passing the password as part of the connection string. As I've mentioned in the past, that is absolutely not acceptable; it will expose the password to all manner of tools (such as ps -ef). You *must* pass the password to psql using the PGPASSWORD environment variable.
 
if we try to load data from the table containing millions of records, UI gets very slow.
Removed the password from the connection string and added 'PGPASSWORD' in the environment. 

Is xtermjs discarding the older buffer contents when it fills up? Can you tell where the memory usage is?
I checked the psql memory consumption in terminal and pgAdmin psql tool memory consumption is the similar. Also tested the performance and query execution timing is also  similar.

OK, so there's probably not much we can do here.
 

 

- 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.  

In addition to the issue above, it looks like the \! blocking may have lost it's ability to ignore quoted strings:

pgweb=# select '\!';
ERROR: Shell commands are disabled in psql for security 
Fixed the issue now it is consistent with the psql terminal. 
 
 


On Tue, May 11, 2021 at 3:40 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--

Regards,
Nikhil Mohite 


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Regards,
Nikhil Mohite 


--

Regards,
Nikhil Mohite 


--


Regards,
Nikhil Mohite


--
Thanks & Regards,
Nikhil Mohite
Software Engineer.
Mob.No: +91-7798364578.


--
Regards,
Nikhil Mohite 


--


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246


disable_psql_for_windows.patch (36K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Dave Page-7
Hi

On Tue, Jun 1, 2021 at 12:58 PM Nikhil Mohite <[hidden email]> wrote:
Hi Team,

Following are few points related to PSQL tool on windows:
  1.  Currently using the pywinpty library on windows to create pty process and execute the  psql.exe.
  2. To read the stderr (errors) currently using '2>>&1' arguments to psql.exe command. (It will redirect stderr to stdout )
  3. Windows conPTY is available on Windows 10 only (released after 2018).
  4. Windows conPTY does not support the Asynchronous I/O, so to get the terminal output, need to add the read function after every command execution. (something like select() is not available)
  5. Also found some performance issues with psql on windows.
    1. To read the output from the terminal need to add some sleep time as it will take time to return the output. 
    2. Resize the terminal is also not consistent and causing the issue if we resize the window faster or multiple times very quickly. 
    3. Loading large dataset sometimes cause system to non-responsive state.(In this state restart requires)
Please find the patch for disable the psql tool for windows platform.(Windows builds are falling due to this sending patch for disable psql on windows.)

Disabling major features on our most common deployment platform really isn't a good option. I assume all of the above options are related to lack of async I/O?

Have you tried forcing the use of winpty rather than conpty?
 

On Tue, May 25, 2021 at 8:20 PM Akshay Joshi <[hidden email]> wrote:
Thanks, patch applied.

I have updated the screenshot and some documentation stuff.

On Tue, May 25, 2021 at 3:08 PM Nikhil Mohite <[hidden email]> wrote:
Hi Akshay,


Please find the updated patch. (V6)
On Tue, May 25, 2021 at 2:55 PM Akshay Joshi <[hidden email]> wrote:
Hi Nikhil

Please rebase and send the patch again.

On Tue, May 25, 2021 at 2:52 PM Nikhil Mohite <[hidden email]> wrote:
Hi Akshay/ Team

On Mon, May 24, 2021 at 9:19 PM Akshay Joshi <[hidden email]> wrote:
Hi Nikhil 

Following are the review comments:
  • Set "ENABLE_PSQL = False", PSQL button from browser tree and context menu option should not be visible.
  • Documentation screenshot should be in standard theme for consistency, and check the size it's very large. Take the screenshot with the new PSQL button on the browser tree.
  • Update 'menu_bar.rst' and 'toolbar.rst' with new changes.
  • Remove commented code (if any)
  • Check SonarQube (I haven't run)
Please find the updated patch, resolve all the review comments, and update the code to resolve the SonarQube issues.

On Thu, May 20, 2021 at 2:52 PM Dave Page <[hidden email]> wrote:
Thanks Nikhil. Can someone else review this version please?

On Wed, May 19, 2021 at 2:42 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/ Team,

On Wed, May 19, 2021 at 1:43 PM Dave Page <[hidden email]> wrote:
Hi

On Wed, May 19, 2021 at 8:58 AM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/ Team,

On Tue, May 18, 2021 at 8:41 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 18, 2021 at 12:12 PM Nikhil Mohite <[hidden email]> wrote:
Hi Dave/Team,


On Mon, May 17, 2021 at 6:47 PM Dave Page <[hidden email]> wrote:
Hi

On Mon, May 17, 2021 at 11:01 AM Nikhil Mohite <[hidden email]> 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.

Hmm, yes - in particular, colours for the different themes. Please move them into the css for the themes. You have a mix of style, layout and code in this file which needs to be cleaned up.
xterm V3 onwards they have provided the API to set the theme and other settings, earlier I tried with CSS to override the theme but couldn’t able to apply the theme properly as some style get applied as in-line style for the HTML, so used the API to set the theme.

OK, but either way we can't hard-code styles from themes in HTML templates for individual features; that way leads to madness.

Perhaps Aditya or one of the other team members can give some assistance?
I have moved the color settings to the respective theme files. Aditya helped in this.
 


Speaking of themes, the background colour for selected text doesn't seem right (it's barely visible) in the dark theme. Can you fix that to match the colouring in the SQL text boxes please? 
I tried the default selection color from SQL for the dark and standard themes but still, it was not readable so just updated the color code with another color as follows.
1. Dark Theme:
Screenshot 2021-05-19 at 6.29.43 PM.png
2. High Contrast: (using default SQL selection color)
Screenshot 2021-05-19 at 6.59.52 PM.png
3. Standard:
image.png
can we go with the colors or should we update it?
 

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:

It looks like that can be fixed by adding:

env={'TERM': 'xterm'}
Added this in the environment when opening the psql panel.

to the subprocess.Popen() call.

I noticed while I was playing with that, that you are passing the password as part of the connection string. As I've mentioned in the past, that is absolutely not acceptable; it will expose the password to all manner of tools (such as ps -ef). You *must* pass the password to psql using the PGPASSWORD environment variable.
 
if we try to load data from the table containing millions of records, UI gets very slow.
Removed the password from the connection string and added 'PGPASSWORD' in the environment. 

Is xtermjs discarding the older buffer contents when it fills up? Can you tell where the memory usage is?
I checked the psql memory consumption in terminal and pgAdmin psql tool memory consumption is the similar. Also tested the performance and query execution timing is also  similar.

OK, so there's probably not much we can do here.
 

 

- 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.  

In addition to the issue above, it looks like the \! blocking may have lost it's ability to ignore quoted strings:

pgweb=# select '\!';
ERROR: Shell commands are disabled in psql for security 
Fixed the issue now it is consistent with the psql terminal. 
 
 


On Tue, May 11, 2021 at 3:40 PM Dave Page <[hidden email]> wrote:
Hi

On Tue, May 11, 2021 at 9:02 AM Akshay Joshi <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
Hi

On Mon, May 10, 2021 at 1:45 PM Nikhil Mohite <[hidden email]> wrote:
Hi Hackers,

Please find the attached patch for RM-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
 
--

Regards,
Nikhil Mohite 


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Regards,
Nikhil Mohite 


--

Regards,
Nikhil Mohite 


--


Regards,
Nikhil Mohite


--
Thanks & Regards,
Nikhil Mohite
Software Engineer.
Mob.No: +91-7798364578.


--
Regards,
Nikhil Mohite 


--


--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
Thanks & Regards
Akshay Joshi
pgAdmin Hacker | Principal Software Architect
EDB Postgres
Mobile: +91 976-788-8246



--
123