Re: Added schema level support for publication.

On Thu, Oct 21, 2021 at 6:47 PM vignesh C <vignesh21(at)gmail(dot)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.