On 04.04.2017 15:41, Dmitry Dolgov wrote:
> Sorry for late reply. Here is a new version of the patch, I rebased it and
> fixed those issues you've mentioned (pretty nasty problems, thank you for
> noticing).
Thank you!
I've looked at the patch again.
I'd like to focus on "refevalfunc" and "refnestedfunc" fields as I did
earlier. I think using Oid type for them is a bad approach. "..._fetch"
and "..._assign" functions in catalog is unnecessary movement to me.
User of subscript of his type may think the same. But he won't see the
code and won't know why he needs these functions.
And so "..._fetch" and "..._assign" functions in catalog is a bad design
to me. But, of course, it is just my opinion. This approach is the main
think which we should resolve first, because after commiting the patch
it will be hard to fix it.
> static int ArrayCount(const char *str, int *dim, char typdelim);
> +bool isAssignmentIndirectionExpr(ExprState *exprstate);
> static void ReadArrayStr(char *arrayStr, const char *origStr,
I think isAssignmentIndirectionExpr() here was forgoten to delete,
because isAssignmentIndirectionExpr() is in execExpr.c now.
> + if (subexpr == NULL)
> + {
> + lowerIndexpr = lappend(lowerIndexpr, subexpr);
> + continue;
> + }
> +
> +
There is the extra line here after the brace.
> if (array_type != sbsref->refcontainertype)
> {
>
> node = coerce_to_target_type(pstate,
> node, array_type,
> sbsref->refcontainertype, sbsref->reftypmod,
> COERCION_ASSIGNMENT,
> COERCE_IMPLICIT_CAST,
> -1);
>
> /* can fail if we had int2vector/oidvector, but not for true domains */
> if (node == NULL && node->type != 0)
> ereport(ERROR,
> (errcode(ERRCODE_CANNOT_COERCE),
> errmsg("cannot cast type %s to %s",
> format_type_be(array_type),
> format_type_be(sbsref->refcontainertype)),
> parser_errposition(pstate, 0)));
>
> PG_RETURN_POINTER(node);
> }
Also I was wondering do we need this code in array_subscript_parse()? I
haven't understood the purpose of it. If it is necessary then would be
good to add explain comment.
--
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company