Re: Support logical replication of DDLs - Mailing list pgsql-hackers

On Fri, 14 Apr 2023 at 13:06, vignesh C <vignesh21@gmail.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