Date: Tue, 5 Jan 2021 13:52:30 +0100
From: Dmitry Dolgov <9erthalion6@gmail.com>
To: Zhihong Yu <zyu@yugabyte.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>
Subject: Re: pg_stat_statements and "IN" conditions
Message-ID: <20210105125230.etgmgixnn6fmpszl@localhost>
References: 
 <CA+q6zcWtUbT_Sxj0V6HY6EZ89uv5wuG5aefpe_9n0Jr3VwntFg@mail.gmail.com>
 <20201118160432.hyko73sqc2bf7nkj@localhost>
 <20201226104635.uvzkftzu72zg5qux@localhost>
 <CALNJ-vQx7u-Nq_wVun72CVrHe4DcGfEurj_ikckJrRy2Pem=JA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="oyjisgzd2vntjkkc"
Content-Disposition: inline
In-Reply-To: 
 <CALNJ-vQx7u-Nq_wVun72CVrHe4DcGfEurj_ikckJrRy2Pem=JA@mail.gmail.com>
Precedence: bulk


--oyjisgzd2vntjkkc
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

> 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?

--oyjisgzd2vntjkkc
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v3-0001-Prevent-jumbling-of-every-element-in-ArrayExpr.patch"