SQL/JSON features for v15

Hi,

(Personal hat, not RMT hat unless otherwise noted).

This thread[1] raised some concerns around the implementation of the
SQL/JSON features that are slated for v15, which includes an outstanding
open item[2]. Given the current state of the discussion, when the RMT
met on Aug 8, they several options, readable here[3]. Given we are now
into the later part of the release cycle, we need to make some decisions
on how to proceed with this feature given the concerns raised.

Per additional discussion on the thread, the group wanted to provide
more visibility into the discussion to get opinions on how to proceed
for the v15 release.

Without rehashing the thread, the options presented were:

1. Fixing the concerns addressed in the thread around the v15 SQL/JSON
features implementation, noting that this would likely entail at least
one more beta release and would push the GA date past our normal timeframe.

2. Try to commit a subset of the features that caused less debate. This
was ruled out.

3. Revert the v15 SQL/JSON features work.

<RMT hat>
Based on the current release timing and the open issues presented on the
thread, and the RMT had recommended reverting, but preferred to drive
consensus on next steps.
</RMT hat>

From a release advocacy standpoint, I need about 6 weeks lead time to
put together the GA launch. We're at the point where I typically deliver
a draft release announcement. From this, given this involves a high
visibility feature, I would want some clarity on what option we would
like to pursue. Once the announcement translation process has begun (and
this is when we have consensus on the release announcement), it becomes
more challenging to change things out.

From a personal standpoint (restating from[3]), I would like to see
what we could do to include support for this batch of the SQL/JSON
features in v15. What is included looks like it closes most of the gap
on what we've been missing syntactically since the standard was adopted,
and the JSON_TABLE work is very convenient for converting JSON data into
a relational format. I believe having this feature set is important for
maintaining standards compliance, interoperability, tooling support, and
general usability. Plus, JSON still seems to be pretty popular.

We're looking for additional input on what makes sense as a best course
of action, given what is presented in[3].

Thanks,

Jonathan

[1]
https://www.postgresql.org/message-id/flat/20220616233130.rparivafipt6doj3%40alap3.anarazel.de
[2] https://wiki.postgresql.org/wiki/PostgreSQL_15_Open_Items
[3]
https://www.postgresql.org/message-id/787cef45-15de-8f1d-ed58-a1c1435bfc0e%40postgresql.org

On 8/9/22 4:58 PM, Jonathan S. Katz wrote:

> We're looking for additional input on what makes sense as a best course
> of action, given what is presented in[3].

Missed adding Amit on the CC.

Jonathan

On 2022-08-09 Tu 16:58, Jonathan S. Katz wrote:
> Hi,
>
> (Personal hat, not RMT hat unless otherwise noted).
>
> This thread[1] raised some concerns around the implementation of the
> SQL/JSON features that are slated for v15, which includes an
> outstanding open item[2]. Given the current state of the discussion,
> when the RMT met on Aug 8, they several options, readable here[3].
> Given we are now into the later part of the release cycle, we need to
> make some decisions on how to proceed with this feature given the
> concerns raised.
>
> Per additional discussion on the thread, the group wanted to provide
> more visibility into the discussion to get opinions on how to proceed
> for the v15 release.
>
> Without rehashing the thread, the options presented were:
>
> 1. Fixing the concerns addressed in the thread around the v15 SQL/JSON
> features implementation, noting that this would likely entail at least
> one more beta release and would push the GA date past our normal
> timeframe.
>
> 2. Try to commit a subset of the features that caused less debate.
> This was ruled out.
>
> 3. Revert the v15 SQL/JSON features work.
>
> <RMT hat>
> Based on the current release timing and the open issues presented on
> the thread, and the RMT had recommended reverting, but preferred to
> drive consensus on next steps.
> </RMT hat>
>
> From a release advocacy standpoint, I need about 6 weeks lead time to
> put together the GA launch. We're at the point where I typically
> deliver a draft release announcement. From this, given this involves a
> high visibility feature, I would want some clarity on what option we
> would like to pursue. Once the announcement translation process has
> begun (and this is when we have consensus on the release
> announcement), it becomes more challenging to change things out.
>
> From a personal standpoint (restating from[3]), I would like to see
> what we could do to include support for this batch of the SQL/JSON
> features in v15. What is included looks like it closes most of the gap
> on what we've been missing syntactically since the standard was
> adopted, and the JSON_TABLE work is very convenient for converting
> JSON data into a relational format. I believe having this feature set
> is important for maintaining standards compliance, interoperability,
> tooling support, and general usability. Plus, JSON still seems to be
> pretty popular.
>
> We're looking for additional input on what makes sense as a best
> course of action, given what is presented in[3].
>
> Thanks,
>
> Jonathan
>
> [1]
> https://www.postgresql.org/message-id/flat/20220616233130.rparivafipt6doj3%40alap3.anarazel.de
> [2] https://wiki.postgresql.org/wiki/PostgreSQL_15_Open_Items
> [3]
> https://www.postgresql.org/message-id/787cef45-15de-8f1d-ed58-a1c1435bfc0e%40postgresql.org

To preserve options I will start preparing reversion patches. Given
there are I think more than 20 commits all told that could be fun, and
will probably take me a little while. The sad part is that to the best
of my knowledge this code is producing correct results, and not
disturbing the stability or performance of anything else. There was a
performance issue but it's been dealt with AIUI.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 8/10/22 11:50 AM, Andrew Dunstan wrote:
>
> On 2022-08-09 Tu 16:58, Jonathan S. Katz wrote:
>> Hi,
>>
>> (Personal hat, not RMT hat unless otherwise noted).
>>
>> This thread[1] raised some concerns around the implementation of the
>> SQL/JSON features that are slated for v15, which includes an
>> outstanding open item[2]. Given the current state of the discussion,
>> when the RMT met on Aug 8, they several options, readable here[3].
>> Given we are now into the later part of the release cycle, we need to
>> make some decisions on how to proceed with this feature given the
>> concerns raised.
>>
>> Per additional discussion on the thread, the group wanted to provide
>> more visibility into the discussion to get opinions on how to proceed
>> for the v15 release.
>>
>> Without rehashing the thread, the options presented were:
>>
>> 1. Fixing the concerns addressed in the thread around the v15 SQL/JSON
>> features implementation, noting that this would likely entail at least
>> one more beta release and would push the GA date past our normal
>> timeframe.
>>
>> 2. Try to commit a subset of the features that caused less debate.
>> This was ruled out.
>>
>> 3. Revert the v15 SQL/JSON features work.
>>
>> <RMT hat>
>> Based on the current release timing and the open issues presented on
>> the thread, and the RMT had recommended reverting, but preferred to
>> drive consensus on next steps.
>> </RMT hat>
>>
>> From a release advocacy standpoint, I need about 6 weeks lead time to
>> put together the GA launch. We're at the point where I typically
>> deliver a draft release announcement. From this, given this involves a
>> high visibility feature, I would want some clarity on what option we
>> would like to pursue. Once the announcement translation process has
>> begun (and this is when we have consensus on the release
>> announcement), it becomes more challenging to change things out.
>>
>> From a personal standpoint (restating from[3]), I would like to see
>> what we could do to include support for this batch of the SQL/JSON
>> features in v15. What is included looks like it closes most of the gap
>> on what we've been missing syntactically since the standard was
>> adopted, and the JSON_TABLE work is very convenient for converting
>> JSON data into a relational format. I believe having this feature set
>> is important for maintaining standards compliance, interoperability,
>> tooling support, and general usability. Plus, JSON still seems to be
>> pretty popular.
>>
>> We're looking for additional input on what makes sense as a best
>> course of action, given what is presented in[3].

> To preserve options I will start preparing reversion patches. Given
> there are I think more than 20 commits all told that could be fun, and
> will probably take me a little while. The sad part is that to the best
> of my knowledge this code is producing correct results, and not
> disturbing the stability or performance of anything else. There was a
> performance issue but it's been dealt with AIUI.

Personally, I hope we don't need to revert. If everything from the open
item standpoint is addressed, I want to ensure we capture and complete
the remaining issues that were raised on the other thread, i.e.

* adding design docs
* simplifying the type-coercion code
* another other design concerns that were presented

We switched this discussion out to a different thread to get some more
visibility on the issue and see if other folks would weigh in. Thus far,
there has not been much additional say either way. It would be good if
other folks chimed in.

Thanks,

Jonathan

Hi,

Continuation from the thread at
https://postgr.es/m/20220811171740.m5b4h7x63g4lzgrk%40awork3.anarazel.de

On 2022-08-11 10:17:40 -0700, Andres Freund wrote:
> On 2022-08-11 13:08:27 -0400, Jonathan S. Katz wrote:
> > With RMT hat on, Andres do you have any thoughts on this?
>
> I think I need to prototype how it'd look like to give a more detailed
> answer. I have a bunch of meetings over the next few hours, but after that I
> can give it a shot.

I started hacking on this Friday. I think there's some relatively easy
improvements that make the code substantially more understandable, at least
for me, without even addressing the structural stuff.

One thing I could use help understanding is the logic behind
ExecEvalJsonNeedsSubTransaction() - there's no useful comments, so it's hard
to follow.

bool
ExecEvalJsonNeedsSubTransaction(JsonExpr *jsexpr,
struct JsonCoercionsState *coercions)
{
/* want error to be raised, so clearly no subtrans needed */
if (jsexpr->on_error->btype == JSON_BEHAVIOR_ERROR)
return false;

if (jsexpr->op == JSON_EXISTS_OP && !jsexpr->result_coercion)
return false;

if (!coercions)
return true;

return false;
}

I guess the !coercions bit is just about the planner, where we want to be
pessimistic about when subtransactions are used, for the purpose of
parallelism? Because that's the only place that passes in NULL.

What really baffles me is that last 'return false' - it seems to indicate that
there's no paths during query execution where
ExecEvalJsonNeedsSubTransaction() returns true. And indeed, tests pass with an
Assert(!needSubtrans) added to ExecEvalJson() (and then unsurprisingly also
after removing the ExecEvalJsonExprSubtrans() indirection).

What's going on here?

We, somewhat confusingly, still rely on subtransactions, heavily
so. Responsible for that is this hunk of code:

bool throwErrors = jexpr->on_error->btype == JSON_BEHAVIOR_ERROR;
[...]
cxt.error = throwErrors ? NULL : &error;
cxt.coercionInSubtrans = !needSubtrans && !throwErrors;
Assert(!needSubtrans || cxt.error);

So basically we start a subtransaction inside ExecEvalJsonExpr(), to coerce
the result type, whenever !needSubtrans (which is always!), unless ERROR ON
ERROR is used.

Which then also explains the theory behind the EXISTS_OP check in
ExecEvalJsonNeedsSubTransaction(). In that case ExecEvalJsonExpr() returns
early, before doing a return value coercion, thus not starting a
subtransaction.

I don't think it's sane from a performance view to start a subtransaction for
every coercion, particularly because most coercion paths will never trigger an
error, leaving things like out-of-memory or interrupts aside. And those are
re-thrown by ExecEvalJsonExprSubtrans(). A quick and dirty benchmark shows
ERROR ON ERROR nearly 2xing speed. I'm worried about the system impact of
using subtransactions this heavily, it's not exactly the best performing
system - the only reason it's kind of ok here is that it's going to be very
rare to allocate a subxid, I think.

Next question:

/*
* We should catch exceptions of category ERRCODE_DATA_EXCEPTION and
* execute the corresponding ON ERROR behavior then.
*/
oldcontext = CurrentMemoryContext;
oldowner = CurrentResourceOwner;

Assert(error);

BeginInternalSubTransaction(NULL);
/* Want to execute expressions inside function's memory context */
MemoryContextSwitchTo(oldcontext);

PG_TRY();
{
res = func(op, econtext, res, resnull, p, error);

/* Commit the inner transaction, return to outer xact context */
ReleaseCurrentSubTransaction();
MemoryContextSwitchTo(oldcontext);
CurrentResourceOwner = oldowner;
}
PG_CATCH();
{
ErrorData *edata;
int ecategory;

/* Save error info in oldcontext */
MemoryContextSwitchTo(oldcontext);
edata = CopyErrorData();
FlushErrorState();

/* Abort the inner transaction */
RollbackAndReleaseCurrentSubTransaction();
MemoryContextSwitchTo(oldcontext);
CurrentResourceOwner = oldowner;

Two points:

1) I suspect it's not safe to switch to oldcontext before calling func().

On error we'll have leaked memory into oldcontext and we'll just continue
on. It might not be very consequential here, because the calling context
presumably isn't very long lived, but that's probably not something we should
rely on.

Also, are we sure that the context will be in a clean state when it's used
within an erroring subtransaction?

I think the right thing here would be to stay in the subtransaction context
and then copy the datum out to the surrounding context in the success case.

2) If there was an out-of-memory error, it'll have been in oldcontext. So
switching back to it before calling CopyErrorData() doesn't seem good - we'll
just hit OOM issues again.

I realize that both of these issues are present in plenty other code (see
e.g. plperl_spi_exec()). So I'm curious why they are ok?

Greetings,

Andres Freund

Hi,

On 16.08.2022 01:38, Andres Freund wrote:
> Continuation from the thread at
> https://postgr.es/m/20220811171740.m5b4h7x63g4lzgrk%40awork3.anarazel.de
>
>
> I started hacking on this Friday. I think there's some relatively easy
> improvements that make the code substantially more understandable, at least
> for me, without even addressing the structural stuff.

I also started hacking Friday, hacked all weekend, and now have a new
version of the patch.

I received your message when I finished writing of mine, so I will
try answer your new questions only in next message. But in short, I
can say that some things like ExecEvalJsonExprSubtrans() were fixed.

I took Amit's patch and tried to simplify execution further.
Explanation of the patches is at the very end of message.

Next, I try to answer some of previous questions.

On Aug 2, 2022 at 9:39 AM Andres Freund<andres(at)anarazel(dot)de> wrote:
> The whole coercion stuff just seems incredibly clunky (in a
> slightly different shape before this patch).
> ExecEvalJsonExprItemCoercion() calls ExecPrepareJsonItemCoercion(),
> which gets a pointer to one of the per-type elements in
> JsonItemCoercionsState, dispatching on the type of the json
> object. Then we later call ExecGetJsonItemCoercion() (via a
> convoluted path), which again will dispatch on the type
> (extracting the json object again afaics!), to then somehow
> eventually get the coerced value. I think it might be possible
> to make this a bit simpler, by not leaving anything
> coercion-related in ExecEvalJsonExpr().

On 2022-08-02 12:05:55 +0900, Amit Langote wrote:
> I left some pieces there, because I thought the error of not finding an
> appropriate coercion must be thrown right away as the code in
> ExecEvalJsonExpr() does after calling ExecGetJsonItemCoercion().

> ExecPrepareJsonItemCoercion() is called later when it's time to
> actually evaluate the coercion. If we move the error path to
> ExecPrepareJsonItemCoercion(), both ExecGetJsonItemCoercion() and the
> error path code in ExecEvalJsonExpr() will be unnecessary. I will
> give that a try.

The first dispatch is done only for throwing error about missing cast
without starting subtransaction in which second dispatch is executed.
I agree, this is bad that result of first dispatch is not used later,
and I have removed second dispatch.

> I don't understand the design of what needs to have error handling,
> and what not.

> I don't think subtransactions per-se are a fundamental problem.
> I think the error handling implementation is ridiculously complicated,
> and while I started to hack on improving it, I stopped when I really
> couldn't understand what errors it actually needs to handle when and
> why.

Here is the diagram that may help to understand error handling in
SQL/JSON functions (I hope it will be displayed correctly):

JSON path -------
expression \
->+-----------+ SQL/JSON +----------+ Result
PASSING args ------->| JSON path |--> item or --->| Output |-> SQL
->| executor | JSONB .->| Coercion | value
/ +-----------+ datum | +----------+
JSON + - - - -+ | | | |
Context ->: FORMAT : v v | v
item : JSON : error? empty? | error?
+ - - - -+ | | | |
| | +----------+ | /
v | | ON EMPTY |--> SQL --' /
error? | +----------+ value /
| | | /
\ | v /
\ \ error? /
\ \ | /
\______ \ | _____________/
\ \ | /
v v v v +----------+
+----------+ | Output | Result
| ON ERROR |--->| Coercion |--> SQL
+----------+ +----------+ value
| |
V V
EXCEPTION EXCEPTION

The first dashed box "FORMAT JSON" used for parsing JSON is absent in
our implementation, because we support only jsonb type which is
pre-parsed. This could be used in queries like that:
JSON_VALUE('invalid json', '$' DEFAULT 'error' ON ERROR) => 'error'

JSON path executor already has error handling and does not need
subtransactions. We had to add functions like numeric_add_opt_error()
which return error flag instead of throwing exceptions.

On Aug 10, 2022 at 3:57 AM Andres Freund<andres(at)anarazel(dot)de> wrote:
>>> One way this code could be drastically simplified is to force all
>>> type-coercions to go through the "io coercion" path, which could be
>>> implemented as a single execution step (which thus could trivially
>>> start/finish a subtransaction) and would remove a lot of the
>>> complicated code around coercions.

>> Could you please clarify how you think we might do the io coercion
>> wrapped with a subtransaction all as a single execution step? I
>> would've thought that we couldn't do the sub-transaction without
>> leaving ExecInterpExpr() anyway, so maybe you meant the io coercion
>> itself was done using some code outside ExecInterpExpr()?

>> The current JsonExpr code does it by recursively calling
>> ExecInterpExpr() using the nested ExprState expressly for the
>> coercion.

> The basic idea is to rip out all the type-dependent stuff out and
> replace it with a single JSON_IOCERCE step, which has a parameter
> about whether to wrap things in a subtransaction or not. That step
> would always perform the coercion by calling the text output function
> of the input and the text input function of the output.

On Aug 3, 2022 at 12:00 AM Andres Freund<andres(at)anarazel(dot)de> wrote:
> But we don't need to wrap arbitrary evaluation in a subtransaction -
> afaics the coercion calls a single function, not an arbitrary
> expression?

SQL standard says that scalar SQL/JSON items are converted to SQL type
through CAST(corresponding_SQL_type_for_item AS returning_type).
Our JSON_VALUE implementation supports arbitrary output types that can
have specific CASTs from numeric, bool, datetime, which we can't
emulate with simple I/O coercion. But supporting of arbitrary types
may be dangerous, because SQL standard denotes only a limited set of
types:

The <data type> contained in the explicit or implicit
<JSON returning clause> shall be a <predefined type> that identifies
a character string data type, numeric data type, boolean data type,
or datetime data type.

I/O coercion will not even work in the following simple case:
JSON_VALUE('1.23', '$' RETURNING int)
It is expected to return 1::int, like ordinary cast 1.23::numeric::int.

Exceptions may come not only from coercions. Expressions in DEFAULT ON
EMPTY can also throw exceptions, which also must be handled.

Here is excerpt from ISO/IEC 19075-6:2021(E) "Part 6: Support for JSON",
which explains SQL standard features in human-readable manner:

6.4.3 JSON_VALUE:

<JSON value error behavior> specifies what to do if there is an
unhandled error. Unhandled errors can arise if there is an input
conversion error (for example, if the context item cannot be parsed),
an error returned by the SQL/JSON path engine, or an output
conversion error. The choices are the same as for
<JSON value empty behavior>.

When using DEFAULT <value expression> for either the empty or error
behavior, what happens if the <value expression> raises an exception?
The answer is that an error during empty behavior "falls through"
to the error behavior. If the error behavior itself has an error,
there is no further recourse but to raise the exception.

So, we need to support execution of arbitrary expressions inside a
subtransaction, and do not try to somehow simplify coercions.

In Amit's fix, wrapping DEFAULT ON EMPTY into subtransactions was
lost, mainly because there were no tests for this case. The following
test should not fall on the second row:

SELECT JSON_VALUE(jsonb '1', '$.a' RETURNING int
DEFAULT 1 / x ON EMPTY
DEFAULT 2 ON ERROR)
FROM (VALUES (1::int), (0)) x(x);

json_value
------------
1
2

I have added this test in 0003.

On Aug 3, 2022 at 12:00 AM Andres Freund<andres(at)anarazel(dot)de> wrote:
> On 2022-08-02 12:05:55 +0900, Amit Langote wrote:
>> I am not really sure if different coercions may be used
>> in the same query over multiple evaluations of the same JSON path
>> expression, but maybe that's also possible.

> Even if the type can change, I don't think that means we need to have
> space for multiple types at the same time - there can't be multiple
> coercions happening at the same time, otherwise there could be two
> coercions of the same type as well. So we don't need memory for
> every coercion type.

