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
