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

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.

Thanks and regards,
Shubham Khanna.