Only the one item coercion is used in execution of JSON_VALUE().
Multiple coercions could be executed, if we supported quite useful SRF
JSON_QUERY() using "RETURNING SETOF type" (I had this idea for a long
time, but I didn't dare to implement it).

I don't understand what "memory" you mean. If we will not emit all
possible expressions statically, we will need to generate them
dynamically at run-time, and this could be hardly acceptable. In the
last version of the fix there is only 4 bytes (int jump) of additional
state space per coercion.

On Aug 6, 2022 at 5:37 Andres Freund<andres(at)anarazel(dot)de> wrote:
> There's one layer of subtransactions in one of the paths in
> ExecEvalJsonExpr(), another in ExecEvalJson(). Some paths of
> ExecEvalJsonExpr() go through subtransactions, others don't.

Really, there is only one level of subtransactions. Outer subtransaction
may be used for FORMAT JSON handling which always requires
subtransaction at the beginning of expression execution.
Inner subtransactions are conditional, they are started only and when
there is no outer subtransaction.

Now, outer subtransactions are not used at all,
ExecEvalJsonNeedsSubtransaction(NULL) always returns false. (AFAIR,
FORMAT JSON was present in older version of SQL/JSON patches, then it
was removed, but outer subtransactions were not). In the last version
of the fix I have removed them completely and moved inner
subtransactions into a separate executor step (see below).

The description of the patches:

0001 - Fix returning of json[b] domains in JSON_VALUE()

(This may require a separate thread.)

I found a bug in returning json[b] domains in JSON_VALUE(). json[b]
has special processing in JSON_VALUE, bypassing oridinary
SQL/JSON item type => SQL type coercions. But json[b] domains miss
this processing:

CREATE DOMAIN jsonb_not_null AS jsonb NOT NULL;

SELECT JSON_VALUE('"123"', '$' RETURNING jsonb);
"123"

SELECT JSON_VALUE( '123', '$' RETURNING jsonb);
123

SELECT JSON_VALUE('"123"', '$' RETURNING jsonb_not_null);
123

SELECT JSON_VALUE( '123', '$' RETURNING jsonb_not_null ERROR ON ERROR);
ERROR: SQL/JSON item cannot be cast to target type

Fixed by examinating output base type in parse_expr.c and skipping
allocation of item coercions, what later will be a signal for special
processing in ExecEvalJsonExpr().

0002 - Add EEOP_SUBTRANS executor step

On 2022-08-02 12:05:55 +0900, Amit Langote wrote:
> So, the problem with inlining coercion evaluation into the main parent
> JsonExpr's is that it needs to be wrapped in a sub-transaction to
> catch any errors and return NULL instead. I don't know a way to wrap
> ExprEvalStep evaluation in a sub-transaction to achieve that effect.

I also don't know way to run subtransactions without recursion in
executor, but I still managed to elimiate subsidary ExprStates.

I have introduced new EEOP_SUBTRANS step which executes its subsequent
steps in a subtransaction. It recursively calls a new variant of
ExecInterpExpr() in which starting stepno is passed. The return from
subtransaction is done with EEOP_DONE step that emitted after
subexpression. This step can be reused for other future expressions,
that's why it has no JSON prefix in its name (you could see recent
message in the thread about casts with default values, which are
missing in PostgreSQL).

But for JIT I still had to construct additional ExprState with a
function compiled from subexpression steps.

0003 - Simplify JsonExpr execution:

- New EEOP_SUBTRANS was used to wrap individual coercion expressions:
after execution it jumps to "done" or "onerror" step
- JSONEXPR_ITEM_COERCE step was removed
- JSONEXPR_COERCE split into JSONEXPR_IOCOERCE and JSONEXPR_POPULATE
- Removed all JsonExprPostEvalState
- JSONEXPR step simply returns jump address to one of its possible
continuations: done, onempty, onerror, coercion, coercion_subtrans,
io_coercion or one of item_coercions
- Fixed JsonExprNeedsSubTransaction(): considired more cases
- Eliminated transactions on Const expressions

--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company

Hi,

On 2022-08-15 15:38:53 -0700, Andres Freund wrote:
> Next question:
>
> /*
> * We should catch exceptions of category ERRCODE_DATA_EXCEPTION and
> * execute the corresponding ON ERROR behavior then.
> */
> oldcontext = CurrentMemoryContext;
> oldowner = CurrentResourceOwner;
>
> Assert(error);
>
> BeginInternalSubTransaction(NULL);
> /* Want to execute expressions inside function's memory context */
> MemoryContextSwitchTo(oldcontext);
>
> PG_TRY();
> {
> res = func(op, econtext, res, resnull, p, error);
>
> /* Commit the inner transaction, return to outer xact context */
> ReleaseCurrentSubTransaction();
> MemoryContextSwitchTo(oldcontext);
> CurrentResourceOwner = oldowner;
> }
> PG_CATCH();
> {
> ErrorData *edata;
> int ecategory;
>
> /* Save error info in oldcontext */
> MemoryContextSwitchTo(oldcontext);
> edata = CopyErrorData();
> FlushErrorState();
>
> /* Abort the inner transaction */
> RollbackAndReleaseCurrentSubTransaction();
> MemoryContextSwitchTo(oldcontext);
> CurrentResourceOwner = oldowner;
>
>
> Two points:
>
> 1) I suspect it's not safe to switch to oldcontext before calling func().
>
> On error we'll have leaked memory into oldcontext and we'll just continue
> on. It might not be very consequential here, because the calling context
> presumably isn't very long lived, but that's probably not something we should
> rely on.
>
> Also, are we sure that the context will be in a clean state when it's used
> within an erroring subtransaction?
>
>
> I think the right thing here would be to stay in the subtransaction context
> and then copy the datum out to the surrounding context in the success case.
>
>
> 2) If there was an out-of-memory error, it'll have been in oldcontext. So
> switching back to it before calling CopyErrorData() doesn't seem good - we'll
> just hit OOM issues again.
>
>
> I realize that both of these issues are present in plenty other code (see
> e.g. plperl_spi_exec()). So I'm curious why they are ok?

Certainly seems to be missing a FreeErrorData() for the happy path?

It'd be nicer if we didn't copy the error. In the case we rethrow we don't
need it, because we can just PG_RE_THROW(). And in the other path we just want
to get the error code. It just risks additional errors to CopyErrorData(). But
it's not entirely obvious that geterrcode() is intended for this:

* This is only intended for use in error callback subroutines, since there
* is no other place outside elog.c where the concept is meaningful.
*/

a PG_CATCH() block isn't really an error callback subroutine. But it should be
fine.

Greetings,

Andres Freund

Hi,

On 2022-08-16 04:02:17 +0300, Nikita Glukhov wrote:
> Hi,
>
>
> On 16.08.2022 01:38, Andres Freund wrote:
> > Continuation from the thread at
> > https://postgr.es/m/20220811171740.m5b4h7x63g4lzgrk%40awork3.anarazel.de
> >
> >
> > I started hacking on this Friday. I think there's some relatively easy
> > improvements that make the code substantially more understandable, at least
> > for me, without even addressing the structural stuff.
>
> I also started hacking Friday, hacked all weekend, and now have a new
> version of the patch.

Cool.

> > I don't understand the design of what needs to have error handling,
> > and what not.
>
> > I don't think subtransactions per-se are a fundamental problem.
> > I think the error handling implementation is ridiculously complicated,
> > and while I started to hack on improving it, I stopped when I really
> > couldn't understand what errors it actually needs to handle when and
> > why.
>
> Here is the diagram that may help to understand error handling in
> SQL/JSON functions (I hope it will be displayed correctly):

I think that is helpful.

> JSON path -------
> expression \
> ->+-----------+ SQL/JSON +----------+ Result
> PASSING args ------->| JSON path |--> item or --->| Output |-> SQL
> ->| executor | JSONB .->| Coercion | value
> / +-----------+ datum | +----------+
> JSON + - - - -+ | | | |
> Context ->: FORMAT : v v | v
> item : JSON : error? empty? | error?
> + - - - -+ | | | |
> | | +----------+ | /
> v | | ON EMPTY |--> SQL --' /
> error? | +----------+ value /
> | | | /
> \ | v /
> \ \ error? /
> \ \ | /
> \______ \ | _____________/
> \ \ | /
> v v v v +----------+
> +----------+ | Output | Result
> | ON ERROR |--->| Coercion |--> SQL
> +----------+ +----------+ value
> | |
> V V
> EXCEPTION EXCEPTION
>
>
> The first dashed box "FORMAT JSON" used for parsing JSON is absent in
> our implementation, because we support only jsonb type which is
> pre-parsed. This could be used in queries like that:
> JSON_VALUE('invalid json', '$' DEFAULT 'error' ON ERROR) => 'error'

> On Aug 3, 2022 at 12:00 AM Andres Freund<andres(at)anarazel(dot)de> wrote:
> > But we don't need to wrap arbitrary evaluation in a subtransaction -
> > afaics the coercion calls a single function, not an arbitrary
> > expression?
>
> SQL standard says that scalar SQL/JSON items are converted to SQL type
> through CAST(corresponding_SQL_type_for_item AS returning_type).
> Our JSON_VALUE implementation supports arbitrary output types that can
> have specific CASTs from numeric, bool, datetime, which we can't
> emulate with simple I/O coercion. But supporting of arbitrary types
> may be dangerous, because SQL standard denotes only a limited set of
> types:
>
> The <data type> contained in the explicit or implicit
> <JSON returning clause> shall be a <predefined type> that identifies
> a character string data type, numeric data type, boolean data type,
> or datetime data type.
>
> I/O coercion will not even work in the following simple case:
> JSON_VALUE('1.23', '$' RETURNING int)
> It is expected to return 1::int, like ordinary cast 1.23::numeric::int.

Whether it's just IO coercions or also coercions through function calls
doesn't matter terribly, as long as both can be wrapped as a single
interpretation step. You can have a EEOP_JSON_COERCE_IO,
EEOP_JSON_COERCE_FUNC that respectively call input/output function and the
transformation routine within a subtransaction. On error they can jump to some
on_error execution step.

The difficulty is likely just dealing with the intermediary nodes like
RelabelType.

> Exceptions may come not only from coercions. Expressions in DEFAULT ON
> EMPTY can also throw exceptions, which also must be handled.

Are there other cases?

> Only the one item coercion is used in execution of JSON_VALUE().
> Multiple coercions could be executed, if we supported quite useful SRF
> JSON_QUERY() using "RETURNING SETOF type" (I had this idea for a long
> time, but I didn't dare to implement it).
>
> I don't understand what "memory" you mean.

I'm not entirely sure what I meant at that time either. Understanding this
code involves a lot of guessing since there's practically no explanatory
comments.

> If we will not emit all possible expressions statically, we will need to
> generate them dynamically at run-time, and this could be hardly acceptable.

I'm not convinced that that's true. We spend a fair amount of memory
generating expression paths for the per-type elements in JsonItemCoercions,
most of which will never be used. Even trivial stuff ends up with ~2kB.

Then there's of course the executor side, where the various ExprStates really
add up:
MemoryContextStats(CurrentMemoryContext) in ExecInitExprRec(), just before
if (jext->coercions)

ExecutorState: 8192 total in 1 blocks; 4464 free (0 chunks); 3728 used
ExprContext: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
Grand total: 16384 bytes in 2 blocks; 12392 free (0 chunks); 3992 used

just after:

ExecutorState: 32768 total in 3 blocks; 15032 free (2 chunks); 17736 used
ExprContext: 8192 total in 1 blocks; 7928 free (0 chunks); 264 used
Grand total: 40960 bytes in 4 blocks; 22960 free (2 chunks); 18000 used

for SELECT JSON_VALUE(NULL::jsonb, '$');

> In the last version of the fix there is only 4 bytes (int jump) of
> additional state space per coercion.

That's certainly a *lot* better.

> On Aug 6, 2022 at 5:37 Andres Freund<andres(at)anarazel(dot)de> wrote:
> > There's one layer of subtransactions in one of the paths in
> > ExecEvalJsonExpr(), another in ExecEvalJson(). Some paths of
> > ExecEvalJsonExpr() go through subtransactions, others don't.
>
> Really, there is only one level of subtransactions. Outer subtransaction
> may be used for FORMAT JSON handling which always requires
> subtransaction at the beginning of expression execution.
> Inner subtransactions are conditional, they are started only and when
> there is no outer subtransaction.

Yea, I realized that by now as well. But the code doesn't make that
understandable. E.g.:

> Now, outer subtransactions are not used at all,
> ExecEvalJsonNeedsSubtransaction(NULL) always returns false. (AFAIR,
> FORMAT JSON was present in older version of SQL/JSON patches, then it
> was removed, but outer subtransactions were not).

is very misleading.

> 0002 - Add EEOP_SUBTRANS executor step
>
> On 2022-08-02 12:05:55 +0900, Amit Langote wrote:
> > So, the problem with inlining coercion evaluation into the main parent
> > JsonExpr's is that it needs to be wrapped in a sub-transaction to
> > catch any errors and return NULL instead. I don't know a way to wrap
> > ExprEvalStep evaluation in a sub-transaction to achieve that effect.
>
> I also don't know way to run subtransactions without recursion in
> executor, but I still managed to elimiate subsidary ExprStates.
>
> I have introduced new EEOP_SUBTRANS step which executes its subsequent
> steps in a subtransaction. It recursively calls a new variant of
> ExecInterpExpr() in which starting stepno is passed. The return from
> subtransaction is done with EEOP_DONE step that emitted after
> subexpression. This step can be reused for other future expressions,
> that's why it has no JSON prefix in its name (you could see recent
> message in the thread about casts with default values, which are
> missing in PostgreSQL).

I've wondered about this as well, but I think it'd require quite careful work
to be safe. And certainly isn't something we can do at this point in the cycle
- it'll potentially impact every query, not just ones with json in, if we
screw up something (or introduce overhead).

> But for JIT I still had to construct additional ExprState with a
> function compiled from subexpression steps.

JIT is one of the reason *not* want to construct subsidiary ExprState's, since
they will trigger separate code generation (and thus overhead).

Why did you have to do this?

> 0003 - Simplify JsonExpr execution:
>
> - New EEOP_SUBTRANS was used to wrap individual coercion expressions:
> after execution it jumps to "done" or "onerror" step
> - JSONEXPR_ITEM_COERCE step was removed
> - JSONEXPR_COERCE split into JSONEXPR_IOCOERCE and JSONEXPR_POPULATE
> - Removed all JsonExprPostEvalState
> - JSONEXPR step simply returns jump address to one of its possible
> continuations: done, onempty, onerror, coercion, coercion_subtrans,
> io_coercion or one of item_coercions
> - Fixed JsonExprNeedsSubTransaction(): considired more cases
> - Eliminated transactions on Const expressions

I pushed a few cleanups to https://github.com/anarazel/postgres/commits/json
while I was hacking on this (ignore that it's based on the meson tree, that's
just faster for me). Some of them might not be applicable anymore, but it
might still make sense for you to look at.

Greetings,

Andres Freund

On Mon, Aug 15, 2022 at 6:39 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> I don't think it's sane from a performance view to start a subtransaction for
> every coercion, particularly because most coercion paths will never trigger an
> error, leaving things like out-of-memory or interrupts aside. And those are
> re-thrown by ExecEvalJsonExprSubtrans(). A quick and dirty benchmark shows
> ERROR ON ERROR nearly 2xing speed. I'm worried about the system impact of
> using subtransactions this heavily, it's not exactly the best performing
> system - the only reason it's kind of ok here is that it's going to be very
> rare to allocate a subxid, I think.

I agree. It kinda surprises me that we thought it was OK to commit
something that uses that many subtransactions. I feel like that's
going to cause people to hose themselves in ways that we can't really
do anything about. Like they'll test it out, it will work, and then
when they put it into production, they'll have constant wraparound
issues for which the only real solution is to not use the feature they
relied on to build the application.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 8/15/22 10:14 PM, Andres Freund wrote:

> I pushed a few cleanups to https://github.com/anarazel/postgres/commits/json
> while I was hacking on this (ignore that it's based on the meson tree, that's
> just faster for me). Some of them might not be applicable anymore, but it
> might still make sense for you to look at.

With RMT hat on, this appears to be making progress. A few questions /
comments for the group:

1. Nikita: Did you have a chance to review Andres's changes as well?

2. There seems to be some, though limited, progress on design docs.
Andres keeps making a point on adding additional comments to the code to
make it easier to follow. Please do not lose sight of this.

3. Robert raised a point about the use of subtransactions and the
increased risk of wraparound on busy systems using the SQL/JSON
features. Do these patches help reduce this risk? I read some clarity on
the use of subtransactions within the patchset, but want to better
understand if the risks pointed out are a concern.

Thanks everyone for your work on this so far!

Jonathan

Hi,

On 17.08.2022 04:45, Jonathan S. Katz wrote:
>
> On 8/15/22 10:14 PM, Andres Freund wrote:
>
>> I pushed a few cleanups to
>> https://github.com/anarazel/postgres/commits/json
>> while I was hacking on this (ignore that it's based on the meson
>> tree, that's
>> just faster for me). Some of them might not be applicable anymore,
>> but it
>> might still make sense for you to look at.
>
> With RMT hat on, this appears to be making progress. A few questions /
> comments for the group:
>
> 1. Nikita: Did you have a chance to review Andres's changes as well?

Yes, I have reviewed Andres's changes, they all are ok.

Then I started to do on the top of it other fixes that help to avoid
subtransactions when they are not needed. And it ended in the new
refactoring of coercion code. Also I moved here from v6-0003 fix of
ExecEvalJsonNeedSubtransaction() which considers more cases.

On 16.08.2022 05:14, Andres Freund wrote:
>> But for JIT I still had to construct additional ExprState with a
>> function compiled from subexpression steps.

> Why did you have to do this?

I simply did not dare to implement compilation of recursively-callable
function with additional parameter stepno. In the v8 patch I did it
by adding a switch with all possible jump addresses of EEOP_SUBTRANS
steps in the beginning of the function. And it really seems to work
faster, but needs more exploration. See patch 0003, where both
variants preserved using #ifdef.

The desciprion of the v7 patches:

0001 Simplify JsonExpr execution
Andres's changes + mine:
- Added JsonCoercionType enum, fields like via_io replaced with it
- Emit only context item steps in JSON_TABLE_OP case
- Skip coercion of NULLs to non-domain types (is it correct?)

0002 Fix returning of json[b] domains in JSON_VALUE:
simply rebase of v6 onto 0001

0003 Add EEOP_SUBTRANS executor step
v6 + new recursive JIT

0004 Split JsonExpr execution into steps
simply rebase of v6 + used LLMBuildSwitch() in EEOP_JSONEXPR

--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company

Hi,

On 8/17/22 11:45 PM, Nikita Glukhov wrote:
> Hi,
>
> On 17.08.2022 04:45, Jonathan S. Katz wrote:
>>
>> On 8/15/22 10:14 PM, Andres Freund wrote:
>>
>>> I pushed a few cleanups to
>>> https://github.com/anarazel/postgres/commits/json
>>> while I was hacking on this (ignore that it's based on the meson
>>> tree, that's
>>> just faster for me). Some of them might not be applicable anymore,
>>> but it
>>> might still make sense for you to look at.
>>
>> With RMT hat on, this appears to be making progress. A few questions /
>> comments for the group:
>>
>> 1. Nikita: Did you have a chance to review Andres's changes as well?
>
> Yes, I have reviewed Andres's changes, they all are ok.

Thank you!

> Then I started to do on the top of it other fixes that help to avoid
> subtransactions when they are not needed. And it ended in the new
> refactoring of coercion code. Also I moved here from v6-0003 fix of
> ExecEvalJsonNeedSubtransaction() which considers more cases.

Great.

Andres, Robert: Do these changes address your concerns about the use of
substransactions and reduce the risk of xid wraparound?

> On 16.08.2022 05:14, Andres Freund wrote:
>>> But for JIT I still had to construct additional ExprState with a
>>> function compiled from subexpression steps.
>
>> Why did you have to do this?
>
> I simply did not dare to implement compilation of recursively-callable
> function with additional parameter stepno. In the v8 patch I did it
> by adding a switch with all possible jump addresses of EEOP_SUBTRANS
> steps in the beginning of the function. And it really seems to work
> faster, but needs more exploration. See patch 0003, where both
> variants preserved using #ifdef.
>
>
> The desciprion of the v7 patches:
>
> 0001 Simplify JsonExpr execution
> Andres's changes + mine:
> - Added JsonCoercionType enum, fields like via_io replaced with it
> - Emit only context item steps in JSON_TABLE_OP case
> - Skip coercion of NULLs to non-domain types (is it correct?)
>
> 0002 Fix returning of json[b] domains in JSON_VALUE:
> simply rebase of v6 onto 0001
>
> 0003 Add EEOP_SUBTRANS executor step
> v6 + new recursive JIT
>
> 0004 Split JsonExpr execution into steps
> simply rebase of v6 + used LLMBuildSwitch() in EEOP_JSONEXPR

