Re: Support logical replication of DDLs

On Wed, Jun 29, 2022 at 3:24 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Wednesday, June 29, 2022 11:07 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Tue, Jun 28, 2022 at 5:43 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> > wrote:
> > >
> > > 5.
> > > +static ObjTree *
> > > +deparse_CreateStmt(Oid objectId, Node *parsetree)
> > > {
> > > ...
> > > + tmp = new_objtree_VA("TABLESPACE %{tablespace}I", 0); if
> > > + (node->tablespacename) append_string_object(tmp, "tablespace",
> > > + node->tablespacename); else { append_null_object(tmp, "tablespace");
> > > + append_bool_object(tmp, "present", false); }
> > > + append_object_object(createStmt, "tablespace", tmp);
> > > ...
> > > }
> > >
> > > Why do we need to append the objects (tablespace, with clause, etc.)
> > > when they are not present in the actual CREATE TABLE statement? The
> > > reason to ask this is that this makes the string that we want to send
> > > downstream much longer than the actual statement given by the user on
> > > the publisher.
> > >
> >
> > After thinking some more on this, it seems the user may want to optionally
> > change some of these attributes, for example, on the subscriber, it may want to
> > associate the table with a different tablespace. I think to address that we can
> > append these additional attributes optionally, say via an additional parameter
> > (append_all_options/append_all_attributes or something like that) in exposed
> > APIs like deparse_utility_command().
>
> I agree and will research this part.
>

Okay, note that similar handling would be required at other places
like deparse_ColumnDef. Few other comments on
v9-0001-Functions-to-deparse-DDL-commands.

1.
+static ObjElem *new_bool_object(bool value);
+static ObjElem *new_string_object(char *value);
+static ObjElem *new_object_object(ObjTree *value);
+static ObjElem *new_array_object(List *array);
+static ObjElem *new_integer_object(int64 value);
+static ObjElem *new_float_object(float8 value);

Here, new_object_object() seems to be declared out-of-order (not in
sync with its order of definition). Similarly, see all other append_*
functions.

2. The function printTypmod in ddl_deparse.c appears to be the same as
the function with the same name in format_type.c. If so, isn't it
better to have a single copy of that function?

3. As I pointed out yesterday, there is some similarity between
format_type_extended and format_type_detailed. Can we try to extract
common functionality? If this is feasible, it is better to do this as
a separate patch. Also, this can obviate the need to expose
printTypmod from format_type.c. I am not really sure if this will be
better than the current one or not but it seems worth trying.

4.
new_objtree_VA()
{
...
switch (type)
+ {
+ case ObjTypeBool:
+ elem = new_bool_object(va_arg(args, int));
+ break;
+ case ObjTypeString:
+ elem = new_string_object(va_arg(args, char *));
+ break;
+ case ObjTypeObject:
+ elem = new_object_object(va_arg(args, ObjTree *));
+ break;
+ case ObjTypeArray:
+ elem = new_array_object(va_arg(args, List *));
+ break;
+ case ObjTypeInteger:
+ elem = new_integer_object(va_arg(args, int64));
+ break;
+ case ObjTypeFloat:
+ elem = new_float_object(va_arg(args, double));
+ break;
+ case ObjTypeNull:
+ /* Null params don't have a value (obviously) */
+ elem = new_null_object();
...

I feel here ObjType's should be handled in the same order as they are
defined in ObjType.

5. There appears to be common code among node_*_object() functions.
Can we just have one function instead new_object(ObjType, void *val)?
In the function based on type, we can typecast the value. Is there a
reason why that won't be better than current one?

6.
deparse_ColumnDef()
{
...
/* Composite types use a slightly simpler format string */
+ if (composite)
+ column = new_objtree_VA("%{name}I %{coltype}T %{collation}s",
+ 3,
+ "type", ObjTypeString, "column",
+ "name", ObjTypeString, coldef->colname,
+ "coltype", ObjTypeObject,
+ new_objtree_for_type(typid, typmod));
+ else
+ column = new_objtree_VA("%{name}I %{coltype}T %{default}s
%{not_null}s %{collation}s",
+ 3,
+ "type", ObjTypeString, "column",
+ "name", ObjTypeString, coldef->colname,
+ "coltype", ObjTypeObject,
+ new_objtree_for_type(typid, typmod));
...
}

To avoid using the same arguments, we can define fmtstr for composite
and non-composite types as the patch is doing in deparse_CreateStmt().

7. It is not clear from comments or otherwise what should be
considered for default format string in functions like
deparse_ColumnDef() or deparse_CreateStmt().

8.
+ * FIXME --- actually, what about default values?
+ */
+static ObjTree *
+deparse_ColumnDef_typed(Relation relation, List *dpcontext, ColumnDef *coldef)

I think we need to handle default values for this FIXME.

--
With Regards,
Amit Kapila.