Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit3f90235

Browse files
committed
Clean up after insufficiently-researched optimization of tuple conversions.
tupconvert.c's functions formerly considered that an explicit tupleconversion was necessary if the input and output tupdescs containeddifferent type OIDs. The point of that was to make sure that a compositedatum resulting from the conversion would contain the destination rowtypeOID in its composite-datum header. However, commit3838074 entirelymisunderstood what that check was for, thinking that it had something to dowith presence or absence of an OID column within the tuple. Removal of thecheck broke the no-op conversion path in ExecEvalConvertRowtype, asreported 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-datumheader fields in the output tuple. In all the rest, we'd much rather avoidan unnecessary conversion whenever the tuples are physically compatible.Moreover, the comments in tupconvert.c only promise physical compatibilitynot a metadata match. So, let's accept the removal of the guarantee aboutthe output tuple's rowtype marking, recognizing that this is a API changethat could conceivably break third-party callers of tupconvert.c. (So,let's remember to mention it in the v10 release notes.)However, commit3838074 did have a bit of a point here, in that twotuples mustn't be considered physically compatible if one has HEAP_HASOIDset and the other doesn't. (Some of the callers of tupconvert.c might notreally care about that, but we can't assume it in general.) The previouscheck accidentally covered that issue, because no RECORD types ever haveOIDs, while if two tupdescs have the same named composite type OID then,a fortiori, they have the same tdhasoid setting. If we're removing thetype OID match check then we'd better include tdhasoid match as part ofthe physical compatibility check.Without that hack in tupconvert.c, we need ExecEvalConvertRowtype to takeresponsibility for inserting the correct rowtype OID label whenevertupconvert.c decides it need not do anything. This is easily done withheap_copy_tuple_as_datum, which will be considerably faster than a tupledisassembly and reassembly anyway; so from a performance standpoint thischange 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 LaneDiscussion:https://postgr.es/m/CAFjFpRfvHABV6+oVvGcshF8rHn+1LfRUhj7Jz1CDZ4gPUwehBg@mail.gmail.com
1 parentac2b095 commit3f90235

File tree

4 files changed

+48
-22
lines changed

4 files changed

+48
-22
lines changed

‎src/backend/access/common/tupconvert.c

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -138,13 +138,14 @@ convert_tuples_by_position(TupleDesc indesc,
138138
nincols,noutcols)));
139139

140140
/*
141-
* Check to see if the map is one-to-one, in which case we need not do
142-
* the tuple conversion. That's not enough though if either source or
143-
* destination (tuples) contains OIDs; we'd need conversion in that case
144-
* to inject the right OID into the tuple datum.
141+
* Check to see if the map is one-to-one, in which case we need not do a
142+
* tuple conversion. We must also insist that both tupdescs either
143+
* specify or don't specify an OID column, else we need a conversion to
144+
* add/remove space for that. (For some callers, presence or absence of
145+
* an OID column perhaps would not really matter, but let's be safe.)
145146
*/
146147
if (indesc->natts==outdesc->natts&&
147-
!indesc->tdhasoid&& !outdesc->tdhasoid)
148+
indesc->tdhasoid==outdesc->tdhasoid)
148149
{
149150
for (i=0;i<n;i++)
150151
{
@@ -215,13 +216,14 @@ convert_tuples_by_name(TupleDesc indesc,
215216
attrMap=convert_tuples_by_name_map(indesc,outdesc,msg);
216217

217218
/*
218-
* Check to see if the map is one-to-one, in which case we need not do
219-
* the tuple conversion. That's not enough though if either source or
220-
* destination (tuples) contains OIDs; we'd need conversion in that case
221-
* to inject the right OID into the tuple datum.
219+
* Check to see if the map is one-to-one, in which case we need not do a
220+
* tuple conversion. We must also insist that both tupdescs either
221+
* specify or don't specify an OID column, else we need a conversion to
222+
* add/remove space for that. (For some callers, presence or absence of
223+
* an OID column perhaps would not really matter, but let's be safe.)
222224
*/
223225
if (indesc->natts==outdesc->natts&&
224-
!indesc->tdhasoid&& !outdesc->tdhasoid)
226+
indesc->tdhasoid==outdesc->tdhasoid)
225227
{
226228
same= true;
227229
for (i=0;i<n;i++)

‎src/backend/executor/execExprInterp.c

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2840,21 +2840,31 @@ ExecEvalConvertRowtype(ExprState *state, ExprEvalStep *op, ExprContext *econtext
28402840
MemoryContextSwitchTo(old_cxt);
28412841
}
28422842

2843-
/*
2844-
* No-op if no conversion needed (not clear this can happen here).
2845-
*/
2846-
if (op->d.convert_rowtype.map==NULL)
2847-
return;
2848-
2849-
/*
2850-
* do_convert_tuple needs a HeapTuple not a bare HeapTupleHeader.
2851-
*/
2843+
/* Following steps need a HeapTuple not a bare HeapTupleHeader */
28522844
tmptup.t_len=HeapTupleHeaderGetDatumLength(tuple);
28532845
tmptup.t_data=tuple;
28542846

2855-
result=do_convert_tuple(&tmptup,op->d.convert_rowtype.map);
2856-
2857-
*op->resvalue=HeapTupleGetDatum(result);
2847+
if (op->d.convert_rowtype.map!=NULL)
2848+
{
2849+
/* Full conversion with attribute rearrangement needed */
2850+
result=do_convert_tuple(&tmptup,op->d.convert_rowtype.map);
2851+
/* Result already has appropriate composite-datum header fields */
2852+
*op->resvalue=HeapTupleGetDatum(result);
2853+
}
2854+
else
2855+
{
2856+
/*
2857+
* The tuple is physically compatible as-is, but we need to insert the
2858+
* destination rowtype OID in its composite-datum header field, so we
2859+
* have to copy it anyway. heap_copy_tuple_as_datum() is convenient
2860+
* for this since it will both make the physical copy and insert the
2861+
* correct composite header fields. Note that we aren't expecting to
2862+
* have to flatten any toasted fields: the input was a composite
2863+
* datum, so it shouldn't contain any. So heap_copy_tuple_as_datum()
2864+
* is overkill here, but its check for external fields is cheap.
2865+
*/
2866+
*op->resvalue=heap_copy_tuple_as_datum(&tmptup,outdesc);
2867+
}
28582868
}
28592869

28602870
/*

‎src/test/regress/expected/rowtypes.out

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -657,6 +657,15 @@ select row_to_json(r) from (select q2,q1 from tt1 offset 0) r;
657657
{"q2":0,"q1":0}
658658
(3 rows)
659659

660+
-- check no-op rowtype conversions
661+
create temp table tt3 () inherits(tt2);
662+
insert into tt3 values(33,44);
663+
select row_to_json(tt3::tt2::tt1) from tt3;
664+
row_to_json
665+
-------------------
666+
{"q1":33,"q2":44}
667+
(1 row)
668+
660669
--
661670
-- IS [NOT] NULL should not recurse into nested composites (bug #14235)
662671
--

‎src/test/regress/sql/rowtypes.sql

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,11 @@ create temp table tt2 () inherits(tt1);
287287
insert into tt2values(0,0);
288288
select row_to_json(r)from (select q2,q1from tt1 offset0) r;
289289

290+
-- check no-op rowtype conversions
291+
create temp table tt3 () inherits(tt2);
292+
insert into tt3values(33,44);
293+
select row_to_json(tt3::tt2::tt1)from tt3;
294+
290295
--
291296
-- IS [NOT] NULL should not recurse into nested composites (bug #14235)
292297
--

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp