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

On Thu, Nov 14, 2024 at 2:09 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Hi Shubham,
>
> +1 for the patch idea.
>
> Improving this error message for subscriber-side generated columns
> will help to remove some confusion.
>
> Here are my review comments for patch v1-0001.
>
> ======
> Commit message.
>
> 1.
> The error message was misleading, as it failed to clarify that the replication
> of regular column on the publisher to the corresponding generated column on
> the subscriber is not supported.
>
> This patch improves the error handling and reporting mechanism to make it clear
> that the replication of regular column on the subscriber is not supported,
> resolving the misleading "missing column" error.
>
> ~
>
> It makes no difference whether the publishing table column is regular
> or generated, so you should not be implying that this has anything to
> do with the replication of just regular columns. AFAIK, the *only*
> thing that matters is that you cannot replicate into a subscriber-side
> generated column or a subscriber-side missing column.
>
> The current master reports replication into either a generated or a
> missing column as the same "missing replication column" error. IIUC,
> the errors were "correct", although clearly, for the generated column
> case the error was quite misleading.
>
> So, this patch is really *only* to improve the error wording when
> attempting to replicate into a subscriber-side generated column.
> That's what the commit message should be conveying.
>
> ======
> src/backend/replication/logical/relation.c
>
> logicalrep_rel_open:
>
> 2.
> Bitmapset *missingatts;
> + StringInfoData gencolsattsbuf;
> + int generatedatts = 0;
> +
> + initStringInfo(&gencolsattsbuf);
>
> The existing "missing columns" error is implemented by building a BMS
> and then passing it to the function 'logicalrep_report_missing_attrs'
> to report the error.
>
> IMO the generated column error is essentially the same, so should be
> implemented with almost identical logic -- i.e. you should build a
> 'generatedattrs' BMS of generated cols with matching names and (if
> that BMS is not empty) then pass that to a new function
> 'logicalrep_report_generated_attrs' (a sibling function to the
> existing one).
>
> ~~~
>
> 3.
> + /*
> + * Check if the subscription table generated column has
> + * same name as a non-generated column in the
> + * corresponding publication table.
> + */
>
> This (misplaced) comment talks about checking if the names are the
> same. But I don't see any name-checking logic here (???). Where is it?
>
> ~~~
>
> 4.
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg_plural("replicating to a target relation's generated column
> \"%s\" for \"%s.%s\" is not supported",
> + "replicating to a target relation's generated column \"%s\" for
> \"%s.%s\" is not supported",
> + generatedatts, gencolsattsbuf.data, remoterel->nspname,
> remoterel->relname)));
>
> There are no plural differences here. This smells like a cut/paste
> mistake from logicalrep_report_generated_attrs'.
>
> IMO this error should close match the existing "missing replication
> columns" error, and use the errmsg_plural correctly. In other words,
> it should look something more like this:
>
> 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",
> ...
>
> ======
> src/test/subscription/t/011_generated.pl
>
> 5.
> +# =============================================================================
> +# Exercise logical replication of a regular column to a subscriber side
> +# generated column.
> +#
> +# A "normal --> generated" replication fails, reporting an error that the
> +# replication of a generated column on subscriber side is not supported.
> +# =============================================================================
> +
> +# --------------------------------------------------
> +# Test Case: normal --> generated
> +# Publisher table has regular columns 'c2' and 'c3'.
> +# Subscriber table has generated columns 'c2' and 'c3'.
> +# --------------------------------------------------
> +
>
> As I have said in previous internal reviews, this test (and the
> comments) can be much more sophisticated. AFAICT by cleverly arranging
> different publication table column types and different subscriber-side
> table column ordering I think you should be able to test multiple
> things at once.
>
> Such as
> - regular -> generated is detected
> - generated -> generated is detected
> - that the error only reports the generated column problems where the
> column names are matching, not others
>
> ~~~~
>
> 6.
> Also, as previously mentioned in internal reviews, this patch should
> include a 2nd test case to do pretty much the same testing but
> expecting to get a "missing replication column".
>
> The reasons to include this 2nd test are:
> a) The missing column was never tested properly before.
> b) This current patch has overlapping logic so you need to be assured
> that adding this new error doesn't break the existing one.
> c) Only one of these errors wins. Adding both tests will define the
> expected order if both error scenarios exist at the same time.
>

I have fixed the given comments. The attached Patch contains the
required changes.

Thanks and regards,
Shubham Khanna.