Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur 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)
> > > +{
> > > + boolisAssignment = 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.

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

I think you forgot commas and conjunction 'and'.

> +   The optional
> +   subscripting_parse_function,
> +   subscripting_assign_function
> +   subscripting_fetch_function
> +   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:

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

subscripting_assign_function is used for updating.

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

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
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index c275387319..42a0d31c31 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -2350,9 +2350,6 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *aref, PlanState
 		fmgr_info(aref->refnestedfunc, nested_finfo);
 	}
 
-	scratch->d.sbsref.eval_finfo = eval_finfo;
-	scratch->d.sbsref.nested_finfo = nested_finfo;
-
 	/* Fill constant fields of SubscriptingRefState */
 	arefstate->isassignment = isAssignment;
 	arefstate->refelemtype = aref->refelemtype;
@@ -2377,9 +2374,6 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *aref, PlanState
 		scratch->d.jump.jumpdone = -1;	/* adjust later */
 		ExprEvalPushStep(state, scratch);
 
-		scratch->d.sbsref.eval_finfo = eval_finfo;
-		scratch->d.sbsref.nested_finfo = nested_finfo;
-
 		adjust_jumps = lappend_int(adjust_jumps,
    state->steps_len - 1);
 	}
@@ -2425,9 +2419,6 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *aref, PlanState
 		scratch->d.sbsref_subscript.jumpdone = -1;	/* adjust later */
 		ExprEvalPushStep(state, scratch);
 
-		scratch->d.sbsref.eval_finfo = eval_finfo;
-		scratch->d.sbsref.nested_finfo = nested_finfo;
-
 		adjust_jumps = lappend_int(adjust_jumps,
    state->steps_len - 1);
 		i++;
@@ -2462,9 +2453,6 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *aref, PlanState
 		scratch->d.sbsref_subscript.jumpdone = -1;	/* adjust later */
 		ExprEvalPushStep(state, scratch);
 
-		scratch->d.sbsref.eval_finfo = eval_finfo;
-		scratch->d.sbsref.nested_finfo = nested_finfo;
-
 		adjust_jumps = lappend_int(adjust_jumps,
    state->steps_len - 1);
 		i++;
@@ -2499,10 +2487,9 @@ ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *aref, PlanState
 		{
 			scratch->opcode = EEOP_SBSREF_OLD;
 			

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur 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)
> +{
> + boolisAssignment = PG_GETARG_BOOL(0);

and

> +Datum
> +custom_subscripting_parse(PG_FUNCTION_ARGS)
> +{
> + boolisAssignment = 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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Dmitry Dolgov]

> On Fri, Sep 22, 2017 at 3:51 PM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> 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.

Re: [HACKERS] [PATCH] Generic type subscripting

[Oleg Bartunov]

On Fri, Sep 22, 2017 at 3:51 PM, Peter Eisentraut
 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 (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


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

Re: [HACKERS] [PATCH] Generic type subscripting

[Peter Eisentraut]

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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Dmitry Dolgov]

> On 20 September 2017 at 17:19, Arthur Zakirov 
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 (+)

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur 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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Peter Eisentraut]

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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur 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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Peter Eisentraut]

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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Dmitry Dolgov]

> On 19 September 2017 at 10:21, Arthur Zakirov 
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 <9erthali...@gmail.com>
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.

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur 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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Dmitry Dolgov]

> On 18 September 2017 at 11:39, Arthur Zakirov 
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).

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur 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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Dmitry Dolgov]

> On 17 September 2017 at 23:34, Arthur Zakirov 
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.

Re: [HACKERS] [PATCH] Generic type subscripting

[Tom Lane]

Arthur Zakirov  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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur 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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Dmitry Dolgov]

> On 17 September 2017 at 00:04, Arthur Zakirov 
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?

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur 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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Dmitry Dolgov]

> On 11 September 2017 at 23:45, Tom Lane  wrote:
>
> Dmitry Dolgov <9erthali...@gmail.com> writes:
> >> On 11 September 2017 at 23:19, Tom Lane  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!

Re: [HACKERS] [PATCH] Generic type subscripting

[Tom Lane]

