On Wed, 31 May 2023 at 14:32, Wei Wang (Fujitsu) <wangw.fnst@fujitsu.com> wrote:
>
> On Tues, May 30, 2023 at 19:19 PM vignesh C <vignesh21@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.
Modified in 0008 patch
> ~~~
>
> 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))
PARTITIONBY 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.
Not applicable for 0008 patch
> ===
> 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
Modified in 0008 patch
> ~~~
>
> 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
Modified in 0008 patch
> ~~~
>
> 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.
Modified in 0008 patch
The attached patch has the changes for the same. The 0008 patch was
modified to fix the above comments.
Regards,
Vignesh
