Re: Support logical replication of DDLs

On Fri, Jun 16, 2023 at 4:01 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> With these changes, I hope the patch-set is somewhat easier to review.
>

Few comments:
=============
1.
+static Jsonb *
+deparse_CreateStmt(Oid objectId, Node *parsetree)
{
...
+ /* PERSISTENCE */
+ appendStringInfoString(&fmtStr, "CREATE %{persistence}s TABLE");
+ new_jsonb_VA(state, NULL, NULL, false, 1,
+ "persistence", jbvString,
+ get_persistence_str(relation->rd_rel->relpersistence));

Do we need to add key/value pair if get_persistence_str() returns an
empty string in the default deparsing mode? Won't it be somewhat
inconsistent with other objects?

2.
+static JsonbValue *
+new_jsonb_VA(JsonbParseState *state, char *parentKey, char *fmt,
+ bool createChildObj, int numobjs,...)
+{
+ va_list args;
+ int i;
+ JsonbValue val;
+ JsonbValue *value = NULL;
+
+ /*
+ * Insert parent key for which we are going to create value object here.
+ */
+ if (parentKey)
+ insert_jsonb_key(state, parentKey);
+
+ /* Set up the toplevel object if not instructed otherwise */
+ if (createChildObj)
+ pushJsonbValue(&state, WJB_BEGIN_OBJECT, NULL);
+
+ /* Set up the "fmt" */
+ if (fmt)
+ fmt_to_jsonb_element(state, fmt);

I think it would be better to handle parentKey, childObj, and fmt in
the callers as this function doesn't seem to be the ideal place to
deal with those. I see that in some cases we already handle those in
the callers. It is bit confusing in which case callers need to deal
vs. the cases where we need to deal here.

3.
+static Jsonb *
+deparse_AlterSeqStmt(Oid objectId, Node *parsetree)
{
...
+
+ new_jsonb_VA(state, NULL, "ALTER SEQUENCE %{identity}D %{definition: }s",
+ false, 0);

Is there a need to call new_jsonb_VA() just to insert format? Won't it
better to do this in the caller itself?

4. The handling for if_not_exists appears to be different in
deparse_CreateSeqStmt() and deparse_CreateStmt(). I think the later
one is correct and we should do that in both places. That means
probably we can't have the entire format key in the beginning of
deparse_CreateSeqStmt().

5.
+ /*
+ * Check if table elements are present, if so, add them. This function
+ * call takes care of both the check and addition.
+ */
+ telems = insert_table_elements(state, &fmtStr, relation,
+ node->tableElts, dpcontext, objectId,
+ false, /* not typed table */
+ false); /* not composite */

Would it be better to name this function to something like
add_table_elems_if_any()? If so, we can remove second part of the
comment: "This function call takes care of both the check and
addition." as that would be obvious from the function name.

6.
+ /*
+ * Check if table elements are present, if so, add them. This function
+ * call takes care of both the check and addition.
+ */
+ telems = insert_table_elements(state, &fmtStr, relation,
+ node->tableElts, dpcontext, objectId,
+ false, /* not typed table */
+ false); /* not composite */
+
+ /*
+ * If no table elements added, then add empty "()" needed for 'inherit'
+ * create table syntax. Example: CREATE TABLE t1 () INHERITS (t0);
+ */
+ if (!telems)
+ appendStringInfoString(&fmtStr, " ()");

In insert_table_elements(), sometimes we access system twice for each
of the columns and this is to identify the above case where no
elements are present. Would it be better if simply for zero element
object array in this case and detect the same on the receiving side?
If this is feasible then we can simply name the function as
add_table_elems/add_table_elements. Also, in this context, can we
change the variable name telems to telems_present to make it bit easy
to follow.

7. It would be better if we can further split the patch to move Alter
case into a separate patch. That will help us to focus on reviewing
Create/Drop in detail.

--
With Regards,
Amit Kapila.