powa-team / pg_qualstats

A PostgreSQL extension for collecting statistics about predicates, helping find what indices are missing
Other
274 stars 26 forks source link

fix constant_position for prepared statement #12

Closed dblugeon closed 3 years ago

dblugeon commented 7 years ago

Hello Dalibo Team. This PR permit to store the constant position for variables in prepared Statement in this pgqsEntry store.

To reproduce the problem :

create a table with 2 or more columns. create a program which use a prepared statement (I used a java program) for an select query.

select * from pg_qualstat()

with previous version : you can see null or -X for constant_position. If you rexecute the test program, the constant_position decrement.

with the pr version you will have constant_position

The constant_position will permit to have an explain plan for prepared statement in powa-web with the good order for variable.

Regards

rjuju commented 7 years ago

Hello,

You're absolutely right, the position is lost for a parameter (which can happen for a prepared statement, but also when using extended protocol).

However, I think your fix isn't right, since it always (not only when the position is -1) overrides the const position with the var position, the var being the column used in the OpExpr).

Unfortunately, I don't think this is something that can be fixed in pg_qualstats. The right fix would be to teach postgres to give to the Const position the original parameter position. Following patch seems to do it:

diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index b19380e1b1..848c29da57 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -2444,6 +2444,7 @@ eval_const_expressions_mutator(Node *node,
                            int16       typLen;
                            bool        typByVal;
                            Datum       pval;
+                           Const      *c;

                            Assert(prm->ptype == param->paramtype);
                            get_typlenbyval(param->paramtype,
@@ -2452,13 +2453,16 @@ eval_const_expressions_mutator(Node *node,
                                pval = prm->value;
                            else
                                pval = datumCopy(prm->value, typByVal, typLen);
-                           return (Node *) makeConst(param->paramtype,
+                           c = makeConst(param->paramtype,
                                                      param->paramtypmod,
                                                      param->paramcollid,
                                                      (int) typLen,
                                                      pval,
                                                      prm->isnull,
                                                      typByVal);
+                           c->location = param->location;
+
+                           return (Node *) c;
                        }
                    }
                }

I'll propose something similar on postgres mailing list, I don't know if it'll be accepted or backported.

rjuju commented 7 years ago

Hello again!

Sorry for the late answer. I did proposed the patch on -hackers (see https://www.postgresql.org/message-id/20170311220932.GJ15188@nol.local), but it was unfortunately ignored. I guess you could try to answer it if you want to attract some interest, but I have no idea if that'd be eventually accepted.

rjuju commented 3 years ago

I have a good news here. I faced the same problem recently and bumped the topic again and the patch was finally merged upstream, see https://github.com/postgres/postgres/commit/be850f1822e4b54d1d570eefa8a7242788011634.

This will however be part of psotgres 15, so it unfortunately won't be usable before ~ september 2022.

dblugeon commented 3 years ago

Hello, Thank you for this. :)