Re: Support logical replication of DDLs

On Fri, 14 Apr 2023 at 13:06, vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> Few comments:

Some more comments on 0001 patch:
Few comments:
1) We could add a space after the 2nd parameter
+ * Note we don't have the luxury of sprintf-like compiler warnings for
+ * malformed argument lists.
+ */
+static ObjTree *
+new_objtree_VA(char *fmt, int numobjs,...)

2) I felt objtree_to_jsonb_element is a helper function for
objtree_to_jsonb_rec, we should update the comments accordingly:
+/*
+ * Helper for objtree_to_jsonb: process an individual element from an object or
+ * an array into the output parse state.
+ */
+static void
+objtree_to_jsonb_element(JsonbParseState *state, ObjElem *object,
+ JsonbIteratorToken elem_token)
+{
+ JsonbValue val;
+
+ switch (object->objtype)
+ {
+ case ObjTypeNull:
+ val.type = jbvNull;
+ pushJsonbValue(&state, elem_token, &val);
+ break;

3) domainId parameter change should be removed from the first patch:
+static List *
+obtainConstraints(List *elements, Oid relationId, Oid domainId,
+ ConstraintObjType objType)
+{
+ Relation conRel;
+ ScanKeyData key;
+ SysScanDesc scan;
+ HeapTuple tuple;
+ ObjTree *constr;
+ Oid relid;
+
+ /* Only one may be valid */
+ Assert(OidIsValid(relationId) ^ OidIsValid(domainId));
+
+ relid = OidIsValid(relationId) ? ConstraintRelidTypidNameIndexId :
+ ConstraintTypidIndexId;

4) Do we have any scenario for CONSTRAINT_TRIGGER in table/index, if
so could we add a test for this?
+ case CONSTRAINT_UNIQUE:
+ contype = "unique";
+ break;
+ case CONSTRAINT_TRIGGER:
+ contype = "trigger";
+ break;
+ case CONSTRAINT_EXCLUSION:
+ contype = "exclusion";
+ break;

5) The below code adds information about compression but the comment
says "USING clause", the comment should be updated accordingly:
+ /* USING clause */
+ tmp_obj = new_objtree("COMPRESSION");
+ if (coldef->compression)
+ append_string_object(tmp_obj, "%{compression_method}I",
+
"compression_method", coldef->compression);
+ else
+ {
+ append_null_object(tmp_obj, "%{compression_method}I");
+ append_not_present(tmp_obj);
+ }
+ append_object_object(ret, "%{compression}s", tmp_obj);

6) Generally we add append_null_object followed by append_not_present,
but it is not present for "COLLATE" handling, is this correct?
+ tmp_obj = new_objtree("COMPRESSION");
+ if (coldef->compression)
+ append_string_object(tmp_obj, "%{compression_method}I",
+
"compression_method", coldef->compression);
+ else
+ {
+ append_null_object(tmp_obj, "%{compression_method}I");
+ append_not_present(tmp_obj);
+ }
+ append_object_object(ret, "%{compression}s", tmp_obj);
+
+ tmp_obj = new_objtree("COLLATE");
+ if (OidIsValid(typcollation))
+ append_object_object(tmp_obj, "%{name}D",
+
new_objtree_for_qualname_id(CollationRelationId,
+
typcollation));
+ else
+ append_not_present(tmp_obj);
+ append_object_object(ret, "%{collation}s", tmp_obj);

7) I felt attrTup can be released after get_atttypetypmodcoll as we
are not using the tuple after that, I'm not sure if it is required to
hold the reference to this tuple till the end of the function:
+static ObjTree *
+deparse_ColumnDef_typed(Relation relation, List *dpcontext, ColumnDef *coldef)
+{
+ ObjTree *ret = NULL;
+ ObjTree *tmp_obj;
+ Oid relid = RelationGetRelid(relation);
+ HeapTuple attrTup;
+ Form_pg_attribute attrForm;
+ Oid typid;
+ int32 typmod;
+ Oid typcollation;
+ bool saw_notnull;
+ ListCell *cell;
+
+ attrTup = SearchSysCacheAttName(relid, coldef->colname);
+ if (!HeapTupleIsValid(attrTup))
+ elog(ERROR, "could not find cache entry for column
\"%s\" of relation %u",
+ coldef->colname, relid);
+ attrForm = (Form_pg_attribute) GETSTRUCT(attrTup);
+
+ get_atttypetypmodcoll(relid, attrForm->attnum,
+ &typid, &typmod,
&typcollation);
+
+ /*
+ * Search for a NOT NULL declaration. As in deparse_ColumnDef,
we rely on
+ * finding a constraint on the column rather than coldef->is_not_null.
+ * (This routine is never used for ALTER cases.)
+ */
+ saw_notnull = false;
+ foreach(cell, coldef->constraints)
+ {
+ Constraint *constr = (Constraint *) lfirst(cell);
+
+ if (constr->contype == CONSTR_NOTNULL)
+ {
+ saw_notnull = true;
+ break;
+ }
+ }

8) This looks like ALTER TABLE ... SET/RESET, the function header
should be updated accordingly:
/*
* ... ALTER COLUMN ... SET/RESET (...)
*
* Verbose syntax
* RESET|SET (%{options:, }s)
*/
static ObjTree *
deparse_RelSetOptions(AlterTableCmd *subcmd)
{
List *sets = NIL;
ListCell *cell;
bool is_reset = subcmd->subtype == AT_ResetRelOptions;

9) Since we don't replicate temporary tables, is this required:
+/*
+ * Deparse the ON COMMIT ... clause for CREATE ... TEMPORARY ...
+ *
+ * Verbose syntax
+ * ON COMMIT %{on_commit_value}s
+ */
+static ObjTree *
+deparse_OnCommitClause(OnCommitAction option)
+{
+ ObjTree *ret = new_objtree("ON COMMIT");
+ switch (option)

10) Since we don't support MATERIALIZED VIEW, VIEW and FOREIGN TABLE,
they can be removed:
+ switch (rel->rd_rel->relkind)
+ {
+ case RELKIND_RELATION:
+ case RELKIND_PARTITIONED_TABLE:
+ reltype = "TABLE";
+ break;
+ case RELKIND_INDEX:
+ case RELKIND_PARTITIONED_INDEX:
+ reltype = "INDEX";
+ break;
+ case RELKIND_VIEW:
+ reltype = "VIEW";
+ break;
+ case RELKIND_COMPOSITE_TYPE:
+ reltype = "TYPE";
+ istype = true;
+ break;
+ case RELKIND_FOREIGN_TABLE:
+ reltype = "FOREIGN TABLE";
+ break;
+ case RELKIND_MATVIEW:
+ reltype = "MATERIALIZED VIEW";
+ break;

Regards,
Vignesh