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

On Tue, Sep 14, 2021 at 2:08 PM vignesh C <vignesh21@gmail.com> wrote:
>
> I have handled this in the patch attached.
>

Few comments:
=============
1.
+ * CREATE PUBLICATION FOR pub_obj [, pub_obj2] [WITH options]
+ *
+ * pub_obj is one of:
+ *
+ * TABLE table [, table2]
..
..
- * ALTER PUBLICATION name ADD TABLE table [, table2]
+ * ALTER PUBLICATION name ADD pub_obj [, pub_obj ...]
  *
- * ALTER PUBLICATION name DROP TABLE table [, table2]
+ * ALTER PUBLICATION name DROP pub_obj [, pub_obj ...]
  *
- * ALTER PUBLICATION name SET TABLE table [, table2]
+ * ALTER PUBLICATION name SET pub_obj [, pub_obj ...]

In all the above places, the object names mentioned in square brackets
are not consistent. I suggest using [, ...] everywhere as that is what
we are using in docs as well.

2.
+/*
+ * Check if the relation schema is member of the schema list.
+ */
+static void
+RelSchemaIsMemberOfSchemaList(List *rels, List *schemaidlist, bool schemacheck)

Can we change the above comment as: "Check if any of the given
relation's schema is a member of the given schema list."?

3.
+ errmsg("cannot add relation \"%s.%s\" to publication",
+ get_namespace_name(relSchemaId),
+ RelationGetRelationName(rel)),
+ errdetail("Table's schema \"%s\" is already part of the publication.",
+ get_namespace_name(relSchemaId)));

This and other parts of error messages in
+RelSchemaIsMemberOfSchemaList are not aligned. I think you can now
run pgindent on your patches that will solve the indentation issues in
the patch.

4.
AlterPublicationSchemas()
{
..
+ /*
+ * If the table option was not specified remove the existing tables
+ * from the publication.
+ */
+ if (!tables)
+ {
+ rels = GetPublicationRelations(pubform->oid, PUBLICATION_PART_ROOT);
+ PublicationDropTables(pubform->oid, rels, false, true);
+ }
+
+ /* Identify which schemas should be dropped */
+ delschemas = list_difference_oid(oldschemaids, schemaidlist);
+
+ /* And drop them */
+ PublicationDropSchemas(pubform->oid, delschemas, true);

Here, you have neither locked tables to be dropped nor schemas. I
think both need to be locked as we do for tables in similar code in
AlterPublicationTables(). Can you please test via debugger what
happens if we try to drop without taking lock here and concurrently
try to drop the actual object? It should give some error. If we decide
to lock here then we should be able to pass the list of relations to
PublicationDropTables() instead of Oids which would then obviate the
need for any change to that function.

Similarly don't we need to lock schemas before dropping them in
AlterPublicationTables()?

5.
+/*
+ * Find the ObjectAddress for a publication tables in schema.  The first
+ * element of the object parameter is the schema name, the second is the
+ * publication name.
+ */
+static ObjectAddress
+get_object_address_publication_schema(List *object, bool missing_ok)

The first part of the above comment is not clear. Can we write it as:
"Find the ObjectAddress for a publication schema. .."?

6.
+List *
+GetSchemaPublicationRelations(Oid schemaid, PublicationPartOpt pub_partopt)
+{
+ Relation classRel;
+ ScanKeyData key[3];
+ TableScanDesc scan;
+ HeapTuple tuple;
+ List    *result = NIL;
+ int keycount = 0;
+
+ Assert(schemaid != InvalidOid);

Isn't it better to use OidIsValid() in the above assert?

7.
@@ -974,6 +974,7 @@ EventTriggerSupportsObjectType(ObjectType obtype)
  case OBJECT_PROCEDURE:
  case OBJECT_PUBLICATION:
  case OBJECT_PUBLICATION_REL:
+ case OBJECT_PUBLICATION_REL_IN_NAMESPACE:
  case OBJECT_ROUTINE:
  case OBJECT_RULE:
  case OBJECT_SCHEMA:
@@ -1050,6 +1051,7 @@ EventTriggerSupportsObjectClass(ObjectClass objclass)
  case OCLASS_EXTENSION:
  case OCLASS_POLICY:
  case OCLASS_PUBLICATION:
+ case OCLASS_PUBLICATION_NAMESPACE:
  case OCLASS_PUBLICATION_REL:
  case OCLASS_SUBSCRIPTION:
  case OCLASS_TRANSFORM:
@@ -2127,6 +2129,7 @@ stringify_grant_objtype(ObjectType objtype)
  case OBJECT_POLICY:
  case OBJECT_PUBLICATION:
  case OBJECT_PUBLICATION_REL:
+ case OBJECT_PUBLICATION_REL_IN_NAMESPACE:
  case OBJECT_ROLE:
  case OBJECT_RULE:
  case OBJECT_STATISTIC_EXT:
@@ -2209,6 +2212,7 @@ stringify_adefprivs_objtype(ObjectType objtype)
  case OBJECT_POLICY:
  case OBJECT_PUBLICATION:
  case OBJECT_PUBLICATION_REL:
+ case OBJECT_PUBLICATION_REL_IN_NAMESPACE:

What is the reason for using different names for object_class and
object_type? Normally, we use the same. Is it making things clear in
any place?

8.
+ if (stmt->action == DEFELEM_ADD)
+ {
+ List *pubschemas = GetPublicationSchemas(pubid);
+
+ /* check if the relation is member of the schema list specified */
+ RelSchemaIsMemberOfSchemaList(rels, schemaidlist, false);
+
+ /*
+ * Check if the relation is member of the existing schema in the
+ * publication.
+ */
+ RelSchemaIsMemberOfSchemaList(rels, pubschemas, false);

Isn't it better to concat the list of schemas and then check the
membership of relations once?

-- 
With Regards,
Amit Kapila.