[PATCH] Generic type subscription

On 9/9/16 6:29 AM, Dmitry Dolgov wrote:

Regarding to the previous conversations [1], [2], here is a patch (with some
improvements from Nikita Glukhov) for the generic type subscription.

Awesome! Please make sure to add it to the Commit Fest app.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532) mobile: 512-569-9461

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

On Fri, 9 Sep 2016 18:29:23 +0700
Dmitry Dolgov <9erthalion6@gmail.com> wrote:

Hi,

Regarding to the previous conversations [1], [2], here is a patch
(with some improvements from Nikita Glukhov) for the generic type
subscription. It allows
to define type-specific subscription logic for any data type and has
implementations for the array and jsonb types. There are following
changes in this
patch:

I've tried to compile this patch with current state of master (commit
51c3e9fade76c12) and found out that, when configured with
--enable-cassert, it doesn't pass make check.

LOG: server process (PID 4643) was terminated by signal 6: Aborted
DETAIL: Failed process was running:
update test_jsonb_subscript set test_json['a'] = '{"b":
1}'::jsonb;

--
Victor

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

I've tried to compile this patch with current state of master (commit
51c3e9fade76c12) and found out that, when configured with

--enable-cassert,

it doesn't pass make check.

Thanks for the feedback. Yes, unexpectedly for me, `ExecEvalExpr` can return
expanded `jbvArray` and `jbvObject` instead `jbvBinary` in both cases. It's
interesting, that this doesn't break anything, but obviously violates
the `pushJsonbValueScalar` semantics. I don't think `ExecEvalExpr` should be
changed for jsonb, we can handle this situation in `pushJsonbValue`
instead. I've
attached a new version of patch with this modification.

On 27 September 2016 at 19:08, Victor Wagner <vitus@wagner.pp.ru> wrote:

Show quoted text

On Fri, 9 Sep 2016 18:29:23 +0700
Dmitry Dolgov <9erthalion6@gmail.com> wrote:

Hi,

Regarding to the previous conversations [1], [2], here is a patch
(with some improvements from Nikita Glukhov) for the generic type
subscription. It allows
to define type-specific subscription logic for any data type and has
implementations for the array and jsonb types. There are following
changes in this
patch:

I've tried to compile this patch with current state of master (commit
51c3e9fade76c12) and found out that, when configured with
--enable-cassert, it doesn't pass make check.

LOG: server process (PID 4643) was terminated by signal 6: Aborted
DETAIL: Failed process was running:
update test_jsonb_subscript set test_json['a'] = '{"b":
1}'::jsonb;

--
Victor

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

On Sat, 1 Oct 2016 16:52:34 +0700
Dmitry Dolgov <9erthalion6@gmail.com> wrote:

I've tried to compile this patch with current state of master
(commit 51c3e9fade76c12) and found out that, when configured with

--enable-cassert,

it doesn't pass make check.

Thanks for the feedback. Yes, unexpectedly for me, `ExecEvalExpr` can
return expanded `jbvArray` and `jbvObject` instead `jbvBinary` in
both cases. It's interesting, that this doesn't break anything, but
obviously violates the `pushJsonbValueScalar` semantics. I don't
think `ExecEvalExpr` should be changed for jsonb, we can handle this
situation in `pushJsonbValue` instead. I've
attached a new version of patch with this modification.

Thanks for you quick responce. I've not yet thoroughly investigated new
patch, but already noticed some minor promlems:

Git complains about whitespace in this version of patch:

$ git apply ../generic_type_subscription_v2.patch
../generic_type_subscription_v2.patch:218: tab in indent.
int first;
../generic_type_subscription_v2.patch:219: tab in indent.
int second;
../generic_type_subscription_v2.patch:225: tab in indent.
SubscriptionExecData *sbsdata = (SubscriptionExecData *) PG_GETARG_POINTER(1);
../generic_type_subscription_v2.patch:226: tab in indent.
Custom *result = (Custom *) sbsdata->containerSource;
../generic_type_subscription_v2.patch:234: tab in indent.
SubscriptionRef *sbsref = (SubscriptionRef *) PG_GETARG_POINTER(0);
warning: squelched 29 whitespace errors
warning: 34 lines add whitespace errors.

It doesn't prevent me from further testing of patch, but worth noticing.

--

Victor

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

On Sat, Oct 1, 2016 at 12:52 PM, Dmitry Dolgov <9erthalion6@gmail.com>
wrote:

I've tried to compile this patch with current state of master (commit
51c3e9fade76c12) and found out that, when configured with

--enable-cassert,

it doesn't pass make check.

Thanks for the feedback. Yes, unexpectedly for me, `ExecEvalExpr` can
return
expanded `jbvArray` and `jbvObject` instead `jbvBinary` in both cases. It's
interesting, that this doesn't break anything, but obviously violates
the `pushJsonbValueScalar` semantics. I don't think `ExecEvalExpr` should
be
changed for jsonb, we can handle this situation in `pushJsonbValue`
instead. I've
attached a new version of patch with this modification.

have you ever run 'make check' ?

=========================
53 of 168 tests failed.
=========================

Show quoted text

On 27 September 2016 at 19:08, Victor Wagner <vitus@wagner.pp.ru> wrote:

On Fri, 9 Sep 2016 18:29:23 +0700
Dmitry Dolgov <9erthalion6@gmail.com> wrote:

Hi,

Regarding to the previous conversations [1], [2], here is a patch
(with some improvements from Nikita Glukhov) for the generic type
subscription. It allows
to define type-specific subscription logic for any data type and has
implementations for the array and jsonb types. There are following
changes in this
patch:

I've tried to compile this patch with current state of master (commit
51c3e9fade76c12) and found out that, when configured with
--enable-cassert, it doesn't pass make check.

LOG: server process (PID 4643) was terminated by signal 6: Aborted
DETAIL: Failed process was running:
update test_jsonb_subscript set test_json['a'] = '{"b":
1}'::jsonb;

--
Victor

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

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

On 5 October 2016 at 03:00, Oleg Bartunov <obartunov@gmail.com> wrote:

have you ever run 'make check' ?

=========================
53 of 168 tests failed.
=========================

Sure, how else could I write tests for this feature? But right now on my
machine
everything is ok (the same for `make installcheck`):

$ make check
....
=======================
All 168 tests passed.
=======================

On Wed, Oct 5, 2016 at 6:48 AM, Dmitry Dolgov <9erthalion6@gmail.com> wrote:

On 5 October 2016 at 03:00, Oleg Bartunov <obartunov@gmail.com> wrote:

have you ever run 'make check' ?

=========================
53 of 168 tests failed.
=========================

Sure, how else could I write tests for this feature? But right now on my
machine
everything is ok (the same for `make installcheck`):

$ make check
....
=======================
All 168 tests passed.
=======================

Oops, something was wrong in my setup :)

Hello,

On 04.10.2016 11:28, Victor Wagner wrote:

Git complains about whitespace in this version of patch:

$ git apply ../generic_type_subscription_v2.patch
../generic_type_subscription_v2.patch:218: tab in indent.
int first;
../generic_type_subscription_v2.patch:219: tab in indent.
int second;
../generic_type_subscription_v2.patch:225: tab in indent.
SubscriptionExecData *sbsdata = (SubscriptionExecData *) PG_GETARG_POINTER(1);
../generic_type_subscription_v2.patch:226: tab in indent.
Custom *result = (Custom *) sbsdata->containerSource;
../generic_type_subscription_v2.patch:234: tab in indent.
SubscriptionRef *sbsref = (SubscriptionRef *) PG_GETARG_POINTER(0);
warning: squelched 29 whitespace errors
warning: 34 lines add whitespace errors.

