Re: [HACKERS] No-op case in ExecEvalConvertRowtype

[Ashutosh Bapat]

On Fri, Apr 7, 2017 at 9:02 AM, Tom Lane  wrote:
> Ashutosh Bapat  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


-- 
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] No-op case in ExecEvalConvertRowtype

[Amit Langote]

On 2017/04/07 12:16, Ashutosh Bapat wrote:
> On Fri, Apr 7, 2017 at 7:28 AM, Amit Langote
>  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




-- 
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] No-op case in ExecEvalConvertRowtype

[Tom Lane]

Ashutosh Bapat  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


-- 
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] No-op case in ExecEvalConvertRowtype

[Ashutosh Bapat]

On Fri, Apr 7, 2017 at 5:06 AM, Tom Lane  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


-- 
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] No-op case in ExecEvalConvertRowtype

[Ashutosh Bapat]

On Fri, Apr 7, 2017 at 7:35 AM, Tom Lane  wrote:
> Amit Langote  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


-- 
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] No-op case in ExecEvalConvertRowtype

[Ashutosh Bapat]

On Fri, Apr 7, 2017 at 7:28 AM, Amit Langote
 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


-- 
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] No-op case in ExecEvalConvertRowtype

[Ashutosh Bapat]

On Thu, Apr 6, 2017 at 8:51 PM, Tom Lane  wrote:
> Ashutosh Bapat  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


-- 
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] No-op case in ExecEvalConvertRowtype

[Tom Lane]

Amit Langote  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


-- 
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] No-op case in ExecEvalConvertRowtype

[Amit Langote]

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




-- 
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] No-op case in ExecEvalConvertRowtype

[Tom Lane]

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


-- 
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] No-op case in ExecEvalConvertRowtype

[Tom Lane]

I wrote:
> Ashutosh Bapat  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


-- 
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] No-op case in ExecEvalConvertRowtype

[Tom Lane]

Ashutosh Bapat  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


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