On Thu, Nov 14, 2024 at 2:09 PM Peter Smith <smithpb2...@gmail.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.
From 2886da4ad94bd2a4c0b99deaa21d57127b210a3e Mon Sep 17 00:00:00 2001
From: Shubham Khanna <shubham.khanna@fujitsu.com>
Date: Thu, 7 Nov 2024 15:54:19 +0530
Subject: [PATCH v2] Error-message-improvement
Currently, if logical replication attempts to target a subscriber-side table
column that is either missing or generated, it produces the following
identical error message:
ERROR: logical replication target relation \"%s.%s\" is missing
replicated columns: %s
While the error itself is valid, the message wording can be misleading for
generated columns. This patch introduces a distinct error message specifically
for the generated column scenario.
---
src/backend/replication/logical/relation.c | 71 +++++++++++++-------
src/test/subscription/t/011_generated.pl | 76 ++++++++++++++++++++++
2 files changed, 124 insertions(+), 23 deletions(-)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index f5a0ef2bd9..276bf210bb 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -220,40 +220,52 @@ logicalrep_rel_att_by_name(LogicalRepRelation *remoterel, const char *attname)
}
/*
- * Report error with names of the missing local relation column(s), if any.
+ * Report error with names of the missing and generated local relation column(s), if any.
*/
static void
-logicalrep_report_missing_attrs(LogicalRepRelation *remoterel,
- Bitmapset *missingatts)
+logicalrep_report_missing_and_gen_attrs(LogicalRepRelation *remoterel,
+ Bitmapset *atts,
+ bool ismissing)
{
- if (!bms_is_empty(missingatts))
+ if (!bms_is_empty(atts))
{
- StringInfoData missingattsbuf;
- int missingattcnt = 0;
+ StringInfoData attsbuf;
+ int attcnt = 0;
int i;
- initStringInfo(&missingattsbuf);
+ initStringInfo(&attsbuf);
i = -1;
- while ((i = bms_next_member(missingatts, i)) >= 0)
+ while ((i = bms_next_member(atts, i)) >= 0)
{
- missingattcnt++;
- if (missingattcnt == 1)
- appendStringInfo(&missingattsbuf, _("\"%s\""),
+ attcnt++;
+ if (attcnt == 1)
+ appendStringInfo(&attsbuf, _("\"%s\""),
remoterel->attnames[i]);
else
- appendStringInfo(&missingattsbuf, _(", \"%s\""),
+ appendStringInfo(&attsbuf, _(", \"%s\""),
remoterel->attnames[i]);
}
- 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)));
+ if (ismissing)
+ 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",
+ attcnt,
+ remoterel->nspname,
+ remoterel->relname,
+ attsbuf.data)));
+
+ else
+ 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",
+ attcnt,
+ remoterel->nspname,
+ remoterel->relname,
+ attsbuf.data)));
}
}
@@ -379,7 +391,9 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
TupleDesc desc;
MemoryContext oldctx;
int i;
- Bitmapset *missingatts;
+ Bitmapset *missingatts; /* Bitmapset for missing attributes. */
+ Bitmapset *generatedattrs = NULL; /* Bitmapset for generated
+ * attributes. */
/* Release the no-longer-useful attrmap, if any. */
if (entry->attrmap)
@@ -421,7 +435,7 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
int attnum;
Form_pg_attribute attr = TupleDescAttr(desc, i);
- if (attr->attisdropped || attr->attgenerated)
+ if (attr->attisdropped)
{
entry->attrmap->attnums[i] = -1;
continue;
@@ -432,12 +446,23 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
entry->attrmap->attnums[i] = attnum;
if (attnum >= 0)
+ {
+ /*
+ * Add to generatedattrs if names match but definitions
+ * differ.
+ */
+ if (attr->attgenerated)
+ generatedattrs = bms_add_member(generatedattrs, i);
+
missingatts = bms_del_member(missingatts, attnum);
+ }
}
- logicalrep_report_missing_attrs(remoterel, missingatts);
+ logicalrep_report_missing_and_gen_attrs(remoterel, generatedattrs, false);
+ logicalrep_report_missing_and_gen_attrs(remoterel, missingatts, true);
/* be tidy */
+ bms_free(generatedattrs);
bms_free(missingatts);
/*
diff --git a/src/test/subscription/t/011_generated.pl b/src/test/subscription/t/011_generated.pl
index 211b54c316..2b1c7924ae 100644
--- a/src/test/subscription/t/011_generated.pl
+++ b/src/test/subscription/t/011_generated.pl
@@ -326,4 +326,80 @@ is( $result, qq(|2|
$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub1");
$node_publisher->safe_psql('postgres', "DROP PUBLICATION pub1");
+# =============================================================================
+# The following test cases exercise logical replication for the combinations
+# where there is a generated column on one or both sides of pub/sub:
+# - normal -> generated and generated -> generated
+# - normal -> missing
+# =============================================================================
+
+# --------------------------------------------------
+# A "normal -> generated" and "generated -> generated" replication fails,
+# reporting an error that the generated column on the subscriber side
+# cannot be replicated.
+#
+# Test Case: normal -> generated and generated -> generated
+# Publisher table has regular column 'c2' and generated column 'c3'.
+# Subscriber table has generated columns 'c2' and 'c3'.
+# --------------------------------------------------
+
+# Create table and publication. Insert data into the table.
+$node_publisher->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE t1(c1 int, c2 int, c3 int GENERATED ALWAYS AS (c1 * 2) STORED);
+ CREATE PUBLICATION pub1 for table t1(c1, c2, c3);
+ INSERT INTO t1 VALUES (1);
+));
+
+# Create table and subscription.
+$node_subscriber->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE t1(c1 int, c2 int GENERATED ALWAYS AS (c1 + 2) STORED, c3 int GENERATED ALWAYS AS (c1 + 2) STORED);
+ CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr' PUBLICATION pub1;
+));
+
+# Verify that an error occurs.
+my $offset = -s $node_subscriber->logfile;
+$node_subscriber->wait_for_log(
+ qr/ERROR: ( [A-Z0-9]:)? cannot replicate to target relation "public.t1" generated columns: "c2", "c3"/,
+ $offset);
+
+# cleanup
+$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub1");
+$node_publisher->safe_psql('postgres', "DROP PUBLICATION pub1");
+
+# --------------------------------------------------
+# A "normal -> missing" replication fails, reporting an error
+# that the subscriber side is missing replicated columns.
+#
+# Testcase: normal -> missing
+# Publisher table has normal columns 'c2' and 'c3'.
+# Subscriber table is missing columns 'c2' and 'c3'.
+# --------------------------------------------------
+
+# Create table and publication. Insert data into the table.
+$node_publisher->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE t2(c1 int, c2 int, c3 int);
+ CREATE PUBLICATION pub1 for table t2(c1, c2, c3);
+ INSERT INTO t2 VALUES (1);
+));
+
+# Create table and subscription.
+$node_subscriber->safe_psql(
+ 'postgres', qq(
+ CREATE TABLE t2(c1 int);
+ CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr' PUBLICATION pub1;
+));
+
+# Verify that an error occurs.
+$offset = -s $node_subscriber->logfile;
+$node_subscriber->wait_for_log(
+ qr/ERROR: ( [A-Z0-9]:)? logical replication target relation "public.t2" is missing replicated columns: "c2", "c3"/,
+ $offset);
+
+# cleanup
+$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub1");
+$node_publisher->safe_psql('postgres', "DROP PUBLICATION pub1");
+
done_testing();
--
2.41.0.windows.3
