Horton

Thread

Commits

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

  1. Evaluate arguments of correlated SubPlans in the referencing ExprState

  2. Doc: improve documentation for jsonpath behavior.

  1. Evaluate arguments of correlated SubPlans in the referencing ExprState

    Andres Freund <andres@anarazel.de> — 2023-02-25T21:44:01Z

    Hi,

Around
https://www.postgresql.org/message-id/20230224015417.75yimxbksejpffh3%40awork3.anarazel.de
I suggested that we should evaluate the arguments of correlated SubPlans as
part of the expression referencing the subplan.

Here's a patch for that.

Ended up simpler than I'd thought. I see small, consistent, speedups and
reductions in memory usage.

I think individual arguments are mainly (always?)  Var nodes. By evaluating
them as part of the containing expression we avoid the increased memory usage,
and the increased dispatch of going through another layer of
ExprState. Because the arguments are a single Var, which end up with a
slot_getattr() via ExecJust*Var, we also elide redundant slot_getattr()
checks. I think we already avoided redundant tuple deforming, because the
parent ExprState will have done that already.

Greetings,

Andres Freund

  2. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-03-02T19:33:35Z

    Andres Freund <andres@anarazel.de> writes:
> Around
> https://www.postgresql.org/message-id/20230224015417.75yimxbksejpffh3%40awork3.anarazel.de
> I suggested that we should evaluate the arguments of correlated SubPlans as
> part of the expression referencing the subplan.

> Here's a patch for that.

I looked through this, and there is one point that is making me really
uncomfortable.  This bit is assuming that we can bind the address of
the es_param_exec_vals array right into the compiled expression:

+		ParamExecData *prm = &estate->es_param_exec_vals[paramid];
+
+		ExecInitExprRec(lfirst(pvar), state, &prm->value, &prm->isnull);

Even if that works today, it'd kill the ability to use the compiled
expression across more than one executor instance, which seems like
a pretty high price.  Also, I think it probably fails already in
EvalPlanQual contexts, because EvalPlanQualStart allocates a separate
es_param_exec_vals array for EPQ execution.

I think we'd be better off inventing an EEOP_SET_PARAM_EXEC step type
that is essentially the inverse of EEOP_PARAM_EXEC/ExecEvalParamExec,
and then evaluating each parameter value into the expression's
scratch Datum/isnull fields and emitting SET_PARAM_EXEC to copy those
to the correct ParamExecData slot.

			regards, tom lane

  3. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Andres Freund <andres@anarazel.de> — 2023-03-02T20:05:49Z

    Hi,

On 2023-03-02 14:33:35 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Around
> > https://www.postgresql.org/message-id/20230224015417.75yimxbksejpffh3%40awork3.anarazel.de
> > I suggested that we should evaluate the arguments of correlated SubPlans as
> > part of the expression referencing the subplan.
> 
> > Here's a patch for that.
> 
> I looked through this, and there is one point that is making me really
> uncomfortable.  This bit is assuming that we can bind the address of
> the es_param_exec_vals array right into the compiled expression:
> 
> +		ParamExecData *prm = &estate->es_param_exec_vals[paramid];
> +
> +		ExecInitExprRec(lfirst(pvar), state, &prm->value, &prm->isnull);
> 
> Even if that works today, it'd kill the ability to use the compiled
> expression across more than one executor instance, which seems like
> a pretty high price.  Also, I think it probably fails already in
> EvalPlanQual contexts, because EvalPlanQualStart allocates a separate
> es_param_exec_vals array for EPQ execution.

Yea, I wasn't super comfortable with that either. I concluded it's ok
because we already cache pointers to the array inside each ExprContext.


> I think we'd be better off inventing an EEOP_SET_PARAM_EXEC step type
> that is essentially the inverse of EEOP_PARAM_EXEC/ExecEvalParamExec,
> and then evaluating each parameter value into the expression's
> scratch Datum/isnull fields and emitting SET_PARAM_EXEC to copy those
> to the correct ParamExecData slot.

Agreed, that'd make sense. If we can build the infrastructure to figure
out what param to use, that'd also provide a nice basis for using params
for CaseTest etc.

Greetings,

Andres Freund

  4. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-03-02T20:10:31Z

    Andres Freund <andres@anarazel.de> writes:
