Re: [HACKERS] Cached plans and statement generalization - Mailing list pgsql-hackers

On Fri, Apr 28, 2017 at 6:01 AM, Konstantin Knizhnik <k.knizhnik@postgrespro.ru> wrote:

Any comments and suggestions for future improvement of this patch are welcome.

+        PG_TRY();
+        {
+            query = parse_analyze_varparams(parse_tree,
+                                            query_string,
+                                            &param_types,
+                                            &num_params);
+        }
+        PG_CATCH();
+        {
+            /*
+             * In case of analyze errors revert back to original query processing
+             * and disable autoprepare for this query to avoid such problems in future.
+             */
+            FlushErrorState();
+            if (snapshot_set) {
+                PopActiveSnapshot();
+            }
+            entry->disable_autoprepare = true;
+            undo_query_plan_changes(parse_tree, const_param_list);
+            MemoryContextSwitchTo(old_context);
+            return false;
+        }
+        PG_END_TRY();

This is definitely not a safe way of using TRY/CATCH.

+
+            /* Convert literal value to parameter value */
+            switch (const_param->literal->val.type)
+            {
+              /*
+               * Convert from integer literal
+               */
+              case T_Integer:
+                switch (ptype) {
+                  case INT8OID:
+                    params->params[paramno].value = Int64GetDatum((int64)const_param->literal->val.val.ival);
+                    break;
+                  case INT4OID:
+                    params->params[paramno].value = Int32GetDatum((int32)const_param->literal->val.val.ival);
+                    break;
+                  case INT2OID:
+                    if (const_param->literal->val.val.ival < SHRT_MIN
+                        || const_param->literal->val.val.ival > SHRT_MAX)
+                    {
+                        ereport(ERROR,
+                                (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+                                 errmsg("smallint out of range")));
+                    }
+                    params->params[paramno].value = Int16GetDatum((int16)const_param->literal->val.val.ival);
+                    break;
+                  case FLOAT4OID:
+                    params->params[paramno].value = Float4GetDatum((float)const_param->literal->val.val.ival);
+                    break;
+                  case FLOAT8OID:
+                    params->params[paramno].value = Float8GetDatum((double)const_param->literal->val.val.ival);
+                    break;
+                  case INT4RANGEOID:
+                    sprintf(buf, "[%ld,%ld]", const_param->literal->val.val.ival, const_param->literal->val.val.ival);
+                    getTypeInputInfo(ptype, &typinput, &typioparam);
+                    params->params[paramno].value = OidInputFunctionCall(typinput, buf, typioparam, -1);
+                    break;
+                  default:
+                    pg_lltoa(const_param->literal->val.val.ival, buf);
+                    getTypeInputInfo(ptype, &typinput, &typioparam);
+                    params->params[paramno].value = OidInputFunctionCall(typinput, buf, typioparam, -1);
+                }
+                break;
+              case T_Null:
+                params->params[paramno].isnull = true;
+                break;
+              default:
+                /*
+                 * Convert from string literal
+                 */
+                getTypeInputInfo(ptype, &typinput, &typioparam);
+                params->params[paramno].value = OidInputFunctionCall(typinput, const_param->literal->val.val.str, typioparam, -1);
+            }

I don't see something with a bunch of hard-coded rules for particular type OIDs having any chance of being acceptable.

Well, what I need is to convert literal value represented in Value struct to parameter datum value.
Struct "value" contains union with integer literal and text.
So this peace of code is just provides efficient handling of most common cases (integer parameters) and uses type's input function in other cases.

This patch seems to duplicate a large amount of existing code.  That would be a good thing to avoid.

Yes,  I have to copy a lot of code from exec_parse_message + exec_bind_message + exec_execute_message functions.
Definitely copying of code is bad flaw. It will be much better and easier just to call three original functions instead of mixing gathering their code into the new function.
But I failed to do it because
1.  Autoprepare should be integrated into exec_simple_query. Before executing query in normal way, I need to perform cache lookup for previously prepared plan for this generalized query.
And generalization of query requires building of query tree (query parsing). In other words, parsing should be done before I can call exec_parse_message.
2. exec_bind_message works with parameters passed by client though libpwq protocol, while autoprepare deals with values of parameters extracted from literals.
3. I do not want to generate dummy name for autoprepared query to handle it as normal prepared statement.
And I can not use unnamed statements because I want lifetime of autoprepared statements will be larger than one statement.
4. I have to use slightly different memory context policy than named or unnamed prepared statements.

Also this three exec_* functions contain prolog/epilog code which is needed because them are serving separate libpq requests.
But in case of autoprepared statements them need to be executed in the context of single libpq message, so most of this code is redundant.

It could use a visit from the style police and a spell-checker, too.

I will definitely fix style and misspelling - I have not submitted yet this patch to commit fest and there is long enough time to next commitfest.
My primary intention of publishing this patch is receive feedback on the proposed approach.
I already got two very useful advices: limit number of cached statements and pay more attention to safety.
This is why I have reimplemented my original approach with substituting string literals with parameters without building parse tree.

Right now I am mostly thinking about two problems:
1. Finding out best criteria of detecting literals which need to be replaced with parameters and which not. It is clear that replacing "limit 10" with "limit $10"
will have not so much sense and can cause worse execution plan. So right now I just ignore sort, group by and limit parts. But may be it is possible to find some more flexible approach.
2. Which type to chose for parameters. I can try to infer type from context (current solution), or try to use type of literal.
The problem with first approach is that query compiler is not always able to do it and even if type can be determined, it may be too generic (for example numeric instead of real
or range instead of integer). The problem with second approach is opposite: type inferred from literal type can be too restrictive - quite often integer literals are used to specify values of floating point constant. The best solution is first try to determine parameter type from context and then refine it based on literal type. But  it will require repeat of query analysis.
Not sure if it is possible.

--

Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Thanks for your feedback.

-- 
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company