Date: Sun, 19 Jul 2020 14:10:16 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Dmitry Dolgov <9erthalion6@gmail.com>
Cc: Pavel Stehule <pavel.stehule@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
	David Steele <david@pgmasters.net>,
	Nikita Glukhov <n.gluhov@postgrespro.ru>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	David Fetter <david@fetter.org>,
	Thomas Munro <thomas.munro@gmail.com>,
	Thomas Munro <thomas.munro@enterprisedb.com>,
	Oleksandr Shulgin <oleksandr.shulgin@zalando.de>,
	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>,
	pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] [PATCH] Generic type subscripting
Message-ID: <20200719191016.GW23581@telsasoft.com>
References: <20200213131228.i6afwfmjdod7obko@localhost>
 <dd01b2ba-b652-8252-0cc3-89e5708339fa@pgmasters.net>
 <20200304170339.g4xgydtxtmgfajbe@localhost>
 <CAFj8pRAX0hAEqo7zvyH=k8egrEQhYV9e-r7h3e4aEfpvJME2KQ@mail.gmail.com>
 <CAFj8pRDQwPD2v+2E+jg+ttDKr5k2o=OAw-bLd5Asj5ds28_YTQ@mail.gmail.com>
 <10957.1584899227@sss.pgh.pa.us>
 <CAFj8pRAHcASw+XP44tf8H5vk+oRud1mNhF8XYwaOYAeY32+XZQ@mail.gmail.com>
 <23946.1584906094@sss.pgh.pa.us>
 <CAFj8pRCcP2R67j=T60E+gRNf8aiv_L9m7JTnn5N7ptAscyY26Q@mail.gmail.com>
 <20200516122802.g4qbfxkr2zr7nlv2@localhost>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="ZcSDOt7TZFizttpn"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20200516122802.g4qbfxkr2zr7nlv2@localhost>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


--ZcSDOt7TZFizttpn
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On Sat, May 16, 2020 at 02:28:02PM +0200, Dmitry Dolgov wrote:
> > On Mon, Mar 23, 2020 at 07:30:25AM +0100, Pavel Stehule wrote:
> > ne 22. 3. 2020 v 20:41 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
> > > Pavel Stehule <pavel.stehule@gmail.com> writes:
> > > > ne 22. 3. 2020 v 18:47 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
> > > >> cfbot reports this as failing because of missing include files.
> > > >> Somebody please post a complete patch set?
> > >
> > > > here it is
> 
> One more rebase to prepare for 2020-07.

I found some minor comment typos.

I don't think it's enough to claim a beer:

+ * container. If you have read until this point, and will submit a meaningful
+ * review of this patch series, I'll owe you a beer at the next PGConfEU.

I'm not sure I understand what this is saying:

+        * It's necessary only for field selection, since for
+        * subscripting it's custom code who should define types.

should maybe say: "its custom code should define types."
(no apostrophe is "possessive")

-- 
Justin

--ZcSDOt7TZFizttpn
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment; filename=0007-fix-Polymorphic-subscriptingtxt

From 6c3fad421619526765127571570e0bcbc7fe1679 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 19 Jul 2020 13:10:21 -0500
Subject: [PATCH 7/9] fix! Polymorphic subscripting

---
 src/backend/utils/adt/jsonfuncs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 77687e7f71..04d3078f68 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -1750,7 +1750,7 @@ push_null_elements(JsonbParseState **ps, int num)
 		pushJsonbValue(ps, WJB_ELEM, &null);
 }
 
-/* Perfrom one subscript assignment step */
+/* Perform one subscript assignment step */
 static void
 jsonb_subscript_step_assignment(SubscriptingRefState *sbstate, Datum value,
 								Oid typid, int num, bool isupper)
@@ -1831,7 +1831,7 @@ jsonb_subscript_step_assignment(SubscriptingRefState *sbstate, Datum value,
 		pushJsonbValue(&astate->ps, WJB_KEY, &key);
 	}
 
