Horton

Re: [PATCH] Generic type subscripting

Artur Zakirov <a.zakirov@postgrespro.ru>

View thread View on postgr.es
From: Arthur Zakirov <a.zakirov@postgrespro.ru>
To: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, David Steele <david@pgmasters.net>, peter.eisentraut@2ndquadrant.com, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2017-04-05T13:06:04Z
Lists: pgsql-hackers 
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

Commits

  1. Throw error when assigning jsonb scalar instead of a composite object

  2. Filling array gaps during jsonb subscripting

  3. Implementation of subscripting for jsonb

  4. Allow ALTER TYPE to update an existing type's typsubscript value.

  5. Allow subscripting of hstore values.

  6. Support subscripting of arbitrary types, not only arrays.

  7. jit: Reference function pointer types via llvmjit_types.c.

  8. Teach contain_leaked_vars that assignment SubscriptingRefs are leaky.

  9. jit: Correct parameter type for generated expression evaluation functions.

  10. Renaming for new subscripting mechanism

  11. Fix assertion failure for SSL connections.

  12. Teach eval_const_expressions() to handle some more cases.