It doesn't prevent me from further testing of patch, but worth noticing.

I agree with Victor. In sgml files whitespaces instead of tabs are
usually used.

I've looked at your patch to make some review.

"subscription"
--------------

The term "subscription" is confusing me. I'm not native english speaker.
But I suppose that this term is not related with the "subscript". So
maybe you should use the "subscripting" or "container" (because you
already use the "container" term in the code). For example:

T_SubscriptingRef
SubscriptingRef
deparseSubscriptingRef()
xsubscripting.sgml
CREATE TYPE custom (
internallength = 4,
input = custom_in,
output = custom_out,
subscripting = custom_subscripting
);
etc.

Subscripting interface
----------------------

In tests I see the query:

+select ('[1, "2", null]'::jsonb)['1'];
+ jsonb
+-------
+ "2"
+(1 row)

Here '1' is used as a second item index. But from the last discussion
/messages/by-id/55D24517.8080609@agliodbs.com it
should be a key:

There is one ambiguous case you need to address:

testjson = '{ "a" : { } }'

SET testjson['a']['a1']['1'] = 42

... so in this case, is '1' a key, or the first item of an array? how
do we determine that? How does the user assign something to an array?

And should return null. Is this right? Or this behaviour was not
accepted in discussion? I didn't find it.

+Datum
+custom_subscription(PG_FUNCTION_ARGS)
+{
+	int						op_type = PG_GETARG_INT32(0);
+	FunctionCallInfoData	target_fcinfo = get_slice_arguments(fcinfo, 1,
+																fcinfo->nargs);
+
+	if (op_type & SBS_VALIDATION)
+		return custom_subscription_prepare(&target_fcinfo);
+
+	if (op_type & SBS_EXEC)
+		return custom_subscription_evaluate(&target_fcinfo);
+
+	elog(ERROR, "incorrect op_type for subscription function: %d", op_type);
+}

I'm not sure but maybe we should use here two different functions? For
example:

Datum
custom_subscripting_validate(PG_FUNCTION_ARGS)
{
}

Datum
custom_subscripting_evaluate(PG_FUNCTION_ARGS)
{
}

CREATE TYPE custom (
internallength = 8,
input = custom_in,
output = custom_out,
subscripting_validate = custom_subscripting_validate,
subscripting_evalute = custom_subscripting_evaluate,
);

What do you think?

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

+<!-- doc/src/sgml/xsubscription.sgml -->
+
+ <sect1 id="xsubscription">
+  <title>User-defined subscription procedure</title>

There is the extra whitespace before <sect1>.

+  </indexterm>
+  When you define a new base type, you can also specify a custom procedure
+  to handle subscription expressions. It should contains logic for verification
+  and for extraction or update your data. For instance:

I suppose a <para> tag is missed after the </indexterm>.

+</programlisting>
+
+ </para>
+
+  <para>

So </para> is redundant here.

Code
----

