[PATCH] Generic type subscripting

classic Classic list List threaded Threaded
129 messages Options
1234567
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

a.zakirov
On Sun, Sep 17, 2017 at 12:27:58AM +0200, Dmitry Dolgov wrote:
> 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?

I have put some thought into it. What about the following syntax?

CREATE SUBSCRIPTING FOR type_name
  INITFUNC = subscripting_init_func
  FETCHFUNC = subscripting_fetch_func
  ASSIGNFUNC = subscripting_assign_func
DROP SUBSCRIPTING FOR type_name

But I am not if the community will like such syntax.

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

Tom Lane-2
Arthur Zakirov <[hidden email]> writes:
> CREATE SUBSCRIPTING FOR type_name
>   INITFUNC = subscripting_init_func
>   FETCHFUNC = subscripting_fetch_func
>   ASSIGNFUNC = subscripting_assign_func
> DROP SUBSCRIPTING FOR type_name

Reasonable, but let's make the syntax more like other similar
utility commands such as CREATE AGGREGATE --- basically just
adding some parens, IIRC.

                        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
In reply to this post by a.zakirov
> On 17 September 2017 at 23:34, Arthur Zakirov <[hidden email]> wrote:
>
> I have put some thought into it. What about the following syntax?
>
> CREATE SUBSCRIPTING FOR type_name
>   INITFUNC = subscripting_init_func
>   FETCHFUNC = subscripting_fetch_func
>   ASSIGNFUNC = subscripting_assign_func
> DROP SUBSCRIPTING FOR type_name

Just to clarify, do you mean that `CREATE SUBSCRIPTING FOR` would only make a
dependency record? In this case `DROP SUBSCRIPTING FOR` actually means just
drop an init function.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

a.zakirov
On Mon, Sep 18, 2017 at 10:31:54AM +0200, Dmitry Dolgov wrote:
> Just to clarify, do you mean that `CREATE SUBSCRIPTING FOR` would only make
> a
> dependency record? In this case `DROP SUBSCRIPTING FOR` actually means just
> drop an init function.

I think it would be good to add new catalog table. It may be named as pg_type_sbs or pg_subscripting (second is better I think).
This table may have the fields:
- oid
- sbstype
- sbsinit
- sbsfetch
- sbsassign

And command 'CREATE SUBSCRIPTING' should add an entry to the pg_subscripting table. It also adds entries to the pg_depend table: dependency between pg_type and pg_subscripting, dependency between pg_type and pg_proc.
'DROP SUBSCRIPTING' should drop this entries, it should not drop init function.

According to the Tom's comment the syntax can be modified in the following way:

CREATE SUBSCRIPTING FOR type_namei (
  INITFUNC = subscripting_init_func
  FETCHFUNC = subscripting_fetch_func
  ASSIGNFUNC = subscripting_assign_func
)
DROP SUBSCRIPTING FOR type_name

--
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 18 September 2017 at 11:39, Arthur Zakirov <[hidden email]> wrote:
> I think it would be good to add new catalog table. It may be named as pg_type_sbs or pg_subscripting (second is better I think).
> This table may have the fields:
> - oid
> - sbstype
> - sbsinit
> - sbsfetch
> - sbsassign

What is `sbstype`?

> And command 'CREATE SUBSCRIPTING' should add an entry to the pg_subscripting table. It also adds entries to the pg_depend table: dependency between pg_type and pg_subscripting, dependency between pg_type and pg_proc.
> 'DROP SUBSCRIPTING' should drop this entries, it should not drop init function.

Why we should keep those subscripting functions? From my understanding they're
totally internal and useless without subscripting context.

Overall I have only one concern about this suggestion - basically it changes
nothing from the perspective of functionality or implementation quality. The
only purpose of it is to make a `subscripting` entity more explicit at the
expense of overhead of having one more catalog table and little bit more
complexity. I'm not really sure if it's necessary or not, and I would
appreciate any commentaries about that topic from the community (to make me
more convinced that this is a good decision or not).
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

a.zakirov
On Mon, Sep 18, 2017 at 12:25:04PM +0200, Dmitry Dolgov wrote:

> > I think it would be good to add new catalog table. It may be named as
> pg_type_sbs or pg_subscripting (second is better I think).
> > This table may have the fields:
> > - oid
> > - sbstype
> > - sbsinit
> > - sbsfetch
> > - sbsassign
>
> What is `sbstype`?

'sbstype' is oid of type from pg_type for which subscripting is created. I.e. pg_type may not have the 'typsubsparse' field.

> > And command 'CREATE SUBSCRIPTING' should add an entry to the
> pg_subscripting table. It also adds entries to the pg_depend table:
> dependency between pg_type and pg_subscripting, dependency between pg_type
> and pg_proc.
> > 'DROP SUBSCRIPTING' should drop this entries, it should not drop init
> function.
>
> Why we should keep those subscripting functions? From my understanding
> they're
> totally internal and useless without subscripting context.
>

Other similar commands do not drop related functions. For example 'DROP CAST' do not drop a function used to perform a cast.

> It also adds entries to the pg_depend table: dependency between pg_type and pg_subscripting, dependency between pg_type and pg_proc.

Here was a typo from me. Last entry is dependency between pg_subscripting and pg_proc.

--
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 19 September 2017 at 10:21, Arthur Zakirov <[hidden email]> wrote:
> On Mon, Sep 18, 2017 at 12:25:04PM +0200, Dmitry Dolgov wrote:
>> > I think it would be good to add new catalog table. It may be named as
>> pg_type_sbs or pg_subscripting (second is better I think).
>> > This table may have the fields:
>> > - oid
>> > - sbstype
>> > - sbsinit
>> > - sbsfetch
>> > - sbsassign
>>
>> What is `sbstype`?
>
>'sbstype' is oid of type from pg_type for which subscripting is created. I.e. pg_type may not have the 'typsubsparse' field.

I'm confused, why do we need it? I mean, isn't it enough to have a subscripting
oid in a pg_type record?

> On 18 September 2017 at 12:25, Dmitry Dolgov <[hidden email]> wrote:
>
> Overall I have only one concern about this suggestion - basically it changes
> nothing from the perspective of functionality or implementation quality.

Few more thoughts about this point. Basically if we're going this way (i.e.
having `pg_subscripting`) there will be one possible change of functionality -
in this case since we store oids of all the required functions, we can pass
them to a `parse` function (so that a custom extension does not need to resolve
them every time).

At the same time there are consequences of storing `pg_subscripting`, e.g.:

* I assume the performance would be worse because we have to do more actions to
  actually call a proper function.

* The implementation itself will be little bit more complex I think.

* Should we think about other functionality besides `CREATE` and `DROP`, for
  example `ALTER` (as far as I see aggregations support that).

and maybe something else that I don't see now.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Peter Eisentraut-6
In reply to this post by a.zakirov
On 9/18/17 05:39, Arthur Zakirov wrote:
> On Mon, Sep 18, 2017 at 10:31:54AM +0200, Dmitry Dolgov wrote:
>> Just to clarify, do you mean that `CREATE SUBSCRIPTING FOR` would only make
>> a
>> dependency record? In this case `DROP SUBSCRIPTING FOR` actually means just
>> drop an init function.
> I think it would be good to add new catalog table.

Would you mind posting a summary of how you go here?

Superficially, it seems like this sort of feature should be handled by a
couple of type attributes and pg_type columns.  But you are talking
about a new system catalog and new DDL, and it's not clear to me how you
got here.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

a.zakirov
On Tue, Sep 19, 2017 at 09:01:57PM -0400, Peter Eisentraut wrote:
> Would you mind posting a summary of how you go here?