What do folks think of these patches?

Thanks,

Jonathan

On 8/19/22 10:11 AM, Jonathan S. Katz wrote:
> Hi,
>
> On 8/17/22 11:45 PM, Nikita Glukhov wrote:
>> Hi,
>>
>> On 17.08.2022 04:45, Jonathan S. Katz wrote:
>>>
>>> On 8/15/22 10:14 PM, Andres Freund wrote:
>>>
>>>> I pushed a few cleanups to
>>>> https://github.com/anarazel/postgres/commits/json
>>>> while I was hacking on this (ignore that it's based on the meson
>>>> tree, that's
>>>> just faster for me). Some of them might not be applicable anymore,
>>>> but it
>>>> might still make sense for you to look at.
>>>
>>> With RMT hat on, this appears to be making progress. A few questions
>>> / comments for the group:
>>>
>>> 1. Nikita: Did you have a chance to review Andres's changes as well?
>>
>> Yes, I have reviewed Andres's changes, they all are ok.
>
> Thank you!
>
>> Then I started to do on the top of it other fixes that help to avoid
>> subtransactions when they are not needed. And it ended in the new
>> refactoring of coercion code.  Also I moved here from v6-0003 fix of
>> ExecEvalJsonNeedSubtransaction() which considers more cases.
>
> Great.
>
> Andres, Robert: Do these changes address your concerns about the use of
> substransactions and reduce the risk of xid wraparound?
>
>> On 16.08.2022 05:14, Andres Freund wrote:
>>>> But for JIT I still had to construct additional ExprState with a
>>>> function compiled from subexpression steps.
>>
>>> Why did you have to do this?
>>
>> I simply did not dare to implement compilation of recursively-callable
>> function with additional parameter stepno.  In the v8 patch I did it
>> by adding a switch with all possible jump addresses of EEOP_SUBTRANS
>> steps in the beginning of the function.  And it really seems to work
>> faster, but needs more exploration.  See patch 0003, where both
>> variants preserved using #ifdef.
>>
>>
>> The desciprion of the v7 patches:
>>
>> 0001 Simplify JsonExpr execution
>>   Andres's changes + mine:
>>    - Added JsonCoercionType enum, fields like via_io replaced with it
>>    - Emit only context item steps in JSON_TABLE_OP case
>>    - Skip coercion of NULLs to non-domain types (is it correct?)
>>
>> 0002 Fix returning of json[b] domains in JSON_VALUE:
>>    simply rebase of v6 onto 0001
>>
>> 0003 Add EEOP_SUBTRANS executor step
>>    v6 + new recursive JIT
>>
>> 0004 Split JsonExpr execution into steps
>>    simply rebase of v6 + used LLMBuildSwitch() in EEOP_JSONEXPR
>
> What do folks think of these patches?

Andres, Andrew, Amit, Robert -- as you have either worked on this or
expressed opinions -- any thoughts on this current patch set?

Thanks,

Jonathan

On Tue, Aug 23, 2022 at 10:52 AM Jonathan S. Katz <jkatz(at)postgresql(dot)org> wrote:
> Andres, Andrew, Amit, Robert -- as you have either worked on this or
> expressed opinions -- any thoughts on this current patch set?

FWIW, I've started looking at these patches.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

Hi,

On 2022-08-22 21:52:01 -0400, Jonathan S. Katz wrote:
> Andres, Andrew, Amit, Robert -- as you have either worked on this or
> expressed opinions -- any thoughts on this current patch set?

To me it feels like there's a probably too much work here to cram it at this
point. If several other committers shared the load of working on this it'd
perhaps be doable, but I've not seen many volunteers.

Greetings,

Andres Freund

On Mon, Aug 22, 2022 at 07:57:29PM -0700, Andres Freund wrote:
> To me it feels like there's a probably too much work here to cram it at this
> point. If several other committers shared the load of working on this it'd
> perhaps be doable, but I've not seen many volunteers.

While 0002 is dead simple, I am worried about the complexity created
by 0001, 0003 (particularly tightening subtransactions with a
CASE_EEOP) and 0004 at this late stage of the release process:
https://www.postgresql.org/message-id/7d83684b-7932-9f29-400b-0beedfafcdd4@postgrespro.ru

This is not a good sign after three betas for a feature as complex as
this one.
--
Michael

Hi Nikita,

On Thu, Aug 18, 2022 at 12:46 PM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
> The desciprion of the v7 patches:
>
> 0001 Simplify JsonExpr execution
> Andres's changes + mine:
> - Added JsonCoercionType enum, fields like via_io replaced with it
> - Emit only context item steps in JSON_TABLE_OP case
> - Skip coercion of NULLs to non-domain types (is it correct?)

I like the parser changes to add JsonCoercionType, because that makes
ExecEvalJsonExprCoercion() so much simpler to follow.

In coerceJsonExpr():

+ if (!allow_io_coercion)
+ return NULL;
+

Might it make more sense to create a JsonCoercion even in this case
and assign it the type JSON_COERCION_ERROR, rather than allow the
coercion to be NULL and doing the following in ExecInitExprRec():

+ if (!*coercion)
+ /* Missing coercion here means missing cast */
+ cstate->type = JSON_COERCION_ERROR;

Likewise in transformJsonFuncExpr():

+ if (coercion_expr != (Node *) placeholder)
+ {
+ jsexpr->result_coercion = makeNode(JsonCoercion);
+ jsexpr->result_coercion->expr = coercion_expr;
+ jsexpr->result_coercion->ctype = JSON_COERCION_VIA_EXPR;
+ }

How about creating a JSON_COERCION_NONE coercion in the else block of
this, just like coerceJsonExpr() does?

Related to that, the JSON_EXISTS_OP block in
ExecEvalJsonExprInternal() sounds to assume that result_coercion would
always be non-NULL, per the comment in the last line:

case JSON_EXISTS_OP:
{
bool exists = JsonPathExists(item, path,
jsestate->args,
error);

*resnull = error && *error;
res = BoolGetDatum(exists);
break; /* always use result coercion */
}

...but it won't be if the above condition is false?

> 0002 Fix returning of json[b] domains in JSON_VALUE:
> simply rebase of v6 onto 0001

Especially after seeing the new comments in this one, I'm wondering if
it makes sense to rename result_coercion to, say, default_coercion?

> 0003 Add EEOP_SUBTRANS executor step
> v6 + new recursive JIT
>
> 0004 Split JsonExpr execution into steps
> simply rebase of v6 + used LLMBuildSwitch() in EEOP_JSONEXPR

Will need to spend more time looking at these.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On Tue, Aug 23, 2022 at 4:48 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Thu, Aug 18, 2022 at 12:46 PM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
> > The desciprion of the v7 patches:
> >
> > 0003 Add EEOP_SUBTRANS executor step
> > v6 + new recursive JIT
> >
> > 0004 Split JsonExpr execution into steps
> > simply rebase of v6 + used LLMBuildSwitch() in EEOP_JSONEXPR
>
> Will need to spend more time looking at these.

0004 adds the following to initJsonItemCoercions():

+ /* When returning JSON types, no need to initialize coercions */
+ /* XXX domain types on json/jsonb */
+ if (returning->typid == JSONBOID || returning->typid == JSONOID)
+ return NULL;

But maybe it's dead code, because 0001 has this:

+ if (jsexpr->returning->typid != JSONOID &&
+ jsexpr->returning->typid != JSONBOID)
+ jsexpr->coercions =
+ initJsonItemCoercions(pstate, jsexpr->returning,
+ exprType(contextItemExpr));

+ /* We need to handle RETURNING int etc. */

Is this a TODO and what does it mean?

+ * "JsonCoercion == NULL" means no cast is available.
+ * "JsonCoercion.expr == NULL" means no coercion is needed.

As said in my previous email, I wonder if these cases are better
handled by adding JSON_COERCION_ERROR and JSON_COERCION_NONE
coercions?

+/* Skip calling ExecEvalJson() on a JsonExpr? */

ExecEvalJsonExpr()

Will look more.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On 8/23/22 12:13 AM, Michael Paquier wrote:
> On Mon, Aug 22, 2022 at 07:57:29PM -0700, Andres Freund wrote:
>> To me it feels like there's a probably too much work here to cram it at this
>> point. If several other committers shared the load of working on this it'd
>> perhaps be doable, but I've not seen many volunteers.
>
> While 0002 is dead simple, I am worried about the complexity created
> by 0001, 0003 (particularly tightening subtransactions with a
> CASE_EEOP) and 0004 at this late stage of the release process:
> https://www.postgresql.org/message-id/7d83684b-7932-9f29-400b-0beedfafcdd4@postgrespro.ru
>
> This is not a good sign after three betas for a feature as complex as
> this one.

I see Amit is taking a closer look at the patches.

The RMT had its regular meeting today and discussed the state of
progress on this and how it reflects release timing. Our feeling is that
regardless if the patchset is included/reverted, it would necessitate a
Beta 4 (to be discussed with release team). While no Beta 4 date is set,
given where we are this would probably push the release into early
October to allow for adequate testing time.

To say it another way, if we want to ensure we can have a 15.1 in the
November update releases, we need to make a decision soon on how we want
to proceed.

Thanks,

Jonathan

On Mon, Aug 22, 2022 at 9:52 PM Jonathan S. Katz <jkatz(at)postgresql(dot)org> wrote:
> > Andres, Robert: Do these changes address your concerns about the use of
> > substransactions and reduce the risk of xid wraparound?
>
> Andres, Andrew, Amit, Robert -- as you have either worked on this or
> expressed opinions -- any thoughts on this current patch set?

I do not think that using subtransactions as part of the expression
evaluation process is a sound idea pretty much under any
circumstances. Maybe if the subtransations aren't commonly created and
don't usually get XIDs there wouldn't be a big problem in practice,
but it's an awfully heavyweight operation to be done inside expression
evaluation even in corner cases. I think that if we need to make
certain operations that would throw errors not throw errors, we need
to refactor interfaces until it's possible to return an error
indicator up to the appropriate level, not just let the error be
thrown and catch it.

The patches in question are thousands of lines of new code that I
simply do not have time or interest to review in detail. I didn't
commit this feature, or write this feature, or review this feature.
I'm not familiar with any of the code. To really know what's going on
here, I would need to review not only the new patches but also all the
code in the original commits, and probably some of the preexisting
code from before those commits that I have never examined in the past.
That would take me quite a few months even if I had absolutely nothing
else to do. And because I haven't been involved in this patch set in
any way, I don't think it's really my responsibility.

At the end of the day, the RMT is going to have to take a call here.
It seems to me that Andres's concerns about code quality and lack of
comments are probably somewhat legitimate, and in particular I do not
think the use of subtransactions is a good idea. I also don't think
that trying to fix those problems or generally improve the code by
committing thousands of lines of new code in August when we're
targeting a release in September or October is necessarily a good
idea. But I'm also not in a position to say that the project is going
to be irreparably damaged if we just ship what we've got, perhaps
after fixing the most acute problems that we currently know about.
This is after all relatively isolated from the rest of the system.
Fixing the stuff that touches the core executor is probably pretty
important, but beyond that, the worst thing that happens is the
feature sucks and people who try to use it have bad experiences. That
would be bad, and might be a sufficient reason to revert, but it's not
nearly as bad as, say, the whole system being slow, or data loss for
every user, or something like that. And we do have other bad code in
the system. Is this a lot worse? I'm not in a position to say one way
or the other.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> At the end of the day, the RMT is going to have to take a call here.
> It seems to me that Andres's concerns about code quality and lack of
> comments are probably somewhat legitimate, and in particular I do not
> think the use of subtransactions is a good idea. I also don't think
> that trying to fix those problems or generally improve the code by
> committing thousands of lines of new code in August when we're
> targeting a release in September or October is necessarily a good
> idea. But I'm also not in a position to say that the project is going
> to be irreparably damaged if we just ship what we've got, perhaps
> after fixing the most acute problems that we currently know about.

The problem here is that this was going to be a headline new feature
for v15. Shipping what apparently is only an alpha-quality implementation
seems pretty problematic unless we advertise it as such, and that's
not something we've done very much in the past. I also wonder how
much any attempts at fixing it later would be constrained by concerns
about compatibility with the v15 version.

> ... And we do have other bad code in the system.

Can't deny that, but a lot of it is legacy code that we wish we could
rip out and can't because backwards compatibility. This is not legacy
code ... not yet anyway.

As you say, we've delegated this sort of decision to the RMT, but
if I were on the RMT I'd be voting to revert.

regards, tom lane

Hi,

On 2022-08-23 11:08:31 -0400, Tom Lane wrote:
> As you say, we've delegated this sort of decision to the RMT, but
> if I were on the RMT I'd be voting to revert.

Yea, I don't really see an alternative at this point. If we really wanted we
could try to cut the more complicated pieces out, e.g., by only supporting
ERROR ON ERROR, but I'm not sure it'd get us far enough.

Greetings,

Andres Freund

On 2022-08-23 Tu 11:08, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> At the end of the day, the RMT is going to have to take a call here.
>> It seems to me that Andres's concerns about code quality and lack of
>> comments are probably somewhat legitimate, and in particular I do not
>> think the use of subtransactions is a good idea. I also don't think
>> that trying to fix those problems or generally improve the code by
>> committing thousands of lines of new code in August when we're
>> targeting a release in September or October is necessarily a good
>> idea. But I'm also not in a position to say that the project is going
>> to be irreparably damaged if we just ship what we've got, perhaps
>> after fixing the most acute problems that we currently know about.
> The problem here is that this was going to be a headline new feature
> for v15. Shipping what apparently is only an alpha-quality implementation
> seems pretty problematic unless we advertise it as such, and that's
> not something we've done very much in the past. I also wonder how
> much any attempts at fixing it later would be constrained by concerns
> about compatibility with the v15 version.
>
>> ... And we do have other bad code in the system.
> Can't deny that, but a lot of it is legacy code that we wish we could
> rip out and can't because backwards compatibility. This is not legacy
> code ... not yet anyway.
>
> As you say, we've delegated this sort of decision to the RMT, but
> if I were on the RMT I'd be voting to revert.
>
>

I know I previously said that this was not really severable, but I've
started having second thoughts about that. If we disabled as Not
Implemented the DEFAULT form of the ON ERROR and ON EMPTY clauses, and
possibly the RETURNING clause in some cases, it's possible we could get
rid of most of what's been controversial. That could still leave us a
good deal of what we want, including JSON_TABLE, which is by far the
most interesting of these features. I haven't looked closely yet at how
possible this is, it only occurred to me today, but I think it's worth
exploring.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi,

On 2022-08-23 10:51:04 -0400, Robert Haas wrote:
> I do not think that using subtransactions as part of the expression
> evaluation process is a sound idea pretty much under any
> circumstances. Maybe if the subtransations aren't commonly created and
> don't usually get XIDs there wouldn't be a big problem in practice,
> but it's an awfully heavyweight operation to be done inside expression
> evaluation even in corner cases. I think that if we need to make
> certain operations that would throw errors not throw errors, we need
> to refactor interfaces until it's possible to return an error
> indicator up to the appropriate level, not just let the error be
> thrown and catch it.

I don't think that's quite realistic - that's the input/output functions for
all types, basically. I'd be somewhat content if we'd a small list of very
common coercion paths we knew wouldn't error out, leaving things like OOM
aside. Even just knowing that for ->text conversions would be a huge deal in
the context of this patch. One problem here is that the whole type coercion
infrastructure doesn't make it easy to know what "happened inside" atm, one
has to reconstruct it from the emitted expressions, where there can be
multiple layers of things to poke through.

Greetings,

Andres Freund

Hi

út 23. 8. 2022 v 17:55 odesílatel Andres Freund <andres(at)anarazel(dot)de> napsal:

> Hi,
>
> On 2022-08-23 10:51:04 -0400, Robert Haas wrote:
> > I do not think that using subtransactions as part of the expression
> > evaluation process is a sound idea pretty much under any
> > circumstances. Maybe if the subtransations aren't commonly created and
> > don't usually get XIDs there wouldn't be a big problem in practice,
> > but it's an awfully heavyweight operation to be done inside expression
> > evaluation even in corner cases. I think that if we need to make
> > certain operations that would throw errors not throw errors, we need
> > to refactor interfaces until it's possible to return an error
> > indicator up to the appropriate level, not just let the error be
> > thrown and catch it.
>
> I don't think that's quite realistic - that's the input/output functions
> for
> all types, basically. I'd be somewhat content if we'd a small list of very
> common coercion paths we knew wouldn't error out, leaving things like OOM
> aside. Even just knowing that for ->text conversions would be a huge deal
> in
> the context of this patch. One problem here is that the whole type
> coercion
> infrastructure doesn't make it easy to know what "happened inside" atm, one
> has to reconstruct it from the emitted expressions, where there can be
> multiple layers of things to poke through.
>

The errors that should be handled are related to json structure errors. I
don't think so we have to handle all errors and all conversions.

The JSON knows only three types - and these conversions can be written
specially for this case - or we can write json io routines to be able to
signal error
without an exception.

Regards

Pavel

>
> Greetings,
>
> Andres Freund
>
>
>

On Tue, Aug 23, 2022 at 11:55 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> I don't think that's quite realistic - that's the input/output functions for
> all types, basically. I'd be somewhat content if we'd a small list of very
> common coercion paths we knew wouldn't error out, leaving things like OOM
> aside. Even just knowing that for ->text conversions would be a huge deal in
> the context of this patch. One problem here is that the whole type coercion
> infrastructure doesn't make it easy to know what "happened inside" atm, one
> has to reconstruct it from the emitted expressions, where there can be
> multiple layers of things to poke through.

But that's exactly what I'm complaining about. Catching an error that
unwound a bunch of stack frames where complicated things are happening
is fraught with peril. There's probably a bunch of errors that could
be thrown from somewhere in that code - out of memory being a great
example - that should not be caught. What you (probably) want is to
know whether one specific error happened or not, and catch only that
one. And the error machinery isn't designed for that. It's not
designed to let you catch specific errors for specific call sites, and
it's also not designed to be particularly efficient if lots of errors
need to be caught over and over again. If you decide to ignore all
that and do it anyway, you'll end up with, at best, code that is
complicated, hard to maintain, and probably slow when a lot of errors
are trapped, and at worst, code that is fragile or outright buggy.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 23.08.2022 19:06, Pavel Stehule wrote:
> Hi
>
> út 23. 8. 2022 v 17:55 odesílatel Andres Freund <andres(at)anarazel(dot)de>
> napsal:
>
> Hi,
>
> On 2022-08-23 10:51:04 -0400, Robert Haas wrote:
> > I do not think that using subtransactions as part of the expression
> > evaluation process is a sound idea pretty much under any
> > circumstances. Maybe if the subtransations aren't commonly
> created and
> > don't usually get XIDs there wouldn't be a big problem in practice,
> > but it's an awfully heavyweight operation to be done inside
> expression
> > evaluation even in corner cases. I think that if we need to make
> > certain operations that would throw errors not throw errors, we need
> > to refactor interfaces until it's possible to return an error
> > indicator up to the appropriate level, not just let the error be
> > thrown and catch it.
>
> I don't think that's quite realistic - that's the input/output
> functions for
> all types, basically.  I'd be somewhat content if we'd a small
> list of very
> common coercion paths we knew wouldn't error out, leaving things
> like OOM
> aside. Even just knowing that for ->text conversions would be a
> huge deal in
> the context of this patch.  One problem here is that the whole
> type coercion
> infrastructure doesn't make it easy to know what "happened inside"
> atm, one
> has to reconstruct it from the emitted expressions, where there can be
> multiple layers of things to poke through.
>
>
> The errors that should be handled are related to json structure
> errors. I don't think so we have to handle all errors and all conversions.
>
> The JSON knows only three types - and these conversions can be written
> specially for this case - or we can write json io routines to be able
> to signal error
> without an exception.

I also wanted to suggest to limit the set of returning types to the
predefined set of JSON-compatible types for which can write safe
conversion functions: character types (text, char), boolean, number
types (integers, floats types, numeric), datetime types. The SQL
standard even does not require support of other returning types.

