Re: Support logical replication of DDLs

On Mon, Jun 19, 2023 at 3:04 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Fri, Jun 16, 2023 at 4:01 PM shveta malik <shveta.malik@gmail.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?
>

Modified it to add 'persistence' only when we have it non-null.

> 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.
>

Moved these things outside of new_jsonb_VA().

> 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?
>

Now  in the latest version, "fmt" is inserted as a normal key-value
pair only, no special handling for this. And thus above call is
retained but with numObjs as 1.

> 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().
>

Modified.

> 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.
>

Modified.

> 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.

Modified telems to telems_present. I am reviewing the first part.
Please allow some more time.

>
> 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.
>

Done. Alter-table deparsing is now patch 0002.

======

Apart from above, did some more optimization on similar lines (i.e.
add elements only if needed) and added 'syntax' related comments for
each alter-table subcmd.

thanks
Shveta