jsonpath

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
198 messages Options
1234567 ... 10
Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Alexander Korotkov
On Mon, Jan 21, 2019 at 6:05 PM Oleg Bartunov <[hidden email]> wrote:

> On Mon, Jan 21, 2019 at 1:40 AM Alexander Korotkov
> <[hidden email]> wrote:
> >
> > On Sun, Jan 20, 2019 at 6:30 AM Alexander Korotkov
> > <[hidden email]> wrote:
> > > I'll continue revising this patchset.  Nikita, could you please write
> > > tests for 3-argument versions of functions?  Also, please answer the
> > > question regarding "id" property.
> >
> > I've some more notes regarding function set provided in jsonpath patch:
> > 1) Function names don't follow the convention we have.  All our
> > functions dealing with jsonb have "jsonb_" prefix.  Exclusions have
> > "jsonb" in other part of name, for example, "to_jsonb(anyelement)".
> > We could name functions at SQL level in the same way they are named in
> > C.  So, they would be jsonb_jsonpath_exists() etc.  But it's probably
> > too long.  What about jsonb_path_exists() etc?
>
> jsonpath_exists is similar to xpath_exists.

That's true.  The question is whether it's more important to follow
json[b] naming convention or xml/xpath naming convention?  I guess
json[b] naming convention is more important in our case.

> Actually, we could use jsonb_exists(jsonb, jsonpath, json), but then
> we should specify the type of the second argument.

Yes, but AFAICS the key point of json[b]_ prefixes is to evade
function overloading.  So, I'm -1 for use jsonb_ prefix and have
function overload because of that.

> > 2) jsonpath_query_wrapped() name seems counter intuitive for me.  What
> > about jsonpath_query_array()?
>
> The question is should we try to provide a functional interface for
> all options of
> JSON_QUERY clause ?  The same is for  other SQL/JSON clauses.
> Currently,  patch contains very limited subset of JSON_QUERY
> functionality,  mostly for jsonpath testing.

Actually, my point is following.  We have jsonpath patch close to the
committable shape.  And we have patch for SQL/JSON clauses including
JSON_QUERY, which is huge, complex and didn't receive any serious
review yet.  So, we'll did our best on that patch during this release
cycle, but I can't guarantee it will get in PostgreSQL 12.  Thus, my
idea is to make jsonpath patch self contained by providing brief and
convenient procedural interface.  This procedural interface is anyway
not a part of standard.  It *might* be inspired by standard clauses,
but might be not.  I think we should try to make this procedural
interface as good and convenient by itself.  It's our extension, and
it wouldn't make us more or less standard conforming.

> > 3) jsonpath_query_wrapped() behavior looks tricky to me.  It returns
> > NULL for no results.  When result item is one, it is returned "as is".
> > When there are two or more result items, they are wrapped into array.
> > This representation of result seems extremely inconvenient for me.  It
> > becomes hard to solve even trivial tasks with that: for instance,
> > iterate all items found.  Without extra assumptions on what is going
> > to be returned it's also impossible for caller to distinguish single
> > array item found from multiple items found wrapped into array.  And
> > that seems very bad.  I know this behavior is inspired by SQL/JSON
> > standard.  But since these functions are anyway our extension, we can
> > implement them as we like.  So, I propose to make this function always
> > return array of items regardless on count of those items (even for 1
> > or 0 items).
>
> Fair enough, but if we agree, that we provide an exact functionality of
> SQL clauses, then better to follow the standard to avoid problems.

No, I see this as our extension.  And I don't see problems in being
different from standard clauses, because it's different anyway.  For
me, in this case it's better to evade problems of users.  And current
behavior of this function seems like just single big pain :)

> > 4) If we change behavior of jsonpath_query_wrapped() as above, we can
> > also implement jsonpath_query_one() function, which would return first
> > result item or NULL for no items.
> > Any thoughts?
>
> I think, that we should postpone this functional interface, which could be
> added later.

The reason we typically postpone things is that they are hard to bring
to committable shape.  jsonpath_query_one() doesn't cost us any real
development.  So, I don't see point in postponing that if consider
that as good part of procedural jsonpath interface.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Alexander Korotkov
The next revision is attached.

Nikita made code and documentation improvements, renamed functions
from "jsonpath_" prefix to "jsonb_path_" prefix.  He also renamed
jsonpath_predicate() to jsonb_path_match() (that looks better for me
too).

I've further renamed jsonb_query_wrapped() to jsonb_query_array(), and
changed that behavior to always wrap result into array.  Also, I've
introduced new function jsonb_query_first(), which returns first item
from result.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

0001-Preliminary-datetime-infrastructure-v25.patch (44K) Download Attachment
0002-Jsonpath-engine-and-docs-v25.patch (382K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Oleg Bartunov-3
On Tue, Jan 22, 2019 at 8:21 AM Alexander Korotkov
<[hidden email]> wrote:

>
> The next revision is attached.
>
> Nikita made code and documentation improvements, renamed functions
> from "jsonpath_" prefix to "jsonb_path_" prefix.  He also renamed
> jsonpath_predicate() to jsonb_path_match() (that looks better for me
> too).
>
> I've further renamed jsonb_query_wrapped() to jsonb_query_array(), and
> changed that behavior to always wrap result into array.

agree with new names

so it mimic the behaviour of JSON_QUERY with UNCONDITIONAL WRAPPER option

 Also, I've
> introduced new function jsonb_query_first(), which returns first item
> from result.
>
> ------
> Alexander Korotkov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company



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

Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Nikita Glukhov
Attached 26th version of the patches:

 * Documentation:
    - Fixed some mistakes
    - Removed mention of syntax extensions not present in this patch
    - Documented '.datetime(format, default_tz)'
 * Removed accidental syntax extension allowing to write '@.key' without '@'

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



0001-Preliminary-datetime-infrastructure-v26.patch (34K) Download Attachment
0002-Jsonpath-engine-and-docs-v26.patch (287K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Alexander Korotkov
On Wed, Jan 23, 2019 at 3:40 AM Nikita Glukhov <[hidden email]> wrote:
> Attached 26th version of the patches:
>
>  * Documentation:
>     - Fixed some mistakes
>     - Removed mention of syntax extensions not present in this patch
>     - Documented '.datetime(format, default_tz)'
>  * Removed accidental syntax extension allowing to write '@.key' without '@'

Thank you!

I've made few more changes to patchset:
 * Change back interface of JsonbExtractScalar().  I decide it would
be better to keep it.
 * Run pgindent
 * Improve documentation of .keyvalue() function.

Finally, I'm going to commit this if no objections.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

0001-Preliminary-datetime-infrastructure-v27.patch (44K) Download Attachment
0002-Jsonpath-engine-and-docs-v27.patch (378K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Alexander Korotkov
In reply to this post by Oleg Bartunov-3
On Tue, Jan 22, 2019 at 12:13 PM Oleg Bartunov <[hidden email]> wrote:

>
> On Tue, Jan 22, 2019 at 8:21 AM Alexander Korotkov
> <[hidden email]> wrote:
> >
> > The next revision is attached.
> >
> > Nikita made code and documentation improvements, renamed functions
> > from "jsonpath_" prefix to "jsonb_path_" prefix.  He also renamed
> > jsonpath_predicate() to jsonb_path_match() (that looks better for me
> > too).
> >
> > I've further renamed jsonb_query_wrapped() to jsonb_query_array(), and
> > changed that behavior to always wrap result into array.
>
> agree with new names
>
> so it mimic the behaviour of JSON_QUERY with UNCONDITIONAL WRAPPER option

Good, thank you for feedback!

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Nikita Glukhov
In reply to this post by Alexander Korotkov
Attached 28th version of the patches.

On 23.01.2019 8:01, Alexander Korotkov wrote:

On Wed, Jan 23, 2019 at 3:40 AM Nikita Glukhov [hidden email] wrote:
Attached 26th version of the patches:

 * Documentation:
    - Fixed some mistakes
    - Removed mention of syntax extensions not present in this patch
    - Documented '.datetime(format, default_tz)'
 * Removed accidental syntax extension allowing to write '@.key' without '@'
Thank you!

I've made few more changes to patchset:
 * Change back interface of JsonbExtractScalar().  I decide it would
be better to keep it.
 * Run pgindent
I have fixed indentation by adding new typedefs to 
src/tools/pgindent/typedefs.list.

Also I have renamed LikeRegexContext => JsonLikeRegexContext.

 * Improve documentation of .keyvalue() function.

Finally, I'm going to commit this if no objections.
Thank you!

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


0001-Preliminary-datetime-infrastructure-v28.patch (34K) Download Attachment
0002-Jsonpath-engine-and-docs-v28.patch (289K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Alexander Korotkov
On Wed, Jan 23, 2019 at 3:24 PM Nikita Glukhov <[hidden email]> wrote:

> Attached 28th version of the patches.
>
> On 23.01.2019 8:01, Alexander Korotkov wrote:
>
> On Wed, Jan 23, 2019 at 3:40 AM Nikita Glukhov <[hidden email]> wrote:
>
> Attached 26th version of the patches:
>
>  * Documentation:
>     - Fixed some mistakes
>     - Removed mention of syntax extensions not present in this patch
>     - Documented '.datetime(format, default_tz)'
>  * Removed accidental syntax extension allowing to write '@.key' without '@'
>
> Thank you!
>
> I've made few more changes to patchset:
>  * Change back interface of JsonbExtractScalar().  I decide it would
> be better to keep it.
>  * Run pgindent
>
> I have fixed indentation by adding new typedefs to
> src/tools/pgindent/typedefs.list.
Good catch, thank you!

> Also I have renamed LikeRegexContext => JsonLikeRegexContext.
>  * Improve documentation of .keyvalue() function.

Ok!

I've also fix bug with possible reference to already freed value of
jsonpath in jsonb_path_execute(), pointed by you.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

0001-Preliminary-datetime-infrastructure-v29.patch (44K) Download Attachment
0002-Jsonpath-engine-and-docs-v29.patch (379K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Alexander Korotkov
In reply to this post by Alexander Korotkov
On Wed, Jan 23, 2019 at 8:01 AM Alexander Korotkov
<[hidden email]> wrote:
> Finally, I'm going to commit this if no objections.

BTW, I decided to postpone commit for few days.  Nikita and me are
still working on better error messages.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Alexander Korotkov
On Sat, Jan 26, 2019 at 4:27 AM Alexander Korotkov
<[hidden email]> wrote:
>
> On Wed, Jan 23, 2019 at 8:01 AM Alexander Korotkov
> <[hidden email]> wrote:
> > Finally, I'm going to commit this if no objections.
>
> BTW, I decided to postpone commit for few days.  Nikita and me are
> still working on better error messages.

Updated patchset is attached.  This patchset includes:

* Improved error handling by Nikita, revised by me,
* Code beautification.

So, I'm going to commit this again.  This time seriously :)

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

0001-Preliminary-datetime-infrastructure-v30.patch (44K) Download Attachment
0002-Jsonpath-engine-and-docs-v30.patch (403K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Alexander Korotkov
On Sun, Jan 27, 2019 at 1:50 PM Alexander Korotkov
<[hidden email]> wrote:

> On Sat, Jan 26, 2019 at 4:27 AM Alexander Korotkov
> <[hidden email]> wrote:
> >
> > On Wed, Jan 23, 2019 at 8:01 AM Alexander Korotkov
> > <[hidden email]> wrote:
> > > Finally, I'm going to commit this if no objections.
> >
> > BTW, I decided to postpone commit for few days.  Nikita and me are
> > still working on better error messages.
>
> Updated patchset is attached.  This patchset includes:
>
> * Improved error handling by Nikita, revised by me,
> * Code beautification.
>
> So, I'm going to commit this again.  This time seriously :)
I'm really sorry for confusing people, but I've one more revision.
This is my first time attempting to commit such a large patch.

Major changes are following:
 * We find it ridiculous to save ErrorData for possible latter throw.
Now, we either throw an error immediately or return jperError.  That
also allows to get rid of unwanted changes in elog.c/elog.h.
 * I decided to change behavior of jsonb_path_match() to throw as less
errors as possible.  The reason is that it's used to implement
potentially (patch is pending) indexable operator.  Index scan is not
always capable to throw as many errors and sequential scan.  So, it's
better to not introduce extra possible index scan and sequential scan
results divergence.

So, this is version I'm going to commit unless Nikita has corrections
or anybody else objects.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

0001-Preliminary-datetime-infrastructure-v31.patch (44K) Download Attachment
0002-Jsonpath-engine-and-docs-v31.patch (399K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Tomas Vondra-4


On 1/28/19 1:22 AM, Alexander Korotkov wrote:

> On Sun, Jan 27, 2019 at 1:50 PM Alexander Korotkov
> <[hidden email]> wrote:
>> On Sat, Jan 26, 2019 at 4:27 AM Alexander Korotkov
>> <[hidden email]> wrote:
>>>
>>> On Wed, Jan 23, 2019 at 8:01 AM Alexander Korotkov
>>> <[hidden email]> wrote:
>>>> Finally, I'm going to commit this if no objections.
>>>
>>> BTW, I decided to postpone commit for few days.  Nikita and me are
>>> still working on better error messages.
>>
>> Updated patchset is attached.  This patchset includes:
>>
>> * Improved error handling by Nikita, revised by me,
>> * Code beautification.
>>
>> So, I'm going to commit this again.  This time seriously :)
>
> I'm really sorry for confusing people, but I've one more revision.
> This is my first time attempting to commit such a large patch.
>

I'm not sure what you're apologizing for, it simply shows how careful
you are when polishing such a large patch.

> Major changes are following:
>  * We find it ridiculous to save ErrorData for possible latter throw.
> Now, we either throw an error immediately or return jperError.  That
> also allows to get rid of unwanted changes in elog.c/elog.h.

OK.

>  * I decided to change behavior of jsonb_path_match() to throw as less
> errors as possible.  The reason is that it's used to implement
> potentially (patch is pending) indexable operator.  Index scan is not
> always capable to throw as many errors and sequential scan.  So, it's
> better to not introduce extra possible index scan and sequential scan
> results divergence.
>

Hmmm, can you elaborate a bit more? Which errors were thrown before and
are not thrown with the current patch version?

regards

--
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Nikita Glukhov

On 28.01.2019 16:50, Tomas Vondra wrote:

On 1/28/19 1:22 AM, Alexander Korotkov wrote:
 * I decided to change behavior of jsonb_path_match() to throw as less
errors as possible.  The reason is that it's used to implement
potentially (patch is pending) indexable operator.  Index scan is not
always capable to throw as many errors and sequential scan.  So, it's
better to not introduce extra possible index scan and sequential scan
results divergence.
Hmmm, can you elaborate a bit more? Which errors were thrown before and
are not thrown with the current patch version?
In the previous version of the patch jsonb_path_match() threw error when
jsonpath did not return a singleton value, but in the last version in such cases
NULL is returned.  This problem arises because we cannot guarantee at compile
time that jsonpath expression used in jsonb_path_match() is a predicate.
Predicates by standard can return only True, False, and Unknown (errors occurred
during execution of their operands are transformed into Unknown values), so 
predicates cannot throw errors, and there are no problems with errors. 

GIN does not attempt to search non-predicate expressions, so there may be no 
problem even we throw "not a singleton" error.


Here I want to remind that ability to use predicates in the root of jsonpath
expression is an our extension to standard that was created specially for the
operator @@.  By standard predicates are allowed only in filters.  Without this
extension we are still able to rewrite @@ using @?:
jsonb @@ 'predicate'        is equivalent to
jsonb @? '$ ? (predicate)'
but such @? expression is a bit slower to execute and a bit verbose to write.

If we introduced special type 'jsonpath_predicate', then we could solve the
problem by checking the type of jsonpath expression at compile-time.


Another problem with error handling is that jsonb_path_query*() functions
always throw SQL/JSON errors and there is no easy and effective way to emulate
NULL ON ERROR behavior, which is used by default in SQL/JSON functions.  So I
think it's worth trying to add some kind of flag 'throwErrors' to
jsonb_path_query*() functions.

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

Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Alexander Korotkov
On Tue, Jan 29, 2019 at 2:17 AM Nikita Glukhov <[hidden email]> wrote:
> In the previous version of the patch jsonb_path_match() threw error when
> jsonpath did not return a singleton value, but in the last version in such cases
> NULL is returned.  This problem arises because we cannot guarantee at compile
> time that jsonpath expression used in jsonb_path_match() is a predicate.
> Predicates by standard can return only True, False, and Unknown (errors occurred
> during execution of their operands are transformed into Unknown values), so
> predicates cannot throw errors, and there are no problems with errors.

Attached patchset provides description of errors suppressed.  It also
clarifies how jsonb_path_match() works.

> GIN does not attempt to search non-predicate expressions, so there may be no
> problem even we throw "not a singleton" error.

Yes, I don't insist on that.  If majority of us wants to bring "not a
singleton" error back, I don't object to it.

> Here I want to remind that ability to use predicates in the root of jsonpath
> expression is an our extension to standard that was created specially for the
> operator @@.  By standard predicates are allowed only in filters.  Without this
> extension we are still able to rewrite @@ using @?:
> jsonb @@ 'predicate'        is equivalent to
> jsonb @? '$ ? (predicate)'
> but such @? expression is a bit slower to execute and a bit verbose to write.
>
> If we introduced special type 'jsonpath_predicate', then we could solve the
> problem by checking the type of jsonpath expression at compile-time.
For me it seems that separate datatype for this kind of problem is overkill.

> Another problem with error handling is that jsonb_path_query*() functions
> always throw SQL/JSON errors and there is no easy and effective way to emulate
> NULL ON ERROR behavior, which is used by default in SQL/JSON functions.  So I
> think it's worth trying to add some kind of flag 'throwErrors' to
> jsonb_path_query*() functions.

Good idea, but let's commit basic jsonpath implementation first.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

0001-Preliminary-datetime-infrastructure-v32.patch (44K) Download Attachment
0002-Jsonpath-engine-and-docs-v32.patch (401K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Andres Freund
Hi,

On 2019-01-29 04:00:19 +0300, Alexander Korotkov wrote:

> + /*
> + * It is safe to use here PG_TRY/PG_CATCH without subtransaction because
> + * no function called inside performs data modification.
> + */
> + PG_TRY();
> + {
> + res = DirectFunctionCall2(func, ldatum, rdatum);
> + }
> + PG_CATCH();
> + {
> + int errcode = geterrcode();
> +
> + if (jspThrowErrors(cxt) ||
> + ERRCODE_TO_CATEGORY(errcode) != ERRCODE_DATA_EXCEPTION)
> + PG_RE_THROW();
> +
> + MemoryContextSwitchTo(mcxt);
> + FlushErrorState();
> +
> + return jperError;
> + }
> + PG_END_TRY();

FWIW, I still think this is a terrible idea and shouldn't be merged this
way. The likelihood of introducing subtle bugs seems way too high - even
if it's possibly not buggy today, who says that it's not going to be in
the future?

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Alexander Korotkov
On Tue, Jan 29, 2019 at 4:03 AM Andres Freund <[hidden email]> wrote:

> On 2019-01-29 04:00:19 +0300, Alexander Korotkov wrote:
> > +     /*
> > +      * It is safe to use here PG_TRY/PG_CATCH without subtransaction because
> > +      * no function called inside performs data modification.
> > +      */
> > +     PG_TRY();
> > +     {
> > +             res = DirectFunctionCall2(func, ldatum, rdatum);
> > +     }
> > +     PG_CATCH();
> > +     {
> > +             int                     errcode = geterrcode();
> > +
> > +             if (jspThrowErrors(cxt) ||
> > +                     ERRCODE_TO_CATEGORY(errcode) != ERRCODE_DATA_EXCEPTION)
> > +                     PG_RE_THROW();
> > +
> > +             MemoryContextSwitchTo(mcxt);
> > +             FlushErrorState();
> > +
> > +             return jperError;
> > +     }
> > +     PG_END_TRY();
>
> FWIW, I still think this is a terrible idea and shouldn't be merged this
> way. The likelihood of introducing subtle bugs seems way too high - even
> if it's possibly not buggy today, who says that it's not going to be in
> the future?

I'm probably not yet understanding all the risks this code have.  So far I see:
1) One of functions called here performs database modification, while
it wasn't suppose to.  So, it becomes not safe to skip subtransaction.
2) ERRCODE_DATA_EXCEPTION was thrown for unexpected reason.  So, it
might appear that ERRCODE_DATA_EXCEPTION is not safe to ignore.
Could you complete this list?

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Alexander Korotkov
In reply to this post by Andres Freund
On Tue, Jan 29, 2019 at 4:03 AM Andres Freund <[hidden email]> wrote:

> On 2019-01-29 04:00:19 +0300, Alexander Korotkov wrote:
> > +     /*
> > +      * It is safe to use here PG_TRY/PG_CATCH without subtransaction because
> > +      * no function called inside performs data modification.
> > +      */
> > +     PG_TRY();
> > +     {
> > +             res = DirectFunctionCall2(func, ldatum, rdatum);
> > +     }
> > +     PG_CATCH();
> > +     {
> > +             int                     errcode = geterrcode();
> > +
> > +             if (jspThrowErrors(cxt) ||
> > +                     ERRCODE_TO_CATEGORY(errcode) != ERRCODE_DATA_EXCEPTION)
> > +                     PG_RE_THROW();
> > +
> > +             MemoryContextSwitchTo(mcxt);
> > +             FlushErrorState();
> > +
> > +             return jperError;
> > +     }
> > +     PG_END_TRY();

BTW, this code also looks... useless.  I can't see whole numeric.c
throwing ERRCODE_DATA_EXCEPTION.   Nikita, what do we mean here?

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Andres Freund
In reply to this post by Alexander Korotkov
On 2019-01-29 04:17:33 +0300, Alexander Korotkov wrote:

> On Tue, Jan 29, 2019 at 4:03 AM Andres Freund <[hidden email]> wrote:
> > On 2019-01-29 04:00:19 +0300, Alexander Korotkov wrote:
> > > +     /*
> > > +      * It is safe to use here PG_TRY/PG_CATCH without subtransaction because
> > > +      * no function called inside performs data modification.
> > > +      */
> > > +     PG_TRY();
> > > +     {
> > > +             res = DirectFunctionCall2(func, ldatum, rdatum);
> > > +     }
> > > +     PG_CATCH();
> > > +     {
> > > +             int                     errcode = geterrcode();
> > > +
> > > +             if (jspThrowErrors(cxt) ||
> > > +                     ERRCODE_TO_CATEGORY(errcode) != ERRCODE_DATA_EXCEPTION)
> > > +                     PG_RE_THROW();
> > > +
> > > +             MemoryContextSwitchTo(mcxt);
> > > +             FlushErrorState();
> > > +
> > > +             return jperError;
> > > +     }
> > > +     PG_END_TRY();
> >
> > FWIW, I still think this is a terrible idea and shouldn't be merged this
> > way. The likelihood of introducing subtle bugs seems way too high - even
> > if it's possibly not buggy today, who says that it's not going to be in
> > the future?
>
> I'm probably not yet understanding all the risks this code have.  So far I see:

I find these *more* than sufficient to not go to the PG_TRY/CATCH
approach.


> 1) One of functions called here performs database modification, while
> it wasn't suppose to.  So, it becomes not safe to skip subtransaction.

It's not just data modifications. Even just modifying some memory
structures that'd normally be invalidated by an xact abort's
invalidation processing isn't safe.


> 2) ERRCODE_DATA_EXCEPTION was thrown for unexpected reason.  So, it
> might appear that ERRCODE_DATA_EXCEPTION is not safe to ignore.

It'd e.g. not surprise me very much if some OOM would end up translating
to ERRCODE_DATA_EXCEPTION, because some library function returned an
error due to ENOMEM.


> Could you complete this list?

3) The expression changed the current expression context, GUCs or any
   other such global variable. Without a proper subtrans reset this
   state isn't reverted.
4) The function acquires an LWLOCK, buffer reference, anything resowner
   owned. Skipping subtrans reset, that's not released in that
   moment. That's going to lead to potential hard deadlocks.
99) sigsetjmp is actually pretty expensive.

Greetings,

Andres Freund

Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Tom Lane-2
In reply to this post by Alexander Korotkov
Alexander Korotkov <[hidden email]> writes:
> On Tue, Jan 29, 2019 at 4:03 AM Andres Freund <[hidden email]> wrote:
>> FWIW, I still think this is a terrible idea and shouldn't be merged this
>> way. The likelihood of introducing subtle bugs seems way too high - even
>> if it's possibly not buggy today, who says that it's not going to be in
>> the future?

> I'm probably not yet understanding all the risks this code have.  So far I see:
> 1) One of functions called here performs database modification, while
> it wasn't suppose to.  So, it becomes not safe to skip subtransaction.
> 2) ERRCODE_DATA_EXCEPTION was thrown for unexpected reason.  So, it
> might appear that ERRCODE_DATA_EXCEPTION is not safe to ignore.
> Could you complete this list?

