pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedBugs in copyfuncs/equalfuncs support for JSON node types
4+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2022-07-05 8d9f9634ef 90d6c65a2c Fix errors in copyfuncs/equalfuncs support for JSON node types.
* Bugs in copyfuncs/equalfuncs support for JSON node types
@ 2022-07-05 01:23 Tom Lane <tgl@sss.pgh.pa.us>
2022-07-05 01:48 ` Re: Bugs in copyfuncs/equalfuncs support for JSON node types Zhihong Yu <zyu@yugabyte.com>
2022-07-05 02:18 ` Re: Bugs in copyfuncs/equalfuncs support for JSON node types Justin Pryzby <pryzby@telsasoft.com>
0 siblings, 2 replies; 4+ messages in thread
From: Tom Lane @ 2022-07-05 01:23 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
In reviewing Peter's patch to auto-generate the backend/nodes
support files, I compared what the patch's script produces to
what is in the code now. I found several discrepancies in the
recently-added parse node types for JSON functions, and as far
as I can see every one of those discrepancies is an error in
the existing code. Some of them are relatively harmless
(e.g. COPY_LOCATION_FIELD isn't really different from
COPY_SCALAR_FIELD), but some of them definitely are live bugs.
I propose the attached patch.
regards, tom lane
Attachments:
fix-json-copy-equal-bugs.patch (text/x-diff, 2.9K) download
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 51d630fa89..706d283a92 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -2703,7 +2703,8 @@ _copyJsonTable(const JsonTable *from)
COPY_NODE_FIELD(plan);
COPY_NODE_FIELD(on_error);
COPY_NODE_FIELD(alias);
- COPY_SCALAR_FIELD(location);
+ COPY_SCALAR_FIELD(lateral);
+ COPY_LOCATION_FIELD(location);
return newnode;
}
@@ -2721,13 +2722,13 @@ _copyJsonTableColumn(const JsonTableColumn *from)
COPY_NODE_FIELD(typeName);
COPY_STRING_FIELD(pathspec);
COPY_STRING_FIELD(pathname);
- COPY_SCALAR_FIELD(format);
+ COPY_NODE_FIELD(format);
COPY_SCALAR_FIELD(wrapper);
COPY_SCALAR_FIELD(omit_quotes);
COPY_NODE_FIELD(columns);
COPY_NODE_FIELD(on_empty);
COPY_NODE_FIELD(on_error);
- COPY_SCALAR_FIELD(location);
+ COPY_LOCATION_FIELD(location);
return newnode;
}
@@ -2742,10 +2743,10 @@ _copyJsonTablePlan(const JsonTablePlan *from)
COPY_SCALAR_FIELD(plan_type);
COPY_SCALAR_FIELD(join_type);
- COPY_STRING_FIELD(pathname);
COPY_NODE_FIELD(plan1);
COPY_NODE_FIELD(plan2);
- COPY_SCALAR_FIELD(location);
+ COPY_STRING_FIELD(pathname);
+ COPY_LOCATION_FIELD(location);
return newnode;
}
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index e747e1667d..fccc0b4a18 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -147,14 +147,29 @@ _equalTableFunc(const TableFunc *a, const TableFunc *b)
return true;
}
+static bool
+_equalJsonTablePlan(const JsonTablePlan *a, const JsonTablePlan *b)
+{
+ COMPARE_SCALAR_FIELD(plan_type);
+ COMPARE_SCALAR_FIELD(join_type);
+ COMPARE_NODE_FIELD(plan1);
+ COMPARE_NODE_FIELD(plan2);
+ COMPARE_STRING_FIELD(pathname);
+ COMPARE_LOCATION_FIELD(location);
+
+ return true;
+}
+
static bool
_equalJsonTable(const JsonTable *a, const JsonTable *b)
{
COMPARE_NODE_FIELD(common);
COMPARE_NODE_FIELD(columns);
+ COMPARE_NODE_FIELD(plan);
COMPARE_NODE_FIELD(on_error);
COMPARE_NODE_FIELD(alias);
- COMPARE_SCALAR_FIELD(location);
+ COMPARE_SCALAR_FIELD(lateral);
+ COMPARE_LOCATION_FIELD(location);
return true;
}
@@ -166,13 +181,14 @@ _equalJsonTableColumn(const JsonTableColumn *a, const JsonTableColumn *b)
COMPARE_STRING_FIELD(name);
COMPARE_NODE_FIELD(typeName);
COMPARE_STRING_FIELD(pathspec);
- COMPARE_SCALAR_FIELD(format);
+ COMPARE_STRING_FIELD(pathname);
+ COMPARE_NODE_FIELD(format);
COMPARE_SCALAR_FIELD(wrapper);
COMPARE_SCALAR_FIELD(omit_quotes);
COMPARE_NODE_FIELD(columns);
COMPARE_NODE_FIELD(on_empty);
COMPARE_NODE_FIELD(on_error);
- COMPARE_SCALAR_FIELD(location);
+ COMPARE_LOCATION_FIELD(location);
return true;
}
@@ -4405,6 +4421,9 @@ equal(const void *a, const void *b)
case T_JsonArgument:
retval = _equalJsonArgument(a, b);
break;
+ case T_JsonTablePlan:
+ retval = _equalJsonTablePlan(a, b);
+ break;
case T_JsonTable:
retval = _equalJsonTable(a, b);
break;
* Re: Bugs in copyfuncs/equalfuncs support for JSON node types
2022-07-05 01:23 Bugs in copyfuncs/equalfuncs support for JSON node types Tom Lane <tgl@sss.pgh.pa.us>
@ 2022-07-05 01:48 ` Zhihong Yu <zyu@yugabyte.com>
1 sibling, 0 replies; 4+ messages in thread
From: Zhihong Yu @ 2022-07-05 01:48 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
On Mon, Jul 4, 2022 at 6:23 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> In reviewing Peter's patch to auto-generate the backend/nodes
> support files, I compared what the patch's script produces to
> what is in the code now. I found several discrepancies in the
> recently-added parse node types for JSON functions, and as far
> as I can see every one of those discrepancies is an error in
> the existing code. Some of them are relatively harmless
> (e.g. COPY_LOCATION_FIELD isn't really different from
> COPY_SCALAR_FIELD), but some of them definitely are live bugs.
> I propose the attached patch.
>
> regards, tom lane
>
> Hi,
Patch looks good to me.
Thanks
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Bugs in copyfuncs/equalfuncs support for JSON node types
2022-07-05 01:23 Bugs in copyfuncs/equalfuncs support for JSON node types Tom Lane <tgl@sss.pgh.pa.us>
@ 2022-07-05 02:18 ` Justin Pryzby <pryzby@telsasoft.com>
2022-07-05 02:40 ` Re: Bugs in copyfuncs/equalfuncs support for JSON node types Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 4+ messages in thread
From: Justin Pryzby @ 2022-07-05 02:18 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-hackers@lists.postgresql.org
On Mon, Jul 04, 2022 at 09:23:08PM -0400, Tom Lane wrote:
> In reviewing Peter's patch to auto-generate the backend/nodes
> support files, I compared what the patch's script produces to
> what is in the code now. I found several discrepancies in the
> recently-added parse node types for JSON functions, and as far
> as I can see every one of those discrepancies is an error in
> the existing code. Some of them are relatively harmless
> (e.g. COPY_LOCATION_FIELD isn't really different from
> COPY_SCALAR_FIELD), but some of them definitely are live bugs.
> I propose the attached patch.
Do the missing fields indicate a deficiency in test coverage ?
_copyJsonTablePlan.pathname and _equalJsonTable.plan.
--
Justin
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Bugs in copyfuncs/equalfuncs support for JSON node types
2022-07-05 01:23 Bugs in copyfuncs/equalfuncs support for JSON node types Tom Lane <tgl@sss.pgh.pa.us>
2022-07-05 02:18 ` Re: Bugs in copyfuncs/equalfuncs support for JSON node types Justin Pryzby <pryzby@telsasoft.com>
@ 2022-07-05 02:40 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 4+ messages in thread
From: Tom Lane @ 2022-07-05 02:40 UTC (permalink / raw)
To: Justin Pryzby <pryzby@telsasoft.com>; +Cc: pgsql-hackers@lists.postgresql.org
Justin Pryzby <pryzby@telsasoft.com> writes:
> Do the missing fields indicate a deficiency in test coverage ?
> _copyJsonTablePlan.pathname and _equalJsonTable.plan.
Yeah, I'd say so, but I think constructing a test case to prove
it's broken might be more trouble than it's worth --- particularly
seeing that we're about to automate this stuff. Because of that,
I wouldn't even be really concerned about these bugs in HEAD; but
this needs to be back-patched into v15.
The existing COPY_PARSE_PLAN_TREES logic purports to test this
area, but it fails to notice these bugs for a few reasons:
* JsonTable.lateral: COPY_PARSE_PLAN_TREES itself failed to detect
this problem because of matching omissions in _copyJsonTable and
_equalJsonTable. But the lack of any follow-on failure implies
that we don't have any test cases where the lateral flag is significant.
Maybe that means the field is useless? This one would be worth a closer
look, perhaps.
* JsonTableColumn.format: this scalar-instead-of-deep-copy bug
would only be detectable if you were able to clobber the original
parse tree after copying. I have no ideas about an easy way to
do that. It'd surely bite somebody in the field someday, but
making a reproducible test is way harder.
* JsonTable.plan: to detect the missed comparison, you'd have to
build a test case where comparing two such trees actually made
a visible difference; which would require a fair amount of thought
I fear. IIUC this node type will only appear down inside jointrees,
which we don't usually do comparisons on.
regards, tom lane
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2022-07-05 02:40 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-07-05 01:23 Bugs in copyfuncs/equalfuncs support for JSON node types Tom Lane <tgl@sss.pgh.pa.us>
2022-07-05 01:48 ` Zhihong Yu <zyu@yugabyte.com>
2022-07-05 02:18 ` Justin Pryzby <pryzby@telsasoft.com>
2022-07-05 02:40 ` 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