On Tue, Sep 21, 2021 at 9:03 AM Greg Nancarrow <gregn4...@gmail.com> wrote:
>
> On Fri, Sep 17, 2021 at 10:09 PM vignesh C <vignes...@gmail.com> wrote:
> >
> > Attached v29 patch has the fixes for the same.
> >
>
> Some minor comments on the v29-0002 patch:
>
> (1)
> In get_object_address_publication_schema(), the error message:
>
> + errmsg("publication tables of schema \"%s\" in publication \"%s\"
> does not exist",
>
> isn't grammatically correct. It should probably be:
>
> + errmsg("publication tables of schema \"%s\" in publication \"%s\" do
> not exist",Modified > (2) > getPublicationSchemaInfo() function header. > "string" should be "strings" > > BEFORE: > + * nspname. Both pubname and nspname are palloc'd string which will be freed > by > AFTER: > + * nspname. Both pubname and nspname are palloc'd strings which will > be freed by Modified > (3) > getPublicationSchemaInfo() > > In the case of "if (!(*nspname))", the following line should probably > be added before returning false: > > *pubname = NULL; I left it as it is, as we don't access it in case of return false. > (4) > GetAllSchemasPublicationRelations() function header > > Shouldn't: > > + * Gets the list of all relations published by FOR ALL TABLES IN SCHEMA > + * publication(s). > > actually say: > > + * Gets the list of all relations published by a FOR ALL TABLES IN SCHEMA > + * publication. > > since it is for a specified publication? Modified > (5) > I'm wondering, in CheckObjSchemaNotAlreadyInPublication(), instead of > checking "checkobjtype" each loop iteration, wouldn't it be better to > just use the same for-loop in each IF block? Modified I have made the changes for the above at the v30 patch attached at [1]. [1] - https://www.postgresql.org/message-id/CALDaNm3aFtXpkD4m28-ENG9F4faBEVdGNUrEhgKV0pHr2S_C2g%40mail.gmail.com Regards, Vignesh
