[PATCH] Generic type subscripting

classic Classic list List threaded Threaded
106 messages Options
1234 ... 6
Reply | Threaded
Open this post in threaded view
|

[PATCH] Generic type subscripting

Dmitry Dolgov
Hi all

Regarding to the previous conversation [1], here is the patch for generic type
subscripting with several improvements. It contains the following changes:

* Subscripting node was separated into `SubscriptingRef` (only for data
  extraction) and `SubscriptingAssignRef` (only for assignment), and common
  data for both nodes is stored in `SubscriptingBase`. When it did make sense, I
  also separated correlated pieces of code. Common code for both nodes works
  with `SubscriptingRef`.

* Type related logic is separated into several functions, and the purpose of
  procedures like `jsonb_subscripting`/`array_subscripting` now is just to
  generate proper node with a correct function oid (I also tried to use a
  function pointer instead of a function oid, and it worked for me in general
  case, but I'm not sure if it will be ok for `_outSubscriptingRef` and
  `_readSubscriptingRef`).

* Function signatures are fixed.

* Functions for type dependent logic are going to be properly verified (where I
  found out such places, e.g. for `check_functions_in_node`)

The only thing left is separated typmod and collation. But since it's necessary
only for future data types, and there is already big enough list of changes
from previous version of this patch, I think it would be great to discuss it now and
implement this feature little bit later.

Generally speaking functionality, which has been implemented in this patch, is
the same. Code itself is little bit clumsy I guess (looks like I need to rename
`SubscriptingRef` to something shorter), but I hope I can refine it soon
enough.

As always, any feedback is welcome.



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

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

Re: [PATCH] Generic type subscripting

Peter Eisentraut-6
On 2/28/17 13:02, Dmitry Dolgov wrote:

> +<programlisting>
> +-- 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"';
> +</programlisting>

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

Tom Lane-2
Peter Eisentraut <[hidden email]> 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 ([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

Tom Lane-2
In reply to this post by Dmitry Dolgov
Dmitry Dolgov <[hidden email]> 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 isn't exactly getting borne out by what you've
done here.  But maybe there's a better way to do that.)

I wouldn't suggest putting a lot of work into the execQual.c part of the
patch right now, as execQual.c is going to look completely different if
Andres' patch gets in.  Better to concentrate on cleaning up the parsenode
struct types and thinking about a less messy answer for nested assignment.

                        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

David Steele
Hi Dmitry,

On 3/14/17 7:10 PM, Tom Lane wrote:
> Dmitry Dolgov <[hidden email]> 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
[hidden email]


--
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 21 March 2017 at 18:16, David Steele <[hidden email]> 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 <[hidden email]> wrote:
> Dmitry Dolgov <[hidden email]> writes:
>
> I looked through this a bit.
>

Thank you for the feedback.

> > On 10 March 2017 at 06:20, Peter Eisentraut <[hidden email]> 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
{
Expr        xpr;
SubscriptContainerRef *containerExpr;
List                *refupperindexpr;
List                *reflowerindexpr;
Oid            refevalfunc;
} SubscriptExtractRef;

typedef struct SubscriptAssignRef
{
Expr        xpr;
SubscriptContainerRef *containerExpr;
Expr        *refassgnexpr;
List                *refupperindexpr;
List                *reflowerindexpr;
Oid            refevalfunc;
} SubscriptAssignRef;

```

It's close to the previous version, but here will be a bit less duplicated code
in comparison with two independent structures, we still can use different nodes
for data assignment and data extraction, and node processing for these nodes
can be little bit more separated, e.g.:

```
    case T_SubscriptExtractRef:
        {
            // extract specific logic
            sbsref->containerExpr = ExecInitExpr((Expr *) sbsref->containerExpr, parent);
        }
    case T_SubscriptAssignRef:
        {
            // assign specific logic
            sbsref->containerExpr = ExecInitExpr((Expr *) sbsref->containerExpr, parent);
        }
    case T_SubscriptContainerRef:
        {
            // subscript container logic 
        }
```
Reply | Threaded
Open this post in threaded view
|

Re: [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 <[hidden email]
> <mailto:[hidden email]>> 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 <[hidden email]
> <mailto:[hidden email]>> wrote:
>> Dmitry Dolgov <[hidden email] <mailto:[hidden email]>>
> 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
[hidden email]


--
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 24 March 2017 at 15:39, David Steele <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Tom Lane-2
In reply to this post by David Steele
David Steele <[hidden email]> 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 ([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 24.03.2017 18:29, Tom Lane wrote:

> David Steele <[hidden email]> 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 ([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
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).


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

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

Re: [PATCH] Generic type subscripting

a.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 ([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 28 March 2017 at 11:58, Arthur Zakirov <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Dmitry Dolgov
On 28 March 2017 at 12:08, Dmitry Dolgov <[hidden email]> wrote:
>
> Wow, I didn't notice that, sorry - will fix it shortly.

So, here is the corrected version of the patch.


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

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

Re: [PATCH] Generic type subscripting

a.zakirov
On 28.03.2017 19:31, Dmitry Dolgov wrote:
> On 28 March 2017 at 12:08, Dmitry Dolgov <[hidden email]
> <mailto:[hidden email]>> 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"?

>       <row>
> +      <entry><structfield>typsubscripting</structfield></entry>
> +      <entry><type>regproc</type></entry>

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

> +  <title>JSON subscripting</title>
> +  <para>
> +   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".

> + <sect2 id="json-subscripting">
> +  <title>JSON subscripting</title>
> +  <para>
> +   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:

+ <sect2 id="jsonb-subscripting">
+  <title><type>jsonb</> Subscripting</title>
+  <para>
+   <type>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 ([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 29 March 2017 at 19:14, Arthur Zakirov <[hidden email]> wrote:
>
> I'm not sure that "typsbsparse" is better than "typsubscripting" or "typsubparse". Maybe "typsubsparse"?

`typsubparse` is more confusing as for me, but I like `typsubsparse`.

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

Great, thank you for confirmation.

I've attached a new version of the patch with related improvements.


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

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

Re: [PATCH] Generic type subscripting

a.zakirov
In reply to this post by a.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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

Dmitry Dolgov
On 30 March 2017 at 19:36, Arthur Zakirov <[hidden email]> 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?
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

a.zakirov
2017-03-31 5:32 GMT+03:00 Dmitry Dolgov <[hidden email]>:
> On 30 March 2017 at 19:36, Arthur Zakirov <[hidden email]> 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 ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Generic type subscripting

a.zakirov
In reply to this post by Dmitry Dolgov
2017-03-28 19:31 GMT+03:00 Dmitry Dolgov <[hidden email]>:
> On 28 March 2017 at 12:08, Dmitry Dolgov <[hidden email]> 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 ([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
1234 ... 6