Sure: every errcode we have is unsafe to treat this way.

The backend coding rule from day one has been that a thrown error requires
(sub)transaction cleanup to be done to make sure that things are back in a
good state.  You can *not* just decide that it's okay to ignore that,
especially not when invoking code outside the immediate area of what
you're doing.

As a counterexample, for any specific errcode you might claim is safe,
a plpgsql function might do something that requires cleanup (which is
not equal to "does data modification") and then throw that errcode.

You might argue that this code will only ever be used to call certain
functions in numeric.c and you've vetted every line of code that those
functions can reach ... but even to say that is to expose how fragile
the argument is.  Every time anybody touched numeric.c, or any code
reachable from there, we'd have to wonder whether we just introduced
a failure hazard for jsonpath.  That's not maintainable.  It's even
less maintainable if anybody decides they'd like to insert extension
hooks into any of that code.  I rather imagine that this could be
broken today by an ErrorContextCallback function, for example.
(That is, even today, "vetting every line" has to include vetting every
errcontext subroutine in the system, along with everything it can call.
Plus equivalent code in external PLs and so on.)

We've rejected code like this every time it was submitted to date,
and I don't see a reason to change that policy for jsonpath.

                        regards, tom lane

Reply | Threaded
Open this post in threaded view
|

Re: jsonpath