> On 2023-03-02 14:33:35 -0500, Tom Lane wrote:
>> I looked through this, and there is one point that is making me really
>> uncomfortable.  This bit is assuming that we can bind the address of
>> the es_param_exec_vals array right into the compiled expression:

> Yea, I wasn't super comfortable with that either. I concluded it's ok
> because we already cache pointers to the array inside each ExprContext.

ExprContext, sure, but compiled expressions?  Considering what it
costs to JIT those, I think we ought to be trying to make them
fairly long-lived.

			regards, tom lane

  5. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Andres Freund <andres@anarazel.de> — 2023-03-02T21:00:31Z

    Hi,

On 2023-03-02 15:10:31 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2023-03-02 14:33:35 -0500, Tom Lane wrote:
> >> I looked through this, and there is one point that is making me really
> >> uncomfortable.  This bit is assuming that we can bind the address of
> >> the es_param_exec_vals array right into the compiled expression:
> 
> > Yea, I wasn't super comfortable with that either. I concluded it's ok
> > because we already cache pointers to the array inside each ExprContext.
> 
> ExprContext, sure, but compiled expressions?  Considering what it
> costs to JIT those, I think we ought to be trying to make them
> fairly long-lived.

I'm not opposed to EXPR_PARAM_SET, to be clear. I'll send an updated
version later. I was just thinking about the correctness in the current
world.

I think it's not just JIT that could benefit, fwiw. I think making
expressions longer lived could also help plpgsql tremendously, for
example.

Greetings,

Andres Freund

  6. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Andres Freund <andres@anarazel.de> — 2023-03-03T00:19:24Z

    Hi,

On 2023-03-02 13:00:31 -0800, Andres Freund wrote:
> I'm not opposed to EXPR_PARAM_SET, to be clear. I'll send an updated
> version later. I was just thinking about the correctness in the current
> world.

Attached.

I named the set EEOP_PARAM_SET EEOP_PARAM_EXEC_SET or such, because I
was wondering if there cases it could also be useful in conjunction with
PARAM_EXTERN, and because nothing really depends on the kind of param.

Greetings,

Andres

  7. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-03-03T20:09:18Z

    Andres Freund <andres@anarazel.de> writes:
> On 2023-03-02 13:00:31 -0800, Andres Freund wrote:
>> I'm not opposed to EXPR_PARAM_SET, to be clear. I'll send an updated
>> version later. I was just thinking about the correctness in the current
>> world.

> Attached.

I've looked through this, and it looks basically OK so I marked it RfC.
I do have a few nitpicks that you might or might not choose to adopt:

It'd be good to have a header comment for ExecInitExprRec documenting
the arguments, particularly that resv/resnull are where to put the
subplan's eventual result.

You could avoid having to assume ExprState's resvalue/resnull being
safe to use by instead using the target resv/resnull.  This would
require putting those into the EEOP_PARAM_SET step so that
ExecEvalParamSet knows where to fetch from, so maybe it's not an
improvement, but perhaps worth considering.

+		/* type isn't needed, but an old value could be confusing */
+		scratch.d.param.paramtype = InvalidOid;
I'd just store the param's type, rather than justifying why you didn't.
It's cheap enough and even less confusing.

I think that ExecEvalParamSet should either set prm->execPlan to NULL,
or maybe better Assert that it is already NULL.

It's a bit weird to keep this in ExecScanSubPlan, when the code there
no longer depends on it:
+	Assert(list_length(subplan->parParam) == list_length(subplan->args));
I'd put that before the forboth() in ExecInitSubPlanExpr instead,
where it does matter.

			regards, tom lane

  8. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Andres Freund <andres@anarazel.de> — 2023-03-07T00:28:30Z

    Hi,

On 2023-03-03 15:09:18 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2023-03-02 13:00:31 -0800, Andres Freund wrote:
> >> I'm not opposed to EXPR_PARAM_SET, to be clear. I'll send an updated
> >> version later. I was just thinking about the correctness in the current
> >> world.
>
> > Attached.
>
> I've looked through this, and it looks basically OK so I marked it RfC.

Thanks!


> I do have a few nitpicks that you might or might not choose to adopt:
>
> It'd be good to have a header comment for ExecInitExprRec documenting
> the arguments, particularly that resv/resnull are where to put the
> subplan's eventual result.

Did you mean ExecInitSubPlanExpr()?