+static Oid
+findTypeSubscriptionFunction(List *procname, Oid typeOid)
+{
+	Oid			argList[1];

Here typeOid argument is not used. Is it should be here?

+ExecEvalSubscriptionRef(SubscriptionRefExprState *sbstate,

I think in this function local arguments have a lot of tabs. It is
normal if not all variables is aligned on the same line. A lot of tabs
are occur in other functions too. Because of this some lines exceed 80
characters.

+	if (sbstate->refupperindexpr != NULL)
+		upper = (Datum *) palloc(sbstate->refupperindexpr->length * sizeof(Datum *));
+
+	if (sbstate->reflowerindexpr != NULL)
+		lower = (Datum *) palloc(sbstate->reflowerindexpr->length * sizeof(Datum *));

Could we use palloc() before the "foreach(l, sbstate->refupperindexpr)"
? I think it could be a little optimization.

-transformArrayType(Oid *arrayType, int32 *arrayTypmod)
+transformArrayType(Oid *containerType, int32 *containerTypmod)

I think name of the function is confusing. Maybe use
transformContainerType()?

+JsonbValue *
+JsonbToJsonbValue(Jsonb *jsonb, JsonbValue *val)
+{

In this function we could palloc JsonbValue every time and don't use val
argument. And maybe better to copy all JsonbContainer from jsonb->root
using memcpy(). Instead of assigning reference to jsonb->root. As is the
case in JsonbValueToJsonb().

It is necessary to remove the notice about JsonbToJsonbValue() in
JsonbValueToJsonb() comments.

-static JsonbValue *
+JsonbValue *
setPath(JsonbIterator **it, Datum *path_elems,

Why did you remove static keyword? setPath() is still static.

-	JsonbValue	v;
+	JsonbValue	v, *new = (JsonbValue *) newval;

Is declaration of "new" variable necessary here? I think it is extra
declaration. Also its name may bring some problems. For example, there
is a thread where guys try to port PostgreSQL to C++.

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

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

On 5 October 2016 at 22:59, Artur Zakirov <a.zakirov@postgrespro.ru> wrote:
I've looked at your patch to make some review.

Thanks for the feedback.

On 04.10.2016 11:28, Victor Wagner wrote: Git complains about whitespace

in

this version of patch:

Fixed.

The term "subscription" is confusing me

Yes, you're right. "container" is too general I think, so I renamed
everything
to "subscripting".

Here '1' is used as a second item index. But from the last discussion
/messages/by-id/55D24517.8080609@agliodbs.com it
should be a key

Well, I missed that, since I used already implemented function "setPath",
and
this function implies that "all path elements before the last must already
exist", so in this case:

select jsonb_set('{"a": {}}', '{a, a1, 1}', '42');

nothing will be changed. I agree, we can implement this as discussed, but
we need
to do it inside "setPath", so it will be like "globally".

I'm not sure but maybe we should use here two different functions?

I thought about that, but finally decided to keep changes into "pg_type" as
small as possible (anyway, these two functions will serve one logical
purpose).

Here typeOid argument is not used. Is it should be here?

No, it shouldn't, fixed. It's interesting, that the same is correct for
"findTypeAnalyzeFunction" (I used this function as an example).

I think name of the function is confusing. Maybe use
transformContainerType()?

The point is that "transformArrayType" still contains some array-specific
code,
although it doesn't affect to any other type. So I think it's not completely
correct to use "transformContainerType" name.

Why did you remove static keyword? setPath() is still static
Is declaration of "new" variable necessary here?

I changed it back.

Here is a new version of patch.

Hello,

Do you have an updated version of the patch?

2016-10-18 20:41 GMT+03:00 Dmitry Dolgov <9erthalion6@gmail.com>:

The term "subscription" is confusing me

Yes, you're right. "container" is too general I think, so I renamed
everything
to "subscripting".

There is another occurrences of "subscription" including file names. Please
fix them.

Also I've sent a personal email, that we have performance degradation of
SELECT queries:

create table test (
a int2[],
b int4[][][]);

With patch:

=> insert into test (a[1:5], b[1:1][1:2][1:2])
select '{1,2,3,4,5}', '{{{0,0},{1,2}}}'
from generate_series(1,100000);
Time: 936,285 ms

=> UPDATE test SET a[0] = '2';
Time: 1605,406 ms (00:01,605)

=> UPDATE test SET b[1:1][1:1][1:2] = '{113, 117}';
Time: 1603,076 ms (00:01,603)

=> explain analyze select a[1], b[1][1][1] from test;
QUERY PLAN

------------------------------------------------------------
-------------------------------------------------
Seq Scan on test (cost=0.00..3858.00 rows=100000 width=6) (actual
time=0.035..241.280 rows=100000 loops=1)
Planning time: 0.087 ms
Execution time: 246.916 ms
(3 rows)

And without patch:

=> insert into test (a[1:5], b[1:1][1:2][1:2])
select '{1,2,3,4,5}', '{{{0,0},{1,2}}}'
from generate_series(1,100000);
Time: 971,677 ms

=> UPDATE test SET a[0] = '2';
Time: 1508,262 ms (00:01,508)

=> UPDATE test SET b[1:1][1:1][1:2] = '{113, 117}';
Time: 1473,459 ms (00:01,473)

=> explain analyze select a[1], b[1][1][1] from test;
QUERY PLAN

------------------------------------------------------------
------------------------------------------------
Seq Scan on test (cost=0.00..5286.00 rows=100000 width=6) (actual
time=0.024..98.475 rows=100000 loops=1)
Planning time: 0.081 ms
Execution time: 105.055 ms
(3 rows)

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

Hi

Sorry for late reply and thank you for the feedback (especially about the
problem with performance).

There is another occurrences of "subscription" including file names

Fixed.

we have performance degradation of SELECT queries

Yes, I figured it out. The main problem was about type info lookups, some of
them were made for each data extraction operation, which is unnecessary.
Here are results on my machine:

with the fixed version of patch

=# explain analyze select a[1], b[1][1][1] from test;
QUERY PLAN

------------------------------------------------------------------------------------------------------------
Seq Scan on test (cost=0.00..5286.00 rows=100000 width=6) (actual
time=0.950..48.370 rows=100000 loops=1)
Planning time: 0.054 ms
Execution time: 51.859 ms
(3 rows)

and without the patch

=# explain analyze select a[1], b[1][1][1] from test;

QUERY PLAN

------------------------------------------------------------------------------------------------------------
Seq Scan on test (cost=0.00..5286.00 rows=100000 width=6) (actual
time=3.443..43.452 rows=100000 loops=1)
Planning time: 0.117 ms
Execution time: 46.875 ms
(3 rows)

It's still slightly slower because of all new logic aroung subscripting
operation, but not that much.

Hello.

I took a look on the latest -v4 patch. I would like to note that this
patch breaks a backward compatibility. For instance sr_plan extension[1]https://github.com/postgrespro/sr_plan
stop to compile with errors like:

```
serialize.c:38:2: error: unknown type name ‘ArrayRef’
JsonbValue *ArrayRef_ser(const ArrayRef *node, JsonbParseState *state,
bool sub_object);
^
serialize.c:913:2: error: unknown type name ‘ArrayRef’
JsonbValue *ArrayRef_ser(const ArrayRef *node, JsonbParseState *state,
bool sub_object)
^
In file included from sr_plan.h:4:0,
from serialize.c:1:

...

```

Other extensions could be affected as well. I'm not saying that it's a
fatal drawback, but it's definitely something you should be aware of.

I personally strongly believe that we shouldn't break extensions between
major releases or at least make it trivial to properly rewrite them.
Unfortunately it's not a case currently.

[1]: https://github.com/postgrespro/sr_plan

--
Best regards,
Aleksander Alekseev

On 15 November 2016 at 15:03, Aleksander Alekseev <

a.alekseev@postgrespro.ru> wrote:

Hello.

I took a look on the latest -v4 patch. I would like to note that this
patch breaks a backward compatibility. For instance sr_plan extension[1]
stop to compile with errors

Thank you for the feedback.

Well, if we're speaking about this particular extension, if I understood
correctly, it fetches all parse tree nodes from Postgres and generates code
using this information. So to avoid compilation problems I believe you need
to
run `make USE_PGXS=1 genparser` again (it worked for me, I don't see any
mentions of `ArrayRef`).

But speaking generally, I don't see how we can provide backward
compatibility
for those extensions, who are strongly coupled with implementation details
of
parsing tree. I mean, in terms of interface it's mostly about to replace
`ArrayRef` to `SubscriptingRef`, but I think it's better to do it in the
extension code.

On Thu, Nov 17, 2016 at 10:56 PM, Dmitry Dolgov <9erthalion6@gmail.com>
wrote:

On 15 November 2016 at 15:03, Aleksander Alekseev <

a.alekseev@postgrespro.ru> wrote:

Hello.

I took a look on the latest -v4 patch. I would like to note that this
patch breaks a backward compatibility. For instance sr_plan extension[1]
stop to compile with errors

Thank you for the feedback.

Well, if we're speaking about this particular extension, if I understood
correctly, it fetches all parse tree nodes from Postgres and generates code
using this information. So to avoid compilation problems I believe you
need to
run `make USE_PGXS=1 genparser` again (it worked for me, I don't see any
mentions of `ArrayRef`).

But speaking generally, I don't see how we can provide backward
compatibility
for those extensions, who are strongly coupled with implementation details
of
parsing tree. I mean, in terms of interface it's mostly about to replace
`ArrayRef` to `SubscriptingRef`, but I think it's better to do it in the
extension code.

Moved to next CF with "needs review" status.

Regards,
Hari Babu
Fujitsu Australia

On 5 December 2016 at 12:03, Haribabu Kommi <kommi.haribabu@gmail.com>

wrote:

Moved to next CF with "needs review" status.

Looks like we stuck here little bit. Does anyone else have any
suggestions/improvements, or this patch is in good enough shape?

2016-12-26 18:49 GMT+03:00 Dmitry Dolgov <9erthalion6@gmail.com>:

On 5 December 2016 at 12:03, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

Moved to next CF with "needs review" status.

Looks like we stuck here little bit. Does anyone else have any
suggestions/improvements, or this patch is in good enough shape?

Would you rebase the patch, please? It seems it is necessary. It can't
be applied now.

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

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

On 27 December 2016 at 04:05, Artur Zakirov <a.zakirov@postgrespro.ru>

wrote:

Would you rebase the patch, please? It seems it is necessary. It can't
be applied now.

Sure, here is a new rebased version.

As I mentioned above [1]/messages/by-id/20161115080324.GA5351@e733.localdomain in my humble opinion this patch is not at all in a
"good shape" until it breaks existing extensions.

[1]: /messages/by-id/20161115080324.GA5351@e733.localdomain

On Mon, Dec 26, 2016 at 10:49:30PM +0700, Dmitry Dolgov wrote:

On 5 December 2016 at 12:03, Haribabu Kommi <kommi.haribabu@gmail.com>

wrote:

Moved to next CF with "needs review" status.

Looks like we stuck here little bit. Does anyone else have any
suggestions/improvements, or this patch is in good enough shape?

--
Best regards,
Aleksander Alekseev

On 27 December 2016 at 16:09, Aleksander Alekseev <

a.alekseev@postgrespro.ru> wrote:

until it breaks existing extensions.

Hm...I already answered, that I managed to avoid compilation problems for
this particular extension
using the `genparser` command again:

On Thu, Nov 17, 2016 at 10:56 PM, Dmitry Dolgov

<9erthalion6(at)gmail(dot)com>

wrote:

On 15 November 2016 at 15:03, Aleksander Alekseev <

a(dot)alekseev(at)postgrespro(dot)ru> wrote:

Hello.

I took a look on the latest -v4 patch. I would like to note that this
patch breaks a backward compatibility. For instance sr_plan extension[1]
stop to compile with errors

Thank you for the feedback.

Well, if we're speaking about this particular extension, if I understood
correctly, it fetches all parse tree nodes from Postgres and generates

code

using this information. So to avoid compilation problems I believe you

need to

run `make USE_PGXS=1 genparser` again (it worked for me, I don't see any
mentions of `ArrayRef`).

But speaking generally, I don't see how we can provide backward
compatibility for those extensions, who are strongly coupled with

implementation details

of parsing tree. I mean, in terms of interface it's mostly about to

replace

`ArrayRef` to `SubscriptingRef`, but I think it's better to do it in the

extension code.

Or is there something else that I missed?