Subject: Re: [HACKERS] Cached plans and statement generalization
To: Heikki Linnakangas <hlinnaka@iki.fi>
Cc: Thomas Munro <thomas.munro@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>
References: 
 <CA+TgmoZAHc4wqL3HvVmTd87s=bobp_8_7R9H=B_kH6q4GcA=Yg@mail.gmail.com>
 <9A6E5062D5D4DB458C80C2B2920BD71D610148@g01jpexmbkw23>
 <CA+q6zcWKsaiFz6oWA1JXdJWNwSSk+wUyuOVO2nryCCSxrr3KRw@mail.gmail.com>
 <EDA4195584F5064680D8130B1CA91C453D0182@G01JPEXMBYT04>
 <6ea1db86-b300-b57c-0f9d-a55dd3966175@postgrespro.ru>
 <9A6E5062D5D4DB458C80C2B2920BD71D6820CB@g01jpexmbkw23>
 <554d0730-f6fc-d1be-fe78-e63973ec621f@postgrespro.ru>
 <35be0423-36bc-cadc-bef8-02d475751f7d@postgrespro.ru>
 <7603a101-6b79-2328-2b4b-3bc3217992e5@postgrespro.ru>
 <CA+hUKGJK2GZewMumkXtPjyATMu2tyEmg53R23WCETY=y48RxQw@mail.gmail.com>
 <f71ca2cb-b2c0-d6ca-4532-822b25f1f81e@postgrespro.ru>
 <CA+hUKGKuxM94eVyBii=0e1bWpa-A05f2L6H-JSij0H5o7t+ZqA@mail.gmail.com>
 <3304c34b-4a9c-909c-4ee1-d8e7f6dcb2de@postgrespro.ru>
 <CA+hUKGL9_ohxMBq04N8qwn-vEaaan2N-06FaXsTz9+seDddN3g@mail.gmail.com>
 <66f47595-1b93-ef67-fdd6-c5414e0f94c3@postgrespro.ru>
 <e9cce21f-942f-31f4-54e4-ace7f2de1a5f@iki.fi>
From: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
Message-ID: <5538dbd6-fcbb-33a7-49d8-28c5cd2df435@postgrespro.ru>
Date: Wed, 31 Jul 2019 22:48:16 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.2
MIME-Version: 1.0
In-Reply-To: <e9cce21f-942f-31f4-54e4-ace7f2de1a5f@iki.fi>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


On 31.07.2019 19:56, Heikki Linnakangas wrote:
> On 09/07/2019 23:59, Konstantin Knizhnik wrote:
>> Fixed patch version of the path is attached.
>
> Much of the patch and the discussion has been around the raw parsing, 
> and guessing which literals are actually parameters that have been 
> inlined into the SQL text. Do we really need to do that, though? The 
> documentation mentions pgbouncer and other connection poolers, where 
> you don't have session state, as a use case for this. But such 
> connection poolers could and should still be using the extended query 
> protocol, with Parse+Bind messages and query parameters, even if they 
> don't use named prepared statements. I'd want to encourage 
> applications and middleware to use out-of-band query parameters, to 
> avoid SQL injection attacks, regardless of whether they use prepared 
> statements or cache query plans. So how about dropping all the raw 
> parse tree stuff, and doing the automatic caching of plans based on 
> the SQL string, somewhere in the exec_parse_message? Check the 
> autoprepare cache in exec_parse_message(), if it was an "unnamed" 
> prepared statement, i.e. if the prepared statement name is an empty 
> string.
>
> (I'm actually not sure if exec_parse/bind_message is the right place 
> for this, but I saw that your current patch did it in 
> exec_simple_query, and exec_parse/bind_message are the equivalent of 
> that for the extended query protocol).

It will significantly simplify this patch and eliminate all complexity 
and troubles  caused by replacing string literals with parameters
if assumption that all client applications are using extended query 
protocol is true.
But I am not sure that we can expect it. At least I myself see many 
applications which are constructing queries by embedding literal values.
May be it is not so good and safe (SQL injection attack), but many 
applications are doing it. And the idea was to improve execution speed
of existed application without changing them.


Also please notice that extended protocol requires passing more message 
which has negative effect on performance.
At my  notebook I get about 21k TPS on "pgbench -S" and 18k TPS on 
"pgbench -M extended -S".
And it is with unix socket connection! I think that in case of remote 
connections difference will be even larger.

So may be committing simple version of this patch which do not need to 
solve any challenged problems is good idea.
But I afraid that it will significantly reduce positive effect of this 
patch.