> You could avoid having to assume ExprState's resvalue/resnull being
> safe to use by instead using the target resv/resnull.  This would
> require putting those into the EEOP_PARAM_SET step so that
> ExecEvalParamSet knows where to fetch from, so maybe it's not an
> improvement, but perhaps worth considering.

I think that'd be a bit worse - we'd have more pointers that can't be handled
in a generic way in JIT.


> I think that ExecEvalParamSet should either set prm->execPlan to NULL,
> or maybe better Assert that it is already NULL.

Agreed.

Greetings,

Andres Freund

  9. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-03-07T00:51:09Z

    Andres Freund <andres@anarazel.de> writes:
> On 2023-03-03 15:09:18 -0500, Tom Lane wrote:
>> It'd be good to have a header comment for ExecInitExprRec documenting
>> the arguments, particularly that resv/resnull are where to put the
>> subplan's eventual result.

> Did you mean ExecInitSubPlanExpr()?

Right, copy-and-pasteo, sorry.

>> You could avoid having to assume ExprState's resvalue/resnull being
>> safe to use by instead using the target resv/resnull.  This would
>> require putting those into the EEOP_PARAM_SET step so that
>> ExecEvalParamSet knows where to fetch from, so maybe it's not an
>> improvement, but perhaps worth considering.

> I think that'd be a bit worse - we'd have more pointers that can't be handled
> in a generic way in JIT.

OK.

			regards, tom lane

  10. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Peter Eisentraut <peter@eisentraut.org> — 2023-10-01T18:41:53Z

    Is this patch still being worked on?

On 07.03.23 01:51, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2023-03-03 15:09:18 -0500, Tom Lane wrote:
>>> It'd be good to have a header comment for ExecInitExprRec documenting
>>> the arguments, particularly that resv/resnull are where to put the
>>> subplan's eventual result.
> 
>> Did you mean ExecInitSubPlanExpr()?
> 
> Right, copy-and-pasteo, sorry.
> 
>>> You could avoid having to assume ExprState's resvalue/resnull being
>>> safe to use by instead using the target resv/resnull.  This would
>>> require putting those into the EEOP_PARAM_SET step so that
>>> ExecEvalParamSet knows where to fetch from, so maybe it's not an
>>> improvement, but perhaps worth considering.
> 
>> I think that'd be a bit worse - we'd have more pointers that can't be handled
>> in a generic way in JIT.
> 
> OK.
> 
> 			regards, tom lane
> 
>

  11. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-10-01T18:53:23Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> Is this patch still being worked on?

I thought Andres simply hadn't gotten back to it yet.
It still seems like a worthwhile improvement.

			regards, tom lane

  12. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Andres Freund <andres@anarazel.de> — 2023-10-10T03:00:44Z

    Hi,

On 2023-10-01 14:53:23 -0400, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
> > Is this patch still being worked on?
> 
> I thought Andres simply hadn't gotten back to it yet.
> It still seems like a worthwhile improvement.

Indeed - I do plan to commit it. I haven't quite shifted into v17 mode yet...

Greetings,

Andres Freund

  13. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Alena Rybakina <lena.ribackina@yandex.ru> — 2023-10-23T20:14:02Z

    Hi!

I looked through your patch and noticed that it was not applied to the 
current version of the master. I rebased it and attached a version. I 
didn't see any problems and, honestly, no big changes were needed, all 
regression tests were passed.

I think it's better to add a test, but to be honest, I haven't been able 
to come up with something yet.

-- 
Regards,
Alena Rybakina

  14. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    John Naylor <johncnaylorls@gmail.com> — 2023-11-22T08:37:16Z

    On Tue, Oct 10, 2023 at 10:00 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2023-10-01 14:53:23 -0400, Tom Lane wrote:
> > Peter Eisentraut <peter@eisentraut.org> writes:
> > > Is this patch still being worked on?
> >
> > I thought Andres simply hadn't gotten back to it yet.
> > It still seems like a worthwhile improvement.
>
> Indeed - I do plan to commit it. I haven't quite shifted into v17 mode yet...

Any shift yet? ;-)

  15. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Peter Smith <smithpb2250@gmail.com> — 2024-01-21T23:30:22Z

    2024-01 Commitfest.

Hi, This patch has a CF status of "Ready for Committer", but it is
currently failing some CFbot tests [1]. Please have a look and post an
updated version..

======
[1] https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/46/4209

