Horton

Thread

Commits

  1. Clean up after insufficiently-researched optimization of tuple conversions.

  2. Be more aggressive in avoiding tuple conversion.

  1. No-op case in ExecEvalConvertRowtype

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-04-06T09:05:52Z

    Hi,
In ExecEvalConvertRowtype(), if the input row doesn't require any
conversion, we simply return that row as is.
2820     /*
2821      * No-op if no conversion needed (not clear this can happen here).
2822      */
2823     if (op->d.convert_rowtype.map == NULL)
2824         return;

If the type of input row is different from that of the output row, the
tree further up would expect typeid in the tuple header to be that of
the output row.

Rajkumar reported a crash while testing with multi-level
partition-wise join with a SELECT clause containing the whole row
reference. In case of multi-level partitioning, per the latest
proposed patch in [1] a whole-row reference on parent translates into
a nested ConvertRowtypeExpr with as many ConvertRowtypeExpr
decorations as there are levels in the partitioning hierarchy. Each
ConvertRowtypeExpr converts a row from lower partition to the row type
of parent. Since the tuples returned by lower ConvertRowtypeExpr were
not stamped with output type, following assertion in the same function
tripped.

2800     Assert(HeapTupleHeaderGetTypeId(tuple) ==  indesc->tdtypeid ||
2801            HeapTupleHeaderGetTypeId(tuple) == RECORDOID);

I tried to create a testcase where this assertion would fail without
multi-level partitioned table, but could not construct one. But I
think this looks like a bug.

PFA the patch to fix this issue by copying the tuple with the output
type stamped in tuple header. Since the tuple could be an on-disk
tuple, it looks like copying is inevitable. But I am not sure if this
is the right fix or if there is any other way to avoid copying.
Comments are welcome.

[1] https://www.postgresql.org/message-id/CAFjFpRfBipiEzP739PFDrzxrWG-UKThHj3tyHkwCeE2hDwP78A%40mail.gmail.com
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  2. Re: No-op case in ExecEvalConvertRowtype

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-06T15:21:52Z

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
> In ExecEvalConvertRowtype(), if the input row doesn't require any
> conversion, we simply return that row as is.

Huh.  That's been like that for a very long time.

> I tried to create a testcase where this assertion would fail without
> multi-level partitioned table, but could not construct one.

You just need nested no-op ConvertRowtypeExprs, which is easily done with
multiple levels of inheritance:

regression=# create table pp (f1 int, f2 text);
CREATE TABLE
regression=# create table cc() inherits (pp);
CREATE TABLE
regression=# create table gc() inherits (cc);
CREATE TABLE
regression=# insert into gc values(11,'foo');
INSERT 0 1
regression=# select (gc.*)::cc from gc;
    gc    
----------
 (11,foo)
(1 row)

regression=# select (gc.*)::cc::pp from gc;
server closed the connection unexpectedly

and in the log I've got

TRAP: FailedAssertion("!(( (tuple)->t_choice.t_datum.datum_typeid ) == indesc->tdtypeid || ( (tuple)->t_choice.t_datum.datum_typeid ) == 2249)", File: "execExprInterp.c", Line: 2824)

Now the question is whether we should go to the trouble of making a tuple
copy just to inject the parent's rowtype.  If the only reason to do so is
to satisfy ExecEvalConvertRowtype's own assertion, it seems like we might
be better advised just to drop the assertion.  On the other hand it seems
like a good general principle that a tuple datum ought to be advertising
a rowtype OID that matches what the expression tree says it should be.

			regards, tom lane

  3. Re: No-op case in ExecEvalConvertRowtype

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-06T21:26:16Z

    I wrote:
> Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
>> In ExecEvalConvertRowtype(), if the input row doesn't require any
>> conversion, we simply return that row as is.

> Huh.  That's been like that for a very long time.

So I imagined that this was an ancient bug, and was proceeding on that
basis, until I noticed that the test case I showed doesn't crash in 9.6
or before.  Which is pretty interesting because the assertion in
ExecEvalConvertRowtype is certainly there, back to 9.4 (and in an even
stronger fashion in older branches).

