Re: Support logical replication of DDLs

On Wed, Apr 12, 2023 at 4:53 PM Amit Kapila  wrote:
>
>
> Few comments on 0001
> ===================
>

Some more comments on 0001
==========================

1.
+/*
+ * Subroutine for CREATE TABLE/CREATE DOMAIN deparsing.
+ *
+ * Given a table OID or domain OID, obtain its constraints and append them to
+ * the given elements list.  The updated list is returned.
+ *
+ * This works for typed tables, regular tables, and domains.
+ *
+ * Note that CONSTRAINT_FOREIGN constraints are always ignored.
+ */
+static List *
+obtainConstraints(List *elements, Oid relationId, Oid domainId,
+   ConstraintObjType objType)

Why do we need to support DOMAIN in this patch? Isn't this only for tables?

2.
obtainConstraints()
{
..
+ switch (constrForm->contype)
+ {
+ case CONSTRAINT_CHECK:
+ contype = "check";
+ break;
+ case CONSTRAINT_FOREIGN:
+ continue; /* not here */
+ case CONSTRAINT_PRIMARY:
+ contype = "primary key";
+ break;
+ case CONSTRAINT_UNIQUE:
+ contype = "unique";
+ break;
+ case CONSTRAINT_TRIGGER:
+ contype = "trigger";
+ break;
+ case CONSTRAINT_EXCLUSION:
+ contype = "exclusion";
+ break;
+ default:
+ elog(ERROR, "unrecognized constraint type");

It looks a bit odd that except CONSTRAINT_NOTNULL all other
constraints are handled here. I think the reason is callers themselves
deal with not null constraints, if so, we can probably add a comment.

3.
obtainConstraints()
{
...
+ if (constrForm->conindid &&
+ (constrForm->contype == CONSTRAINT_PRIMARY ||
+ constrForm->contype == CONSTRAINT_UNIQUE ||
+ constrForm->contype == CONSTRAINT_EXCLUSION))
+ {
+ Oid   tblspc = get_rel_tablespace(constrForm->conindid);
+
+ if (OidIsValid(tblspc))
+ append_string_object(constr,
+ "USING INDEX TABLESPACE %{tblspc}s",
+ "tblspc",
+ get_tablespace_name(tblspc));
...
}

How is it guaranteed that we can get tablespace_name after getting id?
If there is something that prevents tablespace from being removed
between these two calls then it could be better to write a comment for
the same.

4. It seems RelationGetColumnDefault() assumed that the passed
attribute always had a default because it didn't verify the return
value of build_column_default(). Now, all but one of its callers in
deparse_ColumnDef() check that attribute has a default value before
calling this function. I think either we change that caller or have an
error handling in RelationGetColumnDefault(). It might be better to
add a comment in RelationGetColumnDefault() to reflect that callers
ensure that the passed attribute has a default value and then have an
assert for it as well.

5.
+deparse_ColumnDef(Relation relation, List *dpcontext, bool composite,
+   ColumnDef *coldef, bool is_alter, List **exprs)
{
...
+ 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);
...
+ /* IDENTITY COLUMN */
+ if (coldef->identity)
+ {
+ Oid attno = get_attnum(relid, coldef->colname);
...

I think we don't need to perform additional syscache lookup to get
attno as we already have that in this function and is used at other
places.

6.
+deparse_ColumnDef(Relation relation, List *dpcontext, bool composite,
+   ColumnDef *coldef, bool is_alter, List **exprs)
{
...

+ seqrelid = getIdentitySequence(relid, attno, true);
+ if (OidIsValid(seqrelid) && coldef->identitySequence)
+ seqrelid = RangeVarGetRelid(coldef->identitySequence, NoLock, false);
...

It may be better to add some comments to explain what exactly are we doing here.

-- 
With Regards,
Amit Kapila.