Re: Added schema level support for publication.
От | vignesh C |
---|---|
Тема | Re: Added schema level support for publication. |
Дата | |
Msg-id | CALDaNm2SxL=mMB=Ki4E17jtoQ9+ZnwYF3e_1OC9TToYs-DX9UA@mail.gmail.com обсуждение исходный текст |
Ответ на | RE: Added schema level support for publication. ("houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>) |
Список | 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
В списке pgsql-hackers по дате отправления: