Date: Thu, 17 Dec 2020 19:20:03 +0100
From: Dmitry Dolgov <9erthalion6@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Pavel Stehule <pavel.stehule@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	Alexander Korotkov <aekorotkov@gmail.com>,
	Justin Pryzby <pryzby@telsasoft.com>,
	David Steele <david@pgmasters.net>,
	Nikita Glukhov <n.gluhov@postgrespro.ru>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	David Fetter <david@fetter.org>,
	Thomas Munro <thomas.munro@gmail.com>,
	Oleksandr Shulgin <oleksandr.shulgin@zalando.de>,
	Robert Haas <robertmhaas@gmail.com>,
	Michael Paquier <michael.paquier@gmail.com>,
	Oleg Bartunov <obartunov@gmail.com>,
	Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] [PATCH] Generic type subscripting
Message-ID: <20201217182003.nrhxqqiutfwjesen@localhost>
References: <20201209013003.hj4ealuvwmzvczs2@alap3.anarazel.de>
 <3459640.1607477907@sss.pgh.pa.us>
 <3544235.1607482944@sss.pgh.pa.us>
 <3635350.1607536188@sss.pgh.pa.us>
 <20201209183704.x5ue5rctdwmhmlmy@localhost>
 <3674425.1607540422@sss.pgh.pa.us>
 <3724341.1607551174@sss.pgh.pa.us>
 <CAFj8pRCbH_48FfTGHczts8hTA4e0eKhcs5UwdybotoQEH_Pfcg@mail.gmail.com>
 <20201211151957.wqlnxdqjwsd7azzd@localhost>
 <4110100.1607701087@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4110100.1607701087@sss.pgh.pa.us>
Precedence: bulk

> On Fri, Dec 11, 2020 at 10:38:07AM -0500, Tom Lane wrote:
> Dmitry Dolgov <9erthalion6@gmail.com> writes:
> >> On Wed, Dec 09, 2020 at 04:59:34PM -0500, Tom Lane wrote:
> >> 0001 adds the ability to attach a subscript handler to an existing
> >> data type with ALTER TYPE.  This is clearly going to be necessary
> >> if we want extension types to be able to use this facility.  The
> >> only thing that I think might be controversial here is that I did
> >> not add the ability to set pg_type.typelem.
>
> > I'm curious what could be the use case for setting pg_type.typelem for
> > subscripting? I don't see this that much controversial, but maybe I'm
> > missing something.
>
> If you want the result of subscripting to be "text" or some other built-in
> type, then clearly there's no need to use typelem for that, you can just
> refer to the standard OID macros.  The potential use-case that I thought
> of for setting typelem is where an extension defines types A and B and
> would like subscripting of B to yield A.  Installing A's OID as B.typelem
> would save a catalog lookup during subscript parsing, and remove a bunch
> of edge failure cases such as what happens if A gets renamed.  However,
> given the dependency behavior, this would also have the effect of "you
> can't drop A without dropping B, and you can't modify A in any interesting
> way either".  That would be annoyingly restrictive if there weren't any
> actual physical containment relationship.  But on the other hand, maybe
> it's acceptable and we just need to document it.
>
> The other issue is what about existing stored SubscriptingRef structs.
> If our backs were to the wall I'd think about removing the refelemtype
> field so there's no stored image of typelem that needs to be updated.
> But that would incur an extra catalog lookup in array_exec_setup, so
> I don't much like it.  If we do add the ability to set typelem, I'd
> prefer to just warn people to not change it once they've installed a
> subscript handler.
>
> Anyway, between those two issues I'm about -0.1 on adding a way to alter
> typelem.  I won't fight hard if somebody wants it, but I'm inclined
> to leave it out.

Yes, makes sense. Thanks for the clarification.

> On Wed, Dec 09, 2020 at 07:37:04PM +0100, Dmitry Dolgov wrote:
> > On Wed, Dec 09, 2020 at 12:49:48PM -0500, Tom Lane wrote:
> >
> > The jsonb parts now have to be
> > rebased onto this design, which I'm assuming Dmitry will tackle
>
> Yes, I'm already on it, just couldn't keep up with the changes in this
> thread.

While rebasing the jsonb patch I found out that the current subscripting
assignment implementation in transformAssignmentIndirection always
coerce the value to be assigned to the type which subscripting result
suppose to have (refrestype). For arrays it's fine, since those two
indeed must be the same, but for jsonb (and for hstore I guess too) the
result of subscripting is always jsonb (well, text type) and the
assigned value could be of some other type. This leads to assigning
everything converted to text.

Originally this coercion was done in the type specific code, so I hoped
to put it into "transform" routine. Unfortunately "transform" is called
before that (and could not be called later, because type information
from sbsref is required) and all the other hooks are apparently too
late. Probably the most straightforward solution here would be to add a
new argument to transformAssignmentIndirection to signal if coercion
needs to happen or not, and allow the type specific code to specify it
via SubscriptingRef.  Are there any better ideas?