Re: Added schema level support for publication.

On Thu, Jul 8, 2021 at 9:16 AM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> 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.

Modified.

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

Modified.

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

These changes are required to modify the existing tests. I kept it in
0001 so that 0001 patch can be committed independently. I think we can
keep the change as it is, I did not make any changes for this.

> (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;

Modified.

Thanks for the comments, these comments are fixed as part of the v10
patch attached at [1].
[1] - https://www.postgresql.org/message-id/CALDaNm2%2BtR%2B8R-sD1CSyMbZcZbkintZE-avefjsp7LCkm6HMmw%40mail.gmail.com

Regards,
Vignesh