Thanks for the review comments. Attaching v4 patch.
On Fri, Sep 11, 2020 at 6:44 AM Kyotaro Horiguchi <
horikyota.ntt@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
