Repository navigation
chore: incorporate plgpsql_check in test - #63
AndrewJackson2020 wants to merge 2 commits into
Conversation
0ae486a to
72d4fe4
Compare
|
@AndrewJackson2020 Could you elaborate on why is this needed? I'm not understanding the motivation just by looking at the code. |
Apologies, spun this one up late last night, added a description. |
|
Converting to draft as I don't think this should be considered for merging until we do a release on xpg (requires postgres minor version to be up to date) |
|
Also worth pointing out that I have a similar PR for |
|
Setting to ready for review as we now have an official release of xpg with the postgres versions that we need. |
3077b92 to
9cf21d9
Compare
| -- below query based on an example from from pgsql_check's git repo, link below | ||
| -- https://github.com/okbob/plpgsql_check/blob/0ac96adc13b34d071c4be8ba0ec4f73e6e7520f5/README.md?plain=1#L310 | ||
| -- check all plpgsql functions (functions or trigger functions with defined triggers) | ||
| SELECT CAST(((pcf)).functionid AS regprocedure) | ||
| , ((pcf)).lineno | ||
| , ((pcf)).statement | ||
| , ((pcf)).sqlstate | ||
| , ((pcf)).message | ||
| , ((pcf)).detail | ||
| , ((pcf)).hint | ||
| , ((pcf)).level | ||
| , ((pcf)).position | ||
| , ((pcf)).query | ||
| , ((pcf)).context | ||
| FROM (SELECT plpgsql_check_function_tb(pg_proc.oid | ||
| , COALESCE(pg_trigger.tgrelid, 0) | ||
| , oldtable => pg_trigger.tgoldtable | ||
| , newtable => pg_trigger.tgnewtable) AS pcf | ||
| FROM pg_proc | ||
| LEFT JOIN pg_trigger ON pg_trigger.tgfoid = pg_proc.oid | ||
| INNER JOIN pg_catalog.pg_depend ON pg_proc.oid = pg_depend.objid | ||
| INNER JOIN pg_catalog.pg_extension ON pg_depend.refobjid = pg_extension.oid | ||
|
|
||
| WHERE prolang = (SELECT lang.oid | ||
| FROM pg_language AS lang | ||
| WHERE lang.lanname = 'plpgsql') | ||
| AND extname = 'supabase_vault' | ||
| AND pronamespace <> (SELECT nsp.oid | ||
| FROM pg_namespace AS nsp | ||
| WHERE nsp.nspname = 'pg_catalog') | ||
| AND (pg_proc.prorettype <> (SELECT typ.oid | ||
| FROM pg_type AS typ | ||
| WHERE typ.typname = 'trigger') | ||
| OR pg_trigger.tgfoid IS NOT NULL) | ||
| OFFSET 0) AS ss | ||
| ORDER BY CAST(CAST(((pcf)).functionid AS regprocedure) AS text) |
There was a problem hiding this comment.
QQ: This code should be the same as the one on https://github.com/supabase/pg_net/pull/318/changes#diff-d173be2386c671ed8f25c284768dc42c796920e0714f39d888fe04013c292059R1-R36?
Thinking if this could be done by default via xpg somehow.
There was a problem hiding this comment.
There is a bit of a difference between the 2. pgsql_check flags a ton of issues from pgtap so I had to explicitly filter out anything that wasn't owner by supabase vault. They can be made the same now since we merged the full conversion to pg_regress.
I definitely think it can/should be in xpg somehow. If all we did was make pgsql_check a default extension in xpg then that alone would cut out a lot of boilerplate with these PR's. Idk how I would feel about a separate xpg pgsql_check command that gets run in CI or whatever. Its nice having it in pg_regress/pytest because u can grandfather in known issues that could be hard to change (like unused parameters).
There was a problem hiding this comment.
I have reverted the above sql in favor of what exists the in plpgsql_check README.md. We don't have pgtap anymore so no need to deviate from the vendor provided SQL at this point.
3e7a22f to
fe5d1f1
Compare
plpgsql_check is a linting tool for plpgsql code. Wanted to see if it could catch the issue that @hanefi brought up in #49 . It was not able to flag that but it did find 3 unused variables. This PR changes 1 of them, the other 2 are unused parameters and would thus be a breaking change that I don't thinks we should commit to in this PR. Also the maintainers seem to be game to add checks and whatnot so perhaps it could be used to catch the issue that @hanefi brought up without writing a custom test in every repo. Also it appears to be a supported third party extension for the supabase platform. Also I know that we have discussed using postgres-language-server for stuff like this but it appears they have integration with plpgsql_check.
I see this as less dangerous than auto formatting SQL code and we can always choose to ignore it's recommendations (see the changes I made in the .out file for how I ignored the 2 unused function parameters).
https://github.com/okbob/plpgsql_check
https://supabase.com/docs/guides/database/extensions/plpgsql_check?queryGroups=database-method&database-method=sql
https://pg-language-server.com/latest/features/plpgsql/