move sql queries to external file

[dkuku]

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

• I've added this contribution to the changelog.rst.
• 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]

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[j-bennet]

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:

pgspecial/pgspecial/dbcommands.py

Lines 143 to 222 in ae0bbbf

	def list_privileges(cur, pattern, verbose):
	"""Returns (title, rows, headers, status)"""
	sql = SQL(
	"""
	SELECT n.nspname as "Schema",
	c.relname as "Name",
	CASE c.relkind WHEN 'r' THEN 'table'
	WHEN 'v' THEN 'view'
	WHEN 'm' THEN 'materialized view'
	WHEN 'S' THEN 'sequence'
	WHEN 'f' THEN 'foreign table'
	WHEN 'p' THEN 'partitioned table' END as "Type",
	pg_catalog.array_to_string(c.relacl, E'\n') AS "Access privileges",
	pg_catalog.array_to_string(ARRAY(
	SELECT attname || E':\n ' || pg_catalog.array_to_string(attacl, E'\n ')
	FROM pg_catalog.pg_attribute a
	WHERE attrelid = c.oid AND NOT attisdropped AND attacl IS NOT NULL
	), E'\n') AS "Column privileges",
	pg_catalog.array_to_string(ARRAY(
	SELECT polname
	|| CASE WHEN NOT polpermissive THEN
	E' (RESTRICTIVE)'
	ELSE '' END
	|| CASE WHEN polcmd != '*' THEN
	E' (' || polcmd::pg_catalog.text || E'):'
	ELSE E':'
	END
	|| CASE WHEN polqual IS NOT NULL THEN
	E'\n (u): ' || pg_catalog.pg_get_expr(polqual, polrelid)
	ELSE E''
	END
	|| CASE WHEN polwithcheck IS NOT NULL THEN
	E'\n (c): ' || pg_catalog.pg_get_expr(polwithcheck, polrelid)
	ELSE E''
	END || CASE WHEN polroles <> '{0}' THEN
	E'\n to: ' || pg_catalog.array_to_string(
	ARRAY(
	SELECT rolname
	FROM pg_catalog.pg_roles
	WHERE oid = ANY (polroles)
	ORDER BY 1
	), E', ')
	ELSE E''
	END
	FROM pg_catalog.pg_policy pol
	WHERE polrelid = c.oid), E'\n')
	AS "Policies"
	FROM pg_catalog.pg_class c
	LEFT JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace
	"""
	)
	if pattern:
	schema, table = sql_name_pattern(pattern)
	if table:
	pattern = SQL(
	" AND c.relname OPERATOR(pg_catalog.~) {} COLLATE pg_catalog.default "
	).format(table)
	if schema:
	pattern += SQL(
	" AND n.nspname OPERATOR(pg_catalog.~) {} COLLATE pg_catalog.default "
	).format(schema)
	else:
	pattern = SQL(" AND pg_catalog.pg_table_is_visible(c.oid) ")
	where_clause = SQL(
	"""
	WHERE c.relkind IN ('r','v','m','S','f','p')
	{pattern}
	AND n.nspname !~ '^pg_'
	"""
	).format(pattern=pattern)
	sql += where_clause + SQL(" ORDER BY 1, 2 ")
	log.debug(sql.as_string(cur))
	cur.execute(sql)
	if cur.description:
	headers = [x.name for x in cur.description]
	return [(None, cur, headers, cur.statusmessage)]

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

pgspecial/pgspecial/dbcommands.py

Line 525 in ae0bbbf

def list_functions(cur, pattern, verbose):

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

[dkuku]

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]

@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

pgspecial/pgspecial/dbcommands.py

Line 997 in ae0bbbf

{suffix},

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]

@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]

@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]

@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]

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 <#147 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAG4X44I7H23BWIU2NZXY63YDQT7PAVCNFSM6AAAAAA65MQQPSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBTGAYDGMZSGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[dkuku]

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]

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]

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]

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]

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]

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