RE: Added schema level support for publication.

On Wednesday, June 30, 2021 7:43 PM vignesh C <vignesh21@gmail.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