Re: [HACKERS] [PATCH] Generic type subscripting

čt 13. 2. 2020 v 14:11 odesílatel Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
napsal:

> > On Thu, Feb 13, 2020 at 10:15:14AM +0100, Pavel Stehule wrote:
> >
> > I tested last set of patches.
>
> Thanks a lot for testing!
>
> > 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.
>
> The idea about 0005 is mostly performance related, since this change
> (aside from being more pedantic with the standard) also allows to
> squeeze out some visible processing time improvement. But I agree that
> the patch series itself is too big to add something more, that's why I
> concider 0005/0006 mosly as interesting ideas for the future.
>

patch 0006 is necessary from my perspective. Without it, behave of update
is not practical. I didn't review of this patch mainly due issues that was
fixed by 0006 patch

> > 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;
> > +
> >
> > ....
> >
> > 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)
> >
> Sure, I can probably add more commentaries there.
>
> > +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
>
> Hm, IIRC this is actually necessary, but I'll check one more time.
>
> > 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.
>
> Oh, interesting, I never though about this part. Thanks for mentioning,
> I'll take a look about how can we support the same for PLpgSQL.
>
> > It needs rebase, I had to fix some issues.
> >
> > ...
> >
> > regress tests fails
>
> Yep, I wasn't paying much attention recently to this patch, will post
> rebased and fixed version soon. At the same time I must admit, even if
> at the moment I can pursue two goals - either to make this feature
> accepted somehow, or make a longest living CF item ever - neither of
> those goals seems reachable.
>

I think so this feature is not important for existing applications. But it
allows to work with JSON data (or any other) more comfortable (creative) in
plpgsql.

Pavel