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