Repository navigation
Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Since #863, statements in SQL function bodies are only checked by the static typecheck rules, but the Postgres regression harness never analysed them, so a false positive in a body could slip through. This PR makes the harness (and the rule spec tests) analyse function bodies the way the workspace does.
Harness
For every
CREATE FUNCTION ... LANGUAGE sqlwith a string body (AS '...'orAS $$...$$), the body is analysed right after the function, with the function's parameters in scope, by name and as$n. A finding in the body of a function Postgres accepted fails the test, like any other finding on an accepted statement. Findings in bodies are marked(body)in the per-version snapshots, and the header now counts the analysed bodies (347 on 15, 359 on 18).To keep the harness and the workspace in sync, the extraction moved out of
pgls_workspace:pgls_query_ext::utils::sql_function_bodyfinds the body text and its range (moved fromget_sql_fn_body, unchanged).pgls_catalog::resolve::FunctionContext::from_create_functionbuilds the parameters the body sees. It replaces the workspace'sget_sql_fn_signature→function_contextconversion and the copy in thefunctionReturnTypeMismatchcheck.OUT/RETURNS TABLEcolumns are still left out.The legacy
SQLFunctionSignatureis gone:TypecheckDiagnosticsMapperstill returned it, but nothing has read it since #863.False positives found
Across 15–18, one statement:
It is listed in
KNOWN_FALSE_POSITIVESwith its root cause, and shows up under "Known false positives" in the snapshots. The test fails if an entry stops matching, so fixes have to remove their entries. The fix is stacked on this PR.The new body findings on statements Postgres rejected are all correct:
select_into.sql(a table the function creates),window.sql(tenk1, which a fresh database doesn't have) andcompression.sql(fipshash, fromtest_setup).Not covered
Only bodies the workspace extracts today are analysed:
BEGIN ATOMICandRETURNbodies (14–18 accepted functions per version) aren't extracted at all.'...'bodies with doubled quotes (4 per version) can't be found in the source text.#918 and #941 (on the #888 stack) extract
BEGIN ATOMIC,RETURN,''andE'...'bodies. They rewrite the sameget_sql_fn_body, which this PR moves topgls_query_ext, so whichever lands second has to move their extraction behind the shared helper. The harness and the specs then pick up those bodies as well.Tests:
postgres_regresspasses, the rule specs (rules_tests) also analyse bodies now and none of their snapshots changed, there's a new unit test forFunctionContext::from_create_function(moved from the workspace's signature tests), and a workspace test checks that a body statement gets its function's parameters.