Repository navigation
Fix greedy comment regex and deallocate-all blast radius (stacked on #14) - #17
Conversation
Two issues found during review of the function, both silent and both
real:
1. The block-comment regex ('\/\*.+\*\/') is greedy across every /* */
pair in the query, not just one. A query with two separate block
comments had everything between the first "/*" and the last "*/"
deleted - including real code in between - with no error raised.
Fixed with a non-greedy quantifier.
2. The two bare "deallocate all;" calls wipe every prepared statement in
the calling session, not just index_advisor's own internal one. In a
pooled or persistent connection (PostgREST, pgbouncer session mode)
this silently breaks whatever else that connection had prepared.
Fixed by checking pg_prepared_statements for the specific name before
deallocating it - DEALLOCATE has no IF EXISTS clause, so this needs an
explicit existence check rather than DROP-style IF EXISTS syntax.
Edits the same 0.2.0--0.2.1.sql script from #14 rather than bumping to
0.2.2 - 0.2.1 hasn't shipped anywhere yet, so there's no reason for two
version bumps when both PRs land in the same release.
|
Closing and reopening to retry GitHub's stack detection. |
| regexp_replace( | ||
| regexp_replace( | ||
| regexp_replace(query,'\/\*.+\*\/', '', 'g'), | ||
| regexp_replace(query,'\/\*.+?\*\/', '', 'g'), |
There was a problem hiding this comment.
Regex based parsing of SQL code is bound to break in other unexpected ways as well. E.g. what happens if a /* and */ pair exists inside a string? That shouldn't be stripped, but the current regex will strip it. Ideally we should parse the SQL properly for such code transformations, but that's a bigger change, and ok for now to keep as is.
There was a problem hiding this comment.
Yeah agreed, this is a real gap. Proper SQL parsing would be the correct fix. Proceeding with merging this one now and will handle it in a follow-up PR.
| deallocate all; | ||
| if exists (select 1 from pg_prepared_statements where name = prepared_statement_name) then | ||
| execute format('deallocate %I', prepared_statement_name); | ||
| end if; |
There was a problem hiding this comment.
Is there a failure mode here when two calls to the index_advisor function are made at the same time? Will they conflict with each other and corrupt each other's responses? If yes, worth making the prepare_statement_name value unique for each call.
There was a problem hiding this comment.
@imor Locally verified it. Prepared statements are bachend local in postgres . So two different sessions can't collide on the same name . I tested it directly: ran two concurrent sessions both using index_advisor_working_statement, one sleeping mid-transaction while the other prepared/executed/deallocated its own version of the name, and the sleeping session's value came back untouched when it resumed. The only way this could actually conflict is if a single session called index_advisor() reentrantly. But I think we are covered there as a backend runs one statement at a time.
imor
left a comment
imor
left a comment
There was a problem hiding this comment.
Left a couple of comments, ok to merge for now, but we should consider those changes going forward.
Stacked on #14
Targets
utkarashsingh/psqlext-1773-schema-qualify-hypopg-calls-v2(#14) - merge #13 and #14 first.Two issues found reviewing the function while working on PSQLEXT-1773, both silent (no error raised) and both verified before and after the fix:
1. Greedy block-comment regex deletes code between separate comments
The comment-stripping regex (
'\/\*.+\*\/') is greedy across every/* */pair in the query, not just the nearest one. A query with two separate block comments has everything between the first/*and the last*/deleted - including real code in between.Fixed with a non-greedy quantifier (
\/\*.+?\*\/).2.
deallocate allwipes every prepared statement in the sessionThe two bare
deallocate all;calls clear every prepared statement in the calling session, not justindex_advisor's own internal one. In a pooled or persistent connection (PostgREST, pgbouncer session mode) this silently breaks whatever else that connection had prepared.Verified: preparing an unrelated statement, then calling
index_advisor(), leftpg_prepared_statementswith 0 rows afterward.Tracked in https://linear.app/supabase/issue/PSQLEXT-1773