Thanks for the review comments. Attaching v4 patch.
On Fri, Sep 11, 2020 at 6:44 AM Kyotaro Horiguchi <horikyota....@gmail.com>
wrote:
>
> + /* remoterel doesn't contain dropped attributes, see .... */
> - found = 0;
> + missing_attr = bms_add_range(NULL, 0, remoterel->natts - 1);
> for (i = 0; i < desc->natts; i++)
> if (attnum >= 0)
> - found++;
> + missing_attrs = bms_del_member(missing_attrs, attnum);
> - if (found < remoterel->natts)
> + if (!bms_is_empty(missing_attrs))
> + {
> + while ((i = bms_first_memeber(missing_attrs)) >= 0)
> + {
> + if (not_first) appendStringInfo(<delimter>);
> + appendStringInfo(str, remoterel->attnames[i])
> + }
> - erreport("some attrs missing");
> + ereport(ERROR, <blah blah>);
> + }
>
+1. Yes, the remoterel doesn't contain dropped attributes.
>
> > ERROR: logical replication target relation "public.test_tbl1" is
> > missing replicated columns:b1,c1,e1
>
> I think we always double-quote identifiers in error messages. For
> example:
>
> ./catalog/index.c 254: errmsg("primary key column \"%s\" is not marked
NOT NULL",
> ./catalog/heap.c 614: errmsg("column \"%s\" has pseudo-type %s",
> ./catalog/heap.c 706: errmsg("no collation was derived for column \"%s\"
with collatable type %s",
>
Double-quoting identifiers such as database object names helps in clearly
identifying them from the normal error message text. Seems like everywhere
we double-quote columns in the error messages. One exception I found
though, for relation names(there may be more places where we are not
double-quoting) is in errmsg("could not open parent table of index %s",
RelationGetRelationName(indrel).
How about quoting all the individual columns? Looks like quote_identifier()
doesn't serve our purpose here as it selectively quotes or quotes all
identifiers only in case quote_all_identifiers config variable is set.
CREATE TABLE t1(a1 int, b1 text, c1 real, d1 int, e1 int);
2020-09-10 23:08:26.737 PDT [217404] ERROR: logical replication target
relation "public.t1" is missing replicated column: "c1"
2020-09-10 23:08:31.768 PDT [217406] ERROR: logical replication target
relation "public.t1" is missing replicated columns: "c1", "d1"
2020-09-10 23:08:51.898 PDT [217417] ERROR: logical replication target
relation "public.t1" is missing replicated columns: "c1", "d1", "e1"
CREATE TABLE t1("!A1" int, "%b1" text, "@C1" int, d1 real, E1 int);
2020-09-10 23:12:29.768 PDT [217501] ERROR: logical replication target
relation "public.t1" is missing replicated column: "!A1"
2020-09-10 23:12:56.640 PDT [217515] ERROR: logical replication target
relation "public.t1" is missing replicated columns: "!A1", "@C1"
2020-09-10 23:13:31.848 PDT [217533] ERROR: logical replication target
relation "public.t1" is missing replicated columns: "!A1", "@C1", "d1"
>
> And we need a space after the semicolon and commas in the message
> string.
>
Changed.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From 0edd4d9a61af938d703cb06018011b0165df6957 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddy@enterprisedb.com>
Date: Fri, 11 Sep 2020 12:02:44 +0530
Subject: [PATCH v4] Detail message with names of missing columns in logical
replication
In logical replication when a subscriber is missing some columns,
it currently emits an error message that says "some" columns are
missing(see logicalrep_rel_open()), but it doesn't specify what
the missing column names are. The comment there also says that
finding the missing column names is a todo item(/* TODO, detail
message with names of missing columns */).
This patch finds the missing columns on the subscriber relation
using the publisher relation columns and show them in the error
message which makes error to be more informative to the user.
---
src/backend/replication/logical/relation.c | 38 +++++++++++++++++-----
1 file changed, 30 insertions(+), 8 deletions(-)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index 3d2d56295b..1d9fbc9a59 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -282,11 +282,11 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
if (!OidIsValid(entry->localreloid))
{
- int found;
Bitmapset *idkey;
TupleDesc desc;
MemoryContext oldctx;
int i;
+ Bitmapset *missingatts;
/* Check for supported relkind. */
CheckSubscriptionRelkind(entry->localrel->rd_rel->relkind,
@@ -302,7 +302,7 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
entry->attrmap = make_attrmap(desc->natts);
MemoryContextSwitchTo(oldctx);
- found = 0;
+ missingatts = bms_add_range(NULL, 0, remoterel->natts - 1);
for (i = 0; i < desc->natts; i++)
{
int attnum;
@@ -319,16 +319,38 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
entry->attrmap->attnums[i] = attnum;
if (attnum >= 0)
- found++;
+ missingatts = bms_del_member(missingatts, attnum);
}
- /* TODO, detail message with names of missing columns */
- if (found < remoterel->natts)
+ /* Report error with names of the missing localrel column(s). */
+ if (!bms_is_empty(missingatts))
+ {
+ StringInfoData missingattsbuf;
+ int missingattcnt = 0;
+
+ initStringInfo(&missingattsbuf);
+ while ((i = bms_first_member(missingatts)) >= 0)
+ {
+ missingattcnt++;
+ if (missingattcnt > 1)
+ appendStringInfoString(&missingattsbuf, ", ");
+
+ appendStringInfo(&missingattsbuf, "\"%s\"",
+ remoterel->attnames[i]);
+ }
+
+ bms_free(missingatts);
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- errmsg("logical replication target relation \"%s.%s\" is missing "
- "some replicated columns",
- remoterel->nspname, remoterel->relname)));
+ 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)));
+ }
/*
* Check that replica identity matches. We allow for stricter replica
--
2.25.1
