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