Horton

Re: [HACKERS] [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>, Robert Haas <robertmhaas@gmail.com>, Andres Freund <andres@anarazel.de>, Michael Paquier <michael.paquier@gmail.com>, Oleg Bartunov <obartunov@gmail.com>, Peter Eisentraut <peter.eisentraut@2ndquadrant.com>, David Steele <david@pgmasters.net>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2018-01-29T13:41:04Z
Lists: pgsql-hackers 
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

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.