Re: Added schema level support for publication. - Mailing list pgsql-hackers

On Thu, Oct 21, 2021 at 6:47 PM vignesh C <vignesh21@gmail.com> wrote:
>
>
> Thanks for the comments, the attached v45 patch has the fix for the same.
>

The first patch is mostly looking good to me apart from the below
minor comments:

1.
+  <para>
+   The catalog <structname>pg_publication_namespace</structname> contains the
+   mapping between schemas and publications in the database.  This is a
+   many-to-many mapping.

There are extra spaces after mapping at the end which are not required.

2.
+   <literal>CREATE</literal> privilege on the database.  Also, the new owner
+   of a <literal>FOR ALL TABLES</literal> publication must be a superuser.

I think we can modify the second line as: "Also, the new owner of a
<literal>FOR ALL TABLES</literal> or <literal>FOR ALL TABLES IN
SCHEMA</literal> publication must be a superuser.

3.
/table's schema as part of specified schema is not supported./table's
schema as part of the specified schema is not supported.

4.
+  <para>
+   Create a publication that publishes all changes for tables
+   <structname>users</structname>, <structname>departments</structname> and
+   that publishes all changes for all the tables present in the schema
+   <structname>production</structname>:

I don't think '...that publishes...' is required twice in the above sentence.

5.
+static List *OpenReliIdList(List *relids);
 static List *OpenTableList(List *tables);
 static void CloseTableList(List *rels);
 static void PublicationAddTables(Oid pubid, List *rels, bool if_not_exists,
  AlterPublicationStmt *stmt);
 static void PublicationDropTables(Oid pubid, List *rels, bool missing_ok);
+static void LockSchemaList(List *schemalist);
+static void PublicationAddSchemas(Oid pubid, List *schemas, bool if_not_exists,
+   AlterPublicationStmt *stmt);
+static void PublicationDropSchemas(Oid pubid, List *schemas, bool missing_ok);

Keep the later definitions also in this order. I suggest move
LockSchemaList() just after CloseTableList() both in declaration and
definition.

6.
+/*
+ * Convert the PublicationObjSpecType list into schema oid list and rangevar
+ * list.
+ */

I think you need to say publication table instead of rangevar in the
above comment.

7.
+ /*
+ * It is quite possible that for the SET case user has not specified any
+ * schema in which case we need to remove all the existing schemas.
+ */

/schema/schemas

8.
+/*
+ * Open relations specified by a RangeVar list.

/RangeVar/PublicationTable

9.
+static bool
+_equalPublicationObject(const PublicationObjSpec *a,
+ const PublicationObjSpec *b)
+{
+ COMPARE_SCALAR_FIELD(pubobjtype);
+ COMPARE_STRING_FIELD(name);
+ COMPARE_NODE_FIELD(pubtable);
+ COMPARE_LOCATION_FIELD(location);
+
+ return true;
+}
+

Let's define this immediately before _equalPublicationTable as all
publication functions are defined there. Also, make the handling of
T_PublicationObjSpec before T_PublicationTable in equal() function as
that is the way nodes are defined.

-- 
With Regards,
Amit Kapila.