Re: [PATCH] Generic type subscription - Mailing list pgsql-hackers

From Dmitry Dolgov
Subject Re: [PATCH] Generic type subscription
Date
Msg-id CA+q6zcWa_851D_pKiRc28YVjsAoeNEBMyx7hx8g8Z3aqwXbfSQ@mail.gmail.com
Whole thread Raw
In response to Re: [PATCH] Generic type subscription  (Artur Zakirov <a.zakirov@postgrespro.ru>)
Responses Re: [PATCH] Generic type subscription  (Artur Zakirov <a.zakirov@postgrespro.ru>)
List pgsql-hackers
>On 5 October 2016 at 22:59, Artur Zakirov <a.zakirov@postgrespro.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
> 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

pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: Hash Indexes
Next
From: Robert Haas
Date:
Subject: Re: Typo in foreign.h