Re: Added schema level support for publication. - Mailing list pgsql-hackers

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