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

On Tue, Nov 26, 2024 at 1:42 PM vignesh C <vignesh21@gmail.com> wrote:
>.
>
> Few comments:
> 1) Now that attribute string generation is moved to get_attrs_str and
> there are only a couple of error statements in this function, how
> about removing the function:
> +/*
> + * If !bms_is_empty(missingatts), report the error message as 'Missing
> + * replicated columns.' Otherwise, report the error message as
> 'Cannot replicate
> + * to generated columns.'
> + */
> +static void
> +logicalrep_report_missing_and_gen_attrs(LogicalRepRelation *remoterel,
> +
>          Bitmapset *missingatts,
> +
>          Bitmapset *genatts)
> +{
> +       if (!bms_is_empty(missingatts))
>                 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",
> -                                                          missingattcnt,
> -                                                          remoterel->nspname,
> -                                                          remoterel->relname,
> -
> missingattsbuf.data)));
> -       }
> +
> 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",
> +
> bms_num_members(missingatts),
> +                                                         remoterel->nspname,
> +                                                         remoterel->relname,
> +
> get_attrs_str(remoterel, missingatts)));
> +
> +       if (!bms_is_empty(genatts))
> +               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",
> +
> bms_num_members(genatts),
> +                                                         remoterel->nspname,
> +                                                         remoterel->relname,
> +
> get_attrs_str(remoterel, genatts)));
>  }
>

+1. This idea to just inline those errors instead of calling the
function sounds OK to me too.
Please consider also moving my suggested function comment if you
refactor this way.

======
Kind Regards,
Peter Smith.
Fujitsu Australia