-	/* If the value does exits, process and validate its type. */
+	/* If the value does exist, process and validate its type. */
 	if (subscript[1].exists)
 	{
 		if (jbv.type == jbvArray)
-- 
2.17.0


--ZcSDOt7TZFizttpn
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
 filename=0008-fix-Subscripting-documentationtxt

From df3d19b401943758577bf939b07b9f338fc81ff3 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 19 Jul 2020 13:24:42 -0500
Subject: [PATCH 8/9] fix! Subscripting documentation

---
 doc/src/sgml/json.sgml            | 6 +++---
 doc/src/sgml/ref/create_type.sgml | 2 +-
 doc/src/sgml/xsubscripting.sgml   | 8 ++++----
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml
index 3bffe8049b..5c538dca05 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -606,8 +606,8 @@ SELECT jdoc-&gt;'guid', jdoc-&gt;'name' FROM api WHERE jdoc @&gt; '{"tags": ["qu
   <title><type>jsonb</type> Subscripting</title>
   <para>
    <type>jsonb</type> data type supports array-style subscripting expressions
-   to extract or update particular element. It's possible to use multiple
-   subscripting expressions to extract nested values. In this case a chain of
+   to extract or update particular elements. It's possible to use multiple
+   subscripting expressions to extract nested values. In this case, a chain of
    subscripting expressions follows the same rules as the
    <literal>path</literal> argument in <literal>jsonb_set</literal> function,
    e.g. in case of arrays it is a 0-based operation or that negative integers
@@ -634,7 +634,7 @@ SELECT * FROM table_name WHERE jsonb_field['key'] = '"value"';
 </programlisting>
 
   There is no special indexing support for such kind of expressions, but you
-  always can create a functional index that includes it
+  can always create a functional index that includes it
 <programlisting>
 CREATE INDEX idx ON table_name ((jsonb_field['key']));
 </programlisting>
diff --git a/doc/src/sgml/ref/create_type.sgml b/doc/src/sgml/ref/create_type.sgml
index ec67761c66..a34df4d247 100644
--- a/doc/src/sgml/ref/create_type.sgml
+++ b/doc/src/sgml/ref/create_type.sgml
@@ -470,7 +470,7 @@ CREATE TYPE <replaceable class="parameter">name</replaceable>
    and jsonb
    (<replaceable class="parameter">jsonb_subscripting_handler</replaceable>)
    types in <filename>src/backend/utils/adt/arrayfuncs.c</filename> and
-   <filename>src/backend/utils/adt/jsonfuncs.c</filename> corresponding.
+   <filename>src/backend/utils/adt/jsonfuncs.c</filename>, respectively.
   </para>
   </refsect2>
 
diff --git a/doc/src/sgml/xsubscripting.sgml b/doc/src/sgml/xsubscripting.sgml
index d701631223..7224e81fa2 100644
--- a/doc/src/sgml/xsubscripting.sgml
+++ b/doc/src/sgml/xsubscripting.sgml
@@ -7,8 +7,8 @@
     <primary>custom subscripting</primary>
   </indexterm>
   <para>
-  When you define a new base type, you can also specify a custom procedures to
-  handle subscripting expressions. They must contain logic for verification and
+  When you define a new base type, you can also specify a custom procedure to
+  handle subscripting expressions. It must contain logic for verification and
   evaluation of this expression, i.e. fetching or updating some data in this
   data type. For instance:
 </para>
@@ -63,12 +63,12 @@ custom_subscript_assign(Datum containerSource, SubscriptingRefState *sbstate)
 Datum
 custom_subscript_fetch(Datum containerSource, SubscriptingRefState *sbstate)
 {
-    // Some fetch logic based on sbsdata
+    // Some fetch logic based on sbstate
 }]]>
 </programlisting>
 
 <para>
-    Then you can define a subscripting procedures and a custom data type:
+    Then you can define a subscripting procedure and a custom data type:
 </para>
 <programlisting>
 CREATE FUNCTION custom_subscripting_handler(internal)
-- 
2.17.0


--ZcSDOt7TZFizttpn
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
 filename=0009-fix-Base-implementation-of-subscripting-mechanismtxt

From cdbbc489f04a9359c0c7c38f6fc2050b6283b678 Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Sun, 19 Jul 2020 13:38:27 -0500
Subject: [PATCH 9/9] fix! Base implementation of subscripting mechanism

