Re: Added schema level support for publication.

On Fri, Aug 6, 2021 at 5:33 PM vignesh C <vignesh21@gmail.com> wrote:
>
> Thanks for the comments, the attached v19 patch has the fixes for the comments.

Thank you for updating the patch!

Here are some comments on v19 patch:

+                case OCLASS_PUBLICATION_SCHEMA:
+                        RemovePublicationSchemaById(object->objectId);
+                        break;

+void
+RemovePublicationSchemaById(Oid psoid)
+{
+        Relation       rel;
+        HeapTuple      tup;
+
+        rel = table_open(PublicationSchemaRelationId, RowExclusiveLock);
+
+        tup = SearchSysCache1(PUBLICATIONSCHEMA, ObjectIdGetDatum(psoid));
+
+        if (!HeapTupleIsValid(tup))
+                elog(ERROR, "cache lookup failed for publication schema %u",
+                         psoid);
+
+        CatalogTupleDelete(rel, &tup->t_self);
+
+        ReleaseSysCache(tup);
+
+        table_close(rel, RowExclusiveLock);
+}

Since RemovePublicationSchemaById() does simple catalog tuple
deletion, it seems to me that we can DropObjectById() to delete the
row of pg_publication_schema.

 ---
         {
-                ScanKeyInit(&key[0],
+                ScanKeyData skey[1];
+
+                ScanKeyInit(&skey[0],
                                         Anum_pg_class_relkind,
                                         BTEqualStrategyNumber, F_CHAREQ,

CharGetDatum(RELKIND_PARTITIONED_TABLE));

-                scan = table_beginscan_catalog(classRel, 1, key);
+                scan = table_beginscan_catalog(classRel, 1, skey);

Since we specify 1 as the number of keys in table_beginscan_catalog(),
can we reuse 'key' instead of using 'skey'?

---
Even if we drop all tables added to the publication from it, 'pubkind'
doesn't go back to 'empty'. Is that intentional behavior? If we do
that, we can save the lookup of pg_publication_rel and
pg_publication_schema in some cases, and we can switch the publication
that was created as FOR SCHEMA to FOR TABLE and vice versa.

---
+static void
+UpdatePublicationKindTupleValue(Relation rel, HeapTuple tup, int col,
+                                                                char pubkind)

Since all callers of this function specify Anum_pg_publication_pubkind
to 'col', it seems 'col' is not necessary.

---
+static void
+AlterPublicationSchemas(AlterPublicationStmt *stmt, Relation rel,
+                                                HeapTuple tup,
Form_pg_publication pubform)

I think we don't need to pass 'pubform' to this function since we can
get it by GETSTRUCT(tup).

---
+                Oid                    schemaId = get_rel_namespace(relid);
                 List      *pubids = GetRelationPublications(relid);
+                List      *schemaPubids = GetSchemaPublications(schemaId);

Can we defer to get the list of schema publications (i.g.,
'schemaPubids') until we find the PUBKIND_SCHEMA publication? Perhaps
the same is true for building 'pubids'.

---
+                                                   List of publications
+        Name        |          Owner           | All tables | Inserts
| Updates | Deletes | Truncates | Via root | PubKind

+--------------------+--------------------------+------------+---------+---------+---------+-----------+----------+---------
+ testpib_ins_trunct | regress_publication_user | f          | t
| f       | f       | f         | f        | e
+ testpub_default    | regress_publication_user | f          | f
| t       | f       | f         | f        | e

I think it's more readable if \dRp shows 'all tables', 'table',
'schema', and 'empty' in PubKind instead of the single character.

I think 'Pub kind' is more consistent with other column names.

Regards,

-- 
Masahiko Sawada
EDB:  https://www.enterprisedb.com/