From: | vignesh C <vignesh21(at)gmail(dot)com> |
---|---|
To: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
Cc: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Peter Smith <smithpb2250(at)gmail(dot)com>, "tanghy(dot)fnst(at)fujitsu(dot)com" <tanghy(dot)fnst(at)fujitsu(dot)com>, Greg Nancarrow <gregn4422(at)gmail(dot)com>, Ajin Cherian <itsajin(at)gmail(dot)com>, "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>, Rahila Syed <rahilasyed90(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> |
Subject: | Re: Added schema level support for publication. |
Date: | 2021-09-07 07:14:51 |
Message-ID: | CALDaNm07iRE9Odwhh9bGQJaC2pLN4gw0B65fRJod+4dsxVQ_Ew@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Sep 3, 2021 at 4:49 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Sep 2, 2021 at 5:12 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Thu, Sep 2, 2021 at 11:58 AM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> >
> > Below are few comments on v23. If you have already addressed anything
> > in v24, then ignore it.
> >
>
> More Review comments (on latest version v24):
> =======================================
> 1.
> + Oid psnspcid BKI_LOOKUP(pg_class); /* Oid of the schema */
> +} FormData_pg_publication_schema;
>
> Why in the above structure you have used pg_class instead of pg_namespace?
It should be pg_namespace, I have changed it.
> 2. GetSchemaPublicationRelations() uses two different ways to skip
> non-publishable relations in two while loops. Both are correct but I
> think it would be easier to follow if they use the same way and in
> this case I would prefer a check like if (is_publishable_class(relid,
> relForm)). The comments atop function could also be modified to :"Get
> the list of publishable relation oids for a specified schema.". I
> think it is better to write some comments on why you need to scan and
> loop twice.
Modified
> 3. The other point about GetSchemaPublicationRelations() is that I am
> afraid that it will collect duplicate relation oids in the final list
> when the partitioned table and child tables are in the same schema.
Modified it to prepare a list without duplicate relation ids.
> 4. In GetRelationPublicationActions(), the handling related to
> partitions seems to be missing for the schema. It won't be able to
> take care of child tables when they are in a different schema than the
> parent table.
Modified
> 5.
> If I modify the search path to remove public schema then I get the
> below error message:
> postgres=# Create publication mypub for all tables in schema current_schema;
> ERROR: no schema has been selected
>
> I think this message is not very clear. How about changing to
> something like "current_schema doesn't contain any valid schema"? This
> message is used in more than one place, so let's keep it the same at
> all the places if you agree to change it.
I would prefer to use the existing messages as we have used this in a
few other places similarly. Thoughts?
> 6. How about naming ConvertPubObjSpecListToOidList() as
> ObjectsInPublicationToOids()? I see somewhat similarly named functions
> in the code like objectsInSchemaToOids, objectNamesToOids.
Modified
> 7.
> + /*
> + * Schema lock is held until the publication is created to prevent
> + * concurrent schema deletion. No need to unlock the schemas, the
> + * locks will be released automatically at the end of create
> + * publication command.
> + */
>
> In this comment no need to say create publication command as that is
> implicit, we can just write ".... at the end of command".
Modified
> 8. Can you please extract changes like tab-completion, dump/restore in
> separate patches? This can help to review the core (backend) part of
> the patch in more detail.
Modified
This is handled as part of v25 patch attached at [1]
[1] - https://www.postgresql.org/message-id/CALDaNm2SytXy2TDnzzYkXWKgNp74ssPBXrkMXEyac1qVYSRkbw%40mail.gmail.com
Regards,
Vignesh
From | Date | Subject | |
---|---|---|---|
Next Message | vignesh C | 2021-09-07 07:16:24 | Re: Added schema level support for publication. |
Previous Message | Pengchengliu | 2021-09-07 07:11:32 | RE: suboverflowed subtransactions concurrency performance optimize |