Re: [PATCH] Generic type subscription

From: Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
To: Artur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
Cc: Victor Wagner <vitus(at)wagner(dot)pp(dot)ru>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH] Generic type subscription
Date: 2016-10-18 17:41:33
Message-ID: CA+q6zcWa_851D_pKiRc28YVjsAoeNEBMyx7hx8g8Z3aqwXbfSQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

>On 5 October 2016 at 22:59, Artur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru> wrote:
>I've looked at your patch to make some review.

Thanks for the feedback.

> On 04.10.2016 11:28, Victor Wagner wrote: Git complains about whitespace
in
> this version of patch:

Fixed.

> The term "subscription" is confusing me

Yes, you're right. "container" is too general I think, so I renamed
everything
to "subscripting".

> Here '1' is used as a second item index. But from the last discussion
> https://www.postgresql.org/message-id/55D24517.8080609%40agliodbs.com it
> should be a key

Well, I missed that, since I used already implemented function "setPath",
and
this function implies that "all path elements before the last must already
exist", so in this case:

select jsonb_set('{"a": {}}', '{a, a1, 1}', '42');

nothing will be changed. I agree, we can implement this as discussed, but
we need
to do it inside "setPath", so it will be like "globally".

> I'm not sure but maybe we should use here two different functions?

I thought about that, but finally decided to keep changes into "pg_type" as
small as possible (anyway, these two functions will serve one logical
purpose).

> Here typeOid argument is not used. Is it should be here?

No, it shouldn't, fixed. It's interesting, that the same is correct for
"findTypeAnalyzeFunction" (I used this function as an example).

> I think name of the function is confusing. Maybe use
> transformContainerType()?

The point is that "transformArrayType" still contains some array-specific
code,
although it doesn't affect to any other type. So I think it's not completely
correct to use "transformContainerType" name.

> Why did you remove static keyword? setPath() is still static
> Is declaration of "new" variable necessary here?

I changed it back.

Here is a new version of patch.

Attachment Content-Type Size
generic_type_subscription_v3.patch text/x-patch 205.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2016-10-18 17:45:36 Re: Typo in foreign.h
Previous Message Tom Lane 2016-10-18 17:38:14 Re: Hash Indexes