Content-Type: text/plain;
	charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3776.700.51.11.1\))
Subject: Re: Using Expanded Objects other than Arrays from plpgsql
From: Andrey Borodin <x4mmm@yandex-team.ru>
In-Reply-To: <931398.1737905825@sss.pgh.pa.us>
Date: Sun, 26 Jan 2025 22:04:15 +0500
Cc: Michel Pelletier <pelletier.michel@gmail.com>,
 Pavel Stehule <pavel.stehule@gmail.com>,
 pgsql-hackers@lists.postgresql.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <38A31221-C1C4-4846-9709-D66ACD76E87A@yandex-team.ru>
References: <CACxu=vJaKFNsYxooSnW1wEgsAO5u_v1XYBacfVJ14wgJV_PYeg@mail.gmail.com>
 <1342498.1729444411@sss.pgh.pa.us>
 <CACxu=vLXvpzN4X3k+9jsMt6ujuOvFVUSkA80t_cROSsF4y2jQQ@mail.gmail.com>
 <1445998.1729482404@sss.pgh.pa.us>
 <CACxu=vKEF8Qa-OaADFxf0uMg-xw6gH_CNCWd2s+xaqh-gY4=xg@mail.gmail.com>
 <2062830.1729625620@sss.pgh.pa.us> <2265411.1729699470@sss.pgh.pa.us>
 <CACxu=v++HNmss59yGUDkRny7g=M8tZ2YXF07AUXqKVGqcSfxGQ@mail.gmail.com>
 <2354718.1729737539@sss.pgh.pa.us> <2581216.1729794746@sss.pgh.pa.us>
 <CACxu=v+dn37zr8gx5xNP-EZY3OLtGLTHrbx_ZkCQc40HpyMLKA@mail.gmail.com>
 <1948345.1730500073@sss.pgh.pa.us>
 <CACxu=vL7i_U_iSNpREe8eCAMEyKHuPpk9THRpBhk+ar0U1EdOw@mail.gmail.com>
 <CACxu=vKLc6f5N8_DR58LKkE1eohWSxTvThTeGsLm7p7QH1aFBA@mail.gmail.com>
 <CACxu=vJf2S=ysun_h=zmYNu6oUM47+egbpX5mMC0X9BJK=EQwQ@mail.gmail.com>
 <CACxu=vK+S6BXN8ZYyBvqQBWrcwHXqtue1-ZuKO3+XtHGBYcDUQ@mail.gmail.com>
 <CAFj8pRCd6xcH-AYEyHFdGdU89O9JjZ-v-pyQnOwd9zNJkCEdhQ@mail.gmail.com>
 <3797606.1732045516@sss.pgh.pa.us> <647219! ! .1736019347@sss.pgh.pa.us>
 <CACxu=vJ1UeXPjPOzfT2E=tFpSCxPxcB7hgfqHYmefJWh3ZXPew@mail.gmail.com>
 <1417389.1736964543@sss.pgh.pa.us>
 <CACxu=vKModwqSpEjpn-ur1uMAEYkcd-i8CvNsH-z9=dk7F7YQw@mail.gmail.com>
 <3363452.1737483125@sss.pgh.pa.us>
 <0AC229FA-A3F1-43FD-B0DC-A46A73FEAFF7@yandex-team.ru>
 <931398.1737905825@sss.pgh.pa.us>
To: Tom Lane <tgl@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/38A31221-C1C4-4846-9709-D66ACD76E87A%40yandex-team.ru>
Precedence: bulk


> On 26 Jan 2025, at 20:37, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>=20
>> And the coverage of newly invented mark_stmt() 42.37%. Some of =
branches are easy noops, but some are not.
>=20
> Yeah.  I'm not too concerned about that because it's pretty much a
> copy-and-paste of the adjacent code.  Maybe we should think about
> some way of refactoring pl_funcs.c to reduce duplication, but I
> don't have any great ideas about how.

OK, now I got it. The whole purpose of 2nd patch is to do
if (expr->target_param >=3D 0)	expr->target_is_local =3D =
bms_is_member(expr->target_param, local_dnos);
to local variables.=20

>=20
>> expr_is_assignment_source() is named like if it should return nool, =
but it's void.
>=20
> I've been less than satisfied with that name too.  I intended it
> as a statement of fact, "this expression has been found to be=20
> the source of an assignment".  But it does seem confusing.
> Maybe we should recast it as an action.  What do you think of
> "mark_expr_as_assignment_source"?

Sounds better to me. I found no examples of similar functions nether in =
pl_gram.y, nor in gram.y, so IMO mark_expr_as_assignment_source() is the =
best candidate.

>=20
>> I could not grasp from reading the code one generic question about =
new optimization rule. What cost does checking for possible in-place =
update incurs to code cannot have this optimization? Is it =
O(numer_of_arguments) of for every assignment execution?
>=20
> No, the extra effort is incurred at most once per assignment statement
> per session.  (Unless the plpgsql function's cache entry gets
> invalidated, in which case we'd rebuild all of the function's data
> structures and have to redo this work too.)

OK, I think execution benefits justify this preparatory costs.

>  We set up the evaluation
> function "paramfunc" as plpgsql_param_eval_var_check if we think we
> might be able to apply this optimization, or plpgsql_param_eval_var_ro
> if we don't think so but the variable is of varlena type.  At runtime,
> if the variable's current value is not actually expanded, then
> plpgsql_param_eval_var_check falls through doing essentially the same
> work as plpgsql_param_eval_var_ro, so there should be no added cost.
> The first time we observe that the value *is* expanded, we incur the
> cost to detect whether an optimization is really possible, and then
> we change the "paramfunc" pointer to be the appropriate function
> so as to apply the optimization or not without rechecking.  So
> generally speaking, if we're considering a variable of a type that
> doesn't support expansion, there should be zero extra per-execution
> cost.  There is some extra cost at function compilation time to
> determine which expressions are assignment sources (but we were doing
> that already) and to discover whether those assignments are to
> nonlocal variables (which is new work, but only needs to be done in
> functions with exception blocks).

Got it, many thanks for the explanation.

But I've got some new questions:

I'm lost in internals of ExprEvalStep. But void *paramarg and his friend =
void *paramarg2 are cryptic. They always have same type and same =
meaning, but have very generic names.

I wonder if you plan similar optimizations for array_cat(), =
array_remove() etc?

+   a :=3D a || a; -- not optimizable

Why is it not optimizable? Because there is no support function, because =
array_cat() has no support function, or something else?

Besides this, the patch looks good to me.


Best regards, Andrey Borodin.=