Composite Datums containing toasted fields are a bad idea(?)

On Fri, Mar 28, 2014 at 3:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Way way back in commit ae93e5fd6e8a7e2321e87d23165d9d7660cde598,
we established a coding rule that it was okay for composite Datums
to contain external (out-of-line) toasted fields, as long as such
toasting didn't go more than one level deep in any tuple. This meant
that heap_form_tuple had to go through nontrivial pushups to maintain
that invariant: each composite field has to be inspected to see if any
of its component fields are external datums. Surprisingly, no one has
complained about the cost of the lookups that are required to see
whether fields are composite in the first place.

However, in view of today's bug report from Jan Pecek, I'm wondering
if we shouldn't rethink this. Jan pointed out that the array code was
failing to prevent composites-with-external-fields from getting into
arrays, and after a bit of looking around I'm afraid there are more such
bugs elsewhere. One example is in the planner's evaluate_expr(), which
supposes that just PG_DETOAST_DATUM() is enough to make a value safe for
long-term storage in a plan tree. Range types are making the same sort
of assumption as arrays (hm, can you have a range over a composite type?
Probably, considering we have sort operators for composites). And there
are places in the index AMs that seem to assume likewise, which is an
issue for AMs in which an indexed value could be composite.

I think we might be better off to get rid of toast_flatten_tuple_attribute
and instead insist that composite Datums never contain any external toast
pointers in the first place. That is, places that call heap_form_tuple
to make a composite Datum (rather than a tuple that's due to be stored
to disk) would be responsible for detoasting any fields with external
values first. We could make a wrapper routine for heap_form_tuple to
take care of this rather than duplicating the code in lots of places.

From a performance standpoint this is probably a small win. In cases
where a composite Datum is formed and ultimately saved to disk, it should
be a win, since we'd have to detoast those fields anyway, and we can avoid
the overhead of an extra disassembly and reassembly of the composite
value. If the composite value is never sent to disk, it's a bit harder
to tell: we lose if the specific field value is never extracted from the
composite, but on the other hand we win if it's extracted more than once.
In any case, adding the extra syscache lookups involved in doing
toast_flatten_tuple_attribute in lots more places isn't appealing.

From a code correctness standpoint, the question really is whether we can
find all the places that build composite datums more easily than we can
find all the places that ought to be calling toast_flatten_tuple_attribute
and aren't. I have to admit I'm not sure about that. There seem to be
basically two places to fix in the main executor (ExecEvalRow and
ExecEvalFieldStore), and roughly half a dozen calls of heap_form_tuple in
the various PLs, but I'm not sure I've not missed some cases.

One thing we could do to try to flush out missed cases is to remove
heap_form_tuple's setting of the tuple-Datum header fields, pushing
that functionality into the new wrapper routine. Then, any un-updated
code would generate clearly invalid composite datums, rather than only
failing in infrequent corner cases.

Another issue is what about third-party code. There seems to be risk
either way, but it would accrue to completely different code depending
on which way we try to fix this.

Thoughts?

Trying to follow along here. Am I correct in saying that if you make
these changes any SQL level functionality (say, creating a composite
type containing a large array) that used to work will continue to
work?

merlin

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

Merlin Moncure <mmoncure@gmail.com> writes:

Trying to follow along here. Am I correct in saying that if you make
these changes any SQL level functionality (say, creating a composite
type containing a large array) that used to work will continue to
work?

