Re: [HACKERS] [PATCH] Generic type subscripting

From: Arthur Zakirov <a(dot)zakirov(at)postgrespro(dot)ru>
To: Dmitry Dolgov <9erthalion6(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, Andres Freund <andres(at)anarazel(dot)de>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Oleg Bartunov <obartunov(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, David Steele <david(at)pgmasters(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [HACKERS] [PATCH] Generic type subscripting
Date: 2018-01-29 13:41:04
Message-ID: 20180129134103.GA8314@zakirov.localdomain
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Simon Riggs 2018-01-29 13:49:56 Re: Logical Decoding and HeapTupleSatisfiesVacuum assumptions
Previous Message Tomas Vondra 2018-01-29 13:34:24 Re: Logical Decoding and HeapTupleSatisfiesVacuum assumptions