Re: Added schema level support for publication.

On Thu, Sep 2, 2021 at 5:12 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Sep 2, 2021 at 11:58 AM vignesh C <vignesh21@gmail.com> wrote:
> >
>
> Below are few comments on v23. If you have already addressed anything
> in v24, then ignore it.
>

More Review comments (on latest version v24):
=======================================
1.
+    Oid            psnspcid BKI_LOOKUP(pg_class);    /* Oid of the schema */
+} FormData_pg_publication_schema;

Why in the above structure you have used pg_class instead of pg_namespace?

2. GetSchemaPublicationRelations() uses two different ways to skip
non-publishable relations in two while loops. Both are correct but I
think it would be easier to follow if they use the same way and in
this case I would prefer a check like if (is_publishable_class(relid,
relForm)). The comments atop function could also be modified to :"Get
the list of publishable relation oids for a specified schema.". I
think it is better to write some comments on why you need to scan and
loop twice.

3. The other point about GetSchemaPublicationRelations() is that I am
afraid that it will collect duplicate relation oids in the final list
when the partitioned table and child tables are in the same schema.

4. In GetRelationPublicationActions(), the handling related to
partitions seems to be missing for the schema. It won't be able to
take care of child tables when they are in a different schema than the
parent table.

5.
If I modify the search path to remove public schema then I get the
below error message:
postgres=# Create publication mypub for all tables in schema current_schema;
ERROR:  no schema has been selected

I think this message is not very clear. How about changing to
something like "current_schema doesn't contain any valid schema"? This
message is used in more than one place, so let's keep it the same at
all the places if you agree to change it.

6. How about naming ConvertPubObjSpecListToOidList() as
ObjectsInPublicationToOids()? I see somewhat similarly named functions
in the code like objectsInSchemaToOids, objectNamesToOids.

7.
+ /*
+ * Schema lock is held until the publication is created to prevent
+ * concurrent schema deletion. No need to unlock the schemas, the
+ * locks will be released automatically at the end of create
+ * publication command.
+ */

In this comment no need to say create publication command as that is
implicit, we can just write ".... at the end of command".

8. Can you please extract changes like tab-completion, dump/restore in
separate patches? This can help to review the core (backend) part of
the patch in more detail.

-- 
With Regards,
Amit Kapila.