Re: [HACKERS] No-op case in ExecEvalConvertRowtype
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
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
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
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
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
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
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
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
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
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
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
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
[HACKERS] No-op case in ExecEvalConvertRowtype
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 || 2801HeapTupleHeaderGetTypeId(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 ConvertRowtypeNoop.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers