On 2025-Jun-25, Michael Paquier wrote:
> On Tue, Jun 24, 2025 at 07:45:15PM +0200, Alvaro Herrera wrote:
> > + /*
> > +* If we have an external param at this location, but no lists are
> > +* being squashed across the query, then we skip here; this will
> > make
> > +
Hello
I spent a much longer time staring at this patch than I wanted to, and
at a point I almost wanted to boot the whole thing to pg19, but because
upthread we already had an agreement that we should get it in for this
cycle, I decided that the best course of action was to just move forward
with
On Tue, Jun 24, 2025 at 07:45:15PM +0200, Alvaro Herrera wrote:
> + /*
> +* If we have an external param at this location, but no lists are
> +* being squashed across the query, then we skip here; this will make
> +* us print print the characters found in the original
On Thu, Jun 12, 2025 at 11:32 AM Álvaro Herrera wrote:
>
> Hello,
>
> I have pushed that now,
thanks!
> and here's a rebase of patch 0003 to add support
> for PARAM_EXTERN. I'm not really sure about this one yet ...
see v11. I added a missing test to show how external param
normalization behav
Hello,
I have pushed that now, and here's a rebase of patch 0003 to add support
for PARAM_EXTERN. I'm not really sure about this one yet ...
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"La victoria es para quien se atreve a estar solo"
>From 0a836189a2e3af3b
On Tue, Jun 10, 2025 at 07:25:27PM +0200, Alvaro Herrera wrote:
> On 2025-Jun-10, Michael Paquier wrote:
>> I think that this is can be reproduced by
>> -DWRITE_READ_PARSE_PLAN_TREES -DCOPY_PARSE_PLAN_TREES
>> -DRAW_EXPRESSION_COVERAGE_TEST that I always include in my builds.
>> The freebsd task us
On 2025-Jun-10, Michael Paquier wrote:
> I think that this is can be reproduced by
> -DWRITE_READ_PARSE_PLAN_TREES -DCOPY_PARSE_PLAN_TREES
> -DRAW_EXPRESSION_COVERAGE_TEST that I always include in my builds.
> The freebsd task uses the same with debug_copy_parse_plan_trees=on,
> debug_write_read_p
On Mon, Jun 09, 2025 at 12:44:59PM +0200, Alvaro Herrera wrote:
> I also added a recursive call in IsSquashableExpression to itself. The
> check for stack depth can be done without throwing an error. I tested
> this by adding stack bloat in that function. I also renamed it to
> IsSquashableConst
> I've spent a bunch of time looking at this series and here's my take on
> the second one.
Thanks!
> I realized that the whole in_expr production in gram.y is pointless, and
> the whole private struct that was added was unnecessary. A much simpler
> solution is to remove in_expr, expand its use
Hello,
I've spent a bunch of time looking at this series and here's my take on
the second one. (The testing patch is unchanged from Sami's). The
third patch (for PARAM_EXTERNs) should be a mostly trivial rebase on top
of these two.
I realized that the whole in_expr production in gram.y is point
I realized this thread did not have a CF entry,
so here it is https://commitfest.postgresql.org/patch/5801/
--
Sami
> The test additions done in v7-0002 look sensible here.
>
> --- In the following two queries the operator expressions (+) and (@) have
> --- different oppno, and will be given different query_id if squashed, even
> though
> --- the normalized query will be the same
>
> In v7-0002, this comment is
On Thu, May 29, 2025 at 11:30:12AM +0900, Michael Paquier wrote:
> I still need to review the rest of the patch series..
The test additions done in v7-0002 look sensible here.
--- In the following two queries the operator expressions (+) and (@) have
--- different oppno, and will be given differe
On Wed, May 28, 2025 at 04:05:03PM -0500, Sami Imseih wrote:
> That's my mistake. I added a new file called normalize.sql to test
> specific normalization scenarios. Added in v7
Thanks. I was not sure that a new file was worth having for these
tests, knowing that select.sql has similar coverage.
>> * 0001:
> You have mentioned the addition of tests, but v6-0001 includes nothing
> of the kind. Am I missing something? How much coverage did you
> intend to add here? These seem to be included in squashing.sql in
> patch v6-0002, but IMO this should be moved somewhere else to work
> with th
On Tue, May 27, 2025 at 05:05:39PM -0500, Sami Imseih wrote:
> * 0001:
>
> This is a normalization issue discovered when adding new
> tests for squashing. This is also an issue that exists in
> v17 and likely earlier versions and should probably be
> backpatched.
>
> The crux of the problem is if
> I've spent a bit of time looking at this, and I want to
> propose the following patchset.
Sorry about this, but I missed to add a comment in one of the
test cases for 0004 that describes the behavior of parameters
and constants that live outside of the squashed list.
The following 2 cases will
> > therefore, a user supplied query like this:
> > ```
> > select where $5 in ($1, $2, $3) and $6 = $4 and 1 = 2
> > ```
> >
> > will be normalized to:
> > ```
> > select where $1 in ($2 /*...*/) and $3 = $4 and $5 = $6
> > ```
>
> Hmm, interesting.
>
> I think this renumbering should not be a pro
On 2025-May-24, Sami Imseih wrote:
> therefore, a user supplied query like this:
> ```
> select where $5 in ($1, $2, $3) and $6 = $4 and 1 = 2
> ```
>
> will be normalized to:
> ```
> select where $1 in ($2 /*...*/) and $3 = $4 and $5 = $6
> ```
Hmm, interesting.
I think this renumbering should
On Sat, May 24, 2025 at 09:35:24AM -0500, Sami Imseih wrote:
> 2. If we have 1 or more squashed lists, then we can't guarantee
> the $n parameter as was supplied by the user and we simply rename
> the $n starting from 1.
>
> therefore, a user supplied query like this:
> ```
> select where $5 in ($
> In v17, we are a bit smarter with the numbering, with a normalization
> giving the following, starting at $1:
> select where $5 in ($1, $2, $3) and $6 = $4
>
> So your argument about the $n parameters is kind of true, but I think
> the numbering logic in v17 to start at $1 is a less-confusing res
On Fri, May 23, 2025 at 08:05:47PM -0500, Sami Imseih wrote:
> Since we assign new parameter symbols based on the highest external param
> from the original query, as stated in the docs [0] "The parameter
> symbols used to replace
> constants in representative query texts start from the next number
> On Fri, May 23, 2025 at 04:29:45PM +0200, Dmitry Dolgov wrote:
> > I think it's better to recursively call IsSquashableConst on the nested
> > expression (arg or args for FuncExpr). Something like that was done in
> > the original patch version and was concidered too much at that time, but
> > si
On Fri, May 23, 2025 at 04:29:45PM +0200, Dmitry Dolgov wrote:
> I think it's better to recursively call IsSquashableConst on the nested
> expression (arg or args for FuncExpr). Something like that was done in
> the original patch version and was concidered too much at that time, but
> since it loo
> On Fri, May 23, 2025 at 09:05:54AM GMT, Sami Imseih wrote:
> > > On Thu, May 22, 2025 at 10:23:31PM GMT, Sami Imseih wrote:
> > > > This does not get squashed:
> > > > Q: select where 2 in (1, 4) and
> > > > 1 in (5, cast(7 as int), 6, (cast(8 as int)), 9, 10, (cast(8 as
> > > > text))::int);
>
> > On Thu, May 22, 2025 at 10:23:31PM GMT, Sami Imseih wrote:
> > > This does not get squashed:
> > > Q: select where 2 in (1, 4) and
> > > 1 in (5, cast(7 as int), 6, (cast(8 as int)), 9, 10, (cast(8 as
> > > text))::int);
> > > R: select where $1 in ($2 /*, ... */) and
> > > $3 in ($4, cast($5
> On Thu, May 22, 2025 at 10:23:31PM GMT, Sami Imseih wrote:
> > This does not get squashed:
> > Q: select where 2 in (1, 4) and
> > 1 in (5, cast(7 as int), 6, (cast(8 as int)), 9, 10, (cast(8 as
> > text))::int);
> > R: select where $1 in ($2 /*, ... */) and
> > $3 in ($4, cast($5 as int), $6, (
> For example with bind queries like that:
> select where $1 in ($3, $2) and 1 in ($4, cast($5 as int))
> \bind 0 1 2 3 4
>
> Should we have a bit more coverage, where we use multiple IN and/or
> ARRAY lists with constants and/or external parameters?
I will add more test coverage. All the tests we
On Thu, May 22, 2025 at 03:10:34PM -0500, Sami Imseih wrote:
> > IMO adding a struct as suggested is okay, especially if it reduces the
> > overall code complexity. But we don't want a node, just a bare struct.
> > Adding a node would be more troublesome.
>
> In v4, a new private struct is added
> IMO adding a struct as suggested is okay, especially if it reduces the
> overall code complexity. But we don't want a node, just a bare struct.
> Adding a node would be more troublesome.
In v4, a new private struct is added in gram.y, but we are also adding
additional fields to track the expres
On 2025-May-22, Dmitry Dolgov wrote:
> Just to call this out, I don't think there is an agreement on squashing
> Params, which you have added into 0002.
Actually I think we do have agreement on squashing PARAM_EXTERN Params.
https://postgr.es/m/3086744.1746500...@sss.pgh.pa.us
> Now, both flavou
> On Wed, May 21, 2025 at 08:22:19PM GMT, Sami Imseih wrote:
> > > > At the same time AFAICT there isn't much more code paths
> > > > to worry about in case of a LocationExpr as a node
> > >
> > > I can imagine there are others like value expressions,
> > > row expressions, json array expressions,
> > > At the same time AFAICT there isn't much more code paths
> > > to worry about in case of a LocationExpr as a node
> >
> > I can imagine there are others like value expressions,
> > row expressions, json array expressions, etc. that we may
> > want to also normalize.
> Exactly. When using a n
> On Tue, May 20, 2025 at 04:50:12PM GMT, Sami Imseih wrote:
> > At the same time AFAICT there isn't much more code paths
> > to worry about in case of a LocationExpr as a node
>
> I can imagine there are others like value expressions,
> row expressions, json array expressions, etc. that we may
> w
> At the same time AFAICT there isn't much more code paths
> to worry about in case of a LocationExpr as a node
I can imagine there are others like value expressions,
row expressions, json array expressions, etc. that we may
want to also normalize.
> I believe it's worth to not only to keep amoun
> On Tue, May 20, 2025 at 06:30:25AM GMT, Michael Paquier wrote:
> On Mon, May 12, 2025 at 06:40:43PM -0400, Sami Imseih wrote:
> > Also, LocationExpr is not really an expression node, but a wrapper to
> > an expression node, so I think it's wrong to define it as a Node and be
> > required to add t
> On Tue, May 20, 2025 at 11:03:52AM GMT, Dmitry Dolgov wrote:
> > By the way, the new test cases for ARRAY lists are sent in the last
> > patch of the series posted on this thread:
> > https://www.postgresql.org/message-id/7zbzwk4btnxoo4o3xbtzefoqvht54cszjj4bol22fmej5nmgkf@dbcn4wtakw4y
> >
> > The
On Mon, May 12, 2025 at 06:40:43PM -0400, Sami Imseih wrote:
> Also, LocationExpr is not really an expression node, but a wrapper to
> an expression node, so I think it's wrong to define it as a Node and be
> required to add the necessary handling for it in nodeFuncs.c. I think we
> can just define
> On Mon, May 12, 2025 at 06:40:43PM GMT, Sami Imseih wrote:
> > The static variables was only part of the concern, another part was
> > using A_Expr to carry this information, which will have impact on lots
> > of unrelated code.
>
> What would be the problem if A_Expr carries an extra pointer to
> > On Fri, May 09, 2025 at 12:47:19PM GMT, Sami Imseih wrote:
> > So, I think we can create a new parse node ( parsenode.h ) that will only be
> > used in parsing (and gram.c only ) to track the start/end locations
> > and List and
> > based on this node we can create A_ArrayExpr and A_Expr with t
> On Fri, May 09, 2025 at 12:47:19PM GMT, Sami Imseih wrote:
> So, I think we can create a new parse node ( parsenode.h ) that will only be
> used in parsing (and gram.c only ) to track the start/end locations
> and List and
> based on this node we can create A_ArrayExpr and A_Expr with the List
>
> On Fri, May 09, 2025 at 10:12:24AM GMT, Dmitry Dolgov wrote:
> Agree, I'll try to extend number of test cases here as a separate patch.
Here is the extended version, where start/end is replaced by
location/length, array_expr is handled as well, and more ARRAY cases are
added.
>From 81fe0b08473ea
> > To clarify, I had in mind something like in the attached patch. The
> > idea is to make start/end location capturing relatively independent from
> > the constants squashing. The new parsing node conveys the location
> > information, which is then getting transformed to be a part of an
> > Array
> On Fri, May 09, 2025 at 08:47:58AM GMT, Michael Paquier wrote:
> SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
> - query| calls
> -+---
> - SELECT ARRAY[$1 /*, ... */]
> On Fri, May 09, 2025 at 02:35:33PM GMT, Michael Paquier wrote:
> On Fri, May 09, 2025 at 11:05:43AM +0800, Junwang Zhao wrote:
> > Why not a location and a length, it should be more natural, it
> > seems we use this convention in some existing nodes, like
> > RawStmt, InsertStmt etc.
>
> These ar
On Fri, May 9, 2025 at 1:35 PM Michael Paquier wrote:
>
> On Fri, May 09, 2025 at 11:05:43AM +0800, Junwang Zhao wrote:
> > Why not a location and a length, it should be more natural, it
> > seems we use this convention in some existing nodes, like
> > RawStmt, InsertStmt etc.
>
> These are new co
On Fri, May 09, 2025 at 11:05:43AM +0800, Junwang Zhao wrote:
> Why not a location and a length, it should be more natural, it
> seems we use this convention in some existing nodes, like
> RawStmt, InsertStmt etc.
These are new concepts as of Postgres 18 (aka only on HEAD), chosen
mainly to match
Hi Dmitry,
On Fri, May 9, 2025 at 3:36 AM Dmitry Dolgov <9erthali...@gmail.com> wrote:
>
> > On Thu, May 08, 2025 at 02:22:00PM GMT, Michael Paquier wrote:
> > On Wed, May 07, 2025 at 10:41:22AM +0200, Dmitry Dolgov wrote:
> > > Ah, I see what you mean. I think the idea is fine, it will simplify
>
On Thu, May 08, 2025 at 03:50:32PM -0500, Sami Imseih wrote:
> On Thu, May 8, 2025 at 2:36 PM Dmitry Dolgov <9erthali...@gmail.com> wrote:
>> To clarify, I had in mind something like in the attached patch. The
>> idea is to make start/end location capturing relatively independent from
>> the consta
On Thu, May 8, 2025 at 2:36 PM Dmitry Dolgov <9erthali...@gmail.com> wrote:
>
> > On Thu, May 08, 2025 at 02:22:00PM GMT, Michael Paquier wrote:
> > On Wed, May 07, 2025 at 10:41:22AM +0200, Dmitry Dolgov wrote:
> > > Ah, I see what you mean. I think the idea is fine, it will simplify
> > > certain
> On Thu, May 08, 2025 at 02:22:00PM GMT, Michael Paquier wrote:
> On Wed, May 07, 2025 at 10:41:22AM +0200, Dmitry Dolgov wrote:
> > Ah, I see what you mean. I think the idea is fine, it will simplify
> > certain things as well as address the issue. But I'm afraid adding
> > start/end location to
On Wed, May 07, 2025 at 10:41:22AM +0200, Dmitry Dolgov wrote:
> Ah, I see what you mean. I think the idea is fine, it will simplify
> certain things as well as address the issue. But I'm afraid adding
> start/end location to A_Expr is a bit too invasive, as it's being used
> for many other purpose
> On Tue, May 06, 2025 at 03:01:32PM GMT, Sami Imseih wrote:
> > > Without properly accounting for the boundaries of the list of
> > > expressions, i.e.,
> > > the start and end positions of '(' and ')' or '[' and ']' and normalizing
> > > the
> > > expressions in between, it will be very difficu
On Tue, May 06, 2025 at 01:32:48PM -0500, Sami Imseih wrote:
> Without properly accounting for the boundaries of the list of expressions,
> i.e.,
> the start and end positions of '(' and ')' or '[' and ']' and normalizing the
> expressions in between, it will be very difficult for the normalizatio
> > Without properly accounting for the boundaries of the list of expressions,
> > i.e.,
> > the start and end positions of '(' and ')' or '[' and ']' and normalizing
> > the
> > expressions in between, it will be very difficult for the normalization to
> > behave sanely.
>
> I don't think having
> On Tue, May 06, 2025 at 01:32:48PM GMT, Sami Imseih wrote:
> > I also agree with Alvaro that this discussion doesn't justify a
> > revert. If the pre-v18 behavior wasn't chiseled on stone tablets,
> > the new behavior isn't either. We can improve it some more later.
>
> As I was looking further
> I also agree with Alvaro that this discussion doesn't justify a
> revert. If the pre-v18 behavior wasn't chiseled on stone tablets,
> the new behavior isn't either. We can improve it some more later.
As I was looking further into what we currently have in v18 and HEAD
the normalization could b
> On Tue, May 06, 2025 at 11:50:07PM GMT, Junwang Zhao wrote:
> Would it make sense to rename `RecordConstLocation` to something like
> `RecordExpressionLocation` instead?
Yeah, naming is hard. RecordExpressionLocation is somehow more vague,
but I see what you mean, maybe something along these lin
Hi Dmitry,
On Sun, May 4, 2025 at 6:19 PM Dmitry Dolgov <9erthali...@gmail.com> wrote:
>
> > On Thu, May 01, 2025 at 09:55:47PM GMT, Dmitry Dolgov wrote:
> > > On Thu, May 01, 2025 at 09:29:13AM GMT, Michael Paquier wrote:
> > >
> > > I agree that the current solution we have in the tree feels inc
Michael Paquier writes:
> On Thu, May 01, 2025 at 03:57:16PM -0500, Sami Imseih wrote:
>> I think what we should really allow the broader scope of expressions that
>> are allowed via prepared statements, and this will make this implementation
>> consistent between prepared vs non-prepared statemen
On Fri, 2 May 2025 14:56:56 +0200
Álvaro Herrera wrote:
> On 2025-May-02, Michael Paquier wrote:
>
> > That depends. If we conclude that tracking this information through
> > the parser based on the start and end positions in a query string
> > for a set of values is more relevant, then we woul
> On Thu, May 01, 2025 at 09:55:47PM GMT, Dmitry Dolgov wrote:
> > On Thu, May 01, 2025 at 09:29:13AM GMT, Michael Paquier wrote:
> >
> > I agree that the current solution we have in the tree feels incomplete
> > because we are not taking into account the most common cases that
> > users would care
On 2025-May-02, Michael Paquier wrote:
> That depends. If we conclude that tracking this information through
> the parser based on the start and end positions in a query string
> for a set of values is more relevant, then we would be redesigning the
> facility from the ground, so the old approach
> On Fri, May 02, 2025 at 04:18:37PM GMT, Michael Paquier wrote:
> On Fri, May 02, 2025 at 09:13:39AM +0200, Dmitry Dolgov wrote:
> > Squashing constants was ment to be a first step towards doing the same
> > for other types of queries (params, rte_values), reverting it to
> > implement everything
On Fri, May 02, 2025 at 09:13:39AM +0200, Dmitry Dolgov wrote:
> Squashing constants was ment to be a first step towards doing the same
> for other types of queries (params, rte_values), reverting it to
> implement everything at once makes very little sense to me.
That depends. If we conclude tha
> On Fri, May 02, 2025 at 07:10:19AM GMT, Michael Paquier wrote:
> > I am really leaning towards that we should revert this feature as the
> > limitation we have now with parameters is a rather large one and I think
> > we need to go back and address this issue.
>
> I am wondering if this would not
On Thu, May 01, 2025 at 03:57:16PM -0500, Sami Imseih wrote:
> I think what we should really allow the broader scope of expressions that
> are allowed via prepared statements, and this will make this implementation
> consistent between prepared vs non-prepared statements. I don't see why
> not. In
I spent a few hours looking into this today and to your points below:
> > I agree that the current solution we have in the tree feels incomplete
> > because we are not taking into account the most common cases that
> > users would care about. Now, allowing PARAM_EXTERN means that we
> > allow any
> On Thu, May 01, 2025 at 09:29:13AM GMT, Michael Paquier wrote:
>
> I agree that the current solution we have in the tree feels incomplete
> because we are not taking into account the most common cases that
> users would care about. Now, allowing PARAM_EXTERN means that we
> allow any expression
On Wed, Apr 30, 2025 at 04:52:06PM -0500, Sami Imseih wrote:
> 62d712ec added the ability to squash constants from an IN LIST
> for queryId computation purposes. This means that a similar
> queryId will be generated for the same queries that only
> different on the number of values in the IN-LIST.
62d712ec added the ability to squash constants from an IN LIST
for queryId computation purposes. This means that a similar
queryId will be generated for the same queries that only
different on the number of values in the IN-LIST.
The patch lacks the ability to apply this optimization to values
pas
71 matches
Mail list logo
