Re: Improve the error message for logical replication of regular column to generated column.

On Fri, Nov 15, 2024 at 7:07 PM vignesh C <vignesh21@gmail.com> wrote:
>
> On Fri, 15 Nov 2024 at 15:57, Shubham Khanna
> <khannashubham1197@gmail.com> wrote:
> >
> > I have fixed the given comments. The attached Patch contains the
> > required changes.
>
> Few comments:
> 1)
> a)You can mention that "If ismissing is true, report the error message
> as 'Missing replicated columns.' Otherwise, report the error message
> as 'Cannot replicate to generated column."
>  /*
> - * Report error with names of the missing local relation column(s), if any.
> + * Report error with names of the missing and generated local
> relation column(s), if any.
>   */
>
> b) You can keep the line within 80 chars in this case.
>
> 2) Spurious blank line:
> +                       ereport(ERROR,
> +
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> +                                        errmsg_plural("logical
> replication target relation \"%s.%s\" is missing replicated column:
> %s",
> +
> "logical replication target relation \"%s.%s\" is missing replicated
> columns: %s",
> +                                                                  attcnt,
> +
> remoterel->nspname,
> +
> remoterel->relname,
> +
> attsbuf.data)));
> +
> +               else
> +                       ereport(ERROR,
> +
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> +                                        errmsg_plural("cannot
> replicate to target relation \"%s.%s\" generated column: %s",
> +
> "cannot replicate to target relation \"%s.%s\" generated columns: %s",
> +                                                                  attcnt,
> +
> remoterel->nspname,
> +
> remoterel->relname,
> +
> attsbuf.data)));
>
> 3) This comment is not correct as the definition of
> generated(publisher) to generated(subscriber) can be same:
> +                               /*
> +                                * Add to generatedattrs if names
> match but definitions
> +                                * differ.
> +                                */
> +                               if (attr->attgenerated)
> +                                       generatedattrs =
> bms_add_member(generatedattrs, i);
>
> 4)
> a) You can use "regular" instead of "normal":
> +# A "normal -> generated" and "generated -> generated" replication fails,
> +# reporting an error that the generated column on the subscriber side
> +# cannot be replicated.
> +#
> +# Test Case: normal -> generated and generated -> generated
> +# Publisher table has regular column 'c2' and generated column 'c3'.
> +# Subscriber table has generated columns 'c2' and 'c3'.
>
> b) similarly here too:
> +# --------------------------------------------------
> +# A "normal -> missing" replication fails, reporting an error
> +# that the subscriber side is missing replicated columns.
> +#
> +# Testcase: normal -> missing
> +# Publisher table has normal columns 'c2' and 'c3'.
> +# Subscriber table is missing columns 'c2' and 'c3'.
> +# --------------------------------------------------
>

I have fixed the given comments. The attached Patch contains the
required changes.

Thanks and regards,
Shubham Khanna.