Dmitry Dolgov <9erthali...@gmail.com> writes:
>> On 11 September 2017 at 23:19, Tom Lane  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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Dmitry Dolgov]

> On 11 September 2017 at 23:19, Tom Lane  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.

Re: [HACKERS] [PATCH] Generic type subscripting

[Tom Lane]

Dmitry Dolgov <9erthali...@gmail.com> 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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Dmitry Dolgov]

> On 9 September 2017 at 23:33, Arthur Zakirov 
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?

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur Zakirov]

On Thu, Sep 07, 2017 at 10:49:54PM +0200, Dmitry Dolgov wrote:
> On 29 August 2017 at 22:42, Dmitry Dolgov <9erthali...@gmail.com> 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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur 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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur 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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur 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(_type, _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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur 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 (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur Zakirov]

2017-03-28 19:31 GMT+03:00 Dmitry Dolgov <9erthali...@gmail.com>:
> On 28 March 2017 at 12:08, Dmitry Dolgov <9erthali...@gmail.com> wrote:
>>
>> Wow, I didn't notice that, sorry - will fix it shortly.
>
> So, here is the corrected version of the patch.

Thank you!

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.

But, from my point of view, it would be nice if the code mentioned
earlier was improved:

> + foreach(l, sbsref->reflowerindexpr)
> + {
> + List *expr_ai = (List *) lfirst(l);
> + A_Indices *ai = (A_Indices *) lfirst(list_tail(expr_ai));
> +
> + subexpr = (Node *) lfirst(list_head(expr_ai));

It is necessary for arrays of course because of logic mentioned in the
documentation.

> If any dimension is written as a slice, i.e., contains a colon, then all 
> dimensions are treated as slices. Any dimension that has only a single number 
> (no colon) is treated as being from 1 to the number specified.

But it would be better if SubscriptingRef structure had a new field of
List type. This field can store list of booleans, which means is there
slices or not. I think it can improve readability of the code.

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


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

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur Zakirov]

2017-03-31 5:32 GMT+03:00 Dmitry Dolgov <9erthali...@gmail.com>:
> On 30 March 2017 at 19:36, Arthur Zakirov  wrote:
>>
>> The last point is about the tutorial. As Tom pointed it is not useful when
>> the tutorial doesn't execute. It happens because there is not "custom" type
>> in subscripting.sql.
>
> I'm confused. Maybe I'm missing something, but there is "custom" type in
> this file:

Sorry for confusing. I should have been more careful. I've mixed up
NOTICE message with error message and I haven't noticed CREATE TYPE
command.

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


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

Re: [HACKERS] [PATCH] Generic type subscripting

[Dmitry Dolgov]

On 30 March 2017 at 19:36, Arthur Zakirov  wrote:
>
> The last point is about the tutorial. As Tom pointed it is not useful
when the tutorial doesn't execute. It happens because there is not "custom"
type in subscripting.sql.

I'm confused. Maybe I'm missing something, but there is "custom" type in
this file:

```
-- subscripting.sql

CREATE TYPE custom (
   internallength = 8,
   input = custom_in,
   output = custom_out,
   subscripting = custom_subscript_parse
);
```

```
> \i subscripting.sql
psql:subscripting.sql:39: NOTICE:  42704: type "custom" is not yet defined
DETAIL:  Creating a shell type definition.
LOCATION:  compute_return_type, functioncmds.c:141
CREATE FUNCTION
Time: 4.257 ms
psql:subscripting.sql:47: NOTICE:  42809: argument type custom is only a
shell
LOCATION:  interpret_function_parameter_list, functioncmds.c:245
CREATE FUNCTION
Time: 37.038 ms
CREATE FUNCTION
Time: 13.891 ms
CREATE FUNCTION
Time: 0.946 ms
CREATE FUNCTION
Time: 1.161 ms
CREATE TYPE
Time: 1.336 ms
CREATE TABLE
Time: 2.129 ms
INSERT 0 1
Time: 2.501 ms
 data
--
2
(1 row)

Time: 0.960 ms
UPDATE 1
Time: 0.887 ms
```

So the only problem I see is notification about "type 'custom' is not yet
defined", but it's the same for "complex" tutorial

```
> \i complex.sql
psql:complex.sql:39: NOTICE:  42704: type "complex" is not yet defined
DETAIL:  Creating a shell type definition.
LOCATION:  compute_return_type, functioncmds.c:141
CREATE FUNCTION
Time: 1.741 ms
psql:complex.sql:47: NOTICE:  42809: argument type complex is only a shell
LOCATION:  interpret_function_parameter_list, functioncmds.c:245
CREATE FUNCTION
Time: 0.977 ms
psql:complex.sql:55: NOTICE:  42809: return type complex is only a shell
LOCATION:  compute_return_type, functioncmds.c:105
CREATE FUNCTION
Time: 0.975 ms
psql:complex.sql:63: NOTICE:  42809: argument type complex is only a shell
LOCATION:  interpret_function_parameter_list, functioncmds.c:245
CREATE FUNCTION
Time: 0.893 ms
CREATE TYPE
Time: 0.992 ms
...
```

Can you clarify this point?

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur Zakirov]

