Re: [HACKERS] [PATCH] Generic type subscripting

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