Re: Added schema level support for publication.

On Thu, Sep 2, 2021 at 11:58 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>

Below are few comments on v23. If you have already addressed anything
in v24, then ignore it.

1. The commit message says: A new system table "pg_publication_schema"
has been added, to maintain the schemas that the user wants to publish
through the publication.". The alternative name for this system table
could be "pg_publication_namespace". The reason why this alternative
comes to my mind is that the current system table to store schema
information is named pg_namespace. So shouldn't we be consistent here?
What do others think about this?

2. In function check_publication_add_schema(), the primary error
message should be "cannot add schema \"%s\" to publication". See
check_publication_add_relation() for similar error messages.

3.
+ObjectAddress
+publication_add_schema(Oid pubid, Oid schemaoid, bool if_not_exists)

Isn't it better to use 'schemaid' so that it is consistent with 'pubid'?

4.
ConvertPubObjSpecListToOidList()
{
..
+ schemaoid = linitial_oid(search_path);
+ nspname = get_namespace_name(schemaoid);
+ if (nspname == NULL) /* recently-deleted namespace? */
+ ereport(ERROR,
+ errcode(ERRCODE_UNDEFINED_SCHEMA),
+ errmsg("no schema has been selected"));
+
+ list_free(search_path);
..
}

You can free the memory for 'nspname' as that is not used afterward.

5.
+ schemaRels = GetSchemaPublicationRelations(schemaoid, PUBLICATION_PART_ALL);
+
+ /* Invalidate relcache so that publication info is rebuilt. */
+ InvalidatePublicationRels(schemaRels);

It is better to write this comment above
GetSchemaPublicationRelations, something like below:

+ /* Invalidate relcache so that publication info is rebuilt. */
+ schemaRels = GetSchemaPublicationRelations(schemaoid, PUBLICATION_PART_ALL);
+ InvalidatePublicationRels(schemaRels);

6.
+static List *
+GetPubPartitionOptionRelations(List *result, PublicationPartOpt pub_partopt,
+ Oid relid)

I think it is better to name this function as
GetPublicationPartOptRelations as that way it will be more consistent
with existing functions and structure name PublicationPartOpt.

7. All the callers of PublicationAddSchemas() have a superuser check,
then why there is again a check of owner/superuser in that function?

8.
+/*
+ * Gets the list of FOR ALL TABLES IN SCHEMA publication oids associated with a
+ * specified schema oid
+ */
+List *
+GetSchemaPublications(Oid schemaid)

I find it a bit difficult to read this comment. Can we omit "FOR ALL
TABLES IN SCHEMA" from this comment?

9. In the doc patch
(v23-0003-Tests-and-documentation-for-schema-level-support), I see
below line:
<para>
- To add a table to a publication, the invoking user must have ownership
- rights on the table. The <command>FOR ALL TABLES</command> clause requires
- the invoking user to be a superuser.
+ To add a table/schema to a publication, the invoking user must have
+ ownership rights on the table/schema. The <command>FOR ALL TABLES</command>
+ and <command>FOR ALL TABLES IN SCHEMA</command> clause requires the invoking
+ user to be a superuser.

Is it correct to specify the schema in the first line? AFAIU, all
forms of schema addition requires superuser privilege.

--
With Regards,
Amit Kapila.