On 29.03.2017 20:14, Arthur Zakirov wrote:


I wanted to implement subscripting for ltree or hstore extensions.
Subscripting for ltree looks more interesting. Especially with slicing.
But I haven't done it yet. I hope that I will implement it tomorrow.



ltree
-

I've implemented fetching ltree elements using subscripting. But haven't 
implemented assigning ltree elements yet. I'll send second patch after 
implementing assigning.


Now you can execute the following query:

SELECT ('Top.Science.Astronomy.Astrophysics'::ltree)[1:2];
ltree
-
 Top.Science

Comments


But I've noticed about some points.

In array_subscript_parse() passed uninitialized values of "typesource" 
and "typeneeded" variables for coerce_to_target_type().



+   typesource = exprType(assignExpr);
+   typesource = is_slice ? sbsref->refcontainertype : 
sbsref->refelemtype;


Here is the bug. Second variable should be "typeneeded". Moreover these 
assignments should be moved up to first coerce_to_target_type() execution.



+   foreach(l, sbsref->reflowerindexpr)
+   {
+   List *expr_ai = (List *) lfirst(l);
+   A_Indices *ai = (A_Indices *) lfirst(list_tail(expr_ai));
+
+   subexpr = (Node *) lfirst(list_head(expr_ai));


This code looks like a magic. This happens because of appending 
A_Indeces to lowerIndexpr:



-   subexpr = NULL;
+   lowerIndexpr = lappend(lowerIndexpr, 
list_make2(subexpr, ai));
}


And this A_Indeces used only when slicing is not used to make a constant 
1. Maybe there are another way?


Also it would be better if "refevalfunc" and "refnestedfunc" had 
pointers to functions not Oid type. Now you need to create "..._fetch" 
and "..._assign" functions in catalog and in "..._parse" function you 
need get their Oid using to_regproc() function.


Can we use IndexAmRoutine structure method, when you use only pointers 
to necessary functions? You can see an example in blhandler() function 
in blutils.c.


The last point is about the tutorial. As Tom pointed it is not useful 
when the tutorial doesn't execute. It happens because there is not 
"custom" type in subscripting.sql. Also it contradicts the README of 
tutorials.


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


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

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur Zakirov]

On 28.03.2017 19:31, Dmitry Dolgov wrote:

On 28 March 2017 at 12:08, Dmitry Dolgov <9erthali...@gmail.com
> wrote:


Wow, I didn't notice that, sorry - will fix it shortly.


So, here is the corrected version of the patch.


I have some picky comments.

I'm not sure that "typsbsparse" is better than "typsubscripting" or 
"typsubparse". Maybe "typsubsparse"?



  
+  typsubscripting
+  regproc


Here you didn't fix "typsubscripting" to new name.


+  JSON subscripting
+  
+   JSONB data type support array-style subscripting expressions to extract or 
update particular element. An example of subscripting syntax:


Should be "JSONB data type supports".


+  to handle subscripting expressions. It should contains logic for verification
+  and decide which function must be used for evaluation of this expression.
+  For instance:


Should be "It should contain".


