pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedBUG #18422: Assert in expandTupleDesc() fails on row mismatch with additional SRF
11+ messages / 4 participants
[nested] [flat]
* BUG #18422: Assert in expandTupleDesc() fails on row mismatch with additional SRF
@ 2024-04-05 10:00 PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 11+ messages in thread
From: PG Bug reporting form @ 2024-04-05 10:00 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org; +Cc: exclusion@gmail.com
The following bug has been logged on the website:
Bug reference: 18422
Logged by: Alexander Lakhin
Email address: exclusion@gmail.com
PostgreSQL version: 16.2
Operating system: Ubuntu 22.04
Description:
The following query:
SELECT * FROM generate_series(1, 1),
COALESCE(row(1)) AS (a int, b int);
triggers an assertion failure:
TRAP: failed Assert("count <= tupdesc->natts"), File: "parse_relation.c",
Line: 3054, PID: 151361
with the following stack trace:
...
#5 0x000055b935579366 in ExceptionalCondition
(conditionName=conditionName@entry=0x55b93569420d "count <= tupdesc->natts",
fileName=fileName@entry=0x55b9356941e6 "parse_relation.c",
lineNumber=lineNumber@entry=3054) at assert.c:66
#6 0x000055b9351841b5 in expandTupleDesc (tupdesc=0x55b9378c7660,
eref=0x55b9378c7358, count=2, offset=offset@entry=0,
rtindex=rtindex@entry=2, sublevels_up=sublevels_up@entry=0, location=-1,
include_dropped=true, colnames=0x0,
colvars=0x7fffaccae3e0) at parse_relation.c:3054
#7 0x000055b93518713a in expandRTE (rte=rte@entry=0x55b9377fc360,
rtindex=rtindex@entry=2, sublevels_up=sublevels_up@entry=0,
location=location@entry=-1, include_dropped=include_dropped@entry=true,
colnames=colnames@entry=0x0,
colvars=0x7fffaccae3e0) at parse_relation.c:2758
#8 0x000055b9353649b6 in build_physical_tlist
(root=root@entry=0x55b9378c8108, rel=rel@entry=0x55b9378c8cf0) at
plancat.c:1792
#9 0x000055b935330653 in create_scan_plan (root=root@entry=0x55b9378c8108,
best_path=best_path@entry=0x55b9378c9340, flags=<optimized out>,
flags@entry=0) at createplan.c:637
#10 0x000055b93532dd65 in create_plan_recurse
(root=root@entry=0x55b9378c8108, best_path=0x55b9378c9340,
flags=flags@entry=0) at createplan.c:411
#11 0x000055b93532f585 in create_nestloop_plan (root=0x55b9378c8108,
best_path=0x55b9377fc470) at createplan.c:4342
#12 0x000055b93532f6be in create_join_plan (root=root@entry=0x55b9378c8108,
best_path=best_path@entry=0x55b9377fc470) at createplan.c:1076
#13 0x000055b93532dd75 in create_plan_recurse
(root=root@entry=0x55b9378c8108, best_path=0x55b9377fc470,
flags=flags@entry=1) at createplan.c:416
#14 0x000055b93532e575 in create_plan (root=root@entry=0x55b9378c8108,
best_path=<optimized out>) at createplan.c:347
#15 0x000055b93533ff5b in standard_planner (parse=0x55b9377fbf78,
query_string=<optimized out>, cursorOptions=2048, boundParams=<optimized
out>) at planner.c:420
#16 0x000055b9353404f8 in planner (parse=parse@entry=0x55b9377fbf78,
query_string=query_string@entry=0x55b9377fa888 "SELECT * FROM
generate_series(1, 1),\n COALESCE(row(1)) AS (a int, b int);",
cursorOptions=cursorOptions@entry=2048,
boundParams=boundParams@entry=0x0) at planner.c:281
#17 0x000055b93542c257 in pg_plan_query
(querytree=querytree@entry=0x55b9377fbf78,
query_string=query_string@entry=0x55b9377fa888 "SELECT * FROM
generate_series(1, 1),\n COALESCE(row(1)) AS (a int, b int);",
cursorOptions=cursorOptions@entry=2048,
boundParams=boundParams@entry=0x0) at postgres.c:904
#18 0x000055b93542c30d in pg_plan_queries (querytrees=0x55b9378c80b8,
query_string=query_string@entry=0x55b9377fa888 "SELECT * FROM
generate_series(1, 1),\n COALESCE(row(1)) AS (a int, b int);",
cursorOptions=cursorOptions@entry=2048,
boundParams=boundParams@entry=0x0) at postgres.c:996
#19 0x000055b93542c7f0 in exec_simple_query
(query_string=query_string@entry=0x55b9377fa888 "SELECT * FROM
generate_series(1, 1),\n COALESCE(row(1)) AS (a int, b int);") at
postgres.c:1193
(gdb) frame 6
(gdb) p tupdesc->natts
$1 = 1
Reproduced on REL_12_STABLE .. master.
`git bisect` blames d57534740 (8a15b4178 on REL_12_STABLE).
(Thanks to Amit Langote for correcting my initial report where this issue
was hastily attributed to JSON_TABLE().)
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #18422: Assert in expandTupleDesc() fails on row mismatch with additional SRF
@ 2024-04-07 09:41 Tender Wang <tndrwang@gmail.com>
parent: PG Bug reporting form <noreply@postgresql.org>
0 siblings, 1 reply; 11+ messages in thread
From: Tender Wang @ 2024-04-07 09:41 UTC (permalink / raw)
To: exclusion@gmail.com; +Cc: pgsql-bugs@lists.postgresql.org
PG Bug reporting form <noreply@postgresql.org> 于2024年4月5日周五 18:35写道：
> The following bug has been logged on the website:
>
> Bug reference: 18422
> Logged by: Alexander Lakhin
> Email address: exclusion@gmail.com
> PostgreSQL version: 16.2
> Operating system: Ubuntu 22.04
> Description:
>
> The following query:
> SELECT * FROM generate_series(1, 1),
> COALESCE(row(1)) AS (a int, b int);
> triggers an assertion failure:
> TRAP: failed Assert("count <= tupdesc->natts"), File: "parse_relation.c",
> Line: 3054, PID: 151361
>
> with the following stack trace:
> ...
> #5 0x000055b935579366 in ExceptionalCondition
> (conditionName=conditionName@entry=0x55b93569420d "count <=
> tupdesc->natts",
> fileName=fileName@entry=0x55b9356941e6 "parse_relation.c",
> lineNumber=lineNumber@entry=3054) at assert.c:66
> #6 0x000055b9351841b5 in expandTupleDesc (tupdesc=0x55b9378c7660,
> eref=0x55b9378c7358, count=2, offset=offset@entry=0,
> rtindex=rtindex@entry=2, sublevels_up=sublevels_up@entry=0, location=-1,
> include_dropped=true, colnames=0x0,
> colvars=0x7fffaccae3e0) at parse_relation.c:3054
> #7 0x000055b93518713a in expandRTE (rte=rte@entry=0x55b9377fc360,
> rtindex=rtindex@entry=2, sublevels_up=sublevels_up@entry=0,
> location=location@entry=-1, include_dropped=include_dropped@entry=true,
> colnames=colnames@entry=0x0,
> colvars=0x7fffaccae3e0) at parse_relation.c:2758
> #8 0x000055b9353649b6 in build_physical_tlist
> (root=root@entry=0x55b9378c8108, rel=rel@entry=0x55b9378c8cf0) at
> plancat.c:1792
> #9 0x000055b935330653 in create_scan_plan (root=root@entry
> =0x55b9378c8108,
> best_path=best_path@entry=0x55b9378c9340, flags=<optimized out>,
> flags@entry=0) at createplan.c:637
> #10 0x000055b93532dd65 in create_plan_recurse
> (root=root@entry=0x55b9378c8108, best_path=0x55b9378c9340,
> flags=flags@entry=0) at createplan.c:411
> #11 0x000055b93532f585 in create_nestloop_plan (root=0x55b9378c8108,
> best_path=0x55b9377fc470) at createplan.c:4342
> #12 0x000055b93532f6be in create_join_plan (root=root@entry
> =0x55b9378c8108,
> best_path=best_path@entry=0x55b9377fc470) at createplan.c:1076
> #13 0x000055b93532dd75 in create_plan_recurse
> (root=root@entry=0x55b9378c8108, best_path=0x55b9377fc470,
> flags=flags@entry=1) at createplan.c:416
> #14 0x000055b93532e575 in create_plan (root=root@entry=0x55b9378c8108,
> best_path=<optimized out>) at createplan.c:347
> #15 0x000055b93533ff5b in standard_planner (parse=0x55b9377fbf78,
> query_string=<optimized out>, cursorOptions=2048, boundParams=<optimized
> out>) at planner.c:420
> #16 0x000055b9353404f8 in planner (parse=parse@entry=0x55b9377fbf78,
> query_string=query_string@entry=0x55b9377fa888 "SELECT * FROM
> generate_series(1, 1),\n COALESCE(row(1)) AS (a int, b int);",
> cursorOptions=cursorOptions@entry=2048,
> boundParams=boundParams@entry=0x0) at planner.c:281
> #17 0x000055b93542c257 in pg_plan_query
> (querytree=querytree@entry=0x55b9377fbf78,
> query_string=query_string@entry=0x55b9377fa888 "SELECT * FROM
> generate_series(1, 1),\n COALESCE(row(1)) AS (a int, b int);",
> cursorOptions=cursorOptions@entry=2048,
> boundParams=boundParams@entry=0x0) at postgres.c:904
> #18 0x000055b93542c30d in pg_plan_queries (querytrees=0x55b9378c80b8,
> query_string=query_string@entry=0x55b9377fa888 "SELECT * FROM
> generate_series(1, 1),\n COALESCE(row(1)) AS (a int, b int);",
> cursorOptions=cursorOptions@entry=2048,
> boundParams=boundParams@entry=0x0) at postgres.c:996
> #19 0x000055b93542c7f0 in exec_simple_query
> (query_string=query_string@entry=0x55b9377fa888 "SELECT * FROM
> generate_series(1, 1),\n COALESCE(row(1)) AS (a int, b int);") at
> postgres.c:1193
>
> (gdb) frame 6
>
> (gdb) p tupdesc->natts
> $1 = 1
>
> Reproduced on REL_12_STABLE .. master.
>
> `git bisect` blames d57534740 (8a15b4178 on REL_12_STABLE).
> (Thanks to Amit Langote for correcting my initial report where this issue
> was hastily attributed to JSON_TABLE().)
>
Thanks for reporting this issue.
Before d57534740, it returned TYPEFUNC_RECORD in get_expr_result_type().
So it would not call expandTupleDesc(), but it would ereport in
tupledesc_match().
I copy the ereport code in tupledesc_match() into expandRTE() if
funcolcount is larger
than natts. The attached patch looks like a workaround fix.
--
Tender Wang
OpenPie: https://en.openpie.com/
Attachments:
[application/octet-stream] 0001-Fix-RowExpr-with-constant-value-as-COALESCE-argument.patch (3.0K, ../../CAHewXNkfDfKw80kgNvtC0ZvNz=hGxXLyEwiGJzhr3r_fFAkVeQ@mail.gmail.com/3-0001-Fix-RowExpr-with-constant-value-as-COALESCE-argument.patch)
download | inline diff:
From a096c46ec5e9d8cacc114a179ef4724d5b182d41 Mon Sep 17 00:00:00 2001
From: "tender.wang" <tender.wang@openpie.com>
Date: Sun, 7 Apr 2024 17:09:54 +0800
Subject: [PATCH] Fix RowExpr with constant value as COALESCE argument.
Since d575347, Const expr returning RECORDOID, the result of type will
be TYPEFUNC_COMPOSITE not TYPEFUNC_RECORD. It will trigger Assert failed
in this test case.
Before d575347, the test case will report error in tupledesc_match(),
I move that ereport code the place before calling expandTupleDesc().
---
src/backend/parser/parse_relation.c | 10 ++++++++++
src/test/regress/expected/rowtypes.out | 4 ++++
src/test/regress/sql/rowtypes.sql | 2 ++
3 files changed, 16 insertions(+)
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 7ca793a369..e2ffbf55f5 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -2749,6 +2749,16 @@ expandRTE(RangeTblEntry *rte, int rtindex, int sublevels_up,
{
/* Composite data type, e.g. a table's row type */
Assert(tupdesc);
+
+ if (tupdesc->natts < rtfunc->funccolcount)
+ ereport(ERROR,
+ (errcode(ERRCODE_DATATYPE_MISMATCH),
+ errmsg("function return row and query-specified return row do not match"),
+ errdetail_plural("Returned row contains %d attribute, but query expects %d.",
+ "Returned row contains %d attributes, but query expects %d.",
+ tupdesc->natts,
+ tupdesc->natts, rtfunc->funccolcount)));
+
expandTupleDesc(tupdesc, rte->eref,
rtfunc->funccolcount, atts_done,
rtindex, sublevels_up, location,
diff --git a/src/test/regress/expected/rowtypes.out b/src/test/regress/expected/rowtypes.out
index b400b58f76..8bd5806535 100644
--- a/src/test/regress/expected/rowtypes.out
+++ b/src/test/regress/expected/rowtypes.out
@@ -100,6 +100,10 @@ SELECT * FROM pg_input_error_info('(1,1e400)', 'complex');
"1e400" is out of range for type double precision | | | 22003
(1 row)
+SELECT * FROM generate_series(1, 1),
+ COALESCE(row(1)) AS (a int, b int); -- bad
+ERROR: function return row and query-specified return row do not match
+DETAIL: Returned row contains 1 attribute, but query expects 2.
create temp table quadtable(f1 int, q quad);
insert into quadtable values (1, ((3.3,4.4),(5.5,6.6)));
insert into quadtable values (2, ((null,4.4),(5.5,6.6)));
diff --git a/src/test/regress/sql/rowtypes.sql b/src/test/regress/sql/rowtypes.sql
index fd47dc9e0f..94baae2b6b 100644
--- a/src/test/regress/sql/rowtypes.sql
+++ b/src/test/regress/sql/rowtypes.sql
@@ -37,6 +37,8 @@ SELECT pg_input_is_valid('(1,2', 'complex');
SELECT pg_input_is_valid('(1,zed)', 'complex');
SELECT * FROM pg_input_error_info('(1,zed)', 'complex');
SELECT * FROM pg_input_error_info('(1,1e400)', 'complex');
+SELECT * FROM generate_series(1, 1),
+ COALESCE(row(1)) AS (a int, b int); -- bad
create temp table quadtable(f1 int, q quad);
--
2.25.1
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #18422: Assert in expandTupleDesc() fails on row mismatch with additional SRF
@ 2024-04-09 19:34 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tender Wang <tndrwang@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Tom Lane @ 2024-04-09 19:34 UTC (permalink / raw)
To: Tender Wang <tndrwang@gmail.com>; +Cc: exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Tender Wang <tndrwang@gmail.com> writes:
>> The following bug has been logged on the website:
>> Bug reference: 18422
>> Logged by: Alexander Lakhin
>> Email address: exclusion@gmail.com
>> PostgreSQL version: 16.2
>> Operating system: Ubuntu 22.04
>> Description:
>>
>> The following query:
>> SELECT * FROM generate_series(1, 1),
>> COALESCE(row(1)) AS (a int, b int);
>> triggers an assertion failure:
>> TRAP: failed Assert("count <= tupdesc->natts"), File: "parse_relation.c",
>> Line: 3054, PID: 151361
> I copy the ereport code in tupledesc_match() into expandRTE() if
> funcolcount is larger
> than natts. The attached patch looks like a workaround fix.
I think that's mostly a kluge. It seems to me that the actual problem
is that 2ed8f9a01 missed some places that need fixing. The policy
that that commit intended to institute is "if a RangeTblFunction has a
coldeflist, then the function return type is certainly RECORD, and we
don't need to try to dig the column specifications out of the function
expression" (which dodges the problem that the function expression
might not produce what we're expecting). However, expandRTE didn't
get that memo, and would produce the wrong tupdesc in an example such
as this one. IMO, it ought to produce the query-specified columns in
all cases, even when the function expression doesn't match. Throwing
a premature error isn't better, especially if it only detects one
kind of mismatch.
I dug around looking at other callers of get_expr_result_type and
get_expr_result_tupdesc, and found a couple other places that aren't
actively buggy but can be made faster and more robust by adding
this same rule.
There is a remaining place where we're arguably misusing
get_expr_result_type, which is init_sexpr in execSRF.c. However,
that will just result in postponing the eventual error, because
we'll still cross-check the function result tupdesc against the
query-expected one later. So I think it's okay as-is; at least
it doesn't seem worth the trouble to refactor so that init_sexpr
would have access to the correct RangeTblFunction.
BTW, I wondered why the test cases we added for 2ed8f9a01 didn't
catch this, because they sure look superficially similar. The
reason seems to be that we fail because this query produces a join
plan in which we invoke build_physical_tlist for the FunctionScan,
and that's what's calling expandRTE and hitting the assertion.
The older test cases get simplified sufficiently that there's no
join but just a FunctionScan, and that plan node will be required
to produce the query-specified tlist not a physical tlist, so we
accidentally avoid reaching the assertion.
So I end with the attached.
regards, tom lane
Attachments:
[text/x-diff] v2-fix-bug-18422.patch (4.5K, ../../3861246.1712691263@sss.pgh.pa.us/2-v2-fix-bug-18422.patch)
download | inline diff:
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 4badb6ff58..fb768ad52c 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -1854,6 +1854,10 @@ pull_up_constant_function(PlannerInfo *root, Node *jtnode,
if (rtf->funccolcount != 1)
return jtnode; /* definitely composite */
+ /* If it has a coldeflist, it certainly returns RECORD */
+ if (rtf->funccolnames != NIL)
+ return jtnode; /* must be a one-column RECORD type */
+
functypclass = get_expr_result_type(rtf->funcexpr,
&funcrettype,
&tupdesc);
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index b50fe58d1c..59487cbd79 100644
--- a/src/backend/optimizer/util/clauses.c
+++ b/src/backend/optimizer/util/clauses.c
@@ -4431,12 +4431,11 @@ evaluate_function(Oid funcid, Oid result_type, int32 result_typmod,
* Can't simplify if it returns RECORD. The immediate problem is that it
* will be needing an expected tupdesc which we can't supply here.
*
- * In the case where it has OUT parameters, it could get by without an
- * expected tupdesc, but we still have issues: get_expr_result_type()
- * doesn't know how to extract type info from a RECORD constant, and in
- * the case of a NULL function result there doesn't seem to be any clean
- * way to fix that. In view of the likelihood of there being still other
- * gotchas, seems best to leave the function call unreduced.
+ * In the case where it has OUT parameters, we could build an expected
+ * tupdesc from those, but there may be other gotchas lurking. In
+ * particular, if the function were to return NULL, we would produce a
+ * null constant with no remaining indication of which concrete record
+ * type it is. For now, seems best to leave the function call unreduced.
*/
if (funcform->prorettype == RECORDOID)
return NULL;
diff --git a/src/backend/parser/parse_relation.c b/src/backend/parser/parse_relation.c
index 7ca793a369..2f64eaf0e3 100644
--- a/src/backend/parser/parse_relation.c
+++ b/src/backend/parser/parse_relation.c
@@ -2738,12 +2738,17 @@ expandRTE(RangeTblEntry *rte, int rtindex, int sublevels_up,
{
RangeTblFunction *rtfunc = (RangeTblFunction *) lfirst(lc);
TypeFuncClass functypclass;
- Oid funcrettype;
- TupleDesc tupdesc;
+ Oid funcrettype = InvalidOid;
+ TupleDesc tupdesc = NULL;
+
+ /* If it has a coldeflist, it returns RECORD */
+ if (rtfunc->funccolnames != NIL)
+ functypclass = TYPEFUNC_RECORD;
+ else
+ functypclass = get_expr_result_type(rtfunc->funcexpr,
+ &funcrettype,
+ &tupdesc);
- functypclass = get_expr_result_type(rtfunc->funcexpr,
- &funcrettype,
- &tupdesc);
if (functypclass == TYPEFUNC_COMPOSITE ||
functypclass == TYPEFUNC_COMPOSITE_DOMAIN)
{
@@ -3369,6 +3374,10 @@ get_rte_attribute_is_dropped(RangeTblEntry *rte, AttrNumber attnum)
{
TupleDesc tupdesc;
+ /* If it has a coldeflist, it returns RECORD */
+ if (rtfunc->funccolnames != NIL)
+ return false; /* can't have any dropped columns */
+
tupdesc = get_expr_result_tupdesc(rtfunc->funcexpr,
true);
if (tupdesc)
diff --git a/src/test/regress/expected/rangefuncs.out b/src/test/regress/expected/rangefuncs.out
index 2bda957f43..397a8b35d6 100644
--- a/src/test/regress/expected/rangefuncs.out
+++ b/src/test/regress/expected/rangefuncs.out
@@ -2498,3 +2498,6 @@ with a(b) as (values (row(1,2,3)))
select * from a, coalesce(b) as c(d int, e int, f float); -- fail
ERROR: function return row and query-specified return row do not match
DETAIL: Returned type integer at ordinal position 3, but query expects double precision.
+select * from int8_tbl, coalesce(row(1)) as (a int, b int); -- fail
+ERROR: function return row and query-specified return row do not match
+DETAIL: Returned row contains 1 attribute, but query expects 2.
diff --git a/src/test/regress/sql/rangefuncs.sql b/src/test/regress/sql/rangefuncs.sql
index 06d598c5e9..3c47c98e11 100644
--- a/src/test/regress/sql/rangefuncs.sql
+++ b/src/test/regress/sql/rangefuncs.sql
@@ -824,3 +824,4 @@ with a(b) as (values (row(1,2,3)))
select * from a, coalesce(b) as c(d int, e int, f int, g int); -- fail
with a(b) as (values (row(1,2,3)))
select * from a, coalesce(b) as c(d int, e int, f float); -- fail
+select * from int8_tbl, coalesce(row(1)) as (a int, b int); -- fail
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #18422: Assert in expandTupleDesc() fails on row mismatch with additional SRF
@ 2024-04-10 03:25 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 11+ messages in thread
From: Richard Guo @ 2024-04-10 03:25 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tender Wang <tndrwang@gmail.com>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Apr 10, 2024 at 3:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think that's mostly a kluge. It seems to me that the actual problem
> is that 2ed8f9a01 missed some places that need fixing. The policy
> that that commit intended to institute is "if a RangeTblFunction has a
> coldeflist, then the function return type is certainly RECORD, and we
> don't need to try to dig the column specifications out of the function
> expression" (which dodges the problem that the function expression
> might not produce what we're expecting). However, expandRTE didn't
> get that memo, and would produce the wrong tupdesc in an example such
> as this one. IMO, it ought to produce the query-specified columns in
> all cases, even when the function expression doesn't match.
+1. This seems a more principled fix.
> I dug around looking at other callers of get_expr_result_type and
> get_expr_result_tupdesc, and found a couple other places that aren't
> actively buggy but can be made faster and more robust by adding
> this same rule.
I wonder why the v2 patch does not apply this same rule in
process_function_rte_ref(), by checking if the 'rtfunc' has a coldeflist
before it calls get_expr_result_tupdesc.
> BTW, I wondered why the test cases we added for 2ed8f9a01 didn't
> catch this, because they sure look superficially similar. The
> reason seems to be that we fail because this query produces a join
> plan in which we invoke build_physical_tlist for the FunctionScan,
> and that's what's calling expandRTE and hitting the assertion.
> The older test cases get simplified sufficiently that there's no
> join but just a FunctionScan, and that plan node will be required
> to produce the query-specified tlist not a physical tlist, so we
> accidentally avoid reaching the assertion.
Exactly.
Thanks
Richard
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #18422: Assert in expandTupleDesc() fails on row mismatch with additional SRF
@ 2024-04-10 03:32 Tom Lane <tgl@sss.pgh.pa.us>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Tom Lane @ 2024-04-10 03:32 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Tender Wang <tndrwang@gmail.com>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Richard Guo <guofenglinux@gmail.com> writes:
> I wonder why the v2 patch does not apply this same rule in
> process_function_rte_ref(), by checking if the 'rtfunc' has a coldeflist
> before it calls get_expr_result_tupdesc.
I guess we could. It won't matter because the following code will
reject RECORD in any case; but we could save a few cycles by not
calling get_expr_result_tupdesc there.
regards, tom lane
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #18422: Assert in expandTupleDesc() fails on row mismatch with additional SRF
@ 2024-04-10 07:58 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 11+ messages in thread
From: Richard Guo @ 2024-04-10 07:58 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tender Wang <tndrwang@gmail.com>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Apr 10, 2024 at 11:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Richard Guo <guofenglinux@gmail.com> writes:
> > I wonder why the v2 patch does not apply this same rule in
> > process_function_rte_ref(), by checking if the 'rtfunc' has a coldeflist
> > before it calls get_expr_result_tupdesc.
>
> I guess we could. It won't matter because the following code will
> reject RECORD in any case; but we could save a few cycles by not
> calling get_expr_result_tupdesc there.
Indeed. I think it would be better to add this same rule to
process_function_rte_ref(), or at least write some comments there to
explain why it is not necessary to check rtfunc->funccolnames while
other places do.
Thanks
Richard
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #18422: Assert in expandTupleDesc() fails on row mismatch with additional SRF
@ 2024-04-10 14:16 Tom Lane <tgl@sss.pgh.pa.us>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Tom Lane @ 2024-04-10 14:16 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Tender Wang <tndrwang@gmail.com>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Richard Guo <guofenglinux@gmail.com> writes:
> On Wed, Apr 10, 2024 at 11:32 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I guess we could. It won't matter because the following code will
>> reject RECORD in any case; but we could save a few cycles by not
>> calling get_expr_result_tupdesc there.
> Indeed. I think it would be better to add this same rule to
> process_function_rte_ref(), or at least write some comments there to
> explain why it is not necessary to check rtfunc->funccolnames while
> other places do.
Wilco. Another thing I was considering, but didn't pull the trigger
on in the draft patch, was to introduce a funcapi.c function on the
order of
get_expr_result_rtfunc(RangeTblFunction *rtfunc, ...)
which would encapsulate applying either BuildDescFromLists or
get_expr_result_type. I didn't do it because I found that the
only places that would want to use it are the two that are correct
already; the places we still need to fix have short-cuts they can
take rather than building an intermediate tupdesc. (The present
bug could be summarized as "the short-cuts still need to pay
attention to the coldeflist".) But the advantage of doing this
anyway is that its header comment would be a natural place to
document this issue and policy. Thoughts?
regards, tom lane
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #18422: Assert in expandTupleDesc() fails on row mismatch with additional SRF
@ 2024-04-11 07:41 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 11+ messages in thread
From: Richard Guo @ 2024-04-11 07:41 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tender Wang <tndrwang@gmail.com>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Wed, Apr 10, 2024 at 10:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Wilco. Another thing I was considering, but didn't pull the trigger
> on in the draft patch, was to introduce a funcapi.c function on the
> order of
> get_expr_result_rtfunc(RangeTblFunction *rtfunc, ...)
> which would encapsulate applying either BuildDescFromLists or
> get_expr_result_type. I didn't do it because I found that the
> only places that would want to use it are the two that are correct
> already; the places we still need to fix have short-cuts they can
> take rather than building an intermediate tupdesc. (The present
> bug could be summarized as "the short-cuts still need to pay
> attention to the coldeflist".) But the advantage of doing this
> anyway is that its header comment would be a natural place to
> document this issue and policy. Thoughts?
Do you think we can have a parameter in the new get_expr_result_rtfunc()
function to indicate whether we want to build an intermediate tupdesc
when we have a coldeflist? Then we can set it to true in the two places
that are correct already, and set it to false at the places we need to
fix. But I'm not sure if including such a new parameter would be an
improvement or just make it worse.
Thanks
Richard
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #18422: Assert in expandTupleDesc() fails on row mismatch with additional SRF
@ 2024-04-11 14:13 Tom Lane <tgl@sss.pgh.pa.us>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Tom Lane @ 2024-04-11 14:13 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Tender Wang <tndrwang@gmail.com>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Richard Guo <guofenglinux@gmail.com> writes:
> On Wed, Apr 10, 2024 at 10:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Wilco. Another thing I was considering, but didn't pull the trigger
>> on in the draft patch, was to introduce a funcapi.c function on the
>> order of
>> get_expr_result_rtfunc(RangeTblFunction *rtfunc, ...)
>> which would encapsulate applying either BuildDescFromLists or
>> get_expr_result_type.
> Do you think we can have a parameter in the new get_expr_result_rtfunc()
> function to indicate whether we want to build an intermediate tupdesc
> when we have a coldeflist? Then we can set it to true in the two places
> that are correct already, and set it to false at the places we need to
> fix. But I'm not sure if including such a new parameter would be an
> improvement or just make it worse.
I did think about that, but it seems mighty weird. The semantics of
the flag would have to be something like "I want a tupdesc when the
result type is COMPOSITE, but not when it's RECORD", which seems
rather arbitrary.
Perhaps it'd be sufficient to add a note to the header comment of
get_expr_result_type warning about when not to use it.
regards, tom lane
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #18422: Assert in expandTupleDesc() fails on row mismatch with additional SRF
@ 2024-04-12 03:25 Richard Guo <guofenglinux@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 11+ messages in thread
From: Richard Guo @ 2024-04-12 03:25 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Tender Wang <tndrwang@gmail.com>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
On Thu, Apr 11, 2024 at 10:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Richard Guo <guofenglinux@gmail.com> writes:
> > Do you think we can have a parameter in the new get_expr_result_rtfunc()
> > function to indicate whether we want to build an intermediate tupdesc
> > when we have a coldeflist? Then we can set it to true in the two places
> > that are correct already, and set it to false at the places we need to
> > fix. But I'm not sure if including such a new parameter would be an
> > improvement or just make it worse.
>
> I did think about that, but it seems mighty weird. The semantics of
> the flag would have to be something like "I want a tupdesc when the
> result type is COMPOSITE, but not when it's RECORD", which seems
> rather arbitrary.
Indeed.
> Perhaps it'd be sufficient to add a note to the header comment of
> get_expr_result_type warning about when not to use it.
Works for me.
Thanks
Richard
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: BUG #18422: Assert in expandTupleDesc() fails on row mismatch with additional SRF
@ 2024-04-12 03:46 Tom Lane <tgl@sss.pgh.pa.us>
parent: Richard Guo <guofenglinux@gmail.com>
0 siblings, 0 replies; 11+ messages in thread
From: Tom Lane @ 2024-04-12 03:46 UTC (permalink / raw)
To: Richard Guo <guofenglinux@gmail.com>; +Cc: Tender Wang <tndrwang@gmail.com>; exclusion@gmail.com; pgsql-bugs@lists.postgresql.org
Richard Guo <guofenglinux@gmail.com> writes:
> On Thu, Apr 11, 2024 at 10:13 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I did think about that, but it seems mighty weird. The semantics of
>> the flag would have to be something like "I want a tupdesc when the
>> result type is COMPOSITE, but not when it's RECORD", which seems
>> rather arbitrary.
> Indeed.
I did actually spend some time today trying to code that up, to see
what it would look like. The attempt failed though, because there
are existing cases in which get_expr_result_type() returns
TYPEFUNC_RECORD, so we can't use that result code as a positive
indicator that "this RTE has a coldeflist". In a green field
we could invent another TYPEFUNC_XXX code, but that's not a
back-patchable idea. So the new function would need some independent
indicator that it saw a coldeflist, and at that point there's
basically nothing we're hiding from the callers.
>> Perhaps it'd be sufficient to add a note to the header comment of
>> get_expr_result_type warning about when not to use it.
> Works for me.
OK, barring other objections I'll push forward on that basis.
regards, tom lane
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2024-04-12 03:46 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-04-05 10:00 BUG #18422: Assert in expandTupleDesc() fails on row mismatch with additional SRF PG Bug reporting form <noreply@postgresql.org>
2024-04-07 09:41 ` Tender Wang <tndrwang@gmail.com>
2024-04-09 19:34 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-04-10 03:25 ` Richard Guo <guofenglinux@gmail.com>
2024-04-10 03:32 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-04-10 07:58 ` Richard Guo <guofenglinux@gmail.com>
2024-04-10 14:16 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-04-11 07:41 ` Richard Guo <guofenglinux@gmail.com>
2024-04-11 14:13 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-04-12 03:25 ` Richard Guo <guofenglinux@gmail.com>
2024-04-12 03:46 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox