Re: [HACKERS] [PATCH] Generic type subscripting

 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