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(at)gmail(dot)com> wrote:
>
> On Tue, Nov 26, 2024 at 1:42 PM vignesh C <vignesh21(at)gmail(dot)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.