On Thu, Sep 2, 2021 at 5:12 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Thu, Sep 2, 2021 at 11:58 AM vignesh C <vignes...@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.
