Re: Improve the error message for logical replication of regular column to generated column. - Mailing list pgsql-hackers

On Mon, 25 Nov 2024 at 16:06, Shubham Khanna
<khannashubham1197@gmail.com> wrote:
>
> On Mon, Nov 25, 2024 at 8:50 AM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > Hi Shubham,
> >
> > here are my review comments for patch v4-0001.
> >
> > ======
> > src/backend/replication/logical/relation.c
> >
> > logicalrep_report_missing_and_gen_attrs:
> >
> > 1.
> >  static void
> > -logicalrep_report_missing_attrs(LogicalRepRelation *remoterel,
> > - Bitmapset *missingatts)
> > +logicalrep_report_missing_and_gen_attrs(LogicalRepRelation *remoterel,
> > + Bitmapset *atts,
> > + bool ismissing)
> >
> >
> > Maybe the function should be called
> > 'logicalrep_report_missing_or_gen_attrs' (not 'and')
> >
> > ~
> >
> > 2.
> > - if (!bms_is_empty(missingatts))
> > + if (!bms_is_empty(atts))
> >
> > I felt this should be an Assert because the code becomes easier to
> > read if you check this before making the call in the first place. See
> > my NITPICKS patch.
> >
> > ~
> >
> > 3.
> > + if (attcnt == 1)
> > + appendStringInfo(&attsbuf, _("\"%s\""),
> >   remoterel->attnames[i]);
> >   else
> > - appendStringInfo(&missingattsbuf, _(", \"%s\""),
> > + appendStringInfo(&attsbuf, _(", \"%s\""),
> >   remoterel->attnames[i]);
> >   }
> >
> > This code can be simplified (e.g. remove the 'else' etc if you just
> > check > 1 instead). See my NITPICKS patch.
> >
> > SUGGESTION
> > if (attcnt > 1)
> >   appendStringInfo(&attsbuf, _(", "));
> >
> > appendStringInfo(&attsbuf, _("\"%s\""), remoterel->attnames[i]);
> >
> > ~~~
> >
> > logicalrep_rel_open:
> >
> > 4.
> > + /*
> > + * Include it in generatedattrs if publishing to a generated
> > + * column.
> > + */
> > + if (attr->attgenerated)
> > + generatedattrs = bms_add_member(generatedattrs, attnum);
> >
> > That comment can be simpler if indeed it is needed at all.
> >
> > SUGGESTION:
> > /* Remember which subscriber columns are generated. */
> >
> > ~
> >
> > 5.
> > As I reported above (#2), I think it is better to check for empty BMS
> > in the caller because then the code is easier to read. Also, you need
> > to comment on which of these 2 errors will take precedence because if
> > there are simultaneous problems you are still only reporting one kind
> > of error at a time.
> >
> > SUGGESTION:
> > /*
> >  * Report any missing or generated columns. Note, if there are both
> >  * kinds then the 'missing' error takes precedence.
> >  */
> > if (!bms_is_empty(missingatts))
> >   logicalrep_report_missing_and_gen_attrs(remoterel, missingatts,
> >                                           true);
> > if (!bms_is_empty(generatedattrs))
> >   logicalrep_report_missing_and_gen_attrs(remoterel, generatedattrs,
> >                                           false);
> >
> > ======
> > src/test/subscription/t/011_generated.pl
> >
> > 6.
> > +# =============================================================================
> > +# The following test cases exercise logical replication for the combinations
> > +# where there is a generated column on one or both sides of pub/sub:
> > +# - regular -> generated and generated -> generated
> > +# - regular -> missing
> > +# =============================================================================
> >
> >
> > 6a.
> > This comment is not quite right. You can't say "where there is a
> > generated column on one or both sides of pub/sub" because that is not
> > true for the "regular -> missing" case. See NITPICKS for a suggested
> > comment.
> >
> > ~
> >
> > 6b.
> > IMO you should also be testing the "generated -> missing" combination.
> > You don't need more tests -- just more columns.
> >
> > ~
> >
> > 6c
> > You also need to include a test where there are BOTH generated and
> > missing to show the 'missing' error takes precedence. Again, you don't
> > need more separate test cases to achieve this -- just need more
> > columns in the tables.
> >
> > ~~~
> >
> > 7.
> > +# --------------------------------------------------
> > +# A "regular -> generated" and "generated -> generated" replication fails,
> > +# reporting an error that the generated column on the subscriber side
> > +# cannot be replicated.
> >
> > /and/or/
> >
> > ~~~
> >
> > 8.
> > +# --------------------------------------------------
> > +# A "regular -> missing" replication fails, reporting an error
> > +# that the subscriber side is missing replicated columns.
> > +#
> > +# Testcase: regular -> missing
> > +# Publisher table has regular columns 'c2' and 'c3'.
> > +# Subscriber table is missing columns 'c2' and 'c3'.
> > +# --------------------------------------------------
> >
> > I've also added the "generated -> missing" combination and addressed
> > the review comment about intercluding a test where there are BOTH
> > missing and generated columns, so you can see which error takes
> > precedence. Please see the NITPICKS diff.
> >
>
> I have fixed the given comments. The attached Patch contains the
> required changes.

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)));
 }

2) This comment seems to be wrong, "Cannot replicate to generated
columns" error will be thrown only if genatts bitmap is valid.
+/*
+ * 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)

Regards,
Vignesh