Re: Added schema level support for publication. - Mailing list pgsql-hackers
From | Amit Kapila |
---|---|
Subject | Re: Added schema level support for publication. |
Date | |
Msg-id | CAA4eK1Lu8fycAAem1YXdJDtCYW6fD5746QnP4GRHovcJ59r2wg@mail.gmail.com Whole thread Raw |
In response to | Re: Added schema level support for publication. (vignesh C <vignesh21@gmail.com>) |
Responses |
Re: Added schema level support for publication.
RE: Added schema level support for publication. Re: Added schema level support for publication. |
List | pgsql-hackers |
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. 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.
pgsql-hackers by date: