Re: Rethinking plpgsql's assignment implementation

Pavel Stehule <pavel.stehule@gmail.com> writes:
> I checked a performance and it looks so access to record's field is faster,
> but an access to arrays field is significantly slower

Hmm, I'd drawn the opposite conclusion in my own testing ...

>     for i in 1..5000
>     loop
>       if a[i] > a[i+1] then
>         aux := a[i];
>         a[i] := a[i+1]; a[i+1] := aux;
>         rep := true;
>       end if;
>     end loop;

... but I now see that I'd not checked cases like "a[i] := a[j]".
exec_check_rw_parameter() is being too conservative about whether
it can optimize a case like that.  The attached incremental patch
fixes it.

> I tested pi calculation
> ...
> And the performance is 10% slower than on master

Can't reproduce that here.  For the record, I get the following
timings (medians of three runs) for your test cases:

HEAD:

sort:            Time: 13974.709 ms (00:13.975)
pi_est_1(10000000):    Time: 3537.482 ms (00:03.537)
pi_est_2(10000000):    Time: 3546.557 ms (00:03.547)

Patch v1:

sort:            Time: 47053.892 ms (00:47.054)
pi_est_1(10000000):    Time: 3456.078 ms (00:03.456)
pi_est_2(10000000):    Time: 3451.347 ms (00:03.451)

+ exec_check_rw_parameter fix:

sort:            Time: 12199.724 ms (00:12.200)
pi_est_1(10000000):    Time: 3357.955 ms (00:03.358)
pi_est_2(10000000):    Time: 3367.526 ms (00:03.368)

I'm inclined to think that the differences in the pi calculation
timings are mostly chance effects; there's certainly no reason
why exec_check_rw_parameter should affect that test case at all.

            regards, tom lane

diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index 4c8a739bc4..15cb3b312f 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -2583,7 +2583,9 @@ array_set_element_expanded(Datum arraydatum,
     /*
      * Copy new element into array's context, if needed (we assume it's
      * already detoasted, so no junk should be created).  If we fail further
-     * down, this memory is leaked, but that's reasonably harmless.
+     * down, this memory is leaked, but that's reasonably harmless.  Note in
+     * particular that doing this early ensures sanity in case the source
+     * Datum is a pointer to a pass-by-ref element of this same array.
      */
     if (!eah->typbyval && !isNull)
     {
diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 5c1db1dcfb..1378f40d4d 100644
--- a/src/pl/plpgsql/src/pl_exec.c
+++ b/src/pl/plpgsql/src/pl_exec.c
@@ -8181,14 +8181,7 @@ exec_check_rw_parameter(PLpgSQL_expr *expr, int target_dno)
             contains_target_param((Node *) sbsref->refexpr, &target_dno))
             return;

-        /* the other subexpressions must not contain target */
-        if (contains_target_param((Node *) sbsref->refupperindexpr,
-                                  &target_dno) ||
-            contains_target_param((Node *) sbsref->reflowerindexpr,
-                                  &target_dno) ||
-            contains_target_param((Node *) sbsref->refassgnexpr,
-                                  &target_dno))
-            return;
+        /* we do *not* need to restrict the subscripts or source expression */

         /* OK, we can pass target as a read-write parameter */
         expr->rwparam = target_dno;