Kind Regards,
Peter Smith.

  16. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Andres Freund <andres@anarazel.de> — 2024-01-22T20:47:16Z

    Hi,

On 2024-01-22 10:30:22 +1100, Peter Smith wrote:
> 2024-01 Commitfest.
> 
> Hi, This patch has a CF status of "Ready for Committer", but it is
> currently failing some CFbot tests [1]. Please have a look and post an
> updated version..

I think this failure is independent of this patch - by coincidence I just
sent an email about the issue
https://www.postgresql.org/message-id/20240122204117.swton324xcoodnyi%40awork3.anarazel.de
a few minutes ago.

Greetings,

Andres Freund

  17. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    vignesh C <vignesh21@gmail.com> — 2024-01-26T02:37:00Z

    On Tue, 24 Oct 2023 at 01:47, Alena Rybakina <lena.ribackina@yandex.ru> wrote:
>
> Hi!
>
> I looked through your patch and noticed that it was not applied to the current version of the master. I rebased it and attached a version. I didn't see any problems and, honestly, no big changes were needed, all regression tests were passed.
>
> I think it's better to add a test, but to be honest, I haven't been able to come up with something yet.

The patch does not apply anymore as in CFBot at [1]:

=== Applying patches on top of PostgreSQL commit ID
7014c9a4bba2d1b67d60687afb5b2091c1d07f73 ===
=== applying patch
./v2-0001-WIP-Evaluate-arguments-of-correlated-SubPlans-in-the.patch
....
patching file src/include/executor/execExpr.h
Hunk #1 succeeded at 160 (offset 1 line).
Hunk #2 succeeded at 382 (offset 2 lines).
Hunk #3 FAILED at 778.
1 out of 3 hunks FAILED -- saving rejects to file
src/include/executor/execExpr.h.rej
patching file src/include/nodes/execnodes.h
Hunk #1 succeeded at 959 (offset 7 lines).

Please have a look and post an updated version.

[1] - http://cfbot.cputube.org/patch_46_4209.log

Regards,
Vignesh

  18. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Alena Rybakina <lena.ribackina@yandex.ru> — 2024-01-28T09:07:46Z

    On 26.01.2024 05:37, vignesh C wrote:
> On Tue, 24 Oct 2023 at 01:47, Alena Rybakina <lena.ribackina@yandex.ru> wrote:
>> Hi!
>>
>> I looked through your patch and noticed that it was not applied to the current version of the master. I rebased it and attached a version. I didn't see any problems and, honestly, no big changes were needed, all regression tests were passed.
>>
>> I think it's better to add a test, but to be honest, I haven't been able to come up with something yet.
> The patch does not apply anymore as in CFBot at [1]:
>
> === Applying patches on top of PostgreSQL commit ID
> 7014c9a4bba2d1b67d60687afb5b2091c1d07f73 ===
> === applying patch
> ./v2-0001-WIP-Evaluate-arguments-of-correlated-SubPlans-in-the.patch
> ....
> patching file src/include/executor/execExpr.h
> Hunk #1 succeeded at 160 (offset 1 line).
> Hunk #2 succeeded at 382 (offset 2 lines).
> Hunk #3 FAILED at 778.
> 1 out of 3 hunks FAILED -- saving rejects to file
> src/include/executor/execExpr.h.rej
> patching file src/include/nodes/execnodes.h
> Hunk #1 succeeded at 959 (offset 7 lines).
>
> Please have a look and post an updated version.
>
> [1] - http://cfbot.cputube.org/patch_46_4209.log
>
> Regards,
> Vignesh

Thank you!

I fixed it. The code remains the same.

-- 
Regards,
Alena Rybakina
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  19. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-18T20:01:19Z

    Alena Rybakina <lena.ribackina@yandex.ru> writes:
> I fixed it. The code remains the same.

I see the cfbot is again complaining that this patch doesn't apply.

In hopes of pushing this over the finish line, I fixed up the (minor)
patch conflict and also addressed the cosmetic complaints I had
upthread [1].  I think the attached v4 is committable.  If Andres is
too busy, I can push it, but really it's his patch ...

(BTW, I see no need for additional test cases.  Coverage checks show
that all this code is reached during the core regression tests.)

			regards, tom lane

[1] https://www.postgresql.org/message-id/2618533.1677874158%40sss.pgh.pa.us

  20. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Alena Rybakina <lena.ribackina@yandex.ru> — 2024-07-19T19:19:55Z

    On 18.07.2024 23:01, Tom Lane wrote:
