mirror of
https://git.postgresql.org/git/postgresql.git
synced 2025-02-11 19:20:40 +08:00
Clean up after insufficiently-researched optimization of tuple conversions.
tupconvert.c's functions formerly considered that an explicit tuple conversion was necessary if the input and output tupdescs contained different type OIDs. The point of that was to make sure that a composite datum resulting from the conversion would contain the destination rowtype OID in its composite-datum header. However, commit3838074f8
entirely misunderstood what that check was for, thinking that it had something to do with presence or absence of an OID column within the tuple. Removal of the check broke the no-op conversion path in ExecEvalConvertRowtype, as reported by Ashutosh Bapat. It turns out that of the dozen or so call sites for tupconvert.c functions, ExecEvalConvertRowtype is the only one that cares about the composite-datum header fields in the output tuple. In all the rest, we'd much rather avoid an unnecessary conversion whenever the tuples are physically compatible. Moreover, the comments in tupconvert.c only promise physical compatibility not a metadata match. So, let's accept the removal of the guarantee about the output tuple's rowtype marking, recognizing that this is a API change that could conceivably break third-party callers of tupconvert.c. (So, let's remember to mention it in the v10 release notes.) However, commit3838074f8
did have a bit of a point here, in that two tuples mustn't be considered physically compatible if one has HEAP_HASOID set and the other doesn't. (Some of the callers of tupconvert.c might not really care about that, but we can't assume it in general.) The previous check accidentally covered that issue, because no RECORD types ever have OIDs, while if two tupdescs have the same named composite type OID then, a fortiori, they have the same tdhasoid setting. If we're removing the type OID match check then we'd better include tdhasoid match as part of the physical compatibility check. Without that hack in tupconvert.c, we need ExecEvalConvertRowtype to take responsibility for inserting the correct rowtype OID label whenever tupconvert.c decides it need not do anything. This is easily done with heap_copy_tuple_as_datum, which will be considerably faster than a tuple disassembly and reassembly anyway; so from a performance standpoint this change is a win all around compared to what happened in earlier branches. It just means a couple more lines of code in ExecEvalConvertRowtype. Ashutosh Bapat and Tom Lane Discussion: https://postgr.es/m/CAFjFpRfvHABV6+oVvGcshF8rHn+1LfRUhj7Jz1CDZ4gPUwehBg@mail.gmail.com
This commit is contained in:
parent
ac2b095088
commit
3f902354b0
@ -138,13 +138,14 @@ convert_tuples_by_position(TupleDesc indesc,
|
||||
nincols, noutcols)));
|
||||
|
||||
/*
|
||||
* Check to see if the map is one-to-one, in which case we need not do
|
||||
* the tuple conversion. That's not enough though if either source or
|
||||
* destination (tuples) contains OIDs; we'd need conversion in that case
|
||||
* to inject the right OID into the tuple datum.
|
||||
* Check to see if the map is one-to-one, in which case we need not do a
|
||||
* tuple conversion. We must also insist that both tupdescs either
|
||||
* specify or don't specify an OID column, else we need a conversion to
|
||||
* add/remove space for that. (For some callers, presence or absence of
|
||||
* an OID column perhaps would not really matter, but let's be safe.)
|
||||
*/
|
||||
if (indesc->natts == outdesc->natts &&
|
||||
!indesc->tdhasoid && !outdesc->tdhasoid)
|
||||
indesc->tdhasoid == outdesc->tdhasoid)
|
||||
{
|
||||
for (i = 0; i < n; i++)
|
||||
{
|
||||
@ -215,13 +216,14 @@ convert_tuples_by_name(TupleDesc indesc,
|
||||
attrMap = convert_tuples_by_name_map(indesc, outdesc, msg);
|
||||
|
||||
/*
|
||||
* Check to see if the map is one-to-one, in which case we need not do
|
||||
* the tuple conversion. That's not enough though if either source or
|
||||
* destination (tuples) contains OIDs; we'd need conversion in that case
|
||||
* to inject the right OID into the tuple datum.
|
||||
* Check to see if the map is one-to-one, in which case we need not do a
|
||||
* tuple conversion. We must also insist that both tupdescs either
|
||||
* specify or don't specify an OID column, else we need a conversion to
|
||||
* add/remove space for that. (For some callers, presence or absence of
|
||||
* an OID column perhaps would not really matter, but let's be safe.)
|
||||
*/
|
||||
if (indesc->natts == outdesc->natts &&
|
||||
!indesc->tdhasoid && !outdesc->tdhasoid)
|
||||
indesc->tdhasoid == outdesc->tdhasoid)
|
||||
{
|
||||
same = true;
|
||||
for (i = 0; i < n; i++)
|
||||
|
@ -2840,21 +2840,31 @@ ExecEvalConvertRowtype(ExprState *state, ExprEvalStep *op, ExprContext *econtext
|
||||
MemoryContextSwitchTo(old_cxt);
|
||||
}
|
||||
|
||||
/*
|
||||
* No-op if no conversion needed (not clear this can happen here).
|
||||
*/
|
||||
if (op->d.convert_rowtype.map == NULL)
|
||||
return;
|
||||
|
||||
/*
|
||||
* do_convert_tuple needs a HeapTuple not a bare HeapTupleHeader.
|
||||
*/
|
||||
/* Following steps need a HeapTuple not a bare HeapTupleHeader */
|
||||
tmptup.t_len = HeapTupleHeaderGetDatumLength(tuple);
|
||||
tmptup.t_data = tuple;
|
||||
|
||||
result = do_convert_tuple(&tmptup, op->d.convert_rowtype.map);
|
||||
|
||||
*op->resvalue = HeapTupleGetDatum(result);
|
||||
if (op->d.convert_rowtype.map != NULL)
|
||||
{
|
||||
/* Full conversion with attribute rearrangement needed */
|
||||
result = do_convert_tuple(&tmptup, op->d.convert_rowtype.map);
|
||||
/* Result already has appropriate composite-datum header fields */
|
||||
*op->resvalue = HeapTupleGetDatum(result);
|
||||
}
|
||||
else
|
||||
{
|
||||
/*
|
||||
* The tuple is physically compatible as-is, but we need to insert the
|
||||
* destination rowtype OID in its composite-datum header field, so we
|
||||
* have to copy it anyway. heap_copy_tuple_as_datum() is convenient
|
||||
* for this since it will both make the physical copy and insert the
|
||||
* correct composite header fields. Note that we aren't expecting to
|
||||
* have to flatten any toasted fields: the input was a composite
|
||||
* datum, so it shouldn't contain any. So heap_copy_tuple_as_datum()
|
||||
* is overkill here, but its check for external fields is cheap.
|
||||
*/
|
||||
*op->resvalue = heap_copy_tuple_as_datum(&tmptup, outdesc);
|
||||
}
|
||||
}
|
||||
|
||||
/*
|
||||
|
@ -657,6 +657,15 @@ select row_to_json(r) from (select q2,q1 from tt1 offset 0) r;
|
||||
{"q2":0,"q1":0}
|
||||
(3 rows)
|
||||
|
||||
-- check no-op rowtype conversions
|
||||
create temp table tt3 () inherits(tt2);
|
||||
insert into tt3 values(33,44);
|
||||
select row_to_json(tt3::tt2::tt1) from tt3;
|
||||
row_to_json
|
||||
-------------------
|
||||
{"q1":33,"q2":44}
|
||||
(1 row)
|
||||
|
||||
--
|
||||
-- IS [NOT] NULL should not recurse into nested composites (bug #14235)
|
||||
--
|
||||
|
@ -287,6 +287,11 @@ create temp table tt2 () inherits(tt1);
|
||||
insert into tt2 values(0,0);
|
||||
select row_to_json(r) from (select q2,q1 from tt1 offset 0) r;
|
||||
|
||||
-- check no-op rowtype conversions
|
||||
create temp table tt3 () inherits(tt2);
|
||||
insert into tt3 values(33,44);
|
||||
select row_to_json(tt3::tt2::tt1) from tt3;
|
||||
|
||||
--
|
||||
-- IS [NOT] NULL should not recurse into nested composites (bug #14235)
|
||||
--
|
||||
|
Loading…
Reference in New Issue
Block a user