Re: Added schema level support for publication.

Hi,

On Mon, Oct 18, 2021 at 3:14 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Saturday, October 16, 2021 1:57 PM houzj.fnst@fujitsu.com wrote:
> > Based on the V40 patchset, attaching the Top-up patch which try to fix the
> > partition issue in a cleaner way.
>
> Attach the new version patch set which merge the partition fix into it.
> Besides, instead of introducing new function and parameter, just add the
> partition filter in pg_get_publication_tables which makes the code cleaner.
>
> Only 0001 and 0003 was changed.

I've reviewed 0001 and 0002 patch and here are comments:

0001 patch:

+/*
+ * Get the list of publishable relation oids for a specified schema.
+ *
+ * Schema will be having both ordinary('r') relkind tables and partitioned('p')
+ * relkind tables, so two rounds of scan are required.
+ */
+List *
+GetSchemaPublicationRelations(Oid schemaid, PublicationPartOpt pub_partopt)
+{
+        Relation       classRel;
+        ScanKeyData key[3];
+        TableScanDesc scan;

I think it's enough to have key[2], not key[3].

BTW, this function does the table scan on pg_class twice in order to
get OIDs of both normal tables and partitioned tables. But can't we do
that by the single table scan? I think we can set a scan key for
relnamespace, and check relkind inside a scan loop.

---
+                ObjectsInPublicationToOids(stmt->pubobjects, pstate,
&relations,
+
&schemaidlist);
+
+                if (list_length(relations) > 0)
+                {
+                        List      *rels;
+
+                        rels = OpenTableList(relations);
+                        CheckObjSchemaNotAlreadyInPublication(rels,
schemaidlist,
+
PUBLICATIONOBJ_TABLE);
+                        PublicationAddTables(puboid, rels, true, NULL);
+                        CloseTableList(rels);
+                }
+
+                if (list_length(schemaidlist) > 0)
+                {
+                        /* FOR ALL TABLES IN SCHEMA requires superuser */
+                        if (!superuser())
+                                ereport(ERROR,
+
errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                                                errmsg("must be
superuser to create FOR ALL TABLES IN SCHEMA publication"));
+

Perhaps we can do a superuser check before handling "relations"? If
the user doesn't have the permission, we don't need to do anything for
relations.

0002 patch:

postgres(1:13619)=# create publication pub for all TABLES in schema
CURRENT_SCHEMA      pg_catalog          public              s2
information_schema  pg_toast            s1

Since pg_catalog and pg_toast cannot be added to the schema
publication can we exclude them from the completion list?

Regards,

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