[PATCH] Generic type subscripting

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
141 messages Options
12345 ... 8
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Dmitry Dolgov
On 1 April 2017 at 21:26, Arthur Zakirov <[hidden email]> wrote:
>
> The patch looks good to me. But it is not applied :) I think it is
> because of new FTS functions for jsonb. The patch need rebasing.

Sorry for late reply. Here is a new version of the patch, I rebased it and
fixed those issues you've mentioned (pretty nasty problems, thank you for
noticing).


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

generic_type_subscription_v11.patch (297K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

a.zakirov
On 04.04.2017 15:41, Dmitry Dolgov wrote:
> Sorry for late reply. Here is a new version of the patch, I rebased it and
> fixed those issues you've mentioned (pretty nasty problems, thank you for
> noticing).

Thank you!

I've looked at the patch again.

I'd like to focus on "refevalfunc" and "refnestedfunc" fields as I did
earlier. I think using Oid type for them is a bad approach. "..._fetch"
and "..._assign" functions in catalog is unnecessary movement to me.
User of subscript of his type may think the same. But he won't see the
code and won't know why he needs these functions.

And so "..._fetch" and "..._assign" functions in catalog is a bad design
to me. But, of course, it is just my opinion. This approach is the main
think which we should resolve first, because after commiting the patch
it will be hard to fix it.

>  static int ArrayCount(const char *str, int *dim, char typdelim);
> +bool isAssignmentIndirectionExpr(ExprState *exprstate);
>  static void ReadArrayStr(char *arrayStr, const char *origStr,

I think isAssignmentIndirectionExpr() here was forgoten to delete,
because isAssignmentIndirectionExpr() is in execExpr.c now.

> + if (subexpr == NULL)
> + {
> + lowerIndexpr = lappend(lowerIndexpr, subexpr);
> + continue;
> + }
> +
> +

There is the extra line here after the brace.

> if (array_type != sbsref->refcontainertype)
> {
>
> node = coerce_to_target_type(pstate,
> node, array_type,
> sbsref->refcontainertype, sbsref->reftypmod,
> COERCION_ASSIGNMENT,
> COERCE_IMPLICIT_CAST,
> -1);
>
> /* can fail if we had int2vector/oidvector, but not for true domains */
> if (node == NULL && node->type != 0)
> ereport(ERROR,
> (errcode(ERRCODE_CANNOT_COERCE),
> errmsg("cannot cast type %s to %s",
> format_type_be(array_type),
> format_type_be(sbsref->refcontainertype)),
> parser_errposition(pstate, 0)));
>
> PG_RETURN_POINTER(node);
> }

Also I was wondering do we need this code in array_subscript_parse()? I
haven't understood the purpose of it. If it is necessary then would be
good to add explain comment.

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

a.zakirov
On 05.04.2017 16:06, Arthur Zakirov wrote:

> On 04.04.2017 15:41, Dmitry Dolgov wrote:
>> Sorry for late reply. Here is a new version of the patch, I rebased it
>> and
>> fixed those issues you've mentioned (pretty nasty problems, thank you for
>> noticing).
>
> Thank you!
>
> I've looked at the patch again.
>

Sorry maybe it's too naive. Also I was wondering.

> + element_type_id = transformArrayType(&array_type, &array_typ_mode);
> + sbsref->refelemtype = element_type_id;

I don't understand this part of the patch. Why is it necessary to
execute transformArrayType() second time? It was executed in
transformContainerSubscripts().

> + if (!OidIsValid(elementType))
> + elementType = containerType;

This part looks strange to me too.

It this parts are necessary it would be good to add comments.

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

a.zakirov
In reply to this post by a.zakirov
On 05.04.2017 16:06, Arthur Zakirov wrote:

>
> I'd like to focus on "refevalfunc" and "refnestedfunc" fields as I did
> earlier. I think using Oid type for them is a bad approach. "..._fetch"
> and "..._assign" functions in catalog is unnecessary movement to me.
> User of subscript of his type may think the same. But he won't see the
> code and won't know why he needs these functions.
>
> And so "..._fetch" and "..._assign" functions in catalog is a bad design
> to me. But, of course, it is just my opinion. This approach is the main
> think which we should resolve first, because after commiting the patch
> it will be hard to fix it.
>

I've read olders messages and thread. I see now that this approach was
made with other hackers. I've just been confused when I've been
implementing subscript for ltree.

Sorry if I confused you.

Any opinions about the patch?

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Dmitry Dolgov
> On 1 April 2017 at 21:26, Arthur Zakirov <[hidden email]> wrote:
>
> Also I was wondering do we need this code in array_subscript_parse()? I
> haven't understood the purpose of it. If it is necessary then would be
> good to add explain comment.

Well, it's necessary because `array_type` can be modified by
`transformArrayType` and we have to perform coercion again. I'm not sure if
more explanation for that is required, can you suggest something to add here?

> I don't understand this part of the patch. Why is it necessary to
> execute transformArrayType() second time? It was executed in
> transformContainerSubscripts().

Yes, that's my mistake, I removed one from `parse_node.c`.
Here is a new slightly improved version of the patch.


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

generic_type_subscription_v12.patch (297K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Dmitry Dolgov
On 28 February 2017 at 19:02, Dmitry Dolgov <[hidden email]> wrote:
>
> Regarding to the previous conversation [1], here is the patch for generic type
> subscripting with several improvements. It contains the following changes:

So, a few words about current state of the patch:

* after a lot of serious improvements general design of this feature is agreeable

* we introduced a lot of small changes to polish it

* I rebased the patch on the latest version of master, so you can take a look at it again

As always, any feedback is welcome.


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

generic_type_subscription_v13.patch (298K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

a.zakirov
On Wednesday, 10 May 2017 23:43:10 MSK, Dmitry Dolgov wrote:

> So, a few words about current state of the patch:
>
> * after a lot of serious improvements general design of this feature is
> agreeable
>
> * we introduced a lot of small changes to polish it
>
> * I rebased the patch on the latest version of master, so you can take a
> look at it again
>
> As always, any feedback is welcome.

Hello,

Can you rebase the patch please? It is not applyed now. I think it is because
of pgindent.

> +
> + scratch->d.sbsref.eval_finfo = eval_finfo;
> + scratch->d.sbsref.nested_finfo = nested_finfo;
> +

Also I have noticed that assigning eval_finfo and nested_finfo after every time
eval step is pushed is unnecessary in ExecInitSubscriptingRef() function. We
need them only for EEOP_SBSREF_OLD, EEOP_SBSREF_ASSIGN and EEOP_SBSREF_FETCH
steps.

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Dmitry Dolgov
> On 30 June 2017 at 11:34, Arthur Zakirov <[hidden email]> wrote:
>
> Can you rebase the patch please? It is not applyed now. I think it is because
> of pgindent.

Sure, I've attached the rebased version of the patch.

>
> > +
> > +             scratch->d.sbsref.eval_finfo = eval_finfo;
> > +             scratch->d.sbsref.nested_finfo = nested_finfo;
> > +
>
> Also I have noticed that assigning eval_finfo and nested_finfo after every time
> eval step is pushed is unnecessary in ExecInitSubscriptingRef() function. We
> need them only for EEOP_SBSREF_OLD, EEOP_SBSREF_ASSIGN and EEOP_SBSREF_FETCH
> steps.

I'm not sure, because an absence of any of those `eval_finfo`/`nested_finfo`
blocks in `ExecInitSubscriptingRef` breaks few tests.


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

generic_type_subscription_v14.patch (298K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Dmitry Dolgov
Here is a new rebased version of this patch (there were some conflicts in commentaries).


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

generic_type_subscription_v15.patch (299K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Dmitry Dolgov
> On 12 August 2017 at 13:37, Dmitry Dolgov <[hidden email]> wrote:
>
> Here is a new rebased version of this patch (there were some conflicts in commentaries).

To make a review little bit easier I've divided the patch into a few smaller parts.



--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

0001-Base-implementation-of-subscripting-mechanism.patch (219K) Download Attachment
0002-Subscripting-for-arrays.patch (17K) Download Attachment
0003-Subscripting-for-jsonb.patch (45K) Download Attachment
0004-Subscripting-documentation.patch (24K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Dmitry Dolgov
On 29 August 2017 at 22:42, Dmitry Dolgov <[hidden email]> wrote:
>
> To make a review little bit easier I've divided the patch into a few smaller parts.

Apparently I forgot about subscripting for the name data type, so here is a small update of the patch.



--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

0001-Base-implementation-of-subscripting-mechanism.patch (219K) Download Attachment
0002-Subscripting-for-arrays.patch (17K) Download Attachment
0003-Subscripting-for-jsonb.patch (45K) Download Attachment
0004-Subscripting-documentation.patch (24K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

a.zakirov
On Thu, Sep 07, 2017 at 10:49:54PM +0200, Dmitry Dolgov wrote:
> On 29 August 2017 at 22:42, Dmitry Dolgov <[hidden email]> wrote:
> >
> > To make a review little bit easier I've divided the patch into a few
> smaller parts.
>
> Apparently I forgot about subscripting for the name data type, so here is a
> small update of the patch.

Thank you for rebasing the patch!

PostgreSQL and documentation with the patch compiles without any errors. All regression tests passed.

But honestly I still cannot say that I agree with *_extract() and *_assign() functions creation way. For example, there is no entry in pg_depend for them (related with pg_type entry).

Because there is no such entry, there is the following bug:

1 - make and install src/tutorial
2 - run src/tutorial/subscripting.sql
3 - run:

=# drop function custom_subscripting_extract(internal);

4 - and we get the error:

=# select data[0] from test_subscripting;
ERROR:  function 0x55deb7911bfd returned NULL

But of course it is only my opinion and I could be wrong.

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Dmitry Dolgov
> On 9 September 2017 at 23:33, Arthur Zakirov <[hidden email]> wrote:
> PostgreSQL and documentation with the patch compiles without any errors. All
> regression tests passed.

Thank you!

> But honestly I still cannot say that I agree with *_extract() and *_assign()
> functions creation way. For example, there is no entry in pg_depend for them
> ...
> =# drop function custom_subscripting_extract(internal);
> =# select data[0] from test_subscripting;
> ERROR:  function 0x55deb7911bfd returned NULL

Hm...I never thought about the feature in this way. When I was experimenting I
also tried another approach for this - save to `refevalfunc` a function
pointer to an appropriate function. For simple situations it was ok, but there
were questions about how it would work with node-related functions from
`outfuncs`/`copyfuncs` etc. Another my idea was to find out an actual
`refevalfunc` not at the time of a node creation but later on - this was also
questionable since in this case we need to carry a lot of information with a node
just for this sole purpose. Maybe you can suggest something else?

About dependencies between functions - as far as I understand one cannot create
a `pg_depend` entry or any other kind of dependencies between custom
user-defined functions. So yes, looks like with the current approach the only
solution would be to check in the `_parse` function that `_extract` and
`_assign` functions are existed (which is inconvenient).

> For example, there is no entry in pg_depend

Are there any other disadvantages of this approach?
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Tom Lane-2
Dmitry Dolgov <[hidden email]> writes:
> About dependencies between functions - as far as I understand one cannot
> create a `pg_depend` entry or any other kind of dependencies between
> custom user-defined functions.

Uh, what?  Sure you can.  Just because the existing code never has a
reason to create such a dependency doesn't mean it wouldn't work.

                        regards, tom lane


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Dmitry Dolgov
> On 11 September 2017 at 23:19, Tom Lane <[hidden email]> wrote:
>
> Uh, what?  Sure you can.  Just because the existing code never has a
> reason to create such a dependency doesn't mean it wouldn't work.

Well, I thought that `pg_depend` was not intended to be used from user-defined
code and it's something "internal". But if I'm wrong then maybe the problem
Arhur raised is a valid reason for that.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Tom Lane-2
Dmitry Dolgov <[hidden email]> writes:
>> On 11 September 2017 at 23:19, Tom Lane <[hidden email]> wrote:
>> Uh, what?  Sure you can.  Just because the existing code never has a
>> reason to create such a dependency doesn't mean it wouldn't work.

> Well, I thought that `pg_depend` was not intended to be used from
> user-defined code and it's something "internal".

Well, no, we're not expecting that SQL code will manually insert rows
there.  This feature should have some sort of SQL command that will
set up the relevant catalog entries, including the dependencies.
If you don't want to do that, you're going to need the runtime tests.

                        regards, tom lane


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Dmitry Dolgov
> On 11 September 2017 at 23:45, Tom Lane <[hidden email]> wrote:
>
> Dmitry Dolgov <[hidden email]> writes:
> >> On 11 September 2017 at 23:19, Tom Lane <[hidden email]> wrote:
> >> Uh, what?  Sure you can.  Just because the existing code never has a
> >> reason to create such a dependency doesn't mean it wouldn't work.
>
> > Well, I thought that `pg_depend` was not intended to be used from
> > user-defined code and it's something "internal".
>
> Well, no, we're not expecting that SQL code will manually insert rows
> there.  This feature should have some sort of SQL command that will
> set up the relevant catalog entries, including the dependencies.
> If you don't want to do that, you're going to need the runtime tests.

Sure, an SQL command for that purpose is much better than a runtime check.
I'm going to add such command to the patch, thank you for the information!
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Dmitry Dolgov
> On 11 September 2017 at 23:55, Dmitry Dolgov <[hidden email]> wrote:
>
> Sure, an SQL command for that purpose is much better than a runtime check.
> I'm going to add such command to the patch, thank you for the information!

So, I've implemented a patch for that in form of a `DEPENDS ON` syntax for creating a function.
Basically it looks like this (and initially I was looking for something like that in the documentation,
you can find a complete example in the test `create_function_3.sql`):

```
CREATE FUNCTION custom_subscripting_extract(internal)
   RETURNS internal;

CREATE FUNCTION custom_subscripting_assign(internal)
   RETURNS internal;

CREATE FUNCTION custom_subscript_parse(internal)
   RETURNS internal
   DEPENDS ON custom_subscripting_extract, custom_subscripting_assign;
```

I hope it sounds reasonable and can help to address a problem with dependencies between functions.



--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

0001-Base-implementation-of-subscripting-mechanism.patch (219K) Download Attachment
0002-Subscripting-for-arrays.patch (17K) Download Attachment
0003-Subscripting-for-jsonb.patch (45K) Download Attachment
0004-Subscripting-documentation.patch (24K) Download Attachment
0005-Function-depends-on.patch (16K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

a.zakirov
On Fri, Sep 15, 2017 at 10:02:00PM +0200, Dmitry Dolgov wrote:
>
> So, I've implemented a patch for that in form of a `DEPENDS ON` syntax for
> creating a function.

In my opinion, 'DEPENDS ON' syntax is not actually appropriate here. It
also looks like a not very good hack to me.

Moreover user can implement subscripting to its own type without using
'DEPENDS ON' syntax. And he will face the bug mentioned above too.

--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Dmitry Dolgov
> On 17 September 2017 at 00:04, Arthur Zakirov <[hidden email]> wrote:
>
> In my opinion, 'DEPENDS ON' syntax is not actually appropriate here. It
> also looks like a not very good hack to me.

Hm...why do you think about it as a hack?

> Moreover user can implement subscripting to its own type without using
> 'DEPENDS ON' syntax. And he will face the bug mentioned above too.

Yes, but since it will require from a user to create few independent custom
functions for subscripting (as we discussed before, there were few reasons of
having them as a proper separate function), I don't see how to avoid this step
of explicitly marking all of them as related to a subscripting logic for
particular data type. And therefore it's possible to forget to do that step in
spite of what form this step will be. Maybe it's possible to make something
like `CREATE FUNCTION ... FOR SUBSCRIPTING`, then verify that assign/extract
functions are presented and notify user if he missed them (but I would rather
not do this unless it's really necessary, since it looks like an overkill).

But I'm open to any suggestions, do you have something in mind?
12345 ... 8