Horton

Thread

Commits

  1. Fix errors in copyfuncs/equalfuncs support for JSON node types.

  1. Bugs in copyfuncs/equalfuncs support for JSON node types

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-05T01:23:08Z

    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

  2. Re: Bugs in copyfuncs/equalfuncs support for JSON node types

    Zhihong Yu <zyu@yugabyte.com> — 2022-07-05T01:48:48Z

    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

  3. Re: Bugs in copyfuncs/equalfuncs support for JSON node types

    Justin Pryzby <pryzby@telsasoft.com> — 2022-07-05T02:18:57Z

    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

  4. Re: Bugs in copyfuncs/equalfuncs support for JSON node types

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-05T02:40:41Z

    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