Improve error message for replication of generated columns.

Currently, logical replication produces a generic error message when
targeting a subscriber-side table column that is either missing or
generated. The error message can be misleading for generated columns.

This patch introduces a specific error message to clarify the issue when
generated columns are involved.

Author: Shubham Khanna
Reviewed-by: Peter Smith, Vignesh C, Amit Kapila
Discussion: https://postgr.es/m/CAHv8RjJBvYtqU7OAofBizOmQOK2Q8h+w9v2_cQWxT_gO7er3Aw@mail.gmail.com

diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index f5a0ef2bd9dde07d444a3148197fc1e980e913bd..b355f32f03c3e8d2059cfa4eaa1d2a55149db09d 100644 (file)
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -220,41 +220,63 @@ logicalrep_rel_att_by_name(LogicalRepRelation *remoterel, const char *attname)
 }
 
 /*
- * Report error with names of the missing local relation column(s), if any.
+ * Returns a comma-separated string of attribute names based on the provided
+ * relation and bitmap indicating which attributes to include.
  */
-static void
-logicalrep_report_missing_attrs(LogicalRepRelation *remoterel,
-                                                               Bitmapset *missingatts)
+static char *
+logicalrep_get_attrs_str(LogicalRepRelation *remoterel, Bitmapset *atts)
 {
-       if (!bms_is_empty(missingatts))
+       StringInfoData attsbuf;
+       int                     attcnt = 0;
+       int                     i = -1;
+
+       Assert(!bms_is_empty(atts));
+
+       initStringInfo(&attsbuf);
+
+       while ((i = bms_next_member(atts, i)) >= 0)
        {
-               StringInfoData missingattsbuf;
-               int                     missingattcnt = 0;
-               int                     i;
+               attcnt++;
+               if (attcnt > 1)
+                       appendStringInfo(&attsbuf, _(", "));
 
-               initStringInfo(&missingattsbuf);
+               appendStringInfo(&attsbuf, _("\"%s\""), remoterel->attnames[i]);
+       }
 
-               i = -1;
-               while ((i = bms_next_member(missingatts, i)) >= 0)
-               {
-                       missingattcnt++;
-                       if (missingattcnt == 1)
-                               appendStringInfo(&missingattsbuf, _("\"%s\""),
-                                                                remoterel->attnames[i]);
-                       else
-                               appendStringInfo(&missingattsbuf, _(", \"%s\""),
-                                                                remoterel->attnames[i]);
-               }
+       return attsbuf.data;
+}
 
+/*
+ * If attempting to replicate missing or generated columns, report an error.
+ * Prioritize 'missing' errors if both occur though the prioritization is
+ * arbitrary.
+ */
+static void
+logicalrep_report_missing_or_gen_attrs(LogicalRepRelation *remoterel,
+                                                                          Bitmapset *missingatts,
+                                                                          Bitmapset *generatedatts)
+{
+       if (!bms_is_empty(missingatts))
                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)));
-       }
+                               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",
+                                                         bms_num_members(missingatts),
+                                                         remoterel->nspname,
+                                                         remoterel->relname,
+                                                         logicalrep_get_attrs_str(remoterel,
+                                                                                                          missingatts)));
+
+       if (!bms_is_empty(generatedatts))
+               ereport(ERROR,
+                               errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+                               errmsg_plural("logical replication target relation \"%s.%s\" has incompatible generated column: %s",
+                                                         "logical replication target relation \"%s.%s\" has incompatible generated columns: %s",
+                                                         bms_num_members(generatedatts),
+                                                         remoterel->nspname,
+                                                         remoterel->relname,
+                                                         logicalrep_get_attrs_str(remoterel,
+                                                                                                          generatedatts)));
 }
 
 /*
@@ -380,6 +402,7 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
                MemoryContext oldctx;
                int                     i;
                Bitmapset  *missingatts;
+               Bitmapset  *generatedattrs = NULL;
 
                /* Release the no-longer-useful attrmap, if any. */
                if (entry->attrmap)
@@ -421,7 +444,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 +455,20 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
 
                        entry->attrmap->attnums[i] = attnum;
                        if (attnum >= 0)
+                       {
+                               /* Remember which subscriber columns are generated. */
+                               if (attr->attgenerated)
+                                       generatedattrs = bms_add_member(generatedattrs, attnum);
+
                                missingatts = bms_del_member(missingatts, attnum);
+                       }
                }
 
-               logicalrep_report_missing_attrs(remoterel, missingatts);
+               logicalrep_report_missing_or_gen_attrs(remoterel, missingatts,
+                                                                                          generatedattrs);
 
                /* 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 211b54c316214508a87ee9aa707000ad328cceab..66e6d8da5ac776783bdd61875a04f7427fe9698f 100644 (file)
--- a/src/test/subscription/t/011_generated.pl
+++ b/src/test/subscription/t/011_generated.pl
@@ -326,4 +326,46 @@ is( $result, qq(|2|
 $node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub1");
 $node_publisher->safe_psql('postgres', "DROP PUBLICATION pub1");
 
+# =============================================================================
+# The following test verifies the expected error when replicating to a
+# generated subscriber column. Test the following combinations:
+# - regular -> generated
+# - generated -> generated
+# =============================================================================
+
+# --------------------------------------------------
+# A "regular -> generated" or "generated -> generated" replication fails,
+# reporting an error that the generated column on the subscriber side cannot
+# be replicated.
+#
+# Test Case: regular -> 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]:)? logical replication target relation "public.t1" has incompatible generated columns: "c2", "c3"/,
+       $offset);
+
+# cleanup
+$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub1");
+$node_publisher->safe_psql('postgres', "DROP PUBLICATION pub1");
+
 done_testing();