Re: Added schema level support for publication.

On Tue, Sep 7, 2021 at 12:45 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Fri, Sep 3, 2021 at 4:49 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)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?

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?

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?

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.

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

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?

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.

--
With Regards,
Amit Kapila.