Horton

Thread

Commits

  1. Use fast path in plpgsql's RETURN/RETURN NEXT in more cases.

  2. Add support for multiple kinds of external toast datums.

  1. Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-02-10T20:00:35Z

    I've been fooling around with a design to support computation-oriented,
not-necessarily-contiguous-blobs representations of datatypes in memory,
along the lines I mentioned here:
http://www.postgresql.org/message-id/2355.1382710707@sss.pgh.pa.us

In particular this is meant to reduce the overhead for repeated operations
on arrays, records, etc.  We've had several previous discussions about
that, and even some single-purpose patches such as in this thread:
http://www.postgresql.org/message-id/flat/CAFj8pRAKuDU_0md-dg6Ftk0wSupvMLyrV1PB+HyC+GUBZz346w@mail.gmail.com
There was also a thread discussing how this sort of thing could be
useful to PostGIS:
http://www.postgresql.org/message-id/526A61FB.1050209@oslandia.com
and it's been discussed a few other times too, but I'm too lazy to
search the archives any further.

I've now taken this idea as far as building the required infrastructure
and revamping a couple of array operators to use it.  There's a lot yet
to do, but I've done enough to get some preliminary ideas about
performance (see below).

The core ideas of this patch are:

* Invent a new TOAST datum type "pointer to deserialized object", which
is physically just like the existing indirect-toast-pointer concept, but
it has a different va_tag code and somewhat different semantics.

* A deserialized object has a standard header (which is what the toast
pointers point to) and typically will have additional data-type-specific
fields after that.  One component of the standard header is a pointer to
a set of "method" functions that provide ways to accomplish standard
data-type-independent operations on the deserialized object.

* Another standard header component is a MemoryContext identifier: the
header, as well as all subsidiary data belonging to the deserialized
object, must live in this context.  (Well, I guess there could also be
child contexts.)  By exposing an explicit context identifier, we can
accomplish tasks like "move this object into another context" by
reparenting the object's context rather than physically copying anything.

* The only standard "methods" I've found a need for so far are functions
to re-serialize the object, that is generate a plain varlena value that is
semantically equivalent.  To avoid extra copying, this is split into
separate "compute the space needed" and "serialize into this memory"
steps, so that the result can be dropped exactly where the caller needs
it.

* Currently, a deserialized object will be reserialized in that way
whenever we incorporate it into a physical tuple (ie, heap_form_tuple
or index_form_tuple), or whenever somebody applies datumCopy() to it.
I'd like to relax this later, but there's an awful lot of code that
supposes that heap_form_tuple or datumCopy will produce a self-contained
value that survives beyond, eg, destruction of the memory context that
contained the source Datums.  We can get good speedups in a lot of
interesting cases without solving that problem, so I don't feel too bad
about leaving it as a future project.

* In particular, things like PG_GETARG_ARRAYTYPE_P() treat a deserialized
toast pointer as something to be detoasted, and will produce a palloc'd
re-serialized value.  This means that we do not need to convert all the
C functions concerned with a given datatype at the same time (or indeed
ever); a function that hasn't been upgraded will build a re-serialized
representation and then operate on that.  We'll invent alternate
argument-fetching functions that skip the reserialization step, for use
by functions that have been upgraded to handle either case.  This is
basically the same approach we used when we introduced short varlena
headers, and that seems to have gone smoothly enough.

* There's a concept that a deserialized object has a "primary" toast
pointer, which is physically part of the object, as well as "secondary"
toast pointers which might or might not be part of the object.  If you
have a Datum pointer to the primary toast pointer then you are authorized
to modify the object in-place; if you have a Datum pointer to a secondary
toast pointer then you must treat it as read-only (ie, you have to make a
copy if you're going to change it).  Functions that construct a new
deserialized object always return its primary toast pointer; this allows a
nest of functions to modify an object in-place without copying, which was
the primary need that the PostGIS folks expressed.  On the other hand,
plpgsql can hand out secondary toast pointers to deserialized objects
stored in plpgsql function variables, thus ensuring that the objects won't
be modified unexpectedly, while never having to physically copy them if
the called functions just need to inspect them.

* Primary and secondary pointers are physically identical, but the
primary pointer resides in a specific spot in the deserialized object's
standard header.  (So you can tell if you've got the primary pointer via
a simple address comparison.)

* I've modified the array element assignment path in plpgsql's
exec_assign_value so that, instead of passing a secondary toast pointer
to array_set() as you might expect from the above, it passes the primary
toast pointer thus allowing array_set() to modify the variable in-place.
So an operation like "array_variable[x] := y" no longer incurs recopying
of the whole array, once the variable has been converted into deserialized
form.  (If it's not yet, it becomes deserialized after the first such
assignment.)  Also, assignment of an already-deserialized value to a
variable accomplishes that with a MemoryContext parent pointer swing
instead of physical copying, if what we have is the primary toast pointer,
which implies it's not referenced anywhere else.

* Any functions that plpgsql gives a read/write pointer to need to be
exceedingly careful to not leave a corrupted object behind if they fail
partway through.  I've successfully written such a version of array_set(),
and it wasn't too painful, but this may be a limitation on the general
applicability of the whole approach.

* In the current patch, that restriction only applies to array_set()
anyway.  But I would like to allow in-place updates for non-core cases.
For example in something like
	hstore_var := hstore_var || 'foo=>bar';
we could plausibly pass a R/W pointer to hstore_concat and let it modify
hstore_var in place.  But this would require knowing which such functions
are safe, or assuming that they all are, which might be an onerous
restriction.

* I soon noticed that I was getting a lot of empty "deserialized array"
contexts floating around.  The attached patch addresses this in a quick
hack fashion by redefining ResetExprContext() to use
MemoryContextResetAndDeleteChildren() instead of MemoryContextReset(),
so that deserialized objects created within an expression evaluation
context go completely away at ResetExprContext(), rather than being left
behind as empty subcontext shells.  We've talked more than once about
redefining mcxt.c's API so that MemoryContextReset() means what's
currently meant by MemoryContextResetAndDeleteChildren(), and if you
really truly do want to keep empty child contexts around then you need to
call something else instead.  I did not go that far here, but I think we
should seriously consider biting the bullet and finally changing it.

* Although I said above that everything owned by a deserialized object
has to live in a single memory context, I do have ideas about relaxing
that.  The core idea would be to invent a "memory context reset/delete
callback" feature in mcxt.c.  Then a deserialized object could register
such a callback on its own memory context, and use the callback to clean
up resources outside its context.  This is potentially useful for instance
for something like PostGIS, where an object likely includes some data that
was allocated with malloc not palloc because it was created by library
functions that aren't Postgres-aware.  Another likely use-case is for
deserialized objects representing composite types to maintain reference
counts on their tuple descriptors instead of having to copy said
descriptors into their private contexts.  This'd be material for a
separate patch though.


So that's the plan, and attached is a very-much-WIP patch that uses this
approach to speed up plpgsql array element assignments (and not a whole
lot else as yet).  Here's the basic test case I've been using:

create or replace function arraysetint(n int) returns int[] as $$
declare res int[] := '{}';
begin
  for i in 1 .. n loop
    res[i] := i;
  end loop;
  return res;
end
$$ language plpgsql strict;

In HEAD, this function's runtime grows as O(N^2), so for example
(with casserts off on my x86_64 workstation):

regression=# select array_dims(arraysetint(100000));
 array_dims 
------------
 [1:100000]
(1 row)

Time: 7874.070 ms

With variable-length array elements, such as if you change the
int[] arrays to numeric[], it's even worse:

regression=# select array_dims(arraysetnum(100000));
 array_dims 
------------
 [1:100000]
(1 row)

Time: 31177.340 ms

With the attached patch, those timings drop to 80 and 150 ms respectively.

It's not all peaches and cream: for the array_append operator, which is
also accelerated by the patch (mainly because it is too much in bed with
array_set to not fix at the same time ;-)), I tried examples like

explain analyze select array[1,2] || g || g || g from generate_series(1,1000000) g;

Very roughly, HEAD needs about 400 ns per || operator in this scenario.
With the patch, it's about 480 ns for the first operator and then 200 more
for each one accepting a prior operator's output.  (Those numbers could
perhaps be improved with more-invasive refactoring of the array code.)
The extra initial overhead represents the time to convert the array[1,2]
constant to deserialized form during each execution of the first operator.

Still, if the worst-case slowdown is around 20% on trivially-sized arrays,
I'd gladly take that to have better performance on larger arrays.  And I
think this example is close to the worst case for the patch's approach,
since it's testing small, fixed-element-length, no-nulls arrays, which is
what the existing code can handle without spending a lot of cycles.

Note that I've kept all the deserialized-array-specific code in its own file
for now, just for ease of hacking.  That stuff would need to propagate into
the main array-related files in a more complete patch.

BTW, I'm not all that thrilled with the "deserialized object" terminology.
I found myself repeatedly tripping up on which form was serialized and
which de-.  If anyone's got a better naming idea I'm willing to adopt it.