There were several points here to me:
- it is necessary to solve the dependency problem (it can be solved also by adding several oid fields to the pg_type)
- users may want to add subscripting to their existing type, which already created in their database, or drop subscripting from existing type (it cannot be done by CREATE TYPE)
- other type related functionalities have their CREATE command and system catalog table. For example, CREATE CAST, CREATE TRANSFORM (this is a weakest point I think, mostly because of several casts and transforms can be defined to one type, and only one subscripting can be defined to one type).

> Superficially, it seems like this sort of feature should be handled by a
> couple of type attributes and pg_type columns.  But you are talking
> about a new system catalog and new DDL, and it's not clear to me how you
> got here.
>
> --
> Peter Eisentraut              http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

You are right of course. It can be handled by oid columns of *_parse, *_extract and *_assign functions. Also it is clear to me now that the second point can be handled by ALTER TYPE command. The command should be modified to handle it of course. And it can be modified by a separate patch later.

I want to emphasize that I don't insist on CREATE SUBSCRIPTING command. The patch brings important functionality and I don't want to be a person who blocked it from commiting.

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

Peter Eisentraut-6
On 9/20/17 04:12, Arthur Zakirov wrote:
> On Tue, Sep 19, 2017 at 09:01:57PM -0400, Peter Eisentraut wrote:
>> Would you mind posting a summary of how you go here?
>
> There were several points here to me:
> - it is necessary to solve the dependency problem (it can be solved also by adding several oid fields to the pg_type)

A few oid or regproc fields in pg_type seem sensible.

> - users may want to add subscripting to their existing type, which already created in their database, or drop subscripting from existing type (it cannot be done by CREATE TYPE)

That's what ALTER TYPE is for.

> - other type related functionalities have their CREATE command and system catalog table. For example, CREATE CAST, CREATE TRANSFORM (this is a weakest point I think, mostly because of several casts and transforms can be defined to one type, and only one subscripting can be defined to one type).

The difference is that those create associations between two separate
objects (cast: type1 <-> type2, transform: type <-> language).  A
subscripting is just a property of a type.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

a.zakirov
On Wed, Sep 20, 2017 at 09:35:06AM -0400, Peter Eisentraut wrote:
>
> The difference is that those create associations between two separate
> objects (cast: type1 <-> type2, transform: type <-> language).  A
> subscripting is just a property of a type.
>
> --
> Peter Eisentraut              http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Yes, indeed. I agree.

