Date: Thu, 13 Feb 2020 14:12:28 +0100
From: Dmitry Dolgov <9erthalion6@gmail.com>
To: Pavel Stehule <pavel.stehule@gmail.com>
Cc: Nikita Glukhov <n.gluhov@postgrespro.ru>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	David Fetter <david@fetter.org>,
	Thomas Munro <thomas.munro@gmail.com>,
	Thomas Munro <thomas.munro@enterprisedb.com>,
	Oleksandr Shulgin <oleksandr.shulgin@zalando.de>,
	Tom Lane <tgl@sss.pgh.pa.us>, Robert Haas <robertmhaas@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	Michael Paquier <michael.paquier@gmail.com>,
	Oleg Bartunov <obartunov@gmail.com>,
	Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
	David Steele <david@pgmasters.net>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] [PATCH] Generic type subscripting
Message-ID: <20200213131228.i6afwfmjdod7obko@localhost>
References: 
 <CA+q6zcVFue814UirpoMhTP-EKMSJpKULWziYCRyhbpZ0=iztLw@mail.gmail.com>
 <20190925202205.GA23893@alvherre.pgsql>
 <CA+q6zcXrAL+EDW4CDnjVzycTHPce9uKwC77PvmF4Xd_FY6iK=w@mail.gmail.com>
 <099309f0-4b41-2c78-aac8-0835fe9a9ee7@postgrespro.ru>
 <20191110123208.r4kn5k34wsey5zik@localhost>
 <20191219142030.uc6en2ci3rx2jc4c@localhost>
 <CAFj8pRB5DrNAVv7_OoVLu8Dfd21Nba_9WOh7QA3CD0aLtTO6tQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAFj8pRB5DrNAVv7_OoVLu8Dfd21Nba_9WOh7QA3CD0aLtTO6tQ@mail.gmail.com>
Precedence: bulk

> 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.

> 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.