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

Thanks for the revised version.

At Tue, 8 Sep 2020 22:36:17 +0530, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote in
> Thanks for the comments. Attaching the v3 patch.
>
> On Tue, Sep 8, 2020 at 8:19 PM Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> >
> > This looks a bit fiddly. Would it be less cumbersome to use
> > quote_identifier here instead?
> >
>
> Changed. quote_identifier() adds quotes wherever necessary.
>
> >
> > Please do use errmsg_plural -- have the new function return the number
> > of missing columns. Should be pretty straightforward.
> >
>
> Changed. Now the error message looks as below:

^^;

I don't think the logicalrep_find_missing_attrs worth a separate
function. The detection code would be short enough to be embedded in
the checking loop in logicalrep_rel_open. Note that remoterel doesn't
have missing columns since they are already removed when it is
constructed. See fetch_remote_table_info and
logicalrep_rel_att_by_name is relying on that fact. As the result
this code could be reduced to something like the following.

+ /* 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>);
+ }

> 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.c254: errmsg("primary key column \"%s\" is not marked NOT NULL",
./catalog/heap.c614: errmsg("column \"%s\" has pseudo-type %s",
./catalog/heap.c706: errmsg("no collation was derived for column \"%s\" with collatable type %s",

And we need a space after the semicolon and commas in the message
string.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center