This wouldn't change any SQL-level functionality ... as long as we don't
introduce new bugs :-(

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

Interesting bug.

On Fri, Mar 28, 2014 at 04:34:41PM -0400, Tom Lane wrote:

I think we might be better off to get rid of toast_flatten_tuple_attribute
and instead insist that composite Datums never contain any external toast
pointers in the first place. That is, places that call heap_form_tuple
to make a composite Datum (rather than a tuple that's due to be stored
to disk) would be responsible for detoasting any fields with external
values first. We could make a wrapper routine for heap_form_tuple to
take care of this rather than duplicating the code in lots of places.

From a performance standpoint this is probably a small win. In cases

where a composite Datum is formed and ultimately saved to disk, it should
be a win, since we'd have to detoast those fields anyway, and we can avoid
the overhead of an extra disassembly and reassembly of the composite
value. If the composite value is never sent to disk, it's a bit harder
to tell: we lose if the specific field value is never extracted from the
composite, but on the other hand we win if it's extracted more than once.

Performance is the dominant issue here; the hacker-centric considerations you
outlined vanish next to it. Adding a speculative detoast can regress by a
million-fold the performance of a query that passes around, without ever
dereferencing, toast pointers to max-size values. Passing around a record
without consulting all fields is a credible thing to do, so I'd scarcely
consider taking the performance risk of more-aggressively detoasting every
composite. That other cases win is little comfort. Today's PostgreSQL
applications either suffer little enough to care or have already taken
measures to avoid duplicate detoasts. Past discussions have examined general
ways to avoid repetitive detoast traffic; we'll have something good when it
can do that without imposing open-ended penalties on another usage pattern.

Making the array construction functions use toast_flatten_tuple_attribute()
carries a related performance risk, more elusive yet just as costly when
encountered. That much risk seems tolerable for 9.4, though. I won't worry
about performance regressions for range types; using a range to marshal huge
values you'll never detoast is a stretch.

In any case, adding the extra syscache lookups involved in doing
toast_flatten_tuple_attribute in lots more places isn't appealing.

True; alas.

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

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

Noah Misch <noah@leadboat.com> writes:

Interesting bug.
On Fri, Mar 28, 2014 at 04:34:41PM -0400, Tom Lane wrote:

I think we might be better off to get rid of toast_flatten_tuple_attribute
and instead insist that composite Datums never contain any external toast
pointers in the first place.

Performance is the dominant issue here; the hacker-centric considerations you
outlined vanish next to it.

I'm not exactly convinced by that line of argument, especially when it's
entirely unsupported by any evidence as to which implementation approach
will actually perform worse.

However, I have done some investigation into what it'd take to keep the
current approach and teach the array code to detoast composite-type array
elements properly. The attached draft patch only fixes arrays; ranges
need work too, and I'm not sure what else might need adjustment. But this
patch does fix the test case provided by Jan Pecek.

The main problem with this patch, as I see it, is that it'll introduce
extra syscache lookup overhead even when there are no toasted fields
anywhere. I've not really tried to quantify how much, since that would
require first agreeing on a benchmark case --- anybody have a thought
about what that ought to be? But in at least some of these functions,
we'll be paying a cache lookup or two per array element, which doesn't
sound promising.

This could be alleviated by caching the lookup results over longer
intervals, but I don't see any way to do that without invasive changes
to the APIs of a number of exported array functions, which doesn't seem
like a good thing for a bug fix that we need to back-patch. I think the
back-branch fix would have to be pretty much what you see here, even if
we then make changes to reduce the costs in HEAD.

Comments?

regards, tom lane

I wrote:

The main problem with this patch, as I see it, is that it'll introduce
extra syscache lookup overhead even when there are no toasted fields
anywhere. I've not really tried to quantify how much, since that would
require first agreeing on a benchmark case --- anybody have a thought
about what that ought to be? But in at least some of these functions,
we'll be paying a cache lookup or two per array element, which doesn't
sound promising.

I created a couple of test cases that I think are close to worst-case for
accumArrayResult() and array_set(), which are the two functions that seem
most worth worrying about. The accumArrayResult test case is

create table manycomplex as
select row(random(),random())::complex as c
from generate_series(1,1000000);
vacuum analyze manycomplex;

then time:

select array_dims(array_agg(c)) from manycomplex;

On HEAD, this takes about 295-300 msec on my machine in a non-cassert
build. With the patch I sent previously, the time goes to 495-500 msec.
Ouch.

The other test case is

create or replace function timearrayset(n int) returns void language plpgsql as
$$
declare a complex[];
begin
a := array[row(1,2), row(3,4), row(5,6)];
for i in 1..$1 loop
a[2] := row(5,6)::complex;
end loop;
end;
$$;

select timearrayset(1000000);

This goes from circa 1250 ms in HEAD to around 1680 ms.

Some poking around with oprofile says that much of the added time is
coming from added syscache and typcache lookups, as I suspected.

I did some further work on the patch to make it possible to cache
the element tuple descriptor across calls for these two functions.
With the attached patch, the first test case comes down to about 335 ms
and the second to about 1475 ms (plpgsql is still leaving some extra
cache lookups on the table). More could be done with some further API
changes, but I think this is about as much as is safe to back-patch.

At least in the accumArrayResult test case, it would be possible to
bring the runtime back to very close to HEAD if we were willing to
abandon the principle that compressed fields within a tuple should
always get decompressed when the tuple goes into a larger structure.
That would allow flatten_composite_element to quit early if the
tuple doesn't have the HEAP_HASEXTERNAL infomask bit set. I'm not
sure that this would be a good tradeoff however: if we fail to remove nested
compression, we could end up paying for that with double compression.
On the other hand, it's unclear that that case would come up often,
while the overhead of disassembling the tuple is definitely going to
happen every time we assign to an array-of-composites element. (Also,
there is no question here of regression relative to previous releases,
since the existing code fails to prevent nested compression.)

Comments?

regards, tom lane

I lack time to give this a solid review, but here's a preliminary reaction:

On Sun, Apr 20, 2014 at 03:38:23PM -0400, Tom Lane wrote:

On HEAD, this takes about 295-300 msec on my machine in a non-cassert
build. With the patch I sent previously, the time goes to 495-500 msec.

This goes from circa 1250 ms in HEAD to around 1680 ms.

Some poking around with oprofile says that much of the added time is
coming from added syscache and typcache lookups, as I suspected.

I did some further work on the patch to make it possible to cache
the element tuple descriptor across calls for these two functions.
With the attached patch, the first test case comes down to about 335 ms
and the second to about 1475 ms (plpgsql is still leaving some extra
cache lookups on the table). More could be done with some further API
changes, but I think this is about as much as is safe to back-patch.

That particular performance drop seems acceptable. As I hinted upthread, the
large performance risk arises for test cases that do not visit a toast table
today but will do so after the change.

At least in the accumArrayResult test case, it would be possible to
bring the runtime back to very close to HEAD if we were willing to
abandon the principle that compressed fields within a tuple should
always get decompressed when the tuple goes into a larger structure.
That would allow flatten_composite_element to quit early if the
tuple doesn't have the HEAP_HASEXTERNAL infomask bit set. I'm not
sure that this would be a good tradeoff however: if we fail to remove nested
compression, we could end up paying for that with double compression.
On the other hand, it's unclear that that case would come up often,
while the overhead of disassembling the tuple is definitely going to
happen every time we assign to an array-of-composites element. (Also,
there is no question here of regression relative to previous releases,
since the existing code fails to prevent nested compression.)

I wonder how it would work out to instead delay this new detoast effort until
toast_insert_or_update(). That timing has the major advantage of not adding
any toast table visits beyond those actually needed to avoid improperly
writing an embedded toast pointer to disk. It would give us the flexibility
to detoast array elements more lazily than we do today, though I don't propose
any immediate change there. To reduce syscache traffic, make the TupleDesc
record whether any column requires recursive detoast work. Perhaps also use
the TupleDesc as a place to cache the recursive-detoast syscache lookups for
tables that do need them. Thoughts?

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

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

Noah Misch <noah@leadboat.com> writes:

I wonder how it would work out to instead delay this new detoast effort until
toast_insert_or_update().

That would require toast_insert_or_update() to know about every container
datatype. I doubt it could lead to an extensible or maintainable
solution.

I'm actually planning to set this patch on the shelf for a bit and go
investigate the other alternative, ie, not generating composite Datums
containing toast pointers in the first place. We now know that the idea
that we aren't going to take performance hits *somewhere* is an illusion,
and I still suspect that the other way is going to lead to a smaller and
cleaner patch. The main performance downside for plpgsql might be
addressable by making sure that plpgsql record variables fall on the "heap
tuple" rather than the "composite Datum" side of the line. I'm also quite
concerned about correctness: I don't have a lot of confidence that this
patch has closed every loophole with respect to arrays, and it hasn't even
touched ranges or any of the related one-off bugs that I believe exist.

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

Hi,

On 2014-04-20 15:38:23 -0400, Tom Lane wrote:

Some poking around with oprofile says that much of the added time is
coming from added syscache and typcache lookups, as I suspected.

I did some further work on the patch to make it possible to cache
the element tuple descriptor across calls for these two functions.
With the attached patch, the first test case comes down to about 335 ms
and the second to about 1475 ms (plpgsql is still leaving some extra
cache lookups on the table). More could be done with some further API
changes, but I think this is about as much as is safe to back-patch.

I unfortunately haven't followed this in detail, but shouldn't it be
relatively easily to make this even cheaper by checking for
HEAP_HASEXTERNAL? If it's not set we don't need to iterate over the
composite's columns, right?

Greetings,

Andres Freund

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

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

Andres Freund <andres@2ndquadrant.com> writes:

I unfortunately haven't followed this in detail, but shouldn't it be
relatively easily to make this even cheaper by checking for
HEAP_HASEXTERNAL? If it's not set we don't need to iterate over the
composite's columns, right?

That's the point I made further down: we could do that if we were willing
to abandon the principle that nested fields shouldn't be compressed.
It's not very clear what it'd cost us to give that up. (Too bad we didn't
define a HEAP_HASCOMPRESSED flag bit ...)

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

On 2014-04-21 11:30:57 -0400, Tom Lane wrote:

Andres Freund <andres@2ndquadrant.com> writes:

I unfortunately haven't followed this in detail, but shouldn't it be
relatively easily to make this even cheaper by checking for
HEAP_HASEXTERNAL? If it's not set we don't need to iterate over the
composite's columns, right?

That's the point I made further down:

Oh, sorry. I started reading this thread from the end just now.

we could do that if we were willing
to abandon the principle that nested fields shouldn't be compressed.
It's not very clear what it'd cost us to give that up.

I don't think the cost of that would be all that high. As you argue,
without that trick the cost of iterating over all columns will be paid
all the time, whereas double compression will take effect much less
often. And might even end up being beneficial.
The risk of significant performance regressions due to backpatching
seems significantly less likely if we pay heed to HASEXTERNAL.

(Too bad we didn't define a HEAP_HASCOMPRESSED flag bit ...)

And too bad that infomask bits are so scarce :(. We really need to
reclaim HEAP_MOVED_OFF and HEAP_MOVED_IN.

Greetings,

Andres Freund

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

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

On Mon, Apr 21, 2014 at 10:40 AM, Andres Freund <andres@2ndquadrant.com> wrote:

On 2014-04-21 11:30:57 -0400, Tom Lane wrote:

Andres Freund <andres@2ndquadrant.com> writes:

I unfortunately haven't followed this in detail, but shouldn't it be
relatively easily to make this even cheaper by checking for
HEAP_HASEXTERNAL? If it's not set we don't need to iterate over the
composite's columns, right?

That's the point I made further down:

Oh, sorry. I started reading this thread from the end just now.

we could do that if we were willing
to abandon the principle that nested fields shouldn't be compressed.
It's not very clear what it'd cost us to give that up.

I don't think the cost of that would be all that high.

I think it's pretty reasonable too.

And too bad that infomask bits are so scarce :(. We really need to
reclaim HEAP_MOVED_OFF and HEAP_MOVED_IN.

The only consequence of that is losing support for in-place update for
pre-9.0 (of which the only supported version is 8.4). I figure it's
also pretty reasonable to drop support for IPU for out of support
versions for new versions going forward. That would recover the bits
and yield some nice cleanups in tqual.c.

8.4 is scheduled to go out of support in July, so if you agree with my
reasoning 9.4 would be a candidate for not honoring 8.4 upgrade
support.

merlin

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

Merlin Moncure wrote:

On Mon, Apr 21, 2014 at 10:40 AM, Andres Freund <andres@2ndquadrant.com> wrote:

And too bad that infomask bits are so scarce :(. We really need to
reclaim HEAP_MOVED_OFF and HEAP_MOVED_IN.

The only consequence of that is losing support for in-place update for
pre-9.0 (of which the only supported version is 8.4). I figure it's
also pretty reasonable to drop support for IPU for out of support
versions for new versions going forward. That would recover the bits
and yield some nice cleanups in tqual.c.

There's no way to be sure that the bits have been removed from tables
that were upgraded from a 8.4 server to a 9.0 server and from there to a
newer server. We'd need some way to catalogue which tables have been
cleaned prior to an upgrade to a release that no longer accepts those
bits; pg_upgrade would then say "table xyz needs to be vacuumed before
the upgrade" in check mode, or something like that.

--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

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

On Mon, Apr 21, 2014 at 10:57:34AM -0400, Tom Lane wrote:

Noah Misch <noah@leadboat.com> writes:

I wonder how it would work out to instead delay this new detoast effort until
toast_insert_or_update().

That would require toast_insert_or_update() to know about every container
datatype. I doubt it could lead to an extensible or maintainable
solution.

If that's its worst drawback, it's excellent.

I'm actually planning to set this patch on the shelf for a bit and go
investigate the other alternative, ie, not generating composite Datums
containing toast pointers in the first place. We now know that the idea
that we aren't going to take performance hits *somewhere* is an illusion,
and I still suspect that the other way is going to lead to a smaller and
cleaner patch. The main performance downside for plpgsql might be
addressable by making sure that plpgsql record variables fall on the "heap
tuple" rather than the "composite Datum" side of the line. I'm also quite
concerned about correctness: I don't have a lot of confidence that this
patch has closed every loophole with respect to arrays, and it hasn't even
touched ranges or any of the related one-off bugs that I believe exist.

I maintain that the potential slowdown is too great to consider adopting that
for the sake of a cleaner patch. Your last message examined a 67% performance
regression. The strategy you're outlining now can slow a query by 1,000,000%.

--
Noah Misch
EnterpriseDB http://www.enterprisedb.com

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

Noah Misch <noah@leadboat.com> writes:

On Mon, Apr 21, 2014 at 10:57:34AM -0400, Tom Lane wrote:

I'm actually planning to set this patch on the shelf for a bit and go
investigate the other alternative, ie, not generating composite Datums
containing toast pointers in the first place.

I maintain that the potential slowdown is too great to consider adopting that
for the sake of a cleaner patch. Your last message examined a 67% performance
regression. The strategy you're outlining now can slow a query by 1,000,000%.

[ shrug... ] It could also speed up a query by similar factors. I see
no good reason to suppose that it would be a net loss overall. I agree
that it might change performance characteristics in a way that we'd
ideally not do in the back branches. But the fact remains that we've
got a bad bug to fix, and absent a reasonably trustworthy functional fix,
arguing about performance characteristics is a waste of breath. I can
make it arbitrarily fast if it's not required to give the right 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

I wrote:

I'm actually planning to set this patch on the shelf for a bit and go
investigate the other alternative, ie, not generating composite Datums
containing toast pointers in the first place.

Here's a draft patch along those lines. It turned out to be best to
leave heap_form_tuple() alone and instead put the dirty work into
HeapTupleGetDatum(). That has some consequences worth discussing:

* The patch changes HeapTupleGetDatum from a simple inline macro into
a function call. This means that third-party extensions will not get
protection against creation of toast-pointer-containing composite Datums
until they recompile. I'm not sure that this is a big deal, though.
After looking through the existing core and contrib code, it seems that
nearly all users of HeapTupleGetDatum are building their tuples from
locally-sourced data that could not possibly be toasted anyway. (This is
true a-priori for users of BuildTupleFromCStrings, for example, and seems
to be true of all uses of HeapTupleGetDatum in SQL-callable functions.)
So it's entirely possible that there would be no live bug anywhere even
without recompiles.

* If we were sufficiently worried about the previous point, we could
get some protection against unfixed extension code by not removing
toast_flatten_tuple_attribute() in the back branches, only in HEAD.
I'm doubtful that it's worth the cycles though.

* Because HeapTupleGetDatum might allocate a new tuple, the wrong thing
might happen if the caller changes CurrentMemoryContext between
heap_form_tuple and HeapTupleGetDatum. I could only find two places
that did that, though, both in plpgsql. I thought about having
HeapTupleGetDatum try to identify the context the passed tuple had been
allocated in, but the problem with that is the passed tuple isn't
necessarily heap-allocated at all --- in fact, one of the two problematic
places in plpgsql passes a pointer to a stack-local variable, so we'd
actually crash if we tried to apply GetMemoryChunkContext() to it.
Of course we can (and I did) change plpgsql, but the question is whether
any third-party code has copied either coding pattern. On balance it
seems best to just use palloc; that's unlikely to cause anything worse
than a memory leak.

* I was quite pleased with the size of the patch: under 100 net new lines,
half of that being a new regression test case. And it's worth emphasizing
that this is a *complete* fix, modulo the question of third-party code
recompiles. The patch I showed a few days ago added several times that
much just to fix arrays of composites, and I wasn't too confident that it
fixed every case even for arrays.

* The cases where an "extra" detoast would be incurred are pretty narrow:
basically, whole-row-Var references, ROW() constructs, and the outputs of
functions returning tuples. As discussed earlier, whether this would cost
anything or save anything would depend on the number of subsequent uses of
the composite Datum's previously-toasted fields. But I'm thinking that
the amount of application code that would actually be impacted in either
direction is probably pretty small. Moreover, we aren't adding any
noticeable overhead in cases where no detoasting occurs, unlike the
situation with the previous patch. (In fact, we're saving some overhead
by getting rid of syscache lookups in toast_flatten_tuple_attribute.)

So, despite Noah's misgivings, I'm thinking this is the way to proceed.

Comments?

regards, tom lane

On 04/25/2014 02:40 AM, Tom Lane wrote:

* The patch changes HeapTupleGetDatum from a simple inline macro into
a function call. This means that third-party extensions will not get
protection against creation of toast-pointer-containing composite Datums
until they recompile.

One consequence of that is that an extension compiled with headers from
new minor version won't work with binaries from an older minor version.
Packagers beware.

- Heikki

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

Heikki Linnakangas <hlinnakangas@vmware.com> writes:

On 04/25/2014 02:40 AM, Tom Lane wrote:

* The patch changes HeapTupleGetDatum from a simple inline macro into
a function call. This means that third-party extensions will not get
protection against creation of toast-pointer-containing composite Datums
until they recompile.

One consequence of that is that an extension compiled with headers from
new minor version won't work with binaries from an older minor version.
Packagers beware.

Yeah, that's a possible issue, though I think we've done such things
before. In any case, alternative approaches to fixing this would likely
also involve extensions needing to call core functions that don't exist
today; though maybe the number of affected extensions would be smaller.

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

Hi,

On 2014-04-24 19:40:30 -0400, Tom Lane wrote:

* Because HeapTupleGetDatum might allocate a new tuple, the wrong thing
might happen if the caller changes CurrentMemoryContext between
heap_form_tuple and HeapTupleGetDatum.

It's fscking ugly to allocate memory in a PG_RETURN_... But I don't
really have a better backward compatible idea :(

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

Andres Freund <andres@2ndquadrant.com> writes:

On 2014-04-24 19:40:30 -0400, Tom Lane wrote:

* Because HeapTupleGetDatum might allocate a new tuple, the wrong thing
might happen if the caller changes CurrentMemoryContext between
heap_form_tuple and HeapTupleGetDatum.

It's fscking ugly to allocate memory in a PG_RETURN_... But I don't
really have a better backward compatible idea :(

It's hardly without precedent; see PG_RETURN_INT64 or PG_RETURN_FLOAT8 on
a 32-bit machine, for starters. There's never been an assumption that
these macros couldn't do 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