> Alena Rybakina <lena.ribackina@yandex.ru> writes:
>> I fixed it. The code remains the same.
> I see the cfbot is again complaining that this patch doesn't apply.
>
> In hopes of pushing this over the finish line, I fixed up the (minor)
> patch conflict and also addressed the cosmetic complaints I had
> upthread [1].  I think the attached v4 is committable.  If Andres is
> too busy, I can push it, but really it's his patch ...
>
> (BTW, I see no need for additional test cases.  Coverage checks show
> that all this code is reached during the core regression tests.)
>
> 			regards, tom lane
>
> [1] https://www.postgresql.org/message-id/2618533.1677874158%40sss.pgh.pa.us
>
Thank you for your contribution! I looked at the patch again and I agree 
that it is ready to be pushed.

  21. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Andres Freund <andres@anarazel.de> — 2024-07-20T04:17:12Z

    On 2024-07-18 16:01:19 -0400, Tom Lane wrote:
> Alena Rybakina <lena.ribackina@yandex.ru> writes:
> > I fixed it. The code remains the same.
> 
> I see the cfbot is again complaining that this patch doesn't apply.
> 
> In hopes of pushing this over the finish line, I fixed up the (minor)
> patch conflict and also addressed the cosmetic complaints I had
> upthread [1].  I think the attached v4 is committable.  If Andres is
> too busy, I can push it, but really it's his patch ...

Thanks for the rebase - I'll try to get it pushed in the next few days!

  22. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Andres Freund <andres@anarazel.de> — 2024-08-01T02:12:37Z

    Hi,

On 2023-02-25 13:44:01 -0800, Andres Freund wrote:
> Ended up simpler than I'd thought. I see small, consistent, speedups and
> reductions in memory usage.

For the sake of person following the link from the commit message to this
thread in a few years, I thought it'd be useful to have an example for the
differences due to the patch.


Consider e.g. the query used for psql's \d pg_class, just because that's the
first thing using a subplan that I got my hand on:

Memory usage in ExecutorState changes from
  Grand total: 131072 bytes in 12 blocks; 88696 free (2 chunks); 42376 used
to
  Grand total: 131072 bytes in 12 blocks; 93656 free (4 chunks); 37416 used


What's more interesting is that if I - just to show the effect - force JITing,
EXPLAIN ANALYZE's jit section changes from:

JIT:
  Functions: 31
  Options: Inlining true, Optimization true, Expressions true, Deforming true
  Timing: Generation 2.656 ms (Deform 1.496 ms), Inlining 25.147 ms, Optimization 112.853 ms, Emission 81.585 ms, Total 222.241 ms

to

 JIT:
   Functions: 21
   Options: Inlining true, Optimization true, Expressions true, Deforming true
   Timing: Generation 1.883 ms (Deform 0.990 ms), Inlining 23.821 ms, Optimization 85.150 ms, Emission 64.303 ms, Total 175.157 ms

I.e. noticeably reduced overhead, mostly due to the reduction in emitted
functions.

The difference obviously gets bigger the more parameters the subplan has, in
artificial cases it can be very large.


I also see some small performance gains during execution, but for realistic
queries that's in the ~1-3% range.

Greetings,

Andres Freund

  23. Re: Evaluate arguments of correlated SubPlans in the referencing ExprState

    Andres Freund <andres@anarazel.de> — 2024-08-01T03:24:12Z

    Hi,

On 2024-07-19 21:17:12 -0700, Andres Freund wrote:
> On 2024-07-18 16:01:19 -0400, Tom Lane wrote:
> > Alena Rybakina <lena.ribackina@yandex.ru> writes:
> > > I fixed it. The code remains the same.
> > 
> > I see the cfbot is again complaining that this patch doesn't apply.
> > 
> > In hopes of pushing this over the finish line, I fixed up the (minor)
> > patch conflict and also addressed the cosmetic complaints I had
> > upthread [1].  I think the attached v4 is committable.  If Andres is
> > too busy, I can push it, but really it's his patch ...
> 
> Thanks for the rebase - I'll try to get it pushed in the next few days!

And finally done. No code changes. I did spend some more time evaluating the
resource usage benefits actually do exist (see mail upthread).

Thanks for the reviews, rebasing and the reminders!

Greetings,

Andres