Digging further, the reason that the back branches don't crash is that
they don't believe that these tuple conversions are no-ops.  And that
traces to this test in convert_tuples_by_name:

    /*
     * Check to see if the map is one-to-one and the tuple types are the same.
     * (We check the latter because if they're not, we want to do conversion
     * to inject the right OID into the tuple datum.)
     */
    if (indesc->natts == outdesc->natts &&
        indesc->tdtypeid == outdesc->tdtypeid)
    {
        ...

Because a ConvertRowtypeExpr would only be inserted to change a tuple's
rowtype from some composite type to some other composite type, it's
basically impossible for convert_tuples_by_name to decide that the mapping
is a no-op, which explains the comment in ExecEvalConvertRowtype doubting
that a no-op case is possible.  Moreover, if a no-op case did happen, the
tuple being returned would in fact contain the right type OID, so there's
no bug.

Or at least that's how it is in 9.6.  In HEAD, it's been broken by
commit 3838074f8, which as near as I can tell was completely misguided.
Apparently, Robert and Amit misread the comment about "injecting the right
OID" to refer to a possible value in the tuple's OID system column, rather
than the rowtype OID that must be placed in the composite datum's header.

I propose to deal with this by reverting 3838074f8 in toto, and then
trying to clarify that comment, and maybe adding a regression test case
based on the example I showed earlier so that it will be a little more
obvious if someone breaks this again.

However, I see that 3838074f8 touches some partitioning code, which
makes me wonder if there's anything in the partitioning logic that
really depends on this erroneous "optimization".

			regards, tom lane

  4. Re: No-op case in ExecEvalConvertRowtype

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-06T23:36:37Z

    I wrote:
> I propose to deal with this by reverting 3838074f8 in toto, and then
> trying to clarify that comment, and maybe adding a regression test case
> based on the example I showed earlier so that it will be a little more
> obvious if someone breaks this again.
> However, I see that 3838074f8 touches some partitioning code, which
> makes me wonder if there's anything in the partitioning logic that
> really depends on this erroneous "optimization".

After further poking around, I've concluded that that approach is probably
an overreaction.  Of the dozen or so callers of convert_tuples_by_position
and convert_tuples_by_name, it seems that ExecEvalConvertRowtype is the
only one that really needs the correct composite-datum headers in the
converted tuple; and even for it, forcing use of do_convert_tuple is
a pretty expensive, brute-force way to get that result.  Ashutosh's
proposal to use heap_copy_tuple_as_datum when no column rearrangement
is required should be substantially more efficient.

So I now think it's okay to remove consideration of matching the target
rowtype OID from the tupconvert.c functions, although we have to realize
that that is effectively an API change for them, one which has a definite
potential for biting third-party callers.

			regards, tom lane

  5. Re: No-op case in ExecEvalConvertRowtype

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-04-07T01:58:59Z

    On 2017/04/07 8:36, Tom Lane wrote:
> I wrote:
>> I propose to deal with this by reverting 3838074f8 in toto, and then
>> trying to clarify that comment, and maybe adding a regression test case
>> based on the example I showed earlier so that it will be a little more
>> obvious if someone breaks this again.
>> However, I see that 3838074f8 touches some partitioning code, which
>> makes me wonder if there's anything in the partitioning logic that
>> really depends on this erroneous "optimization".

Definitely misread the comment there, but was mystified why the tests
didn't break.  The partitioning tuple-routing code optionally avoids
converting tuples by using this optimization.  Since TupleDesc.tdtypeid of
the parent and the partition to which a tuple is routed are never the
same, tuples would always have to be converted before 3838074f8.  One of
the earlier versions of that patch introduced a consider_typeid parameter
for which only ExecEvalConvertRowtype() passed true.

> After further poking around, I've concluded that that approach is probably
> an overreaction.  Of the dozen or so callers of convert_tuples_by_position
> and convert_tuples_by_name, it seems that ExecEvalConvertRowtype is the
> only one that really needs the correct composite-datum headers in the
> converted tuple; and even for it, forcing use of do_convert_tuple is
> a pretty expensive, brute-force way to get that result.  Ashutosh's
> proposal to use heap_copy_tuple_as_datum when no column rearrangement
> is required should be substantially more efficient.
> 
> So I now think it's okay to remove consideration of matching the target
> rowtype OID from the tupconvert.c functions, although we have to realize
> that that is effectively an API change for them, one which has a definite
> potential for biting third-party callers.

And I see that just in 3f902354b08 lets the partition tuple-routing code
keep utilizing that optimization.

Thanks,
Amit

  6. Re: No-op case in ExecEvalConvertRowtype

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-07T02:05:28Z

    Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> ... One of
> the earlier versions of that patch introduced a consider_typeid parameter
> for which only ExecEvalConvertRowtype() passed true.

Yeah, I was thinking of adding a flag along that line to fix this, but
desisted after figuring out that ExecEvalConvertRowtype was the only
candidate for using it.  Ashutosh's patch had already shown that it'd
be better to pass "false" there too, so we'd end up with no use cases
at all.

			regards, tom lane

  7. Re: No-op case in ExecEvalConvertRowtype

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-04-07T03:03:56Z

    On Thu, Apr 6, 2017 at 8:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
>> In ExecEvalConvertRowtype(), if the input row doesn't require any
>> conversion, we simply return that row as is.
>
> Huh.  That's been like that for a very long time.
>
>> I tried to create a testcase where this assertion would fail without
>> multi-level partitioned table, but could not construct one.
>
> You just need nested no-op ConvertRowtypeExprs, which is easily done with
> multiple levels of inheritance:
>
> regression=# create table pp (f1 int, f2 text);
> CREATE TABLE
> regression=# create table cc() inherits (pp);
> CREATE TABLE
> regression=# create table gc() inherits (cc);
> CREATE TABLE
> regression=# insert into gc values(11,'foo');
> INSERT 0 1
> regression=# select (gc.*)::cc from gc;
>     gc
> ----------
>  (11,foo)
> (1 row)
>
> regression=# select (gc.*)::cc::pp from gc;
> server closed the connection unexpectedly

Oh, I tried multi-level inheritance, but always tried to select on the
topmost parent. Obviously that didn't work since we flatten
inheritance in planner. I tried to cast rows of one table to the type
of another table with the same definition. We don't allow such
coercion. I missed
    if (typeInheritsFrom(inputTypeId, targetTypeId)
        || typeIsOfTypedTable(inputTypeId, targetTypeId))
in coerce_type().

>
> and in the log I've got
>
> TRAP: FailedAssertion("!(( (tuple)->t_choice.t_datum.datum_typeid ) == indesc->tdtypeid || ( (tuple)->t_choice.t_datum.datum_typeid ) == 2249)", File: "execExprInterp.c", Line: 2824)
>
> Now the question is whether we should go to the trouble of making a tuple
> copy just to inject the parent's rowtype.  If the only reason to do so is
> to satisfy ExecEvalConvertRowtype's own assertion, it seems like we might
> be better advised just to drop the assertion.  On the other hand it seems
> like a good general principle that a tuple datum ought to be advertising
> a rowtype OID that matches what the expression tree says it should be.

Yes, I too came to the same conclusion.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  8. Re: No-op case in ExecEvalConvertRowtype

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-04-07T03:16:21Z

    On Fri, Apr 7, 2017 at 7:28 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>
> And I see that just in 3f902354b08 lets the partition tuple-routing code
> keep utilizing that optimization.

I am not able to find this commit
fatal: ambiguous argument '3f902354b08': unknown revision or path not
in the working tree.
Use '--' to separate paths from revisions


-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  9. Re: No-op case in ExecEvalConvertRowtype

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-04-07T03:18:18Z

    On Fri, Apr 7, 2017 at 7:35 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
>> ... One of
>> the earlier versions of that patch introduced a consider_typeid parameter
>> for which only ExecEvalConvertRowtype() passed true.
>
> Yeah, I was thinking of adding a flag along that line to fix this, but
> desisted after figuring out that ExecEvalConvertRowtype was the only
> candidate for using it.  Ashutosh's patch had already shown that it'd
> be better to pass "false" there too, so we'd end up with no use cases
> at all.

Probably we should also add an assertion there to make sure
ExecEvalConvertRowtype never gets same input and output types. If
that's the case, we don't need the copy as well.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  10. Re: No-op case in ExecEvalConvertRowtype

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-04-07T03:19:07Z

    On Fri, Apr 7, 2017 at 5:06 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So I now think it's okay to remove consideration of matching the target
> rowtype OID from the tupconvert.c functions, although we have to realize
> that that is effectively an API change for them, one which has a definite
> potential for biting third-party callers.

While I agree that we can remove indesc->tdtypeid ==
outdesc->tdtypeid, I am not sure whether it should be replaced by
!indesc->tdhasoid && !outdesc->tdhasoid. If that's required, it seems
to be a bug that needs to be fixed in earlier braches as well.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

  11. Re: No-op case in ExecEvalConvertRowtype

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-04-07T03:32:45Z

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
> While I agree that we can remove indesc->tdtypeid ==
> outdesc->tdtypeid, I am not sure whether it should be replaced by
> !indesc->tdhasoid && !outdesc->tdhasoid.

No, that was overly conservative; the correct test is that the tdhasoid
settings must be equal.  Reading Robert's commit message for 3838074f8
and mine for 3f902354b might clarify this.

> If that's required, it seems
> to be a bug that needs to be fixed in earlier braches as well.

It's not a bug in older branches, because the tdtypeid comparison
was enough to guarantee the same tdhasoid values.

			regards, tom lane

  12. Re: No-op case in ExecEvalConvertRowtype

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2017-04-07T04:42:19Z

    On 2017/04/07 12:16, Ashutosh Bapat wrote:
> On Fri, Apr 7, 2017 at 7:28 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>
>> And I see that just in 3f902354b08 lets the partition tuple-routing code
>> keep utilizing that optimization.
> 
> I am not able to find this commit
> fatal: ambiguous argument '3f902354b08': unknown revision or path not
> in the working tree.
> Use '--' to separate paths from revisions

Sorry I probably wasn't clear.  3f902354b08 is what Tom committed earlier
today.

Thanks,
Amit

  13. Re: No-op case in ExecEvalConvertRowtype

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-04-07T05:48:15Z

    On Fri, Apr 7, 2017 at 9:02 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
>> While I agree that we can remove indesc->tdtypeid ==
>> outdesc->tdtypeid, I am not sure whether it should be replaced by
>> !indesc->tdhasoid && !outdesc->tdhasoid.
>
> No, that was overly conservative; the correct test is that the tdhasoid
> settings must be equal.  Reading Robert's commit message for 3838074f8
> and mine for 3f902354b might clarify this.

Thanks for the commit and testcases.

>
>> If that's required, it seems
>> to be a bug that needs to be fixed in earlier braches as well.
>
> It's not a bug in older branches, because the tdtypeid comparison
> was enough to guarantee the same tdhasoid values.

Ok.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company