On Friday, September 16, 2022 1:42 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Sep 15, 2022 at 6:27 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > Attach the new version patch which added suggested restriction for
> > column list and merged Vignesh's patch.
> >
>
> Few comments:
> ============
> 1.
> static void
> -CheckPubRelationColumnList(List *tables, const char *queryString,
> +CheckPubRelationColumnList(List *tables, bool publish_schema,
> + const char *queryString,
> bool pubviaroot)
>
> It is better to keep bool parameters together at the end.
>
> 2.
> /*
> + * Disallow using column list if any schema is in the publication.
> + */
> + if (publish_schema)
> + ereport(ERROR,
> + errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("cannot use publication column list for relation \"%s.%s\"",
> + get_namespace_name(RelationGetNamespace(pri->relation)),
> + RelationGetRelationName(pri->relation)),
> + errdetail("Column list cannot be specified if any schema is part of
> the publication or specified in the list."));
>
> I think it would be better to explain why we disallow this case.
>
> 3.
> + if (!heap_attisnull(coltuple, Anum_pg_publication_rel_prattrs, NULL))
> + ereport(ERROR, errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> + errmsg("cannot add schema to the publication"), errdetail("Schema
> + cannot be added if any table that specifies column
> list is already part of the publication"));
>
> A full stop is missing at the end in the errdetail message.
>
> 4. I have modified a few comments in the attached. Please check and if you like
> the changes then please include those in the next version.
Thanks for the comments.
Attach the new version patch which addressed above comments and ran pgident.
I also improved some codes and documents based on some comments
given by Vignesh and Peter offlist.
Best regards,
Hou zj
