Re: Support logical replication of DDLs

On Tue, Jun 6, 2023 at 4:26 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
>
> Few assorted comments:
> ===================
>

Few comments on 0008* patch:
==============================
1. After 0008*, deparse_CreateStmt(), forms dpcontext before forming
identity whereas it is required only after it. It may not matter
practically but it is better to do the work when it is required. Also,
before 0008, it appears to be formed after identity, so not sure if
the change in 0008 is intentional, if so, then please let me know the
reason, and may be it is better to add a comment for the same.

2. Similarly, what is need to separately do insert_identity_object()
in deparse_CreateStmt() instead of directly doing something like
new_objtree_for_qualname() as we are doing in 0001 patch? For this, I
guess you need objtype handling in new_jsonb_VA().

3.
/*
* It will be of array type for multi-columns table, so lets begin
* an arrayobject. deparse_TableElems_ToJsonb() will add elements
* to it.
*/
pushJsonbValue(&state, WJB_BEGIN_ARRAY, NULL);

deparse_TableElems_ToJsonb(state, relation, node->tableElts, dpcontext,
false, /* not typed table */
false); /* not composite */
deparse_Constraints_ToJsonb(state, objectId);

pushJsonbValue(&state, WJB_END_ARRAY, NULL);

This part of the code is repeated in deparse_CreateStmt(). Can we move
this to a separate function?

4.
* Note we ignore constraints in the parse node here; they are extracted from
* system catalogs instead.
*/

static void
deparse_TableElems_ToJsonb(JsonbParseState *state, Relation relation,

An extra empty line between the comments end and function appears unnecessary.

5.
+ /* creat name and type elements for column */

/creat/create

--
With Regards,
Amit Kapila.