move sql queries to external file

dkuku commented 1 year ago

Description

This is a proof of concept to have the sql code in external sql file and load it using aiosql. The benefits are:

easy code formatting
syntax highlight
easy copy and paste
easy formatting of the sql - you can just run a standard formatter (the queries are pretty complicated and every sql formatter I tried is failing somewhere - probably it's a space to created a new better one 🤣 )
no need to use logger to view the generated sql that is sent to database
easy portability of the code - I wanted to use it in my project that is written in elixir but it's a bit of a nightmare to extract the code. This way I can share the file. Let me know your opinion on this change and if I should continue this. It adds a dependency but I think this is worth for the clarity of code.

Checklist

[x] I've added this contribution to the changelog.rst.
[x] I installed pre-commit hooks (pip install pre-commit && pre-commit install), and ran black on my code.
[ ] Please squash merge this pull request (uncheck if you'd like us to merge as multiple commits)

codecov-commenter commented 1 year ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

:exclamation: No coverage uploaded for pull request base (main@50ab8a1). Click here to learn what that means.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #147 +/- ## ======================================= Coverage ? 64.21% ======================================= Files ? 6 Lines ? 978 Branches ? 0 ======================================= Hits ? 628 Misses ? 350 Partials ? 0 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

j-bennet commented 1 year ago

Hey @dkuku, thank you for the PR! I like the idea. However, implementation may appear to be a bit more complicated that this POC. You only handle variations of pattern and verbose in list_databases, so you have to store 2 versions of the SQL query. However, it already becomes less simple in list_privileges, where pattern may contain schema, or schema and table:

https://github.com/dbcli/pgspecial/blob/ae0bbbf9c2efba9c9421dde110964e3707c58bc1/pgspecial/dbcommands.py#L143-L222

and then with list_functions, we have branching on Postgres version:

https://github.com/dbcli/pgspecial/blob/ae0bbbf9c2efba9c9421dde110964e3707c58bc1/pgspecial/dbcommands.py#L525

I would still say, take a stab at it and see what happens. We're always open to PRs.

dkuku commented 1 year ago

I know about the multiple versions stuff. The only problem with this for me is testing it - I need to do it with older postgres to be sure I did not break anything. And with verbose - would it be ok to have only the verbose query and just drop the verbose columns in python when it's not verbose. This would reduce the amount of permutations.

Anyway - pr updated with the easier ones. I did some refactors to the filtering that allow only having a single query - have a look and lmk what you think.

    WHERE
    CASE WHEN :nspname != '.*'
    THEN n.nspname ~ :nspname
    ELSE pg_catalog.pg_table_is_visible(c.oid)
    END
    AND c.relname OPERATOR(pg_catalog.~) :relname

Not all functions from the sql file are used yet - I did it already some time ago but now I'm backporting it here

dkuku commented 1 year ago

@j-bennet I managed to port it. I ran test on postgres 10,12,14,15 in docker - on 9.5 the tests fail because the test setup is not compatible. On 15 I have a test failing about the database owner name test_slash_dn - it's not due to my changes afaik. But I have some questions: Where is the suffix https://github.com/dbcli/pgspecial/blob/ae0bbbf9c2efba9c9421dde110964e3707c58bc1/pgspecial/dbcommands.py#L997 used ? This is casted to the table_info tuple defined at the top of the file but it seems that the order of the columns is wrong - the suffix (reloptions??) is c.relhasoids, {suffix}, c.reltablespace, 0 AS reloftype, but in the tuple definition it's "hasoids", "tablespace", "reloptions", "reloftype"

j-bennet commented 1 year ago

@dkuku I can't say for sure where the suffix is used, but those queries were reverse-engineered running psql with -E. That way, it shows you queries that it runs on meta-commands:

postgres=> \l
********* QUERY **********
SELECT d.datname as "Name",
       pg_catalog.pg_get_userbyid(d.datdba) as "Owner",
       pg_catalog.pg_encoding_to_char(d.encoding) as "Encoding",
       d.datcollate as "Collate",
       d.datctype as "Ctype",
       pg_catalog.array_to_string(d.datacl, E'\n') AS "Access privileges"
FROM pg_catalog.pg_database d
ORDER BY 1;
**************************

See if that helps.

dkuku commented 1 year ago

@dkuku I can't say for sure where the suffix is used, but those queries were reverse-engineered running psql with -E. That way, it shows you queries that it runs on meta-commands:

Thanks - I will play with that a bit. I understand it may not be used at all ? I could not find it in the code.

j-bennet commented 1 year ago

@dkuku

I understand it may not be used at all ? I could not find it in the code.

I think you're right, the positioning of the query fields vs tuple fields looks off:

# query:
1 - c.relchecks,
2 - c.relkind,
3 - c.relhasindex,
4 - c.relhasrules,
5 - c.relhastriggers,
6 - {relhasoids},
7 - {suffix},
8 - c.reltablespace,
9 - c.reloftype,
10 - c.relpersistence,
11 - c.relispartition

# tuple:
1 - "checks",
2 - "relkind",
3 - "hasindex",
4 - "hasrules",
5 - "hastriggers",
6 - "hasoids",
7 - "tablespace",
8 - "reloptions",
9 - "reloftype",
10 - "relpersistence",
11 - "relispartition",

Suffix should probaly be reloptions, but it maps to tablespace. So reloptions and tablespace are switched. This is likely a bug, and we just don't have a test case for it.

dkuku commented 1 year ago

Adding more tests will be needed first before merging this. I also think that it may be hidden behind a flag so if I messed something up then rolling back to existing logic is possible.

On Thu, 9 Nov 2023, 01:05 Irina Truong, @.***> wrote:

@dkuku https://github.com/dkuku

I understand it may not be used at all ? I could not find it in the code.

I think you're right, the positioning of the query fields vs tuple fields looks off:

query:

1 - c.relchecks, 2 - c.relkind, 3 - c.relhasindex, 4 - c.relhasrules, 5 - c.relhastriggers, 6 - {relhasoids}, 7 - {suffix}, 8 - c.reltablespace, 9 - c.reloftype, 10 - c.relpersistence, 11 - c.relispartition

tuple:

1 - "checks", 2 - "relkind", 3 - "hasindex", 4 - "hasrules", 5 - "hastriggers", 6 - "hasoids", 7 - "tablespace", 8 - "reloptions", 9 - "reloftype", 10 - "relpersistence", 11 - "relispartition",

Suffix should really be reloptions, but it maps to tablespace. So reloptions and tablespace are switched. This is probably a bug, and we just don't have a test case for it.

— Reply to this email directly, view it on GitHub https://github.com/dbcli/pgspecial/pull/147#issuecomment-1803003323, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG4X44I7H23BWIU2NZXY63YDQT7PAVCNFSM6AAAAAA65MQQPSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBTGAYDGMZSGM . You are receiving this because you were mentioned.Message ID: @.***>

dkuku commented 12 months ago

I swapped the suffix position in the other pr. Here I extracted the queries and left only the ones that are verbose. Not needed fields are later dropped in python. I consider this pr as done.

j-bennet commented 11 months ago

Hey @dkuku , thanks for your work. I didn't forget about this PR, but I need a good chunk of time to review it. It's a lot.

In the meantime, did you try installing pgspecial from your branch, and running pgcli test suite with it? Did you catch any errors?

dkuku commented 11 months ago

Hey @dkuku , thanks for your work. I didn't forget about this PR, but I need a good chunk of time to review it. It's a lot.

In the meantime, did you try installing pgspecial from your branch, and running pgcli test suite with it? Did you catch any errors?

@j-bennet I updated the package to include the sql file. I ran the tests with pgspecial installed from this branch and it seems to work unless I messed something up and it ran the old version I removed first the package to be sure:

 cd ../pgcli                                                           ✔    ▼  python-3.11  
direnv: loading ~/Projects/pgcli/.envrc
direnv: export +VIRTUAL_ENV ~PATH
    ~/Projects/pgcli    main !1 ?2  rm -rf .direnv/python-3.11/lib/python3.11/site-packages/pgspecial*                    ✔    ▼  python-3.11  
    ~/Projects/pgcli    main !1 ?2  pip install git+https://github.com/dkuku/pgspecial.git@move_sql_to_external_file      ✔    ▼  python-3.11  
Collecting git+https://github.com/dkuku/pgspecial.git@move_sql_to_external_file
  Cloning https://github.com/dkuku/pgspecial.git (to revision move_sql_to_external_file) to /tmp/pip-req-build-vjaadqgy
  Running command git clone --filter=blob:none --quiet https://github.com/dkuku/pgspecial.git /tmp/pip-req-build-vjaadqgy
  Running command git checkout -b move_sql_to_external_file --track origin/move_sql_to_external_file
  Switched to a new branch 'move_sql_to_external_file'
  branch 'move_sql_to_external_file' set up to track 'origin/move_sql_to_external_file'.
  Resolved https://github.com/dkuku/pgspecial.git to commit 3aa28f24083d527166f321493c0244c2713531bd
  Installing build dependencies ... done
  Getting requirements to build wheel ... done
  Installing backend dependencies ... done
  Preparing metadata (pyproject.toml) ... done
Requirement already satisfied: click>=4.1 in ./.direnv/python-3.11/lib/python3.11/site-packages (from pgspecial==2.1.1) (8.1.7)
Requirement already satisfied: sqlparse>=0.1.19 in ./.direnv/python-3.11/lib/python3.11/site-packages (from pgspecial==2.1.1) (0.4.4)
Requirement already satisfied: psycopg>=3.0.10 in ./.direnv/python-3.11/lib/python3.11/site-packages (from pgspecial==2.1.1) (3.1.13)
Requirement already satisfied: aiosql>=9.0 in ./.direnv/python-3.11/lib/python3.11/site-packages (from pgspecial==2.1.1) (9.0)
Requirement already satisfied: typing-extensions>=4.1 in ./.direnv/python-3.11/lib/python3.11/site-packages (from psycopg>=3.0.10->pgspecial==2.1.1) (4.8.0)
Building wheels for collected packages: pgspecial
  Building wheel for pgspecial (pyproject.toml) ... done
  Created wheel for pgspecial: filename=pgspecial-2.1.1-py3-none-any.whl size=36251 sha256=45b8eb9b6b5844c769bd473fa30f6264be993caed17417316a67f8b88981ac19
  Stored in directory: /tmp/pip-ephem-wheel-cache-ytisuoid/wheels/a9/5c/ab/b01218ec3045371585c0c8459cac2d7f9a3d62c0ee616e634d
Successfully built pgspecial
Installing collected packages: pgspecial
Successfully installed pgspecial-2.1.1

[notice] A new release of pip is available: 23.2.1 -> 23.3.1
[notice] To update, run: pip install --upgrade pip
    ~/Pr/pgcli    main !1 ?2  py.test -s                                                                           ✔  4s     ▼  python-3.11  
==================================================================== test session starts ====================================================================
platform linux -- Python 3.11.5, pytest-7.4.3, pluggy-1.3.0
rootdir: /home/kuku/Projects/pgcli
collected 2636 items                                                                                                                                        

tests/test_auth.py ...Load your password from keyring returned:
Boom!
To remove this message do one of the following:
- prepare keyring as described at: https://keyring.readthedocs.io/en/stable/
- uninstall keyring: pip uninstall keyring
- disable keyring in our configuration: add keyring = False to [main]
..Set password in keyring returned:
Boom!
To remove this message do one of the following:
- prepare keyring as described at: https://keyring.readthedocs.io/en/stable/
- uninstall keyring: pip uninstall keyring
- disable keyring in our configuration: add keyring = False to [main]
.
tests/test_completion_refresher.py ....
tests/test_config.py ......
tests/test_fuzzy_completion.py ......
tests/test_main.py ...............................Time: 0.006s
Time: 0.002s
Waiting for 10 seconds before repeating
Time: 0.002s
Waiting for 10 seconds before repeating
Time: 0.002s
Waiting for 4 seconds before repeating
Time: 0.002s
Waiting for 4 seconds before repeating
Time: 0.002s
Waiting for 4 seconds before repeating
Time: 0.002s
Waiting for 5 seconds before repeating
Time: 0.002s
Waiting for 5 seconds before repeating
Time: 0.002s
Waiting for 5 seconds before repeating
Time: 0.002s
Waiting for 5 seconds before repeating
........
tests/test_naive_completion.py .......
tests/test_pgcompleter.py ............
tests/test_pgexecute.py ................/tmp/pytest-of-kuku/pytest-9/test_execute_from_commented_fi0/rcfile
.['No help available for "/*comment4 */"\nTry \\h with no arguments to see available help.']
........................................................................................
tests/test_pgspecial.py ...........
tests/test_prioritization.py .
tests/test_prompt_utils.py ..
tests/test_rowlimit.py ...
tests/test_smart_completion_multiple_schemata.py .................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
tests/test_smart_completion_public_schema_only.py ..................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
tests/test_sqlcompletion.py ................................................................................x...........................................................................................
tests/test_ssh_tunnel.py ...
tests/formatter/test_sqlformatter.py ....['UPDATE "user" SET', '  "name" = \'Jackson\'', ', "email" = \'jackson_test@gmail.com\'', ', "phone" = \'132454789\'', ', "description" = \'\'', ', "created_at" = \'2022-09-09 19:44:32.712343+08\'', ', "updated_at" = \'2022-09-09 19:44:32.712343+08\'', 'WHERE "id" = \'1\';']
.
tests/parseutils/test_ctes.py ..............
tests/parseutils/test_function_metadata.py .
tests/parseutils/test_parseutils.py ....................X.......................................................................

======================================================== 2634 passed, 1 xfailed, 1 xpassed in 11.30s ========================================================

dkuku commented 11 months ago

I'm currently comparing the output of my changes, old version and psql and I see there are some differences between our and psql - I will try to align this before we can merge it. For ex du in psql has 3 or 5 columns with a nice name, we display a lot more. I will change this. dt also is missing some data in pgcli. I will post an update when it's done

j-bennet commented 11 months ago

I'm currently comparing the output of my changes, old version and psql and I see there are some differences between our and psql - I will try to align this before we can merge it. For ex du in psql has 3 or 5 columns with a nice name, we display a lot more. I will change this. dt also is missing some data in pgcli. I will post an update when it's done

@dkuku Let's limit this PR to externalizing queries only, and preserve the same functionality that pgcli /pgspecial had before. Aligning pgcli and psql can be a follow-up, and we can discuss if it needs to be aligned.

dkuku commented 11 months ago

@j-bennet Ok, I reverted the last pr. It's ready for review.

dbcli / pgspecial