RE: Added schema level support for publication.

On Wednesday, June 30, 2021 7:43 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> Thanks for reporting this issue, the attached v9 patch fixes this issue. This also fixes the other issue you reported at [1].

Hi,

I had a look at the patch, please consider following comments.

(1)
- if (pub->alltables)
+ if (pub->pubtype == PUBTYPE_ALLTABLES)
{
publish = true;
if (pub->pubviaroot && am_partition)
publish_as_relid = llast_oid(get_partition_ancestors(relid));
}

+ if (pub->pubtype == PUBTYPE_SCHEMA)
+ {
+ Oid schemaId = get_rel_namespace(relid);
+ List *pubschemas = GetPublicationSchemas(pub->oid);
+
+ if (list_member_oid(pubschemas, schemaId))
+ {

It might be better use "else if" for the second check here.
Like: else if (pub->pubtype == PUBTYPE_SCHEMA)

Besides, we already have the {schemaoid, pubid} set here, it might be
better to scan the cache PUBLICATIONSCHEMAMAP instead of invoking
GetPublicationSchemas() which will scan the whole table.

(2)
+ /* Identify which schemas should be dropped. */
+ foreach(oldlc, oldschemaids)
+ {
+ Oid oldschemaid = lfirst_oid(oldlc);
+ ListCell *newlc;
+ bool found = false;
+
+ foreach(newlc, schemaoidlist)
+ {
+ Oid newschemaid = lfirst_oid(newlc);
+
+ if (newschemaid == oldschemaid)
+ {
+ found = true;
+ break;
+ }
+ }

It seems we can use " if (list_member_oid(schemaoidlist, oldschemaid)) "
to replace the second foreach loop.

(3)
there are some testcases change in 0001 patch, it might be better move them
to 0002 patch.

(4)
+ case OBJECT_PUBLICATION_SCHEMA:
+ objnode = (Node *) list_make2(linitial(name), linitial(args));
+ break;
case OBJECT_USER_MAPPING:
objnode = (Node *) list_make2(linitial(name), linitial(args));
break;

Does it looks better to merge these two switch cases ?
Like:
case OBJECT_PUBLICATION_SCHEMA:
case OBJECT_USER_MAPPING:
objnode = (Node *) list_make2(linitial(name), linitial(args));
break;

best regards,
houzj