Re: [HACKERS] [PATCH] Generic type subscripting

Hi

čt 19. 12. 2019 v 15:20 odesílatel Dmitry Dolgov <9erthalion6@gmail.com>
napsal:

> > On Sun, Nov 10, 2019 at 01:32:08PM +0100, Dmitry Dolgov wrote:
> >
> > > I had to write new assignment logic reusing only some parts of
> setPath(),
> > > because the loop in setPath() should be broken on every level.  During
> this
> > > process, I decided to implement assignment behavior similar to
> PostgreSQL's
> > > array behavior and added two new features:
> > >  - creation of jsonb arrays/objects container from NULL values
> > >  - appending/prepending array elements on the specified position, gaps
> filled
> > >    with nulls (JavaScript has similar behavior)
> >
> > What is the reason for the last one?
>
> I've splitted the last patch into polymorphic itself and jsonb array
> behaviour changes, since I'm afraid it could be a questionable part.
>

 I tested last set of patches.

I like patch 0006 - filling gaps by NULLs - it fixed my objections if I
remember correctly.  Patch 0005 - polymorphic subscribing - I had not a
idea, what is a use case? Maybe can be good to postpone this patch. I have
not strong opinion about it, but generally is good to reduce size of
initial patch. I have nothing against a compatibility with SQL, but this
case doesn't looks too realistic for me, and can be postponed without
future compatibility issues.

I did some notes:

It needs rebase, I had to fix some issues.

I miss deeper comments for

+static Oid
+findTypeSubscriptingFunction(List *procname, Oid typeOid, bool parseFunc)

+/* Callback function signatures --- see xsubscripting.sgml for more info.
*/
+typedef SubscriptingRef * (*SubscriptingPrepare) (bool isAssignment,
SubscriptingRef *sbsef);
+
+typedef SubscriptingRef * (*SubscriptingValidate) (bool isAssignment,
SubscriptingRef *sbsef,
+<-><--><--><--><--><--><--><--><--><--><--><-->   struct ParseState
*pstate);
+
+typedef Datum (*SubscriptingFetch) (Datum source, struct
SubscriptingRefState *sbsrefstate);
+
+typedef Datum (*SubscriptingAssign) (Datum source, struct
SubscriptingRefState *sbrsefstate);
+
+typedef struct SubscriptRoutines
+{
+<->SubscriptingPrepare><-->prepare; #### .
+<->SubscriptingValidate<-->validate;
+<->SubscriptingFetch<-><-->fetch;
+<->SubscriptingAssign<><-->assign;
+
+} SubscriptRoutines;
+

regress tests fails

+Datum
+array_subscript_fetch(Datum containerSource, SubscriptingRefState *sbstate)

there is a variable "is_slice". Original code had not this variable.
Personally I think so original code was better readable without this
variable.

so instead

+<->if (is_slice)
+<->{
+<-><-->for(i = 0; i < sbstate->numlower; i++)
+<-><--><-->l_index.indx[i] = DatumGetInt32(sbstate->lowerindex[i]);
+<->}

is more readable


  if (sbstate->numlower > 0)
  {
     /* read lower part of indexes */
     for (i = 0; i < sbstate->numlower; ...


I miss comments (what is checked here - some like - subscript have to be
int4 and number of subscripts should be less than MAXDIM)

+
+SubscriptingRef *
+array_subscript_prepare(bool isAssignment, SubscriptingRef *sbsref)

+SubscriptingRef *
+array_subscript_validate(bool isAssignment, SubscriptingRef *sbsref,
+<-><--><--><--><-->  ParseState *pstate)

Regression tests fails - see a attachment

I really miss a PLpgSQL support

postgres=# do $$
declare j jsonb = '{"a":10, "b":20}';
begin
  raise notice '%', j;
  raise notice '%', j['a'];
  j['a'] = '20';
  raise notice '%', j;
end;
$$;
NOTICE:  {"a": 10, "b": 20}
NOTICE:  10
ERROR:  subscripted object is not an array
CONTEXT:  PL/pgSQL function inline_code_block line 6 at assignment

With PLpgSQL support it will be great patch, and really important
functionality. It can perfectly cover some gaps of plpgsql.


Regards

Pavel