Re: [HACKERS] [PATCH] Generic type subscripting

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.

[1]:
/message-id/flat/CA+q6zcVDuGBv=M0FqBYX8DPebS3F_0KQ6OVFobGJPM507_SZ_w(at)mail(dot)gmail(dot)com#CA+q6zcVDuGBv=M0FqBYX8DPebS3F_0KQ6OVFobGJPM507_SZ_w@mail.gmail.com

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

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> 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

Dmitry Dolgov <9erthalion6(at)gmail(dot)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 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

Hi Dmitry,

On 3/14/17 7:10 PM, Tom Lane wrote:
> Dmitry Dolgov <9erthalion6(at)gmail(dot)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
david(at)pgmasters(dot)net

> On 21 March 2017 at 18:16, David Steele <david(at)pgmasters(dot)net> 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 <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Dmitry Dolgov <9erthalion6(at)gmail(dot)com> writes:
>
> I looked through this a bit.
>

Thank you for the feedback.

> > On 10 March 2017 at 06:20, Peter Eisentraut <
peter(dot)eisentraut(at)2ndquadrant(dot)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
{
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
}
```

Hi Dmitry,

On 3/21/17 4:42 PM, Dmitry Dolgov wrote:
>> On 21 March 2017 at 18:16, David Steele <david(at)pgmasters(dot)net
> <mailto:david(at)pgmasters(dot)net>> 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 <tgl(at)sss(dot)pgh(dot)pa(dot)us
> <mailto:tgl(at)sss(dot)pgh(dot)pa(dot)us>> wrote:
>> Dmitry Dolgov <9erthalion6(at)gmail(dot)com <mailto:9erthalion6(at)gmail(dot)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
david(at)pgmasters(dot)net

On 24 March 2017 at 15:39, David Steele <david(at)pgmasters(dot)net> 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.

David Steele <david(at)pgmasters(dot)net> 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

On 24.03.2017 18:29, Tom Lane wrote:
> David Steele <david(at)pgmasters(dot)net> 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

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

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

On 28 March 2017 at 11:58, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru> 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.

On 28 March 2017 at 12:08, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
>
> Wow, I didn't notice that, sorry - will fix it shortly.

So, here is the corrected version of the patch.

On 28.03.2017 19:31, Dmitry Dolgov wrote:
> On 28 March 2017 at 12:08, Dmitry Dolgov <9erthalion6(at)gmail(dot)com
> <mailto:9erthalion6(at)gmail(dot)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"?

> <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.
/message-id/CAKNkYnz_WWkzzxyFx934N%3DEp47CAFju-Rk-sGeZo0ui8QdrGmw%40mail.gmail.com

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

On 29 March 2017 at 19:14, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru> 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.

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

On 30 March 2017 at 19:36, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru> 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?

2017-03-31 5:32 GMT+03:00 Dmitry Dolgov <9erthalion6(at)gmail(dot)com>:
> On 30 March 2017 at 19:36, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru> 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

2017-03-28 19:31 GMT+03:00 Dmitry Dolgov <9erthalion6(at)gmail(dot)com>:
> On 28 March 2017 at 12:08, Dmitry Dolgov <9erthalion6(at)gmail(dot)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

On 1 April 2017 at 21:26, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru> wrote:
>
> The patch looks good to me. But it is not applied :) I think it is
> because of new FTS functions for jsonb. The patch need rebasing.

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

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

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

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

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

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

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

This part looks strange to me too.

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

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

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

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

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

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

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

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

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

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

* we introduced a lot of small changes to polish it

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

As always, any feedback is welcome.

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

> On 30 June 2017 at 11:34, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru> wrote:
>
> Can you rebase the patch please? It is not applyed now. I think it is
because
> of pgindent.

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

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

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

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

> On 12 August 2017 at 13:37, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
>
> Here is a new rebased version of this patch (there were some conflicts in
commentaries).

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

On 29 August 2017 at 22:42, Dmitry Dolgov <9erthalion6(at)gmail(dot)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.

On Thu, Sep 07, 2017 at 10:49:54PM +0200, Dmitry Dolgov wrote:
> On 29 August 2017 at 22:42, Dmitry Dolgov <9erthalion6(at)gmail(dot)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

> On 9 September 2017 at 23:33, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
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?

Dmitry Dolgov <9erthalion6(at)gmail(dot)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

> On 11 September 2017 at 23:19, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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.

Dmitry Dolgov <9erthalion6(at)gmail(dot)com> writes:
>> On 11 September 2017 at 23:19, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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

> On 11 September 2017 at 23:45, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Dmitry Dolgov <9erthalion6(at)gmail(dot)com> writes:
> >> On 11 September 2017 at 23:19, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> 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!

> On 11 September 2017 at 23:55, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
wrote:
>
> Sure, an SQL command for that purpose is much better than a runtime check.
> I'm going to add such command to the patch, thank you for the information!

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

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

CREATE FUNCTION custom_subscripting_assign(internal)
RETURNS internal;

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

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

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

> On 17 September 2017 at 00:04, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
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?

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

Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru> 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

> On 17 September 2017 at 23:34, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
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.

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

> On 18 September 2017 at 11:39, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
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).

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

> On 19 September 2017 at 10:21, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
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 <9erthalion6(at)gmail(dot)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.

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

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

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

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

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

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

On Fri, Sep 22, 2017 at 3:51 PM, Peter Eisentraut
<peter(dot)eisentraut(at)2ndquadrant(dot)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.

+1

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

> On Fri, Sep 22, 2017 at 3:51 PM, Peter Eisentraut <
peter(dot)eisentraut(at)2ndquadrant(dot)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.

> On 30 September 2017 at 22:13, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
wrote:
> I'm still working on that, but obviously I'll not manage to finish it
within
> this CF, so I'm going to move it to the next one.

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

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

* introduce dependencies from a data type

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

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

On Sun, Oct 29, 2017 at 10:56:19PM +0100, Dmitry Dolgov wrote:
>
> So, here is the new version of patch that contains modifications we've
> discussed, namely:
>
> * store oids of `parse`, `fetch` and `assign` functions
>
> * introduce dependencies from a data type
>
> * as a side effect of previous two I also eliminated some unnecessary
> arguments
> in `parse` function.

Thank you for new version of the patch.

There are some notes.

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

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

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

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

Code
----

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

Otherwise next cases possible:

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

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

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

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

and

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

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

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

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

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

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

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

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

I will wait.

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

> On 31 October 2017 at 16:05, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
wrote:
> Documentation is compiled. But there are warnings about end-tags. Now it
is necessary to have full named end-tags

Fixed, thanks for noticing.

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

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

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

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

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

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

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

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

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

Fixed.

Thank you for fixing.

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

Understood.

>
> > > + scratch->d.sbsref.eval_finfo = eval_finfo;
> > > + scratch->d.sbsref.nested_finfo = nested_finfo;
> > > +
> > As I mentioned earlier we need assigning eval_finfo and nested_finfo only
> for EEOP_SBSREF_OLD, EEOP_SBSREF_ASSIGN and EEOP_SBSREF_FETCH steps.
> > Also they should be assigned before calling ExprEvalPushStep(), not
> after. Otherwise some bugs may appear in future.
>
> I'm really confused about this one. Can you tell me the exact line numbers?
> Because if I remove any of these lines "blindly", tests are failing.

Field scratch->d is union. Its fields should be changed only before calling ExprEvalPushStep(), which copies 'scratch'. To be more specific I attached the patch 0005-Fix-ExprEvalStep.patch, which can be applyed over your patches.

Some other notes are below.

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

I think you forgot commas and conjunction 'and'.

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

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

It seems that in the following you switched descriptions:

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

subscripting_assign_function is used for updating.

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

subscripting_fetch_function is used for fetching.

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

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

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

> On 8 November 2017 at 17:25, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
wrote:

Thanks for your review!

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

I've incorporated this variable into the tutorial.

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

Oh, now I see, thank you.

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

Shame on me :) Fixed.

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

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

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

On Sat, Nov 11, 2017 at 04:34:31PM +0100, Dmitry Dolgov wrote:
> > >
> > > Here isAssignment is unused variable, so it could be removed.
> >
> > In this case I disagree - the purpose of these examples is to show
> everything
> > you can use. So I just need to come up with some example that involves
> > `isAssignment`.
>
> I've incorporated this variable into the tutorial.

Great. I think users will know how to use isAssignment now.

> > I have a little complain about how ExprEvalStep gets resvalue. resvalue is
> > assigned in one place (within ExecEvalSubscriptingRefFetch(),
> > ExecEvalSubscriptingRefAssign()), resnull is assigned in another place
> > (within jsonb_subscript_fetch(), jsonb_subscript_assign()).
>
> Hm...I'm afraid I don't get this. `resnull` is never assigned inside
> `jsonb_subscript_fetch` or `jsonb_subscript_assign`, instead it's coming
> from `ExecInterpExp` as `isnull` if I remember correctly. Are we talking
> about
> the same thing?

No, I meant ExprEvalStep struct. For example, within ExecEvalSubscriptingRefFetch() you assign *op->resvalue but *op->resnull is leaved as unchanged:

> ExecEvalSubscriptingRefFetch(ExprState *state, ExprEvalStep *op)
> ...
> *op->resvalue = FunctionCall2(op->d.sbsref.eval_finfo,
> PointerGetDatum(*op->resvalue),
> PointerGetDatum(op));

For jsonb *op->resnull is changed within jsonb_subscript_fetch() for arrays within array_subscript_fetch() (which are called by ExecEvalSubscriptingRefFetch()):

> return jsonb_get_element(DatumGetJsonbP(containerSource),
> sbstate->upper,
> sbstate->numupper,
> step->resnull, /* step->resnull is changed within jsonb_get_element() */
> false);

It is not critical, but it may be good to change them in one place.

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

New tests passed.

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

> On 13 November 2017 at 14:11, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
wrote:
> > I have a little complain about how ExprEvalStep gets resvalue. resvalue
is
> > assigned in one place (within ExecEvalSubscriptingRefFetch(),
> > ExecEvalSubscriptingRefAssign()), resnull is assigned in another place
> > (within jsonb_subscript_fetch(), jsonb_subscript_assign()).
>
> Hm...I'm afraid I don't get this. `resnull` is never assigned inside
> `jsonb_subscript_fetch` or `jsonb_subscript_assign`, instead it's coming
> from `ExecInterpExp` as `isnull` if I remember correctly. Are we talking
> about
> the same thing?
>
> No, I meant ExprEvalStep struct. For example, within
> ExecEvalSubscriptingRefFetch() you assign op->resvalue but op->resnull is
> leaved as unchanged

Oh, I see now, thanks for the explanation. Actually, it's how it was
implemented before in array subscripting, and I tried to be consistent with
this implementation. But now I wonder if `resnull` is really needed in
`jsonb_get_element`, `array_get_element` and it seems to me that I can even
get
rid of it so, that everything would be assigned in `jsonb_subscript_fetch`,
`array_subscript_fetch` - I'll send a new version of the patch soon.

> On 14 November 2017 at 22:25, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
> But now I wonder if `resnull` is really needed in `jsonb_get_element`,
> `array_get_element` and it seems to me that I can even get rid of it so

On the second thought, no, looks like I'm wrong and it should be like this.
The
reason is that any `fetch` function should be in form

(container, internal) -> extracted value

which means that we need to return an extracted value (for jsonb it's a
`jsonb`,
for array it's an `anyelement`). But at the same time in general case we can
figure out if the result is null only inside a `fetch` function,
(`jsonb_get_element` for jsonb or whatever it may be for a custom data type)
because it returns Datum. So the only way to return this information is by
reference through the `internal` argument. To summarize, If as you said it's
not that critical, I would suggest to leave it as it is.

On Wed, Nov 15, 2017 at 11:02:59PM +0100, Dmitry Dolgov wrote:
> On the second thought, no, looks like I'm wrong and it should be like this.
> The
> reason is that any `fetch` function should be in form
>
> (container, internal) -> extracted value
>
> which means that we need to return an extracted value (for jsonb it's a
> `jsonb`,
> for array it's an `anyelement`). But at the same time in general case we can
> figure out if the result is null only inside a `fetch` function,
> (`jsonb_get_element` for jsonb or whatever it may be for a custom data type)
> because it returns Datum. So the only way to return this information is by
> reference through the `internal` argument. To summarize, If as you said it's
> not that critical, I would suggest to leave it as it is.

Actually it is not only way to return isnull information. You can also return it using pointer to a boolean argument.
*fetch() functions also doesn't need in ExprEvalStep struct, you can pass SubscriptingRefState struct instead.

I mean the following code:

ExecEvalSubscriptingRefFetch(ExprState *state, ExprEvalStep *op)
{
...
*op->resvalue = FunctionCall2(op->d.sbsref.eval_finfo,
PointerGetDatum(*op->resvalue),
PointerGetDatum(op->d.sbsref.state),
PointerGetDatum(op->resnull));
}

Datum
jsonb_subscript_fetch(PG_FUNCTION_ARGS)
{
Datum containerSource = PG_GETARG_DATUM(0);
SubscriptingRefState *state = (SubscriptingRefState *) PG_GETARG_POINTER(1);
bool *isNull = (bool *) PG_GETARG_POINTER(2);

return jsonb_get_element(DatumGetJsonbP(containerSource),
state->upper,
state->numupper,
isNull,
false);
}

> To summarize, If as you said it's
> not that critical, I would suggest to leave it as it is.

Yes, I just wanted to share an opinion how to improve the code. I thought that the current approach may confuse programmers, who will implement subscribting.

Also you can see extractValue() function of GIN [1]. It returns if values is null in same way.

1 - /docs/current/static/gin-extensibility.html

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

> On 16 November 2017 at 12:40, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
wrote:
>
> Actually it is not only way to return isnull information.

What i've meant is that it's the only way if we want to keep the function
signature. Actually I would prefer this

(container, internal) -> extracted value

over this (I assume that's exactly what you've suggested?)

(container, internal, internal) -> extracted value

because it makes the purpose of the function more clear (especially for
custom
data types). Also it would be consistent with `assign` functions (since
`isnull` is not assigned there). But I see your point, a separate argument
for
`isnull` will make it more straightforward in terms of null handling.

> fetch() functions also doesn't need in ExprEvalStep struct

I had hard time parsing this, but from your examples I assume you're talking
about passing little bit different arguments to `fetch` function (am I
right?).
Just from functionality point of view I don't see a big difference in what
argument to use to return `isnull` by reference. So at the end I think we
need
to choose between having one or two `internal` arguments for `fetch`
functions.
Any other opinions?

On Thu, Nov 16, 2017 at 10:37:40PM +0100, Dmitry Dolgov wrote:
> I had hard time parsing this, but from your examples I assume you're talking
> about passing little bit different arguments to `fetch` function (am I
> right?).

Yes, I meant to pass the following arguments:

Datum source, SubscriptingRefState *state, bool *isNull

I think it is time to mark the patch as "Ready for Commiter". I've
marked it.

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

> On 19 November 2017 at 16:13, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
wrote:
>
> I think it is time to mark the patch as "Ready for Commiter". I've
> marked it.

Good, thank you for the comprehensive review.

On Mon, Nov 20, 2017 at 7:47 PM, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
>> On 19 November 2017 at 16:13, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
>> wrote:
>>
>> I think it is time to mark the patch as "Ready for Commiter". I've
>> marked it.
>
> Good, thank you for the comprehensive review.

Documentation in patch 4 has conflicts. Please rebase. I am moving
this patch to next CF with "waiting on author".
--
Michael

> On 1 December 2017 at 06:34, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:
>
> Documentation in patch 4 has conflicts. Please rebase. I am moving
> this patch to next CF with "waiting on author".

Thanks for noticing. Here is the rebased version (the conflict itself was
quite
trivial, but I also cleaned up functions signature a bit).

> On 4 December 2017 at 01:26, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
>
> Thanks for noticing. Here is the rebased version (the conflict itself was
quite
> trivial, but I also cleaned up functions signature a bit).

Another rebased version of this patch.

Dmitry Dolgov <9erthalion6(at)gmail(dot)com> writes:
> Another rebased version of this patch.

Apologies for not having paid attention to this patch for so long.
Coming back to it now, I wonder what happened to the plan to separate
assignment and fetch into two different node types. I can see that
that didn't happen so far as primnodes.h is concerned, but you've
made some changes that seem to assume it did happen, eg this bit
in clauses.c:

@@ -1345,12 +1345,10 @@ contain_nonstrict_functions_walker(Node *node, void *context)
/* a window function could return non-null with null input */
return true;
}
- if (IsA(node, ArrayRef))
+ if (IsA(node, SubscriptingRef))
{
/* array assignment is nonstrict, but subscripting is strict */
- if (((ArrayRef *) node)->refassgnexpr != NULL)
- return true;
- /* else fall through to check args */
+ return true;
}
if (IsA(node, DistinctExpr))
{

Treating the two cases alike here is just wrong.

Also, the reason I was looking at clauses.c was I realized that
my recent commit 3decd150a broke this patch, because it introduced
understanding of ArrayRef into eval_const_expressions(). I think
that you can probably just do s/ArrayRef/SubscriptingRef/ there,
but it might deserve a closer look than I've given it.

I'm not terribly happy with the cosmetics of this patch at the moment.
There are too many places where it's achingly obvious that you did
s/ArrayRef/SubscriptingRef/g and nothing else, leaving code that does not
pass the test of "does it look like it was written like that to begin
with". There are a lot of variables still named "aref" or "arefstate"
or similar when that's no longer an apropos name; there are a lot of
sentences reading "an SubscriptingRef" which is bad English; there are a
lot of comments whose layout is not going to be too hot after pgindent
because "SubscriptingRef" is so much longer than "ArrayRef". (I'm tempted
to suggest that we call the node type just "Subscript", to buy back some
of that.) I'm not necessarily putting it on you to fix that stuff --- it
might be easier for a native English speaker --- but I am saying that if
I commit this, there are going to be a lot of differences in detail from
what's here now.

regards, tom lane

> On 4 January 2018 at 03:05, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> I wonder what happened to the plan to separate assignment and fetch into two
> different node types. I can see that that didn't happen so far as primnodes.h
> is concerned, but you've made some changes that seem to assume it did happen.

There was one version of this patch that followed this plan. It turns out that
it's quite unnatural approach (at least within the current implementation),
because I had to duplicate or reuse a lot of code for those two node types.
Eventually we decided to play it back. Unfortunately, I did it somehow sloppy
and forgot about those simple cases, thank you for noticing!

> I'm not terribly happy with the cosmetics of this patch at the moment.
> There are too many places where it's achingly obvious that you did
> s/ArrayRef/SubscriptingRef/g and nothing else, leaving code that does not
> pass the test of "does it look like it was written like that to begin
> with".

Yes, I paid not enough attention to these small details. I cleaned this up to
make it easier for a native speaker. Here is a new rebased version of the patch
with incorporated improvements that you've mentioned.

Dmitry Dolgov <9erthalion6(at)gmail(dot)com> writes:
>> On 4 January 2018 at 03:05, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I wonder what happened to the plan to separate assignment and fetch into two
>> different node types. I can see that that didn't happen so far as primnodes.h
>> is concerned, but you've made some changes that seem to assume it did happen.

> There was one version of this patch that followed this plan. It turns out that
> it's quite unnatural approach (at least within the current implementation),
> because I had to duplicate or reuse a lot of code for those two node types.

I'm not entirely convinced by that argument. Sure, there would be a lot of
duplicative code in the node support functions, but that's inherent in our
approach to those functions. I'm more concerned about readability,
especially given that exposing this feature to extensions is going to set
all these decisions in concrete forever.

> Here is a new rebased version of the patch
> with incorporated improvements that you've mentioned.

I spent a couple of hours looking through this. I'm still pretty unhappy
with the state of the parser APIs. In the first place, you haven't
documented what those APIs are in any meaningful way. I do not think it's
adequate to tell people to go read array_subscript_parse as the first and
only way to understand what a subscript parse function must do. We do
often expect extension authors to pick up small details that way, but
there should be a textual API spec of some sort --- for example, look at
the index AM API specs in indexam.sgml, which give pretty clear high-level
definitions of what each AM API function has to do.

Part of the reason why I'm insistent on that is that I think it will
expose that the division of labor between the core parser and the
datatype-specific parse function is still a mess. One particular sore
spot is the question of who decides what the return data type of a
subscripting function is. Right now you seem to be making that decision
in the core parser, at least for the assignment case, which is pretty
bad from an extensibility standpoint and also leads to all of this
messiness:

* You changed transformArrayType so that it doesn't throw an error if
the given type isn't an array --- without, I note, updating either the
function header comment or the internal comment that you falsified.

* That in turn means that where transformAssignmentSubscripts thinks
it's determining the result type, it may or may not be producing
InvalidOid instead (again, with no comment to warn the reader).

* And then you had to kludge transformAssignmentIndirection horribly
(and I'm not sure you kludged it enough, either, because there are
still a bunch of places where it uses targetTypeId without any concern
for the possibility that that's zero). It doesn't seem to me to be
acceptable to just ignore coercion failure, as that code does now.
If it's no longer the responsibility of this function to guarantee
that the result is of the right type, why is it attempting coercion
at all? In any case you failed to update its header comment to
explain what it's doing differently than before.

In short the division of labor in this area still needs to be thought
about. I don't think we really want transformAssignmentSubscripts
determining the required input data type at all; that should be
farmed out to the datatype-specific code. I'm also pretty unconvinced
about refnestedfunc --- why do we need that?

I also notice that you still haven't done anything about the problem
of the subscripting operation possibly yielding a different typmod
or collation than the container type has. It was okay to let that
go for awhile, but we're not shipping it like this, because it's
going to be awfully hard to change that struct type once extensions
are building them.

While I'm on the topic, I am not really happy with s/array/container/
as you've done in some of this code. To my mind, "container type"
includes composite types. Particularly in the parse_target code, where
we're currently dealing with either composites or arrays, making it say
that we're dealing with either composites or containers is just a recipe
for confusion. Unfortunately I can't think of a better word offhand,
but some attention to this is needed. As far as the comments go,
we might be able to use the term "subscriptable type", but not sure if
that will work for function/variable names.

On the executor side of things, I suspect Andres will be unhappy that
you are making ExprEvalStep part of the API for datatypes --- he
objected to my exposing it to plpgsql param eval in
https://postgr.es/m/20171220174243.n4y3hgzf7xd3mm5e@alap3.anarazel.de
and there was a lot more reason to do so there than there is here, IMO.
It looks like what you actually need is just the SubscriptingRefState and
an isnull flag, so it might be better to specify that the fetch and assign
functions have signatures like
Datum fetch(Datum val, SubscriptingRefState *state, bool *isnull)
(representing both of the last two arguments as INTERNAL at SQL level).

Now on the other hand, maybe the right way to go is to embrace a similar
approach to what I did for plpgsql param eval, and let the datatype
control what gets generated as the expression execution step. The main
point here would be to let the datatype provide the address of a callback
function that gets executed for a subscripting step, rather than having it
specify the OID of a pg_proc entry to call. There would be two big wins
from that:

* The callback function would have a plain C call signature, so we would
not have to go through FunctionCallN, saving a few cycles. This is
attractive because it would pretty much eliminate any concern about this
patch making array access slower at execution time.

* There would no longer be a wired-in restriction that there be two and
only two subscripting execution functions per datatype, since there would
not be any need for those functions to be identified in pg_type.

Basically, with this approach, a subscriptable data type would need to
provide two cataloged support functions: parse, as we have now, and
compile. Actual execution functions would be outside that. (Possibly
we could merge the support functions into one function that takes an
operation code, similar to one of your earlier designs. Not sure that
that's better, but it'd be easier to extend in future if we decide we
need three support operations...)

The two disadvantages I can see of approaching things this way are:

* There'd be at least some connection of subscriptable types to
expression compilation, which is what Andres was objecting to in the
message I cited above. Possibly we could alleviate that a bit by
providing helper functions that mask exactly what goes into the
expression step structs, but I'm not sure that that gets us far.

* We'd not have OIDs of execution functions in the parse trees for
subscripting operations, which would mean that we would not have
a convenient way to identify subscripting operations that are
mutable, parallel-unsafe, or leaky. Probably it'd be fine to assume
that subscripting is always immutable and parallel-safe, although
I'm slightly more concerned about whether people would want the
option to label it leaky vs leakproof. As against that, the approach
that's there right now adds planning overhead that wasn't there before
for exactly those function property lookups, and again I'm a bit worried
about the performance impact. (I did some crude performance tests
today that indicated that the existing patch has small but measurable
penalties, maybe on the order of 10 percent; and it'd be more by the
time we're done because I'm pretty sure you've missed some places that
ought to check these function properties if we're going to have them.
So I'm afraid that we'll get pushback from people who don't care about
extensible subscripts and do care about array performance.)

So roughly speaking, I'm imagining that we'd go back to a design similar
to one I recall you had at one point, where there's a single SQL-visible
subscripting support function per datatype, with a signature like

subscript_support(int opcode, internal other_info) returns internal

but the opcodes would now be "parse" and "compile". Actual execution
would use callback functions that don't have to be SQL-visible. This is
closer to the approach we've been using of late for things like AM APIs:
to the extent possible, there's just one SQL-registered handler function
and all else is a callback. Actually, we could make it *exactly* like
that, and have the registered handler give back a struct full of function
pointers rather than doing anything much itself. Maybe that's an even
better way to go.

regards, tom lane

Hi,

Tom pointed me towards this thread. I've not followed the topic, so
I might miss a bit of context while commenting on expression eval
related bits...

On 2018-01-07 17:39:00 -0500, Tom Lane wrote:
> On the executor side of things, I suspect Andres will be unhappy that
> you are making ExprEvalStep part of the API for datatypes --- he
> objected to my exposing it to plpgsql param eval in
> https://postgr.es/m/20171220174243.n4y3hgzf7xd3mm5e@alap3.anarazel.de
> and there was a lot more reason to do so there than there is here,
> IMO.

Indeed.

> It looks like what you actually need is just the SubscriptingRefState and
> an isnull flag, so it might be better to specify that the fetch and assign
> functions have signatures like
> Datum fetch(Datum val, SubscriptingRefState *state, bool *isnull)
> (representing both of the last two arguments as INTERNAL at SQL level).

That'd definitely be better.

> Now on the other hand, maybe the right way to go is to embrace a similar
> approach to what I did for plpgsql param eval, and let the datatype
> control what gets generated as the expression execution step. The main
> point here would be to let the datatype provide the address of a callback
> function that gets executed for a subscripting step, rather than having it
> specify the OID of a pg_proc entry to call. There would be two big wins
> from that:
>
> * The callback function would have a plain C call signature, so we would
> not have to go through FunctionCallN, saving a few cycles. This is
> attractive because it would pretty much eliminate any concern about this
> patch making array access slower at execution time.

I'll note that I'm not convinced that the goal this paragraph states and
having the datatype control the entire expression step are full
dependent on each other. It seems quite possible to have
ExecInitSubscriptingRef() call a datatype specific function that returns
C callbacks.

> The two disadvantages I can see of approaching things this way are:
>
> * There'd be at least some connection of subscriptable types to
> expression compilation, which is what Andres was objecting to in the
> message I cited above. Possibly we could alleviate that a bit by
> providing helper functions that mask exactly what goes into the
> expression step structs, but I'm not sure that that gets us far.

Yea, I'm not the greatest fan of that. With plpgsql it's at least
something in-core that's exposed, but I suspect the subscripotion
interface will get used outside of core, and I really want to whack some
of the expression stuff around some more.

> Actually, we could make it *exactly* like that, and have the
> registered handler give back a struct full of function pointers rather
> than doing anything much itself. Maybe that's an even better way to
> go.

I'd definitely advocate for that.

Greetings,

Andres Freund

Andres Freund <andres(at)anarazel(dot)de> writes:
> On 2018-01-07 17:39:00 -0500, Tom Lane wrote:
>> Now on the other hand, maybe the right way to go is to embrace a similar
>> approach to what I did for plpgsql param eval, and let the datatype
>> control what gets generated as the expression execution step.

> I'll note that I'm not convinced that the goal this paragraph states and
> having the datatype control the entire expression step are full
> dependent on each other. It seems quite possible to have
> ExecInitSubscriptingRef() call a datatype specific function that returns
> C callbacks.

Yeah, that's a good point. We could define the compile support function
as simply being allowed to examine the expression tree and give back
the address of the callback function to use, with the rest of the compiled
expression structure being predetermined. The more general approach would
only be useful if you imagine some sort of high-performance extension that
wants to compile specialized expr steps for its subscripting activity.
Probably the need for that is pretty far away yet.

BTW, if you wanted to change the way plpgsql param callbacks work to be
like this design (ie push the actual generation of the ExprStep back into
core, with plpgsql just supplying a callback address), I wouldn't object.

regards, tom lane

> On 7 January 2018 at 23:39, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Dmitry Dolgov <9erthalion6(at)gmail(dot)com> writes:
>>> On 4 January 2018 at 03:05, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> I wonder what happened to the plan to separate assignment and fetch into two
>>> different node types. I can see that that didn't happen so far as primnodes.h
>>> is concerned, but you've made some changes that seem to assume it did happen.
>
>> There was one version of this patch that followed this plan. It turns out that
>> it's quite unnatural approach (at least within the current implementation),
>> because I had to duplicate or reuse a lot of code for those two node types.
>
> I'm not entirely convinced by that argument. Sure, there would be a lot of
> duplicative code in the node support functions, but that's inherent in our
> approach to those functions. I'm more concerned about readability,
> especially given that exposing this feature to extensions is going to set
> all these decisions in concrete forever.

That version I'm talking about actually have got not that much readability from
this separation (at least from my perspective). For extenions it maybe more
justified, but at the same time `isAssignment` flag the only thing you need to
check to figure out whether it's a fetch or assignment operation - it doesn't
sound as something significantly worse for readability. Or do you mean
something else?

Anyway, it's quite possible that I just failed to implement a proper approach
to implement this. I'm going to keep pondering this in case I'll come up with
something better - it should be more or less straightforward to change this
logic at any time.

> * You changed transformArrayType so that it doesn't throw an error if
> the given type isn't an array --- without, I note, updating either the
> function header comment or the internal comment that you falsified.
>
> * That in turn means that where transformAssignmentSubscripts thinks
> it's determining the result type, it may or may not be producing
> InvalidOid instead (again, with no comment to warn the reader).
>
> * And then you had to kludge transformAssignmentIndirection horribly
> (and I'm not sure you kludged it enough, either, because there are
> still a bunch of places where it uses targetTypeId without any concern
> for the possibility that that's zero). It doesn't seem to me to be
> acceptable to just ignore coercion failure, as that code does now.
> If it's no longer the responsibility of this function to guarantee
> that the result is of the right type, why is it attempting coercion
> at all? In any case you failed to update its header comment to
> explain what it's doing differently than before.
>
> In short the division of labor in this area still needs to be thought
> about. I don't think we really want transformAssignmentSubscripts
> determining the required input data type at all; that should be
> farmed out to the datatype-specific code. I'm also pretty unconvinced
> about refnestedfunc --- why do we need that?

Yes, I see, there is a room for improvements. I'm going to post a new version
soon, where I'll try to address this. About `refnestedfunc` - I added it as
part of my modification for functions caching, but looks like now this function
is useless.

> While I'm on the topic, I am not really happy with s/array/container/
> as you've done in some of this code. To my mind, "container type"
> includes composite types. Particularly in the parse_target code, where
> we're currently dealing with either composites or arrays, making it say
> that we're dealing with either composites or containers is just a recipe
> for confusion. Unfortunately I can't think of a better word offhand,
> but some attention to this is needed. As far as the comments go,
> we might be able to use the term "subscriptable type", but not sure if
> that will work for function/variable names.

Is there a plausible use case when "container type" can be something else than
a "composite type"? Maybe it's ok just to say that "container type" is equal to
"composite type"?

> On the executor side of things, I suspect Andres will be unhappy that
> you are making ExprEvalStep part of the API for datatypes --- he
> objected to my exposing it to plpgsql param eval in
> https://postgr.es/m/20171220174243.n4y3hgzf7xd3mm5e@alap3.anarazel.de
> and there was a lot more reason to do so there than there is here, IMO.
> It looks like what you actually need is just the SubscriptingRefState and
> an isnull flag, so it might be better to specify that the fetch and assign
> functions have signatures like
> Datum fetch(Datum val, SubscriptingRefState *state, bool *isnull)
> (representing both of the last two arguments as INTERNAL at SQL level).

Yes, I agree.

> Now on the other hand, maybe the right way to go is to embrace a similar
> approach to what I did for plpgsql param eval, and let the datatype
> control what gets generated as the expression execution step. The main
> point here would be to let the datatype provide the address of a callback
> function that gets executed for a subscripting step, rather than having it
> specify the OID of a pg_proc entry to call.

I had one implementation that resembles this approach. But as far as I see
(please correct me if something in the following chain is wrong) it's necessary
to store pointers to these callback functions in a `SubscriptingRef` node,
which means that they would be missing in functions from
`readfuncs`/`outfuncs`/`copyfuncs`. Is there any situation when this can lead
to undesirable consequences?

> * We'd not have OIDs of execution functions in the parse trees for
> subscripting operations, which would mean that we would not have
> a convenient way to identify subscripting operations that are
> mutable, parallel-unsafe, or leaky. Probably it'd be fine to assume
> that subscripting is always immutable and parallel-safe, although
> I'm slightly more concerned about whether people would want the
> option to label it leaky vs leakproof. As against that, the approach
> that's there right now adds planning overhead that wasn't there before
> for exactly those function property lookups, and again I'm a bit worried
> about the performance impact. (I did some crude performance tests
> today that indicated that the existing patch has small but measurable
> penalties, maybe on the order of 10 percent; and it'd be more by the
> time we're done because I'm pretty sure you've missed some places that
> ought to check these function properties if we're going to have them.
> So I'm afraid that we'll get pushback from people who don't care about
> extensible subscripts and do care about array performance.)

We also wouldn't have a convenient way to specify costs of subscripting
operations. I had in mind a situation, when someone wants to have a quite heavy
subscripting function, which should affect planner's decisions. It maybe a rare
case though.

Hi,

On 2018-01-04 16:47:50 +0100, Dmitry Dolgov wrote:
> diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
> index 16f908037c..ee50fda4ef 100644
> --- a/src/backend/executor/execExpr.c
> +++ b/src/backend/executor/execExpr.c
> @@ -64,7 +64,8 @@ static void ExecInitExprSlots(ExprState *state, Node *node);
> static bool get_last_attnums_walker(Node *node, LastAttnumInfo *info);
> static void ExecInitWholeRowVar(ExprEvalStep *scratch, Var *variable,
> ExprState *state);
> -static void ExecInitArrayRef(ExprEvalStep *scratch, ArrayRef *aref,
> +static void ExecInitSubscriptingRef(ExprEvalStep *scratch,
> + SubscriptingRef *sbsref,
> ExprState *state,
> Datum *resv, bool *resnull);
> static bool isAssignmentIndirectionExpr(Expr *expr);
> @@ -857,11 +858,11 @@ ExecInitExprRec(Expr *node, ExprState *state,
> break;
> }
>
> - case T_ArrayRef:
> + case T_SubscriptingRef:
> {
> - ArrayRef *aref = (ArrayRef *) node;
> + SubscriptingRef *sbsref = (SubscriptingRef *) node;
>
> - ExecInitArrayRef(&scratch, aref, state, resv, resnull);
> + ExecInitSubscriptingRef(&scratch, sbsref, state, resv, resnull);
> break;
> }
>
> @@ -1176,7 +1177,7 @@ ExecInitExprRec(Expr *node, ExprState *state,
> /*
> * Use the CaseTestExpr mechanism to pass down the old
> * value of the field being replaced; this is needed in
> - * case the newval is itself a FieldStore or ArrayRef that
> + * case the newval is itself a FieldStore or SubscriptingRef that
> * has to obtain and modify the old value. It's safe to
> * reuse the CASE mechanism because there cannot be a CASE
> * between here and where the value would be needed, and a
> @@ -2401,34 +2402,40 @@ ExecInitWholeRowVar(ExprEvalStep *scratch, Var *variable, ExprState *state)
> }
>
> /*
> - * Prepare evaluation of an ArrayRef expression.
> + * Prepare evaluation of a SubscriptingRef expression.
> */
> static void
> -ExecInitArrayRef(ExprEvalStep *scratch, ArrayRef *aref,
> +ExecInitSubscriptingRef(ExprEvalStep *scratch, SubscriptingRef *sbsref,
> ExprState *state, Datum *resv, bool *resnull)
> {
> - bool isAssignment = (aref->refassgnexpr != NULL);
> - ArrayRefState *arefstate = palloc0(sizeof(ArrayRefState));
> - List *adjust_jumps = NIL;
> - ListCell *lc;
> - int i;
> + bool isAssignment = (sbsref->refassgnexpr != NULL);
> + SubscriptingRefState *sbsrefstate = palloc0(sizeof(SubscriptingRefState));
> + List *adjust_jumps = NIL;
> + ListCell *lc;
> + int i;
> + FmgrInfo *eval_finfo, *nested_finfo;
> +
> + eval_finfo = palloc0(sizeof(FmgrInfo));
> + nested_finfo = palloc0(sizeof(FmgrInfo));
> +
> + fmgr_info(sbsref->refevalfunc, eval_finfo);
> + if (OidIsValid(sbsref->refnestedfunc))
> + {
> + fmgr_info(sbsref->refnestedfunc, nested_finfo);
> + }
>
> - /* Fill constant fields of ArrayRefState */
> - arefstate->isassignment = isAssignment;
> - arefstate->refelemtype = aref->refelemtype;
> - arefstate->refattrlength = get_typlen(aref->refarraytype);
> - get_typlenbyvalalign(aref->refelemtype,
> - &arefstate->refelemlength,
> - &arefstate->refelembyval,
> - &arefstate->refelemalign);
> + /* Fill constant fields of SubscriptingRefState */
> + sbsrefstate->isassignment = isAssignment;
> + sbsrefstate->refelemtype = sbsref->refelemtype;
> + sbsrefstate->refattrlength = get_typlen(sbsref->refcontainertype);
>
> /*
> * Evaluate array input. It's safe to do so into resv/resnull, because we
> * won't use that as target for any of the other subexpressions, and it'll
> - * be overwritten by the final EEOP_ARRAYREF_FETCH/ASSIGN step, which is
> + * be overwritten by the final EEOP_SBSREF_FETCH/ASSIGN step, which is
> * pushed last.
> */
> - ExecInitExprRec(aref->refexpr, state, resv, resnull);
> + ExecInitExprRec(sbsref->refexpr, state, resv, resnull);
>
> /*
> * If refexpr yields NULL, and it's a fetch, then result is NULL. We can
> @@ -2440,92 +2447,95 @@ ExecInitArrayRef(ExprEvalStep *scratch, ArrayRef *aref,
> scratch->opcode = EEOP_JUMP_IF_NULL;
> scratch->d.jump.jumpdone = -1; /* adjust later */
> ExprEvalPushStep(state, scratch);
> +
> adjust_jumps = lappend_int(adjust_jumps,
> state->steps_len - 1);
> }
>
> /* Verify subscript list lengths are within limit */
> - if (list_length(aref->refupperindexpr) > MAXDIM)
> + if (list_length(sbsref->refupperindexpr) > MAX_SUBSCRIPT_DEPTH)
> ereport(ERROR,
> (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
> errmsg("number of array dimensions (%d) exceeds the maximum allowed (%d)",
> - list_length(aref->refupperindexpr), MAXDIM)));
> + list_length(sbsref->refupperindexpr), MAX_SUBSCRIPT_DEPTH)));
>
> - if (list_length(aref->reflowerindexpr) > MAXDIM)
> + if (list_length(sbsref->reflowerindexpr) > MAX_SUBSCRIPT_DEPTH)
> ereport(ERROR,
> (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
> errmsg("number of array dimensions (%d) exceeds the maximum allowed (%d)",
> - list_length(aref->reflowerindexpr), MAXDIM)));
> + list_length(sbsref->reflowerindexpr), MAX_SUBSCRIPT_DEPTH)));
>
> /* Evaluate upper subscripts */
> i = 0;
> - foreach(lc, aref->refupperindexpr)
> + foreach(lc, sbsref->refupperindexpr)
> {
> Expr *e = (Expr *) lfirst(lc);
>
> /* When slicing, individual subscript bounds can be omitted */
> if (!e)
> {
> - arefstate->upperprovided[i] = false;
> + sbsrefstate->upperprovided[i] = false;
> i++;
> continue;
> }
>
> - arefstate->upperprovided[i] = true;
> + sbsrefstate->upperprovided[i] = true;
>
> /* Each subscript is evaluated into subscriptvalue/subscriptnull */
> ExecInitExprRec(e, state,
> - &arefstate->subscriptvalue, &arefstate->subscriptnull);
> -
> - /* ... and then ARRAYREF_SUBSCRIPT saves it into step's workspace */
> - scratch->opcode = EEOP_ARRAYREF_SUBSCRIPT;
> - scratch->d.arrayref_subscript.state = arefstate;
> - scratch->d.arrayref_subscript.off = i;
> - scratch->d.arrayref_subscript.isupper = true;
> - scratch->d.arrayref_subscript.jumpdone = -1; /* adjust later */
> + &sbsrefstate->subscriptvalue, &sbsrefstate->subscriptnull);
> +
> + /* ... and then SBSREF_SUBSCRIPT saves it into step's workspace */
> + scratch->opcode = EEOP_SBSREF_SUBSCRIPT;
> + scratch->d.sbsref_subscript.state = sbsrefstate;
> + scratch->d.sbsref_subscript.off = i;
> + scratch->d.sbsref_subscript.isupper = true;
> + scratch->d.sbsref_subscript.jumpdone = -1; /* adjust later */
> ExprEvalPushStep(state, scratch);
> +
> adjust_jumps = lappend_int(adjust_jumps,
> state->steps_len - 1);
> i++;
> }
> - arefstate->numupper = i;
> + sbsrefstate->numupper = i;
>
> /* Evaluate lower subscripts similarly */
> i = 0;
> - foreach(lc, aref->reflowerindexpr)
> + foreach(lc, sbsref->reflowerindexpr)
> {
> Expr *e = (Expr *) lfirst(lc);
>
> /* When slicing, individual subscript bounds can be omitted */
> if (!e)
> {
> - arefstate->lowerprovided[i] = false;
> + sbsrefstate->lowerprovided[i] = false;
> i++;
> continue;
> }
>
> - arefstate->lowerprovided[i] = true;
> + sbsrefstate->lowerprovided[i] = true;
>
> /* Each subscript is evaluated into subscriptvalue/subscriptnull */
> ExecInitExprRec(e, state,
> - &arefstate->subscriptvalue, &arefstate->subscriptnull);
> -
> - /* ... and then ARRAYREF_SUBSCRIPT saves it into step's workspace */
> - scratch->opcode = EEOP_ARRAYREF_SUBSCRIPT;
> - scratch->d.arrayref_subscript.state = arefstate;
> - scratch->d.arrayref_subscript.off = i;
> - scratch->d.arrayref_subscript.isupper = false;
> - scratch->d.arrayref_subscript.jumpdone = -1; /* adjust later */
> + &sbsrefstate->subscriptvalue, &sbsrefstate->subscriptnull);
> +
> + /* ... and then SBSREF_SUBSCRIPT saves it into step's workspace */
> + scratch->opcode = EEOP_SBSREF_SUBSCRIPT;
> + scratch->d.sbsref_subscript.state = sbsrefstate;
> + scratch->d.sbsref_subscript.off = i;
> + scratch->d.sbsref_subscript.isupper = false;
> + scratch->d.sbsref_subscript.jumpdone = -1; /* adjust later */
> ExprEvalPushStep(state, scratch);
> +
> adjust_jumps = lappend_int(adjust_jumps,
> state->steps_len - 1);
> i++;
> }
> - arefstate->numlower = i;
> + sbsrefstate->numlower = i;
>
> /* Should be impossible if parser is sane, but check anyway: */
> - if (arefstate->numlower != 0 &&
> - arefstate->numupper != arefstate->numlower)
> + if (sbsrefstate->numlower != 0 &&
> + sbsrefstate->numupper != sbsrefstate->numlower)
> elog(ERROR, "upper and lower index lists are not same length");
>
> if (isAssignment)
> @@ -2535,7 +2545,7 @@ ExecInitArrayRef(ExprEvalStep *scratch, ArrayRef *aref,
>
> /*
> * We might have a nested-assignment situation, in which the
> - * refassgnexpr is itself a FieldStore or ArrayRef that needs to
> + * refassgnexpr is itself a FieldStore or SubscriptingRef that needs to
> * obtain and modify the previous value of the array element or slice
> * being replaced. If so, we have to extract that value from the
> * array and pass it down via the CaseTestExpr mechanism. It's safe
> @@ -2547,37 +2557,45 @@ ExecInitArrayRef(ExprEvalStep *scratch, ArrayRef *aref,
> * Since fetching the old element might be a nontrivial expense, do it
> * only if the argument actually needs it.
> */
> - if (isAssignmentIndirectionExpr(aref->refassgnexpr))
> + if (isAssignmentIndirectionExpr(sbsref->refassgnexpr))
> {
> - scratch->opcode = EEOP_ARRAYREF_OLD;
> - scratch->d.arrayref.state = arefstate;
> + scratch->opcode = EEOP_SBSREF_OLD;
> + scratch->d.sbsref.state = sbsrefstate;
> + scratch->d.sbsref.eval_finfo = eval_finfo;
> + scratch->d.sbsref.nested_finfo = nested_finfo;
> ExprEvalPushStep(state, scratch);
> }
>
> - /* ARRAYREF_OLD puts extracted value into prevvalue/prevnull */
> + /* SBSREF_OLD puts extracted value into prevvalue/prevnull */
> save_innermost_caseval = state->innermost_caseval;
> save_innermost_casenull = state->innermost_casenull;
> - state->innermost_caseval = &arefstate->prevvalue;
> - state->innermost_casenull = &arefstate->prevnull;
> + state->innermost_caseval = &sbsrefstate->prevvalue;
> + state->innermost_casenull = &sbsrefstate->prevnull;
>
> /* evaluate replacement value into replacevalue/replacenull */
> - ExecInitExprRec(aref->refassgnexpr, state,
> - &arefstate->replacevalue, &arefstate->replacenull);
> + ExecInitExprRec(sbsref->refassgnexpr, state,
> + &sbsrefstate->replacevalue, &sbsrefstate->replacenull);
>
> state->innermost_caseval = save_innermost_caseval;
> state->innermost_casenull = save_innermost_casenull;
>
> /* and perform the assignment */
> - scratch->opcode = EEOP_ARRAYREF_ASSIGN;
> - scratch->d.arrayref.state = arefstate;
> + scratch->opcode = EEOP_SBSREF_ASSIGN;
> + scratch->d.sbsref.state = sbsrefstate;
> + scratch->d.sbsref.eval_finfo = eval_finfo;
> + scratch->d.sbsref.nested_finfo = nested_finfo;
> ExprEvalPushStep(state, scratch);
> +
> }
> else
> {
> /* array fetch is much simpler */
> - scratch->opcode = EEOP_ARRAYREF_FETCH;
> - scratch->d.arrayref.state = arefstate;
> + scratch->opcode = EEOP_SBSREF_FETCH;
> + scratch->d.sbsref.state = sbsrefstate;
> + scratch->d.sbsref.eval_finfo = eval_finfo;
> + scratch->d.sbsref.nested_finfo = nested_finfo;
> ExprEvalPushStep(state, scratch);
> +
> }
>
> /* adjust jump targets */
> @@ -2585,10 +2603,10 @@ ExecInitArrayRef(ExprEvalStep *scratch, ArrayRef *aref,
> {
> ExprEvalStep *as = &state->steps[lfirst_int(lc)];
>
> - if (as->opcode == EEOP_ARRAYREF_SUBSCRIPT)
> + if (as->opcode == EEOP_SBSREF_SUBSCRIPT)
> {
> - Assert(as->d.arrayref_subscript.jumpdone == -1);
> - as->d.arrayref_subscript.jumpdone = state->steps_len;
> + Assert(as->d.sbsref_subscript.jumpdone == -1);
> + as->d.sbsref_subscript.jumpdone = state->steps_len;
> }
> else
> {
> @@ -2600,8 +2618,8 @@ ExecInitArrayRef(ExprEvalStep *scratch, ArrayRef *aref,
> }
>
> /*
> - * Helper for preparing ArrayRef expressions for evaluation: is expr a nested
> - * FieldStore or ArrayRef that needs the old element value passed down?
> + * Helper for preparing SubscriptingRef expressions for evaluation: is expr a nested
> + * FieldStore or SubscriptingRef that needs the old element value passed down?
> *
> * (We could use this in FieldStore too, but in that case passing the old
> * value is so cheap there's no need.)
> @@ -2624,11 +2642,11 @@ isAssignmentIndirectionExpr(Expr *expr)
> if (fstore->arg && IsA(fstore->arg, CaseTestExpr))
> return true;
> }
> - else if (IsA(expr, ArrayRef))
> + else if (IsA(expr, SubscriptingRef))
> {
> - ArrayRef *arrayRef = (ArrayRef *) expr;
> + SubscriptingRef *sbsRef = (SubscriptingRef *) expr;
>
> - if (arrayRef->refexpr && IsA(arrayRef->refexpr, CaseTestExpr))
> + if (sbsRef->refexpr && IsA(sbsRef->refexpr, CaseTestExpr))
> return true;
> }
> return false;
> diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
> index 2e88417265..0c72e80b58 100644
> --- a/src/backend/executor/execExprInterp.c
> +++ b/src/backend/executor/execExprInterp.c
> @@ -364,10 +364,10 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
> &&CASE_EEOP_FIELDSELECT,
> &&CASE_EEOP_FIELDSTORE_DEFORM,
> &&CASE_EEOP_FIELDSTORE_FORM,
> - &&CASE_EEOP_ARRAYREF_SUBSCRIPT,
> - &&CASE_EEOP_ARRAYREF_OLD,
> - &&CASE_EEOP_ARRAYREF_ASSIGN,
> - &&CASE_EEOP_ARRAYREF_FETCH,
> + &&CASE_EEOP_SBSREF_SUBSCRIPT,
> + &&CASE_EEOP_SBSREF_OLD,
> + &&CASE_EEOP_SBSREF_ASSIGN,
> + &&CASE_EEOP_SBSREF_FETCH,
> &&CASE_EEOP_DOMAIN_TESTVAL,
> &&CASE_EEOP_DOMAIN_NOTNULL,
> &&CASE_EEOP_DOMAIN_CHECK,
> @@ -1367,43 +1367,43 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
> EEO_NEXT();
> }
>
> - EEO_CASE(EEOP_ARRAYREF_SUBSCRIPT)
> + EEO_CASE(EEOP_SBSREF_SUBSCRIPT)
> {
> /* Process an array subscript */
>
> /* too complex for an inline implementation */
> - if (ExecEvalArrayRefSubscript(state, op))
> + if (ExecEvalSubscriptingRef(state, op))
> {
> EEO_NEXT();
> }
> else
> {
> - /* Subscript is null, short-circuit ArrayRef to NULL */
> - EEO_JUMP(op->d.arrayref_subscript.jumpdone);
> + /* Subscript is null, short-circuit SubscriptingRef to NULL */
> + EEO_JUMP(op->d.sbsref_subscript.jumpdone);
> }
> }
>
> - EEO_CASE(EEOP_ARRAYREF_OLD)
> + EEO_CASE(EEOP_SBSREF_OLD)
> {
> /*
> - * Fetch the old value in an arrayref assignment, in case it's
> + * Fetch the old value in an sbsref assignment, in case it's
> * referenced (via a CaseTestExpr) inside the assignment
> * expression.
> */
>
> /* too complex for an inline implementation */
> - ExecEvalArrayRefOld(state, op);
> + ExecEvalSubscriptingRefOld(state, op);
>
> EEO_NEXT();
> }
>
> /*
> - * Perform ArrayRef assignment
> + * Perform SubscriptingRef assignment
> */
> - EEO_CASE(EEOP_ARRAYREF_ASSIGN)
> + EEO_CASE(EEOP_SBSREF_ASSIGN)
> {
> /* too complex for an inline implementation */
> - ExecEvalArrayRefAssign(state, op);
> + ExecEvalSubscriptingRefAssign(state, op);
>
> EEO_NEXT();
> }
> @@ -1411,10 +1411,10 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
> /*
> * Fetch subset of an array.
> */
> - EEO_CASE(EEOP_ARRAYREF_FETCH)
> + EEO_CASE(EEOP_SBSREF_FETCH)
> {
> /* too complex for an inline implementation */
> - ExecEvalArrayRefFetch(state, op);
> + ExecEvalSubscriptingRefFetch(state, op);
>
> EEO_NEXT();
> }
> @@ -2702,197 +2702,115 @@ ExecEvalFieldStoreForm(ExprState *state, ExprEvalStep *op, ExprContext *econtext
> }
>
> /*
> - * Process a subscript in an ArrayRef expression.
> + * Process a subscript in a SubscriptingRef expression.
> *
> * If subscript is NULL, throw error in assignment case, or in fetch case
> * set result to NULL and return false (instructing caller to skip the rest
> - * of the ArrayRef sequence).
> + * of the SubscriptingRef sequence).
> *
> * Subscript expression result is in subscriptvalue/subscriptnull.
> * On success, integer subscript value has been saved in upperindex[] or
> * lowerindex[] for use later.
> */
> bool
> -ExecEvalArrayRefSubscript(ExprState *state, ExprEvalStep *op)
> +ExecEvalSubscriptingRef(ExprState *state, ExprEvalStep *op)
> {
> - ArrayRefState *arefstate = op->d.arrayref_subscript.state;
> - int *indexes;
> - int off;
> + SubscriptingRefState *sbsrefstate = op->d.sbsref_subscript.state;
> + Datum *indexes;
> + int off;
>
> /* If any index expr yields NULL, result is NULL or error */
> - if (arefstate->subscriptnull)
> + if (sbsrefstate->subscriptnull)
> {
> - if (arefstate->isassignment)
> + if (sbsrefstate->isassignment)
> ereport(ERROR,
> (errcode(ERRCODE_NULL_VALUE_NOT_ALLOWED),
> - errmsg("array subscript in assignment must not be null")));
> + errmsg("subscript in assignment must not be null")));
> *op->resnull = true;
> return false;
> }
>
> /* Convert datum to int, save in appropriate place */
> - if (op->d.arrayref_subscript.isupper)
> - indexes = arefstate->upperindex;
> + if (op->d.sbsref_subscript.isupper)
> + indexes = sbsrefstate->upper;
> else
> - indexes = arefstate->lowerindex;
> - off = op->d.arrayref_subscript.off;
> + indexes = sbsrefstate->lower;
> + off = op->d.sbsref_subscript.off;
>
> - indexes[off] = DatumGetInt32(arefstate->subscriptvalue);
> + indexes[off] = sbsrefstate->subscriptvalue;
>
> return true;
> }
>
> /*
> - * Evaluate ArrayRef fetch.
> + * Evaluate SubscriptingRef fetch.
> *
> - * Source array is in step's result variable.
> + * Source container is in step's result variable.
> */
> void
> -ExecEvalArrayRefFetch(ExprState *state, ExprEvalStep *op)
> +ExecEvalSubscriptingRefFetch(ExprState *state, ExprEvalStep *op)
> {
> - ArrayRefState *arefstate = op->d.arrayref.state;
> -
> - /* Should not get here if source array (or any subscript) is null */
> + /* Should not get here if source container (or any subscript) is null */
> Assert(!(*op->resnull));
>
> - if (arefstate->numlower == 0)
> - {
> - /* Scalar case */
> - *op->resvalue = array_get_element(*op->resvalue,
> - arefstate->numupper,
> - arefstate->upperindex,
> - arefstate->refattrlength,
> - arefstate->refelemlength,
> - arefstate->refelembyval,
> - arefstate->refelemalign,
> - op->resnull);
> - }
> - else
> - {
> - /* Slice case */
> - *op->resvalue = array_get_slice(*op->resvalue,
> - arefstate->numupper,
> - arefstate->upperindex,
> - arefstate->lowerindex,
> - arefstate->upperprovided,
> - arefstate->lowerprovided,
> - arefstate->refattrlength,
> - arefstate->refelemlength,
> - arefstate->refelembyval,
> - arefstate->refelemalign);
> - }
> + *op->resvalue = FunctionCall2(op->d.sbsref.eval_finfo,
> + PointerGetDatum(*op->resvalue),
> + PointerGetDatum(op));
> }
>
> /*
> - * Compute old array element/slice value for an ArrayRef assignment
> - * expression. Will only be generated if the new-value subexpression
> - * contains ArrayRef or FieldStore. The value is stored into the
> - * ArrayRefState's prevvalue/prevnull fields.
> + * Compute old container element/slice value for a SubscriptingRef assignment
> + * expression. Will only be generated if the new-value subexpression
> + * contains SubscriptingRef or FieldStore. The value is stored into the
> + * SubscriptingRefState's prevvalue/prevnull fields.
> */
> void
> -ExecEvalArrayRefOld(ExprState *state, ExprEvalStep *op)
> +ExecEvalSubscriptingRefOld(ExprState *state, ExprEvalStep *op)
> {
> - ArrayRefState *arefstate = op->d.arrayref.state;
> + SubscriptingRefState *sbsrefstate = op->d.sbsref.state;
>
> if (*op->resnull)
> {
> - /* whole array is null, so any element or slice is too */
> - arefstate->prevvalue = (Datum) 0;
> - arefstate->prevnull = true;
> - }
> - else if (arefstate->numlower == 0)
> - {
> - /* Scalar case */
> - arefstate->prevvalue = array_get_element(*op->resvalue,
> - arefstate->numupper,
> - arefstate->upperindex,
> - arefstate->refattrlength,
> - arefstate->refelemlength,
> - arefstate->refelembyval,
> - arefstate->refelemalign,
> - &arefstate->prevnull);
> + /* whole container is null, so any element or slice is too */
> + sbsrefstate->prevvalue = (Datum) 0;
> + sbsrefstate->prevnull = true;
> }
> else
> {
> - /* Slice case */
> - /* this is currently unreachable */
> - arefstate->prevvalue = array_get_slice(*op->resvalue,
> - arefstate->numupper,
> - arefstate->upperindex,
> - arefstate->lowerindex,
> - arefstate->upperprovided,
> - arefstate->lowerprovided,
> - arefstate->refattrlength,
> - arefstate->refelemlength,
> - arefstate->refelembyval,
> - arefstate->refelemalign);
> - arefstate->prevnull = false;
> + sbsrefstate->prevvalue = FunctionCall2(op->d.sbsref.nested_finfo,
> + PointerGetDatum(*op->resvalue),
> + PointerGetDatum(op));
> +
> + if (sbsrefstate->numlower != 0)
> + sbsrefstate->prevnull = false;
> +
> }
> }
>
> /*
> - * Evaluate ArrayRef assignment.
> + * Evaluate SubscriptingRef assignment.
> *
> - * Input array (possibly null) is in result area, replacement value is in
> - * ArrayRefState's replacevalue/replacenull.
> + * Input container (possibly null) is in result area, replacement value is in
> + * SubscriptingRefState's replacevalue/replacenull.
> */
> void
> -ExecEvalArrayRefAssign(ExprState *state, ExprEvalStep *op)
> +ExecEvalSubscriptingRefAssign(ExprState *state, ExprEvalStep *op)
> {
> - ArrayRefState *arefstate = op->d.arrayref.state;
> -
> + SubscriptingRefState *sbsrefstate = op->d.sbsref.state;
> /*
> - * For an assignment to a fixed-length array type, both the original array
> - * and the value to be assigned into it must be non-NULL, else we punt and
> - * return the original array.
> + * For an assignment to a fixed-length container type, both the original
> + * container and the value to be assigned into it must be non-NULL, else we
> + * punt and return the original container.
> */
> - if (arefstate->refattrlength > 0) /* fixed-length array? */
> + if (sbsrefstate->refattrlength > 0)
> {
> - if (*op->resnull || arefstate->replacenull)
> + if (*op->resnull || sbsrefstate->replacenull)
> return;
> }
>
> - /*
> - * For assignment to varlena arrays, we handle a NULL original array by
> - * substituting an empty (zero-dimensional) array; insertion of the new
> - * element will result in a singleton array value. It does not matter
> - * whether the new element is NULL.
> - */
> - if (*op->resnull)
> - {
> - *op->resvalue = PointerGetDatum(construct_empty_array(arefstate->refelemtype));
> - *op->resnull = false;
> - }
> -
> - if (arefstate->numlower == 0)
> - {
> - /* Scalar case */
> - *op->resvalue = array_set_element(*op->resvalue,
> - arefstate->numupper,
> - arefstate->upperindex,
> - arefstate->replacevalue,
> - arefstate->replacenull,
> - arefstate->refattrlength,
> - arefstate->refelemlength,
> - arefstate->refelembyval,
> - arefstate->refelemalign);
> - }
> - else
> - {
> - /* Slice case */
> - *op->resvalue = array_set_slice(*op->resvalue,
> - arefstate->numupper,
> - arefstate->upperindex,
> - arefstate->lowerindex,
> - arefstate->upperprovided,
> - arefstate->lowerprovided,
> - arefstate->replacevalue,
> - arefstate->replacenull,
> - arefstate->refattrlength,
> - arefstate->refelemlength,
> - arefstate->refelembyval,
> - arefstate->refelemalign);
> - }
> + *op->resvalue = FunctionCall2(op->d.sbsref.eval_finfo,
> + PointerGetDatum(*op->resvalue),
> + PointerGetDatum(op));
> }

You might not love me for this suggestion, but I'd like to see the
renaming here split from the rest of the patch. There's a lot of diff
that's just more or less automatic changes, making it hard to see the
actual meaningful changes.

- Andres

Andres Freund <andres(at)anarazel(dot)de> writes:
> You might not love me for this suggestion, but I'd like to see the
> renaming here split from the rest of the patch. There's a lot of diff
> that's just more or less automatic changes, making it hard to see the
> actual meaningful changes.

Yeah, I'm beginning to wonder if we should do the renaming at all.
It's useful for being sure we've found everyplace that needs to change
... but if lots of those places don't actually need more than the
name changes, maybe it's just make-work and code thrashing.

There's a set of other issues that are starting to bother me.
Perhaps it's not in this patch's charter to resolve them, but I think
we need to figure out whether that's true. It's a bit hard to
explain clearly, but let me see how well I can state these:

* The complaint I had about the "container" terminology isn't just
terminology. Rather, there is a bunch of knowledge in the system that
some data types can be found embedded in other types; for one example,
see find_composite_type_dependencies. In the case of standard arrays,
it's clear that the array type does contain its element type in this
sense, and we'd better be aware of that in situations such as applying
DDL that changes the element type. It's much less clear what it means
if you say that type X has a subscripting function that yields type Y.
I think the issue can be ignored as long as Y is not a type mutable by
any provided DDL commands, but is that OK as a permanent restriction?
If not, do we need to do anything about it in the v1 patch? If we don't,
do we need to enforce some restriction on what Y can be for types
other than true arrays?

* There are other identifiable array-specific behaviors that people
might expect a generic subscripting feature to let them into.
For example, if we allow JSONB to be subscripted to obtain TEXT,
does that mean a polymorphic function f(x anyarray) should now match
JSONB? It's even more fun if you decide that subscripting JSONB
should yield JSONB, which is probably a more useful definition than
TEXT really. Then ANYARRAY and ANYELEMENT would need to be the same
type, which is likely to blow holes in the polymorphic type code,
though I've not looked closely for problems. In the same vein, if
JSONB is subscriptable, should "x = ANY(y)" work for y of type JSONB?
I'm not actually sure that we'd want these sorts of things to happen,
even as follow-on extensions. For instance, a polymorphic function
f(x anyarray) would very likely think it can apply array_dims(x) or
iterate from array_lower(x,1) to array_upper(x,1). Providing it
a subscripting function won't get you far if the subscripts aren't
consecutive integers.

* There's an awful lot of places in the backend that call get_element_type
or get_base_element_type or type_is_array or type_is_array_domain, and
aren't touched by the patch as it stands. Some of them might represent
dependencies that we need to worry about that don't fall under either of
the above categories. So just touching the places that mess with ArrayRef
isn't getting us real far in terms of being sure we've considered
everything that needs considering.

regards, tom lane

On Wed, Jan 10, 2018 at 7:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> * The complaint I had about the "container" terminology isn't just
> terminology. Rather, there is a bunch of knowledge in the system that
> some data types can be found embedded in other types; for one example,
> see find_composite_type_dependencies. In the case of standard arrays,
> it's clear that the array type does contain its element type in this
> sense, and we'd better be aware of that in situations such as applying
> DDL that changes the element type. It's much less clear what it means
> if you say that type X has a subscripting function that yields type Y.
> I think the issue can be ignored as long as Y is not a type mutable by
> any provided DDL commands, but is that OK as a permanent restriction?
> If not, do we need to do anything about it in the v1 patch? If we don't,
> do we need to enforce some restriction on what Y can be for types
> other than true arrays?

Well, if I set things up so that subscripting foo by integer returns
bar, that doesn't seem to be much different, from the point of view of
type dependencies, from CREATE FUNCTION some_function_name(foo,
integer) RETURNS bar. I suppose that if this gets stored in the
pg_type entry for foo, that type just develops a dependencies on the
subscripting functions which, I suppose, makes them indirectly
dependent on the return types of those functions. I'm not sure why
that wouldn't be sufficient. It's true that you might be able to
alter the type in some way that would break it, but that's equally
true of the CREATE FUNCTION case:

rhaas=# create table foo (a int, b text);
CREATE TABLE
rhaas=# create or replace function fooify(foo) returns int as $$select
$1.a$$ language sql;
CREATE FUNCTION
rhaas=# select fooify('(1,tgl)'::foo);
fooify
--------
1
(1 row)
rhaas=# alter table foo rename column a to c;
ALTER TABLE
rhaas=# select fooify('(1,tgl)'::foo);
ERROR: column "a" not found in data type foo
LINE 1: select $1.a
^
QUERY: select $1.a
CONTEXT: SQL function "fooify" during inlining

> * There are other identifiable array-specific behaviors that people
> might expect a generic subscripting feature to let them into.
> For example, if we allow JSONB to be subscripted to obtain TEXT,
> does that mean a polymorphic function f(x anyarray) should now match
> JSONB? It's even more fun if you decide that subscripting JSONB
> should yield JSONB, which is probably a more useful definition than
> TEXT really. Then ANYARRAY and ANYELEMENT would need to be the same
> type, which is likely to blow holes in the polymorphic type code,
> though I've not looked closely for problems. In the same vein, if
> JSONB is subscriptable, should "x = ANY(y)" work for y of type JSONB?
> I'm not actually sure that we'd want these sorts of things to happen,
> even as follow-on extensions. For instance, a polymorphic function
> f(x anyarray) would very likely think it can apply array_dims(x) or
> iterate from array_lower(x,1) to array_upper(x,1). Providing it
> a subscripting function won't get you far if the subscripts aren't
> consecutive integers.

Our SQL dialect is statically typed; trying to support duck-typing
seems likely to create a lot of problems. True, we have a single
datatype for one-dimensional and multi-dimensional arrays, but I think
most people would view that as an anti-feature. We also have some
provision for dynamic record types, but it feels pretty kludgy.

> * There's an awful lot of places in the backend that call get_element_type
> or get_base_element_type or type_is_array or type_is_array_domain, and
> aren't touched by the patch as it stands. Some of them might represent
> dependencies that we need to worry about that don't fall under either of
> the above categories. So just touching the places that mess with ArrayRef
> isn't getting us real far in terms of being sure we've considered
> everything that needs considering.

No argument there.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Jan 10, 2018 at 7:09 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> * The complaint I had about the "container" terminology isn't just
>> terminology. Rather, there is a bunch of knowledge in the system that
>> some data types can be found embedded in other types; for one example,
>> see find_composite_type_dependencies.

> Well, if I set things up so that subscripting foo by integer returns
> bar, that doesn't seem to be much different, from the point of view of
> type dependencies, from CREATE FUNCTION some_function_name(foo,
> integer) RETURNS bar.

I think you missed the point. The question is whether the existence of a
subscripting function means that we need to treat the subscriptable type
as physically containing the subscript result type. For example, if the
subscript result type is composite, do we need to do something about a
column of the subscriptable type when somebody does an ALTER TYPE
... ALTER ATTRIBUTE TYPE on the result type? The dependency mechanism
doesn't have enough information to answer that. It's fairly easy to
imagine cases where it wouldn't be true --- for instance, if you had
a subscripting conversion from JSONB to my_composite_type, changing
my_composite_type would likely change the set of JSONB values for which
the subscripting function would succeed, but it wouldn't create a need
to physically rewrite any JSONB columns. But perhaps somebody might
try to build a subscriptable type for which they did need that.

After further thought, I think I'm prepared to say (for the moment) that
only true arrays need be deemed to be containers in this sense. If you
make a subscripting function for anything else, we'll treat it as just a
function that happens to yield the result type but doesn't imply that that
is what is physically stored. Perhaps at some point that will need to
change, but I'm failing to think of near-term use cases where it would be
important to have such a property.

This is, however, a good reason why I don't like the use of "container"
terminology in the patch. I think we want to reserve "container" for
types where physical containment is assumed.

>> * There are other identifiable array-specific behaviors that people
>> might expect a generic subscripting feature to let them into.

> Our SQL dialect is statically typed; trying to support duck-typing
> seems likely to create a lot of problems.

Agreed; that's pretty much what my point was too. I'm just trying
to be clear about how far we expect this capability to reach.

regards, tom lane

On Thu, Jan 11, 2018 at 1:15 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I think you missed the point. The question is whether the existence of a
> subscripting function means that we need to treat the subscriptable type
> as physically containing the subscript result type. For example, if the
> subscript result type is composite, do we need to do something about a
> column of the subscriptable type when somebody does an ALTER TYPE
> ... ALTER ATTRIBUTE TYPE on the result type? The dependency mechanism
> doesn't have enough information to answer that. It's fairly easy to
> imagine cases where it wouldn't be true --- for instance, if you had
> a subscripting conversion from JSONB to my_composite_type, changing
> my_composite_type would likely change the set of JSONB values for which
> the subscripting function would succeed, but it wouldn't create a need
> to physically rewrite any JSONB columns.

I don't think I missed the point at all -- this is the exact same set
of issues that arise with respect to functions. Indeed, I gave an
example of a function that needs to be updated if a column of the
input type is altered. In the case of functions, we've decided that
it's not our problem. If the user updates the composite type and
fails to update the function definitions as needed, things might
break, so they should do that. If they don't, it's not our bug.

> After further thought, I think I'm prepared to say (for the moment) that
> only true arrays need be deemed to be containers in this sense. If you
> make a subscripting function for anything else, we'll treat it as just a
> function that happens to yield the result type but doesn't imply that that
> is what is physically stored. Perhaps at some point that will need to
> change, but I'm failing to think of near-term use cases where it would be
> important to have such a property.

In other words, we're vigorously agreeing.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Jan 11, 2018 at 1:15 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I think you missed the point. The question is whether the existence of a
>> subscripting function means that we need to treat the subscriptable type
>> as physically containing the subscript result type.

> I don't think I missed the point at all -- this is the exact same set
> of issues that arise with respect to functions. Indeed, I gave an
> example of a function that needs to be updated if a column of the
> input type is altered. In the case of functions, we've decided that
> it's not our problem.

Right, but in the case of stored arrays, we've decided that it *is*
our problem (as indeed it must be, because the user has no tools with
which they could fix a representation change for stored data). The
question is to what extent that need would propagate to pseudo array
types.

> In other words, we're vigorously agreeing.

I think we're agreed on what should be in the v1 version of the patch.
I'm not 100% convinced that the problem won't come up eventually.

regards, tom lane

On Thu, Jan 11, 2018 at 1:37 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I don't think I missed the point at all -- this is the exact same set
>> of issues that arise with respect to functions. Indeed, I gave an
>> example of a function that needs to be updated if a column of the
>> input type is altered. In the case of functions, we've decided that
>> it's not our problem.
>
> Right, but in the case of stored arrays, we've decided that it *is*
> our problem (as indeed it must be, because the user has no tools with
> which they could fix a representation change for stored data). The
> question is to what extent that need would propagate to pseudo array
> types.

I think I view the rationale a bit differently. Let's say that a user
defines a composite type as (a int, b text) and uses that composite
type as a column type. Then, somebody tries to change column a to
have type text, and suppose we don't throw an error but simply permit
the operation. If the user now tries to select from the offending
column, the server will very likely crash. In contrast, in the case
where the user has defined an SQL function that selects $1.a and
returns it as an int, they will get a runtime error when they try to
use the function. In my mind, that is the critical difference. An
operation that can cause the server to crash or emit internal errors
must be prohibited, whereas an operation that might cause stuff to
fail with suitable error messages in the future can be allowed.

In other words, the problem isn't that the user has no tools to fix
the problem; it's that, with certain exceptions like superusers
indulging in random catalog-hackery, unprivileged users shouldn't be
allowed to break the world. You might point out that the chances of
break-the-world behavior for type subscripting are pretty high, since
we're slinging around arguments of type internal. But C functions are
always an exception to the notion that we'll trap and report errors.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Jan 11, 2018 at 1:37 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Right, but in the case of stored arrays, we've decided that it *is*
>> our problem (as indeed it must be, because the user has no tools with
>> which they could fix a representation change for stored data). The
>> question is to what extent that need would propagate to pseudo array
>> types.

> I think I view the rationale a bit differently. Let's say that a user
> defines a composite type as (a int, b text) and uses that composite
> type as a column type. Then, somebody tries to change column a to
> have type text, and suppose we don't throw an error but simply permit
> the operation. If the user now tries to select from the offending
> column, the server will very likely crash. In contrast, in the case
> where the user has defined an SQL function that selects $1.a and
> returns it as an int, they will get a runtime error when they try to
> use the function. In my mind, that is the critical difference.

There are two critical differences --- that's one, and the other is
that there are SQL-level ways to fix the problem, ie change the function
text with CREATE OR REPLACE FUNCTION. We don't have a SQL command that
says "now go update the representation of table T column C".

But I think we've probably beaten this topic to death ...

regards, tom lane

On Thu, Jan 11, 2018 at 2:20 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> But I think we've probably beaten this topic to death ...

Yep.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Sorry for late reply. I've attached a new version of the patch with following
changes:

> On 7 January 2018 at 23:39, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Part of the reason why I'm insistent on that is that I think it will
> expose that the division of labor between the core parser and the
> datatype-specific parse function is still a mess.

I moved all logic that busy with determining required input data type into the
datatype specific code (except everything domain related). Unfortunately, we
need some of those types before the custom code can perform validation - to
address this `parse` function now splitted into two phases, initial preparation
(when the custom code can provide all required information), and actual
validation (when everything, e.g. rhs is processed). In general I hope this
approach makes separation of concerns more clear.

> On the executor side of things, I suspect Andres will be unhappy that
> you are making ExprEvalStep part of the API for datatypes --- he
> objected to my exposing it to plpgsql param eval in
> https://postgr.es/m/20171220174243.n4y3hgzf7xd3mm5e@alap3.anarazel.de
> and there was a lot more reason to do so there than there is here, IMO.

I've changes fetch/assign functions so that ExprEvalStep isn't exposed anymore.

> Now on the other hand, maybe the right way to go is to embrace a similar
> approach to what I did for plpgsql param eval, and let the datatype
> control what gets generated as the expression execution step. The main
> point here would be to let the datatype provide the address of a callback
> function that gets executed for a subscripting step, rather than having it
> specify the OID of a pg_proc entry to call. There would be two big wins
> from that:
>
> * The callback function would have a plain C call signature, so we would
> not have to go through FunctionCallN, saving a few cycles. This is
> attractive because it would pretty much eliminate any concern about this
> patch making array access slower at execution time.
>
> * There would no longer be a wired-in restriction that there be two and
> only two subscripting execution functions per datatype, since there would
> not be any need for those functions to be identified in pg_type.
>
> The two disadvantages I can see of approaching things this way are:
>
> * There'd be at least some connection of subscriptable types to
> expression compilation, which is what Andres was objecting to in the
> message I cited above. Possibly we could alleviate that a bit by
> providing helper functions that mask exactly what goes into the
> expression step structs, but I'm not sure that that gets us far.
>
> * We'd not have OIDs of execution functions in the parse trees for
> subscripting operations, which would mean that we would not have
> a convenient way to identify subscripting operations that are
> mutable, parallel-unsafe, or leaky. Probably it'd be fine to assume
> that subscripting is always immutable and parallel-safe, although
> I'm slightly more concerned about whether people would want the
> option to label it leaky vs leakproof. As against that, the approach
> that's there right now adds planning overhead that wasn't there before
> for exactly those function property lookups, and again I'm a bit worried
> about the performance impact. (I did some crude performance tests
> today that indicated that the existing patch has small but measurable
> penalties, maybe on the order of 10 percent; and it'd be more by the
> time we're done because I'm pretty sure you've missed some places that
> ought to check these function properties if we're going to have them.
> So I'm afraid that we'll get pushback from people who don't care about
> extensible subscripts and do care about array performance.)

I tired to apply this approach with callback functions for fetch/assign logic
(these functions I mentioned above for prepare/validate are also implemented
like that). This part is actually can be done more or less independently from
changes above. Is it close to what you suggesting?

As a side note, I'm not sure why this main function should have a signature
with opcode:

subscript_support(int opcode, internal other_info) returns internal

since instead of calling it with different opcode we can just return a set of
callback and execute what's necessary at this particular point.

I haven't evaluated performance of this implementation yet, will do that soon.
But in the meantime I want to align on what can be accepted as the best solution
here.

> Yeah, I'm beginning to wonder if we should do the renaming at all.
> It's useful for being sure we've found everyplace that needs to change
> ... but if lots of those places don't actually need more than the
> name changes, maybe it's just make-work and code thrashing.

I'm strongly in favor of renaming, just because I don't feel comfortable
when a name of a concept is not exactly deliver the meaning. In the current
version I kept the name "container" so far due lack of feasible alternatives.

> While I'm on the topic, I am not really happy with s/array/container/
> as you've done in some of this code. To my mind, "container type"
> includes composite types. Particularly in the parse_target code, where
> we're currently dealing with either composites or arrays, making it say
> that we're dealing with either composites or containers is just a recipe
> for confusion. Unfortunately I can't think of a better word offhand,
> but some attention to this is needed. As far as the comments go,
> we might be able to use the term "subscriptable type", but not sure if
> that will work for function/variable names.
>
> After further thought, I think I'm prepared to say (for the moment) that
> only true arrays need be deemed to be containers in this sense. If you
> make a subscripting function for anything else, we'll treat it as just a
> function that happens to yield the result type but doesn't imply that that
> is what is physically stored. Perhaps at some point that will need to
> change, but I'm failing to think of near-term use cases where it would be
> important to have such a property.
>
> This is, however, a good reason why I don't like the use of "container"
> terminology in the patch. I think we want to reserve "container" for
> types where physical containment is assumed.

I see the point. But to my understanding all other datatypes are "containers"
too, since a subscripting function most likely will return some data from it,
maybe in some transformed form. Arrays are just more strict containers, so
maybe reserve "typed/strict_container" for them?

One more note. In the current version of the patch I haven't updated a tutorial
part, since as I said I want to discuss and have an agreement on details stated
above. Also, this idea about separating renaming stuff from everything else is
actually paid off - I found out few more places where I forgot to revert some
remnants of the implementation with two separated node types. I'm going to fix
them within few days in the next version.

> On 22 January 2018 at 23:38, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
> Sorry for late reply. I've attached a new version of the patch with following
> changes:
>
> I found out few more places where I forgot to revert some remnants of the
> implementation with two separated node types. I'm going to fix them within
> few days in the next version.

Here is a new version of the patch:

* rebased to the latest master

* fixed issues I mentioned above

* updated an example from the tutorial part

Hi,

On Sun, Jan 28, 2018 at 06:26:56PM +0100, Dmitry Dolgov wrote:
>
> Here is a new version of the patch:
>
> * rebased to the latest master
>
> * fixed issues I mentioned above
>
> * updated an example from the tutorial part

I have a few comments.

0002-Base-implementation-of-subscripting-mechanism-v6.patch:

> - if (op->d.arrayref_subscript.isupper)
> - indexes = arefstate->upperindex;
> + if (op->d.sbsref_subscript.isupper)
> + indexes = sbsrefstate->upper;

I think upperindex is better here. There was no need to rename it. Same
for lowerindex/lower.

There are a couple changes which unrelated to the patch. For example:

> - * subscripting. Adjacent A_Indices nodes have to be treated as a single
> + * subscripting. Adjacent A_Indices nodes have to be treated as a single

It is better to avoid it for the sake of decrease size of the patch.

> - * typmod to be applied to the base type. Subscripting a domain is an
> + * typmod to be applied to the base type. Subscripting a domain is an

Same here.

> +/* Non-inline data for container operations */
> +typedef struct SubscriptingRefState
> +{
> + bool isassignment; /* is it assignment, or just fetch? */
> ...
> +} SubscriptingRefState;

It is not good to move up SubscriptingRefState, because it is hard to
see changes between SubscriptingRefState and ArrayRefState.

> + FmgrInfo *eval_finfo; /* function to evaluate subscript */
> + FmgrInfo *nested_finfo; /* function to handle nested assignment */

I think eval_finfo and nested_finfo are not needed anymore.

> +typedef Datum (*SubscriptingFetch) (Datum source, struct SubscriptingRefState *sbsefstate);
> +
> +typedef Datum (*SubscriptingAssign) (Datum source, struct SubscriptingRefState *sbsefstate);

Typo here? Did you mean sbsrefstate in the second argument?

> +typedef struct SbsRoutines
> +{
> + SubscriptingPrepare prepare;
> + SubscriptingValidate validate;
> + SubscriptingFetch fetch;
> + SubscriptingAssign assign;
> +
> +} SbsRoutines;

SbsRoutines is not good name for me. SubscriptRoutines or
SubscriptingRoutines sound better and it is consistent with other
structures.

0005-Subscripting-documentation-v6.patch:

> + <replaceable class="parameter">type_modifier_output_function</replaceable>,
> + <replaceable class="parameter">analyze_function</replaceable>,
> + <replaceable class="parameter">subscripting_handler_function</replaceable>,
> are optional. Generally these functions have to be coded in C

Extra comma here.

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

> On 29 January 2018 at 14:41, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru> wrote:
>
> I have a few comments.

Thanks for suggestions, I've incorporated all of them in a new version of the
patch.

> SbsRoutines is not good name for me. SubscriptRoutines or
> SubscriptingRoutines sound better and it is consistent with other
> structures.

In general I agree, and I thought about that while implementing this structure.
But my concern is that some parts of subscripting infrastructure are annoyingly
verbose, maybe it makes sense to come up with a short abbreviation and use it
everywhere across this code.

> On 30 January 2018 at 16:47, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
> > On 29 January 2018 at 14:41, Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
wrote:
> >
> > I have a few comments.
>
> Thanks for suggestions, I've incorporated all of them in a new version of
the
> patch.

Few more updates. I've attached a new version with some minor changes,
mostly
about moving a subscripting depth check to type related logic. Also I've
made
some performance tests for arrays using pgbench:

pgbench -c 10 -j 2 -T 600 -f test.sql -r

with queries like:

select (ARRAY[1, 2, 3])[0];
select (ARRAY[1, 2, 3, ..., 98, 99, 100])[0];
select (ARRAY[[[[[[1]]]]]])[1][1][1][1][1][1];
select (ARRAY[[[[[[1, 2, 3]]]]]])[1][1][1][1][1:2];

and the difference in average latency was about 2%:

* with the patch

number of transactions actually processed: 349211
latency average = 1.718 ms
tps = 5820.048783 (including connections establishing)
tps = 5820.264433 (excluding connections establishing)

* without the patch

number of transactions actually processed: 356024
latency average = 1.685 ms
tps = 5933.538195 (including connections establishing)
tps = 5934.124599 (excluding connections establishing)

> On 22 February 2018 at 18:30, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
>
> Few more updates. I've attached a new version with some minor changes,
> mostly
> about moving a subscripting depth check to type related logic. Also I've
> made
> some performance tests for arrays using pgbench:
>
> pgbench -c 10 -j 2 -T 600 -f test.sql -r
>
> with queries like:
>
> select (ARRAY[1, 2, 3])[0];
> select (ARRAY[1, 2, 3, ..., 98, 99, 100])[0];
> select (ARRAY[[[[[[1]]]]]])[1][1][1][1][1][1];
> select (ARRAY[[[[[[1, 2, 3]]]]]])[1][1][1][1][1:2];
>
> and the difference in average latency was about 2%:
>
> * with the patch
>
> number of transactions actually processed: 349211
> latency average = 1.718 ms
> tps = 5820.048783 (including connections establishing)
> tps = 5820.264433 (excluding connections establishing)
>
> * without the patch
>
> number of transactions actually processed: 356024
> latency average = 1.685 ms
> tps = 5933.538195 (including connections establishing)
> tps = 5934.124599 (excluding connections establishing)
>

One more small update after fd1a421fe6 in attachments.

On Tue, Mar 6, 2018 at 6:21 PM, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:

>
> One more small update after fd1a421fe6 in attachments.
>

Before looking at the code I have a few comments about documentation:

in json.sgml:

+-- Extract value by key
+SELECT ('{"a": 1}'::jsonb)['a'];

What is the result of running this query? What is the resulting data type?

+-- 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'];

What is the result here? Why subscript is a string and not a number? Are
subscription indexes 0- or 1-based?

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

Please capitalize: SET, FROM, WHERE.

Use of double quotes around "value" requires some explanation, I think.
Should the user expect that a suitable index is used by the query planner
for this query?

In other words, I would like to see this part of documentation to be
extended beyond just showcasing the syntax.

Regards,
--
Oleksandr "Alex" Shulgin | Database Engineer | Zalando SE | Tel: +49 176
127-59-707

> On 20 March 2018 at 11:09, Oleksandr Shulgin <oleksandr(dot)shulgin(at)zalando(dot)de> wrote:
>> On Tue, Mar 6, 2018 at 6:21 PM, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
>>
>>
>> One more small update after fd1a421fe6 in attachments.
>
>
> Before looking at the code I have a few comments about documentation:
>
> ...
>
> In other words, I would like to see this part of documentation to be
> extended beyond just showcasing the syntax.

Good point, thanks for noticing. The thing is that the implementation of
subscripting for jsonb data type in this patch relies on the `setPath` function
and follows the same rules as e.g. `jsonb_set`, but I need to mention this
explicitly in the documentation. Speaking about your questions:

> +-- Extract value by key
> +SELECT ('{"a": 1}'::jsonb)['a'];
>
> What is the result of running this query? What is the resulting data type?
>

Jsonb subscripting expression always returns another jsonb

> +-- Extract element by index
> +SELECT ('[1, "2", null]'::jsonb)['1'];
>
> What is the result here? Why subscript is a string and not a number? Are
> subscription indexes 0- or 1-based?
>

For jsonb arrays an index is 0 based. It's also not necessary to have an index
as a string in this situation (so `data['1']` and `data[1]` are actually equal)

> +-- Select records using where clause with subscripting
> +SELECT * from table_name where jsonb_field['key'] = '"value"';
>
> Use of double quotes around "value" requires some explanation, I think.

In case of comparison, since a subscripting expression returns something of
jsonb data type, we're going to compare two objects of type jsonb. Which means
we need to convert 'value' to a jsonb scalar, and for that purpose it should be
in double quotes.

> Should the user expect that a suitable index is used by the query planner
> for this query?

There is no specific indexing support for subscripting expressions, so if you
need you can create a functional index using it.

Here is the updated version of patch, rebased after recent conflicts and with
suggested documentation improvements.

> On 22 March 2018 at 23:25, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
>
> Here is the updated version of patch, rebased after recent conflicts and with
> suggested documentation improvements.

Another rebased version of the patch.

> On Thu, 26 Apr 2018 at 16:44, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
>
> > On 22 March 2018 at 23:25, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
> >
> > Here is the updated version of patch, rebased after recent conflicts and with
> > suggested documentation improvements.
>
> Another rebased version of the patch.

I've noticed, that I never updated llvmjit code for the arrayref expressions,
and it's important to do so, since the patch introduces another layer of
flexibility. Hence here is the new version.

> On Fri, 20 Jul 2018 at 23:32, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
>
> > On Thu, 26 Apr 2018 at 16:44, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
> >
> > > On 22 March 2018 at 23:25, Dmitry Dolgov <9erthalion6(at)gmail(dot)com> wrote:
> > >
> > > Here is the updated version of patch, rebased after recent conflicts and with
> > > suggested documentation improvements.
> >
> > Another rebased version of the patch.
>
> I've noticed, that I never updated llvmjit code for the arrayref expressions,
> and it's important to do so, since the patch introduces another layer of
> flexibility. Hence here is the new version.

Here is another rebased version, and a bit of history: the first prototypes of
this patch were sent more than 3 years ago. Of course the patch evolved
significantly over this period, and I take it as a good sign that it wasn't
rejected and keeps moving through the commitfests. At the same time the lack of
attention makes things a bit frustrating. I have an impression that it's sort
of regular situation and wonder if there are any ideas (besides the well known
advice of putting some efforts into review patches from other people, since I'm
already doing my best and enjoying this) how to make progress in such cases?

ne 30. 9. 2018 v 0:21 odesílatel Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
napsal:

> > On Fri, 20 Jul 2018 at 23:32, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
> wrote:
> >
> > > On Thu, 26 Apr 2018 at 16:44, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
> wrote:
> > >
> > > > On 22 March 2018 at 23:25, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
> wrote:
> > > >
> > > > Here is the updated version of patch, rebased after recent conflicts
> and with
> > > > suggested documentation improvements.
> > >
> > > Another rebased version of the patch.
> >
> > I've noticed, that I never updated llvmjit code for the arrayref
> expressions,
> > and it's important to do so, since the patch introduces another layer of
> > flexibility. Hence here is the new version.
>
> Here is another rebased version, and a bit of history: the first
> prototypes of
> this patch were sent more than 3 years ago. Of course the patch evolved
> significantly over this period, and I take it as a good sign that it wasn't
> rejected and keeps moving through the commitfests. At the same time the
> lack of
> attention makes things a bit frustrating. I have an impression that it's
> sort
> of regular situation and wonder if there are any ideas (besides the well
> known
> advice of putting some efforts into review patches from other people,
> since I'm
> already doing my best and enjoying this) how to make progress in such
> cases?
>

This feature looks nice, and it can be great when some values of some not
atomic type should be updated.

Regards

Pavel

Hi

ne 30. 9. 2018 v 8:23 odesílatel Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
napsal:

>
>
> ne 30. 9. 2018 v 0:21 odesílatel Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
> napsal:
>
>> > On Fri, 20 Jul 2018 at 23:32, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
>> wrote:
>> >
>> > > On Thu, 26 Apr 2018 at 16:44, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
>> wrote:
>> > >
>> > > > On 22 March 2018 at 23:25, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
>> wrote:
>> > > >
>> > > > Here is the updated version of patch, rebased after recent
>> conflicts and with
>> > > > suggested documentation improvements.
>> > >
>> > > Another rebased version of the patch.
>> >
>> > I've noticed, that I never updated llvmjit code for the arrayref
>> expressions,
>> > and it's important to do so, since the patch introduces another layer of
>> > flexibility. Hence here is the new version.
>>
>> Here is another rebased version, and a bit of history: the first
>> prototypes of
>> this patch were sent more than 3 years ago. Of course the patch evolved
>> significantly over this period, and I take it as a good sign that it
>> wasn't
>> rejected and keeps moving through the commitfests. At the same time the
>> lack of
>> attention makes things a bit frustrating. I have an impression that it's
>> sort
>> of regular situation and wonder if there are any ideas (besides the well
>> known
>> advice of putting some efforts into review patches from other people,
>> since I'm
>> already doing my best and enjoying this) how to make progress in such
>> cases?
>>
>
> This feature looks nice, and it can be great when some values of some not
> atomic type should be updated.
>

I am playing with this feature little bit

I have one idea - can be possible to use integer subscript for record
fields? It can helps with iteration over record.

example:

select ('{"a":{"a":[10,20]}}'::jsonb)[0];--> NULL, but can be more
practical if it returns same like select
('{"a":{"a":[10,"20"]}}'::jsonb)['a'];

I don't like quite ignoring bad subsript in update

postgres=# insert into test(v) values( '[]');
INSERT 0 1
postgres=# update test set v[1000] = 'a';
UPDATE 1
postgres=# update test set v[1000] = 'a';
UPDATE 1
postgres=# update test set v[1000] = 'a';
UPDATE 1
postgres=# select * from test;
┌────┬─────────────────┐
│ id │ v │
╞════╪═════════════════╡
│ │ ["a", "a", "a"] │
└────┴─────────────────┘
(1 row)