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

On Fri, 15 Nov 2024 at 15:57, Shubham Khanna
<khannashubham1197(at)gmail(dot)com> wrote:
>
> 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 for providing the patch.
I have few comments:

1. Getting segmentation fault for following test case:

Publisher:
CREATE TABLE t1 (a INT, b INT);
create publication pub1 for table t1(b)

Subscriber:
CREATE TABLE t1 (a INT, b int GENERATED ALWAYS AS (a + 1) STORED NOT NULL)
create subscription test1 connection 'dbname=postgres host=localhost
port=5432' publication pub1

Subscriber logs:
2024-11-16 17:23:16.919 IST [3842385] LOG: logical replication apply
worker for subscription "test1" has started
2024-11-16 17:23:16.931 IST [3842389] LOG: logical replication table
synchronization worker for subscription "test1", table "t1" has
started
2024-11-16 17:29:47.855 IST [3842359] LOG: background worker "logical
replication tablesync worker" (PID 3842389) was terminated by signal
11: Segmentation fault
2024-11-16 17:29:47.856 IST [3842359] LOG: terminating any other
active server processes

2.
+ initStringInfo(&attsbuf);

'attsbuf' not free'd. I think we should pfree it.

Thanks and Regards,
Shlok Kyal