Thread: [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

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 

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

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 
https://www.postgresql.org/message-id/55D24517.8080609%40agliodbs.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

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

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

As I mentioned above [1] in my humble opinion this patch is not at all in a
"good shape" until it breaks existing extensions.

[1] http://postgr.es/m/20161115080324.GA5351%40e733.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

2016-12-27 14:42 GMT+05:00 Dmitry Dolgov <9erthalion6@gmail.com>:
>> 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:

I suppose that a separate node type could solve it. But I'm not
convinced about how to distinguish ArrayRef node with new
SubscriptingRef node. Maybe it could be done in the
transformIndirection() function. If I understand all correctly.

Also Tom pointed that he had bad experience with using ArrayRef node:
https://www.postgresql.org/message-id/518.1439846343%40sss.pgh.pa.us
> No.  Make a new expression node type.
>
> (Salesforce did something similar for an internal feature, and it was a
> disaster both for code modularity and performance.  We had to change it to
> a separate node type, which I just got finished doing.  Don't go down that
> path.  While you're at it, I'd advise that fetch and assignment be two
> different node types rather than copying ArrayRef's bad precedent of using
> only one.)

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

> Yes, but it was related to the idea of having `ArrayRef` and `JsonbRef` nodes
> for specific types. Since now there is generic `SubscriptingRef` node, I think
> it should be ok.

Sorry I misunderstood it.

> Just to be clear - as far as I understood, these compilation problems were
> caused not because the extension knew something about ArrayRef node in
> particular, but because the extension tried to extract all nodes to generate
> code from them. It means any change will require "refetching", so I think it's
> natural for this extension.

Agree. It will be hard to maintain both nodes. And it is not so smart
to have two nodes ArrayRef (deprecated) and SubscriptingRef. It is not
hard to replace ArrayRef node with SubscriptingRef  in other
extensions.

There is a little note:

>  #include "utils/lsyscache.h"
> +#include "utils/syscache.h"
>  #include "utils/memutils.h"

I think "utils/syscache.h" isn't necessary here. PostgreSQL could be
compiled without this include.

I suppose that this patch can be marked as "Ready for commiter". Any opinions?

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

Dmitry Dolgov <9erthalion6@gmail.com> writes:
> [ generic_type_subscription_v6.patch ]

Not too surprisingly, this doesn't apply anymore in the wake of commit
ea15e1867.  Could you rebase?  Changes for that should be pretty trivial
I'd expect.

I took an extremely quick look over the patch --- mostly looking
at the header file changes not the code --- and have a couple of
comments:

1. As I mentioned previously, it's a seriously bad idea that ArrayRef
is used for both array subscripting and array assignment.  Let's fix
that while we're at it, rather than setting that mistake in even more
stone by embedding it in a datatype extension API.

2. I'm not very pleased that the subscripting functions have signature
"subscripting(internal) returns internal"; we have more than enough of
those already, and each one is a hazard for plugging the wrong function
into the wrong place.  Worse yet, you're flat out lying about the number
of arguments that these functions actually expect to receive, which is
something that could get broken by any number of plausible future changes.
Can we arrange to do that differently?  I'd prefer something in which the
argument and result types are visibly connected to the actual datatypes
at hand, for instance  array_subscript(anyarray, internal) returns anyelement  array_assign(anyarray, internal,
anyelement)returns anyarray
 
where the "internal" argument is some representation of only the subscript
expressions.  This would allow CREATE TYPE to perform some amount of
checking that the right function(s) had been specified.  (If that means
we use two functions not one per datatype, that's fine with me.)  If that
seems impractical, let's at least pick a signature that doesn't conflict
with any other INTERNAL-using APIs, and preferably has some connection
to what the arguments really are.

3. The contents of ArrayRef are designed on the assumption that the same
typmod and collation values apply to both an array and its elements.
That's okay for standard arrays, but I do not think it holds for every
other container situation.  For example, hstore doesn't have collation
last I checked, but it would likely want to treat its element type as
being text, which does.  So this needs to be generalized.

4. It looks like your work on the node processing infrastructure has been
limited to s/ArrayRef/SubscriptingRef/g, but that's not nearly enough.
SubscriptingRef needs to be regarded as an opportunity to invoke a
user-defined function, which means that it now acts quite a bit like
FuncExpr.  For example, the function-to-be-invoked needs to be checked for
volatility, leakproofness, parallel safety, etc in operations that want to
know about such things.  So check_functions_in_node(), for one, needs to
consider SubscriptingRef, and really you'll have to look at everyplace
that deals with FuncExpr and see if it needs a case for SubscriptingRef
now.  I'd advise adding the OID of the subscripting function to
SubscriptingRef, so that those places don't need to do additional catalog
lookups to get it.

BTW, a different approach that might be worth considering is to say that
the nodetree representation of one of these things *is* a FuncExpr, and
the new magic thing is just that we invent a new CoercionForm value
which causes ruleutils.c to print the expression as a subscripting op.
I'm not quite convinced that that's a good idea --- a "coercion format"
that says "subscript" seems a bit weird --- but it would greatly reduce
the number of places you'd have to touch.
        regards, tom lane

I wrote:
> ... (If that means
> we use two functions not one per datatype, that's fine with me.)

Actually, after thinking about that a bit more: you've really squeezed
*three* different APIs into one function.  Namely, subscript-reference
parse analysis, array subscripting execution, and array assignment
execution.  It would be cleaner, and would reduce runtime overhead a bit,
if those were embodied as three separate functions.

It might be possible to get away with having only one pg_type column,
pointing at the parse-analysis function.  That function would generate
a SubscriptingRef tree node containing the OID of the appropriate
execution function, which execQual.c could call without ever knowing
its name explicitly.

This clearly would work for built-in types, since the parse-analysis
function could rely on fmgroids.h for the OIDs of the execution functions.
But I'm less sure if it would work in extensions, which won't have
predetermined OIDs for their functions.  Is there a way for a function
in an extension to find the OID of one of its sibling functions?
        regards, tom lane

On 1/23/17 1:36 PM, Tom Lane wrote:
> Is there a way for a function
> in an extension to find the OID of one of its sibling functions?

Obviously there's regprocedure (or it's C equivalent), but then you're 
stuck re-computing at runtime. I've messed around with that a bit in an 
effort to have an extension depend on another extension that allows the 
user to specify it's schema. If you're already doing metaprogramming 
it's not an enormous problem... if you're not already doing that it 
sucks. Trying to make that work in C would be somewhere between 
impossible and a nightmare.

Since this kind of thing affects extensions that depend on extensions, 
it'd certainly be nice if there was some way to address it.

BTW, I actually do use SPI to call one of the reg casts in my variant 
type, but that's just a hack I used in the beginning and haven't gotten 
around to replacing. Since there's a static variable that gets set to 
the relevant OID it's not that bad performance-wise from what I can 
tell, but I suspect that's not something we want to be recommending to 
others...
-- 
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)

I wrote:
> BTW, a different approach that might be worth considering is to say that
> the nodetree representation of one of these things *is* a FuncExpr, and
> the new magic thing is just that we invent a new CoercionForm value
> which causes ruleutils.c to print the expression as a subscripting op.
> I'm not quite convinced that that's a good idea --- a "coercion format"
> that says "subscript" seems a bit weird --- but it would greatly reduce
> the number of places you'd have to touch.

After sleeping on it, that approach is starting to sound better to me.
Consider a design like this:

* Leave ArrayRef strictly alone, and introduce new infrastructure beside
it for non-array containers.  That sounds ugly at first, but it has two
significant advantages: you don't have to refactor or even touch any
array-related code, and you do not have to worry about somebody objecting
to the patch because it adds unacceptable overhead to existing array
operations.  Converting array ops to go through a function-call API surely
must add *some* overhead, and at this point we don't have enough info to
be certain it would be negligibly small.  (Testing the existing patch
cannot prove that, since as I noted yesterday, you're missing a lot of
plan-time manipulations that need to happen for a generic function call.)

* Let the node representation for non-array-container access be FuncExpr
with new value(s) of funcformat.  You'd need to design the exact
representation to be used for the subscript arguments, but that doesn't
seem horribly complicated.  In this way, you're not on the hook to
duplicate all the node-processing infrastructure for either ArrayRef
or FuncExpr --- ideally, you won't need to do much more in the core
system than provide the parse-time callout and write suitable deparsing
logic in ruleutils.c.

The ugliness of thinking that "subscripting" is a kind of coercion could
be dealt with by changing funcformat to some new enum type named something
like, say, FuncDisplayForm.  This would require touching all the places
that mess with funcformat, but that could be a good thing anyway, because
you'd be sure that you'd looked at everything that might need changes.
        regards, tom lane

On Mon, Jan 23, 2017 at 2:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Can we arrange to do that differently?  I'd prefer something in which the
> argument and result types are visibly connected to the actual datatypes
> at hand, for instance
>           array_subscript(anyarray, internal) returns anyelement
>           array_assign(anyarray, internal, anyelement) returns anyarray

What about having no internal arguments here at all?  Like if you want
to support foo[4] define a subscript function that takes (mytype, int)
and returns whatever.  You might have to allow for multiple
subscripting functions with different argument types for this to
really work, though.

/me ducks

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jan 23, 2017 at 2:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Can we arrange to do that differently?  I'd prefer something in which the
>> argument and result types are visibly connected to the actual datatypes
>> at hand, for instance
>> array_subscript(anyarray, internal) returns anyelement
>> array_assign(anyarray, internal, anyelement) returns anyarray

> What about having no internal arguments here at all?  Like if you want
> to support foo[4] define a subscript function that takes (mytype, int)
> and returns whatever.  You might have to allow for multiple
> subscripting functions with different argument types for this to
> really work, though.

Yeah, the problem is if you're trying to support the full generality
of the array slice syntax, it gets kind of painful to shoehorn that
into simple SQL types.  Consider array[1][2:3][4:] or something like
that.  We could say that extensions only have access to the basic
non-slice notation, but I'm sure someone would be unhappy with that
--- and even then, you don't really want to define six functions to
deal with six possible numbers of subscripts.

Another issue is that, if someone is trying to use this facility to define
array semantics that are less screwball than what Berkeley bequeathed us,
they might not be happy with the idea that a single function
"array_subscript(anyarray, internal) returns anyelement" is what
determines the result type of a subscripting operation.  It might be for
example that you need to look at the subscripted object, as well as the
number of subscripts, before you know whether the result is an element or
a lower-dimension array.  So I'd really prefer that the functionality
involve a parser callout, and that would certainly need "internal"
argument(s).

Given that it's a parser callout, what the signatures of the runtime
function(s) are is really not our problem; the callout can construct
any darn expression tree it pleases.  There would only be some subset
of that space that ruleutils.c would know how to print as a subscripting
construct, but many people might be happy with the results reverse-listing
as a regular function or operator call.
        regards, tom lane

On Thu, Jan 26, 2017 at 1:38 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> So I'd really prefer that the functionality
> involve a parser callout, and that would certainly need "internal"
> argument(s).

Thanks, I see now.

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

On Sat, Jan 28, 2017 at 2:31 AM, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>> On 24 January 2017 at 02:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I took an extremely quick look over the patch
>
> Thank you for the feedback. It took some time for me to think about all
> suggestions and notes.

Okay, I am marking the patch as returned with feedback seeing those
reviews. Please don't hesitate to submit a new version.
-- 
Michael