public inbox for [email protected]  
help / color / mirror / Atom feed
[PATCH v43 2/2] Filling gaps in jsonb
9+ messages / 4 participants
[nested] [flat]

* [PATCH v43 2/2] Filling gaps in jsonb
@ 2020-12-31 14:19  Dmitrii Dolgov <[email protected]>
  0 siblings, 0 replies; 9+ messages in thread

From: Dmitrii Dolgov @ 2020-12-31 14:19 UTC (permalink / raw)

Introduces two new modes for jsonb assignment:

* Appending array elements on the specified position, gaps filled with
nulls (similar to JavaScript behavior). This mode also instructs to
create the whole path in a jsonb object if some part of the path (more
than just the last element) is not present.

* Assigning keeps array positions consistent by prevent prepending of
elements.

Originally proposed by Nikita Glukhov based on polymorphic subscripting
patch, but transformed into an independent change.
---
 doc/src/sgml/json.sgml              |  33 ++++
 src/backend/utils/adt/jsonfuncs.c   | 226 ++++++++++++++++++++++++++--
 src/test/regress/expected/jsonb.out | 135 +++++++++++++++++
 src/test/regress/sql/jsonb.sql      |  81 ++++++++++
 4 files changed, 460 insertions(+), 15 deletions(-)

diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml
index 100d1a60f4..9af015d222 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -645,6 +645,39 @@ UPDATE table_name SET jsonb_field['a'] = '1';
 
 -- If jsonb_field here is NULL, the result is [1]
 UPDATE table_name SET jsonb_field[0] = '1';
+</programlisting>
+
+   Jsonb assignment via subscripting handles few edge cases differently
+   from <literal>jsonb_set</literal>. When assigning to the jsonb array
+   to the specified index, but there are no other elements present, the
+   result will be a jsonb array with the ewn value by specified index and
+   <type>null</type> elements from the first index to the specified index.
+
+<programlisting>
+-- If jsonb_field is [], the result is [null, null, 2]
+UPDATE table_name SET jsonb_field[2] = '2';
+</programlisting>
+
+   When assigning to the jsonb array to the specified index, but position
+   of the last element in the array is less than the specified index, the
+   result will be a jsonb array with the new value by specified index and
+   <type>null</type> elements from the last index to the specified index.
+
+<programlisting>
+-- If jsonb_field is [0], the result is [0, null, 2]
+UPDATE table_name SET jsonb_field[2] = '2';
+</programlisting>
+
+   When assigning using the path which is not present in the source jsonb,
+   the result will be a jsonb with the specified path created and the new
+   value at the end of the path.
+
+<programlisting>
+-- If jsonb_field is {}, the result is {'a': [{'b': 1}]}
+UPDATE table_name SET jsonb_field['a'][0]['b'] = '1';
+
+-- If jsonb_field is [], the result is [{'a': 1}]
+UPDATE table_name SET jsonb_field[0]['a'] = '1';
 </programlisting>
 
   </para>
diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c
index 5a0ba6b220..f14f6c3191 100644
--- a/src/backend/utils/adt/jsonfuncs.c
+++ b/src/backend/utils/adt/jsonfuncs.c
@@ -44,6 +44,8 @@
 #define JB_PATH_INSERT_AFTER			0x0010
 #define JB_PATH_CREATE_OR_INSERT \
 	(JB_PATH_INSERT_BEFORE | JB_PATH_INSERT_AFTER | JB_PATH_CREATE)
+#define JB_PATH_FILL_GAPS				0x0020
+#define JB_PATH_CONSISTENT_POSITION		0x0040
 
 /* state for json_object_keys */
 typedef struct OkeysState
@@ -1634,14 +1636,116 @@ jsonb_set_element(Jsonb* jb, Datum *path, int path_len,
 
 	it = JsonbIteratorInit(&jb->root);
 
-	res = setPath(&it, path, path_nulls, path_len, &state, 0,
-				  newval, JB_PATH_CREATE);
+	res = setPath(&it, path, path_nulls, path_len, &state, 0, newval,
+				  JB_PATH_CREATE | JB_PATH_FILL_GAPS |
+				  JB_PATH_CONSISTENT_POSITION);
 
 	pfree(path_nulls);
 
 	PG_RETURN_JSONB_P(JsonbValueToJsonb(res));
 }
 
+static void
+push_null_elements(JsonbParseState **ps, int num)
+{
+		JsonbValue	null;
+
+		null.type = jbvNull;
+
+		while (num-- > 0)
+				pushJsonbValue(ps, WJB_ELEM, &null);
+}
+
+/*
+ * Prepare a new structure containing nested empty objects and arrays
+ * corresponding to the specified path, and assign a new value at the end of
+ * this path. E.g. the path [a][0][b] with the new value 1 will produce the
+ * structure {a: [{b: 1}]}.
+ *
+ * Called is responsible to make sure such path does not exist yet.
+ */
+static void
+push_path(JsonbParseState **st, int level, Datum *path_elems,
+		  bool *path_nulls, int path_len, JsonbValue *newval)
+{
+	/*
+	 * tpath contains expected type of an empty jsonb created at each level
+	 * higher or equal than the current one, either jbvObject or jbvArray.
+	 * Since it contains only information about path slice from level to the
+	 * end, the access index must be normalized by level.
+	 */
+	enum jbvType *tpath = palloc0((path_len - level) * sizeof(enum jbvType));
+	long		 lindex;
+	JsonbValue	 newkey;
+
+	/*
+	 * Create first part of the chain with beginning tokens. For the current
+	 * level WJB_BEGIN_OBJECT/WJB_BEGIN_ARRAY was already created, so start
+	 * with the next one.
+	 */
+	for(int i = level + 1; i < path_len; i++)
+	{
+		char   	   *c, *badp;
+
+		if (path_nulls[i])
+			break;
+
+		/*
+		 * Try to convert to an integer to find out the expected type,
+		 * object or array.
+		 */
+		c = TextDatumGetCString(path_elems[i]);
+		errno = 0;
+		lindex = strtol(c, &badp, 10);
+		if (errno != 0 || badp == c || *badp != '\0' || lindex > INT_MAX ||
+			lindex < INT_MIN)
+		{
+			/* text, an object is expected */
+			newkey.type = jbvString;
+			newkey.val.string.len = VARSIZE_ANY_EXHDR(path_elems[i]);
+			newkey.val.string.val = VARDATA_ANY(path_elems[i]);
+
+			(void) pushJsonbValue(st, WJB_BEGIN_OBJECT, NULL);
+			(void) pushJsonbValue(st, WJB_KEY, &newkey);
+
+			tpath[i - level] = jbvObject;
+		}
+		else
+		{
+			/* integer, an array is expected */
+			(void) pushJsonbValue(st, WJB_BEGIN_ARRAY, NULL);
+
+			push_null_elements(st, lindex);
+
+			tpath[i - level] = jbvArray;
+		}
+
+	}
+
+	/* Insert an actual value for either an object or array */
+	if (tpath[(path_len - level) - 1] == jbvArray)
+	{
+		(void) pushJsonbValue(st, WJB_ELEM, newval);
+	}
+	else
+		(void) pushJsonbValue(st, WJB_VALUE, newval);
+
+	/*
+	 * Close everything up to the last but one level. The last one will be
+	 * closed outside of this function.
+	 */
+	for(int i = path_len - 1; i > level; i--)
+	{
+		if (path_nulls[i])
+			break;
+
+		if (tpath[i - level] == jbvObject)
+			(void) pushJsonbValue(st, WJB_END_OBJECT, NULL);
+		else
+			(void) pushJsonbValue(st, WJB_END_ARRAY, NULL);
+	}
+}
+
 /*
  * Return the text representation of the given JsonbValue.
  */