---
 src/backend/commands/typecmds.c   |  4 ++--
 src/backend/parser/parse_node.c   | 10 +++++-----
 src/backend/parser/parse_target.c |  2 +-
 src/include/catalog/pg_type.h     |  2 +-
 src/include/parser/parse_node.h   |  2 +-
 5 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 8a1703c871..cd6855643f 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -1915,7 +1915,7 @@ findTypeSubscriptingFunction(List *procname, Oid typeOid, bool parseFunc)
 	if (parseFunc)
 	{
 		/*
-		 * Subscripting function parse always take two INTERNAL argument and
+		 * Subscripting parse functions always take two INTERNAL arguments and
 		 * return INTERNAL.
 		 */
 		argList[0] = INTERNALOID;
@@ -1924,7 +1924,7 @@ findTypeSubscriptingFunction(List *procname, Oid typeOid, bool parseFunc)
 	else
 	{
 		/*
-		 * Subscripting functions fetch/assign always take one typeOid
+		 * Subscripting fetch/assign functions always take one typeOid
 		 * argument, one INTERNAL argument and return typeOid.
 		 */
 		argList[0] = typeOid;
diff --git a/src/backend/parser/parse_node.c b/src/backend/parser/parse_node.c
index 3710d01aa2..d1c4ea8573 100644
--- a/src/backend/parser/parse_node.c
+++ b/src/backend/parser/parse_node.c
@@ -184,7 +184,7 @@ pcb_error_callback(void *arg)
  * transformContainerType()
  *		Identify the types involved in a subscripting operation for container
  *
- * On entry, containerType/containerTypmod  are modified if necessary to
+ * On entry, containerType/containerTypmod are modified if necessary to
  * identify the actual container type and typmod.
  */
 void
@@ -227,13 +227,13 @@ transformContainerType(Oid *containerType, int32 *containerTypmod)
  * with the source data inserted into the right part of the container.
  *
  * For both cases, this function contains only general subscripting logic while
- * type-specific logic (e.g. type verifications and coersion) is placen in
- * separate procedure indicated by typsubshandler. There is only one exception
- * for now about domain-over-container, if the source container is of a
+ * type-specific logic (e.g. type verifications and coercion) is placed in
+ * separate procedures indicated by typsubshandler. There is only one exception
+ * for now about domain-over-container: if the source container is of a
  * domain-over-container type, the result is of the base container type or its
  * element type; essentially, we must fold a domain to its base type before
  * applying subscripting. (Note that int2vector and oidvector are treated as
- * domains here.) An error will appear in case if current container type
+ * domains here.) An error will appear in the case the current container type
  * doesn't have a subscripting procedure.
  *
  * pstate			Parse state
diff --git a/src/backend/parser/parse_target.c b/src/backend/parser/parse_target.c
index 3ca0791500..73a8c88c12 100644
--- a/src/backend/parser/parse_target.c
+++ b/src/backend/parser/parse_target.c
@@ -949,7 +949,7 @@ transformAssignmentSubscripts(ParseState *pstate,
 										 rhs,
 										 location);
 
-	/* Provide fully prepared subscriptinng information for custom validation */
+	/* Provide fully prepared subscripting information for custom validation */
 	sbsref->refassgnexpr = (Expr *) rhs;
 	sbsroutines->validate(rhs != NULL, sbsref, pstate);
 
diff --git a/src/include/catalog/pg_type.h b/src/include/catalog/pg_type.h
index 5dbdc880dc..ea237ec61d 100644
--- a/src/include/catalog/pg_type.h
+++ b/src/include/catalog/pg_type.h
@@ -222,7 +222,7 @@ CATALOG(pg_type,1247,TypeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(71,TypeRelati
 	Oid			typcollation BKI_DEFAULT(0) BKI_LOOKUP(pg_collation);
 
 	/*
-	 * Type specific subscripting logic. If typsubshandler is none, it means
+	 * Type specific subscripting logic. If typsubshandler is NULL, it means
 	 * that this type doesn't support subscripting.
 	 */
 	regproc		typsubshandler BKI_DEFAULT(-) BKI_LOOKUP(pg_proc);
diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h
index f0786ac570..b4736206d1 100644
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -325,7 +325,7 @@ extern SubscriptingRef *transformContainerSubscripts(ParseState *pstate,
 													 int32 containerTypMod,
 													 List *indirection,
 													 Node *assignFrom);
-extern SubscriptRoutines* getSubscriptingRoutines(Oid containerType);
+extern SubscriptRoutines *getSubscriptingRoutines(Oid containerType);
 extern Const *make_const(ParseState *pstate, Value *value, int location);
 
 #endif							/* PARSE_NODE_H */
-- 
2.17.0


--ZcSDOt7TZFizttpn--