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

On Sat, Nov 16, 2024 at 5:43 PM Shlok Kyal <shlok(dot)kyal(dot)oss(at)gmail(dot)com> wrote:
>
> 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.
>

I have fixed the given comments. The v3 version patch attached at [1]
has the changes for the same.
[1] - https://www.postgresql.org/message-id/CAHv8RjJ4Qpqia9HccAZ0UWXmgYDebF3su2pw1jFYRYzSkk_QQQ%40mail.gmail.com

Thanks and Regards,
Shubham Khanna.