Thread: ToDo: fast update of arrays with fixed length fields for PL/pgSQL

Hi,

On 2013-10-02 18:56:51 +0200, Pavel Stehule wrote:
> Hello
>
> this proposal is related to reported issue
> http://www.postgresql.org/message-id/E1VQuta-0007Y4-Ip@wrigleys.postgresql.org
>
>
> We can directly modify any fields of int, float, double arrays (when result
> size will be immutable). This trick is used now for acceleration of some
> aggregates.
>
> Ideas, comments?

A specific array might be located directly on a buffer - starting to
manipulate them inplace will lead to havoc in such scenarios. I don't
think we have the infrastructure for detecting such cases.

Greetings,

Andres Freund

--
 Andres Freund                     http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Pavel Stehule <pavel.stehule@gmail.com> writes:
> If you can do a update of some array in plpgsql now, then you have to work
> with local copy only. It is a necessary precondition, and I am think it is
> valid.

If the proposal only relates to assignments to elements of plpgsql local
variables, it's probably safe, but it's also probably not of much value.
plpgsql has enough overhead that I'm doubting you'd get much real-world
speedup.  I'm also not very excited about putting even more low-level
knowledge about array representation into plpgsql.

                        regards, tom lane

On 2013-10-07 16:00:54 +0200, Pavel Stehule wrote:
>                               /*
>                                * We need to do subscript evaluation, which might require
> @@ -4321,6 +4322,14 @@ exec_assign_value(PLpgSQL_execstate *estate,
>                                       oldarrayval = (ArrayType *) DatumGetPointer(oldarraydatum);
>
>                               /*
> +                              * support fast update for array scalar variable is enabled only
> +                              * when target is a scalar variable and variable holds a local
> +                              * copy of some array.
> +                              */
> +                             inplace_update = (((PLpgSQL_datum *) target)->dtype == PLPGSQL_DTYPE_VAR
> +                                                         && ((PLpgSQL_var *) target)->freeval);
> +
> +                             /*
>                                * Build the modified array value.
>                                */

Will this recognize if the local Datum is just a reference to an
external toast Datum (i.e. varattrib_1b_e)?

I don't know much about plpgsql's implementation, so please excuse if
the question is stupid.

Greetings,

Andres Freund

--
 Andres Freund                     http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 07.10.2013 17:00, Pavel Stehule wrote:

Hello

I fixed patch - there was a missing cast to domain when it was used (and
all regress tests are ok now).

This doesn't work correctly for varlen datatypes. I modified your quicksort example to work with text[], and got this:

postgres=#  SELECT array_upper(quicksort(1,20000,array_agg((g::text))),1) FROM generate_series(1,20000) g;
ERROR:  invalid input syntax for integer: " "
CONTEXT:  PL/pgSQL function quicksort(integer,integer,text[]) line 10 at assignment
PL/pgSQL function quicksort(integer,integer,text[]) line 16 at assignment

The handling of array domains is also wrong. You replace the new value to the array and only check the domain constraint after that. If the constraint is violated, it's too late to roll that back. For example:

postgres=# create domain posarray as int[] check (value[1] > 0);
CREATE DOMAIN
postgres=# do $$
declare
  iarr posarray;
begin
  begin
    iarr[1] := 1;
    iarr[1] := -1;
  exception when others then raise notice 'got error: %', sqlerrm; end;
  raise notice 'value: %', iarr[1];
end;
$$;
NOTICE:  got error: value for domain posarray violates check constraint "posarray_check"
NOTICE:  value: -1
DO

Without the patch, it prints 1, but with the patch, -1.


In general, I'm not convinced this patch is worth the trouble. The speedup isn't all that great; manipulating large arrays in PL/pgSQL is still so slow that if you care about performance you'll want to rewrite your function in some other language or use temporary tables. And you only get a gain with arrays of fixed-length element type with no NULLs.

So I think we should drop this patch in its current form. If we want to make array manipulation in PL/pgSQL, I think we'll have to do something similar to how we handle "row" variables, or something else entirely. But that's a much bigger patch, and I'm not sure it's worth the trouble either.

Looking at perf profile and the functions involved, though, I see some low-hanging fruit: in array_set, the new array is allocated with palloc0'd, but we only need to zero out a few alignment bytes where the new value is copied. Replacing it with palloc() will save some cycles, per the attached patch. Nowhere near as much as your patch, but this is pretty uncontroversial.

- Heikki

Heikki Linnakangas <heikki@localhost.vmware.com> writes:
> In general, I'm not convinced this patch is worth the trouble. The
> speedup isn't all that great; manipulating large arrays in PL/pgSQL is
> still so slow that if you care about performance you'll want to rewrite
> your function in some other language or use temporary tables. And you
> only get a gain with arrays of fixed-length element type with no NULLs.

> So I think we should drop this patch in its current form. If we want to
> make array manipulation in PL/pgSQL, I think we'll have to do something
> similar to how we handle "row" variables, or something else entirely.

I think that this area would be a fruitful place to make use of the
noncontiguous datatype storage ideas that we were discussing with the
PostGIS guys recently.  I agree that tackling it in the context of plpgsql
alone is not a good way to go at it.

I'm not saying this in a vacuum of information, either.  Some of the guys
at Salesforce have been poking at noncontiguous storage for arrays and
have gotten nice speedups --- but their patch is for plpgsql only and
only addresses arrays, which makes it enough of a kluge that I've not
wanted to bring it to the community.  I think we should work towards
a general solution instead.

                        regards, tom lane