Re: pgsql: Add more SQL/JSON constructor functions

Hi,

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.

--
Thanks, Amit Langote