Re: pgsql: Add more SQL/JSON constructor functions

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.

--
Thanks, Amit Langote