Re: Added schema level support for publication.

On Tue, Sep 7, 2021 at 5:10 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Sep 7, 2021 at 12:45 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > On Fri, Sep 3, 2021 at 4:49 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> >
> > > 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.
> >
> > I would prefer to use the existing messages as we have used this in a
> > few other places similarly. Thoughts?
> >
>
> Yeah, I also see the same message in code but I think here usage is a
> bit different. If you see a similar SQL statement that causes the same
> error message then can you please give an example?

Changed it to "no schema has been selected for CURRENT_SCHEMA"

> Few comments on latest patch
> (v25-0002-Added-schema-level-support-for-publication):
> =====================================================================
> 1.
> getPublicationSchemaInfo()
> ..
> + *nspname = get_namespace_name(pnform->pnnspcid);
> + if (!(*nspname))
> + {
> + Oid schemaid = pnform->pnpubid;
> +
> + pfree(*pubname);
> + ReleaseSysCache(tup);
> + if (!missing_ok)
> + elog(ERROR, "cache lookup failed for schema %u",
> + schemaid);
>
> Why are you using pnform->pnpubid to get schemaid? I think you need
> pnform->pnnspcid here and it was like that in the previous version,
> not sure, why you changed it?

Modified

> 2.
> @@ -369,15 +531,20 @@ AlterPublicationTables(AlterPublicationStmt
> *stmt, Relation rel,
>   (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
>   errmsg("publication \"%s\" is defined as FOR ALL TABLES",
>   NameStr(pubform->pubname)),
> - errdetail("Tables cannot be added to or dropped from FOR ALL TABLES
> publications.")));
> + errdetail("Tables cannot be added to, dropped from, or set on FOR
> ALL TABLES publications.")));
>
> Why is this message changed? Have we changed anything related to this
> as part of this patch?

This change is not required in this patch, reverted

> 3.
> + Oid pnnspcid BKI_LOOKUP(pg_namespace); /* Oid of the schema */
> +} FormData_pg_publication_namespace;
> +
> ...
> ...
> +DECLARE_UNIQUE_INDEX(pg_publication_namespace_pnnspcid_pnpubid_index,
> 8903, PublicationNamespacePnnspcidPnpubidIndexId, on
> pg_publication_namespace using btree(pnnspcid oid_ops, pnpubid
> oid_ops));
>
> Let's use nspid instead nspcid at all places as before we refer that
> way in the code. The way you have done is also okay but it is better
> to be consistent with existing code.

Modified

> 4. Need to think of comments in GetSchemaPublicationRelations.
> + /* get all the ordinary tables present in schema schemaid */
> ..
>
> Let's change the above comment to something like: "get all the
> relations present in the given schema"
>
> +
> + /*
> + * Get all relations that inherit from the partition table, directly or
> + * indirectly.
> + */
> + scan = table_beginscan_catalog(classRel, keycount, key);
>
>
> Let's change the above comment to something like: "It is quite
> possible that some of the partitions are in a different schema than
> the parent table, so we need to get such partitions separately."

Modified

> 5.
> + if (list_member_oid(schemaidlist, relSchemaId))
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("cannot add relation \"%s.%s\" to publication",
> +    get_namespace_name(relSchemaId),
> +    RelationGetRelationName(rel)),
> + errdetail("Table's schema is already included as part of ALL TABLES
> IN SCHEMA option."));
>
> I think the better errdetail would be: "Table's schema \"%s\" is
> already part of the publication."
>
> + if (list_member_oid(schemaidlist, relSchemaId))
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("cannot add schema \"%s\" to publication",
> +    get_namespace_name(get_rel_namespace(tableOid))),
> + errdetail("Table \"%s.%s\" is part of publication, adding same
> schema \"%s\" is not supported",
> +   get_namespace_name(get_rel_namespace(tableOid)),
> +   get_rel_name(tableOid),
> +   get_namespace_name(get_rel_namespace(tableOid))));
>
> I think this errdetail message can also be improved. One idea could
> be: "Table \"%s\" in schema \"%s\" is already part of the publication,
> adding the same schema is not supported.". Do you or anyone else have
> better ideas?

Modified

> 6. I think instead of two different functions
> CheckRelschemaInSchemaList and CheckSchemaInRels, let's have a single
> function RelSchemaIsMemberOfSchemaList and have a boolean variable to
> distinguish the two cases.

Modified
Thanks for the comments, the attached v26 patch has the changes for the same.

Regards,
VIgnesh