Обсуждение: CREATE FUNCTION broken
Hi, Someone changed the parser to build a TypeName node on CREATE FUNCTION in any case. As a side effect, ALL! functions created got the proretset attribute to true. Thus for a SELECT the parser wrapped an Iter node around the Expr and since singleton functions set isDone the Iter returns no tuple up. Until later, Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #======================================== jwieck@debis.com (Jan Wieck) # *** define.c.orig Fri Feb 13 12:14:17 1998 --- define.c Fri Feb 13 12:14:38 1998 *************** *** 94,100 **** TypeName *setType = (TypeName *) returnType; *prorettype_p = setType->name; ! *returnsSet_p = true; } else { --- 94,100 ---- TypeName *setType = (TypeName *) returnType; *prorettype_p = setType->name; ! *returnsSet_p = setType->setof; } else {
> Someone changed the parser to build a TypeName node on CREATE > FUNCTION in any case. As a side effect, ALL! functions > created got the proretset attribute to true. Thus for a > SELECT the parser wrapped an Iter node around the Expr and > since singleton functions set isDone the Iter returns no > tuple up. Ah. I broke it (though the regression tests did not find the problem). What I changed was the code in gram.y, which used to just create a string node for the return type clause _unless_ the return type was a "SETOF type". In that case a Typename node was created, and the setof attribute was explicitly set. What you found is that farther along, the setof attribute was forced to be true if _any_ Typename node is present. It looks like your patch will completely fix things, and is better than my reverting the gram.y code. Can you suggest a small test case to include in the regression suite? Unless there are objections from others (with a preference for reverting the gram.y code) I'll go ahead and apply Jan's patch. - Tom
Tom wrote: > > > Someone changed the parser to build a TypeName node on CREATE > > FUNCTION in any case. As a side effect, ALL! functions > > created got the proretset attribute to true. Thus for a > > SELECT the parser wrapped an Iter node around the Expr and > > since singleton functions set isDone the Iter returns no > > tuple up. > > Ah. I broke it (though the regression tests did not find the problem). What > I changed was the code in gram.y, which used to just create a string node > for the return type clause _unless_ the return type was a "SETOF type". In > that case a Typename node was created, and the setof attribute was > explicitly set. > > What you found is that farther along, the setof attribute was forced to be > true if _any_ Typename node is present. Haven't spend time to analyze if other places might have been affected by that - just thought we should trust the parser about the SETOF flag in TypeName node instead of knowing it better deep down in the utility commands. ;-) > > It looks like your patch will completely fix things, and is better than my > reverting the gram.y code. Can you suggest a small test case to include in > the regression suite? Small test case - hmmm. The regression tests found it - but you wouldn't expect it there. It's in the trigger test, where at some places SELECT set_ttdummy(0) returns 0 columns instead of one. Anyway - add a little function in regress.c returning a basetype value. Then add tests that use it in SELECT queries. int32 allways_one() { return 1; } SELECT allways_one() AS one; SELECT a, allways_one() AS one FROM t; > > Unless there are objections from others (with a preference for reverting > the gram.y code) I'll go ahead and apply Jan's patch. Even if reverting the gram.y code - my patch could only make things better. Until later, Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #======================================== jwieck@debis.com (Jan Wieck) #
> > > Someone changed the parser to build a TypeName node on CREATE > > > FUNCTION in any case. As a side effect, ALL! functions > > > created got the proretset attribute to true. Thus for a > > > SELECT the parser wrapped an Iter node around the Expr and > > > since singleton functions set isDone the Iter returns no > > > tuple up. > > It looks like your patch will completely fix things, and is better than my > > reverting the gram.y code. Can you suggest a small test case to include in > > the regression suite? > The regression tests found it - but you wouldn't expect it > there. It's in the trigger test, where at some places SELECT > set_ttdummy(0) returns 0 columns instead of one. Ah! This might be all of the problem with the trigger regression test then? I had wanted Vadim to look at it because I wasn't sure what the behavior should be. Does this test look good to you now? > Even if reverting the gram.y code - my patch could only make > things better. Yes, and scrappy already applied it :) - Tom
Tom wrote: > > > > > Someone changed the parser to build a TypeName node on CREATE > > > > FUNCTION in any case. As a side effect, ALL! functions > > > > created got the proretset attribute to true. Thus for a > > > > SELECT the parser wrapped an Iter node around the Expr and > > > > since singleton functions set isDone the Iter returns no > > > > tuple up. > > > It looks like your patch will completely fix things, and is better than my > > > reverting the gram.y code. Can you suggest a small test case to include in > > > the regression suite? > > The regression tests found it - but you wouldn't expect it > > there. It's in the trigger test, where at some places SELECT > > set_ttdummy(0) returns 0 columns instead of one. > > Ah! This might be all of the problem with the trigger regression test then? I > had wanted Vadim to look at it because I wasn't sure what the behavior should > be. Does this test look good to you now? Looks better now. In some places the triggers.sql has comments that an error is expected. And these errors now happen :-) But for the different NOTICE messages I get I'm not sure too. Who's the one who created the trigger test's (Vadim)? Could that guy please have a look at the results now and update the expected/triggers.out to what's really expected? In addition to the triggers I get a failed on the float8 tests too. The test in question is SELECT '' AS bad, f.f1 ^ '1e200' from FLOAT8_TBL f; ====== float8 ====== 200c200,208 < ERROR: pow() result is out of range --- > bad|?column? > ---+-------- > |0 > |NaN > |NaN > |NaN > |NaN > (5 rows) > Content of table is QUERY: SELECT '' AS five, FLOAT8_TBL.*; five|f1 ----+-------------------- |0 |1004.3 |-34.84 |1.2345678901234e+200 |1.2345678901234e-200 (5 rows) What's correct on the overflow - NaN or ERROR? > > > Even if reverting the gram.y code - my patch could only make > > things better. > > Yes, and scrappy already applied it :) > > - Tom > > Until later, Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #======================================== jwieck@debis.com (Jan Wieck) #
I added typmod to the TypeName structure, but am not aware of adding any TypeName structure instance to anything. Nice you have a patch for us. > > Hi, > > Someone changed the parser to build a TypeName node on CREATE > FUNCTION in any case. As a side effect, ALL! functions > created got the proretset attribute to true. Thus for a > SELECT the parser wrapped an Iter node around the Expr and > since singleton functions set isDone the Iter returns no > tuple up. > > > Until later, Jan > > -- > > #======================================================================# > # It's easier to get forgiveness for being wrong than for being right. # > # Let's break this rule - forgive me. # > #======================================== jwieck@debis.com (Jan Wieck) # > > > *** define.c.orig Fri Feb 13 12:14:17 1998 > --- define.c Fri Feb 13 12:14:38 1998 > *************** > *** 94,100 **** > TypeName *setType = (TypeName *) returnType; > > *prorettype_p = setType->name; > ! *returnsSet_p = true; > } > else > { > --- 94,100 ---- > TypeName *setType = (TypeName *) returnType; > > *prorettype_p = setType->name; > ! *returnsSet_p = setType->setof; > } > else > { > > -- Bruce Momjian maillist@candle.pha.pa.us
Jan Wieck wrote: > > Tom wrote: > > > > Ah! This might be all of the problem with the trigger regression test then? I > > had wanted Vadim to look at it because I wasn't sure what the behavior should > > be. Does this test look good to you now? > > Looks better now. In some places the triggers.sql has > comments that an error is expected. And these errors now > happen :-) > > But for the different NOTICE messages I get I'm not sure too. > Who's the one who created the trigger test's (Vadim)? Could > that guy please have a look at the results now and update the > expected/triggers.out to what's really expected? I'll take a look... Vadim