As a conclusion:
- additional field are needed to pg_type for *_fetch and *_assign functions to solve dependency problem
- ALTER TYPE requires a modification to add or drop subscripting from existing type (I am not sure that it should be done by this patch, maybe better to do it by a separate 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 20 September 2017 at 17:19, Arthur Zakirov <[hidden email]> wrote:
> As a conclusion:
> * additional field are needed to pg_type for *_fetch and *_assign functions to solve dependency problem

One last thing that I need to clarify. Initially there was an idea to minimize
changes in `pg_type` - that's why I added only one column there that contains an
OID of main subscripting function (and everything else you should find out
inside it). But I have no objections about adding more columns if everyone is
ok with that. Basically pros and cons (marked as + and -):

one new column in `pg_type`:

* less intrusive (+)
* it's neccessary to make a dependency record between subscripting functions
  explicitly (-)

three new columns in `pg_type`:

* more intrusive (-)
* we can create a dependency record between subscripting functions
  simultaneously with a custom type creation (+)
* custom subscripting code does not need to resolve `fetch` and `assign`
  functions (+)
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Peter Eisentraut-6
On 9/21/17 11:24, Dmitry Dolgov wrote:
> One last thing that I need to clarify. Initially there was an idea to
> minimize changes in `pg_type`

I see, but there is no value in that if it makes everything else more
complicated.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


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

Oleg Bartunov-2
On Fri, Sep 22, 2017 at 3:51 PM, Peter Eisentraut
<[hidden email]> wrote:
> On 9/21/17 11:24, Dmitry Dolgov wrote:
>> One last thing that I need to clarify. Initially there was an idea to
>> minimize changes in `pg_type`
>
> I see, but there is no value in that if it makes everything else more
> complicated.

+1



>
> --
> Peter Eisentraut              http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>
> --
> Sent via pgsql-hackers mailing list ([hidden email])
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


--
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 Fri, Sep 22, 2017 at 3:51 PM, Peter Eisentraut <[hidden email]> wrote:
> On 9/21/17 11:24, Dmitry Dolgov wrote:
>> One last thing that I need to clarify. Initially there was an idea to
>> minimize changes in `pg_type`
>
> I see, but there is no value in that if it makes everything else more
> complicated.

I'm still working on that, but obviously I'll not manage to finish it within this CF,
so I'm going to move it to the next one.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Dmitry Dolgov
> On 30 September 2017 at 22:13, Dmitry Dolgov <[hidden email]> wrote:
> I'm still working on that, but obviously I'll not manage to finish it within
> this CF, so I'm going to move it to the next one.

So, here is the new version of patch that contains modifications we've
discussed, namely:

* store oids of `parse`, `fetch` and `assign` functions

* introduce dependencies from a data type

* as a side effect of previous two I also eliminated some unnecessary arguments
  in `parse` function.

I'm going to make few more improvements, but in the meantime I hope we can
continue to review 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 (230K) Download Attachment
0002-Subscripting-for-arrays.patch (16K) Download Attachment
0003-Subscripting-for-jsonb.patch (44K) 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 Sun, Oct 29, 2017 at 10:56:19PM +0100, Dmitry Dolgov wrote:

>
> So, here is the new version of patch that contains modifications we've
> discussed, namely:
>
> * store oids of `parse`, `fetch` and `assign` functions
>
> * introduce dependencies from a data type
>
> * as a side effect of previous two I also eliminated some unnecessary
> arguments
>   in `parse` function.

Thank you for new version of the patch.

There are some notes.

Documentation
-------------

Documentation is compiled. But there are warnings about end-tags. Now it is necessary to have full named end-tags:

=# make -C doc/src/sgml check
/usr/sbin/onsgmls:json.sgml:574:20:W: empty end-tag
...

Documentation is out of date:
- catalogs.sgml needs to add information about additional pg_type fields
- create_type.sgml needs information about subscripting_parse, subscripting_assign and subscripting_fetch options
- xsubscripting.sgml is out of date

Code
----

I think it is necessary to check Oids of subscripting_parse, subscripting_assign, subscripting_fetch. Maybe within TypeCreate().

Otherwise next cases possible:

=# CREATE TYPE custom (
   internallength = 8,
   input = custom_in,
   output = custom_out,
   subscripting_parse = custom_subscripting_parse);
=# CREATE TYPE custom (
   internallength = 8,
   input = custom_in,
   output = custom_out,
   subscripting_fetch = custom_subscripting_fetch);

Are all subscripting_* fields mandatory? If so if user provided at least one of them then all fields should be provided.

Should all types have support assigning via subscript? If not then subscripting_assign parameter is optional.