I'm not sure exactly how to push this forward.  I would not want to
commit it without converting a significant number of array functions to
understand about deserialized inputs, and by the time I've finished that
work it's likely to be too late for 9.5.  OTOH I'm sure that the PostGIS
folk would love to have this infrastructure in 9.5 not 9.6 so they could
make a start on fixing their issues.  (Further down the pike, I'd plan to
look at adapting composite-type operations, JSONB, etc, to make use of
this approach, but that certainly isn't happening for 9.5.)

Thoughts, advice, better ideas?

			regards, tom lane

  2. Re: Manipulating complex types as non-contiguous structures in-memory

    Stephen Frost <sfrost@snowman.net> — 2015-02-10T21:00:22Z

    Tom,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> I've now taken this idea as far as building the required infrastructure
> and revamping a couple of array operators to use it.  There's a lot yet
> to do, but I've done enough to get some preliminary ideas about
> performance (see below).

Nice!

> * Although I said above that everything owned by a deserialized object
> has to live in a single memory context, I do have ideas about relaxing
> that.  The core idea would be to invent a "memory context reset/delete
> callback" feature in mcxt.c.  Then a deserialized object could register
> such a callback on its own memory context, and use the callback to clean
> up resources outside its context.  This is potentially useful for instance
> for something like PostGIS, where an object likely includes some data that
> was allocated with malloc not palloc because it was created by library
> functions that aren't Postgres-aware.  Another likely use-case is for
> deserialized objects representing composite types to maintain reference
> counts on their tuple descriptors instead of having to copy said
> descriptors into their private contexts.  This'd be material for a
> separate patch though.

Being able to register a callback to be used on deletion of the context
would certainly be very nice and strikes me as pretty independent of the
rest of this.  You've probably thought of this already, but registering
the callback should probably allow the caller to pass in a pointer to be
passed back to the callback function when the delete happens, so that
there's a place for the metadata to be stored about what the callback
function needs to clean up when it's called.

> So that's the plan, and attached is a very-much-WIP patch that uses this
> approach to speed up plpgsql array element assignments (and not a whole
> lot else as yet).  Here's the basic test case I've been using:

I've not looked at the code at all as yet, but it makes sense to me.

> With the attached patch, those timings drop to 80 and 150 ms respectively.

And those numbers are pretty fantastic and would address an area we
regularly get dinged on.

> Still, if the worst-case slowdown is around 20% on trivially-sized arrays,
> I'd gladly take that to have better performance on larger arrays.  And I
> think this example is close to the worst case for the patch's approach,
> since it's testing small, fixed-element-length, no-nulls arrays, which is
> what the existing code can handle without spending a lot of cycles.

Agreed.

> BTW, I'm not all that thrilled with the "deserialized object" terminology.
> I found myself repeatedly tripping up on which form was serialized and
> which de-.  If anyone's got a better naming idea I'm willing to adopt it.

Unfortunately, nothing comes to mind.  Serialization is, at least, a
pretty well understood concept and so the naming will likely make sense
to newcomers, even if it's difficult to keep track of which is
serialized and which is deserialized.

> I'm not sure exactly how to push this forward.  I would not want to
> commit it without converting a significant number of array functions to
> understand about deserialized inputs, and by the time I've finished that
> work it's likely to be too late for 9.5.  OTOH I'm sure that the PostGIS
> folk would love to have this infrastructure in 9.5 not 9.6 so they could
> make a start on fixing their issues.  (Further down the pike, I'd plan to
> look at adapting composite-type operations, JSONB, etc, to make use of
> this approach, but that certainly isn't happening for 9.5.)
> 
> Thoughts, advice, better ideas?

I'm not really a big fan of putting an infrastructure out there for
modules to use that we don't use ourselves (particularly when it's clear
that there are places where we could/should be).  On the other hand,
this doesn't impact on-disk format and therefore I'm less worried that
we'll end up with a release-critical issue when we're getting ready to
put 9.5 out there.

So, I'm on the fence about it.  I'd love to see all of this in 9.5 with
the array functions converted, but I don't think it'd be horrible if
only a subset had been done in time for 9.5.  The others aren't going to
go anywhere and will still work.  I do think it'd be better to have at
least some core users of this new infrastructure rather than just
putting it out there for modules to use but I agree it'd be a bit grotty
to have only some of the array functions converted.

	Thanks!

		Stephen

  3. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-02-10T23:01:14Z

    [ this is addressing a tangential point ... ]

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> * Although I said above that everything owned by a deserialized object
>> has to live in a single memory context, I do have ideas about relaxing
>> that.  The core idea would be to invent a "memory context reset/delete
>> callback" feature in mcxt.c.  Then a deserialized object could register
>> such a callback on its own memory context, and use the callback to clean
>> up resources outside its context.

> Being able to register a callback to be used on deletion of the context
> would certainly be very nice and strikes me as pretty independent of the
> rest of this.  You've probably thought of this already, but registering
> the callback should probably allow the caller to pass in a pointer to be
> passed back to the callback function when the delete happens, so that
> there's a place for the metadata to be stored about what the callback
> function needs to clean up when it's called.

Yeah, there would likely be use-cases for that outside of deserialized
objects.  I could submit a separate patch for that now, but I'm hesitant
to add a mechanism without any use-case in the same patch.  But maybe we
could find a caller somewhere in the core aggregate code --- there are
some aggregates that need cleanup callbacks already, IIRC, and maybe we
could change them to use a memory context callback instead of whatever
they're doing now.

			regards, tom lane

  4. Re: Manipulating complex types as non-contiguous structures in-memory

    Jim Nasby <jim.nasby@bluetreble.com> — 2015-02-10T23:09:57Z

    Without having read the patch, I think this is great. I've been wishing 
for something like this while working on my variant data type.

Are there any cases where we would want to use this on a non-variant? 
Perhaps types where we're paying an alignment penalty?

On 2/10/15 3:00 PM, Stephen Frost wrote:
>> >BTW, I'm not all that thrilled with the "deserialized object" terminology.
>> >I found myself repeatedly tripping up on which form was serialized and
>> >which de-.  If anyone's got a better naming idea I'm willing to adopt it.
> Unfortunately, nothing comes to mind.  Serialization is, at least, a
> pretty well understood concept and so the naming will likely make sense
> to newcomers, even if it's difficult to keep track of which is
> serialized and which is deserialized.

Apologies if I'm just dense, but what's the confusion? Is it what a 
serialized datum means in this context? (de)serialized seems like a 
perfectly logical name to me...

>> >I'm not sure exactly how to push this forward.  I would not want to
>> >commit it without converting a significant number of array functions to
>> >understand about deserialized inputs, and by the time I've finished that
>> >work it's likely to be too late for 9.5.  OTOH I'm sure that the PostGIS
>> >folk would love to have this infrastructure in 9.5 not 9.6 so they could
>> >make a start on fixing their issues.  (Further down the pike, I'd plan to
>> >look at adapting composite-type operations, JSONB, etc, to make use of
>> >this approach, but that certainly isn't happening for 9.5.)
>> >
>> >Thoughts, advice, better ideas?
> I'm not really a big fan of putting an infrastructure out there for
> modules to use that we don't use ourselves (particularly when it's clear
> that there are places where we could/should be).  On the other hand,
> this doesn't impact on-disk format and therefore I'm less worried that
> we'll end up with a release-critical issue when we're getting ready to
> put 9.5 out there.
>
> So, I'm on the fence about it.  I'd love to see all of this in 9.5 with
> the array functions converted, but I don't think it'd be horrible if
> only a subset had been done in time for 9.5.  The others aren't going to
> go anywhere and will still work.  I do think it'd be better to have at
> least some core users of this new infrastructure rather than just
> putting it out there for modules to use but I agree it'd be a bit grotty
> to have only some of the array functions converted.

I think the solution here is to have people other than Tom do the 
gruntwork of applying this to the remaining array code. My thought is 
that if Tom shows how to do this correctly in a rather complex case (ie, 
where you need to worry about primary vs secondary), then less 
experienced hackers should be able to take the ball and run with it.

Maybe we won't get complete array coverage, but I think any performance 
gains here are a win. And really, don't we just need enough usage so the 
buildfarm will tell us if we accidentally break something?
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

  5. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-02-10T23:19:12Z

    Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
> Without having read the patch, I think this is great. I've been wishing 
> for something like this while working on my variant data type.

> Are there any cases where we would want to use this on a non-variant? 
> Perhaps types where we're paying an alignment penalty?

What do you mean by non-variant?

The use cases that have come to mind for me are:

* arrays, of course
* composite types (records)
* PostGIS geometry type
* JSONB, hstore
* possibly regex patterns (we could invent a data type representing these
and then store the compiled form as a deserialized representation;
although there would be some issues to be worked out to get any actual
win, probably)

The principal thing that's a bit hard to figure out is when it's a win to
convert to a deserialized representation and when you should just leave
well enough alone.  I'm planning to investigate that further in the
context of plpgsql array variables, but I'm not sure how well those
answers will carry over to datatypes that plpgsql has no intrinsic
understanding of.

			regards, tom lane

  6. Re: Manipulating complex types as non-contiguous structures in-memory

    Jim Nasby <jim.nasby@bluetreble.com> — 2015-02-11T05:58:07Z

    On 2/10/15 5:19 PM, Tom Lane wrote:
> Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
>> Without having read the patch, I think this is great. I've been wishing
>> for something like this while working on my variant data type.
>
>> Are there any cases where we would want to use this on a non-variant?
>> Perhaps types where we're paying an alignment penalty?
>
> What do you mean by non-variant?

Ugh, sorry, brainfart. I meant to say non-varlena.

I can't think of any non-varlena types we'd want this for, but maybe 
someone else can think of a case. If there is a use-case I wouldn't 
handle it with this patch, but we'd want to consider it...
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

  7. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-02-11T13:25:08Z

    Jim Nasby <Jim.Nasby@BlueTreble.com> writes:
> On 2/10/15 5:19 PM, Tom Lane wrote:
>> What do you mean by non-variant?

> Ugh, sorry, brainfart. I meant to say non-varlena.

> I can't think of any non-varlena types we'd want this for, but maybe 
> someone else can think of a case. If there is a use-case I wouldn't 
> handle it with this patch, but we'd want to consider it...

There isn't any practical way to interpose TOAST pointers for non-varlena
types, since we make no assumptions about the bit contents of fixed-length
types.  But I'm having a hard time thinking of a fixed-length type in
which you'd have any need for a deserialized representation, either.
I think restricting this feature to varlena types is just fine.

			regards, tom lane

  8. Re: Manipulating complex types as non-contiguous structures in-memory

    Stephen Frost <sfrost@snowman.net> — 2015-02-11T16:40:59Z

    * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> I think restricting this feature to varlena types is just fine.

Agreed.

	Thanks,

		Stephen

  9. Re: Manipulating complex types as non-contiguous structures in-memory

    Robert Haas <robertmhaas@gmail.com> — 2015-02-12T13:52:56Z

    On Tue, Feb 10, 2015 at 3:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I've now taken this idea as far as building the required infrastructure
> and revamping a couple of array operators to use it.  There's a lot yet
> to do, but I've done enough to get some preliminary ideas about
> performance (see below).

Very impressive.  This is something that's been mentioned before and
which I always thought would be great to have, but I didn't expect it
would be this easy to cobble together a working implementation.  Or
maybe "easy" isn't the right term, but this isn't a very big patch.

> BTW, I'm not all that thrilled with the "deserialized object" terminology.
> I found myself repeatedly tripping up on which form was serialized and
> which de-.  If anyone's got a better naming idea I'm willing to adopt it.

My first thought is that we should form some kind of TOAST-like
backronym, like Serialization Avoidance Loading and Access Device
(SALAD) or Break-up, Read, Edit, Assemble, and Deposit (BREAD).  I
don't think there is anything per se wrong with the terms
serialization and deserialization; indeed, I used the same ones in the
parallel-mode stuff.  But they are fairly general terms, so it might
be nice to have something more specific that applies just to this
particular usage.

I found the notion of "primary" and "secondary" TOAST pointers to be
quite confusing.  I *think* what you are doing is storing two pointers
to the object in the object, and a pointer to the object is really a
pointer to one of those two pointers to the object.  Depending on
which one it is, you can write the object, or not.  This is a clever
representation, but it's hard to wrap your head around, and I'm not
sure "primary" and "secondary" are the best names, although I don't
have an idea as to what would be better.  I'm a bit confused, though:
once you give out a secondary pointer, how is it safe to write the
object through the primary pointer?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  10. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-02-12T14:50:18Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Feb 10, 2015 at 3:00 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> BTW, I'm not all that thrilled with the "deserialized object" terminology.
>> I found myself repeatedly tripping up on which form was serialized and
>> which de-.  If anyone's got a better naming idea I'm willing to adopt it.

> My first thought is that we should form some kind of TOAST-like
> backronym, like Serialization Avoidance Loading and Access Device
> (SALAD) or Break-up, Read, Edit, Assemble, and Deposit (BREAD).  I
> don't think there is anything per se wrong with the terms
> serialization and deserialization; indeed, I used the same ones in the
> parallel-mode stuff.  But they are fairly general terms, so it might
> be nice to have something more specific that applies just to this
> particular usage.

Hm.  I'm not against the concept, but those particular suggestions don't
grab me.

> I found the notion of "primary" and "secondary" TOAST pointers to be
> quite confusing.  I *think* what you are doing is storing two pointers
> to the object in the object, and a pointer to the object is really a
> pointer to one of those two pointers to the object.  Depending on
> which one it is, you can write the object, or not.

There's more to it than that.  (Writing more docs is one of the to-do
items ;-).)  We could alternatively have done that with two different
va_tag values for "read write" and "read only", which indeed was my
initial intention before I thought of this dodge.  However, then you
have to figure out where to store such pointers, which is problematic
both for plpgsql variable assignment and for ExecMakeSlotContentsReadOnly,
especially the latter which would have to put any freshly-made pointer
in a long-lived context resulting in query-lifespan memory leaks.
So I early decided that the read-write pointer should live right in the
object's own context where it need not be copied when swinging the
context ownership someplace else, and later realized that there should
also be a permanent read-only pointer in there for the use of
ExecMakeSlotContentsReadOnly, and then realized that they didn't need
to have different va_tag values if we implemented the "is read-write
pointer" test as it's done in the patch.  Having only one va_tag value
not two saves cycles, I think, because there are a lot of low-level
tests that don't need to distinguish, eg VARTAG_SIZE().  However it
does make it more expensive when you do need to distinguish, so I might
reconsider that decision later.  (Since these will never go to disk,
we can whack the representation around pretty freely if needed.)

