Horton

Re: pg_stat_statements and "IN" conditions

Zhihong Yu <zyu@yugabyte.com>

View thread
From: Zhihong Yu <zyu@yugabyte.com>
To: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>, Tom Lane <tgl@sss.pgh.pa.us>, Greg Stark <stark@mit.edu>, Pavel Trukhanov <pavel.trukhanov@gmail.com>
Date: 2021-01-05T15:51:42Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Introduce squashing of constant lists in query jumbling

  2. Make documentation builds reproducible

  3. Include values of A_Const nodes in query jumbling

  4. Teach planner about more monotonic window functions

  5. Split up guc.c for better build speed and ease of maintenance.

Hi, Dmitry:

+   int         lastExprLenght = 0;

Did you mean to name the variable lastExprLenghth ?

w.r.t. extracting to helper method, the second and third if (currentExprIdx
== pgss_merge_threshold - 1) blocks are similar.
It is up to you whether to create the helper method.
I am fine with the current formation.

Cheers

On Tue, Jan 5, 2021 at 4:51 AM Dmitry Dolgov <9erthalion6@gmail.com> wrote:

> > On Sat, Dec 26, 2020 at 08:53:28AM -0800, Zhihong Yu wrote:
> > Hi,
> > A few comments.
> >
> > +               foreach(lc, (List *) expr)
> > +               {
> > +                   Node * subExpr = (Node *) lfirst(lc);
> > +
> > +                   if (!IsA(subExpr, Const))
> > +                   {
> > +                       allConst = false;
> > +                       break;
> > +                   }
> > +               }
> >
> > It seems the above foreach loop (within foreach(temp, (List *) node)) can
> > be preceded with a check that allConst is true. Otherwise the loop can be
> > skipped.
>
> Thanks for noticing. Now that I look at it closer I think it's the other
> way around, the loop above checking constants for the first expression
> is not really necessary.
>
> > +                   if (currentExprIdx == pgss_merge_threshold - 1)
> > +                   {
> > +                       JumbleExpr(jstate, expr);
> > +
> > +                       /*
> > +                        * A const expr is already found, so JumbleExpr
> must
> > +                        * record it. Mark it as merged, it will be the
> > first
> > +                        * merged but still present in the statement
> query.
> > +                        */
> > +                       Assert(jstate->clocations_count > 0);
> > +                       jstate->clocations[jstate->clocations_count -
> > 1].merged = true;
> > +                       currentExprIdx++;
> > +                   }
> >
> > The above snippet occurs a few times. Maybe extract into a helper method.
>
> Originally I was hesitant to extract it was because it's quite small
> part of the code. But now I've realized that the part relevant to lists
> is not really correct, which makes those bits even more different, so I
> think it makes sense to leave it like that. What do you think?
>