RE: Support logical replication of DDLs - Mailing list pgsql-hackers

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.

~~~

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
BYRANGE (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