Also, I have hopes of allowing deserialized-object pointers to be copied
into tuples as pointers rather than by reserialization, if we can
establish that the tuple is short-lived enough that the pointer will stay
good, which would be true in a lot of cases during execution of queries by
plpgsql.  With the patch's design, a pointer so copied will automatically
be considered read-only, which I *think* is the behavior we'd need.  If it
turns out that it's okay to propagate read-write-ness through such a copy
step then that would argue in favor of using two va_tag values.

It may be that this solution is overly cute and we should just use two
tag values.  But I wanted to be sure it was possible for copying of a
pointer to automatically lose read-write-ness, in case we need to have
such a guarantee.

> This is a clever
> representation, but it's hard to wrap your head around, and I'm not
> sure "primary" and "secondary" are the best names, although I don't
> have an idea as to what would be better.  I'm a bit confused, though:
> once you give out a secondary pointer, how is it safe to write the
> object through the primary pointer?

It's no different from allowing plpgsql to update the values of variables
of pass-by-reference types even though it has previously given out Datums
that are pointers to them: by the time we're ready to execute an
assignment, any query execution that had such a pointer is over and done
with.  (This implies that cursor parameters have to be physically copied
into the cursor's execution state, which is one of a depressingly large
number of reasons why datumCopy() has to physically copy a deserialized
value rather than just copying the pointer.  But otherwise it works.)

There is more work to do to figure out how we can safely give out a
read/write pointer for cases like
	hstore_var := hstore_concat(hstore_var, ...);
Aside from the question of whether hstore_concat guarantees not to trash
the value on failure, we'd have to restrict this (I think) to expressions
in which there is only one reference to the target variable and it's an
argument of the topmost function/operator.  But that's something I've not
tried to implement yet.

			regards, tom lane

  11. Re: Manipulating complex types as non-contiguous structures in-memory

    Robert Haas <robertmhaas@gmail.com> — 2015-02-12T15:07:13Z

    On Thu, Feb 12, 2015 at 9:50 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> My first thought is that we should form some kind of TOAST-like
>> backronym, like Serialization Avoidance Loading and Access Device
>> (SALAD) or Break-up, Read, Edit, Assemble, and Deposit (BREAD).  I
>> don't think there is anything per se wrong with the terms
>> serialization and deserialization; indeed, I used the same ones in the
>> parallel-mode stuff.  But they are fairly general terms, so it might
>> be nice to have something more specific that applies just to this
>> particular usage.
>
> Hm.  I'm not against the concept, but those particular suggestions don't
> grab me.

Fair enough.  I guess the core of my point is just that I suggest we
invent a name for this thing.  "Serialize" and "deserialize" describe
what you are doing just fine, but the mechanism itself should be
called something, I think.  When you say "varlena header" or "TOAST
pointer" that is a name for a very particular thing, not just a
general category of things you might do.  If we replaced every
instance of "TOAST pointer" to "reference to where the full value is
stored", it would be much less clear, and naming all of the related
functions would be harder.

>> This is a clever
>> representation, but it's hard to wrap your head around, and I'm not
>> sure "primary" and "secondary" are the best names, although I don't
>> have an idea as to what would be better.  I'm a bit confused, though:
>> once you give out a secondary pointer, how is it safe to write the
>> object through the primary pointer?
>
> It's no different from allowing plpgsql to update the values of variables
> of pass-by-reference types even though it has previously given out Datums
> that are pointers to them: by the time we're ready to execute an
> assignment, any query execution that had such a pointer is over and done
> with.  (This implies that cursor parameters have to be physically copied
> into the cursor's execution state, which is one of a depressingly large
> number of reasons why datumCopy() has to physically copy a deserialized
> value rather than just copying the pointer.  But otherwise it works.)

OK, I see.  So giving out a secondary pointer doesn't necessarily
preclude further changes via the primary pointer, but you'd better be
sure that you don't try until such time as all of those secondary
references are gone.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  12. Re: Manipulating complex types as non-contiguous structures in-memory

    Martijn van Oosterhout <kleptog@svana.org> — 2015-02-13T08:04:54Z

    On Thu, Feb 12, 2015 at 08:52:56AM -0500, Robert Haas wrote:
> > BTW, I'm not all that thrilled with the "deserialized object" terminology.
> > I found myself repeatedly tripping up on which form was serialized and
> > which de-.  If anyone's got a better naming idea I'm willing to adopt it.
> 
> My first thought is that we should form some kind of TOAST-like
> backronym, like Serialization Avoidance Loading and Access Device
> (SALAD) or Break-up, Read, Edit, Assemble, and Deposit (BREAD).  I
> don't think there is anything per se wrong with the terms
> serialization and deserialization; indeed, I used the same ones in the
> parallel-mode stuff.  But they are fairly general terms, so it might
> be nice to have something more specific that applies just to this
> particular usage.

The words that sprung to mind for me were: packed/unpacked.

Have a nice day,
-- 
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> He who writes carelessly confesses thereby at the very outset that he does
> not attach much importance to his own thoughts.
   -- Arthur Schopenhauer

  13. Re: Manipulating complex types as non-contiguous structures in-memory

    Jim Nasby <jim.nasby@bluetreble.com> — 2015-02-13T23:40:43Z

    On 2/13/15 2:04 AM, Martijn van Oosterhout wrote:
> On Thu, Feb 12, 2015 at 08:52:56AM -0500, Robert Haas wrote:
>>> > >BTW, I'm not all that thrilled with the "deserialized object" terminology.
>>> > >I found myself repeatedly tripping up on which form was serialized and
>>> > >which de-.  If anyone's got a better naming idea I'm willing to adopt it.
>> >
>> >My first thought is that we should form some kind of TOAST-like
>> >backronym, like Serialization Avoidance Loading and Access Device
>> >(SALAD) or Break-up, Read, Edit, Assemble, and Deposit (BREAD).  I
>> >don't think there is anything per se wrong with the terms
>> >serialization and deserialization; indeed, I used the same ones in the
>> >parallel-mode stuff.  But they are fairly general terms, so it might
>> >be nice to have something more specific that applies just to this
>> >particular usage.
> The words that sprung to mind for me were: packed/unpacked.

+1

After thinking about it, I don't think having a more distinctive name 
(like TOAST) is necessary for this feature. TOAST is something that's 
rather visible to end users, whereas packing would only matter to 
someone creating a new varlena type.
-- 
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

  14. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-02-14T15:45:17Z

    Martijn van Oosterhout <kleptog@svana.org> writes:
> On Thu, Feb 12, 2015 at 08:52:56AM -0500, Robert Haas wrote:
>>> BTW, I'm not all that thrilled with the "deserialized object" terminology.
>>> I found myself repeatedly tripping up on which form was serialized and
>>> which de-.  If anyone's got a better naming idea I'm willing to adopt it.

>> My first thought is that we should form some kind of TOAST-like
>> backronym, like Serialization Avoidance Loading and Access Device
>> (SALAD) or Break-up, Read, Edit, Assemble, and Deposit (BREAD).  I
>> don't think there is anything per se wrong with the terms
>> serialization and deserialization; indeed, I used the same ones in the
>> parallel-mode stuff.  But they are fairly general terms, so it might
>> be nice to have something more specific that applies just to this
>> particular usage.

> The words that sprung to mind for me were: packed/unpacked.

Trouble is that we're already using "packed" with a specific connotation
in that same area of the code, namely for short-header varlena values.
(See pg_detoast_datum_packed() etc.)  So I don't think this will work.
But maybe a different adjective?

			regards, tom lane

  15. Re: Manipulating complex types as non-contiguous structures in-memory

    Robert Haas <robertmhaas@gmail.com> — 2015-02-14T17:24:57Z

    On Sat, Feb 14, 2015 at 10:45 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Martijn van Oosterhout <kleptog@svana.org> writes:
>> On Thu, Feb 12, 2015 at 08:52:56AM -0500, Robert Haas wrote:
>>>> BTW, I'm not all that thrilled with the "deserialized object" terminology.
>>>> I found myself repeatedly tripping up on which form was serialized and
>>>> which de-.  If anyone's got a better naming idea I'm willing to adopt it.
>
>>> My first thought is that we should form some kind of TOAST-like
>>> backronym, like Serialization Avoidance Loading and Access Device
>>> (SALAD) or Break-up, Read, Edit, Assemble, and Deposit (BREAD).  I
>>> don't think there is anything per se wrong with the terms
>>> serialization and deserialization; indeed, I used the same ones in the
>>> parallel-mode stuff.  But they are fairly general terms, so it might
>>> be nice to have something more specific that applies just to this
>>> particular usage.
>
>> The words that sprung to mind for me were: packed/unpacked.
>
> Trouble is that we're already using "packed" with a specific connotation
> in that same area of the code, namely for short-header varlena values.
> (See pg_detoast_datum_packed() etc.)  So I don't think this will work.
> But maybe a different adjective?

expanded?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  16. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-02-14T17:53:58Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, Feb 14, 2015 at 10:45 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Martijn van Oosterhout <kleptog@svana.org> writes:
>>> The words that sprung to mind for me were: packed/unpacked.

>> Trouble is that we're already using "packed" with a specific connotation
>> in that same area of the code, namely for short-header varlena values.
>> (See pg_detoast_datum_packed() etc.)  So I don't think this will work.
>> But maybe a different adjective?

> expanded?

That seems to work from the standpoint of not conflicting with other
nearby usages in our code, and it's got the right semantics I think.

Any other suggestions out there?  Otherwise I'll probably go with this.

			regards, tom lane

  17. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-02-15T23:41:47Z

    Here's an updated version of the patch I sent before.  Notable changes:

* I switched over to calling "deserialized" objects "expanded" objects,
and the default representation is now called "flat" or "flattened" instead
of "reserialized".  Per suggestion from Robert.

* I got rid of the bit about detecting read-write pointers by address
comparison.  Instead there are now two vartag values for R/W and R/O
pointers.  After further reflection I concluded that my previous worry
about wanting copied pointers to automatically become read-only was
probably wrong, so there's no need for extra confusion here.

* I added support for extracting array elements from expanded values
(array_ref).

* I hacked plpgsql to force values of array-type variables into expanded
form; this is needed to get any win from the array_ref change if the
function doesn't do any assignments to elements of the array.  This is an
improvement over the original patch, which hardwired array_set to force
expansion, but I remain unsatisfied with it as a long-term answer.  It's
not clear that it's always a win to do this (but the tradeoff will change
as we convert more array support functions to handle expanded inputs, so
it's probably not worth getting too excited about that aspect of it yet).
A bigger complaint is that this approach cannot fix things for non-builtin
types such as hstore.  I'm hesitant to add a pg_type column carrying an
expansion function OID, but there may be no other workable answer for
extension types.

The patch as it stands is able to do nice things with

create or replace function arraysetnum(n int) returns numeric[] as $$
declare res numeric[] := '{}';
begin
  for i in 1 .. n loop
    res[i] := i;
  end loop;
  return res;
end
$$ language plpgsql strict;

create or replace function arraysumnum(arr numeric[]) returns numeric as $$
declare res numeric := 0;
begin
  for i in array_lower(arr, 1) .. array_upper(arr, 1) loop
    res := res + arr[i];
  end loop;
  return res;
end
$$ language plpgsql strict;

regression=# select arraysumnum(arraysetnum(100000));
 arraysumnum 
-------------
  5000050000
(1 row)

Time: 304.336 ms

(versus approximately 1 minute in 9.4, although these numbers are for
cassert builds so should be taken with a grain of salt.)  There are
still a couple more flattening/expansion conversions than I'd like,
in particular the array returned by arraysetnum() gets flattened on its
way out, which would be good to avoid.

I'm going to stick this into the commitfest even though it's not really
close to being committable; I see some other people doing likewise with
their pet patches ;-).  What it could particularly do with some reviewing
help on is exploring the performance changes it creates; what cases does
it make substantially worse?

			regards, tom lane

  18. Re: Manipulating complex types as non-contiguous structures in-memory

    Robert Haas <robertmhaas@gmail.com> — 2015-02-15T23:47:41Z

    On Sun, Feb 15, 2015 at 6:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm going to stick this into the commitfest even though it's not really
> close to being committable; I see some other people doing likewise with
> their pet patches ;-).  What it could particularly do with some reviewing
> help on is exploring the performance changes it creates; what cases does
> it make substantially worse?

It's perfectly reasonable to add stuff that isn't committable yet to
the CF app; the point of the CF app is to track what needs reviewing.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  19. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-02-17T04:54:34Z

    Attached is an 0.3 version, rebased over today's HEAD changes (applies to
commit 9e3ad1aac52454569393a947c06be0d301749362 or later), and with some
better logic for transferring expanded array values into and out of plpgsql
functions.  Using this example:

create or replace function arraysetnum(n int) returns numeric[] as $$
declare res numeric[] := '{}';
begin
  for i in 1 .. n loop
    res[i] := i;
  end loop;
  return res;
end
$$ language plpgsql strict;

create or replace function arraysumnum(arr numeric[]) returns numeric as $$
declare res numeric := 0;
begin
  for i in array_lower(arr, 1) .. array_upper(arr, 1) loop
    res := res + arr[i];
  end loop;
  return res;
end
$$ language plpgsql strict;

create or replace function arraytimenum(n int) returns numeric as $$
declare tmp numeric[];
begin
  tmp := arraysetnum(n);
  return arraysumnum(tmp);
end
$$ language plpgsql strict;

either of the test cases

select arraysumnum(arraysetnum(100000));
select arraytimenum(100000);

involve exactly one coercion from flat to expanded array (during the
initial assignment of the '{}' constant to the "res" variable), no
coercions from expanded to flat, and no bulk copy operations.

So I'm starting to feel fairly good about this.  Obviously there's a
nontrivial amount of work to do with integrating the array-code changes
and teaching the rest of the array functions about expanded arrays (or
at least as many of them as seem performance-critical).  But that looks
like just a few days of basically-mechanical effort.  A larger question
is what we ought to do about extending the array-favoring hacks in plpgsql
to support this type of optimization for non-built-in types.

Realize that what this patch is able to improve are basically two types
of cases:

* nests of function calls that take and return the same complex datatype,
think foo(bar(baz(x))), where x is stored in some flat format but foo()
bar() and baz() all agree on an expanded format that's easier to process.

* plpgsql variables stored in an expanded format that's easier to process
for most functions that might work with their values.

The first case can be implemented by mutual agreement among the functions
of the datatype; it does not need any additional help beyond what's in
this patch.  But the second case does not work very well unless plpgsql
takes some proactive step to force variable values into the expanded
format.  Otherwise you get a win only if the last assignment to the
variable happened to come from a source that supplied a read-write
expanded value.  You can make that happen with appropriate coding in
the plpgsql function, of course, but it's klugy to have to do that.

I would not be ashamed to ship this in 9.5 as just an array optimization
and leave the larger question for next time ... but it does feel a bit
unfinished like this.  OTOH, I'm not sure whether the PostGIS folk care
all that much about the intermediate-values-in-plpgsql-variables
scenario.  They didn't bring it up in the discussion a year or so back
about their requirements.  We do know very well that plpgsql array
variables are a performance pain point, so maybe fixing that is enough
of a goal for 9.5.

(BTW, the nested-function-calls case sure seems like it's dead center
of the wheelhouse for JSONB.  Just sayin'.  I do not myself have time
to think about applying this technology to JSONB right now, but does
anyone else want to step up?)

			regards, tom lane

  20. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-02-19T21:32:12Z

    Here's an 0.4 version, in which I've written some user docs, refactored
the array-specific code into a more reasonable arrangement, and adjusted
a lot of the built-in array functions to support expanded arrays directly.
This is about as far as I feel a need to take the latter activity, at
least for now; there are a few remaining operations that might be worth
converting but it's not clear they'd really offer much benefit.

I think this is actually now a serious candidate to be committed as-is,
not just a prototype.  What we lack though is a clear understanding of
the performance characteristics.

			regards, tom lane

  21. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-02-26T20:32:35Z

    I wrote:
> Here's an 0.4 version, in which I've written some user docs, refactored
> the array-specific code into a more reasonable arrangement, and adjusted
> a lot of the built-in array functions to support expanded arrays directly.
> This is about as far as I feel a need to take the latter activity, at
> least for now; there are a few remaining operations that might be worth
> converting but it's not clear they'd really offer much benefit.

Attached is an updated version.  Aside from rebasing over some recent
commits that touched the same areas, this improves one more case, which
is plpgsql arrays with typmods.  I noticed that while

create or replace function arraysetnum(n int) returns numeric[] as $$
declare res numeric[] := '{}';
begin
  for i in 1 .. n loop
    res[i] := i;
  end loop;
  return res;
end
$$ language plpgsql strict;

was nicely speedy, performance went back in the toilet again as soon as
you stuck a typmod onto the array, for example

declare res numeric(20,0)[] := '{}';

