On Tues, May 30, 2023 at 19:19 PM vignesh C <vignes...@gmail.com> wrote:
> The attached patch has the changes for the above.
Thanks for updating the new patch set.
Here are some comments:
===
For patch 0001
1. In the function deparse_Seq_As.
```
+ if (OidIsValid(seqdata->seqtypid))
+ append_object_object(ret, "seqtype",
+
new_objtree_for_type(seqdata->seqtypid, -1));
+ else
+ append_not_present(ret);
```
I think seqdata->seqtypid is always valid because we get this value from
pg_sequence. I think it's fine to not check this value here.
~~~
2. Deparsed results of the partition table.
When I run the following SQLs:
```
create table parent (a int primary key) partition by range (a);
create table child partition of parent default;
```
I got the following two deparsed results:
```
CREATE TABLE public.parent (a pg_catalog.int4 STORAGE PLAIN , CONSTRAINT
parent_pkey PRIMARY KEY (a)) PARTITION BY RANGE (a)
CREATE TABLE public.child PARTITION OF public.parent (CONSTRAINT child_pkey
PRIMARY KEY (a)) DEFAULT
```
When I run these two deparsed results on another instance, I got the following
error:
```
postgres=# CREATE TABLE public.parent (a pg_catalog.int4 STORAGE PLAIN ,
CONSTRAINT parent_pkey PRIMARY KEY (a)) PARTITION BY RANGE (a);
CREATE TABLE
postgres=# CREATE TABLE public.child PARTITION OF public.parent (CONSTRAINT
child_pkey PRIMARY KEY (a)) DEFAULT;
ERROR: multiple primary keys for table "child" are not allowed
```
I think that we could skip deparsing the primary key related constraint for
partition (child) table in the function obtainConstraints for this case.
===
For patch 0008
3. Typos in the comments atop the function append_object_to_format_string
```
- * Return the object name which is extracted from the input "*%{name[:.]}*"
- * style string. And append the input format string to the ObjTree.
+ * Append new jsonb key:value pair to the output parse state -- varargs
version.
+ *
+ * The "fmt" argument is used to append as a "fmt" element in current object.
+ * The "skipObject" argument indicates if we want to skip object creation
+ * considering it will be taken care by the caller.
+ * The "numobjs" argument indicates the number of extra elements to append;
+ * for each one, a name (string), type (from the jbvType enum) and value must
+ * be supplied. The value must match the type given; for instance, jbvBool
+ * requires an * bool, jbvString requires a char * and so no,
+ * Each element type must match the conversion specifier given in the format
+ * string, as described in ddl_deparse_expand_command.
+ *
+ * Note we don't have the luxury of sprintf-like compiler warnings for
+ * malformed argument lists.
*/
-static char *
-append_object_to_format_string(ObjTree *tree, char *sub_fmt)
+static JsonbValue *
+new_jsonb_VA(JsonbParseState *state, char *fmt, bool skipObject, int
numobjs,...)
```
s/and so no/and so on
s/requires an * bool/requires an bool
s/type must/type must
~~~
4. Typos of the function new_jsonb_for_type
```
/*
- * Allocate a new object tree to store parameter values.
+ * A helper routine to insert jsonb for coltyp to the output parse state.
*/
-static ObjTree *
-new_objtree(char *fmt)
+static void
+new_jsonb_for_type(JsonbParseState *state, Oid typeId, int32 typmod)
...
+ format_type_detailed(typeId, typmod,
+ &typnspid, &type_name,
&typmodstr, &type_array);
```
s/coltyp/typId
s/typeId/typId
~~~
5. In the function deparse_ColumnDef_toJsonb
+ /*
+ * create coltype object having 4 elements: schemaname, typename,
typemod,
+ * typearray
+ */
+ {
+ /* Push the key first */
+ insert_jsonb_key(state, "coltype");
+
+ /* Push the value */
+ new_jsonb_for_type(state, typid, typmod);
+ }
The '{ }' here seems a little strange. Do we need them?
Many places have written the same as here in this patch.
Regards,
Wang wei
