Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Andres Freund 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
On 2015-05-13 21:01:43 -0400, Tom Lane wrote: > Andres Freund 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Andres Freund 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
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? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Andres Freund writes: > On 2015-05-10 12:09:41 -0400, Tom Lane wrote: >> Andres Freund 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
On 2015-05-13 20:28:52 -0400, Tom Lane wrote: > Andres Freund 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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Andres Freund 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
On 2015-05-10 21:09:14 -0400, Tom Lane wrote: > Andres Freund 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, 1))) 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..10 loop > x := array_length(a::int8[], 1); > end loop; > end$$; with the loop count set to 1 instead, I get: before: after: tps = 20.940092 (including connections establishing) after: tps = 20.568730 (including connections establishing) Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Andres Freund 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, 1))) 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..10 loop x := array_length(a::int8[], 1); end loop; end$$; Anyway, thanks for poking at it! 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
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
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:
textdata bss dec hex filename
8142400 50562 295952 8488914 8187d2 src/backend/postgres
optimize:
textdata bss dec hex filename
6892928 50022 295920 7238870 6e74d6 src/backend/postgres
After:
assert:
textdata bss dec hex filename
8133040 50562 295952 8479554 816342 src/backend/postgres
optimize:
textdata 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, 1))) 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
>From 1fe208cda8df5341794ef6b76e11578ecd46cdd8 Mon Sep 17 00:00:00 2001
From: Andres Freund
Date: Mon, 11 May 2015 02:43:06 +0200
Subject: [PATCH 1/2] WIP: Use inline functions for iteration.
---
src/backend/utils/adt/arrayfuncs.c | 68 +
src/include/c.h| 2 +
src/include/utils/array.h | 78 +-
3 files changed, 115 insertions(+), 33 deletions(-)
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index 26fa648..287aca9 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -1037,7 +1037,7 @@ array_out(PG_FUNCTION_ARGS)
int ndim,
*dims,
*lb;
- ARRAY_ITER ARRAY_ITER_VARS(iter);
+ array_iter iter;
ArrayMetaState *my_extra;
/*
@@ -1105,7 +1105,7 @@ array_out(PG_FUNCTION_ARGS)
needquotes = (bool *) palloc(nitems * sizeof(bool));
overall_length = 1; /* don't forget to count \0 at end. */
- ARRAY_ITER_SETUP(iter, v);
+ array_iter_setup(&iter, v);
for (i = 0; i < nitems; i++)
{
@@ -1114,7 +1114,8 @@ array_out(PG_FUNCTION_ARGS)
bool needquote;
/* Get source element, checking for NULL */
- ARRAY_ITER_NEXT(iter, i, itemvalue, isnull, typlen, typbyval, typalign);
+ itemvalue = array_iter_next(&iter, i, &isnull,
+ typlen, typbyval, typalign);
if (isnull)
{
@@ -1542,7 +1543,7 @@ array_send(PG_FUNCTION_ARGS)
*dim,
*lb;
StringInfoData buf;
- ARRAY_ITER ARRAY_ITER_VARS(iter);
+ array_iter iter;
ArrayMetaState *my_extra;
/*
@@ -1597,7 +1598,7 @@ array_send(PG_FUNCTION_ARGS)
}
/* Send the array elements using the element's own sendproc */
- ARRAY_ITER_SETUP(iter, v);
+ array_iter_setup(&iter, v);
for (i = 0; i < nitems; i++)
{
@@ -1605,7 +1606,8 @@ array_send(PG_FUNCTION_ARGS)
bool isnull;
/* Get source element, checking for NULL */
- ARRAY_ITER_NEXT(iter, i, itemvalue, isnull, typlen, typbyval, typalign);
+ itemvalue = array_iter_next(&iter, i, &isnull,
+ typlen, typbyval, typalign);
if (isnull)
{
@@ -3105,7 +3107,7 @@ array_map(FunctionCallInfo fcinfo, Oid retType, ArrayMapState *amstate)
int typlen;
bool typbyval;
char typalign;
- ARRAY_ITER ARRAY_ITER_VARS(iter);
+ array_iter iter;
ArrayMetaState *inp_extra;
ArrayMetaState *ret_extra;
@@ -3165,7 +3167,7 @@ array_map(FunctionCallInfo fcinfo, Oid retType, ArrayMapState *amstate)
nulls = (bool *) palloc(nitems * sizeof(bool));
/* Loop over source data */
- ARRAY_ITER_SETUP(iter, v);
+ array_iter_setup(&iter, v);
hasnulls = false;
for (i = 0; i < nitems; i++)
@@ -3173,8 +3175,8 @@ array_map(FunctionCallInfo fcinfo, Oid retType, ArrayMapState *amstate)
bool callit = true;
/* Get source element, checking for NULL */
- ARRAY_ITER_NEXT(iter, i, fcinfo->arg[
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
On 2015-05-10 12:09:41 -0400, Tom Lane wrote: > Andres Freund 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Andres Freund 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
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, 1) d) b(arr), generate_series(1, 10) 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
2015-05-06 18:54 GMT+02:00 Tom Lane : > Pavel Stehule writes: > > 2015-05-06 15:50 GMT+02:00 Tom Lane : > >> 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 >
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Pavel Stehule writes: > 2015-05-06 15:50 GMT+02:00 Tom Lane : >> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
2015-05-06 15:50 GMT+02:00 Tom Lane :
> Pavel Stehule writes:
> > Multidimensional append is slower 2x .. but it is probably corner case
>
> > declare a int[] := '{}'; begin for i in 1..9 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
>
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Pavel Stehule writes:
> Multidimensional append is slower 2x .. but it is probably corner case
> declare a int[] := '{}'; begin for i in 1..9 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
2015-05-06 0:50 GMT+02:00 Tom Lane :
> I wrote:
> > Pavel Stehule writes:
> >> Significant slowdown is on following test:
>
> >> do $$ declare a int[] := '{}'; begin for i in 1..9 loop a := a ||
> 10;
> >> end loop; end$$ language plpgsql;
> >> do $$ declare a numeric[] := '{}'; begin for i in 1..9 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..9 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..9 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
>
>
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Pavel Stehule 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..9 loop a := a || 10;
> end loop; end$$ language plpgsql;
> do $$ declare a numeric[] := '{}'; begin for i in 1..9 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
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..9 loop a := a || 10;
end loop; end$$ language plpgsql;
do $$ declare a numeric[] := '{}'; begin for i in 1..9 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 :
> Pavel Stehule writes:
> > Test for 3000 elements:
>
> >Original Patch
> > Integer55sec 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
>
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Pavel Stehule writes: > Test for 3000 elements: >Original Patch > Integer55sec 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
2015-05-01 20:53 GMT+02:00 Pavel Stehule : > > > 2015-05-01 20:11 GMT+02:00 Tom Lane : > >> Pavel Stehule 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 Integer55sec 8sec Numeric341sec 8sec Quicksort is about 3x faster -- so a benefit of this patch is clear. Regards Pavel > > Regards > > Pavel > > >> >> regards, tom lane >> > >
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
2015-05-01 20:11 GMT+02:00 Tom Lane : > Pavel Stehule 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 >
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Andres Freund 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
On 2015-05-01 11:11:14 -0700, Tom Lane wrote: > Pavel Stehule 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Pavel Stehule 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
2015-05-01 18:35 GMT+02:00 Tom Lane : > Andres Freund 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 >
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
On 2015-05-01 09:35:08 -0700, Tom Lane wrote: > Andres Freund 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Andres Freund 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
On 04/17/2015 03:58 PM, Tom Lane wrote:
Heikki Linnakangas 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Heikki Linnakangas 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
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;
intbitmask
}
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
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(10));
select arraytimenum(10);
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
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index 867035d..860ad78 100644
*** a/src/backend/access/common/heaptuple.c
--- b/src/backend/access/common/heaptuple.c
***
*** 60,65
--- 60,66
#include "access/sysattr.h"
#include "access/tuptoaster.h"
#include "executor/tuptable.h"
+ #include "utils/expandeddatum.h"
/* Does att's datatype allow packing into the 1-byte-header varlena format? */
*** heap_compute_data_size(TupleDesc tupleDe
*** 93,105
for (i = 0; i < numberOfAttributes; i++)
{
Datum val;
if (isnull[i])
continue;
val = values[i];
! if (ATT_IS_PACKABLE(att[i]) &&
VARATT_CAN_MAKE_SHORT(DatumGetPointer(val)))
{
/*
--- 94,108
for (i = 0; i < numberOfAttributes; i++)
{
Datum val;
+ Form_pg_attribute atti;
if (isnull[i])
continue;
val = values[i];
+ atti = att[i];
! if (ATT_IS_PACKABLE(atti) &&
VARATT_CAN_MAKE_SHORT(DatumGetPointer(val)))
{
/*
*** heap_compute_data_size(TupleDesc tupleDe
*** 108,118
*/
data_length += VARATT_CONVERTED_SHORT_SIZE(DatumGetPointer(val));
}
else
{
! data_length = att_align_datum(data_length, att[i]->attalign,
! att[i]->attlen, val);
! data_length = att_addlength_datum(data_length, att[i]->attlen,
val);
}
}
--- 111,131
*/
data_length += VARATT_CONVERTED_SHORT_SIZE(DatumGetPointer
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
On Sun, Feb 15, 2015 at 6:41 PM, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
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(10));
arraysumnum
-
55
(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
diff --git a/src/backend/access/common/heaptuple.c b/src/backend/access/common/heaptuple.c
index 867035d..860ad78 100644
*** a/src/backend/access/common/heaptuple.c
--- b/src/backend/access/common/heaptuple.c
***
*** 60,65
--- 60,66
#include "access/sysattr.h"
#include "access/tuptoaster.h"
#include "executor/tuptable.h"
+ #include "utils/expandeddatum.h"
/* Does att's datatype allow packing into the 1-byte-header varlena format? */
*** heap_compute_data_size(TupleDesc tupleDe
*** 93,105
for (i = 0; i < numberOfAttributes; i++)
{
Datum val;
if (isnull[i])
continue;
val = values[i];
! if (ATT_IS_PACKABLE(att[i]) &&
VARATT_CAN_MAKE_SHORT(DatumGetPointer(val)))
{
/*
--- 94,108
for (i = 0; i < numberOfAttributes; i++)
{
Datum val;
+ Form_pg_attribute atti;
if (isnull[i])
continue;
val = values[i];
+ atti = att[i];
! if (ATT_IS_PACKABLE(atti) &&
VARATT_CAN_MAKE_SHORT(DatumGetPointer(val)))
{
/*
*** heap_compute_data_size(TupleDesc tupleDe
*** 108,118
*/
data_length += VARATT_CONVERTED_SHORT_SIZE(DatumGetPointer(val));
}
else
{
! data_length = att_align_datum(data_length, att[i]->attalign,
! att[i]->attlen, val);
! data_length = att_addlength_datum(data_length, att[i]->attlen,
val);
}
}
--- 111,131
*/
data_length += VARATT_CONVERTED_SHORT_SIZE(DatumGetPointer(val));
}
+ else if (atti->attlen == -1 &&
+ VARATT_IS_EXTERNAL_EXPANDED(DatumGetPointer(val)))
+ {
+ /*
+ * we want to flatten the expanded value so that the constructed
+ * tuple doesn't depend on it
+ */
+ data_length = att_align_nominal(data_length, atti->attalign);
+ data_length += EOH_get_flat_size(DatumGetEOHP(val));
+ }
else
{
! data_length = att_align_datum(data_length, atti->attalign,
! atti->attlen, val);
! data_length = att_addlength_datum(data_length, atti->attlen,
val);
}
}
*** heap_fill_tuple(TupleDesc tupleDesc,
*** 203,212 ***
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Robert Haas writes: > On Sat, Feb 14, 2015 at 10:45 AM, Tom Lane wrote: >> Martijn van Oosterhout 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
On Sat, Feb 14, 2015 at 10:45 AM, Tom Lane wrote: > Martijn van Oosterhout 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Martijn van Oosterhout 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
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 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 signature.asc Description: Digital signature
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
On Thu, Feb 12, 2015 at 9:50 AM, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Robert Haas writes: > On Tue, Feb 10, 2015 at 3:00 PM, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
On Tue, Feb 10, 2015 at 3:00 PM, Tom Lane 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
* Tom Lane (t...@sss.pgh.pa.us) wrote: > I think restricting this feature to varlena types is just fine. Agreed. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Jim Nasby 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
On 2/10/15 5:19 PM, Tom Lane wrote: Jim Nasby 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Jim Nasby 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
[ this is addressing a tangential point ... ] Stephen Frost writes: > * Tom Lane (t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Manipulating complex types as non-contiguous structures in-memory
Tom, * Tom Lane (t...@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 signature.asc Description: Digital signature
[HACKERS] Manipulating complex types as non-contiguous structures in-memory
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.1382710...@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+gubzz3...@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.1050...@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