The reason is that exec_cast_value would then insist on feeding the whole
array through I/O conversion to apply the typmod :-(.  This was in fact
completely useless activity because we had already carefully applied the
typmod to the new array element; but exec_cast_value didn't know that.
In the attached patch, I've dealt with this by teaching exec_eval_expr,
exec_assign_value, exec_cast_value, etc to track typmods not just type
OIDs.  In this way we avoid a useless conversion whenever a value is known
to match the desired typmod already.  This is probably something that
should've been done to plpgsql a very long time ago; the overhead is
really minimal and the potential savings when dealing with
length-constrained variables is significant.

			regards, tom lane

  22. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-03-28T21:24:36Z

    I wrote:
> [ expanded-arrays-1.0.patch ]

This is overdue for a rebase; attached.  No functional changes, but some
of what was in the original patch has already been merged, and other parts
were superseded.

			regards, tom lane

  23. Re: Manipulating complex types as non-contiguous structures in-memory

    Heikki Linnakangas <hlinnaka@iki.fi> — 2015-04-17T09:23:41Z

    On 03/28/2015 11:24 PM, Tom Lane wrote:
> + /*
> +  * Macros for iterating through elements of a flat or expanded array.
> +  * Use "ARRAY_ITER  ARRAY_ITER_VARS(name);" to declare the local variables
> +  * needed for an iterator (more than one set can be used in the same function,
> +  * if they have different names).
> +  * Use "ARRAY_ITER_SETUP(name, arrayptr);" to prepare to iterate, and
> +  * "ARRAY_ITER_NEXT(name, index, datumvar, isnullvar, ...);" to fetch the
> +  * next element into datumvar/isnullvar.  "index" must be the zero-origin
> +  * element number; we make caller provide this since caller is generally
> +  * counting the elements anyway.
> +  */
> + #define ARRAY_ITER				/* dummy type name to keep pgindent happy */
> +
> + #define ARRAY_ITER_VARS(iter) \
> + 	Datum	   *iter##datumptr; \
> + 	bool	   *iter##isnullptr; \
> + 	char	   *iter##dataptr; \
> + 	bits8	   *iter##bitmapptr; \
> + 	int			iter##bitmask

How about a struct instead?

struct ArrayIter {
     Datum   datumptr;
     bool   isnullptr;
     char   dataptr;
     bits8  bitmapptr;
     int	   bitmask
}

Seems more natural.

> + #define ARRAY_ITER_SETUP(iter, arrayptr) \
> [long and complicated macro]
> +
> + #define ARRAY_ITER_NEXT(iter,i, datumvar,isnullvar, elmlen,elmbyval,elmalign) \
> [another long and complicated macro]

How about turning these into functions? We have a bunch of macros like 
this, but IMHO functions are much more readable and easier to debug, so 
would prefer functions in new code.

In general, refactoring the array iteration code to a macro/function 
like this is a good idea. It would make sense to commit that separately, 
regardless of the rest of the patch.

- Heikki

  24. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-04-17T12:58:02Z

    Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 03/28/2015 11:24 PM, Tom Lane wrote:
>> +  * Macros for iterating through elements of a flat or expanded array.

> How about a struct instead?

> struct ArrayIter {
>      Datum   datumptr;
>      bool   isnullptr;
>      char   dataptr;
>      bits8  bitmapptr;
>      int	   bitmask
> }

> Seems more natural.

Yes, and much less efficient I'm afraid.  Most compilers would be unable
to put the variables into registers, which is important for these inner
loops.

> How about turning these into functions?

Likewise.  The point of doing it like this was to avoid taking an
efficiency hit compared to the existing code.

It's conceivable that we could avoid such a hit by marking the functions
all "inline", but I'm not certain that they'd get inlined, and the
question of whether the variables could be in registers would remain.

			regards, tom lane

  25. Re: Manipulating complex types as non-contiguous structures in-memory

    Heikki Linnakangas <hlinnaka@iki.fi> — 2015-04-17T14:21:58Z

    On 04/17/2015 03:58 PM, Tom Lane wrote:
> Heikki Linnakangas <hlinnaka@iki.fi> writes:
>> On 03/28/2015 11:24 PM, Tom Lane wrote:
>>> +  * Macros for iterating through elements of a flat or expanded array.
>
>> How about a struct instead?
>
>> struct ArrayIter {
>>       Datum   datumptr;
>>       bool   isnullptr;
>>       char   dataptr;
>>       bits8  bitmapptr;
>>       int	   bitmask
>> }
>
>> Seems more natural.
>
> Yes, and much less efficient I'm afraid.  Most compilers would be unable
> to put the variables into registers, which is important for these inner
> loops.

That would surprise me. Surely most compilers know to keep fields of a 
struct in registers, when the struct itself or a pointer to it is not 
passed anywhere.

>> How about turning these into functions?
>
> Likewise.  The point of doing it like this was to avoid taking an
> efficiency hit compared to the existing code.
>
> It's conceivable that we could avoid such a hit by marking the functions
> all "inline", but I'm not certain that they'd get inlined, and the
> question of whether the variables could be in registers would remain.

Ok, this one I believe.

- Heikki

  26. Re: Manipulating complex types as non-contiguous structures in-memory

    Andres Freund <andres@anarazel.de> — 2015-05-01T15:37:20Z

    On 2015-03-28 17:24:36 -0400, Tom Lane wrote:
> I wrote:
> > [ expanded-arrays-1.0.patch ]
> 
> This is overdue for a rebase; attached.  No functional changes, but some
> of what was in the original patch has already been merged, and other parts
> were superseded.

What are your plans with this WRT 9.5?

Andres

  27. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-05-01T16:35:08Z

    Andres Freund <andres@anarazel.de> writes:
> On 2015-03-28 17:24:36 -0400, Tom Lane wrote:
>> This is overdue for a rebase; attached.  No functional changes, but some
>> of what was in the original patch has already been merged, and other parts
>> were superseded.

> What are your plans with this WRT 9.5?

I'd like to get it committed into 9.5.  I've been hoping somebody would do
a performance review.

			regards, tom lane

  28. Re: Manipulating complex types as non-contiguous structures in-memory

    Andres Freund <andres@anarazel.de> — 2015-05-01T16:39:10Z

    On 2015-05-01 09:35:08 -0700, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > What are your plans with this WRT 9.5?
> 
> I'd like to get it committed into 9.5.  I've been hoping somebody would do
> a performance review.

Ok. I'll try to have a look, but it'll be the second half of next week.

Greetings,

Andres Freund

  29. Re: Manipulating complex types as non-contiguous structures in-memory

    Pavel Stehule <pavel.stehule@gmail.com> — 2015-05-01T17:47:32Z

    2015-05-01 18:35 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:

> Andres Freund <andres@anarazel.de> writes:
> > On 2015-03-28 17:24:36 -0400, Tom Lane wrote:
> >> This is overdue for a rebase; attached.  No functional changes, but some
> >> of what was in the original patch has already been merged, and other
> parts
> >> were superseded.
>
> > What are your plans with this WRT 9.5?
>
> I'd like to get it committed into 9.5.  I've been hoping somebody would do
> a performance review.
>

I am looking on this patch, but it cannot be applied now.

lxml2 -lssl -lcrypto -lrt -lcrypt -ldl -lm -o postgres
utils/fmgrtab.o:(.rodata+0x2678): undefined reference to `array_append'
utils/fmgrtab.o:(.rodata+0x2698): undefined reference to `array_prepend'
collect2: error: ld returned 1 exit status
Makefile:57: recipe for target 'postgres' failed
make[2]: *** [postgres] Error 1
make[2]: Leaving directory '/home/pavel/src/postgresql/src/backend'
Makefile:34: recipe for target 'all-backend-recurse' failed
make[1]: *** [all-backend-recurse] Error 2
make[1]: Leaving directory '/home/pavel/src/postgresql/src'
GNUmakefile:11: recipe for target 'all-src-recurse' failed


Regards

Pavel


>
>                         regards, tom lane
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

  30. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-05-01T18:11:14Z

    Pavel Stehule <pavel.stehule@gmail.com> writes:
> I am looking on this patch, but it cannot be applied now.

> lxml2 -lssl -lcrypto -lrt -lcrypt -ldl -lm -o postgres
> utils/fmgrtab.o:(.rodata+0x2678): undefined reference to `array_append'
> utils/fmgrtab.o:(.rodata+0x2698): undefined reference to `array_prepend'

What are you trying to apply it to?  I see array_append() in
src/backend/utils/adt/array_userfuncs.c in HEAD.  Also, are
you checking the 1.1 version of the patch?

			regards, tom lane

  31. Re: Manipulating complex types as non-contiguous structures in-memory

    Andres Freund <andres@anarazel.de> — 2015-05-01T18:19:40Z

    On 2015-05-01 11:11:14 -0700, Tom Lane wrote:
> Pavel Stehule <pavel.stehule@gmail.com> writes:
> > I am looking on this patch, but it cannot be applied now.
> 
> > lxml2 -lssl -lcrypto -lrt -lcrypt -ldl -lm -o postgres
> > utils/fmgrtab.o:(.rodata+0x2678): undefined reference to `array_append'
> > utils/fmgrtab.o:(.rodata+0x2698): undefined reference to `array_prepend'
> 
> What are you trying to apply it to?  I see array_append() in
> src/backend/utils/adt/array_userfuncs.c in HEAD.  Also, are
> you checking the 1.1 version of the patch?

That's very likely due to the transforms patch, with added another
column to pg_proc...

Greetings,

Andres Freund

  32. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-05-01T18:39:12Z

    Andres Freund <andres@anarazel.de> writes:
> On 2015-05-01 11:11:14 -0700, Tom Lane wrote:
>> What are you trying to apply it to?  I see array_append() in
>> src/backend/utils/adt/array_userfuncs.c in HEAD.  Also, are
>> you checking the 1.1 version of the patch?

> That's very likely due to the transforms patch, with added another
> column to pg_proc...

No, my patch doesn't touch pg_proc.h.  I'm certainly prepared to believe
it's suffered bit rot in the last couple of weeks, but I don't understand
how it would apply successfully and then generate a complaint about
array_append not being there.  array_append *is* there in HEAD, and has
been for awhile.

			regards, tom lane

  33. Re: Manipulating complex types as non-contiguous structures in-memory

    Pavel Stehule <pavel.stehule@gmail.com> — 2015-05-01T18:53:05Z

    2015-05-01 20:11 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:

> Pavel Stehule <pavel.stehule@gmail.com> writes:
> > I am looking on this patch, but it cannot be applied now.
>
> > lxml2 -lssl -lcrypto -lrt -lcrypt -ldl -lm -o postgres
> > utils/fmgrtab.o:(.rodata+0x2678): undefined reference to `array_append'
> > utils/fmgrtab.o:(.rodata+0x2698): undefined reference to `array_prepend'
>
> What are you trying to apply it to?  I see array_append() in
> src/backend/utils/adt/array_userfuncs.c in HEAD.  Also, are
> you checking the 1.1 version of the patch?
>

I tested old version. 1.1. looks well.

Regards

Pavel


>
>                         regards, tom lane
>

  34. Re: Manipulating complex types as non-contiguous structures in-memory

    Pavel Stehule <pavel.stehule@gmail.com> — 2015-05-01T19:16:52Z

    2015-05-01 20:53 GMT+02:00 Pavel Stehule <pavel.stehule@gmail.com>:

>
>
> 2015-05-01 20:11 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
>
>> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> > I am looking on this patch, but it cannot be applied now.
>>
>> > lxml2 -lssl -lcrypto -lrt -lcrypt -ldl -lm -o postgres
>> > utils/fmgrtab.o:(.rodata+0x2678): undefined reference to `array_append'
>> > utils/fmgrtab.o:(.rodata+0x2698): undefined reference to `array_prepend'
>>
>> What are you trying to apply it to?  I see array_append() in
>> src/backend/utils/adt/array_userfuncs.c in HEAD.  Also, are
>> you checking the 1.1 version of the patch?
>>
>
> I tested old version. 1.1. looks well.
>

It is hard to believe how it is fast

I use buble sort for plpgsql benchmarking. Following variant is suboptimal
(but it is perfect for this test)

CREATE OR REPLACE FUNCTION public.buble(a anyarray, OUT r anyarray)
 RETURNS anyarray
 LANGUAGE plpgsql
AS $function$
DECLARE
  aux r%type;
  sorted bool := false;
BEGIN
  r := a;
  WHILE NOT sorted
  LOOP
    sorted := true;
    FOR i IN array_lower(a,1) .. array_upper(a,1) - 1
    LOOP
      IF r[i] > r[i+1] THEN
        sorted := false;
        aux[1] := r[i];
        r[i] := r[i+1]; r[i+1] := aux[1];
      END IF;
    END LOOP;
  END LOOP;
END;
$function$

CREATE OR REPLACE FUNCTION public.array_generator(integer, anyelement, OUT
r anyarray)
 RETURNS anyarray
 LANGUAGE plpgsql
AS $function$
BEGIN
  r := (SELECT ARRAY(SELECT random()*$2 FROM generate_series(1,$1)));
END;
$function$

Test for 3000 elements:

                   Original     Patch
Integer            55sec      8sec
Numeric        341sec      8sec

Quicksort is about 3x faster -- so a benefit of this patch is clear.

Regards

Pavel


>
> Regards
>
> Pavel
>
>
>>
>>                         regards, tom lane
>>
>
>

  35. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-05-01T19:59:21Z

    Pavel Stehule <pavel.stehule@gmail.com> writes:
> Test for 3000 elements:

>                    Original     Patch
> Integer            55sec      8sec
> Numeric           341sec      8sec

> Quicksort is about 3x faster -- so a benefit of this patch is clear.

Yeah, the patch should pretty much blow the doors off any case that's
heavily dependent on access or update of individual array elements ...
especially for arrays with variable-length element type, such as numeric.

What I'm concerned about is that it could make things *slower* for
scenarios where that isn't the main thing being done with the arrays,
as a result of useless conversions between "flat" and "expanded"
array formats.  So what we need is to try to benchmark some cases
that don't involve single-element operations but rather whole-array
operations (on arrays that are plpgsql variables), and see if those
cases have gotten noticeably worse.

			regards, tom lane

  36. Re: Manipulating complex types as non-contiguous structures in-memory

    Pavel Stehule <pavel.stehule@gmail.com> — 2015-05-03T19:18:24Z

    Hi

I did some test with unlogged table in shared buffers

foo(a int[]);  -- 2K long array 100K rows

for queries

select max(v) from (unnest(a) from foo) x;
select max(a[1]) from foo
select max(a[2000]) from foo

I didn't find significant slowdown.

Some slowdown is visible (about 10%) for query

update foo set a = a || 1;

Significant slowdown is on following test:

do $$ declare a int[] := '{}'; begin for i in 1..90000 loop a := a || 10;
end loop; end$$ language plpgsql;
do $$ declare a numeric[] := '{}'; begin for i in 1..90000 loop a := a ||
10.1; end loop; end$$ language plpgsql;

integer master 14sec x patched 55sec
numeric master 43sec x patched 108sec

It is probably worst case - and it is known plpgsql antipattern

Regards

Pavel



2015-05-01 21:59 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:

> Pavel Stehule <pavel.stehule@gmail.com> writes:
> > Test for 3000 elements:
>
> >                    Original     Patch
> > Integer            55sec      8sec
> > Numeric           341sec      8sec
>
> > Quicksort is about 3x faster -- so a benefit of this patch is clear.
>
> Yeah, the patch should pretty much blow the doors off any case that's
> heavily dependent on access or update of individual array elements ...
> especially for arrays with variable-length element type, such as numeric.
>
> What I'm concerned about is that it could make things *slower* for
> scenarios where that isn't the main thing being done with the arrays,
> as a result of useless conversions between "flat" and "expanded"
> array formats.  So what we need is to try to benchmark some cases
> that don't involve single-element operations but rather whole-array
> operations (on arrays that are plpgsql variables), and see if those
> cases have gotten noticeably worse.
>
>                         regards, tom lane
>

  37. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-05-03T19:57:43Z

    Pavel Stehule <pavel.stehule@gmail.com> writes:
> Some slowdown is visible (about 10%) for query

> update foo set a = a || 1;

> Significant slowdown is on following test:

> do $$ declare a int[] := '{}'; begin for i in 1..90000 loop a := a || 10;
> end loop; end$$ language plpgsql;
> do $$ declare a numeric[] := '{}'; begin for i in 1..90000 loop a := a ||
> 10.1; end loop; end$$ language plpgsql;

> integer master 14sec x patched 55sec
> numeric master 43sec x patched 108sec

> It is probably worst case - and it is known plpgsql antipattern

Yeah, I have not expended a great deal of effort on the array_append/
array_prepend/array_cat code paths.  Still, in these plpgsql cases,
we should in principle have gotten down from two array copies per loop to
one, so it's disappointing to not have better results there, even granting
that the new "copy" step is not just a byte-by-byte copy.  Let me see if
there's anything simple to be done about that.

			regards, tom lane

  38. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-05-05T22:50:13Z

    I wrote:
> Pavel Stehule <pavel.stehule@gmail.com> writes:
>> Significant slowdown is on following test:

>> do $$ declare a int[] := '{}'; begin for i in 1..90000 loop a := a || 10;
>> end loop; end$$ language plpgsql;
>> do $$ declare a numeric[] := '{}'; begin for i in 1..90000 loop a := a ||
>> 10.1; end loop; end$$ language plpgsql;

>> integer master 14sec x patched 55sec
>> numeric master 43sec x patched 108sec

>> It is probably worst case - and it is known plpgsql antipattern

> Yeah, I have not expended a great deal of effort on the array_append/
> array_prepend/array_cat code paths.  Still, in these plpgsql cases,
> we should in principle have gotten down from two array copies per loop to
> one, so it's disappointing to not have better results there, even granting
> that the new "copy" step is not just a byte-by-byte copy.  Let me see if
> there's anything simple to be done about that.

The attached updated patch reduces both of those do-loop tests to about
60 msec on my machine.  It contains two improvements over the 1.1 patch:

1. There's a fast path for copying an expanded array to another expanded
array when the element type is pass-by-value: we can just memcpy the
Datum array instead of working element-by-element.  In isolation, that
change made the patch a little faster than 9.4 on your int-array case,
though of course it doesn't help for the numeric-array case (and I do not
see a way to avoid working element-by-element for pass-by-ref cases).

2. pl/pgsql now detects cases like "a := a || x" and allows the array "a"
to be passed as a read-write pointer to array_append, so that array_append
can modify expanded arrays in-place and avoid inessential data copying
altogether.  (The earlier patch had made array_append and array_prepend
safe for this usage, but there wasn't actually any way to invoke them
with read-write pointers.)  I had speculated about doing this in my
earliest discussion of this patch, but there was no code for it before.

The key question for change #2 is how do we identify what is a "safe"
top-level function that can be trusted not to corrupt the read-write value
if it fails partway through.  I did not have a good answer before, and
I still don't; what this version of the patch does is to hard-wire
array_append and array_prepend as the functions considered safe.
Obviously that is crying out for improvement, but we can leave that
question for later; at least now we have infrastructure that makes it
possible to do it.

Change #1 is actually not relevant to these example cases, because we
don't copy any arrays within the loop given change #2.  But I left it in
because it's not much code and it will help for situations where change #2
doesn't apply.

			regards, tom lane

  39. Re: Manipulating complex types as non-contiguous structures in-memory

    Pavel Stehule <pavel.stehule@gmail.com> — 2015-05-06T08:20:12Z

    2015-05-06 0:50 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:

> I wrote:
> > Pavel Stehule <pavel.stehule@gmail.com> writes:
> >> Significant slowdown is on following test:
>
> >> do $$ declare a int[] := '{}'; begin for i in 1..90000 loop a := a ||
> 10;
> >> end loop; end$$ language plpgsql;
> >> do $$ declare a numeric[] := '{}'; begin for i in 1..90000 loop a := a
> ||
> >> 10.1; end loop; end$$ language plpgsql;
>
> >> integer master 14sec x patched 55sec
> >> numeric master 43sec x patched 108sec
>
> >> It is probably worst case - and it is known plpgsql antipattern
>
> > Yeah, I have not expended a great deal of effort on the array_append/
> > array_prepend/array_cat code paths.  Still, in these plpgsql cases,
> > we should in principle have gotten down from two array copies per loop to
> > one, so it's disappointing to not have better results there, even
> granting
> > that the new "copy" step is not just a byte-by-byte copy.  Let me see if
> > there's anything simple to be done about that.
>
> The attached updated patch reduces both of those do-loop tests to about
> 60 msec on my machine.  It contains two improvements over the 1.1 patch:
>
> 1. There's a fast path for copying an expanded array to another expanded
> array when the element type is pass-by-value: we can just memcpy the
> Datum array instead of working element-by-element.  In isolation, that
> change made the patch a little faster than 9.4 on your int-array case,
> though of course it doesn't help for the numeric-array case (and I do not
> see a way to avoid working element-by-element for pass-by-ref cases).
>
> 2. pl/pgsql now detects cases like "a := a || x" and allows the array "a"
> to be passed as a read-write pointer to array_append, so that array_append
> can modify expanded arrays in-place and avoid inessential data copying
> altogether.  (The earlier patch had made array_append and array_prepend
> safe for this usage, but there wasn't actually any way to invoke them
> with read-write pointers.)  I had speculated about doing this in my
> earliest discussion of this patch, but there was no code for it before.
>
> The key question for change #2 is how do we identify what is a "safe"
> top-level function that can be trusted not to corrupt the read-write value
> if it fails partway through.  I did not have a good answer before, and
> I still don't; what this version of the patch does is to hard-wire
> array_append and array_prepend as the functions considered safe.
> Obviously that is crying out for improvement, but we can leave that
> question for later; at least now we have infrastructure that makes it
> possible to do it.
>
> Change #1 is actually not relevant to these example cases, because we
> don't copy any arrays within the loop given change #2.  But I left it in
> because it's not much code and it will help for situations where change #2
> doesn't apply.
>

I can confirm this speedup - pretty nice.

Multidimensional append is slower 2x .. but it is probably corner case

declare a int[] := '{}'; begin for i in 1..90000 loop a := a || ARRAY[[i
]]; end loop; raise notice '%', 'aa'; end$$ language plpgsql;

but this optimization doesn't work for code - that is semantically same
like a || i;

declare a int[] := '{}'; begin for i in 1..90000 loop a := a || ARRAY[i ];
end loop; raise notice '%', 'aa'; end$$ language plpgsql;

So there is some to much sensible

There are slowdown with MD arrays, but it is not typical use case, and the
speedup is about 5-10x and faster - so I'll be very happy if this patch
will be in 9.5

Regards

Pavel


>
>                         regards, tom lane
>
>

  40. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-05-06T13:50:09Z

    Pavel Stehule <pavel.stehule@gmail.com> writes:
> Multidimensional append is slower 2x .. but it is probably corner case

> declare a int[] := '{}'; begin for i in 1..90000 loop a := a || ARRAY[[i
> ]]; end loop; raise notice '%', 'aa'; end$$ language plpgsql;

Yeah, that's array_cat(), which I've not done anything with.  I'm not
really excited about adding code to it; I think use-cases like this one
are probably too uncommon to justify more code.  In any case we could
go back and improve it later if there are enough complaints.

Another way to look at it is that in this example, plpgsql's attempts to
force the "a" array into expanded form are a mistake: we never get any
benefit because array_cat() just wants it in flat form again, and delivers
it in flat form.  (It's likely that this is an unrealistic worst case:
it's hard to imagine real array-using applications that never do any
element-by-element access.)  Possibly we could improve matters with a more
refined heuristic about whether to force arrays to expanded form during
assignments --- but I'm not sure what that would look like.  plpgsql has
very little direct knowledge of which operations will be applied to the
array later.

			regards, tom lane

  41. Re: Manipulating complex types as non-contiguous structures in-memory

    Pavel Stehule <pavel.stehule@gmail.com> — 2015-05-06T14:35:32Z

    2015-05-06 15:50 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:

> Pavel Stehule <pavel.stehule@gmail.com> writes:
> > Multidimensional append is slower 2x .. but it is probably corner case
>
> > declare a int[] := '{}'; begin for i in 1..90000 loop a := a || ARRAY[[i
> > ]]; end loop; raise notice '%', 'aa'; end$$ language plpgsql;
>
> Yeah, that's array_cat(), which I've not done anything with.  I'm not
> really excited about adding code to it; I think use-cases like this one
> are probably too uncommon to justify more code.  In any case we could
> go back and improve it later if there are enough complaints.
>
> Another way to look at it is that in this example, plpgsql's attempts to
> force the "a" array into expanded form are a mistake: we never get any
> benefit because array_cat() just wants it in flat form again, and delivers
> it in flat form.  (It's likely that this is an unrealistic worst case:
> it's hard to imagine real array-using applications that never do any
> element-by-element access.)  Possibly we could improve matters with a more
> refined heuristic about whether to force arrays to expanded form during
> assignments --- but I'm not sure what that would look like.  plpgsql has
> very little direct knowledge of which operations will be applied to the
> array later.
>

Isn't better to push information about possible target to  function?

array_cat(a, b, result)
{
  if (undef(result))
     return a || b;

   if (b == result)
        array_extend(result, a);
        return result;
   else if (a == result)
        array_extend(result, b);
        return result;
   else
        return a || b;
}

It can be used for arrays, for strings?