+ 
+  JSON subscripting
+  
+   JSONB data type support array-style subscripting expressions to extract or 
update particular element. An example of subscripting syntax:


You have implemented jsonb subscripting. The documentation should be 
fixed to:


+ 
+  jsonb Subscripting
+  
+   jsonb data type support array-style subscripting 
expressions to extract or update particular element. An example of 
subscripting syntax:


I think IsOneOf() macros should be removed. Since it is not used anywhere.


+   Assert(subexpr != NULL);
+
+   if (subexpr == NULL)
+   ereport(ERROR,
+   (errcode(ERRCODE_DATATYPE_MISMATCH),
+errmsg("jsonb subscript does not support 
slices"),
+parser_errposition(pstate, 
exprLocation(
+   ((Node *) 
lfirst(sbsref->refupperindexpr->head));


Here one of the conditions is excess. Better to delete assert condition 
I think.


I've tried tests from message [1]. They looks good. Performance looks 
similar for subscripting without patch and with patch.


I wanted to implement subscripting for ltree or hstore extensions. 
Subscripting for ltree looks more interesting. Especially with slicing. 
But I haven't done it yet. I hope that I will implement it tomorrow.


1. 
https://www.postgresql.org/message-id/CAKNkYnz_WWkzzxyFx934N%3DEp47CAFju-Rk-sGeZo0ui8QdrGmw%40mail.gmail.com


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


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

Re: [HACKERS] [PATCH] Generic type subscripting

[Dmitry Dolgov]

On 28 March 2017 at 11:58, Arthur Zakirov  wrote:
>
> Your patch reverts commits from 25-26 march. And therefore contains 15000
lines.

Wow, I didn't notice that, sorry - will fix it shortly.

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur Zakirov]

Hello,

On 27.03.2017 23:28, Dmitry Dolgov wrote:

Here is a new version of this patch. What was changed:

* I rebased code against the latest version of master and adapted recent
  changes about the expression execution

* Several names (functions and related pg_type column) were changed

* A new oid function field was introduced to handle nested assignment
situation

* I updated the documentation for patch

* `MAXDIM` was replaced by `MAX_SUBSCRIPT_DEPTH`

* I returned one `SubscriptingRef` for both fetch & assign operations, since
  there is no real readability improvements at this point (they're already
  separated at the time of evaluation, and besides the evaluation code
fetch &
  assign are handled almost identically).


Your patch reverts commits from 25-26 march. And therefore contains 
15000 lines.


I think the patch needs rebasing.

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


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

Re: [HACKERS] [PATCH] Generic type subscripting

[Arthur Zakirov]

On 24.03.2017 18:29, Tom Lane wrote:

David Steele  writes:

Do you have an idea when you will have a patch ready?  We are now into
the last week of the commitfest.  I see one question for Tom, but it's
not clear that this would prevent you from producing a new patch.


FWIW, I'm up to my eyeballs in Andres' faster-expressions patch, and
won't have time to think about this one for at least a couple more
days.

regards, tom lane




I can try to review a new version of the patch, when Dmitry will send 
it. If no one objects. Besides, I've seen the previous versions of the 
patch from previous commitfest.


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


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

Re: [HACKERS] [PATCH] Generic type subscripting

[Tom Lane]

David Steele  writes:
> Do you have an idea when you will have a patch ready?  We are now into 
> the last week of the commitfest.  I see one question for Tom, but it's 
> not clear that this would prevent you from producing a new patch.

FWIW, I'm up to my eyeballs in Andres' faster-expressions patch, and
won't have time to think about this one for at least a couple more
days.

regards, tom lane


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

Re: [HACKERS] [PATCH] Generic type subscripting

[Dmitry Dolgov]

On 24 March 2017 at 15:39, David Steele  wrote:
>
> Do you have an idea when you will have a patch ready?

Yes, I'll prepare a new version with most important changes in two days.

Re: [HACKERS] [PATCH] Generic type subscripting

[David Steele]

Hi Dmitry,

On 3/21/17 4:42 PM, Dmitry Dolgov wrote:

On 21 March 2017 at 18:16, David Steele > wrote:


This thread has been idle for over a week.


Yes, sorry for the late reply. I'm still trying to find a better
solution for
some of the problems, that arose in this patch.


On 15 March 2017 at 00:10, Tom Lane > wrote:

Dmitry Dolgov <9erthali...@gmail.com >

writes:


I looked through this a bit.


Do you have an idea when you will have a patch ready?  We are now into 
the last week of the commitfest.  I see one question for Tom, but it's 
not clear that this would prevent you from producing a new patch.


Please post a new patch by 2017-03-28 00:00 AoE (UTC-12) or this 
submission will be marked "Returned with Feedback".


Thanks,
--
-David
da...@pgmasters.net


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

Re: [HACKERS] [PATCH] Generic type subscripting

[Dmitry Dolgov]

> On 21 March 2017 at 18:16, David Steele  wrote:
>
> This thread has been idle for over a week.

Yes, sorry for the late reply. I'm still trying to find a better solution
for
some of the problems, that arose in this patch.

> On 15 March 2017 at 00:10, Tom Lane  wrote:
> Dmitry Dolgov <9erthali...@gmail.com> writes:
>
> I looked through this a bit.
>

Thank you for the feedback.

> > On 10 March 2017 at 06:20, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:
> > I see a possible problem here:  This design only allows one subscripting
> > function.  But what you'd really want in this case is at least two: one
> > taking an integer type for selecting by array index, and one taking text
> > for selecting by field name.
>
> I would guess that what we really want for jsonb is the ability to
> intermix integer and text subscripts, so that
>  jsonbcol['foo'][42]['bar']
> would extract the "bar" field of an object in position 42 of an
> array in field "foo" of the given jsonb value.
>

Maybe I misunderstood you, but isn't it already possible?

```
=# select ('{"a": [{"b": 1}, {"c": 2}]}'::jsonb)['a'][0]['b'];
 jsonb
---
 1
(1 row)

=# select * from test;
data
-
 {"a": [{"b": 1}, {"c": 2}]}
(1 row)

=# update test set data['a'][0]['b'] = 42;
UPDATE 1

=# select * from test;
data
-
 {"a": [{"b": 42}, {"c": 2}]}
(1 row)
```

> I'm not totally excited about the naming you've chosen though,
> particularly the function names "array_subscripting()" and
> "jsonb_subscripting()" --- those are too generic, and a person coming to
> them for the first time would probably expect that they actually execute
> subscripting, when they do no such thing.  Names like
> "array_subscript_parse()" might be better.  Likewise the name of the
> new pg_type column doesn't really convey what it does, though I suppose
> "typsubscriptparse" is too much of a mouthful.  "typsubparse" seems short
> enough but might be confusing too.

It looks quite tempting for me to replace the word "subscript" by an
abbreviation all over the patch. Then it will become something like
"typsbsparse" which is not that mouthful, but I'm not sure if it will be
easily
recognizable.

> I wonder also if we should try to provide some helper functions rather
> than expecting every data type to know all there is to know about parsing
> and execution of subscripting.  Not sure what would be helpful, however.

I don't really see what details we can hide behind this helper, because
almost
all code there is type specific (e.g. to check if subscript indexes are
correct), can you elaborate on that?

> The documentation needs a lot of work of course, and I do not think
> you're doing either yourself or anybody else any favors with the proposed
> additions to src/tutorial/.

Yes, unfortunately, I forget to update documentation from the previous
version
of the patch. I'll fix it soon in the next version.

> Aren't SBS_VALIDATION and SBS_EXEC just hangovers from the previous
> design?  They're still in parse_node.h, and they're still mentioned in
> the docs, but I don't see them used in actual code anywhere.

Yes, these are from the previous version too, I'll remove them.

> Another question here is whether every datatype will be on board
> with the current rules about null subscripts, or whether we need to
> delegate null-handling to the datatype-specific execution function.
> I'm not sure; it would complicate the API significantly for what might be
> useless flexibility.

It looks for me that it's a great idea to perform null-handling inside
datatype
specific code and I'm not sure, what would be complicated? All necessary
information for that is already in `SubscriptingExecData` (I just have to
use
`eisnull` in a different way).

> I do not think that the extra SubscriptingBase data structure is paying
> for itself either; you're taking a hit in readability from the extra level
> of struct, and as far as I can find it's not buying you one single thing,
> because there's no code that operates on a SubscriptingBase argument.
> I'd just drop that idea and make two independent struct types, or else
> stick with the original ArrayRef design that made one struct serve both
> purposes.  (IOW, my feeling that a separate assign struct would be a
> readability improvement isn't exactly getting borne out by what you've
> done here.  But maybe there's a better way to do that.)

I'm thinking to replace these structures by more meaningful ones, something
like:

```
typedef struct SubscriptContainerRef
{
Expr xpr;
Oid refcontainertype;
Oid refelemtype;
int32 reftypmod;
Oid refcollid;
} SubscriptBase;

typedef struct SubscriptExtractRef
{
Exprxpr;
SubscriptContainerRef *containerExpr;
List*refupperindexpr;
List*reflowerindexpr;
Oidrefevalfunc;
} SubscriptExtractRef;

typedef struct SubscriptAssignRef
{

Re: [HACKERS] [PATCH] Generic type subscripting

[David Steele]

Hi Dmitry,

On 3/14/17 7:10 PM, Tom Lane wrote:

Dmitry Dolgov <9erthali...@gmail.com> writes:

[ generic_type_subscription_v7.patch ]


I looked through this a bit.


This thread has been idle for over a week.  Please respond and/or post a 
new patch by 2017-03-24 00:00 AoE (UTC-12) or this submission will be 
marked "Returned with Feedback".


Thanks,
--
-David
da...@pgmasters.net


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

Re: [HACKERS] [PATCH] Generic type subscripting

[Tom Lane]

Dmitry Dolgov <9erthali...@gmail.com> writes:
> [ generic_type_subscription_v7.patch ]

I looked through this a bit.

I think that the basic design of having a type-specific parse analysis
function that returns a constructed SubscriptingRef node is fine.

I'm not totally excited about the naming you've chosen though,
particularly the function names "array_subscripting()" and
"jsonb_subscripting()" --- those are too generic, and a person coming to
them for the first time would probably expect that they actually execute
subscripting, when they do no such thing.  Names like
"array_subscript_parse()" might be better.  Likewise the name of the
new pg_type column doesn't really convey what it does, though I suppose
"typsubscriptparse" is too much of a mouthful.  "typsubparse" seems short
enough but might be confusing too.

I wonder also if we should try to provide some helper functions rather
than expecting every data type to know all there is to know about parsing
and execution of subscripting.  Not sure what would be helpful, however.

One thing that needs more thought for sure is the nested-assignment case
(the logic around isAssignmentIndirectionExpr) --- the design you've got
here implies that *every* container-like datatype would need to duplicate
that logic, and I don't think we want to go there.

The documentation needs a lot of work of course, and I do not think
you're doing either yourself or anybody else any favors with the proposed
additions to src/tutorial/.  You'll never be sure that that stuff even
compiles let alone accurately represents what people need to do.  Actual
running code is much better.  It may be that jsonb_subscript is enough
of an extension example, but perhaps adding a subscripting feature to some
contrib module would be better.

Aren't SBS_VALIDATION and SBS_EXEC just hangovers from the previous
design?  They're still in parse_node.h, and they're still mentioned in
the docs, but I don't see them used in actual code anywhere.
get_slice_arguments seems to be a hangover as well, which is good
because it's mighty ugly and undocumented.

It seems rather silly for ExecEvalSubscriptingRef to be palloc'ing some
per-subscript arrays each time through when it's got other arrays that are
still of fixed size MAXDIM.  I can believe that it might be a good idea
to increase or remove the MAXDIM limit, but this doesn't do it.  In any
case, you don't want to add the overhead of a couple of pallocs per
execution.  Using OidFunctionCall2 is even worse: that's adding a system
catalog lookup per execution.  You need to be caching the function address
as is done for regular function and operator calls.  (I take it you've not
done performance testing yet.)

I'm not really finding this to be an improvement:
- errmsg("array subscript in assignment must 
not be null")));
+ errmsg("container subscript in assignment 
must not be null")));
"Container" is going to seem like jargon to users.  Maybe it'd be okay to
drop the word altogether and just say "subscript in assignment must not be
null".  (Another question here is whether every datatype will be on board
with the current rules about null subscripts, or whether we need to
delegate null-handling to the datatype-specific execution function.
I'm not sure; it would complicate the API significantly for what might be
useless flexibility.)

I'm tempted to propose that it'd be a good idea to separate the regular
(varlena) array code paths from the fixed-length-array code paths
altogether, which you could do in this implementation by having separate
execution functions for them.  That would buy back some fraction of
whatever overhead we're adding with the additional level of function call.
Maybe even separate execution functions for the slice and not-slice
cases, though that might be overkill.

I'm not on board with these APPLY_OPERATOR_TO_TYPE macros.  If you
think you have a cute idea for improving the notation in the node support
files, great; submit a patch that changes all of the support functions
at once.  Patches that introduce one or two support functions that look
radically different from all the rest are not acceptable.

Likewise, what you did in places like JumbleExpr is too cute by half.
Just make two separate cases for the two new node types.  You're not
saving enough code to justify the additional intellectual complexity
and maintenance burden of doing it like that.

I do not think that the extra SubscriptingBase data structure is paying
for itself either; you're taking a hit in readability from the extra level
of struct, and as far as I can find it's not buying you one single thing,
because there's no code that operates on a SubscriptingBase argument.
I'd just drop that idea and make two independent struct types, or else
stick with the original ArrayRef design that made one struct serve both
purposes.  (IOW, my feeling that a separate assign struct would be a
readability improvement 

Re: [HACKERS] [PATCH] Generic type subscripting

[Tom Lane]

Peter Eisentraut  writes:
> I see a possible problem here:  This design only allows one subscripting
> function.  But what you'd really want in this case is at least two: one
> taking an integer type for selecting by array index, and one taking text
> for selecting by field name.

No, I think you're missing the point: there is just one function per
datatype for *parse analysis* of a subscripting operation applied to
that datatype.  What it chooses to allow as subscript type, and what
function it determines will be used at execution, is up to it.

I agree that the given jsonb_subscript is failing to handle the
subscript-an-array-with-an-integer case, but that's a datatype-specific
shortcoming not a failure of the overall design.

I would guess that what we really want for jsonb is the ability to
intermix integer and text subscripts, so that
 jsonbcol['foo'][42]['bar']
would extract the "bar" field of an object in position 42 of an
array in field "foo" of the given jsonb value.  So you probably
end up still having one jsonb execution function, not two, and
it would have different code paths depending on whether it sees
the type of the next subscript expression to be integer or text.

> It looks like your jsonb subscripting function just returns null if it
> can't find a field, which is also a bit dubious.

Nah, that seems fine.  Our precedent for standard array subscripting is to
return NULL for out-of-range subscripts, and the jsonb -> operator also
returns NULL if there's no such field.  It would be rather surprising if
jsonb subscripting threw an error instead; and I do not think it would be
more useful.

regards, tom lane


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

Re: [HACKERS] [PATCH] Generic type subscripting

[Peter Eisentraut]

On 2/28/17 13:02, Dmitry Dolgov wrote:
> +
> +-- Extract value by key
> +SELECT ('{"a": 1}'::jsonb)['a'];
> +
> +-- Extract nested value by key path
> +SELECT ('{"a": {"b": {"c": 1}}}'::jsonb)['a']['b']['c'];
> +
> +-- Extract element by index
> +SELECT ('[1, "2", null]'::jsonb)['1'];
> +
> +-- Update value by key
> +UPDATE table_name set jsonb_field['key'] = 1;
> +
> +-- Select records using where clause with subscripting
> +SELECT * from table_name where jsonb_field['key'] = '"value"';
> +

I see a possible problem here:  This design only allows one subscripting
function.  But what you'd really want in this case is at least two: one
taking an integer type for selecting by array index, and one taking text
for selecting by field name.

I suppose that since a given value can only be either an array or an
object, there is no ambiguity, but I think this might also lose some
error checking.  It might also not work the same way for other types.

It looks like your jsonb subscripting function just returns null if it
can't find a field, which is also a bit dubious.

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


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