Re: pgsql: Add more SQL/JSON constructor functions

On Fri, Sep 6, 2024 at 12:07 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Thu, Sep 5, 2024 at 9:58 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > On Tue, Sep 3, 2024 at 6:05 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> > > On Mon, Sep 2, 2024 at 4:18 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > > v2-0001 looks good to me.
> > > +-- Test JSON_TABLE() column deparsing -- don't emit default ON ERROR / EMPTY
> > > +-- behavior
> > > +EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text PATH '$'));
> > > +EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text
> > > PATH '$') ERROR ON ERROR);
> > >
> > > Are these tests duplicated? appears both in v2-0002 and v2-0003.
> > >
> > > 0002 output is:
> > > +EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text
> > > PATH '$') ERROR ON ERROR);
> > > +
> > > QUERY PLAN
> > > +------------------------------------------------------------------------------------------------------------------------------------------------
> > > + Table Function Scan on "json_table" (cost=0.01..1.00 rows=100 width=32)
> > > + Output: a
> > > + Table Function Call: JSON_TABLE('"a"'::jsonb, '$' AS
> > > json_table_path_0 COLUMNS (a text PATH '$' NULL ON EMPTY NULL ON
> > > ERROR) ERROR ON ERROR)
> > > +(3 rows)
> > >
> > > 0003 output is:
> > > EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text
> > > PATH '$') ERROR ON ERROR);
> > > QUERY PLAN
> > > --------------------------------------------------------------------------------------------------------------------
> > > Table Function Scan on "json_table" (cost=0.01..1.00 rows=100 width=32)
> > > Output: a
> > > Table Function Call: JSON_TABLE('"a"'::jsonb, '$' AS
> > > json_table_path_0 COLUMNS (a text PATH '$') ERROR ON ERROR)
> > > (3 rows)
> > >
> > > two patches with different output,
> > > overall we should merge 0002 and 0003?
> >
> > Looks like I ordered the patches wrong. I'd like to commit the two separately.
> >
> > > if (jsexpr->on_error &&
> > > jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR &&
> > > (jsexpr->on_error->btype != JSON_BEHAVIOR_NULL || returning_domain))
> > >
> > > we can be simplified as
> > > if ( jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR &&
> > > (jsexpr->on_error->btype != JSON_BEHAVIOR_NULL || returning_domain))
> > >
> > > since if jsexpr->on_error is NULL, then segfault will appear at the beginning of
> > > ExecInitJsonExpr
> >
> > Ok, done.
> >
> > > + *
> > > + * Only add the extra steps for a NULL-valued expression when RETURNING a
> > > + * domain type to check the constraints, if any.
> > > */
> > > + jsestate->jump_error = state->steps_len;
> > > if (jsexpr->on_error &&
> > > - jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR)
> > > + jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR &&
> > > + (jsexpr->on_error->btype != JSON_BEHAVIOR_NULL || returning_domain))
> > >
> > > + *
> > > + * Only add the extra steps for a NULL-valued expression when RETURNING a
> > > + * domain type to check the constraints, if any.
> > > */
> > > + jsestate->jump_empty = state->steps_len;
> > > if (jsexpr->on_empty != NULL &&
> > > - jsexpr->on_empty->btype != JSON_BEHAVIOR_ERROR)
> > > + jsexpr->on_empty->btype != JSON_BEHAVIOR_ERROR &&
> > > + (jsexpr->on_empty->btype != JSON_BEHAVIOR_NULL || returning_domain))
> > >
> > > I am a little bit confused with the comments.
> > > not sure the "NULL-valued expression" refers to.
> >
> > It refers to on_error/on_empty->expr, which is a Const node encoding
> > the NULL (constisnull is true) for that behavior.
> >
> > That's actually the case also for behaviors UNKNOWN and EMPTY, so the
> > condition should actually be checking whether the expr is a
> > NULL-valued expression.
> >
> > I've updated the patch.
> >
> > > i think it is:
> > > implicitly default for ON EMPTY | ERROR clause is NULL (JSON_BEHAVIOR_NULL)
> > > for that situation, we can skip the json coercion process,
> > > but this only applies when the returning type of JsonExpr is not domain,
> >
> > I've reworded the comment to mention that this speeds up the default
> > ON ERROR / EMPTY handling for JSON_QUERY() and JSON_VALUE().
> >
> > Plan to push these tomorrow.
>
> Pushed.

Reverted 0002-0004 from both master and REL_17_STABLE due to BF failures.

0002-0003 are easily fixed by changing the newly added tests to not
use EXPLAIN VERBOSE to test deparsing related changes, so will re-push
those shortly.

0004 perhaps doesn't play nicely with LLVM compilation but I don't yet
understand why.

--
Thanks, Amit Langote