On second hand it decrease readability related functions :( (but not all
functions should to support this optimization).

Regards

Pavel


>                         regards, tom lane
>

  42. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-05-06T16:54:01Z

    Pavel Stehule <pavel.stehule@gmail.com> writes:
> 2015-05-06 15:50 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
>> Another way to look at it is that in this example, plpgsql's attempts to
>> force the "a" array into expanded form are a mistake: we never get any
>> benefit because array_cat() just wants it in flat form again, and delivers
>> it in flat form.  (It's likely that this is an unrealistic worst case:
>> it's hard to imagine real array-using applications that never do any
>> element-by-element access.)  Possibly we could improve matters with a more
>> refined heuristic about whether to force arrays to expanded form during
>> assignments --- but I'm not sure what that would look like.  plpgsql has
>> very little direct knowledge of which operations will be applied to the
>> array later.

> Isn't better to push information about possible target to  function?

I don't think that would solve the problem.  For example, one of the cases
I worry about is a function that does read-only examination of an array
argument; consider something like

	create function sum_squares(a numeric[]) returns numeric as $$
	    declare s numeric := 0;
	    begin
	        for i in array_lower(a, 1) .. array_upper(a, 1) loop
		    s := s + a[i] * a[i];
		end loop;
		return s;
	    end;
	$$ language plpgsql strict immutable;

array_get_element() is not in a position here to force expansion of the
array variable, so unless plpgsql itself does something we're not going
to get a performance win (unless the argument happens to be already
expanded on arrival).

I'm inclined to think that we need to add information to pg_type about
whether a type supports expansion (and how to convert to expanded form
if so).  In the patch as it stands, plpgsql just has hard-wired knowledge
that it can call expand_array() on arrays that it's putting into function
local variables.  I'd be okay with shipping 9.5 like that, but pretty soon
we'll want a solution that extension data types can use too.

More generally, it'd be nice if the mechanism could be more flexible than
"always force variables of this type to expanded form".  But I don't see
how to teach plpgsql itself how to decide that intelligently, let alone
how we might design an API that lets some third-party data type decide it
at arm's length from plpgsql ...

			regards, tom lane

  43. Re: Manipulating complex types as non-contiguous structures in-memory

    Pavel Stehule <pavel.stehule@gmail.com> — 2015-05-06T18:39:00Z

    2015-05-06 18:54 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:

> Pavel Stehule <pavel.stehule@gmail.com> writes:
> > 2015-05-06 15:50 GMT+02:00 Tom Lane <tgl@sss.pgh.pa.us>:
> >> Another way to look at it is that in this example, plpgsql's attempts to
> >> force the "a" array into expanded form are a mistake: we never get any
> >> benefit because array_cat() just wants it in flat form again, and
> delivers
> >> it in flat form.  (It's likely that this is an unrealistic worst case:
> >> it's hard to imagine real array-using applications that never do any
> >> element-by-element access.)  Possibly we could improve matters with a
> more
> >> refined heuristic about whether to force arrays to expanded form during
> >> assignments --- but I'm not sure what that would look like.  plpgsql has
> >> very little direct knowledge of which operations will be applied to the
> >> array later.
>
> > Isn't better to push information about possible target to  function?
>
> I don't think that would solve the problem.  For example, one of the cases
> I worry about is a function that does read-only examination of an array
> argument; consider something like
>
>         create function sum_squares(a numeric[]) returns numeric as $$
>             declare s numeric := 0;
>             begin
>                 for i in array_lower(a, 1) .. array_upper(a, 1) loop
>                     s := s + a[i] * a[i];
>                 end loop;
>                 return s;
>             end;
>         $$ language plpgsql strict immutable;
>

I remember this issue

>
> array_get_element() is not in a position here to force expansion of the
> array variable, so unless plpgsql itself does something we're not going
> to get a performance win (unless the argument happens to be already
> expanded on arrival).
>
> I'm inclined to think that we need to add information to pg_type about
> whether a type supports expansion (and how to convert to expanded form
> if so).  In the patch as it stands, plpgsql just has hard-wired knowledge
> that it can call expand_array() on arrays that it's putting into function
> local variables.  I'd be okay with shipping 9.5 like that, but pretty soon
> we'll want a solution that extension data types can use too.
>
> More generally, it'd be nice if the mechanism could be more flexible than
> "always force variables of this type to expanded form".  But I don't see
> how to teach plpgsql itself how to decide that intelligently, let alone
> how we might design an API that lets some third-party data type decide it
> at arm's length from plpgsql ...
>

I agree - the core of work have to be elsewhere than in plpgsql. Some years
ago there was a idea about toast cache.



>
>                         regards, tom lane
>

  44. Re: Manipulating complex types as non-contiguous structures in-memory

    Andres Freund <andres@anarazel.de> — 2015-05-09T22:44:54Z

    Hi,

> The attached updated patch reduces both of those do-loop tests to about
> 60 msec on my machine.  It contains two improvements over the 1.1 patch:

Looking at this. First reading the patch to understand the details.

* The VARTAG_IS_EXPANDED(tag) trick in VARTAG_SIZE is unlikely to
  beneficial, before the compiler could implement the whole thing as a
  computed goto or lookup table, afterwards not.
* It'd be nice if the get_flat_size comment in expandeddatm.h could
  specify whether the header size is included. That varies enough around
  toast that it seems worthwhile.
* You were rather bothered by the potential of multiple evaluations for
  the ilist stuff. And now the AARR macros are full of them...
* I find the ARRAY_ITER_VARS/ARRAY_ITER_NEXT macros rather ugly. I don't
  buy the argument that turning them into functions will be slower. I'd
  bet the contrary on common platforms.
* Not a fan of the EH_ prefix in array_expanded.c and EOH_
  elsewhere. Just looks ugly to me. Whatever.
* The list of hardwired safe ops in exec_check_rw_parameter is somewhat
  sad. Don't have a better idea though.
* "Also, a C function that is modifying a read-write expanded value
  in-place should take care to leave the value in a sane state if it
  fails partway through." - that's a pretty hefty requirement imo.  I
  wonder if it'd not be possible to convert RW to RO if a value
  originates from outside an exception block.  IIRC that'd be useful for
  a bunch of other error cases we currently basically shrug away
  (something around toast and aborted xacts comes to mind).
* The forced RW->RO conversion in subquery scans is a bit sad, but I
  seems like something left for later.

These are more judgement calls than anything else...

Somewhere in the thread you comment on the fact that it's a bit sad that
plpgsql is the sole benefactor of this (unless some function forces
expansion internally).  I'd be ok to leave it at that for now. It'd be
quite cool to get some feedback from postgis folks about the suitability
of this for their cases.

I've not really looked into performance improvements around this,
choosing to look into somewhat reasonable cases where it'll
regress.

ISTM that the worst case for the new situation is large arrays that
exist as plpgsql variables but are only ever passed on. Say e.g. a
function that accepts an array among other parameters and passes it on
to another function.

As rather extreme case of this:
CREATE OR REPLACE FUNCTION plpgsql_array_length(p_a anyarray)
RETURNS int LANGUAGE plpgsql AS $$
BEGIN
    RETURN array_length(p_a, 1);
END; $$;

SELECT plpgsql_array_length(b.arr)
FROM (SELECT array_agg(d) FROM generate_series(1, 10000) d) b(arr),
   generate_series(1, 100000) repeat;
with \o /dev/null redirecting the output.

in an assert build it goes from 325.511 ms to 655.733 ms
optimized from 94.648 ms to 287.574 ms.

Now this is a fairly extreme example; and I don't think it'll get much
worse than that. But I do think there's a bunch of cases where values
exist in plpgsql that won't actually be accessed. Say, e.g. return
values from queries that are then conditionally returned and such.

I'm not sure it's possible to do anything about that. Expanding only in
cases where it'd be beneficial is going to be hard.

Greetings,

Andres Freund

  45. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-05-10T16:09:41Z

    Andres Freund <andres@anarazel.de> writes:
> Looking at this. First reading the patch to understand the details.

> * The VARTAG_IS_EXPANDED(tag) trick in VARTAG_SIZE is unlikely to
>   beneficial, before the compiler could implement the whole thing as a
>   computed goto or lookup table, afterwards not.

Well, if you're worried about the speed of VARTAG_SIZE() then the right
thing to do would be to revert your change that made enum vartag_external
distinct from the size of the struct, so that we could go back to just
using the second byte of a varattrib_1b_e datum as its size.  As I said
at the time, inserting pad bytes to force each different type of toast
pointer to be a different size would probably be a better tradeoff than
what commit 3682025015 did.

> * It'd be nice if the get_flat_size comment in expandeddatm.h could
>   specify whether the header size is included. That varies enough around
>   toast that it seems worthwhile.

OK.

> * You were rather bothered by the potential of multiple evaluations for
>   the ilist stuff. And now the AARR macros are full of them...

Yeah, there is doubtless some added cost there.  But I think it's a better
answer than duplicating each function in toto; the code space that that
would take isn't free either.

> * I find the ARRAY_ITER_VARS/ARRAY_ITER_NEXT macros rather ugly. I don't
>   buy the argument that turning them into functions will be slower. I'd
>   bet the contrary on common platforms.

Perhaps; do you want to do some testing and see?

> * Not a fan of the EH_ prefix in array_expanded.c and EOH_
>   elsewhere. Just looks ugly to me. Whatever.

I'm not wedded to that naming if you have a better suggestion.

> * The list of hardwired safe ops in exec_check_rw_parameter is somewhat
>   sad. Don't have a better idea though.

It's very sad, and it will be high on my list to improve that in 9.6.
But I do not think it's a fatal problem to ship it that way in 9.5,
because *as things stand today* those are the only two functions that
could benefit anyway.  It won't really matter until we have extensions
that want to use this mechanism.

> * "Also, a C function that is modifying a read-write expanded value
>   in-place should take care to leave the value in a sane state if it
>   fails partway through." - that's a pretty hefty requirement imo.

It is, which is one reason that I'm nervous about relaxing the test in
exec_check_rw_parameter.  Still, it was possible to code array_set_element
to meet that restriction without too much pain.  I'm inclined to leave the
stronger requirement in the docs for now, until we get more push-back.

> * The forced RW->RO conversion in subquery scans is a bit sad, but I
>   seems like something left for later.

Yes, there are definitely some things that look like future opportunities
here.

> Somewhere in the thread you comment on the fact that it's a bit sad that
> plpgsql is the sole benefactor of this (unless some function forces
> expansion internally).  I'd be ok to leave it at that for now. It'd be
> quite cool to get some feedback from postgis folks about the suitability
> of this for their cases.

Indeed, that's the main reason I'm eager to ship something in 9.5, even if
it's not perfect.  I hope those guys will look at it and use it, and maybe
give us feedback on ways to improve it.

> ISTM that the worst case for the new situation is large arrays that
> exist as plpgsql variables but are only ever passed on.

Well, more to the point, large arrays that are forced into expanded format
(costing a conversion step) but then we never do anything with them that
would benefit from that.  Just saying they're "passed on" doesn't prove
much since the called function might or might not get any benefit.
array_length doesn't, but some other things would.

Your example with array_agg() is interesting, since one of the things on
my to-do list is to see whether we could change array_agg to return an
expanded array.  It would not be hard to make it build that representation
directly, instead of its present ad-hoc internal state.  The trick would
be, when can you return the internal state without an additional copy
step?  But maybe it could return a R/O pointer ...

> ... Expanding only in
> cases where it'd be beneficial is going to be hard.

Yeah, improving that heuristic looks like a research project.  Still, even
with all the limitations and to-do items in the patch now, I'm pretty sure
this will be a net win for practically all applications.

			regards, tom lane

  46. Re: Manipulating complex types as non-contiguous structures in-memory

    Andres Freund <andres@anarazel.de> — 2015-05-10T23:06:56Z

    On 2015-05-10 12:09:41 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Looking at this. First reading the patch to understand the details.
> 
> > * The VARTAG_IS_EXPANDED(tag) trick in VARTAG_SIZE is unlikely to
> >   beneficial, before the compiler could implement the whole thing as a
> >   computed goto or lookup table, afterwards not.
> 
> Well, if you're worried about the speed of VARTAG_SIZE() then the right
> thing to do would be to revert your change that made enum vartag_external
> distinct from the size of the struct, so that we could go back to just
> using the second byte of a varattrib_1b_e datum as its size.  As I said
> at the time, inserting pad bytes to force each different type of toast
> pointer to be a different size would probably be a better tradeoff than
> what commit 3682025015 did.

I doubt that'd be a net positive. Anyway, all I'm saying is that I can't
see the VARTAG_IS_EXPANDED trick being beneficial in comparison to
checking both explicit values.

> > * You were rather bothered by the potential of multiple evaluations for
> >   the ilist stuff. And now the AARR macros are full of them...
> 
> Yeah, there is doubtless some added cost there.  But I think it's a better
> answer than duplicating each function in toto; the code space that that
> would take isn't free either.

Yea, duplicating would be horrid. I'm more thinking of declaring some
iterator state outside the macro, or just using an inline function.

> > * I find the ARRAY_ITER_VARS/ARRAY_ITER_NEXT macros rather ugly. I don't
> >   buy the argument that turning them into functions will be slower. I'd
> >   bet the contrary on common platforms.
> 
> Perhaps; do you want to do some testing and see?

Not exactly with great joy, but I will.

> > * The list of hardwired safe ops in exec_check_rw_parameter is somewhat
> >   sad. Don't have a better idea though.
> 
> It's very sad, and it will be high on my list to improve that in 9.6.


> But I do not think it's a fatal problem to ship it that way in 9.5,
> because *as things stand today* those are the only two functions that
> could benefit anyway.  It won't really matter until we have extensions
> that want to use this mechanism.

Agreed that it's not fatal.

> > ISTM that the worst case for the new situation is large arrays that
> > exist as plpgsql variables but are only ever passed on.
> 
> Well, more to the point, large arrays that are forced into expanded format
> (costing a conversion step) but then we never do anything with them that
> would benefit from that.  Just saying they're "passed on" doesn't prove
> much since the called function might or might not get any benefit.
> array_length doesn't, but some other things would.

Right. But I'm not sure it's that uncommon.

> Your example with array_agg() is interesting, since one of the things on
> my to-do list is to see whether we could change array_agg to return an
> expanded array.

Well, I chose array_agg only because it was a trivial way to generate a
large array. The values could actually come from disk or something.

> It would not be hard to make it build that representation
> directly, instead of its present ad-hoc internal state.  The trick would
> be, when can you return the internal state without an additional copy
> step?  But maybe it could return a R/O pointer ...

R/O or R/W?

> > ... Expanding only in
> > cases where it'd be beneficial is going to be hard.
> 
> Yeah, improving that heuristic looks like a research project.  Still, even
> with all the limitations and to-do items in the patch now, I'm pretty sure
> this will be a net win for practically all applications.

I wonder if we could somehow 'mark' other toast pointers as 'expand if
useful'. I.e. have something pretty much like ExpandedObjectHeader,
except that it initially works like the indirect toast stuff.  So
eoh_context is set, but the data is still in the original datum. When
accessed via 'plain' accessors that don't know about the expanded format
the pointed to datum is returned. But when accessed by something
"desiring" the expanded version it's expanded.  It seemed that'd be
doable expanding the new infrastructure a bit more.

Greetings,

Andres Freund

  47. Re: Manipulating complex types as non-contiguous structures in-memory

    Andres Freund <andres@anarazel.de> — 2015-05-11T00:58:39Z

    On 2015-05-10 12:09:41 -0400, Tom Lane wrote:
> > * I find the ARRAY_ITER_VARS/ARRAY_ITER_NEXT macros rather ugly. I don't
> >   buy the argument that turning them into functions will be slower. I'd
> >   bet the contrary on common platforms.

> Perhaps; do you want to do some testing and see?

I've added new iterator functions using a on-stack state variable and
array_iter_setup/next functions pretty analogous to the macros. And then
converted arrayfuncs.c to use them.

Codesize before introducing inline functions:

assert:
   text    data     bss     dec     hex filename
8142400   50562   295952 8488914  8187d2     src/backend/postgres
optimize:
   text    data     bss     dec     hex filename
6892928   50022   295920 7238870  6e74d6     src/backend/postgres

After:

assert:
   text    data     bss     dec     hex filename
8133040   50562   295952 8479554  816342     src/backend/postgres
optimize:
   text    data     bss     dec     hex filename
6890256   50022   295920 7236198  6e6a66     src/backend/postgres

That's a small decrease.

I'm not sure what exactly to use as a performance benchmark
here. For now I chose
SELECT * FROM (SELECT ARRAY(SELECT generate_series(1, 10000))) d, generate_series(1, 1000) repeat(i);
that'll hit array_out, which uses iterators.

pgbench -P 10 -h /tmp -p 5440 postgres -n -f /tmp/bench.sql -c 4 -T 60
(I chose parallel because it'll show icache efficiency differences)

before, best of four:
tps = 4.921260 (including connections establishing)

after, best of four:
tps = 5.046437 (including connections establishing)

That's a relatively small difference. I'm not surprised, I'd not have
expected anything major.

Personally I think something roughly along those lines is both more
robust and easier to maintain. Even if possibly need to protect against
inlines not being available.


Similarly using inline funcs for AARR_NDIMS/HASNULL does not appear to
hamper performance and gets rid of the multiple evaluation risk.

These patches are obviously WIP. Especially with the iter stuff it's
possible that the concept could be extended a bit further.

Greetings,

Andres Freund

  48. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-05-11T01:09:14Z

    Andres Freund <andres@anarazel.de> writes:
> I'm not sure what exactly to use as a performance benchmark
> here. For now I chose
> SELECT * FROM (SELECT ARRAY(SELECT generate_series(1, 10000))) d, generate_series(1, 1000) repeat(i);
> that'll hit array_out, which uses iterators.

Hmm, probably those results are swamped by I/O functions though.
I'd suggest trying something that exercises array_map(), which
it looks like means doing an array coercion.  Perhaps like so:

do $$
declare a int4[];
x int;
begin
  a := array(select generate_series(1,1000));
  for i in 1..100000 loop
    x := array_length(a::int8[], 1);
  end loop;
end$$;

Anyway, thanks for poking at it!

			regards, tom lane

  49. Re: Manipulating complex types as non-contiguous structures in-memory

    Andres Freund <andres@anarazel.de> — 2015-05-11T01:36:02Z

    On 2015-05-10 21:09:14 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I'm not sure what exactly to use as a performance benchmark
> > here. For now I chose
> > SELECT * FROM (SELECT ARRAY(SELECT generate_series(1, 10000))) d, generate_series(1, 1000) repeat(i);
> > that'll hit array_out, which uses iterators.
> 
> Hmm, probably those results are swamped by I/O functions though.

I did check with a quick profile, and the iteration itself is a
significant part of the total execution time.

> I'd suggest trying something that exercises array_map(), which
> it looks like means doing an array coercion.  Perhaps like so:

> do $$
> declare a int4[];
> x int;
> begin
>   a := array(select generate_series(1,1000));
>   for i in 1..100000 loop
>     x := array_length(a::int8[], 1);
>   end loop;
> end$$;

with the loop count set to 10000 instead, I get:
before:
after:
tps = 20.940092 (including connections establishing)
after:
tps = 20.568730 (including connections establishing)

Greetings,

Andres Freund

  50. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-05-14T00:28:52Z

    Andres Freund <andres@anarazel.de> writes:
> On 2015-05-10 12:09:41 -0400, Tom Lane wrote:
>>> * I find the ARRAY_ITER_VARS/ARRAY_ITER_NEXT macros rather ugly. I don't
>>> buy the argument that turning them into functions will be slower. I'd
>>> bet the contrary on common platforms.

>> Perhaps; do you want to do some testing and see?

> I've added new iterator functions using a on-stack state variable and
> array_iter_setup/next functions pretty analogous to the macros. And then
> converted arrayfuncs.c to use them.

I confirm that this doesn't seem to be any slower (at least not on a
compiler with inline functions).  And it's certainly less ugly, so I've
adopted it.

> Similarly using inline funcs for AARR_NDIMS/HASNULL does not appear to
> hamper performance and gets rid of the multiple evaluation risk.

I'm less excited about that part though.  The original ARR_FOO macros
mostly have multiple-evaluation risks as well, and that's been totally
academic so far.  By the time you get done dealing with the
STATIC_IF_INLINE dance, it's quite messy to have these be inline
functions, and I am not seeing a useful return from adding the mess.

			regards, tom lane

  51. Re: Manipulating complex types as non-contiguous structures in-memory

    Andres Freund <andres@anarazel.de> — 2015-05-14T00:30:42Z

    On 2015-05-13 20:28:52 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > Similarly using inline funcs for AARR_NDIMS/HASNULL does not appear to
> > hamper performance and gets rid of the multiple evaluation risk.
> 
> I'm less excited about that part though.  The original ARR_FOO macros
> mostly have multiple-evaluation risks as well, and that's been totally
> academic so far.

Fair point.

  52. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-05-14T00:48:51Z

    Andres Freund <andres@anarazel.de> writes:
> On 2015-05-10 12:09:41 -0400, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> * The VARTAG_IS_EXPANDED(tag) trick in VARTAG_SIZE is unlikely to
>>> beneficial, before the compiler could implement the whole thing as a
>>> computed goto or lookup table, afterwards not.

>> Well, if you're worried about the speed of VARTAG_SIZE() then the right
>> thing to do would be to revert your change that made enum vartag_external
>> distinct from the size of the struct, so that we could go back to just
>> using the second byte of a varattrib_1b_e datum as its size.  As I said
>> at the time, inserting pad bytes to force each different type of toast
>> pointer to be a different size would probably be a better tradeoff than
>> what commit 3682025015 did.

> I doubt that'd be a net positive. Anyway, all I'm saying is that I can't
> see the VARTAG_IS_EXPANDED trick being beneficial in comparison to
> checking both explicit values.

I did some microbenchmarking on this, and AFAICT doing it your way makes
it slower.

I still think that going back to defining the second byte as the size
would be better.  Fortunately, since this is only a matter of in-memory
representations, we aren't committed to any particular answer.

			regards, tom lane

  53. Re: Manipulating complex types as non-contiguous structures in-memory

    Andres Freund <andres@anarazel.de> — 2015-05-14T00:53:01Z

    On 2015-05-13 20:48:51 -0400, Tom Lane wrote:
> I still think that going back to defining the second byte as the size
> would be better.  Fortunately, since this is only a matter of in-memory
> representations, we aren't committed to any particular answer.

Requiring sizes to be different still strikes me as a disaster. Or is
that not what you're proposing?

  54. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-05-14T01:01:43Z

    Andres Freund <andres@anarazel.de> writes:
> On 2015-05-13 20:48:51 -0400, Tom Lane wrote:
>> I still think that going back to defining the second byte as the size
>> would be better.  Fortunately, since this is only a matter of in-memory
>> representations, we aren't committed to any particular answer.

> Requiring sizes to be different still strikes me as a disaster. Or is
> that not what you're proposing?

It is, but why would it be a disaster?  We could add StaticAsserts
verifying that the sizes actually are different.  I doubt that the pad
space itself could amount to any issue performance-wise, since it would
only ever exist in transient in-memory tuples, and even that only seldom.

			regards, tom lane

  55. Re: Manipulating complex types as non-contiguous structures in-memory

    Andres Freund <andres@anarazel.de> — 2015-05-14T01:06:09Z

    On 2015-05-13 21:01:43 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2015-05-13 20:48:51 -0400, Tom Lane wrote:
> >> I still think that going back to defining the second byte as the size
> >> would be better.  Fortunately, since this is only a matter of in-memory
> >> representations, we aren't committed to any particular answer.
> 
> > Requiring sizes to be different still strikes me as a disaster. Or is
> > that not what you're proposing?
> 
> It is, but why would it be a disaster?  We could add StaticAsserts
> verifying that the sizes actually are different.  I doubt that the pad
> space itself could amount to any issue performance-wise, since it would
> only ever exist in transient in-memory tuples, and even that only seldom.

The sizes would be platform dependant. It's also just incredibly ugly to
have to add pad bytes to structures so we can disambiguate them.

Anyway, I think we can live with your & or my proposed additional branch
for now. I can't see either variant being a relevant performance
bottleneck anytime soon.

  56. Re: Manipulating complex types as non-contiguous structures in-memory

    Tom Lane <tgl@sss.pgh.pa.us> — 2015-05-14T01:19:54Z

    Andres Freund <andres@anarazel.de> writes:
> On 2015-05-13 21:01:43 -0400, Tom Lane wrote:
>> It is, but why would it be a disaster?  We could add StaticAsserts
>> verifying that the sizes actually are different.  I doubt that the pad
>> space itself could amount to any issue performance-wise, since it would
>> only ever exist in transient in-memory tuples, and even that only seldom.

> The sizes would be platform dependant.

So what?  There are lots of platform-dependent constants in PG.

> It's also just incredibly ugly to
> have to add pad bytes to structures so we can disambiguate them.

Well, I agree it's not too pretty, but you were the one who brought up
the issue of the speed of VARTAG_SIZE().  We definitely gave up some
performance there already, and my patch will make it worse.

> Anyway, I think we can live with your & or my proposed additional branch
> for now. I can't see either variant being a relevant performance
> bottleneck anytime soon.

Actually, after having microbenchmarked the difference between those
two proposals, I'm not too sure that VARTAG_SIZE() is down in the noise.
But it doesn't matter for the moment --- any one of these alternatives
would be a very localized code change, and none of them would create
an on-disk compatibility break.  We can let it go until someone wants
to put together a more definitive benchmark for testing.

			regards, tom lane