For the float8 and datetime types we already have safe input functions
like float8in_internal_opt_error() and parse_datetime() which are used
inside jsonpath and return error code instead of throwing errors.
We need to implement numeric_intN_safe() and maybe a few other trivial
functions like that.

The set of returning types, for which we do not need any special
coercions, is very limited: json, jsonb, text. More precisely,
even RETURNING json[b] can throw errors in JSON_QUERY(OMIT QUOTES),
and we also need safe json parsing, but it can be easily done
with pg_parse_json(), which returns error code.

On 8/23/22 11:08 AM, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> At the end of the day, the RMT is going to have to take a call here.
>> It seems to me that Andres's concerns about code quality and lack of
>> comments are probably somewhat legitimate, and in particular I do not
>> think the use of subtransactions is a good idea. I also don't think
>> that trying to fix those problems or generally improve the code by
>> committing thousands of lines of new code in August when we're
>> targeting a release in September or October is necessarily a good
>> idea. But I'm also not in a position to say that the project is going
>> to be irreparably damaged if we just ship what we've got, perhaps
>> after fixing the most acute problems that we currently know about.
>
> The problem here is that this was going to be a headline new feature
> for v15. Shipping what apparently is only an alpha-quality implementation
> seems pretty problematic unless we advertise it as such, and that's
> not something we've done very much in the past.

With my user hat on, we have done this before -- if inadvertently -- but
agree it's not recommended nor a habit we should get into.

> As you say, we've delegated this sort of decision to the RMT, but
> if I were on the RMT I'd be voting to revert.

With RMT hat on,the RMT does have power of forced commit/revert in
absence of consensus through regular community processes[1]. We did
explicitly discuss at our meeting today if we were going to make the
decision right now. We decided that we would come back and set a
deadline on letting the community processes play out, otherwise we will
make the decision.

For decision deadline: if there is no community consensus by end of Aug
28, 2022 AoE, the RMT will make the decision. I know Andrew has been
prepping for the outcome of a revert -- this should give enough for
review and merge prior to a Beta 4 release (targeted for Sep 8). If
there is concern about that, the RMT can move up the decision timeframe.

Taking RMT hat off, if the outcome is "revert", I do want to ensure we
don't lose momentum on getting this into v16. I know a lot of time and
effort has gone into this featureset and it seems to be trending in the
right direction. We have a mixed history on reverts in terms of if/when
they are committed and I don't want to see that happen to these
features. I do think this will remain a headline feature even if we
delay it for v16.

I saw Andrew suggest that the controversial parts of the patchset may be
severable from some of the new functionality, so I would like to see
that proposal and if it is enough to overcome concerns.

Thanks,

Jonathan

[1] https://wiki.postgresql.org/wiki/Release_Management_Team

Hi,

On 2022-08-23 12:26:55 -0400, Robert Haas wrote:
> On Tue, Aug 23, 2022 at 11:55 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > I don't think that's quite realistic - that's the input/output functions for
> > all types, basically. I'd be somewhat content if we'd a small list of very
> > common coercion paths we knew wouldn't error out, leaving things like OOM
> > aside. Even just knowing that for ->text conversions would be a huge deal in
> > the context of this patch. One problem here is that the whole type coercion
> > infrastructure doesn't make it easy to know what "happened inside" atm, one
> > has to reconstruct it from the emitted expressions, where there can be
> > multiple layers of things to poke through.
>
> But that's exactly what I'm complaining about. Catching an error that
> unwound a bunch of stack frames where complicated things are happening
> is fraught with peril. There's probably a bunch of errors that could
> be thrown from somewhere in that code - out of memory being a great
> example - that should not be caught.

The code as is handles this to some degree. Only ERRCODE_DATA_EXCEPTION,
ERRCODE_INTEGRITY_CONSTRAINT_VIOLATION are caught, the rest is immediately
rethrown.

> What you (probably) want is to know whether one specific error happened or
> not, and catch only that one. And the error machinery isn't designed for
> that. It's not designed to let you catch specific errors for specific call
> sites, and it's also not designed to be particularly efficient if lots of
> errors need to be caught over and over again. If you decide to ignore all
> that and do it anyway, you'll end up with, at best, code that is
> complicated, hard to maintain, and probably slow when a lot of errors are
> trapped, and at worst, code that is fragile or outright buggy.

I'm not sure what the general alternative is though. Part of the feature is
generating a composite type from json - there's just no way we can make all
possible coercion pathways not error out. That'd necessitate requiring all
builtin types and extensions types out there to provide input functions that
don't throw on invalid input and all coercions to not throw either. That just
seems unrealistic.

I think the best we could without subtransactions do perhaps is to add
metadata to pg_cast, pg_type telling us whether certain types of errors are
possible, and requiring ERROR ON ERROR when coercion paths are required that
don't have those options.

Greetings,

Andres Freund

"Jonathan S. Katz" <jkatz(at)postgresql(dot)org> writes:
> I saw Andrew suggest that the controversial parts of the patchset may be
> severable from some of the new functionality, so I would like to see
> that proposal and if it is enough to overcome concerns.

It's an interesting suggestion. Do people have the cycles available
to make it happen in the next few days?

regards, tom lane

Hi,

On 2022-08-23 13:18:49 -0400, Jonathan S. Katz wrote:
> Taking RMT hat off, if the outcome is "revert", I do want to ensure we don't
> lose momentum on getting this into v16. I know a lot of time and effort has
> gone into this featureset and it seems to be trending in the right
> direction. We have a mixed history on reverts in terms of if/when they are
> committed and I don't want to see that happen to these features. I do think
> this will remain a headline feature even if we delay it for v16.

We could decide to revert this for 15, but leave it in tree for HEAD.

Greetings,

Andres Freund

Hi,

On 2022-08-23 18:06:22 +0200, Pavel Stehule wrote:
> The errors that should be handled are related to json structure errors. I
> don't think so we have to handle all errors and all conversions.
>
> The JSON knows only three types - and these conversions can be written
> specially for this case - or we can write json io routines to be able to
> signal error
> without an exception.

I think that's not true unfortunately. You can specify return types, and
composite types can be populated. Which essentially requires arbitrary
coercions.

Greetings,

Andres Freund

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2022-08-23 12:26:55 -0400, Robert Haas wrote:
>> But that's exactly what I'm complaining about. Catching an error that
>> unwound a bunch of stack frames where complicated things are happening
>> is fraught with peril. There's probably a bunch of errors that could
>> be thrown from somewhere in that code - out of memory being a great
>> example - that should not be caught.

> The code as is handles this to some degree. Only ERRCODE_DATA_EXCEPTION,
> ERRCODE_INTEGRITY_CONSTRAINT_VIOLATION are caught, the rest is immediately
> rethrown.

That's still a lot of territory, considering how nonspecific most
SQLSTATEs are. Even if you can prove that only the intended cases
are caught today, somebody could inadvertently break it next week
by using one of those codes somewhere else.

I agree with the upthread comments that we only need/want to catch
foreseeable incorrect-input errors, and that the way to make that
happen is to refactor the related type input functions, and that
a lot of the heavy lifting for that has been done already.

regards, tom lane

On Tue, Aug 23, 2022 at 1:23 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > But that's exactly what I'm complaining about. Catching an error that
> > unwound a bunch of stack frames where complicated things are happening
> > is fraught with peril. There's probably a bunch of errors that could
> > be thrown from somewhere in that code - out of memory being a great
> > example - that should not be caught.
>
> The code as is handles this to some degree. Only ERRCODE_DATA_EXCEPTION,
> ERRCODE_INTEGRITY_CONSTRAINT_VIOLATION are caught, the rest is immediately
> rethrown.

AFAIK, Tom has rejected every previous effort to introduce this type
of coding into the tree rather forcefully. What makes it OK now?

> I'm not sure what the general alternative is though. Part of the feature is
> generating a composite type from json - there's just no way we can make all
> possible coercion pathways not error out. That'd necessitate requiring all
> builtin types and extensions types out there to provide input functions that
> don't throw on invalid input and all coercions to not throw either. That just
> seems unrealistic.

Well, I think that having input functions report input that is not
valid for the data type in some way other than just chucking an error
as they'd also do for a missing TOAST chunk would be a pretty sensible
plan. I'd support doing that if we forced a hard compatibility break,
and I'd support that if we provided some way for old code to continue
running in degraded mode. I haven't thought too much about the
coercion case, but I suppose the issues are similar. What I don't
support is saying -- well, upgrading our infrastructure is hard, so
let's just kludge it.

--
Robert Haas
EDB: http://www.enterprisedb.com

út 23. 8. 2022 v 19:27 odesílatel Andres Freund <andres(at)anarazel(dot)de> napsal:

> Hi,
>
> On 2022-08-23 18:06:22 +0200, Pavel Stehule wrote:
> > The errors that should be handled are related to json structure errors. I
> > don't think so we have to handle all errors and all conversions.
> >
> > The JSON knows only three types - and these conversions can be written
> > specially for this case - or we can write json io routines to be able to
> > signal error
> > without an exception.
>
> I think that's not true unfortunately. You can specify return types, and
> composite types can be populated. Which essentially requires arbitrary
> coercions.
>

Please, can you send an example? Maybe we try to fix a feature that is not
required by standard.

Regards

Pavel

>
> Greetings,
>
> Andres Freund
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> út 23. 8. 2022 v 19:27 odesílatel Andres Freund <andres(at)anarazel(dot)de> napsal:
>> I think that's not true unfortunately. You can specify return types, and
>> composite types can be populated. Which essentially requires arbitrary
>> coercions.

> Please, can you send an example? Maybe we try to fix a feature that is not
> required by standard.

Even if it is required by spec, I'd have zero hesitation about tossing
that case overboard if that's what we need to do to get to a shippable
feature.

regards, tom lane

On 2022-08-23 Tu 13:24, Tom Lane wrote:
> "Jonathan S. Katz" <jkatz(at)postgresql(dot)org> writes:
>> I saw Andrew suggest that the controversial parts of the patchset may be
>> severable from some of the new functionality, so I would like to see
>> that proposal and if it is enough to overcome concerns.
> It's an interesting suggestion. Do people have the cycles available
> to make it happen in the next few days?
>
>

I will make time although probably Nikita and/or Amit would be quicker
than I would be.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 23.08.2022 20:38, Pavel Stehule wrote:
> út 23. 8. 2022 v 19:27 odesílatel Andres Freund <andres(at)anarazel(dot)de>
> napsal:
>
> Hi,
>
> On 2022-08-23 18:06:22 +0200, Pavel Stehule wrote:
> > The errors that should be handled are related to json structure
> errors. I
> > don't think so we have to handle all errors and all conversions.
> >
> > The JSON knows only three types - and these conversions can be
> written
> > specially for this case - or we can write json io routines to be
> able to
> > signal error
> > without an exception.
>
> I think that's not true unfortunately. You can specify return
> types, and
> composite types can be populated. Which essentially requires arbitrary
> coercions.
>
>
> Please, can you send an example? Maybe we try to fix a feature that is
> not required by standard.

- Returning arbitrary types in JSON_VALUE using I/O coercion
from JSON string (more precisely, text::arbitrary_type cast):

SELECT JSON_QUERY(jsonb '"1, 2"', '$' RETURNING point);
json_query
------------
(1,2)
(1 row)

- Returning composite and array types in JSON_QUERY, which is implemented
reusing the code of our json[b]_populate_record[set]():

SELECT JSON_QUERY(jsonb '[1, "2", null]', '$' RETURNING int[]);
json_query
------------
{1,2,NULL}
(1 row)

--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company

Hi,

On 2022-08-23 13:33:42 -0400, Robert Haas wrote:
> On Tue, Aug 23, 2022 at 1:23 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > But that's exactly what I'm complaining about. Catching an error that
> > > unwound a bunch of stack frames where complicated things are happening
> > > is fraught with peril. There's probably a bunch of errors that could
> > > be thrown from somewhere in that code - out of memory being a great
> > > example - that should not be caught.
> >
> > The code as is handles this to some degree. Only ERRCODE_DATA_EXCEPTION,
> > ERRCODE_INTEGRITY_CONSTRAINT_VIOLATION are caught, the rest is immediately
> > rethrown.
>
> AFAIK, Tom has rejected every previous effort to introduce this type
> of coding into the tree rather forcefully. What makes it OK now?

I didn't say it was! I don't like it much - I was just saying that it handles
that case to some degree.

> > I'm not sure what the general alternative is though. Part of the feature is
> > generating a composite type from json - there's just no way we can make all
> > possible coercion pathways not error out. That'd necessitate requiring all
> > builtin types and extensions types out there to provide input functions that
> > don't throw on invalid input and all coercions to not throw either. That just
> > seems unrealistic.
>
> Well, I think that having input functions report input that is not
> valid for the data type in some way other than just chucking an error
> as they'd also do for a missing TOAST chunk would be a pretty sensible
> plan. I'd support doing that if we forced a hard compatibility break,
> and I'd support that if we provided some way for old code to continue
> running in degraded mode. I haven't thought too much about the
> coercion case, but I suppose the issues are similar. What I don't
> support is saying -- well, upgrading our infrastructure is hard, so
> let's just kludge it.

I guess the 'degraded mode' approach is kind of what I was trying to describe
with:

> I think the best we could without subtransactions do perhaps is to add
> metadata to pg_cast, pg_type telling us whether certain types of errors are
> possible, and requiring ERROR ON ERROR when coercion paths are required that
> don't have those options.

Greetings,

Andres Freund

Hi,

On 2022-08-23 13:28:50 -0400, Tom Lane wrote:
> I agree with the upthread comments that we only need/want to catch
> foreseeable incorrect-input errors, and that the way to make that
> happen is to refactor the related type input functions, and that
> a lot of the heavy lifting for that has been done already.

I think it's a good direction to go in. What of the heavy lifting for that has
been done already? I'd have guessed that the hard part is to add different,
optional, type input, type coercion signatures, and then converting a lot of
types to that?

Greetings,

Andres Freund

On 8/23/22 2:10 PM, Andrew Dunstan wrote:
>
> On 2022-08-23 Tu 13:24, Tom Lane wrote:
>> "Jonathan S. Katz" <jkatz(at)postgresql(dot)org> writes:
>>> I saw Andrew suggest that the controversial parts of the patchset may be
>>> severable from some of the new functionality, so I would like to see
>>> that proposal and if it is enough to overcome concerns.
>> It's an interesting suggestion. Do people have the cycles available
>> to make it happen in the next few days?
>>
> I will make time although probably Nikita and/or Amit would be quicker
> than I would be.

If you all can, you have my +1 to try it and see what folks think.

Thanks,

Jonathan

On 8/23/22 1:26 PM, Andres Freund wrote:
> Hi,
>
> On 2022-08-23 13:18:49 -0400, Jonathan S. Katz wrote:
>> Taking RMT hat off, if the outcome is "revert", I do want to ensure we don't
>> lose momentum on getting this into v16. I know a lot of time and effort has
>> gone into this featureset and it seems to be trending in the right
>> direction. We have a mixed history on reverts in terms of if/when they are
>> committed and I don't want to see that happen to these features. I do think
>> this will remain a headline feature even if we delay it for v16.
>
> We could decide to revert this for 15, but leave it in tree for HEAD.

If it comes to that, I think that is a reasonable suggestion so long as
we're committed to making the requisite changes.

Thanks,

Jonathan

On 2022-08-23 Tu 15:32, Jonathan S. Katz wrote:
> On 8/23/22 1:26 PM, Andres Freund wrote:
>> Hi,
>>
>> On 2022-08-23 13:18:49 -0400, Jonathan S. Katz wrote:
>>> Taking RMT hat off, if the outcome is "revert", I do want to ensure
>>> we don't
>>> lose momentum on getting this into v16. I know a lot of time and
>>> effort has
>>> gone into this featureset and it seems to be trending in the right
>>> direction. We have a mixed history on reverts in terms of if/when
>>> they are
>>> committed and I don't want to see that happen to these features. I
>>> do think
>>> this will remain a headline feature even if we delay it for v16.
>>
>> We could decide to revert this for 15, but leave it in tree for HEAD.
>
> If it comes to that, I think that is a reasonable suggestion so long
> as we're committed to making the requisite changes.
>
>

One good reason for this is that way we're not fighting against the node
changes, which complicate any reversion significantly.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2022-08-23 13:28:50 -0400, Tom Lane wrote:
>> I agree with the upthread comments that we only need/want to catch
>> foreseeable incorrect-input errors, and that the way to make that
>> happen is to refactor the related type input functions, and that
>> a lot of the heavy lifting for that has been done already.

> I think it's a good direction to go in. What of the heavy lifting for that has
> been done already? I'd have guessed that the hard part is to add different,
> optional, type input, type coercion signatures, and then converting a lot of
> types to that?

I was assuming that we would only bother to do this for a few core types.
Of those, at least the datetime types were already done for previous
JSON-related features. If we want extensibility, then as Robert said
there's going to have to be work done to create a common API that type
input functions can implement, which seems like a pretty heavy lift.
We could get it done for v16 if we start now, I imagine.

regards, tom lane

út 23. 8. 2022 v 21:54 odesílatel Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> napsal:

> Andres Freund <andres(at)anarazel(dot)de> writes:
> > On 2022-08-23 13:28:50 -0400, Tom Lane wrote:
> >> I agree with the upthread comments that we only need/want to catch
> >> foreseeable incorrect-input errors, and that the way to make that
> >> happen is to refactor the related type input functions, and that
> >> a lot of the heavy lifting for that has been done already.
>
> > I think it's a good direction to go in. What of the heavy lifting for
> that has
> > been done already? I'd have guessed that the hard part is to add
> different,
> > optional, type input, type coercion signatures, and then converting a
> lot of
> > types to that?
>
> I was assuming that we would only bother to do this for a few core types.
> Of those, at least the datetime types were already done for previous
> JSON-related features. If we want extensibility, then as Robert said
> there's going to have to be work done to create a common API that type
> input functions can implement, which seems like a pretty heavy lift.
> We could get it done for v16 if we start now, I imagine.
>

+1

Pavel

> regards, tom lane
>
>
>

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> On 2022-08-23 Tu 15:32, Jonathan S. Katz wrote:
>> On 8/23/22 1:26 PM, Andres Freund wrote:
>>> We could decide to revert this for 15, but leave it in tree for HEAD.

>> If it comes to that, I think that is a reasonable suggestion so long
>> as we're committed to making the requisite changes.

I'm not particularly on board with that. In the first place, I'm
unconvinced that very much of the current code will survive, and
I don't want people contorting the rewrite in order to salvage
committed code that would be better off junked. In the second
place, if we still don't have a shippable feature in a year, then
undoing it again is going to be just that much harder.

> One good reason for this is that way we're not fighting against the node
> changes, which complicate any reversion significantly.

Having said that, I'm prepared to believe that a lot of the node
infrastructure won't change because it's dictated by the SQL-spec
grammar. So we could leave that part alone in HEAD; at worst
it adds some dead code in backend/nodes.

regards, tom lane

On 23.08.2022 22:31, Jonathan S. Katz wrote:
> On 8/23/22 2:10 PM, Andrew Dunstan wrote:
>>
>> On 2022-08-23 Tu 13:24, Tom Lane wrote:
>>> "Jonathan S. Katz" <jkatz(at)postgresql(dot)org> writes:
>>>> I saw Andrew suggest that the controversial parts of the patchset
>>>> may be
>>>> severable from some of the new functionality, so I would like to see
>>>> that proposal and if it is enough to overcome concerns.
>>> It's an interesting suggestion.  Do people have the cycles available
>>> to make it happen in the next few days?
>>>
>> I will make time although probably Nikita and/or Amit would be quicker
>> than I would be.
>
> If you all can, you have my +1 to try it and see what folks think.

I am ready to start hacking now, but it's already night in Moscow, so
any result will be only tomorrow.

Here is my plan:

0. Take my last v7-0001 patch as a base. It already contains refactoring
of JsonCoercion code. (Fix 0002 is not needed anymore, because it is for
json[b] domains, which simply will not be supported.)

1. Replace JSON_COERCION_VIA_EXPR in JsonCoercion with new
JsonCoercionType(s) for hardcoded coercions.

2. Disable all non-JSON-compatible output types in coerceJsonFuncExpr().

3. Add missing safe type input functions for integers, numerics, and
maybe others.

4. Implement hardcoded coercions using these functions in
ExecEvalJsonExprCoercion().

