Re: [PATCH] Generic type subscripting

From: Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
To: Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
Cc: Oleg Bartunov <obartunov(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, David Steele <david(at)pgmasters(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [PATCH] Generic type subscripting
Date: 2017-11-07 20:00:43
Message-ID: CA+q6zcX3nL_u3hf7Ofwb=ERANgZMX+PBJf0u96X3+bAR9Db=kQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
0001-Base-implementation-of-subscripting-mechanism.patch application/octet-stream 168.1 KB
0002-Subscripting-for-array.patch application/octet-stream 12.1 KB
0003-Subscripting-for-jsonb.patch application/octet-stream 32.2 KB
0004-Subscripting-documentation.patch application/octet-stream 20.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2017-11-07 20:41:36 Re: Moving relation extension locks out of heavyweight lock manager
Previous Message Andres Freund 2017-11-07 19:32:00 Re: [POC] Faster processing at Gather node