Re: [HACKERS] [PATCH] Generic type subscripting

ne 20. 9. 2020 v 17:46 odesílatel Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
napsal:

> > On Fri, Sep 18, 2020 at 07:23:11PM +0200, Pavel Stehule wrote:
> >
> > > In this way (returning an error on a negative indices bigger than the
> > > number of elements) functionality for assigning via subscripting will
> be
> > > already significantly differ from the original one via jsonb_set. Which
> > > in turn could cause a new wave of something similar to "why assigning
> an
> > > SQL NULL as a value returns NULL instead of jsonb?". Taking into
> account
> > > that this is not absolutely new interface, but rather a convenient
> > > shortcut for the existing one it probably makes sense to try to find a
> > > balance between both consistency with regular array and similarity with
> > > already existing jsonb modification functions.
> > >
> > > Having said that, my impression is that this balance should be not
> fully
> > > shifted towards consistensy with the regular array type, as jsonb array
> > > and regular array are fundamentally different in terms of
> > > implementation. If any differences are of concern, they should be
> > > addressed at different level. At the same time I've already sort of
> gave
> > > up on this patch in the form I wanted to see it anyway, so anything
> goes
> > > if it helps bring it to the finish point. In case if there would be no
> > > more arguments from other involved sides, I can post the next version
> > > with your suggestion included.
> > >
> >
> > This is a relatively new interface and at this moment we can decide if it
> > will be consistent or not. I have not a problem if I have different
> > functions with different behaviors, but I don't like one interface with
> > slightly different behaviors for different types. I understand your
> > argument about implementing a lighter interface to some existing API.
> But I
> > think so more important should be consistency in maximall possible rate
> > (where it has sense).
> >
> > For me "jsonb" can be a very fundamental type in PLpgSQL development - it
> > can bring a lot of dynamic to this environment (it can work perfectly
> like
> > PL/SQL collection or like Perl dictionary), but for this purpose the
> > behaviour should be well consistent without surprising elements.
>
> And here we are, the rebased version with the following changes:
>
> insert into test_jsonb_subscript values (1, '[]');
> update test_jsonb_subscript set test_json[5] = 1;
> select * from test_jsonb_subscript;
> id | test_json
> ----+-----------------------------------
> 1 | [null, null, null, null, null, 1]
> (1 row)
>
> update test_jsonb_subscript set test_json[-8] = 1;
> ERROR: path element at position 1 is out of range: -8
>
> Thanks for the suggestions!
>

Thank you for accepting my suggestions.

I checked this set of patches and it looks well.

I have only one minor comment. I understand the error message, but I am not
sure if without deeper knowledge I can understand.

+update test_jsonb_subscript set test_json[-8] = 1;
+ERROR: path element at position 1 is out of range: -8

Maybe 'value of subscript "-8" is out of range'. Current error message is
fully correct - but people probably have to think "what is a path element
at position 1?" It doesn't look intuitive.

Do you have some idea?

My comment is minor, and I mark this patch with pleasure as ready for
committer.

patching and compiling - without problems
implemented functionality - I like it
Building doc - without problems
make check-world - passed

Regards

Pavel