5. Try to allow only constants (and also maybe column/parameter
references) in JSON_VALUE's DEFAULT expressions. This should be enough
for the most of practical cases. JSON_QUERY even does not have DEFAULT
expressions -- it has only EMPTY ARRAY and EMPTY OBJECT, which can be
treated as simple JSON constants.

But it is possible to allow all other expressions in ERROR ON ERROR
case, and I don't know if it will be consistent enough to allow some
expressions in one case and deny in other.

And there is another problem: expressions can be only checked for
Const-ness only after expression simplification. AFAIU, at the
parsing stage they look like 'string'::type. So, it's unclear if it
is correct to check expressions in ExecInitExpr().

6. Remove subtransactions.

--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company

On 2022-08-23 Tu 17:29, Nikita Glukhov wrote:
>
>
> On 23.08.2022 22:31, Jonathan S. Katz wrote:
>> On 8/23/22 2:10 PM, Andrew Dunstan wrote:
>>>
>>> On 2022-08-23 Tu 13:24, Tom Lane wrote:
>>>> "Jonathan S. Katz" <jkatz(at)postgresql(dot)org> writes:
>>>>> I saw Andrew suggest that the controversial parts of the patchset
>>>>> may be
>>>>> severable from some of the new functionality, so I would like to see
>>>>> that proposal and if it is enough to overcome concerns.
>>>> It's an interesting suggestion.  Do people have the cycles available
>>>> to make it happen in the next few days?
>>>>
>>> I will make time although probably Nikita and/or Amit would be quicker
>>> than I would be.
>>
>> If you all can, you have my +1 to try it and see what folks think.
> I am ready to start hacking now, but it's already night in Moscow, so
> any result will be only tomorrow.
>
> Here is my plan:
>
> 0. Take my last v7-0001 patch as a base. It already contains refactoring
> of JsonCoercion code. (Fix 0002 is not needed anymore, because it is for
> json[b] domains, which simply will not be supported.)
>
> 1. Replace JSON_COERCION_VIA_EXPR in JsonCoercion with new
> JsonCoercionType(s) for hardcoded coercions.
>
> 2. Disable all non-JSON-compatible output types in coerceJsonFuncExpr().
>
> 3. Add missing safe type input functions for integers, numerics, and
> maybe others.
>
> 4. Implement hardcoded coercions using these functions in
> ExecEvalJsonExprCoercion().
>
> 5. Try to allow only constants (and also maybe column/parameter
> references) in JSON_VALUE's DEFAULT expressions. This should be enough
> for the most of practical cases. JSON_QUERY even does not have DEFAULT
> expressions -- it has only EMPTY ARRAY and EMPTY OBJECT, which can be
> treated as simple JSON constants.

er, really? This is from the regression output:

SELECT JSON_QUERY(jsonb '[]', '$[*]' DEFAULT '"empty"' ON EMPTY);
 json_query
------------
 "empty"
(1 row)

SELECT JSON_QUERY(jsonb '[1,2]', '$[*]' DEFAULT '"empty"' ON ERROR);
 json_query
------------
 "empty"
(1 row)

>
> But it is possible to allow all other expressions in ERROR ON ERROR
> case, and I don't know if it will be consistent enough to allow some
> expressions in one case and deny in other.
>
> And there is another problem: expressions can be only checked for
> Const-ness only after expression simplification. AFAIU, at the
> parsing stage they look like 'string'::type. So, it's unclear if it
> is correct to check expressions in ExecInitExpr().
>
> 6. Remove subtransactions.
>

Sounds like a good plan, modulo the issues in item 5. I would rather
lose some features temporarily than try to turn handsprings to make them
work and jeopardize the rest.

I'll look forward to seeing your patch in the morning :-)

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi Nikita,

On Wed, Aug 24, 2022 at 6:29 AM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
> Here is my plan:
>
> 0. Take my last v7-0001 patch as a base. It already contains refactoring
> of JsonCoercion code. (Fix 0002 is not needed anymore, because it is for
> json[b] domains, which simply will not be supported.)
>
> 1. Replace JSON_COERCION_VIA_EXPR in JsonCoercion with new
> JsonCoercionType(s) for hardcoded coercions.
>
> 2. Disable all non-JSON-compatible output types in coerceJsonFuncExpr().
>
> 3. Add missing safe type input functions for integers, numerics, and
> maybe others.
>
> 4. Implement hardcoded coercions using these functions in
> ExecEvalJsonExprCoercion().
>
> 5. Try to allow only constants (and also maybe column/parameter
> references) in JSON_VALUE's DEFAULT expressions. This should be enough
> for the most of practical cases. JSON_QUERY even does not have DEFAULT
> expressions -- it has only EMPTY ARRAY and EMPTY OBJECT, which can be
> treated as simple JSON constants.
>
> But it is possible to allow all other expressions in ERROR ON ERROR
> case, and I don't know if it will be consistent enough to allow some
> expressions in one case and deny in other.
>
> And there is another problem: expressions can be only checked for
> Const-ness only after expression simplification. AFAIU, at the
> parsing stage they look like 'string'::type. So, it's unclear if it
> is correct to check expressions in ExecInitExpr().

IIUC, the idea is to remove the support for `DEFAULT expression` in
the following, no?

json_value ( context_item, path_expression
...
[ { ERROR | NULL | DEFAULT expression } ON EMPTY ]
[ { ERROR | NULL | DEFAULT expression } ON ERROR ])

json_query ( context_item, path_expression
...
[ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
ON EMPTY ]
[ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
ON ERROR ])

If that's the case, I'd imagine that `default_expr` in the following
will be NULL for now:

/*
* JsonBehavior -
* representation of JSON ON ... BEHAVIOR clause
*/
typedef struct JsonBehavior
{
NodeTag type;
JsonBehaviorType btype; /* behavior type */
Node *default_expr; /* default expression, if any */
} JsonBehavior;

And if so, no expression left to check the Const-ness of?

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On Wed, Aug 24, 2022 at 11:55 AM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Wed, Aug 24, 2022 at 6:29 AM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
> > Here is my plan:
> >
> > 0. Take my last v7-0001 patch as a base. It already contains refactoring
> > of JsonCoercion code. (Fix 0002 is not needed anymore, because it is for
> > json[b] domains, which simply will not be supported.)
> >
> > 1. Replace JSON_COERCION_VIA_EXPR in JsonCoercion with new
> > JsonCoercionType(s) for hardcoded coercions.
> >
> > 2. Disable all non-JSON-compatible output types in coerceJsonFuncExpr().
> >
> > 3. Add missing safe type input functions for integers, numerics, and
> > maybe others.
> >
> > 4. Implement hardcoded coercions using these functions in
> > ExecEvalJsonExprCoercion().
> >
> > 5. Try to allow only constants (and also maybe column/parameter
> > references) in JSON_VALUE's DEFAULT expressions. This should be enough
> > for the most of practical cases. JSON_QUERY even does not have DEFAULT
> > expressions -- it has only EMPTY ARRAY and EMPTY OBJECT, which can be
> > treated as simple JSON constants.
> >
> > But it is possible to allow all other expressions in ERROR ON ERROR
> > case, and I don't know if it will be consistent enough to allow some
> > expressions in one case and deny in other.
> >
> > And there is another problem: expressions can be only checked for
> > Const-ness only after expression simplification. AFAIU, at the
> > parsing stage they look like 'string'::type. So, it's unclear if it
> > is correct to check expressions in ExecInitExpr().
>
> IIUC, the idea is to remove the support for `DEFAULT expression` in
> the following, no?
>
> json_value ( context_item, path_expression
> ...
> [ { ERROR | NULL | DEFAULT expression } ON EMPTY ]
> [ { ERROR | NULL | DEFAULT expression } ON ERROR ])
>
> json_query ( context_item, path_expression
> ...
> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> ON EMPTY ]
> [ { ERROR | NULL | EMPTY { [ ARRAY ] | OBJECT } | DEFAULT expression }
> ON ERROR ])

Or is the idea rather to restrict the set of data types we allow in `[
RETURNING data_type ]`?

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

Hi,

On 24.08.2022 01:12, Andrew Dunstan wrote:
> On 2022-08-23 Tu 17:29, Nikita Glukhov wrote:
>
>> Here is my plan:
>>
>> 0. Take my last v7-0001 patch as a base. It already contains refactoring
>> of JsonCoercion code. (Fix 0002 is not needed anymore, because it is for
>> json[b] domains, which simply will not be supported.)
>>
>> 1. Replace JSON_COERCION_VIA_EXPR in JsonCoercion with new
>> JsonCoercionType(s) for hardcoded coercions.

JsonCoerion node were completely removed because they are not needed
anymore (see p. 4).

>> 2. Disable all non-JSON-compatible output types in coerceJsonFuncExpr().
>>
>> 3. Add missing safe type input functions for integers, numerics, and
>> maybe others.

We need to write much more functions, than I expected. And I still didn't
implemented safe input functions for numeric and datetime types.
I will start to do it tomorrow.

>> 4. Implement hardcoded coercions using these functions in
>> ExecEvalJsonExprCoercion().

That was done using simple `switch (returning_typid) { .. }`,
which can be nested into `switch (jbv->type)`.

>> 5. Try to allow only constants (and also maybe column/parameter
>> references) in JSON_VALUE's DEFAULT expressions. This should be enough
>> for the most of practical cases. JSON_QUERY even does not have DEFAULT
>> expressions -- it has only EMPTY ARRAY and EMPTY OBJECT, which can be
>> treated as simple JSON constants.

I have not tried to implement this yet.

> er, really? This is from the regression output:
>
>
> SELECT JSON_QUERY(jsonb '[]', '$[*]' DEFAULT '"empty"' ON EMPTY);
> json_query
> ------------
> "empty"
> (1 row)
>
> SELECT JSON_QUERY(jsonb '[1,2]', '$[*]' DEFAULT '"empty"' ON ERROR);
> json_query
> ------------
> "empty"
> (1 row)
>
This is another extension. SQL standard defines only
EMPTY ARRAY an EMPTY OBJECT behavior for JSON_QUERY:

<JSON query empty behavior> ::=
ERROR
| NULL
| EMPTY ARRAY
| EMPTY OBJECT

<JSON query error behavior> ::=
ERROR
| NULL
| EMPTY ARRAY
| EMPTY OBJECT

>> But it is possible to allow all other expressions in ERROR ON ERROR
>> case, and I don't know if it will be consistent enough to allow some
>> expressions in one case and deny in other.
>>
>> And there is another problem: expressions can be only checked for
>> Const-ness only after expression simplification. AFAIU, at the
>> parsing stage they look like 'string'::type. So, it's unclear if it
>> is correct to check expressions in ExecInitExpr().
>>
>> 6. Remove subtransactions.

They were completely removed. Only DEFAULT expression needs to be fixed now.

> Sounds like a good plan, modulo the issues in item 5. I would rather
> lose some features temporarily than try to turn handsprings to make them
> work and jeopardize the rest.
>
> I'll look forward to seeing your patch in the morning :-)
>
v8 - is a highly WIP patch, which I failed to finish today.
Even some test cases fail now, and they simply show unfinished
things like casts to bytea (they can be simply removed) and missing
safe input functions.

--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company

On 2022-08-24 We 20:05, Nikita Glukhov wrote:
>
>
> v8 - is a highly WIP patch, which I failed to finish today.
> Even some test cases fail now, and they simply show unfinished
> things like casts to bytea (they can be simply removed) and missing
> safe input functions.
>

Thanks for your work, please keep going.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 8/24/22 8:16 PM, Andrew Dunstan wrote:
>
> On 2022-08-24 We 20:05, Nikita Glukhov wrote:
>>
>>
>> v8 - is a highly WIP patch, which I failed to finish today.
>> Even some test cases fail now, and they simply show unfinished
>> things like casts to bytea (they can be simply removed) and missing
>> safe input functions.
>>
>
> Thanks for your work, please keep going.

Thanks for the efforts Nikita.

With RMT hat on, I want to point out that it's nearing the end of the
week, and if we are going to go forward with this path, we do need to
review soon. The Beta 4 release date is set to 9/8, and if we are going
to commit or revert, we should leave enough time to ensure that we have
enough time to review and the patches are able to successfully get
through the buildfarm.

Thanks,

Jonathan

On 2022-08-26 Fr 12:36, Jonathan S. Katz wrote:
> On 8/24/22 8:16 PM, Andrew Dunstan wrote:
>>
>> On 2022-08-24 We 20:05, Nikita Glukhov wrote:
>>>
>>>
>>> v8 - is a highly WIP patch, which I failed to finish today.
>>> Even some test cases fail now, and they simply show unfinished
>>> things like casts to bytea (they can be simply removed) and missing
>>> safe input functions.
>>>
>>
>> Thanks for your work, please keep going.
>
> Thanks for the efforts Nikita.
>
> With RMT hat on, I want to point out that it's nearing the end of the
> week, and if we are going to go forward with this path, we do need to
> review soon. The Beta 4 release date is set to 9/8, and if we are
> going to commit or revert, we should leave enough time to ensure that
> we have enough time to review and the patches are able to successfully
> get through the buildfarm.
>
>

Also I'm going to be traveling and more or less offline from Sept 5th,
so if I'm going to be involved we'd need a decision by Sept 1st or 2nd,
I think, so time is running very short. Of course, others could do the
required commit work either way a bit later, but not much.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi,

On 26.08.2022 22:25, Andrew Dunstan wrote:
> On 2022-08-24 We 20:05, Nikita Glukhov wrote:
>> v8 - is a highly WIP patch, which I failed to finish today.
>> Even some test cases fail now, and they simply show unfinished
>> things like casts to bytea (they can be simply removed) and missing
>> safe input functions.
> Thanks for your work, please keep going.

I have completed in v9 all the things I previously planned:

- Added missing safe I/O and type conversion functions for
datetime, float4, varchar, bpchar. This introduces a lot
of boilerplate code for returning errors and also maybe
adds some overhead.

- Added JSON_QUERY coercion to UTF8 bytea using pg_convert_to().

- Added immutability checks that were missed with elimination
of coercion expressions.
Coercions text::datetime, datetime1::datetime2 and even
datetime::text for some datetime types are mutable.
datetime::text can be made immutable by passing ISO date
style into output functions (like in jsonpath).

- Disabled non-Const expressions in DEFAULT ON EMPTY in non
ERROR ON ERROR case. Non-constant expressions are tried to
evaluate into Const directly inside transformExpr().
Maybe it would be better to simply remove DEFAULT ON EMPTY.

It is possible to easily split this patch into several subpatches,
I will do it if needed.

--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company

On 2022-08-26 Fr 16:11, Nikita Glukhov wrote:
>
> Hi,
>
> On 26.08.2022 22:25, Andrew Dunstan wrote:
>> On 2022-08-24 We 20:05, Nikita Glukhov wrote:
>>> v8 - is a highly WIP patch, which I failed to finish today.
>>> Even some test cases fail now, and they simply show unfinished
>>> things like casts to bytea (they can be simply removed) and missing
>>> safe input functions.
>> Thanks for your work, please keep going.
> I have completed in v9 all the things I previously planned:
>
> - Added missing safe I/O and type conversion functions for
> datetime, float4, varchar, bpchar. This introduces a lot
> of boilerplate code for returning errors and also maybe
> adds some overhead.
>
> - Added JSON_QUERY coercion to UTF8 bytea using pg_convert_to().
>
> - Added immutability checks that were missed with elimination
> of coercion expressions.
> Coercions text::datetime, datetime1::datetime2 and even
> datetime::text for some datetime types are mutable.
> datetime::text can be made immutable by passing ISO date
> style into output functions (like in jsonpath).
>
> - Disabled non-Const expressions in DEFAULT ON EMPTY in non
> ERROR ON ERROR case. Non-constant expressions are tried to
> evaluate into Const directly inside transformExpr().
> Maybe it would be better to simply remove DEFAULT ON EMPTY.

Yes, I think that's what I suggested upthread. I don't think DEFAULT ON
EMPTY matters that much, and we can revisit it for release 16. If it's
simpler please do it that way.

>
>
> It is possible to easily split this patch into several subpatches,
> I will do it if needed.

Thanks, probably a good idea but I will start reviewing what you have
now. Andres and others please chime in if you can.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 8/26/22 4:36 PM, Andrew Dunstan wrote:
>
> On 2022-08-26 Fr 16:11, Nikita Glukhov wrote:
>>
>> Hi,
>>
>> On 26.08.2022 22:25, Andrew Dunstan wrote:
>>> On 2022-08-24 We 20:05, Nikita Glukhov wrote:
>>>> v8 - is a highly WIP patch, which I failed to finish today.
>>>> Even some test cases fail now, and they simply show unfinished
>>>> things like casts to bytea (they can be simply removed) and missing
>>>> safe input functions.
>>> Thanks for your work, please keep going.
>> I have completed in v9 all the things I previously planned:
>>
>> - Added missing safe I/O and type conversion functions for
>> datetime, float4, varchar, bpchar. This introduces a lot
>> of boilerplate code for returning errors and also maybe
>> adds some overhead.
>>
>> - Added JSON_QUERY coercion to UTF8 bytea using pg_convert_to().
>>
>> - Added immutability checks that were missed with elimination
>> of coercion expressions.
>> Coercions text::datetime, datetime1::datetime2 and even
>> datetime::text for some datetime types are mutable.
>> datetime::text can be made immutable by passing ISO date
>> style into output functions (like in jsonpath).
>>
>> - Disabled non-Const expressions in DEFAULT ON EMPTY in non
>> ERROR ON ERROR case. Non-constant expressions are tried to
>> evaluate into Const directly inside transformExpr().
>> Maybe it would be better to simply remove DEFAULT ON EMPTY.
>
>
> Yes, I think that's what I suggested upthread. I don't think DEFAULT ON
> EMPTY matters that much, and we can revisit it for release 16. If it's
> simpler please do it that way.
>
>
>> It is possible to easily split this patch into several subpatches,
>> I will do it if needed.
>
>
> Thanks, probably a good idea but I will start reviewing what you have
> now. Andres and others please chime in if you can.

Thanks Nikita!

I looked through the tests to see if we would need any doc changes, e.g.
in [1]. I noticed that this hint:

"HINT: Use ERROR ON ERROR clause or try to simplify expression into
constant-like form"

lacks a period on the end, which is convention.

I don't know if the SQL/JSON standard calls out if domains should be
castable, but if it does, we should document in [1] that we are not
currently supporting them as return types, so that we're only supporting
"constant-like" expressions with examples.

Looking forward to hearing other feedback.

Thanks,

Jonathan

[1] https://www.postgresql.org/docs/15/functions-json.html#FUNCTIONS-SQLJSON

On Sat, Aug 27, 2022 at 5:11 AM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
> On 26.08.2022 22:25, Andrew Dunstan wrote:
>
> On 2022-08-24 We 20:05, Nikita Glukhov wrote:
>
> v8 - is a highly WIP patch, which I failed to finish today.
> Even some test cases fail now, and they simply show unfinished
> things like casts to bytea (they can be simply removed) and missing
> safe input functions.
>
> Thanks for your work, please keep going.
>
> I have completed in v9 all the things I previously planned:
>
> - Added missing safe I/O and type conversion functions for
> datetime, float4, varchar, bpchar. This introduces a lot
> of boilerplate code for returning errors and also maybe
> adds some overhead.

Didn't know that we have done similar things in the past for jsonpath, as in:

commit 16d489b0fe058e527619f5e9d92fd7ca3c6c2994
Author: Alexander Korotkov <akorotkov(at)postgresql(dot)org>
Date: Sat Mar 16 12:21:19 2019 +0300

Numeric error suppression in jsonpath

BTW, maybe the following hunk in boolin_opt_error() is unnecessary?

- len = strlen(str);
+ len -= str - in_str;

> - Added JSON_QUERY coercion to UTF8 bytea using pg_convert_to().
>
> - Added immutability checks that were missed with elimination
> of coercion expressions.
> Coercions text::datetime, datetime1::datetime2 and even
> datetime::text for some datetime types are mutable.
> datetime::text can be made immutable by passing ISO date
> style into output functions (like in jsonpath).
>
> - Disabled non-Const expressions in DEFAULT ON EMPTY in non
> ERROR ON ERROR case. Non-constant expressions are tried to
> evaluate into Const directly inside transformExpr().

I am not sure if it's OK to eval_const_expressions() on a Query
sub-expression during parse-analysis. IIUC, it is only correct to
apply it to after the rewriting phase.

> Maybe it would be better to simply remove DEFAULT ON EMPTY.

So +1 to this for now.

> It is possible to easily split this patch into several subpatches,
> I will do it if needed.

That would be nice indeed.

