Re: Logical Replication - detail message with names of missing columns

Thanks for the review comments. Attaching v4 patch.

On Fri, Sep 11, 2020 at 6:44 AM Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)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