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.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/replication/logical/relation.c
b/src/backend/replication/logical/relation.c
index 2c7be7d..21f4f12 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -224,49 +224,46 @@ logicalrep_rel_att_by_name(LogicalRepRelation *remoterel,
const char *attname)
* generated columns.'
*/
static void
-logicalrep_report_missing_and_gen_attrs(LogicalRepRelation *remoterel,
+logicalrep_report_missing_or_gen_attrs(LogicalRepRelation *remoterel,
Bitmapset *atts,
bool ismissing)
{
- if (!bms_is_empty(atts))
- {
- StringInfoData attsbuf;
- int attcnt = 0;
- int i;
+ StringInfoData attsbuf;
+ int attcnt = 0;
+ int i;
- initStringInfo(&attsbuf);
+ Assert(!bms_is_empty(atts));
- i = -1;
- while ((i = bms_next_member(atts, i)) >= 0)
- {
- attcnt++;
- if (attcnt == 1)
- appendStringInfo(&attsbuf, _("\"%s\""),
-
remoterel->attnames[i]);
- else
- appendStringInfo(&attsbuf, _(", \"%s\""),
-
remoterel->attnames[i]);
- }
+ initStringInfo(&attsbuf);
- 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)));
+ i = -1;
+ while ((i = bms_next_member(atts, i)) >= 0)
+ {
+ attcnt++;
+ if (attcnt > 1)
+ appendStringInfo(&attsbuf, _(", "));
+
+ appendStringInfo(&attsbuf, _("\"%s\""), remoterel->attnames[i]);
}
+
+ 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)));
}
/*
@@ -447,10 +444,7 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE
lockmode)
entry->attrmap->attnums[i] = attnum;
if (attnum >= 0)
{
- /*
- * Include it in generatedattrs if publishing
to a generated
- * column.
- */
+ /* Remember which subscriber columns are
generated. */
if (attr->attgenerated)
generatedattrs =
bms_add_member(generatedattrs, attnum);
@@ -458,12 +452,16 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE
lockmode)
}
}
- /* Report replicating to generated columns. */
- logicalrep_report_missing_and_gen_attrs(remoterel,
generatedattrs,
-
false);
- /* Report missing columns. */
- logicalrep_report_missing_and_gen_attrs(remoterel, missingatts,
-
true);
+ /*
+ * 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_or_gen_attrs(remoterel,
missingatts,
+
true);
+ if (!bms_is_empty(generatedattrs))
+ logicalrep_report_missing_or_gen_attrs(remoterel,
generatedattrs,
+
false);
/* be tidy */
bms_free(generatedattrs);
diff --git a/src/test/subscription/t/011_generated.pl
b/src/test/subscription/t/011_generated.pl
index 2a2df36..5a360de 100644
--- a/src/test/subscription/t/011_generated.pl
+++ b/src/test/subscription/t/011_generated.pl
@@ -327,14 +327,17 @@ $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:
-# - regular -> generated and generated -> generated
+# The following tests for expected errors when attempting to replicate to a
+# missing or a generated subscriber column. Test the folowing combinations
+# - regular -> generated
+# - generated -> generated
# - regular -> missing
+# - generated -> missing
+# - test if both errors exist, then the missing error is reported first
# =============================================================================
# --------------------------------------------------
-# A "regular -> generated" and "generated -> generated" replication fails,
+# A "regular -> generated" or "generated -> generated" replication fails,
# reporting an error that the generated column on the subscriber side
# cannot be replicated.
#
@@ -369,33 +372,37 @@ $node_subscriber->safe_psql('postgres', "DROP
SUBSCRIPTION sub1");
$node_publisher->safe_psql('postgres', "DROP PUBLICATION pub1");
# --------------------------------------------------
-# A "regular -> missing" replication fails, reporting an error
-# that the subscriber side is missing replicated columns.
+# A "regular -> missing" or "generated -> 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'.
+# Testcase: regular -> missing and generated -> missing
+# Publisher table has regular column 'c4' and generated column 'c5'.
+# Subscriber table is missing columns 'c4' and 'c5'.
+#
+# Notice how the first 3 columns of t2 are identical to the previous test,
+# so this table also has generated column errors, but they are not reported
+# because the 'missing' column error takes precedence.
# --------------------------------------------------
# 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);
+ CREATE TABLE t2(c1 int, c2 int, c3 int GENERATED ALWAYS AS (c1 * 2)
STORED, c4 int, c5 int GENERATED ALWAYS AS (c1 * 2) STORED);
+ CREATE PUBLICATION pub1 for table t2(c1, c2, c3, c4, c5);
INSERT INTO t2 VALUES (1);
));
# Create table and subscription.
$node_subscriber->safe_psql(
'postgres', qq(
- CREATE TABLE t2(c1 int);
+ CREATE TABLE t2(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.
$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"/,
+ qr/ERROR: ( [A-Z0-9]:)? logical replication target relation "public.t2"
is missing replicated columns: "c4", "c5"/,
$offset);
# cleanup
