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

On Tue, Nov 26, 2024 at 9:47 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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.
>

Keeping them isolated in a function is better as it keeps the caller
function logicalrep_rel_open() easier to follow.

-- 
With Regards,
Amit Kapila.