> +Datum
> +jsonb_subscript_parse(PG_FUNCTION_ARGS)
> +{
> + bool isAssignment = PG_GETARG_BOOL(0);

and

> +Datum
> +custom_subscripting_parse(PG_FUNCTION_ARGS)
> +{
> + bool isAssignment = PG_GETARG_BOOL(0);

Here isAssignment is unused variable, so it could be removed.

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

As I mentioned earlier we need assigning eval_finfo and nested_finfo only for EEOP_SBSREF_OLD, EEOP_SBSREF_ASSIGN and EEOP_SBSREF_FETCH steps.
Also they should be assigned before calling ExprEvalPushStep(), not after. Otherwise some bugs may appear in future.

> - ArrayRef   *aref = makeNode(ArrayRef);
> + NodeTag sbstag = nodeTag(src_expr);
> + Size nodeSize = sizeof(SubscriptingRef);
> + SubscriptingRef *sbsref = (SubscriptingRef *) newNode(nodeSize, sbstag);

Is there necessity to use newNode() instead using makeNode(). The previous code was shorter.

There is no changes in execnodes.h except removed line. So I think execnodes.h could be removed from the patch.

>
> I'm going to make few more improvements, but in the meantime I hope we can
> continue to review the patch.

I will wait.

--
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 31 October 2017 at 16:05, Arthur Zakirov <[hidden email]> wrote:
> Documentation is compiled. But there are warnings about end-tags. Now it is necessary to have full named end-tags

Fixed, thanks for noticing.

> I think it is necessary to check Oids of subscripting_parse, subscripting_assign, subscripting_fetch. Maybe within TypeCreate().

Yes, I agree. I implemented it in a way that all subscripting-related functions
must be provided if `subscripting_parse` is there - in this case if you want to
prevent assign or fetch, you can just do it inside a corresponding function and
it allows to provide a custom message about that.

> > +Datum
> > +custom_subscripting_parse(PG_FUNCTION_ARGS)
> > +{
> > +     bool                            isAssignment = PG_GETARG_BOOL(0);
>
> Here isAssignment is unused variable, so it could be removed.

In this case I disagree - the purpose of these examples is to show everything
you can use. So I just need to come up with some example that involves
`isAssignment`.

> > +     scratch->d.sbsref.eval_finfo = eval_finfo;
> > +     scratch->d.sbsref.nested_finfo = nested_finfo;
> > +
> As I mentioned earlier we need assigning eval_finfo and nested_finfo only for EEOP_SBSREF_OLD, EEOP_SBSREF_ASSIGN and EEOP_SBSREF_FETCH steps.
> Also they should be assigned before calling ExprEvalPushStep(), not after. Otherwise some bugs may appear in future.

I'm really confused about this one. Can you tell me the exact line numbers?
Because if I remove any of these lines "blindly", tests are failing.

> > -             ArrayRef   *aref = makeNode(ArrayRef);
> > +             NodeTag sbstag = nodeTag(src_expr);
> > +             Size nodeSize = sizeof(SubscriptingRef);
> > +             SubscriptingRef *sbsref = (SubscriptingRef *) newNode(nodeSize, sbstag);
>
> Is there necessity to use newNode() instead using makeNode(). The previous code was shorter.

Good catch! It was a leftover from the version when I had two different nodes
for subscripting.

> There is no changes in execnodes.h except removed line. So I think execnodes.h could be removed from the patch.

Fixed.


--
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 (231K) Download Attachment
0002-Subscripting-for-array.patch (16K) Download Attachment
0003-Subscripting-for-jsonb.patch (44K) Download Attachment
0004-Subscripting-documentation.patch (28K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

a.zakirov
Thank you for fixing.

On Tue, Nov 07, 2017 at 09:00:43PM +0100, Dmitry Dolgov wrote:

> > > +Datum
> > > +custom_subscripting_parse(PG_FUNCTION_ARGS)
> > > +{
> > > +     bool                            isAssignment = PG_GETARG_BOOL(0);
> >
> > Here isAssignment is unused variable, so it could be removed.
>
> In this case I disagree - the purpose of these examples is to show
> everything
> you can use. So I just need to come up with some example that involves
> `isAssignment`.
Understood.

>
> > > +     scratch->d.sbsref.eval_finfo = eval_finfo;
> > > +     scratch->d.sbsref.nested_finfo = nested_finfo;
> > > +
> > As I mentioned earlier we need assigning eval_finfo and nested_finfo only
> for EEOP_SBSREF_OLD, EEOP_SBSREF_ASSIGN and EEOP_SBSREF_FETCH steps.
> > Also they should be assigned before calling ExprEvalPushStep(), not
> after. Otherwise some bugs may appear in future.
>
> I'm really confused about this one. Can you tell me the exact line numbers?
> Because if I remove any of these lines "blindly", tests are failing.
Field scratch->d is union. Its fields should be changed only before calling ExprEvalPushStep(), which copies 'scratch'. To be more specific I attached the patch 0005-Fix-ExprEvalStep.patch, which can be applyed over your patches.

Some other notes are below.

>     <replaceable class="parameter">type_modifier_output_function</replaceable> and
> -   <replaceable class="parameter">analyze_function</replaceable>
> +   <replaceable class="parameter">analyze_function</replaceable>,
> +   <replaceable class="parameter">subscripting_parse_function</replaceable>
> +   <replaceable class="parameter">subscripting_assign_function</replaceable>
> +   <replaceable class="parameter">subscripting_fetch_function</replaceable>

I think you forgot commas and conjunction 'and'.

> +   The optional
> +   <replaceable class="parameter">subscripting_parse_function</replaceable>,
> +   <replaceable class="parameter">subscripting_assign_function</replaceable>
> +   <replaceable class="parameter">subscripting_fetch_function</replaceable>
> +   contains type-specific logic for subscripting of the data type.

Here you forgot comma or 'and'. Also 'contain' should be used instead 'contains'.

It seems that in the following you switched descriptions:

> +    <term><replaceable class="parameter">subscripting_assign_function</replaceable></term>
> +    <listitem>
> +     <para>
> +      The name of a function that contains type-specific subscripting logic for
> +      fetching an element from the data type.
> +     </para>

subscripting_assign_function is used for updating.

> +    <term><replaceable class="parameter">subscripting_fetch_function</replaceable></term>
> +    <listitem>
> +     <para>
> +      The name of a function that contains type-specific subscripting logic for
> +      updating an element in the data type.
> +     </para>

subscripting_fetch_function is used for fetching.

I have a little complain about how ExprEvalStep gets resvalue. resvalue is assigned in one place (within ExecEvalSubscriptingRefFetch(), ExecEvalSubscriptingRefAssign()), resnull is assigned in another place (within jsonb_subscript_fetch(), jsonb_subscript_assign()). I'm not sure that it is a good idea, but it is not critical, it is just complaint.

After your fixing I think we should wait for opinion of senior community members and mark the patch as 'Ready for Commiter'. Maybe I will do more tests and try to implement subscripting to another type.

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

0005-Fix-ExprEvalStep.patch (2K) Download Attachment
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 8 November 2017 at 17:25, Arthur Zakirov <[hidden email]> wrote:

Thanks for your review!

> > On Tue, Nov 07, 2017 at 09:00:43PM +0100, Dmitry Dolgov wrote:
> > > +Datum
> > > +custom_subscripting_parse(PG_FUNCTION_ARGS)
> > > +{
> > > +     bool                            isAssignment = PG_GETARG_BOOL(0);
> >
> > Here isAssignment is unused variable, so it could be removed.
>
> In this case I disagree - the purpose of these examples is to show everything
> you can use. So I just need to come up with some example that involves
> `isAssignment`.

I've incorporated this variable into the tutorial.

> To be more specific I attached the patch 0005-Fix-ExprEvalStep.patch, which can be applyed over your patches.

Oh, now I see, thank you.

> I think you forgot commas and conjunction 'and'.
> Here you forgot comma or 'and'. Also 'contain' should be used instead 'contains'.
> It seems that in the following you switched descriptions:

Shame on me :) Fixed.

> I have a little complain about how ExprEvalStep gets resvalue. resvalue is
> assigned in one place (within ExecEvalSubscriptingRefFetch(),
> ExecEvalSubscriptingRefAssign()), resnull is assigned in another place
> (within jsonb_subscript_fetch(), jsonb_subscript_assign()).

Hm...I'm afraid I don't get this. `resnull` is never assigned inside
`jsonb_subscript_fetch` or `jsonb_subscript_assign`, instead it's coming
from `ExecInterpExp` as `isnull` if I remember correctly. Are we talking about
the same thing?

In this version of the patch I also improved NULL handling, you can see it in
the tests.


--
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 (231K) Download Attachment
0002-Subscripting-for-array.patch (17K) Download Attachment
0003-Subscripting-for-jsonb.patch (44K) Download Attachment
0004-Subscripting-documentation.patch (29K) Download Attachment
1234567