Re: Added schema level support for publication.

On Fri, Aug 27, 2021 at 11:43 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> On Wed, Aug 25, 2021 at 3:07 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
>
> I have implemented this in the 0003 patch, I have kept it separate to
> reduce the testing effort and also it will be easier if someone
> disagrees with the syntax. I will merge it to the main patch later
> based on the feedback. Attached v22 patch has the changes for the
> same.
>

Few comments on v22-0001-Added-schema-level-support-for-publication:
========================================================
1. Why in publication_add_schema(), you are registering invalidation
for all schema relations? It seems this is to allow rebuilding the
publication info for decoding sessions. But that is not required as
you are registering rel_sync_cache_publication_cb for
publication_schema relation. In rel_sync_cache_publication_cb, we are
marking replicate_valid as false for each entry which will allow
publication info to be rebuilt in get_rel_sync_entry.

I see that it is done for a single relation in the current code in
function publication_add_relation but I guess that is also not
required. You can once test this. If you still think it is required,
can you please explain me why and then we can accordingly add some
comments in the patch.

Peter E., Sawada-San, can you please let me know if I am missing
something in this regard? In the code, I see a comment "/* Invalidate
relcache so that publication info is rebuilt. */" in function
publication_add_relation() but I don't understand why it is required
as per my explanation above?

2.
+ * Publication object type
+ */
+typedef enum PublicationObjSpecType
+{
+ PUBLICATIONOBJ_TABLE, /* Table type */
+ PUBLICATIONOBJ_SCHEMA, /* Schema type */
+ PUBLICATIONOBJ_SEQUENCE, /* Sequence type */

Why add anything related to the sequence in this patch?

3.
+get_object_address_publication_schema(List *object, bool missing_ok)
+{
+ ObjectAddress address;
+ char *pubname;
+ Publication *pub;
+ char *schemaname;
+ Oid schemaoid;
+
+ ObjectAddressSet(address, PublicationSchemaRelationId, InvalidOid);
+
+ /* Fetch schema name and publication name from input list */
+ schemaname = strVal(linitial(object));
+ pubname = strVal(lsecond(object));
+
+ schemaoid = get_namespace_oid(schemaname, false);
+
+ /* Now look up the pg_publication tuple */
+ pub = GetPublicationByName(pubname, missing_ok);
+ if (!pub)
+ return address;

Why the schema name handling is different from publication name? Why
can't we pass missing_ok for schema api and handle it similar
publication api?

4. In getPublicationSchemaInfo(), why the missing_ok flag is not used
in get_publication_name() whereas it is used for all other syscache
searches in that function?

5. Don't we need to expose a view for publication schemas similar to
pg_publication_tables?

6.
publication_add_schema()
{
..
+ /* Can't be system namespace */
+ if (IsCatalogNamespace(schemaoid) || IsToastNamespace(schemaoid))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" is a system schema",
+ get_namespace_name(schemaoid)),
+ errdetail("System schemas cannot be added to publications.")));
+
+ /* Can't be temporary namespace */
+ if (isAnyTempNamespace(schemaoid))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("\"%s\" is a temporary schema",
+ get_namespace_name(schemaoid)),
+ errdetail("Temporary schemas cannot be added to publications.")));
..
}

Can we change the first detail message as: "This operation is not
supported for system schemas." and the second detail message as:
"Temporary schemas cannot be replicated."? This is to make these
messages similar to corresponding messages for relations in function
check_publication_add_relation(). Can we move these checks to a
separate function?

--
With Regards,
Amit Kapila.