Re: Added schema level support for publication.

On Thu, Sep 2, 2021 at 5:12 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> 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?

Modified

> 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.

Modified

> 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'?

Modified

> 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.

I have removed get_namespace_name, no need to validate the schemaid,
fetch_search_path will return valid schemaids.

> 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);

Modified

> 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.

Modified

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

Modified to remove the check

> 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?

Modified.

> 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.

That is not required, modified.

Attached v25 patch has the changes for the same.
Currently I have used a different way to parse which does not require
"Add PublicationTable and PublicationRelInfo structs" committed
changes, I have currently removed the changes in this patch. I will
analyze further and use whichever parsing is better. I will handle
this in the next version.

Regards,
Vignesh