@@ -4782,6 +4886,21 @@ IteratorConcat(JsonbIterator **it1, JsonbIterator **it2,
  * Bits JB_PATH_INSERT_BEFORE and JB_PATH_INSERT_AFTER in op_type
  * behave as JB_PATH_CREATE if new value is inserted in JsonbObject.
  *
+ * If JB_PATH_FILL_GAPS bit is set, this will change an assignment logic in
+ * case if target is an array. The assignment index will not be restricted by
+ * number of elements in the array, and if there are any empty slots between
+ * last element of the array and a new one they will be filled with nulls. If
+ * the index is negative, it still will be considered an an index from the end
+ * of the array. Of a part of the path is not present and this part is more
+ * than just one last element, this flag will instruct to create the whole
+ * chain of corresponding objects and insert the value.
+ *
+ * JB_PATH_CONSISTENT_POSITION for an array indicates that the called wants to
+ * keep values with fixed indices. Indices for existing elements could be
+ * changed (shifted forward) in case if the array is prepended with a new value
+ * and a negative index out of the range, so this behavior will be prevented
+ * and return an error.
+ *
  * All path elements before the last must already exist
  * whatever bits in op_type are set, or nothing is done.
  */
@@ -4876,6 +4995,8 @@ setPathObject(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
 			memcmp(k.val.string.val, VARDATA_ANY(path_elems[level]),
 				   k.val.string.len) == 0)
 		{
+			done = true;
+
 			if (level == path_len - 1)
 			{
 				/*
@@ -4895,7 +5016,6 @@ setPathObject(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
 					(void) pushJsonbValue(st, WJB_KEY, &k);
 					(void) pushJsonbValue(st, WJB_VALUE, newval);
 				}
-				done = true;
 			}
 			else
 			{
@@ -4940,6 +5060,31 @@ setPathObject(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
 			}
 		}
 	}
+
+	/*
+	 * If we got here there are only few possibilities:
+	 * - no target path was found, and an open object with some keys/values was
+	 *   pushed into the state
+	 * - an object is empty, only WJB_BEGIN_OBJECT is pushed
+	 *
+	 * In both cases if instructed to create the path when not present,
+	 * generate the whole chain of empty objects and insert the new value
+	 * there.
+	 */
+	if (!done && (op_type & JB_PATH_FILL_GAPS) && (level < path_len - 1))
+	{
+		JsonbValue	 newkey;
+
+		newkey.type = jbvString;
+		newkey.val.string.len = VARSIZE_ANY_EXHDR(path_elems[level]);
+		newkey.val.string.val = VARDATA_ANY(path_elems[level]);
+
+		(void) pushJsonbValue(st, WJB_KEY, &newkey);
+		(void) push_path(st, level, path_elems, path_nulls,
+						 path_len, newval);
+
+		/* Result is closed with WJB_END_OBJECT outside of this function */
+	}
 }
 
 /*
@@ -4978,25 +5123,48 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
 	if (idx < 0)
 	{
 		if (-idx > nelems)
-			idx = INT_MIN;
+		{
+			/*
+			 * If asked to keep elements position consistent, it's not allowed
+			 * to prepend the array.
+			 */
+			if (op_type & JB_PATH_CONSISTENT_POSITION)
+				ereport(ERROR,
+						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+						 errmsg("path element at position %d is out of range: %d",
+								level + 1, idx)));
+			else
+				idx = INT_MIN;
+		}
 		else
 			idx = nelems + idx;
 	}
 
-	if (idx > 0 && idx > nelems)
-		idx = nelems;
+	/*
+	 * Filling the gaps means there are no limits on the positive index are
+	 * imposed, we can set any element. Otherwise limit the index by nelems.
+	 */
+	if (!(op_type & JB_PATH_FILL_GAPS))
+	{
+		if (idx > 0 && idx > nelems)
+			idx = nelems;
+	}
 
 	/*
 	 * if we're creating, and idx == INT_MIN, we prepend the new value to the
 	 * array also if the array is empty - in which case we don't really care
 	 * what the idx value is
 	 */
-
 	if ((idx == INT_MIN || nelems == 0) && (level == path_len - 1) &&
 		(op_type & JB_PATH_CREATE_OR_INSERT))
 	{
 		Assert(newval != NULL);
+
+		if (op_type & JB_PATH_FILL_GAPS && nelems == 0 && idx > 0)
+			push_null_elements(st, idx);
+
 		(void) pushJsonbValue(st, WJB_ELEM, newval);
+
 		done = true;
 	}
 
@@ -5007,6 +5175,8 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
 
 		if (i == idx && level < path_len)
 		{
+			done = true;
+
 			if (level == path_len - 1)
 			{
 				r = JsonbIteratorNext(it, &v, true);	/* skip */
@@ -5024,8 +5194,6 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
 
 				if (op_type & (JB_PATH_INSERT_AFTER | JB_PATH_REPLACE))
 					(void) pushJsonbValue(st, WJB_ELEM, newval);
-
-				done = true;
 			}
 			else
 				(void) setPath(it, path_elems, path_nulls, path_len,
@@ -5053,14 +5221,42 @@ setPathArray(JsonbIterator **it, Datum *path_elems, bool *path_nulls,
 					(void) pushJsonbValue(st, r, r < WJB_BEGIN_ARRAY ? &v : NULL);
 				}
 			}
-
-			if ((op_type & JB_PATH_CREATE_OR_INSERT) && !done &&
-				level == path_len - 1 && i == nelems - 1)
-			{
-				(void) pushJsonbValue(st, WJB_ELEM, newval);
-			}
 		}
 	}
+
+	if ((op_type & JB_PATH_CREATE_OR_INSERT) && !done && level == path_len - 1)
+	{
+		/*
+		 * If asked to fill the gaps, idx could be bigger than nelems,
+		 * so prepend the new element with nulls if that's the case.
+		 */
+		if (op_type & JB_PATH_FILL_GAPS && idx > nelems)
+			push_null_elements(st, idx - nelems);
+
+		(void) pushJsonbValue(st, WJB_ELEM, newval);
+		done = true;
+	}
+
+	/*
+	 * If we got here there are only few possibilities:
+	 * - no target path was found, and an open array with some keys/values was
+	 *   pushed into the state
+	 * - an array is empty, only WJB_BEGIN_ARRAY is pushed
+	 *
+	 * In both cases if instructed to create the path when not present,
+	 * generate the whole chain of empty objects and insert the new value
+	 * there.
+	 */
+	if (!done && (op_type & JB_PATH_FILL_GAPS) && (level < path_len - 1))
+	{
+		if (idx > 0)
+			push_null_elements(st, idx - nelems);
+
+		(void) push_path(st, level, path_elems, path_nulls,
+						 path_len, newval);
+
+		/* Result is closed with WJB_END_OBJECT outside of this function */
+	}
 }
 
 /*
diff --git a/src/test/regress/expected/jsonb.out b/src/test/regress/expected/jsonb.out
index bb3f25ec3f..b7c268b53f 100644
--- a/src/test/regress/expected/jsonb.out
+++ b/src/test/regress/expected/jsonb.out
@@ -4999,6 +4999,141 @@ select * from test_jsonb_subscript;
   3 | [1]
 (3 rows)
 
+-- Fill the gaps logic
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '[0]');
+update test_jsonb_subscript set test_json[5] = '1';
+select * from test_jsonb_subscript;
+ id |           test_json            
+----+--------------------------------
+  1 | [0, null, null, null, null, 1]
+(1 row)
+
+update test_jsonb_subscript set test_json[-4] = '1';
+select * from test_jsonb_subscript;
+ id |          test_json          
+----+-----------------------------
+  1 | [0, null, 1, null, null, 1]
+(1 row)
+
+update test_jsonb_subscript set test_json[-8] = '1';
+ERROR:  path element at position 1 is out of range: -8
+select * from test_jsonb_subscript;
+ id |          test_json          
+----+-----------------------------
+  1 | [0, null, 1, null, null, 1]
+(1 row)
+
+-- keep consistent values position
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '[]');
+update test_jsonb_subscript set test_json[5] = '1';
+select * from test_jsonb_subscript;
+ id |             test_json             
+----+-----------------------------------
+  1 | [null, null, null, null, null, 1]
+(1 row)
+
+-- create the whole path
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '{}');
+update test_jsonb_subscript set test_json['a'][0]['b'][0]['c'] = '1';
+select * from test_jsonb_subscript;
+ id |         test_json          
+----+----------------------------
+  1 | {"a": [{"b": [{"c": 1}]}]}
+(1 row)
+
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '{}');
+update test_jsonb_subscript set test_json['a'][2]['b'][2]['c'][2] = '1';
+select * from test_jsonb_subscript;
+ id |                            test_json                             
+----+------------------------------------------------------------------
+  1 | {"a": [null, null, {"b": [null, null, {"c": [null, null, 1]}]}]}
+(1 row)
+
+-- create the whole path with already existing keys
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '{"b": 1}');
+update test_jsonb_subscript set test_json['a'][0] = '2';
+select * from test_jsonb_subscript;
+ id |     test_json      
+----+--------------------
+  1 | {"a": [2], "b": 1}
+(1 row)
+
+-- the start jsonb is an object, first subscript is treated as a key
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '{}');
+update test_jsonb_subscript set test_json[0]['a'] = '1';
+select * from test_jsonb_subscript;
+ id |    test_json    
+----+-----------------
+  1 | {"0": {"a": 1}}
+(1 row)
+
+-- the start jsonb is an array
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '[]');
+update test_jsonb_subscript set test_json[0]['a'] = '1';
+update test_jsonb_subscript set test_json[2]['b'] = '2';
+select * from test_jsonb_subscript;
+ id |         test_json          
+----+----------------------------
+  1 | [{"a": 1}, null, {"b": 2}]
+(1 row)
+
+-- overwriting an existing path
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '{}');
+update test_jsonb_subscript set test_json['a']['b'][1] = '1';
+update test_jsonb_subscript set test_json['a']['b'][10] = '1';
+select * from test_jsonb_subscript;
+ id |                                 test_json                                  
+----+----------------------------------------------------------------------------
+  1 | {"a": {"b": [null, 1, null, null, null, null, null, null, null, null, 1]}}
+(1 row)
+
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '[]');
+update test_jsonb_subscript set test_json[0][0][0] = '1';
+update test_jsonb_subscript set test_json[0][0][1] = '1';
+select * from test_jsonb_subscript;
+ id | test_json  
+----+------------
+  1 | [[[1, 1]]]
+(1 row)
+
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '{}');
+update test_jsonb_subscript set test_json['a']['b'][10] = '1';
+update test_jsonb_subscript set test_json['a'][10][10] = '1';
+select * from test_jsonb_subscript;
+ id |                                                                      test_json                                                                       
+----+------------------------------------------------------------------------------------------------------------------------------------------------------
+  1 | {"a": {"b": [null, null, null, null, null, null, null, null, null, null, 1], "10": [null, null, null, null, null, null, null, null, null, null, 1]}}
+(1 row)
+
+-- an empty sub element
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '{"a": {}}');
+update test_jsonb_subscript set test_json['a']['b']['c'][2] = '1';
+select * from test_jsonb_subscript;
+ id |              test_json               
+----+--------------------------------------
+  1 | {"a": {"b": {"c": [null, null, 1]}}}
+(1 row)
+
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '{"a": []}');
+update test_jsonb_subscript set test_json['a'][1]['c'][2] = '1';
+select * from test_jsonb_subscript;
+ id |               test_json               
+----+---------------------------------------
+  1 | {"a": [null, {"c": [null, null, 1]}]}
+(1 row)
+
 -- jsonb to tsvector
 select to_tsvector('{"a": "aaa bbb ddd ccc", "b": ["eee fff ggg"], "c": {"d": "hhh iii"}}'::jsonb);
                                 to_tsvector                                
diff --git a/src/test/regress/sql/jsonb.sql b/src/test/regress/sql/jsonb.sql
index 20aa8fe0e2..0320db0ea4 100644
--- a/src/test/regress/sql/jsonb.sql
+++ b/src/test/regress/sql/jsonb.sql
@@ -1290,6 +1290,87 @@ update test_jsonb_subscript set test_json = NULL where id = 3;
 update test_jsonb_subscript set test_json[0] = '1';
 select * from test_jsonb_subscript;
 
+-- Fill the gaps logic
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '[0]');
+
+update test_jsonb_subscript set test_json[5] = '1';
+select * from test_jsonb_subscript;
+
+update test_jsonb_subscript set test_json[-4] = '1';
+select * from test_jsonb_subscript;
+
+update test_jsonb_subscript set test_json[-8] = '1';
+select * from test_jsonb_subscript;
+
+-- keep consistent values position
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '[]');
+
+update test_jsonb_subscript set test_json[5] = '1';
+select * from test_jsonb_subscript;
+
+-- create the whole path
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '{}');
+update test_jsonb_subscript set test_json['a'][0]['b'][0]['c'] = '1';
+select * from test_jsonb_subscript;
+
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '{}');
+update test_jsonb_subscript set test_json['a'][2]['b'][2]['c'][2] = '1';
+select * from test_jsonb_subscript;
+
+-- create the whole path with already existing keys
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '{"b": 1}');
+update test_jsonb_subscript set test_json['a'][0] = '2';
+select * from test_jsonb_subscript;
+
+-- the start jsonb is an object, first subscript is treated as a key
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '{}');
+update test_jsonb_subscript set test_json[0]['a'] = '1';
+select * from test_jsonb_subscript;
+
+-- the start jsonb is an array
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '[]');
+update test_jsonb_subscript set test_json[0]['a'] = '1';
+update test_jsonb_subscript set test_json[2]['b'] = '2';
+select * from test_jsonb_subscript;
+
+-- overwriting an existing path
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '{}');
+update test_jsonb_subscript set test_json['a']['b'][1] = '1';
+update test_jsonb_subscript set test_json['a']['b'][10] = '1';
+select * from test_jsonb_subscript;
+
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '[]');
+update test_jsonb_subscript set test_json[0][0][0] = '1';
+update test_jsonb_subscript set test_json[0][0][1] = '1';
+select * from test_jsonb_subscript;
+
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '{}');
+update test_jsonb_subscript set test_json['a']['b'][10] = '1';
+update test_jsonb_subscript set test_json['a'][10][10] = '1';
+select * from test_jsonb_subscript;
+
+-- an empty sub element
+
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '{"a": {}}');
+update test_jsonb_subscript set test_json['a']['b']['c'][2] = '1';
+select * from test_jsonb_subscript;
+
+delete from test_jsonb_subscript;
+insert into test_jsonb_subscript values (1, '{"a": []}');
+update test_jsonb_subscript set test_json['a'][1]['c'][2] = '1';
+select * from test_jsonb_subscript;
+
 -- jsonb to tsvector
 select to_tsvector('{"a": "aaa bbb ddd ccc", "b": ["eee fff ggg"], "c": {"d": "hhh iii"}}'::jsonb);
 
-- 
2.21.0


--6snw7vcvuujiswgu--





^ permalink  raw  reply  [nested|flat] 9+ messages in thread

* psql: Add role's membership options to the \du+ command
@ 2023-01-09 16:09  Pavel Luzanov <[email protected]>
  0 siblings, 2 replies; 9+ messages in thread

From: Pavel Luzanov @ 2023-01-09 16:09 UTC (permalink / raw)
  To: [email protected] <[email protected]>

When you include one role in another, you can specify three options:
ADMIN, INHERIT (added in e3ce2de0) and SET (3d14e171).

For example.

CREATE ROLE alice LOGIN;

GRANT pg_read_all_settings TO alice WITH ADMIN TRUE, INHERIT TRUE, SET TRUE;
GRANT pg_stat_scan_tables TO alice WITH ADMIN FALSE, INHERIT FALSE, SET 
FALSE;
GRANT pg_read_all_stats TO alice WITH ADMIN FALSE, INHERIT TRUE, SET FALSE;

For information about the options, you need to look in the pg_auth_members:

SELECT roleid::regrole, admin_option, inherit_option, set_option
FROM pg_auth_members
WHERE member = 'alice'::regrole;
         roleid        | admin_option | inherit_option | set_option
----------------------+--------------+----------------+------------
  pg_read_all_settings | t            | t              | t
  pg_stat_scan_tables  | f            | f              | f
  pg_read_all_stats    | f            | t              | f
(3 rows)

I think it would be useful to be able to get this information with a 
psql command
like \du (and \dg). With proposed patch the \du command still only lists
the roles of which alice is a member:

\du alice
                                      List of roles
  Role name | Attributes |                          Member of
-----------+------------+--------------------------------------------------------------
  alice     |            | 
{pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables}

But the \du+ command adds information about the selected ADMIN, INHERIT
and SET options:

\du+ alice
                                     List of roles
  Role name | Attributes |                   Member of                   
| Description
-----------+------------+-----------------------------------------------+-------------
  alice     |            | pg_read_all_settings WITH ADMIN, INHERIT, SET+|
            |            | pg_read_all_stats WITH INHERIT               +|
            |            | pg_stat_scan_tables                           |

One more change. The roles in the "Member of" column are sorted for both
\du+ and \du for consistent output.

Any comments are welcome.

-- 
Pavel Luzanov
Postgres Professional: https://postgrespro.com


Attachments:

  [text/x-patch] add_role_membership_options_to_du.patch (5.8K, ../../[email protected]/2-add_role_membership_options_to_du.patch)
  download | inline diff:
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 8a5285da9a..ef3e87fa32 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1724,9 +1724,8 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
         <literal>S</literal> modifier to include system roles.
         If <replaceable class="parameter">pattern</replaceable> is specified,
         only those roles whose names match the pattern are listed.
-        If the form <literal>\dg+</literal> is used, additional information
-        is shown about each role; currently this adds the comment for each
-        role.
+        If the form <literal>\dg+</literal> is used, each role is listed
+        with its associated description and options for each role's membership.
         </para>
         </listitem>
       </varlistentry>
@@ -1964,9 +1963,8 @@ INSERT INTO tbl1 VALUES ($1, $2) \bind 'first value' 'second value' \g
         <literal>S</literal> modifier to include system roles.
         If <replaceable class="parameter">pattern</replaceable> is specified,
         only those roles whose names match the pattern are listed.
-        If the form <literal>\du+</literal> is used, additional information
-        is shown about each role; currently this adds the comment for each
-        role.
+        If the form <literal>\du+</literal> is used, each role is listed
+        with its associated description and options for each role's membership.
         </para>
         </listitem>
       </varlistentry>
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index df166365e8..c7b10f96f3 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -3624,11 +3624,29 @@ describeRoles(const char *pattern, bool verbose, bool showSystem)
 	printfPQExpBuffer(&buf,
 					  "SELECT r.rolname, r.rolsuper, r.rolinherit,\n"
 					  "  r.rolcreaterole, r.rolcreatedb, r.rolcanlogin,\n"
-					  "  r.rolconnlimit, r.rolvaliduntil,\n"
-					  "  ARRAY(SELECT b.rolname\n"
-					  "        FROM pg_catalog.pg_auth_members m\n"
-					  "        JOIN pg_catalog.pg_roles b ON (m.roleid = b.oid)\n"
-					  "        WHERE m.member = r.oid) as memberof");
+					  "  r.rolconnlimit, r.rolvaliduntil,\n");
+
+	if (verbose)
+	{
+		appendPQExpBufferStr(&buf,
+							 "  (SELECT string_agg (quote_ident(b.rolname) || regexp_replace(\n"
+							 "     CASE WHEN m.admin_option THEN ', ADMIN' ELSE '' END ||\n"
+							 "     CASE WHEN m.inherit_option THEN ', INHERIT' ELSE '' END ||\n"
+							 "     CASE WHEN m.set_option THEN ', SET' ELSE '' END, \n"
+							 "     '^,', ' WITH', 1), E'\\n' ORDER BY b.rolname)\n"
+							 "  FROM pg_catalog.pg_auth_members m\n"
+							 "  JOIN pg_catalog.pg_roles b ON (m.roleid = b.oid)\n"
+							 "  WHERE m.member = r.oid) as memberof");
+	}
+	else
+	{
+		appendPQExpBufferStr(&buf,
+							 "  ARRAY(SELECT b.rolname\n"
+							 "        FROM pg_catalog.pg_auth_members m\n"
+							 "        JOIN pg_catalog.pg_roles b ON (m.roleid = b.oid)\n"
+							 "        WHERE m.member = r.oid\n"
+							 "        ORDER BY b.rolname) as memberof");
+	}
 
 	if (verbose)
 	{
diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out
index 8fc62cebd2..3a2eb65850 100644
--- a/src/test/regress/expected/psql.out
+++ b/src/test/regress/expected/psql.out
@@ -6578,3 +6578,24 @@ cross-database references are not implemented: "no.such.database"."no.such.schem
 cross-database references are not implemented: "no.such.database"."no.such.schema"."no.such.data.type"
 \dX "no.such.database"."no.such.schema"."no.such.extended.statistics"
 cross-database references are not implemented: "no.such.database"."no.such.schema"."no.such.extended.statistics"
+-- check \du
+CREATE ROLE regress_du LOGIN;
+COMMENT ON ROLE regress_du IS 'Description for regress_du role';
+GRANT pg_read_all_settings TO regress_du WITH ADMIN TRUE, INHERIT TRUE, SET TRUE;
+GRANT pg_stat_scan_tables TO regress_du WITH ADMIN FALSE, INHERIT FALSE, SET FALSE;
+GRANT pg_read_all_stats TO regress_du WITH ADMIN FALSE, INHERIT TRUE, SET FALSE;
+\du regress_du
+                                     List of roles
+ Role name  | Attributes |                          Member of                           
+------------+------------+--------------------------------------------------------------
+ regress_du |            | {pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables}
+
+\du+ regress_du
+                                               List of roles
+ Role name  | Attributes |                   Member of                   |           Description           
+------------+------------+-----------------------------------------------+---------------------------------
+ regress_du |            | pg_read_all_settings WITH ADMIN, INHERIT, SET+| Description for regress_du role
+            |            | pg_read_all_stats WITH INHERIT               +| 
+            |            | pg_stat_scan_tables                           | 
+
+DROP ROLE regress_du;
diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql
index 2da9665a19..6a5a57ab72 100644
--- a/src/test/regress/sql/psql.sql
+++ b/src/test/regress/sql/psql.sql
@@ -1791,3 +1791,14 @@ DROP FUNCTION psql_error;
 \dP "no.such.database"."no.such.schema"."no.such.partitioned.relation"
 \dT "no.such.database"."no.such.schema"."no.such.data.type"
 \dX "no.such.database"."no.such.schema"."no.such.extended.statistics"
+
+-- check \du
+CREATE ROLE regress_du LOGIN;
+COMMENT ON ROLE regress_du IS 'Description for regress_du role';
+GRANT pg_read_all_settings TO regress_du WITH ADMIN TRUE, INHERIT TRUE, SET TRUE;
+GRANT pg_stat_scan_tables TO regress_du WITH ADMIN FALSE, INHERIT FALSE, SET FALSE;
+GRANT pg_read_all_stats TO regress_du WITH ADMIN FALSE, INHERIT TRUE, SET FALSE;
+
+\du regress_du
+\du+ regress_du
+DROP ROLE regress_du;


^ permalink  raw  reply  [nested|flat] 9+ messages in thread

* Re: psql: Add role's membership options to the \du+ command
@ 2023-01-24 17:16  David G. Johnston <[email protected]>
  parent: Pavel Luzanov <[email protected]>
  1 sibling, 1 reply; 9+ messages in thread

From: David G. Johnston @ 2023-01-24 17:16 UTC (permalink / raw)
  To: Pavel Luzanov <[email protected]>; +Cc: [email protected] <[email protected]>

On Mon, Jan 9, 2023 at 9:09 AM Pavel Luzanov <[email protected]>
wrote:

> When you include one role in another, you can specify three options:
> ADMIN, INHERIT (added in e3ce2de0) and SET (3d14e171).
>
> For example.
>
> CREATE ROLE alice LOGIN;
>
> GRANT pg_read_all_settings TO alice WITH ADMIN TRUE, INHERIT TRUE, SET
> TRUE;
> GRANT pg_stat_scan_tables TO alice WITH ADMIN FALSE, INHERIT FALSE, SET
> FALSE;
> GRANT pg_read_all_stats TO alice WITH ADMIN FALSE, INHERIT TRUE, SET FALSE;
>
> For information about the options, you need to look in the pg_auth_members:
>
> SELECT roleid::regrole, admin_option, inherit_option, set_option
> FROM pg_auth_members
> WHERE member = 'alice'::regrole;
>          roleid        | admin_option | inherit_option | set_option
> ----------------------+--------------+----------------+------------
>   pg_read_all_settings | t            | t              | t
>   pg_stat_scan_tables  | f            | f              | f
>   pg_read_all_stats    | f            | t              | f
> (3 rows)
>
> I think it would be useful to be able to get this information with a
> psql command
> like \du (and \dg). With proposed patch the \du command still only lists
> the roles of which alice is a member:
>
> \du alice
>                                       List of roles
>   Role name | Attributes |                          Member of
>
> -----------+------------+--------------------------------------------------------------
>   alice     |            |
> {pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables}
>
> But the \du+ command adds information about the selected ADMIN, INHERIT
> and SET options:
>
> \du+ alice
>                                      List of roles
>   Role name | Attributes |                   Member of
> | Description
>
> -----------+------------+-----------------------------------------------+-------------
>   alice     |            | pg_read_all_settings WITH ADMIN, INHERIT, SET+|
>             |            | pg_read_all_stats WITH INHERIT               +|
>             |            | pg_stat_scan_tables                           |
>
> One more change. The roles in the "Member of" column are sorted for both
> \du+ and \du for consistent output.
>
> Any comments are welcome.
>
>
Yeah, I noticed the lack too, then went a bit too far afield with trying to
compose a graph of the roles.  I'm still working on that but at this point
it probably won't be something I try to get committed to psql.  Something
more limited like this does need to be included.

One thing I did was name the situation where none of the grants are true -
EMPTY.  So: pg_stat_scan_tables WITH EMPTY.

I'm not too keen on the idea of converting the existing array into a
newline separated string.  I would try hard to make the modification here
purely additional.  If users really want to build up queries on their own
they should be using the system catalog.  So concise human readability
should be the goal here.  Keeping those two things in mind I would add a
new text[] column to the views with the following possible values in each
cell the meaning of which should be self-evident or this probably isn't a
good approach...

ais
ai
as
a
is
i
s
empty

That said, I do find the newline based output to be quite useful in the
graph query I'm writing and so wouldn't be disappointed if we changed over
to that.  I'd probably stick with abbreviations though.  Another thing I
did with the graph was have both "member" and "memberof" columns in the
output.  In short, every grant row in pg_auth_members appears twice, once
in each column, so the role being granted membership and the role into
which membership is granted both have visibility when you filter on them.
For the role graph I took this idea and extended out to an entire chain of
roles (and also broke out user and group separately) but I think doing the
direct-grant only here would still be a big improvement.

postgres=# \dgS+ pg_read_all_settings
                         List of roles
      Role name       |  Attributes  | Member of | Members | Description
----------------------+--------------+-----------+-------------
 pg_read_all_settings | Cannot login | {}        | { pg_monitor }       |

David J.


^ permalink  raw  reply  [nested|flat] 9+ messages in thread

* Re: psql: Add role's membership options to the \du+ command
@ 2023-01-26 13:53  Pavel Luzanov <[email protected]>
  parent: David G. Johnston <[email protected]>
  0 siblings, 0 replies; 9+ messages in thread

From: Pavel Luzanov @ 2023-01-26 13:53 UTC (permalink / raw)
  To: David G. Johnston <[email protected]>; +Cc: [email protected] <[email protected]>

On 24.01.2023 20:16, David G. Johnston wrote:
> Yeah, I noticed the lack too, then went a bit too far afield with 
> trying to compose a graph of the roles.  I'm still working on that but 
> at this point it probably won't be something I try to get committed to 
> psql.  Something more limited like this does need to be included.

Glad to hear that you're working on it.

> I'm not too keen on the idea of converting the existing array into a 
> newline separated string.  I would try hard to make the modification 
> here purely additional.

I agree with all of your arguments. A couple of months I tried to find 
an acceptable variant in the background.
But apparently tried not very hard ))

In the end, the variant proposed in the patch seemed to me worthy to 
show and
start a discussion. But I'm not sure that this is the best choice.

> Another thing I did with the graph was have both "member" and 
> "memberof" columns in the output.  In short, every grant row in 
> pg_auth_members appears twice, once in each column, so the role being 
> granted membership and the role into which membership is granted both 
> have visibility when you filter on them.  For the role graph I took 
> this idea and extended out to an entire chain of roles (and also broke 
> out user and group separately) but I think doing the direct-grant only 
> here would still be a big improvement.

It will be interesting to see the result.

-- 
Pavel Luzanov


^ permalink  raw  reply  [nested|flat] 9+ messages in thread

* Re: psql: Add role's membership options to the \du+ command
@ 2023-02-10 21:08  David Zhang <[email protected]>
  parent: Pavel Luzanov <[email protected]>
  1 sibling, 1 reply; 9+ messages in thread

From: David Zhang @ 2023-02-10 21:08 UTC (permalink / raw)
  To: Pavel Luzanov <[email protected]>; [email protected] <[email protected]>

Thanks a lot for the improvement, and it will definitely help provide 
more very useful information.

I noticed the document psql-ref.sgml has been updated for both `du+` and 
`dg+`, but only `du` and `\du+` are covered in regression test. Is that 
because `dg+` is treated exactly the same as `du+` from testing point of 
view?

The reason I am asking this question is that I notice that `pg_monitor` 
also has the detailed information, so not sure if more test cases required.

postgres=# \duS+
List of roles
           Role name          | Attributes                         
|                   Member of                   | Description
-----------------------------+------------------------------------------------------------+-----------------------------------------------+-------------
  alice |                                                            | 
pg_read_all_settings WITH ADMIN, INHERIT, SET |
  pg_checkpoint               | Cannot login 
|                                               |
  pg_database_owner           | Cannot login 
|                                               |
  pg_execute_server_program   | Cannot login 
|                                               |
  pg_maintain                 | Cannot login 
|                                               |
  pg_monitor                  | Cannot 
login                                               | 
pg_read_all_settings WITH INHERIT, SET       +|
|                                                            | 
pg_read_all_stats WITH INHERIT, SET          +|
|                                                            | 
pg_stat_scan_tables WITH INHERIT, SET         |

Best regards,

David

On 2023-01-09 8:09 a.m., Pavel Luzanov wrote:
> When you include one role in another, you can specify three options:
> ADMIN, INHERIT (added in e3ce2de0) and SET (3d14e171).
>
> For example.
>
> CREATE ROLE alice LOGIN;
>
> GRANT pg_read_all_settings TO alice WITH ADMIN TRUE, INHERIT TRUE, SET 
> TRUE;
> GRANT pg_stat_scan_tables TO alice WITH ADMIN FALSE, INHERIT FALSE, 
> SET FALSE;
> GRANT pg_read_all_stats TO alice WITH ADMIN FALSE, INHERIT TRUE, SET 
> FALSE;
>
> For information about the options, you need to look in the 
> pg_auth_members:
>
> SELECT roleid::regrole, admin_option, inherit_option, set_option
> FROM pg_auth_members
> WHERE member = 'alice'::regrole;
>         roleid        | admin_option | inherit_option | set_option
> ----------------------+--------------+----------------+------------
>  pg_read_all_settings | t            | t              | t
>  pg_stat_scan_tables  | f            | f              | f
>  pg_read_all_stats    | f            | t              | f
> (3 rows)
>
> I think it would be useful to be able to get this information with a 
> psql command
> like \du (and \dg). With proposed patch the \du command still only lists
> the roles of which alice is a member:
>
> \du alice
>                                      List of roles
>  Role name | Attributes |                          Member of
> -----------+------------+-------------------------------------------------------------- 
>
>  alice     |            | 
> {pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables}
>
> But the \du+ command adds information about the selected ADMIN, INHERIT
> and SET options:
>
> \du+ alice
>                                     List of roles
>  Role name | Attributes |                   Member 
> of                   | Description
> -----------+------------+-----------------------------------------------+------------- 
>
>  alice     |            | pg_read_all_settings WITH ADMIN, INHERIT, SET+|
>            |            | pg_read_all_stats WITH INHERIT               +|
>            |            | pg_stat_scan_tables                           |
>
> One more change. The roles in the "Member of" column are sorted for both
> \du+ and \du for consistent output.
>
> Any comments are welcome.
>






^ permalink  raw  reply  [nested|flat] 9+ messages in thread

* Re: psql: Add role's membership options to the \du+ command
@ 2023-02-10 22:27  David G. Johnston <[email protected]>
  parent: David Zhang <[email protected]>
  0 siblings, 1 reply; 9+ messages in thread

From: David G. Johnston @ 2023-02-10 22:27 UTC (permalink / raw)
  To: David Zhang <[email protected]>; +Cc: Pavel Luzanov <[email protected]>; [email protected] <[email protected]>

On Fri, Feb 10, 2023 at 2:08 PM David Zhang <[email protected]> wrote:

>
> I noticed the document psql-ref.sgml has been updated for both `du+` and
> `dg+`, but only `du` and `\du+` are covered in regression test. Is that
> because `dg+` is treated exactly the same as `du+` from testing point of
> view?
>

Yes.

>
> The reason I am asking this question is that I notice that `pg_monitor`
> also has the detailed information, so not sure if more test cases required.
>

Of course it does.  Why does that bother you?  And what does that have to
do with the previous paragraph?

David J.


^ permalink  raw  reply  [nested|flat] 9+ messages in thread

* Re: psql: Add role's membership options to the \du+ command
@ 2023-02-15 21:31  David Zhang <[email protected]>
  parent: David G. Johnston <[email protected]>
  0 siblings, 1 reply; 9+ messages in thread

From: David Zhang @ 2023-02-15 21:31 UTC (permalink / raw)
  To: David G. Johnston <[email protected]>; +Cc: Pavel Luzanov <[email protected]>; [email protected] <[email protected]>

On 2023-02-10 2:27 p.m., David G. Johnston wrote:
> On Fri, Feb 10, 2023 at 2:08 PM David Zhang <[email protected]> wrote:
>
>
>     I noticed the document psql-ref.sgml has been updated for both
>     `du+` and
>     `dg+`, but only `du` and `\du+` are covered in regression test. Is
>     that
>     because `dg+` is treated exactly the same as `du+` from testing
>     point of
>     view?
>
>
> Yes.
>
>
>     The reason I am asking this question is that I notice that
>     `pg_monitor`
>     also has the detailed information, so not sure if more test cases
>     required.
>
>
> Of course it does.  Why does that bother you?  And what does that have 
> to do with the previous paragraph?

There is a default built-in role `pg_monitor` and the behavior changed 
after the patch. If `\dg+` and `\du+` is treated as the same, and `make 
check` all pass, then I assume there is no test case to verify the 
output of `duS+`. My point is should we consider add a test case?

Before patch the output for `pg_monitor`,

postgres=# \duS+
List of roles
           Role name          | Attributes                         | 
Member of                           | Description
-----------------------------+------------------------------------------------------------+--------------------------------------------------------------+-------------
  alice |                                                            | 
{pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables} |
  pg_checkpoint               | Cannot 
login                                               | 
{}                                                           |
  pg_database_owner           | Cannot 
login                                               | 
{}                                                           |
  pg_execute_server_program   | Cannot 
login                                               | 
{}                                                           |
  pg_maintain                 | Cannot 
login                                               | 
{}                                                           |
  pg_monitor                  | Cannot 
login                                               | 
{pg_read_all_settings,pg_read_all_stats,pg_stat_scan_tables} |
  pg_read_all_data            | Cannot 
login                                               | 
{}                                                           |
  pg_read_all_settings        | Cannot 
login                                               | 
{}                                                           |
  pg_read_all_stats           | Cannot 
login                                               | 
{}                                                           |
  pg_read_server_files        | Cannot 
login                                               | 
{}                                                           |
  pg_signal_backend           | Cannot 
login                                               | 
{}                                                           |
  pg_stat_scan_tables         | Cannot 
login                                               | 
{}                                                           |
  pg_use_reserved_connections | Cannot 
login                                               | 
{}                                                           |
  pg_write_all_data           | Cannot 
login                                               | 
{}                                                           |
  pg_write_server_files       | Cannot 
login                                               | 
{}                                                           |
  ubuntu                      | Superuser, Create role, Create DB, 
Replication, Bypass RLS | 
{}                                                           |

After patch the output for `pg_monitor`,

postgres=# \duS+
List of roles
           Role name          | Attributes                         
|                   Member of                   | Description
-----------------------------+------------------------------------------------------------+-----------------------------------------------+-------------
  alice |                                                            | 
pg_read_all_settings WITH ADMIN, INHERIT, SET+|
|                                                            | 
pg_read_all_stats WITH INHERIT               +|
|                                                            | 
pg_stat_scan_tables                           |
  pg_checkpoint               | Cannot login 
|                                               |
  pg_database_owner           | Cannot login 
|                                               |
  pg_execute_server_program   | Cannot login 
|                                               |
  pg_maintain                 | Cannot login 
|                                               |
  pg_monitor                  | Cannot 
login                                               | 
pg_read_all_settings WITH INHERIT, SET       +|
|                                                            | 
pg_read_all_stats WITH INHERIT, SET          +|
|                                                            | 
pg_stat_scan_tables WITH INHERIT, SET         |
  pg_read_all_data            | Cannot login 
|                                               |
  pg_read_all_settings        | Cannot login 
|                                               |
  pg_read_all_stats           | Cannot login 
|                                               |
  pg_read_server_files        | Cannot login 
|                                               |
  pg_signal_backend           | Cannot login 
|                                               |
  pg_stat_scan_tables         | Cannot login 
|                                               |
  pg_use_reserved_connections | Cannot login 
|                                               |
  pg_write_all_data           | Cannot login 
|                                               |
  pg_write_server_files       | Cannot login 
|                                               |
  ubuntu                      | Superuser, Create role, Create DB, 
Replication, Bypass RLS |                                               |


Best regards,

David

>
> David J.

^ permalink  raw  reply  [nested|flat] 9+ messages in thread

* Re: psql: Add role's membership options to the \du+ command
@ 2023-02-15 21:37  David G. Johnston <[email protected]>
  parent: David Zhang <[email protected]>
  0 siblings, 1 reply; 9+ messages in thread

From: David G. Johnston @ 2023-02-15 21:37 UTC (permalink / raw)
  To: David Zhang <[email protected]>; +Cc: Pavel Luzanov <[email protected]>; [email protected] <[email protected]>

On Wed, Feb 15, 2023 at 2:31 PM David Zhang <[email protected]> wrote:

> There is a default built-in role `pg_monitor` and the behavior changed
> after the patch. If `\dg+` and `\du+` is treated as the same, and `make
> check` all pass, then I assume there is no test case to verify the output
> of `duS+`. My point is should we consider add a test case?
>

I mean, either you accept the change in how this meta-command presents its
information or you don't.  I don't see how a test case is particularly
beneficial.  Or, at least the pg_monitor role is not special in this
regard.  Alice changed too and you don't seem to be including it in your
complaint.

David J.


^ permalink  raw  reply  [nested|flat] 9+ messages in thread

* Re: psql: Add role's membership options to the \du+ command
@ 2023-02-15 22:52  David Zhang <[email protected]>
  parent: David G. Johnston <[email protected]>
  0 siblings, 0 replies; 9+ messages in thread

From: David Zhang @ 2023-02-15 22:52 UTC (permalink / raw)
  To: David G. Johnston <[email protected]>; Pavel Luzanov <[email protected]>; +Cc: [email protected] <[email protected]>

On 2023-02-15 1:37 p.m., David G. Johnston wrote:

> On Wed, Feb 15, 2023 at 2:31 PM David Zhang <[email protected]> wrote:
>
>     There is a default built-in role `pg_monitor` and the behavior
>     changed after the patch. If `\dg+` and `\du+` is treated as the
>     same, and `make check` all pass, then I assume there is no test
>     case to verify the output of `duS+`. My point is should we
>     consider add a test case?
>
> I mean, either you accept the change in how this meta-command presents 
> its information or you don't.  I don't see how a test case is 
> particularly beneficial.  Or, at least the pg_monitor role is not 
> special in this regard.  Alice changed too and you don't seem to be 
> including it in your complaint.
Good improvement, +1.

^ permalink  raw  reply  [nested|flat] 9+ messages in thread


end of thread, other threads:[~2023-02-15 22:52 UTC | newest]

Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-12-31 14:19 [PATCH v43 2/2] Filling gaps in jsonb Dmitrii Dolgov <[email protected]>
2023-01-09 16:09 psql: Add role's membership options to the \du+ command Pavel Luzanov <[email protected]>
2023-01-24 17:16 ` Re: psql: Add role's membership options to the \du+ command David G. Johnston <[email protected]>
2023-01-26 13:53   ` Re: psql: Add role's membership options to the \du+ command Pavel Luzanov <[email protected]>
2023-02-10 21:08 ` Re: psql: Add role's membership options to the \du+ command David Zhang <[email protected]>
2023-02-10 22:27   ` Re: psql: Add role's membership options to the \du+ command David G. Johnston <[email protected]>
2023-02-15 21:31     ` Re: psql: Add role's membership options to the \du+ command David Zhang <[email protected]>
2023-02-15 21:37       ` Re: psql: Add role's membership options to the \du+ command David G. Johnston <[email protected]>
2023-02-15 22:52         ` Re: psql: Add role's membership options to the \du+ command David Zhang <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox