Suggestion: PG::Connection#complile

[intale]

Introduce PG::Connection#complile that take sql string and position parameters and compiles it into a plain sql. I couldn't find another way how to do it using built-in tools. The motivation behind this implementation:

• debugging sql queries. When you have queries with dozens(or even hundreds) of positional parameters - it is pretty cancer to replace each positional param by hands for later inspection
• such feature as PostreSQL view does not support positional parameters and requires pre-computed sql string. So, imaging you have some sql builder in your app that results in sql with positional params. And now you need to work with PostgreSQL view which does not support them. You will end up implementing very similar to the changes in this PR or somehow compute an sql string based on your app's logic. Both variants would be annoying.

The downside of this implementation is that it can't reliably encode binary data into a string representation except some common cases like with booleans, but do we need them at all?

[larskanis]

That sounds like a great idea! It's because I manually inserted too many parameters into too many queries from rails debug log in order to execute it in psql. That way a more useful debug output in rails would be possible and that would be my intention.

[larskanis]

I added a commit to use generic PG::TypeMap methods only and regarding binary data and binary format.

The method name "compile" is not very specific. Is it used in any other database driver? How sounds embed_params instead?

[intale]

@larskanis thank you. Could you please explain how your changes apply to PG::TypeMapAllStrings as well? Because it has its own interface, all static, not exposed to public and PG::TypeMapAllStrings the default class for type mapping. Nvm, I'm blind. pg_tmas_type is inherited from pg_typemap_type and you added changes exactly into pg_typemap_type. I mixed it with PG::TypeMapByClass. Lgtm

[intale]

The method name "compile" is not very specific. Is it used in any other database driver? How sounds embed_params instead?

I don't have any strong preferences regarding this. embed_params works as well

[larskanis]

🎉 With the latest change the whole activerecord test suite runs through with only 2 expected errors, when I change the following AR lines to got through embed_params and sending the resulting SQL text without binds:
https://github.com/rails/rails/blob/09bf0878ec2a604f4f1430c8757db23d16925f14/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb#L162-L184

[intale]

@larskanis thank you for your effort and covering cases I was not aware of.

[cbandy]

IME, these kinds of not-quite-parsers are vulnerable to SQL injection.

Perhaps a stronger or more specific statement about how this should/not be used with user input.

[intale]

I agree. If there is any mistake in regexp - the result may end up containing sql injection. For example

embed_params("select '$1' as one, $1", [%{', 2 as two, }])

may result in

[{"?column?" => "', 2 as two, ", "two" => "2", "one" => ""}]

instead this

[{"one" => "$1", "?column?" => "', 2 as two, "}]

if we forget to cover regular string literal(e.g. 'my str') in PLACEHOLDER_RE. Thus, as long as we cover all possible variations of string literals and escape the value - we are safe. Fortunately I covered all possible scenarios in tests. As an additional measure - I can put here a guard that fails on new postgresql versions and a dev should check if any new string literals appear in a new postgresql version and adjust the implementation accordingly. Something like this(in #embed_params_and_check):

# Query returns a string like "18.4 (Ubuntu 18.4-0ubuntu0.26.04.1)"
current_pg_version = conn.exec('SHOW server_version').first['server_version'].split(' ').first
current_pg_version = Gem::Version.new(current_pg_version)
expect(current_pg_version).to be < Gem::Version.new('19'), "PostgreSQL 19 is not tested. Please ensure there are no new forms of a string literal and increase the version to make this test pass."

What do you think, guys?