Alexander Korotkov
In reply to this post by Alexander Korotkov
On Tue, Jan 29, 2019 at 4:30 AM Alexander Korotkov
<[hidden email]> wrote:

> On Tue, Jan 29, 2019 at 4:03 AM Andres Freund <[hidden email]> wrote:
> > On 2019-01-29 04:00:19 +0300, Alexander Korotkov wrote:
> > > +     /*
> > > +      * It is safe to use here PG_TRY/PG_CATCH without subtransaction because
> > > +      * no function called inside performs data modification.
> > > +      */
> > > +     PG_TRY();
> > > +     {
> > > +             res = DirectFunctionCall2(func, ldatum, rdatum);
> > > +     }
> > > +     PG_CATCH();
> > > +     {
> > > +             int                     errcode = geterrcode();
> > > +
> > > +             if (jspThrowErrors(cxt) ||
> > > +                     ERRCODE_TO_CATEGORY(errcode) != ERRCODE_DATA_EXCEPTION)
> > > +                     PG_RE_THROW();
> > > +
> > > +             MemoryContextSwitchTo(mcxt);
> > > +             FlushErrorState();
> > > +
> > > +             return jperError;
> > > +     }
> > > +     PG_END_TRY();
>
> BTW, this code also looks... useless.  I can't see whole numeric.c
> throwing ERRCODE_DATA_EXCEPTION.   Nikita, what do we mean here?

Oh, sorry.  I've missed we have ERRCODE_TO_CATEGORY() here.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

1234567 ... 10