I'm wondering if you're going to change the PASSING values
initialization to add the steps into the parent JsonExpr's ExprState,
like the previous patch was doing?

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On 8/29/22 8:56 AM, Amit Langote wrote:
> On Sat, Aug 27, 2022 at 5:11 AM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:

> I am not sure if it's OK to eval_const_expressions() on a Query
> sub-expression during parse-analysis. IIUC, it is only correct to
> apply it to after the rewriting phase.
>
>> Maybe it would be better to simply remove DEFAULT ON EMPTY.
>
> So +1 to this for now.

+1, if this simplifies the patch and makes it acceptable for v15

>> It is possible to easily split this patch into several subpatches,
>> I will do it if needed.
>
> That would be nice indeed.

With RMT hat on, the RMT has its weekly meetings on Tuesdays. Based on
the timing of the Beta 4 commit freeze[1] and how both
including/reverting are nontrivial operations (e.g. we should ensure
we're confident in both and that they pass through the buildfarm), we
are going to have to make a decision on how to proceed at the next meeting.

Can folks please chime in on what they think of the current patchset and
if this is acceptable for v15?

Thanks,

Jonathan

[1]
https://www.postgresql.org/message-id/9d251aec-cea2-bc1a-5ed8-46ef0bcf6c69@postgresql.org

On 2022-08-29 Mo 09:35, Jonathan S. Katz wrote:
> On 8/29/22 8:56 AM, Amit Langote wrote:
>> On Sat, Aug 27, 2022 at 5:11 AM Nikita Glukhov
>> <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
>
>> I am not sure if it's OK to eval_const_expressions() on a Query
>> sub-expression during parse-analysis.  IIUC, it is only correct to
>> apply it to after the rewriting phase.
>>
>>>     Maybe it would be better to simply remove DEFAULT ON EMPTY.
>>
>> So +1 to this for now.
>
> +1, if this simplifies the patch and makes it acceptable for v15
>
>>> It is possible to easily split this patch into several subpatches,
>>> I will do it if needed.
>>
>> That would be nice indeed.
>
> With RMT hat on, the RMT has its weekly meetings on Tuesdays. Based on
> the timing of the Beta 4 commit freeze[1] and how both
> including/reverting are nontrivial operations (e.g. we should ensure
> we're confident in both and that they pass through the buildfarm), we
> are going to have to make a decision on how to proceed at the next
> meeting.
>
> Can folks please chime in on what they think of the current patchset
> and if this is acceptable for v15?
>
>

I think at a pinch we could probably go with it, but it's a close call.
I think it deals with the most pressing issues that have been raised. If
people are still worried I think it would be trivial to add in calls
that error out of the DEFAULT clauses are used at all.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Hi,

On 29.08.2022 15:56, Amit Langote wrote:
> On Sat, Aug 27, 2022 at 5:11 AM Nikita Glukhov<n(dot)gluhov(at)postgrespro(dot)ru> wrote:
>> On 26.08.2022 22:25, Andrew Dunstan wrote:
>>
>> On 2022-08-24 We 20:05, Nikita Glukhov wrote:
>>
>> I have completed in v9 all the things I previously planned:
>>
>> - Added missing safe I/O and type conversion functions for
>> datetime, float4, varchar, bpchar. This introduces a lot
>> of boilerplate code for returning errors and also maybe
>> adds some overhead.
> Didn't know that we have done similar things in the past for jsonpath, as in:
>
> commit 16d489b0fe058e527619f5e9d92fd7ca3c6c2994
> Author: Alexander Korotkov<akorotkov(at)postgresql(dot)org>
> Date: Sat Mar 16 12:21:19 2019 +0300
>
> Numeric error suppression in jsonpath

This was necessary for handling errors in arithmetic operations.

> BTW, maybe the following hunk in boolin_opt_error() is unnecessary?
>
> - len = strlen(str);
> + len -= str - in_str;
>
This is really not necessary, but helps to avoid extra strlen() call.
I have replaced it with more intuitive

+ {

str++;
+ len--;
+ }

- len = strlen(str);

>> - Added JSON_QUERY coercion to UTF8 bytea using pg_convert_to().
>>
>> - Added immutability checks that were missed with elimination
>> of coercion expressions.
>> Coercions text::datetime, datetime1::datetime2 and even
>> datetime::text for some datetime types are mutable.
>> datetime::text can be made immutable by passing ISO date
>> style into output functions (like in jsonpath).
>>
>> - Disabled non-Const expressions in DEFAULT ON EMPTY in non
>> ERROR ON ERROR case. Non-constant expressions are tried to
>> evaluate into Const directly inside transformExpr().
> I am not sure if it's OK to eval_const_expressions() on a Query
> sub-expression during parse-analysis. IIUC, it is only correct to
> apply it to after the rewriting phase.

I also was not sure. Maybe it can be moved to rewriting phase or
even to execution phase.

>> Maybe it would be better to simply remove DEFAULT ON EMPTY.
> So +1 to this for now.

See last patch #9.

>> It is possible to easily split this patch into several subpatches,
>> I will do it if needed.
> That would be nice indeed.

I have extracted patches #1-6 with numerous safe input and type conversion
functions.

> I'm wondering if you're going to change the PASSING values
> initialization to add the steps into the parent JsonExpr's ExprState,
> like the previous patch was doing?

I forget to incorporate your changes for subsidary ExprStates elimination.
See patch #8.

--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company

On Tue, Aug 30, 2022 at 6:49 AM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
> On 29.08.2022 15:56, Amit Langote wrote:
> On Sat, Aug 27, 2022 at 5:11 AM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
> I have completed in v9 all the things I previously planned:
>
> BTW, maybe the following hunk in boolin_opt_error() is unnecessary?
>
> - len = strlen(str);
> + len -= str - in_str;
>
> This is really not necessary, but helps to avoid extra strlen() call.
> I have replaced it with more intuitive
>
> + {
>
> str++;
> + len--;
> + }
>
> - len = strlen(str);

+1

> - Added JSON_QUERY coercion to UTF8 bytea using pg_convert_to().
>
> - Added immutability checks that were missed with elimination
> of coercion expressions.
> Coercions text::datetime, datetime1::datetime2 and even
> datetime::text for some datetime types are mutable.
> datetime::text can be made immutable by passing ISO date
> style into output functions (like in jsonpath).
>
> - Disabled non-Const expressions in DEFAULT ON EMPTY in non
> ERROR ON ERROR case. Non-constant expressions are tried to
> evaluate into Const directly inside transformExpr().
>
> I am not sure if it's OK to eval_const_expressions() on a Query
> sub-expression during parse-analysis. IIUC, it is only correct to
> apply it to after the rewriting phase.
>
> I also was not sure. Maybe it can be moved to rewriting phase or
> even to execution phase.

I suppose we wouldn't need to bother with doing this when we
eventually move to supporting the DEFAULT expressions.

> Maybe it would be better to simply remove DEFAULT ON EMPTY.
>
> So +1 to this for now.
>
> See last patch #9.
>
>
> It is possible to easily split this patch into several subpatches,
> I will do it if needed.
>
> That would be nice indeed.
>
> I have extracted patches #1-6 with numerous safe input and type conversion
> functions.
>
>
> I'm wondering if you're going to change the PASSING values
> initialization to add the steps into the parent JsonExpr's ExprState,
> like the previous patch was doing?
>
> I forget to incorporate your changes for subsidary ExprStates elimination.
> See patch #8.

Thanks. Here are some comments.

First of all, regarding 0009, my understanding was that we should
disallow DEFAULT expression ON ERROR too for now, so something like
the following does not occur:

SELECT JSON_VALUE(jsonb '"err"', '$' RETURNING numeric DEFAULT ('{"'
|| -1+a || '"}')::text ON ERROR) from foo;
ERROR: invalid input syntax for type numeric: "{"0"}"

Patches 0001-0006:

Yeah, these add the overhead of an extra function call (typin() ->
typin_opt_error()) in possibly very common paths. Other than
refactoring *all* places that call typin() to use the new API, the
only other option seems to be to leave the typin() functions alone and
duplicate their code in typin_opt_error() versions for all the types
that this patch cares about. Though maybe, that's not necessarily a
better compromise than accepting the extra function call overhead.

Patch 0007:

+
+ /* Override default coercion in OMIT QUOTES case */
+ if (ExecJsonQueryNeedsIOCoercion(jexpr, res, *resnull))
+ {
+ char *str = JsonbUnquote(DatumGetJsonbP(res));
...
+ else if (ret_typid == VARCHAROID || ret_typid == BPCHAROID ||
+ ret_typid == BYTEAOID)
+ {
+ Jsonb *jb = DatumGetJsonbP(res);
+ char *str = JsonbToCString(NULL, &jb->root, VARSIZE(jb));
+
+ return ExecJsonStringCoercion(str, strlen(str),
ret_typid, ret_typmod);
+ }

I think it might be better to create ExecJsonQueryCoercion() similar
to ExecJsonValueCoercion() and put the above block in that function
rather than inlining it in ExecEvalJsonExprInternal().

+ ExecJsonStringCoercion(const char *str, int32 len, Oid typid, int32 typmod)

I'd suggest renaming this one to ExecJsonConvertCStringToText().

+ ExecJsonCoercionToText(PGFunction outfunc, Datum value, Oid typid,
int32 typmod)
+ ExecJsonDatetimeCoercion(Datum val, Oid val_typid, Oid typid, int32 typmod,
+ ExecJsonBoolCoercion(bool val, Oid typid, int32 typmod, Datum *res)

And also rename these to sound like verbs:

ExecJsonCoerceToText
ExecJsonCoerceDatetime[ToType]
ExecJsonCoerceBool[ToType]

+ /*
+ * XXX coercion to text is done using output functions, and they
+ * are mutable for non-time[tz] types due to using of DateStyle.
+ * We can pass USE_ISO_DATES, which is used inside jsonpath, to
+ * make these coercions and JSON_VALUE(RETURNING text) immutable.
+ *
+ * XXX Also timestamp[tz] output functions can throw "out of range"
+ * error, but this error seem to be not possible.
+ */

Are we planning to fix these before committing?

+static Datum
+JsonbPGetTextDatum(Jsonb *jb)

Maybe static inline?

- coercion = &coercions->composite;
- res = JsonbPGetDatum(JsonbValueToJsonb(item));
+ Assert(0); /* non-scalars must be rejected by JsonPathValue() */

I didn't notice any changes to JsonPathValue(). Is the new comment
referring to an existing behavior of JsonPathValue() or something that
must be done by the patch?

@@ -411,6 +411,26 @@ contain_mutable_functions_walker(Node *node, void *context)
{
JsonExpr *jexpr = castNode(JsonExpr, node);
Const *cnst;
+ bool returns_datetime;
+
+ /*
+ * Input fuctions for datetime types are stable. They can be
+ * called in JSON_VALUE(), when the resulting SQL/JSON is a
+ * string.
+ */
...

Sorry if you've mentioned it before, but are these hunks changing
contain_mutable_functions_walker() fixing a bug? That is, did the
original SQL/JSON patch miss doing this?

+ Oid collation; /* OID of collation, or InvalidOid if none */

I think the comment should rather say: /* Collation of <what>, ... */

+
+bool
+expr_can_throw_errors(Node *expr)
+{
+ if (!expr)
+ return false;
+
+ if (IsA(expr, Const))
+ return false;
+
+ /* TODO consider more cases */
+ return true;
+}

+extern bool expr_can_throw_errors(Node *expr);
+

Not used anymore.

Patch 0008:

Thanks for re-introducing this.

+bool
+ExecEvalJsonExprSkip(ExprState *state, ExprEvalStep *op)
+{
+ JsonExprState *jsestate = op->d.jsonexpr_skip.jsestate;
+
+ /*
+ * Skip if either of the input expressions has turned out to be
+ * NULL, though do execute domain checks for NULLs, which are
+ * handled by the coercion step.
+ */

I think the part starting with ", though" is no longer necessary.

+ * Return value:
+ * 1 - Ok, jump to the end of JsonExpr
+ * 0 - empty result, need to execute DEFAULT ON EMPTY expression
+ * -1 - error occured, need to execute DEFAULT ON ERROR expression

...need to execute ON EMPTY/ERROR behavior

+ return 0; /* jump to ON EMPTY expression */
...
+ return -1; /* jump to ON ERROR expression */

Likewise:

/* jump to handle ON EMPTY/ERROR behavior */

+ * Jump to coercion step if true was returned,
+ * which signifies skipping of JSON path evaluation,
...

Jump to "end" if true was returned.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On 2022-Aug-30, Amit Langote wrote:

> Patches 0001-0006:
>
> Yeah, these add the overhead of an extra function call (typin() ->
> typin_opt_error()) in possibly very common paths. Other than
> refactoring *all* places that call typin() to use the new API, the
> only other option seems to be to leave the typin() functions alone and
> duplicate their code in typin_opt_error() versions for all the types
> that this patch cares about. Though maybe, that's not necessarily a
> better compromise than accepting the extra function call overhead.

I think another possibility is to create a static inline function in the
corresponding .c module (say boolin_impl() in bool.c), which is called
by both the opt_error variant as well as the regular one. This would
avoid the duplicate code as well as the added function-call overhead.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Syntax error: function hell() needs an argument.
Please choose what hell you want to involve.

On Tue, Aug 30, 2022 at 6:19 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> On 2022-Aug-30, Amit Langote wrote:
>
> > Patches 0001-0006:
> >
> > Yeah, these add the overhead of an extra function call (typin() ->
> > typin_opt_error()) in possibly very common paths. Other than
> > refactoring *all* places that call typin() to use the new API, the
> > only other option seems to be to leave the typin() functions alone and
> > duplicate their code in typin_opt_error() versions for all the types
> > that this patch cares about. Though maybe, that's not necessarily a
> > better compromise than accepting the extra function call overhead.
>
> I think another possibility is to create a static inline function in the
> corresponding .c module (say boolin_impl() in bool.c), which is called
> by both the opt_error variant as well as the regular one. This would
> avoid the duplicate code as well as the added function-call overhead.

+1

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On 2022-08-30 Tu 06:29, Amit Langote wrote:
> On Tue, Aug 30, 2022 at 6:19 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>> On 2022-Aug-30, Amit Langote wrote:
>>
>>> Patches 0001-0006:
>>>
>>> Yeah, these add the overhead of an extra function call (typin() ->
>>> typin_opt_error()) in possibly very common paths. Other than
>>> refactoring *all* places that call typin() to use the new API, the
>>> only other option seems to be to leave the typin() functions alone and
>>> duplicate their code in typin_opt_error() versions for all the types
>>> that this patch cares about. Though maybe, that's not necessarily a
>>> better compromise than accepting the extra function call overhead.
>> I think another possibility is to create a static inline function in the
>> corresponding .c module (say boolin_impl() in bool.c), which is called
>> by both the opt_error variant as well as the regular one. This would
>> avoid the duplicate code as well as the added function-call overhead.
> +1
>

Makes plenty of sense, I'll try to come up with replacements for these
forthwith.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 8/30/22 9:16 AM, Andrew Dunstan wrote:
>
> On 2022-08-30 Tu 06:29, Amit Langote wrote:
>> On Tue, Aug 30, 2022 at 6:19 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>>> On 2022-Aug-30, Amit Langote wrote:
>>>
>>>> Patches 0001-0006:
>>>>
>>>> Yeah, these add the overhead of an extra function call (typin() ->
>>>> typin_opt_error()) in possibly very common paths. Other than
>>>> refactoring *all* places that call typin() to use the new API, the
>>>> only other option seems to be to leave the typin() functions alone and
>>>> duplicate their code in typin_opt_error() versions for all the types
>>>> that this patch cares about. Though maybe, that's not necessarily a
>>>> better compromise than accepting the extra function call overhead.
>>> I think another possibility is to create a static inline function in the
>>> corresponding .c module (say boolin_impl() in bool.c), which is called
>>> by both the opt_error variant as well as the regular one. This would
>>> avoid the duplicate code as well as the added function-call overhead.
>> +1
>>
>
> Makes plenty of sense, I'll try to come up with replacements for these
> forthwith.

The RMT had its weekly meeting today to discuss open items. As stated
last week, to keep the v15 release within a late Sept / early Oct
timeframe, we need to make a decision about the inclusion of SQL/JSON
this week.

First, we appreciate all of the effort and work that has gone into
incorporating community feedback into the patches. We did note that
folks working on this made a lot of progress over the past week.

The RMT still has a few concerns, summarized as:

1. There is not yet consensus on the current patch proposals as we
approach the end of the major release cycle
2. There is a lack of general feedback from folks who raised concerns
about the implementation

The RMT is still inclined to revert, but will give folks until Sep 1
0:00 AoE[1] to reach consensus on if SQL/JSON can be included in v15.
This matches up to Andrew's availability timeline for a revert, and
gives enough time to get through the buildfarm prior to the Beta 4
release[2].

After the deadline, if there is no consensus on how to proceed, the RMT
will request that the patches are reverted.

While noting that this RMT has no decision making over v16, in the event
of a revert we do hope this recent work can be the basis of the feature
in v16.

Again, we appreciate the efforts that have gone into addressing the
community feedback.

Sincerely,

John, Jonathan, Michael

[1] https://en.wikipedia.org/wiki/Anywhere_on_Earth
[2]
https://www.postgresql.org/message-id/9d251aec-cea2-bc1a-5ed8-46ef0bcf6c69@postgresql.org

On 30.08.2022 11:09, Amit Langote wrote:
>> - Added JSON_QUERY coercion to UTF8 bytea using pg_convert_to().
>>
>> - Added immutability checks that were missed with elimination
>> of coercion expressions.
>> Coercions text::datetime, datetime1::datetime2 and even
>> datetime::text for some datetime types are mutable.
>> datetime::text can be made immutable by passing ISO date
>> style into output functions (like in jsonpath).
>>
>> - Disabled non-Const expressions in DEFAULT ON EMPTY in non
>> ERROR ON ERROR case. Non-constant expressions are tried to
>> evaluate into Const directly inside transformExpr().
>>
>> I am not sure if it's OK to eval_const_expressions() on a Query
>> sub-expression during parse-analysis. IIUC, it is only correct to
>> apply it to after the rewriting phase.
>>
>> I also was not sure. Maybe it can be moved to rewriting phase or
>> even to execution phase.
> I suppose we wouldn't need to bother with doing this when we
> eventually move to supporting the DEFAULT expressions.
>> Maybe it would be better to simply remove DEFAULT ON EMPTY.
>>
>> So +1 to this for now.
>>
>> See last patch #9.
>>
>>
>> It is possible to easily split this patch into several subpatches,
>> I will do it if needed.
>>
>> That would be nice indeed.
>>
>> I have extracted patches #1-6 with numerous safe input and type conversion
>> functions.
>>
>>
>> I'm wondering if you're going to change the PASSING values
>> initialization to add the steps into the parent JsonExpr's ExprState,
>> like the previous patch was doing?
>>
>> I forget to incorporate your changes for subsidary ExprStates elimination.
>> See patch #8.
> Thanks. Here are some comments.
>
> First of all, regarding 0009, my understanding was that we should
> disallow DEFAULT expression ON ERROR too for now, so something like
> the following does not occur:
>
> SELECT JSON_VALUE(jsonb '"err"', '$' RETURNING numeric DEFAULT ('{"'
> || -1+a || '"}')::text ON ERROR) from foo;
> ERROR: invalid input syntax for type numeric: "{"0"}"

Personally, I don't like complete removal of DEFAULT behaviors, but
I've done it in patch #10 (JsonBehavior node removed, grammar fixed).

> Patches 0001-0006:

On 30.08.2022 13:29, Amit Langote wrote:
> On Tue, Aug 30, 2022 at 6:19 PM Alvaro Herrera<alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>> On 2022-Aug-30, Amit Langote wrote:
>>
>>> Patches 0001-0006:
>>>
>>> Yeah, these add the overhead of an extra function call (typin() ->
>>> typin_opt_error()) in possibly very common paths. Other than
>>> refactoring *all* places that call typin() to use the new API, the
>>> only other option seems to be to leave the typin() functions alone and
>>> duplicate their code in typin_opt_error() versions for all the types
>>> that this patch cares about. Though maybe, that's not necessarily a
>>> better compromise than accepting the extra function call overhead.
>> I think another possibility is to create a static inline function in the
>> corresponding .c module (say boolin_impl() in bool.c), which is called
>> by both the opt_error variant as well as the regular one. This would
>> avoid the duplicate code as well as the added function-call overhead.
> +1

I always thought about such internal inline functions, I 've added them in v10.

> Patch 0007:
>
> +
> + /* Override default coercion in OMIT QUOTES case */
> + if (ExecJsonQueryNeedsIOCoercion(jexpr, res, *resnull))
> + {
> + char *str = JsonbUnquote(DatumGetJsonbP(res));
> ...
> + else if (ret_typid == VARCHAROID || ret_typid == BPCHAROID ||
> + ret_typid == BYTEAOID)
> + {
> + Jsonb *jb = DatumGetJsonbP(res);
> + char *str = JsonbToCString(NULL, &jb->root, VARSIZE(jb));
> +
> + return ExecJsonStringCoercion(str, strlen(str),
> ret_typid, ret_typmod);
> + }
>
> I think it might be better to create ExecJsonQueryCoercion() similar
> to ExecJsonValueCoercion() and put the above block in that function
> rather than inlining it in ExecEvalJsonExprInternal().

Extracted ExecJsonQueryCoercion().

> + ExecJsonStringCoercion(const char *str, int32 len, Oid typid, int32 typmod)
>
> I'd suggest renaming this one to ExecJsonConvertCStringToText().
>
> + ExecJsonCoercionToText(PGFunction outfunc, Datum value, Oid typid,
> int32 typmod)
> + ExecJsonDatetimeCoercion(Datum val, Oid val_typid, Oid typid, int32 typmod,
> + ExecJsonBoolCoercion(bool val, Oid typid, int32 typmod, Datum *res)
>
> And also rename these to sound like verbs:
>
> ExecJsonCoerceToText
> ExecJsonCoerceDatetime[ToType]
> ExecJsonCoerceBool[ToType]

Fixed.

> + /*
> + * XXX coercion to text is done using output functions, and they
> + * are mutable for non-time[tz] types due to using of DateStyle.
> + * We can pass USE_ISO_DATES, which is used inside jsonpath, to
> + * make these coercions and JSON_VALUE(RETURNING text) immutable.
> + *
> + * XXX Also timestamp[tz] output functions can throw "out of range"
> + * error, but this error seem to be not possible.
> + */
>
> Are we planning to fix these before committing?

I don't know, but the first issue is critical for building functional indexes
on JSON_VALUE().

> +static Datum
> +JsonbPGetTextDatum(Jsonb *jb)
>
> Maybe static inline?

Fixed.

> - coercion = &coercions->composite;
> - res = JsonbPGetDatum(JsonbValueToJsonb(item));
> + Assert(0); /* non-scalars must be rejected by JsonPathValue() */
>
> I didn't notice any changes to JsonPathValue(). Is the new comment
> referring to an existing behavior of JsonPathValue() or something that
> must be done by the patch?

JsonPathValue() has a check for non-scalars items, this is simply a new comment.

> @@ -411,6 +411,26 @@ contain_mutable_functions_walker(Node *node, void *context)
> {
> JsonExpr *jexpr = castNode(JsonExpr, node);
> Const *cnst;
> + bool returns_datetime;
> +
> + /*
> + * Input fuctions for datetime types are stable. They can be
> + * called in JSON_VALUE(), when the resulting SQL/JSON is a
> + * string.
> + */
> ...
>
>
> Sorry if you've mentioned it before, but are these hunks changing
> contain_mutable_functions_walker() fixing a bug? That is, did the
> original SQL/JSON patch miss doing this?

In the original patch there were checks for mutability of expressions contained
in JsonCoercion nodes. After their removal, we need to use hardcoded checks.

> + Oid collation; /* OID of collation, or InvalidOid if none */
>
> I think the comment should rather say: /* Collation of <what>, ... */

Fixed.

> +
> +bool
> +expr_can_throw_errors(Node *expr)
> +{
> + if (!expr)
> + return false;
> +
> + if (IsA(expr, Const))
> + return false;
> +
> + /* TODO consider more cases */
> + return true;
> +}
>
> +extern bool expr_can_throw_errors(Node *expr);
> +
>
> Not used anymore.

expr_can_throw_errors() removed.

> Patch 0008:
>
> Thanks for re-introducing this.
>
> +bool
> +ExecEvalJsonExprSkip(ExprState *state, ExprEvalStep *op)
> +{
> + JsonExprState *jsestate = op->d.jsonexpr_skip.jsestate;
> +
> + /*
> + * Skip if either of the input expressions has turned out to be
> + * NULL, though do execute domain checks for NULLs, which are
> + * handled by the coercion step.
> + */
>
> I think the part starting with ", though" is no longer necessary.

Fixed.

> + * Return value:
> + * 1 - Ok, jump to the end of JsonExpr
> + * 0 - empty result, need to execute DEFAULT ON EMPTY expression
> + * -1 - error occured, need to execute DEFAULT ON ERROR expression
>
> ...need to execute ON EMPTY/ERROR behavior
>
> + return 0; /* jump to ON EMPTY expression */
> ...
> + return -1; /* jump to ON ERROR expression */
>
> Likewise:
>
> /* jump to handle ON EMPTY/ERROR behavior */
>
> + * Jump to coercion step if true was returned,
> + * which signifies skipping of JSON path evaluation,
> ...
>
> Jump to "end" if true was returned.

Fixed, but I leaved "expression" instead of "behavior" because
these jumps are needed only for execution of DEFAULT expressions.

--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company

On Wed, Aug 31, 2022 at 6:25 AM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
> On 30.08.2022 11:09, Amit Langote wrote:
> First of all, regarding 0009, my understanding was that we should
> disallow DEFAULT expression ON ERROR too for now, so something like
> the following does not occur:
>
> SELECT JSON_VALUE(jsonb '"err"', '$' RETURNING numeric DEFAULT ('{"'
> || -1+a || '"}')::text ON ERROR) from foo;
> ERROR: invalid input syntax for type numeric: "{"0"}"
>
> Personally, I don't like complete removal of DEFAULT behaviors, but
> I've done it in patch #10 (JsonBehavior node removed, grammar fixed).

To clarify, I had meant to ask if the standard specifies how to handle
the errors of evaluating the DEFAULT ON ERROR expressions themselves?
My understanding is that the sub-transaction that is being removed
would have caught and suppressed the above error too, so along with
removing the sub-transactions, we should also remove anything that
might cause such errors.

> On 30.08.2022 13:29, Amit Langote wrote:
> On Tue, Aug 30, 2022 at 6:19 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> On 2022-Aug-30, Amit Langote wrote:
>
> Patches 0001-0006:
>
> Yeah, these add the overhead of an extra function call (typin() ->
> typin_opt_error()) in possibly very common paths. Other than
> refactoring *all* places that call typin() to use the new API, the
> only other option seems to be to leave the typin() functions alone and
> duplicate their code in typin_opt_error() versions for all the types
> that this patch cares about. Though maybe, that's not necessarily a
> better compromise than accepting the extra function call overhead.
>
> I think another possibility is to create a static inline function in the
> corresponding .c module (say boolin_impl() in bool.c), which is called
> by both the opt_error variant as well as the regular one. This would
> avoid the duplicate code as well as the added function-call overhead.
>
> +1
>
> I always thought about such internal inline functions, I 've added them in v10.

Thanks.

In 0003:

-Datum
-float4in(PG_FUNCTION_ARGS)
+static float
+float4in_internal(char *num, bool *have_error)

Looks like you forgot the inline marker?

In 0006:

-static inline Datum jsonb_from_cstring(char *json, int len, bool unique_keys);
...
+extern Datum jsonb_from_cstring(char *json, int len, bool unique_keys,
+ bool *error);

Did you intentionally remove the inline marker from
jsonb_from_cstring() as opposed to the other cases?

> Patch 0007:
>
> +
> + /* Override default coercion in OMIT QUOTES case */
> + if (ExecJsonQueryNeedsIOCoercion(jexpr, res, *resnull))
> + {
> + char *str = JsonbUnquote(DatumGetJsonbP(res));
> ...
> + else if (ret_typid == VARCHAROID || ret_typid == BPCHAROID ||
> + ret_typid == BYTEAOID)
> + {
> + Jsonb *jb = DatumGetJsonbP(res);
> + char *str = JsonbToCString(NULL, &jb->root, VARSIZE(jb));
> +
> + return ExecJsonStringCoercion(str, strlen(str),
> ret_typid, ret_typmod);
> + }
>
> I think it might be better to create ExecJsonQueryCoercion() similar
> to ExecJsonValueCoercion() and put the above block in that function
> rather than inlining it in ExecEvalJsonExprInternal().
>
> Extracted ExecJsonQueryCoercion().

Thanks.

+/* Coerce JSONB datum to the output typid(typmod) */
static Datum
+ExecJsonQueryCoercion(JsonExpr *jexpr, Oid typid, int32 typmod,
+ Datum jb, bool *error)

Might make sense to expand to comment to mention JSON_QUERY, say as:

/* Coerce JSONB datum returned by JSON_QUERY() to the output typid(typmod) */

+/* Coerce SQL/JSON item to the output typid */
+static Datum
+ExecJsonValueCoercion(JsonbValue *item, Oid typid, int32 typmod,
+ bool *isnull, bool *error)

While at it, also update the comment of ExecJsonValueCoercion() as:

/* Coerce SQL/JSON item returned by JSON_VALUE() to the output typid */

> + /*
> + * XXX coercion to text is done using output functions, and they
> + * are mutable for non-time[tz] types due to using of DateStyle.
> + * We can pass USE_ISO_DATES, which is used inside jsonpath, to
> + * make these coercions and JSON_VALUE(RETURNING text) immutable.
> + *
> + * XXX Also timestamp[tz] output functions can throw "out of range"
> + * error, but this error seem to be not possible.
> + */
>
> Are we planning to fix these before committing?
>
> I don't know, but the first issue is critical for building functional indexes
> on JSON_VALUE().

Ok.

> - coercion = &coercions->composite;
> - res = JsonbPGetDatum(JsonbValueToJsonb(item));
> + Assert(0); /* non-scalars must be rejected by JsonPathValue() */
>
> I didn't notice any changes to JsonPathValue(). Is the new comment
> referring to an existing behavior of JsonPathValue() or something that
> must be done by the patch?
>
> JsonPathValue() has a check for non-scalars items, this is simply a new comment.

Ok.

> @@ -411,6 +411,26 @@ contain_mutable_functions_walker(Node *node, void *context)
> {
> JsonExpr *jexpr = castNode(JsonExpr, node);
> Const *cnst;
> + bool returns_datetime;
> +
> + /*
> + * Input fuctions for datetime types are stable. They can be
> + * called in JSON_VALUE(), when the resulting SQL/JSON is a
> + * string.
> + */
> ...
>
>
> Sorry if you've mentioned it before, but are these hunks changing
> contain_mutable_functions_walker() fixing a bug? That is, did the
> original SQL/JSON patch miss doing this?
>
> In the original patch there were checks for mutability of expressions contained
> in JsonCoercion nodes. After their removal, we need to use hardcoded checks.

Ah, okay, makes sense. Though I do wonder why list the individual
type OIDs here rather than checking the mutability markings on their
input/output functions? For example, we could do what the following
blob in check_funcs_in_node() that is called by
contain_mutable_functions_walker() does:

case T_CoerceViaIO:
{
CoerceViaIO *expr = (CoerceViaIO *) node;
Oid iofunc;
Oid typioparam;
bool typisvarlena;

/* check the result type's input function */
getTypeInputInfo(expr->resulttype,
&iofunc, &typioparam);
if (checker(iofunc, context))
return true;
/* check the input type's output function */
getTypeOutputInfo(exprType((Node *) expr->arg),
&iofunc, &typisvarlena);
if (checker(iofunc, context))
return true;
}

I guess that's what would get used when the JsonCoercion nodes were present.

On 0010:

@@ -5402,7 +5401,7 @@ ExecEvalJsonExprSkip(ExprState *state, ExprEvalStep *op)
* true - Ok, jump to the end of JsonExpr
* false - error occured, need to execute DEFAULT ON ERROR expression
*/
-bool
+void

Looks like you forgot to update the comment.

SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING int DEFAULT 111 ON ERROR);
- json_value
-------------
- 111
-(1 row)
-
+ERROR: syntax error at or near "DEFAULT"
+LINE 1: ...ELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING int DEFAULT 11...

Is it intentional that you left many instances of the regression test
output changes like the above?

Finally, I get this warning:

execExprInterp.c: In function ‘ExecJsonCoerceCStringToText’:
execExprInterp.c:4765:3: warning: missing braces around initializer
[-Wmissing-braces]
NameData encoding = {0};
^
execExprInterp.c:4765:3: warning: (near initialization for
‘encoding.data’) [-Wmissing-braces]

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On Wed, Aug 31, 2022 at 3:51 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Wed, Aug 31, 2022 at 6:25 AM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)ru> wrote:
> > v10 patches
>
> Finally, I get this warning:
>
> execExprInterp.c: In function ‘ExecJsonCoerceCStringToText’:
> execExprInterp.c:4765:3: warning: missing braces around initializer
> [-Wmissing-braces]
> NameData encoding = {0};
> ^
> execExprInterp.c:4765:3: warning: (near initialization for
> ‘encoding.data’) [-Wmissing-braces]

Given the time constraints on making a decision on this, I'd like to
also mention that other than the things mentioned in my last email,
which don't sound like a big deal for Nikita to take care of, I don't
have any further comments on the patches.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On Wed, Aug 31, 2022 at 3:51 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING int DEFAULT 111 ON ERROR);
> - json_value
> -------------
> - 111
> -(1 row)
> -
> +ERROR: syntax error at or near "DEFAULT"
> +LINE 1: ...ELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING int DEFAULT 11...
>
> Is it intentional that you left many instances of the regression test
> output changes like the above?

Actually, thinking more about this, I am wondering if we should not
remove the DEFAULT expression productions in gram.y. Maybe we can
keep the syntax and give an unsupported error during parse-analysis,
like the last version of the patch did for DEFAULT ON EMPTY. Which
also means to also leave JsonBehavior alone but with default_expr
always NULL for now.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On 2022-08-31 We 07:01, Amit Langote wrote:
> On Wed, Aug 31, 2022 at 3:51 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>> SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING int DEFAULT 111 ON ERROR);
>> - json_value
>> -------------
>> - 111
>> -(1 row)
>> -
>> +ERROR: syntax error at or near "DEFAULT"
>> +LINE 1: ...ELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING int DEFAULT 11...
>>
>> Is it intentional that you left many instances of the regression test
>> output changes like the above?
> Actually, thinking more about this, I am wondering if we should not
> remove the DEFAULT expression productions in gram.y. Maybe we can
> keep the syntax and give an unsupported error during parse-analysis,
> like the last version of the patch did for DEFAULT ON EMPTY. Which
> also means to also leave JsonBehavior alone but with default_expr
> always NULL for now.
>

Producing an error in the parse analysis phase seems best to me.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 8/31/22 8:38 AM, Andrew Dunstan wrote:
>
> On 2022-08-31 We 07:01, Amit Langote wrote:
>> On Wed, Aug 31, 2022 at 3:51 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>>> SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING int DEFAULT 111 ON ERROR);
>>> - json_value
>>> -------------
>>> - 111
>>> -(1 row)
>>> -
>>> +ERROR: syntax error at or near "DEFAULT"
>>> +LINE 1: ...ELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING int DEFAULT 11...
>>>
>>> Is it intentional that you left many instances of the regression test
>>> output changes like the above?
>> Actually, thinking more about this, I am wondering if we should not
>> remove the DEFAULT expression productions in gram.y. Maybe we can
>> keep the syntax and give an unsupported error during parse-analysis,
>> like the last version of the patch did for DEFAULT ON EMPTY. Which
>> also means to also leave JsonBehavior alone but with default_expr
>> always NULL for now.
>>
>
> Producing an error in the parse analysis phase seems best to me.

Andres, Robert, Tom: With this recent work, have any of your opinions
changed on including SQL/JSON in v15?

Thanks,

Jonathan

Hi,

On 2022-08-31 10:20:24 -0400, Jonathan S. Katz wrote:
> Andres, Robert, Tom: With this recent work, have any of your opinions
> changed on including SQL/JSON in v15?

I don't really know what to do here. It feels blatantly obvious that this code
isn't even remotely close to being releasable. I'm worried about the impact of
the big revert at this stage of the release cycle, and that's not getting
better by delaying further. And I'm getting weary of being asked to make the
obvious call that the authors of this feature as well as the RMT should have
made a while ago.

From my POV the only real discussion is whether we'd want to revert this in 15
and HEAD or just 15. There's imo a decent point to be made to just revert in
15 and aggressively press forward with the changes posted in this thread.

Greetings,

Andres Freund

On 2022-08-30 Tu 17:25, Nikita Glukhov wrote:
>
>
>
>>>> Patches 0001-0006:
>>>>
>>>> Yeah, these add the overhead of an extra function call (typin() ->
>>>> typin_opt_error()) in possibly very common paths. Other than
>>>> refactoring *all* places that call typin() to use the new API, the
>>>> only other option seems to be to leave the typin() functions alone and
>>>> duplicate their code in typin_opt_error() versions for all the types
>>>> that this patch cares about. Though maybe, that's not necessarily a
>>>> better compromise than accepting the extra function call overhead.
>>> I think another possibility is to create a static inline function in the
>>> corresponding .c module (say boolin_impl() in bool.c), which is called
>>> by both the opt_error variant as well as the regular one. This would
>>> avoid the duplicate code as well as the added function-call overhead.
>> +1
> I always thought about such internal inline functions, I 've added them in v10.
>
>

A couple of questions about these:

1. Patch 5 changes the API of DecodeDateTime() and DecodeTimeOnly() by
adding an extra parameter bool *error. Would it be better to provide
_opt_error flavors of these?

2. Patch 6 changes jsonb_from_cstring so that it's no longer static
inline. Shouldn't we have a static inline function that can be called
from inside jsonb.c and is called by the extern function?

changing both of these things would be quite trivial and should not hold
anything up.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2022-08-31 10:20:24 -0400, Jonathan S. Katz wrote:
>> Andres, Robert, Tom: With this recent work, have any of your opinions
>> changed on including SQL/JSON in v15?

> I don't really know what to do here. It feels blatantly obvious that this code
> isn't even remotely close to being releasable. I'm worried about the impact of
> the big revert at this stage of the release cycle, and that's not getting
> better by delaying further. And I'm getting weary of being asked to make the
> obvious call that the authors of this feature as well as the RMT should have
> made a while ago.

I have to agree. There is a large amount of code at stake here.
We're being asked to review a bunch of hastily-produced patches
to that code on an even more hasty schedule (and personally
I have other things I need to do today...) I think the odds
of a favorable end result are small.

> From my POV the only real discussion is whether we'd want to revert this in 15
> and HEAD or just 15. There's imo a decent point to be made to just revert in
> 15 and aggressively press forward with the changes posted in this thread.

I'm not for that. Code that we don't think is ready to ship
has no business being in the common tree, nor does it make
review any easier to be looking at one bulky set of
already-committed patches and another bulky set of deltas.

I'm okay with making an exception for the include/nodes/ and
backend/nodes/ files in HEAD, since the recent changes in that
area mean it'd be a lot of error-prone work to produce a reverting
patch there. We can leave those in as dead code temporarily, I think.

regards, tom lane

On Wed, Aug 31, 2022 at 10:20 AM Jonathan S. Katz <jkatz(at)postgresql(dot)org> wrote:
> Andres, Robert, Tom: With this recent work, have any of your opinions
> changed on including SQL/JSON in v15?

No. Nothing's been committed, and there's no time to review anything
in detail, and there was never going to be. Nikita said he was ready
to start hacking in mid-August. That's good of him, but feature freeze
was in April. We don't start hacking on a feature 4 months after the
freeze. I'm unwilling to drop everything I'm working on to review
patches that were written in a last minute rush. Even if these patches
were more important to me than my own work, which they are not, I
couldn't possibly do a good job reviewing complex patches on top of
other complex patches in an area that I haven't studied in years. And
if I could do a good job, no doubt I'd find a bunch of problems -
whether they would be large or small, I don't know - and then that
would lead to more changes even closer to the intended release date.

I just don't understand what the RMT thinks it is doing here. When a
concern is raised about whether a feature is anywhere close to being
in a releasable state in August, "hack on it some more and then see
where we're at" seems like an obviously impractical way forward. It
seemed clear to me from the moment that Andres raised his concerns
that the only two viable strategies were (1) revert the feature and be
sad or (2) decide to ship it anyway and hope that Andres is incorrect
in thinking that it will become an embarrassment to the project. The
RMT has chosen neither of these, and in fact, really seems to want
someone else to make the decision. But that's not how it works. The
RMT concept was invented precisely to solve problems like this one,
where the patch authors don't really want to revert it but other
people think it's pretty busted. If such problems were best addressed
by waiting for a long time to see whether anything changes, we
wouldn't need an RMT. That's exactly how we used to handle these kinds
of problems, and it sucked.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 2022-08-31 12:26:29 -0400, Robert Haas wrote:
> On Wed, Aug 31, 2022 at 10:20 AM Jonathan S. Katz <jkatz(at)postgresql(dot)org> wrote:
> > Andres, Robert, Tom: With this recent work, have any of your opinions
> > changed on including SQL/JSON in v15?
>
> No. Nothing's been committed, and there's no time to review anything
> in detail, and there was never going to be. Nikita said he was ready
> to start hacking in mid-August. That's good of him, but feature freeze
> was in April.

As additional context: I had started raising those concerns mid June.

Greetings,

Andres Freund

On 8/31/22 12:26 PM, Robert Haas wrote:
> On Wed, Aug 31, 2022 at 10:20 AM Jonathan S. Katz <jkatz(at)postgresql(dot)org> wrote:
>> Andres, Robert, Tom: With this recent work, have any of your opinions
>> changed on including SQL/JSON in v15?
>
> No. Nothing's been committed, and there's no time to review anything
> in detail, and there was never going to be.

OK. Based on this feedback, the RMT is going to request that this is
reverted.

With RMT hat on -- Andrew can you please revert the patchset?

> Nikita said he was ready
> to start hacking in mid-August. That's good of him, but feature freeze
> was in April. We don't start hacking on a feature 4 months after the
> freeze. I'm unwilling to drop everything I'm working on to review
> patches that were written in a last minute rush. Even if these patches
> were more important to me than my own work, which they are not, I
> couldn't possibly do a good job reviewing complex patches on top of
> other complex patches in an area that I haven't studied in years. And
> if I could do a good job, no doubt I'd find a bunch of problems -
> whether they would be large or small, I don't know - and then that
> would lead to more changes even closer to the intended release date.
>
> I just don't understand what the RMT thinks it is doing here. When a
> concern is raised about whether a feature is anywhere close to being
> in a releasable state in August, "hack on it some more and then see
> where we're at" seems like an obviously impractical way forward. It
> seemed clear to me from the moment that Andres raised his concerns
> that the only two viable strategies were (1) revert the feature and be
> sad or (2) decide to ship it anyway and hope that Andres is incorrect
> in thinking that it will become an embarrassment to the project. The
> RMT has chosen neither of these, and in fact, really seems to want
> someone else to make the decision. But that's not how it works. The
> RMT concept was invented precisely to solve problems like this one,
> where the patch authors don't really want to revert it but other
> people think it's pretty busted. If such problems were best addressed
> by waiting for a long time to see whether anything changes, we
> wouldn't need an RMT. That's exactly how we used to handle these kinds
> of problems, and it sucked.

This is fair feedback. However, there are a few things to consider here:

1. When Andres raised his initial concerns, the RMT did recommend to
revert but did not force it. Part of the RMT charter is to try to get
consensus before doing so and after we've exhausted the community
process. As we moved closer, the patch others proposed some suggestions
which other folks were amenable to trying.

Unfortunately, time has run out. However,

2. One of the other main goals of the RMT is to ensure the release ships
"on time" which we define to be late Q3/early Q4. We factored that into
the decision making process around this. We are still on time for the
release.

I take responsibility for the decision making. I would be open to
discuss this further around what worked / what didn't with the RMT and
where we can improve in the future.

Thanks,

Jonathan

I wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
>> From my POV the only real discussion is whether we'd want to revert this in 15
>> and HEAD or just 15. There's imo a decent point to be made to just revert in
>> 15 and aggressively press forward with the changes posted in this thread.

> I'm not for that. Code that we don't think is ready to ship
> has no business being in the common tree, nor does it make
> review any easier to be looking at one bulky set of
> already-committed patches and another bulky set of deltas.

To enlarge on that a bit: it seems to me that the really fundamental
issue here is how to catch datatype-specific input and conversion
errors without using subtransactions, because those are too expensive
and can mask errors we'd rather not be masking, such as OOM. (Andres
had some additional, more localized concerns, but I think this is the
one with big-picture implications.)

The currently proposed patchset hacks up a relatively small number
of core datatypes to be able to do that. But it's just a hack
and there's no prospect of extension types being able to join
in the fun. I think where we need to start, for v16, is making
an API design that will let any datatype have this functionality.
(I don't say that we'd convert every datatype to do so right away;
in the long run we should, but I'm content to start with just the
same core types touched here.) Beside the JSON stuff, there is
another even more pressing application for such behavior, namely
the often-requested COPY functionality to be able to shunt bad data
off somewhere without losing the entire transfer. In the COPY case
I think we'd want to be able to capture the error message that
would have been issued, which means the current patches are not
at all appropriate as a basis for that API design: they're just
returning a bool without any details.

So that's why I'm in favor of reverting and starting over.
There are probably big chunks of what's been done that can be
re-used, but it all needs to be re-examined with this sort of
design in mind.

As a really quick sketch of what such an API might look like:
we could invent a new node type, say IOCallContext, which is
intended to be passed as FunctionCallInfo.context to type
input functions and perhaps type conversion functions.
Call sites wishing to have no-thrown-error functionality would
initialize one of these to show "no error" and then pass it
to the data type's usual input function. Old-style input
functions would ignore this and just throw errors as usual;
sorry, you don't get the no-error functionality you wanted.
But I/O functions that had been updated would know to store the
report of a relevant error into that node and then return NULL.
(Although I think there may be assumptions somewhere that
I/O functions don't return NULL, so maybe "just return any
dummy value" is a better idea? Although likely it wouldn't
be hard to remove such assumptions from callers using this
functionality.) The caller would detect the presence of an error
by examining the node contents and then do whatever it needs to do.

regards, tom lane

On Wed, Aug 31, 2022 at 1:06 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> The currently proposed patchset hacks up a relatively small number
> of core datatypes to be able to do that. But it's just a hack
> and there's no prospect of extension types being able to join
> in the fun. I think where we need to start, for v16, is making
> an API design that will let any datatype have this functionality.

This would be really nice to have.

> (I don't say that we'd convert every datatype to do so right away;
> in the long run we should, but I'm content to start with just the
> same core types touched here.)

I would be in favor of making more of an effort than just a few token
data types. The initial patch could just touch a few, but once the
infrastructure is in place we should really make a sweep through the
tree and tidy up.

> Beside the JSON stuff, there is
> another even more pressing application for such behavior, namely
> the often-requested COPY functionality to be able to shunt bad data
> off somewhere without losing the entire transfer. In the COPY case
> I think we'd want to be able to capture the error message that
> would have been issued, which means the current patches are not
> at all appropriate as a basis for that API design: they're just
> returning a bool without any details.

Fully agreed.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Aug 31, 2022 at 1:06 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> (I don't say that we'd convert every datatype to do so right away;
>> in the long run we should, but I'm content to start with just the
>> same core types touched here.)

> I would be in favor of making more of an effort than just a few token
> data types. The initial patch could just touch a few, but once the
> infrastructure is in place we should really make a sweep through the
> tree and tidy up.

Sure, but my point is that we can do that in a time-extended fashion
rather than having a flag day where everything must be updated.
The initial patch just needs to update a few types as proof of concept.

regards, tom lane

On 2022-08-31 We 12:48, Jonathan S. Katz wrote:
>
>
> With RMT hat on -- Andrew can you please revert the patchset?

:-(

Yes, I'll do it, starting with the v15 branch. Might take a day or so.

cheers (kinda)

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Wed, Aug 31, 2022 at 12:26:29PM -0400, Robert Haas wrote:
> someone else to make the decision. But that's not how it works. The
> RMT concept was invented precisely to solve problems like this one,
> where the patch authors don't really want to revert it but other
> people think it's pretty busted. If such problems were best addressed
> by waiting for a long time to see whether anything changes, we
> wouldn't need an RMT. That's exactly how we used to handle these kinds
> of problems, and it sucked.

I saw the RMT/Jonathan stated August 28 as the cut-off date for a
decision, which was later changed to September 1:

> The RMT is still inclined to revert, but will give folks until Sep 1 0:00
> AoE[1] to reach consensus on if SQL/JSON can be included in v15. This matches
> up to Andrew's availability timeline for a revert, and gives enough time to
> get through the buildfarm prior to the Beta 4 release[2].

I guess you are saying that setting a cut-off was a bad idea, or that
the cut-off was too close to the final release date. For me, I think
there were three questions:

1. Were subtransactions acceptable, consensus no
2. Could trapping errors work for PG 15, consensus no
3. Could the feature be trimmed back for PG 15 to avoid these, consensus ?

I don't think our community works well when there are three issues in
play at once.

--
Bruce Momjian <bruce(at)momjian(dot)us> https://momjian.us
EDB https://enterprisedb.com

Indecision is a decision. Inaction is an action. Mark Batterson

On Wed, Aug 31, 2022 at 12:04:44PM -0400, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)de> writes:
> > From my POV the only real discussion is whether we'd want to revert this in 15
> > and HEAD or just 15. There's imo a decent point to be made to just revert in
> > 15 and aggressively press forward with the changes posted in this thread.
>
> I'm not for that. Code that we don't think is ready to ship
> has no business being in the common tree, nor does it make
> review any easier to be looking at one bulky set of
> already-committed patches and another bulky set of deltas.

Agreed on removing from PG 15 and master --- it would be confusing to
have lots of incomplete code in master that is not in PG 15.

> I'm okay with making an exception for the include/nodes/ and
> backend/nodes/ files in HEAD, since the recent changes in that
> area mean it'd be a lot of error-prone work to produce a reverting
> patch there. We can leave those in as dead code temporarily, I think.

I don't have an opinion on this.

--
Bruce Momjian <bruce(at)momjian(dot)us> https://momjian.us
EDB https://enterprisedb.com

Indecision is a decision. Inaction is an action. Mark Batterson

Bruce Momjian <bruce(at)momjian(dot)us> writes:
> I guess you are saying that setting a cut-off was a bad idea, or that
> the cut-off was too close to the final release date. For me, I think
> there were three questions:

> 1. Were subtransactions acceptable, consensus no
> 2. Could trapping errors work for PG 15, consensus no
> 3. Could the feature be trimmed back for PG 15 to avoid these, consensus ?

We could probably have accomplished #3 if there was more time,
but we're out of time. (I'm not entirely convinced that spending
effort towards #3 was productive anyway, given that we're now thinking
about a much differently-scoped patch with API changes.)

> I don't think our community works well when there are three issues in
> play at once.

To the extent that there was a management failure here, it was that
we didn't press for a resolution sooner. Given the scale of the
concerns raised in June, I kind of agree with Andres' opinion that
fixing them post-freeze was doomed to failure. It was definitely
doomed once we reached August with no real work done towards it.

regards, tom lane

On 2022-08-31 We 14:45, Tom Lane wrote:
> To the extent that there was a management failure here, it was that
> we didn't press for a resolution sooner. Given the scale of the
> concerns raised in June, I kind of agree with Andres' opinion that
> fixing them post-freeze was doomed to failure. It was definitely
> doomed once we reached August with no real work done towards it.

I'm not going to comment publicly in general about this, you might
imagine what my reaction is. The decision is the RMT's to make and I
have no quarrel with that.

But I do want it understood that there was work being done right from
the time in June when Andres' complaints were published. These were
difficult issues, and we didn't let the grass grow looking for a fix. I
concede that might not have been visible until later.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 8/31/22 3:08 PM, Andrew Dunstan wrote:
>
> On 2022-08-31 We 14:45, Tom Lane wrote:
>> To the extent that there was a management failure here, it was that
>> we didn't press for a resolution sooner. Given the scale of the
>> concerns raised in June, I kind of agree with Andres' opinion that
>> fixing them post-freeze was doomed to failure. It was definitely
>> doomed once we reached August with no real work done towards it.
>
>
> I'm not going to comment publicly in general about this, you might
> imagine what my reaction is. The decision is the RMT's to make and I
> have no quarrel with that.
>
> But I do want it understood that there was work being done right from
> the time in June when Andres' complaints were published. These were
> difficult issues, and we didn't let the grass grow looking for a fix. I
> concede that might not have been visible until later.

June was a bit of a rough month too -- we had the issues that spawned
the out-of-cycle release at the top of the month, which started almost
right after Beta 1, and then almost immediately into Beta 2 after 14.4.
I know that consumed a lot of my cycles. At that point in time for the
v15 release process I was primarily focused on monitoring open items at
that point, so I missed the June comments.

Jonathan

On 31.08.2022 20:14, Tom Lane wrote:
> Robert Haas<robertmhaas(at)gmail(dot)com> writes:
>> On Wed, Aug 31, 2022 at 1:06 PM Tom Lane<tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> The currently proposed patchset hacks up a relatively small number
>>> of core datatypes to be able to do that. But it's just a hack
>>> and there's no prospect of extension types being able to join
>>> in the fun. I think where we need to start, for v16, is making
>>> an API design that will let any datatype have this functionality.
>>>
>>> (I don't say that we'd convert every datatype to do so right away;
>>> in the long run we should, but I'm content to start with just the
>>> same core types touched here.) Beside the JSON stuff, there is
>>> another even more pressing application for such behavior, namely
>>> the often-requested COPY functionality to be able to shunt bad data
>>> off somewhere without losing the entire transfer. In the COPY case
>>> I think we'd want to be able to capture the error message that
>>> would have been issued, which means the current patches are not
>>> at all appropriate as a basis for that API design: they're just
>>> returning a bool without any details.
>>>
>> I would be in favor of making more of an effort than just a few token
>> data types. The initial patch could just touch a few, but once the
>> infrastructure is in place we should really make a sweep through the
>> tree and tidy up.
> Sure, but my point is that we can do that in a time-extended fashion
> rather than having a flag day where everything must be updated.
> The initial patch just needs to update a few types as proof of concept.
>
And here is a quick POC patch with an example for COPY and float4:

=# CREATE TABLE test (i int, f float4);
CREATE TABLE

=# COPY test (f) FROM stdin WITH (null_on_error (f));
1
err
2
\.

COPY 3

=# SELECT f FROM test;
f
---
1

2
(3 rows)

=# COPY test (i) FROM stdin WITH (null_on_error (i));
ERROR: input function for datatype "integer" does not support error handling

PG_RETURN_ERROR() is a reincarnation of ereport_safe() macro for returning
ErrorData, which was present in older versions (~v18) of SQL/JSON patches.
Later it was replaced with `bool *have_error` and less magical
`if (have_error) ... else ereport(...)`.

Obviously, this needs a separate thread.

--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company

On 31.08.2022 23:39, Nikita Glukhov wrote:

> And here is a quick POC patch with an example for COPY and float4

I decided to go further and use new API in SQL/JSON functions
(even if it does not make real sense now).

I have added function for checking expressions trees, special
executor steps for handling errors in FuncExpr, CoerceViaIO,
CoerceToDomain which are passed through ExprState.edata.

Of course, there is still a lot of work:
1. JIT for new expression steps
2. Removal of subsidary ExprStates (needs another solution for
ErrorData passing)
  3. Checking of domain constraint expressions
4. Error handling in coercion to bytea
5. Error handling in json_populate_type()
6. Error handling in jsonb::type casts
7. ...

Also I have added lazy creation of JSON_VALUE coercions, which was
not present in previous patches. It really greatly speeds up JIT
and reduces memory consumption. But it requires using of subsidary
ExprStates.

jsonb_sqljson test now fails because of points 4, 5, 6.

--
Nikita Glukhov
Postgres Professional:http://www.postgrespro.com
The Russian Postgres Company

On 2022-08-31 We 14:22, Andrew Dunstan wrote:
> On 2022-08-31 We 12:48, Jonathan S. Katz wrote:
>>
>> With RMT hat on -- Andrew can you please revert the patchset?
>
> :-(
>
>
> Yes, I'll do it, starting with the v15 branch. Might take a day or so.
>
>

done

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On 9/1/22 5:13 PM, Andrew Dunstan wrote:
>
> On 2022-08-31 We 14:22, Andrew Dunstan wrote:
>> On 2022-08-31 We 12:48, Jonathan S. Katz wrote:
>>>
>>> With RMT hat on -- Andrew can you please revert the patchset?
>>
>> :-(
>>
>>
>> Yes, I'll do it, starting with the v15 branch. Might take a day or so.
>>
>>
>
> done

Thank you Andrew.

Jonathan

On Wed, Aug 31, 2022 at 03:51:18PM +0900, Amit Langote wrote:
> Finally, I get this warning:
>
> execExprInterp.c: In function ‘ExecJsonCoerceCStringToText’:
> execExprInterp.c:4765:3: warning: missing braces around initializer
> [-Wmissing-braces]
> NameData encoding = {0};
> ^
> execExprInterp.c:4765:3: warning: (near initialization for
> ‘encoding.data’) [-Wmissing-braces]

With what compiler ?

This has came up before:
20211202033145(dot)GK17618(at)telsasoft(dot)com
20220716115932(dot)GV18011(at)telsasoft(dot)com

On Fri, Sep 2, 2022 at 8:56 PM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> On Wed, Aug 31, 2022 at 03:51:18PM +0900, Amit Langote wrote:
> > Finally, I get this warning:
> >
> > execExprInterp.c: In function ‘ExecJsonCoerceCStringToText’:
> > execExprInterp.c:4765:3: warning: missing braces around initializer
> > [-Wmissing-braces]
> > NameData encoding = {0};
> > ^
> > execExprInterp.c:4765:3: warning: (near initialization for
> > ‘encoding.data’) [-Wmissing-braces]
>
> With what compiler ?
>
> This has came up before:
> 20211202033145(dot)GK17618(at)telsasoft(dot)com
> 20220716115932(dot)GV18011(at)telsasoft(dot)com

Didn't realize it when I was reviewing the patch but somehow my build
script had started using gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-44),
which I know is old.

- Amit

On 2022-09-01 Th 09:54, Nikita Glukhov wrote:
>
> On 31.08.2022 23:39, Nikita Glukhov wrote:
>
>> And here is a quick POC patch with an example for COPY and float4
> I decided to go further and use new API in SQL/JSON functions
> (even if it does not make real sense now).
>
> I have added function for checking expressions trees, special
> executor steps for handling errors in FuncExpr, CoerceViaIO,
> CoerceToDomain which are passed through ExprState.edata.
>
> Of course, there is still a lot of work:
> 1. JIT for new expression steps
> 2. Removal of subsidary ExprStates (needs another solution for
> ErrorData passing)
>   3. Checking of domain constraint expressions
> 4. Error handling in coercion to bytea
> 5. Error handling in json_populate_type()
> 6. Error handling in jsonb::type casts
> 7. ...
>
>
> Also I have added lazy creation of JSON_VALUE coercions, which was
> not present in previous patches. It really greatly speeds up JIT
> and reduces memory consumption. But it requires using of subsidary
> ExprStates.
>
>
> jsonb_sqljson test now fails because of points 4, 5, 6.

It looks like this needs to be rebased anyway.

I suggest just submitting the Input function stuff on its own, I think
that means not patches 3,4,15 at this stage. Maybe we would also need a
small test module to call the functions, or at least some of them.

The earlier we can get this in the earlier SQL/JSON patches based on it
can be considered.

A few comments:

. proissafe isn't really a very informative name. Safe for what? maybe
proerrorsafe or something would be better?

. I don't think we need the if test or else clause here:

+   if (edata)
+       return InputFunctionCallInternal(flinfo, str, typioparam,
typmod, edata);
+   else
+       return InputFunctionCall(flinfo, str, typioparam, typmod);

. I think we should probably cover float8 as well as float4, and there
might be some other odd gaps.

As mentioned previously, this should really go in a new thread, so
please don't reply to this but start a completely new thread.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew(at)dunslane(dot)net> writes:
> I suggest just submitting the Input function stuff on its own, I think
> that means not patches 3,4,15 at this stage. Maybe we would also need a
> small test module to call the functions, or at least some of them.
> The earlier we can get this in the earlier SQL/JSON patches based on it
> can be considered.

+1

> . proissafe isn't really a very informative name. Safe for what? maybe
> proerrorsafe or something would be better?

I strongly recommend against having a new pg_proc column at all.
I doubt that you really need it, and having one will create
enormous mechanical burdens to making the conversion. (For example,
needing a catversion bump every time we convert one more function,
or an extension version bump to convert extensions.)

regards, tom lane