Re: Logical Replication - detail message with names of missing columns
On 2020-Oct-06, Amit Kapila wrote: > I have made a few changes, (a) moved free of missingatts in the caller > where we are allocating it. (b) added/edited/removed comments, (c) ran > pgindent. This is committed as f07707099c17. Thanks
Re: Logical Replication - detail message with names of missing columns
On Tue, Oct 6, 2020 at 5:24 PM Amit Kapila wrote: > > On Tue, Oct 6, 2020 at 12:14 PM Bharath Rupireddy > wrote: > > > > On Mon, Oct 5, 2020 at 9:39 AM Amit Kapila wrote: > > > > > > > > > 3. The patch doesn't seem to be freeing the memory allocated for > > > missingattsbuf. > > > > > > > I don't think we need to do that. We are passing missingattsbuf.data to > > ereport and we are safe without freeing up missingattsbuf(we don't reach > > the code after ereprot(ERROR,...)as the table sync worker anyways goes away > > after throwing missing attributes error( if (sigsetjmp(local_sigjmp_buf, 1) > > != 0) in StartBackgroundWorker and then proc_exit(1)). Each time a new > > table sync bg worker is spawned. > > > > Okay, by that logic, we don't even need to free memory for missingatts. > > I have made a few changes, (a) moved free of missingatts in the caller > where we are allocating it. (b) added/edited/removed comments, (c) ran > pgindent. > Thanks Amit. v8 patch looks good to me. > > Shall we backpatch this? I don't see any particular need to backpatch > this as this is a minor error message improvement and nobody has > reported any problem due to this. What do you think? > IMO, no backpatch is required as this is not a bug or something reported by anyone. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Re: Logical Replication - detail message with names of missing columns
On Tue, Oct 6, 2020 at 12:14 PM Bharath Rupireddy wrote: > > On Mon, Oct 5, 2020 at 9:39 AM Amit Kapila wrote: > > > > > > 3. The patch doesn't seem to be freeing the memory allocated for > > missingattsbuf. > > > > I don't think we need to do that. We are passing missingattsbuf.data to > ereport and we are safe without freeing up missingattsbuf(we don't reach the > code after ereprot(ERROR,...)as the table sync worker anyways goes away after > throwing missing attributes error( if (sigsetjmp(local_sigjmp_buf, 1) != 0) > in StartBackgroundWorker and then proc_exit(1)). Each time a new table sync > bg worker is spawned. > Okay, by that logic, we don't even need to free memory for missingatts. I have made a few changes, (a) moved free of missingatts in the caller where we are allocating it. (b) added/edited/removed comments, (c) ran pgindent. Shall we backpatch this? I don't see any particular need to backpatch this as this is a minor error message improvement and nobody has reported any problem due to this. What do you think? -- With Regards, Amit Kapila. v8-0001-Display-the-names-of-missing-columns-in-error-dur.patch Description: Binary data
Re: Logical Replication - detail message with names of missing columns
Thanks Amit for the review comments.
On Mon, Oct 5, 2020 at 9:39 AM Amit Kapila wrote:
>
> Few comments:
> ==
> 1.
> + /* Report error with names of the missing localrel column(s). */
> + if (!bms_is_empty(missingatts))
> + {
> + StringInfoData missingattsbuf;
> + intmissingattcnt = 0;
> + remoterel->nspname,
> + remoterel->relname,
> + missingattsbuf.data)));
> + }
>
> I think it is better to move the above code in a separate function
> (say logicalrep_report_missing_attrs or something like that).
>
Added a new function logicalrep_report_missing_attrs().
>
> 2. I think we always need to call bms_free(missingatts) because it is
> possible that there is no missing attribute and in that case, we won't
> free the memory allocated in bms_add_range.
>
Done. Yes we palloc memory for missingatts bitmap irrespective of missing
attributes. Added bms_free() out of if(!bms_is_empty(missingatts)) as well.
I also kept bms_free() before ereport(ERROR,..) to free up before throwing
the error. In anycase, only one bms_free() would get hit.
if (!bms_is_empty(missingatts))
{
StringInfoData missingattsbuf;
int missingattcnt = 0;
bms_free(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)));
}
bms_free(missingatts);
>
> 3. The patch doesn't seem to be freeing the memory allocated for
missingattsbuf.
>
I don't think we need to do that. We are passing missingattsbuf.data to
ereport and we are safe without freeing up missingattsbuf(we don't reach
the code after ereprot(ERROR,...)as the table sync worker anyways goes away
after throwing missing attributes error( if (sigsetjmp(local_sigjmp_buf, 1)
!= 0) in StartBackgroundWorker and then proc_exit(1)). Each time a new
table sync bg worker is spawned.
2020-10-06 10:18:27.063 IST [1599963] ERROR: logical replication target
relation "public.t1" is missing replicated column: "@C1"
2020-10-06 10:18:47.179 IST [1600134] ERROR: logical replication target
relation "public.t1" is missing replicated column: "@C1"
2020-10-06 10:18:57.234 IST [1600214] ERROR: logical replication target
relation "public.t1" is missing replicated column: "@C1"
2020-10-06 10:19:27.415 IST [1600458] ERROR: logical replication target
relation "public.t1" is missing replicated columns: "%b1", "@C1"
2020-10-06 10:19:42.506 IST [1600588] ERROR: logical replication target
relation "public.t1" is missing replicated columns: "%b1", "@C1"
2020-10-06 10:19:52.565 IST [1600669] ERROR: logical replication target
relation "public.t1" is missing replicated columns: "%b1", "@C1"
>
> 4.
> ereport(ERROR,
> (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> - errmsg("logical replication target relation \"%s.%s\" is missing "
>
> From the second line onwards, the message lines are not aligned in
> errmsg_plural.
>
Done.
>
> 5. Also, in the above message, keep the error string in a single line.
> For ex. see one of the existing messages:
> errmsg_plural("WAL segment size must be a power of two between 1 MB
> and 1 GB, but the control file specifies %d byte", .. I think it will
> be easy to read that way. I know this is not exactly related to your
> patch but improving it while changing this message seems fine.
>
Done.
Attaching v7 patch please consider it for further review.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From c050262f5ca70712f324129c32ccd4527657b2c5 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy
Date: Tue, 6 Oct 2020 10:49:25 +0530
Subject: [PATCH v7] 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 | 56 ++
1 file changed, 46 insertions(+), 10 deletions(-)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index 2bb8e7d57b..123e49d953 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -228,6 +228,47 @@ logicalrep_rel_att_by_name(LogicalRepRelation *remoterel, const char *attname)
return -1;
}
+/*
+ * Report error with names of the missing local relation column(s) if any,
+ * otherwise return.
+ */
+static
Re: Logical Replication - detail message with names of missing columns
On Mon, Oct 5, 2020 at 8:00 PM Bharath Rupireddy wrote: > > Thanks Amit for the review comments. I will post an updated patch soon. > > On Mon, Oct 5, 2020 at 9:39 AM Amit Kapila wrote: > > > > 6. I think we should add one test case for this in the existing > > regression test (src/test/subscription/t/008_diff_schema). > > > > This patch logs the missing column names message in subscriber server > logs. Is there a way to see the logs in these tests and use that as > expected result for the test case? > I don't think there is any direct way to achieve that. What we can do is to check that the data is not replicated in such a case but I don't think it is worth to add a test for that behavior. So, I think we can skip adding a test for this unless someone else has a better idea to do the same. -- With Regards, Amit Kapila.
Re: Logical Replication - detail message with names of missing columns
Thanks Amit for the review comments. I will post an updated patch soon. On Mon, Oct 5, 2020 at 9:39 AM Amit Kapila wrote: > > 6. I think we should add one test case for this in the existing > regression test (src/test/subscription/t/008_diff_schema). > This patch logs the missing column names message in subscriber server logs. Is there a way to see the logs in these tests and use that as expected result for the test case? With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Re: Logical Replication - detail message with names of missing columns
On Wed, Sep 16, 2020 at 9:58 PM Bharath Rupireddy
wrote:
>
> Attaching v6 patch, rebased because of a recent commit
> 3d65b0593c5578014f62e09d4008006f1783f64d. Please consider this for
> further review.
>
Few comments:
==
1.
+ /* Report error with names of the missing localrel column(s). */
+ if (!bms_is_empty(missingatts))
+ {
+ StringInfoData missingattsbuf;
+ intmissingattcnt = 0;
+
+ initStringInfo();
+ while ((i = bms_first_member(missingatts)) >= 0)
+ {
+ missingattcnt++;
+ if (missingattcnt == 1)
+ appendStringInfo(, _("\"%s\""),
+ remoterel->attnames[i]);
+ else
+ appendStringInfo(, _(", \"%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)));
+ }
I think it is better to move the above code in a separate function
(say logicalrep_report_missing_attrs or something like that).
2. I think we always need to call bms_free(missingatts) because it is
possible that there is no missing attribute and in that case, we won't
free the memory allocated in bms_add_range.
3. The patch doesn't seem to be freeing the memory allocated for missingattsbuf.
4.
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)));
>From the second line onwards, the message lines are not aligned in
errmsg_plural.
5. Also, in the above message, keep the error string in a single line.
For ex. see one of the existing messages:
errmsg_plural("WAL segment size must be a power of two between 1 MB
and 1 GB, but the control file specifies %d byte", .. I think it will
be easy to read that way. I know this is not exactly related to your
patch but improving it while changing this message seems fine.
6. I think we should add one test case for this in the existing
regression test (src/test/subscription/t/008_diff_schema).
--
With Regards,
Amit Kapila.
Re: Logical Replication - detail message with names of missing columns
Attaching v6 patch, rebased because of a recent commit 3d65b0593c5578014f62e09d4008006f1783f64d. Please consider this for further review. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com v6-0001-Detail-message-with-names-of-missing-columns-in-l.patch Description: Binary data
Re: Logical Replication - detail message with names of missing columns
On Fri, Sep 11, 2020 at 9:05 PM Alvaro Herrera wrote:
>
> On 2020-Sep-11, Tom Lane wrote:
>
> > Check, but you could imagine that the column-list string is constructed
> > with code along the lines of
> >
> > if (first)
> > appendStringInfo(buf, _("\"%s\""), colname);
> > else
> > appendStringInfo(buf, _(", \"%s\""), colname);
> > thus allowing a translator to replace the quote marks.
>
> This works OK for my language at least. I couldn't find any guidance on
> whether there's a problem doing things this way for RTL languages etc,
> but +1 for doing it this way.
>
Thanks for the comments. I changed the patch to use the string
preparation in below fashion. Attaching the v5 patch. Please let me
know if there are any further inputs.
+if (missingattcnt == 1)
+appendStringInfo(, _("\"%s\""),
+ remoterel->attnames[i]);
+else
+appendStringInfo(, _(", \"%s\""),
+ remoterel->attnames[i]);
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
v5-0001-Detail-message-with-names-of-missing-columns-in-l.patch
Description: Binary data
Re: Logical Replication - detail message with names of missing columns
Alvaro Herrera writes: > This works OK for my language at least. I couldn't find any guidance on > whether there's a problem doing things this way for RTL languages etc, > but +1 for doing it this way. Hmm ... fortunately, there's not any large semantic significance to the order in which the columns are mentioned here. Maybe an RTL speaker would read the column names in the opposite order than we do, but I don't think it's a problem. regards, tom lane
Re: Logical Replication - detail message with names of missing columns
On 2020-Sep-11, Tom Lane wrote:
> Check, but you could imagine that the column-list string is constructed
> with code along the lines of
>
> if (first)
> appendStringInfo(buf, _("\"%s\""), colname);
> else
> appendStringInfo(buf, _(", \"%s\""), colname);
> thus allowing a translator to replace the quote marks.
This works OK for my language at least. I couldn't find any guidance on
whether there's a problem doing things this way for RTL languages etc,
but +1 for doing it this way.
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Logical Replication - detail message with names of missing columns
Alvaro Herrera writes:
> On 2020-Sep-11, Tom Lane wrote:
>> NO. The convention is to write \"...\" in the translatable message.
> There is a problem here though, which is that the quoted strings in
> question are part of a list of columns. There's no way to keep that
> list as a translatable string, so that approach doesn't work here.
> What appears in the translatable string is:
> logical replication target relation "%s" is missing replicated columns: %s
Check, but you could imagine that the column-list string is constructed
with code along the lines of
if (first)
appendStringInfo(buf, _("\"%s\""), colname);
else
appendStringInfo(buf, _(", \"%s\""), colname);
thus allowing a translator to replace the quote marks. Might not be
worth the trouble. In any case, the point here is that we're not
trying to construct valid SQL so quote_identifier is not the right
tool for the job.
regards, tom lane
Re: Logical Replication - detail message with names of missing columns
On 2020-Sep-11, Tom Lane wrote: > > 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. > > NO. The convention is to write \"...\" in the translatable message. > Not all languages use double quote symbols for this purpose, so that > way lets translators replace them with something else. > > Yeah, this means that messages containing names that contain double > quotes might be a bit ambiguous. We live with it. There is a problem here though, which is that the quoted strings in question are part of a list of columns. There's no way to keep that list as a translatable string, so that approach doesn't work here. What appears in the translatable string is: logical replication target relation "%s" is missing replicated columns: %s Or in the singular case: logical replication target relation "%s" is missing replicated columns: %s where the second %s is the list of columns. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Logical Replication - detail message with names of missing columns
Bharath Rupireddy writes:
>> I think we always double-quote identifiers in error messages. For
>> example:
>> ./catalog/heap.c 706: errmsg("no collation was derived for column \"%s\"
>> with collatable type %s",
Right. Anything in this patch that is not doing that needs to be fixed.
(As this example shows, type names are exempt from the rule.)
> 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.
NO. The convention is to write \"...\" in the translatable message.
Not all languages use double quote symbols for this purpose, so that
way lets translators replace them with something else.
Yeah, this means that messages containing names that contain double
quotes might be a bit ambiguous. We live with it.
regards, tom lane
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
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();
> + appendStringInfo(str, remoterel->attnames[i])
> + }
> - erreport("some attrs missing");
> + ereport(ERROR, );
> + }
>
+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
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,
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
wrote in
> Thanks for the comments. Attaching the v3 patch.
>
> On Tue, Sep 8, 2020 at 8:19 PM Alvaro Herrera
> 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();
+ appendStringInfo(str, remoterel->attnames[i])
+ }
- erreport("some attrs missing");
+ ereport(ERROR, );
+ }
> 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
Re: Logical Replication - detail message with names of missing columns
Added this to the commitfest - https://commitfest.postgresql.org/30/2727/ With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Re: Logical Replication - detail message with names of missing columns
Thanks for the comments. Attaching the v3 patch.
On Tue, Sep 8, 2020 at 8:19 PM Alvaro Herrera 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:
CREATE TABLE test_tbl1(a1 int, b1 text, c1 int, d1 real, e1 int);
ERROR: logical replication target relation "public.test_tbl1" is
missing replicated column:c1
ERROR: logical replication target relation "public.test_tbl1" is
missing replicated columns:b1,c1
ERROR: logical replication target relation "public.test_tbl1" is
missing replicated columns:b1,c1,e1
CREATE TABLE test_tbl1("!A1" int, "%b1" text, "@C1" int, d1 real, E1 int);
ERROR: logical replication target relation "public.test_tbl1" is
missing replicated column:"@C1"
ERROR: logical replication target relation "public.test_tbl1" is
missing replicated columns:"@C1",d1
ERROR: logical replication target relation "public.test_tbl1" is
missing replicated columns:"!A1","@C1",d1
ERROR: logical replication target relation "public.test_tbl1" is
missing replicated columns:"!A1","@C1",d1,e1
ERROR: logical replication target relation "public.test_tbl1" is
missing replicated columns:"!A1","%b1","@C1",d1,e1
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
v3-0001-Detail-message-with-names-of-missing-columns-in-l.patch
Description: Binary data
Re: Logical Replication - detail message with names of missing columns
On 2020-Sep-08, Bharath Rupireddy wrote:
> + /* Find the remote attributes that are missing in the local relation. */
> + for (i = 0; i < remoterel->natts; i++)
> + {
> + if (!bms_is_member(i, localattnums))
> + {
> + if (missingatts->len == 0)
> + {
> + appendStringInfoChar(missingatts, '"');
> + appendStringInfoString(missingatts,
> remoterel->attnames[i]);
> + }
> + else if (missingatts->len > 0)
> + {
> + appendStringInfoChar(missingatts, ',');
> + appendStringInfoChar(missingatts, '"');
> + appendStringInfo(missingatts, "%s",
> remoterel->attnames[i]);
> + }
> +
> + appendStringInfoChar(missingatts, '"');
> + }
This looks a bit fiddly. Would it be less cumbersome to use
quote_identifier here instead?
> 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)));
> + "replicated columns:%s",
> + remoterel->nspname,
> remoterel->relname,
> + missingatts.data)));
Please do use errmsg_plural -- have the new function return the number
of missing columns. Should be pretty straightforward.
--
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Logical Replication - detail message with names of missing columns
Thanks for the comments, v2 patch is attached.
>
> On Tue, Sep 8, 2020 at 6:50 AM Kyotaro Horiguchi
> wrote:
> >
> > +1 for objective. However, that can be done simpler way that doesn't
> > need additional loops by using bitmapset to hold missing remote
> > attribute numbers. This also make the variable "found" useless.
> >
>
> Thanks. I will look into it and post a v2 patch soon.
>
Changed.
> >
> > FWIW, I would prefer that the message be like
> >
> > logical replication target relation "public.test_1" is missing
> > replicated columns: "a1", "c1"
> >
>
> This looks fine, I will change that.
>
Changed. Now the error looks like as shown below:
ERROR: logical replication target relation "public.test_tbl1" is
missing replicated columns:"d1"
ERROR: logical replication target relation "public.test_tbl1" is
missing replicated columns:"c1","d1"
ERROR: logical replication target relation "public.test_tbl1" is
missing replicated columns:"a1","c1","d1"
ERROR: logical replication target relation "public.test_tbl1" is
missing replicated columns:"a1","b1","c1","d1"
ERROR: logical replication target relation "public.test_tbl1" is
missing replicated columns:"a1","b1","c1","d1","e1"
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From 9fbda09a015895a447bdce6683e8138350b133bd Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy
Date: Tue, 8 Sep 2020 02:45:04 -0700
Subject: [PATCH v2] 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 | 60 --
1 file changed, 57 insertions(+), 3 deletions(-)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index a60c73d74d..2ecb4766a4 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -227,6 +227,53 @@ logicalrep_rel_att_by_name(LogicalRepRelation *remoterel, const char *attname)
return -1;
}
+/*
+ * Finds the attributes that are missing in the local relation
+ * compared to remote relation.
+ */
+static void
+logicalrep_find_missing_atts(AttrMap*attrmap,
+ LogicalRepRelation *remoterel,
+ StringInfo missingatts)
+{
+ int i;
+ Bitmapset *localattnums = NULL;
+
+ /* Add local relation attnums to a bitmapset. */
+ for (i = 0; i < attrmap->maplen; i++)
+ {
+ /*
+ * Do not add attnums with values -1 to bitmapset
+ * as it doesn't take negative values. Attnums value
+ * is -1 for a dropped or a generated local attribute.
+ */
+ if (attrmap->attnums[i] >= 0)
+ localattnums = bms_add_member(localattnums, attrmap->attnums[i]);
+ }
+
+ /* Find the remote attributes that are missing in the local relation. */
+ for (i = 0; i < remoterel->natts; i++)
+ {
+ if (!bms_is_member(i, localattnums))
+ {
+ if (missingatts->len == 0)
+ {
+appendStringInfoChar(missingatts, '"');
+appendStringInfoString(missingatts, remoterel->attnames[i]);
+ }
+ else if (missingatts->len > 0)
+ {
+appendStringInfoChar(missingatts, ',');
+appendStringInfoChar(missingatts, '"');
+appendStringInfo(missingatts, "%s", remoterel->attnames[i]);
+ }
+
+ appendStringInfoChar(missingatts, '"');
+ }
+ }
+ bms_free(localattnums);
+}
+
/*
* Open the local relation associated with the remote one.
*
@@ -322,13 +369,20 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
found++;
}
- /* TODO, detail message with names of missing columns */
+ /* Report error with names of the missing localrel column(s). */
if (found < remoterel->natts)
+ {
+ StringInfoData missingatts;
+
+ initStringInfo();
+ logicalrep_find_missing_atts(entry->attrmap, remoterel, );
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)));
+ "replicated columns:%s",
+ remoterel->nspname, remoterel->relname,
+ missingatts.data)));
+ }
/*
* Check that replica identity matches. We allow for stricter replica
--
2.25.1
Re: Logical Replication - detail message with names of missing columns
On Tue, Sep 8, 2020 at 6:50 AM Kyotaro Horiguchi wrote: > > +1 for objective. However, that can be done simpler way that doesn't > need additional loops by using bitmapset to hold missing remote > attribute numbers. This also make the variable "found" useless. > Thanks. I will look into it and post a v2 patch soon. > > > Here's a snapshot how the error looks with the patch: > > 2020-09-04 01:00:36.721 PDT [843128] ERROR: logical replication > > target relation "public.test_1" is missing "b1, d1" replicated columns > > 2020-09-04 01:00:46.784 PDT [843132] ERROR: logical replication > > target relation "public.test_1" is missing "b1" replicated columns > > 2020-09-06 21:24:53.645 PDT [902945] ERROR: logical replication > > target relation "public.test_1" is missing "a1, c1, d1, b1" replicated > > columns > > > > Thoughts? > > FWIW, I would prefer that the message be like > > logical replication target relation "public.test_1" is missing > replicated columns: "a1", "c1" > This looks fine, I will change that. > > I'm not sure we need to have separate messages for singular and plural. > I don't think we need to have separate messages. To keep it simple, let's have plural form. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Re: Logical Replication - detail message with names of missing columns
Thank you for working on this. At Mon, 7 Sep 2020 16:30:59 +0530, Bharath Rupireddy wrote in > Hi, > > I observed that, 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 */). > > I propose a patch to find the missing columns on the subscriber > relation using the publisher relation columns and show them in the > error message which can make the error more informative to the user. +1 for objective. However, that can be done simpler way that doesn't need additional loops by using bitmapset to hold missing remote attribute numbers. This also make the variable "found" useless. > Here's a snapshot how the error looks with the patch: > 2020-09-04 01:00:36.721 PDT [843128] ERROR: logical replication > target relation "public.test_1" is missing "b1, d1" replicated columns > 2020-09-04 01:00:46.784 PDT [843132] ERROR: logical replication > target relation "public.test_1" is missing "b1" replicated columns > 2020-09-06 21:24:53.645 PDT [902945] ERROR: logical replication > target relation "public.test_1" is missing "a1, c1, d1, b1" replicated > columns > > Thoughts? FWIW, I would prefer that the message be like logical replication target relation "public.test_1" is missing replicated columns: "a1", "c1" I'm not sure we need to have separate messages for singlar and plural. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Logical Replication - detail message with names of missing columns
Hi,
I observed that, 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 */).
I propose a patch to find the missing columns on the subscriber
relation using the publisher relation columns and show them in the
error message which can make the error more informative to the user.
Here's a snapshot how the error looks with the patch:
2020-09-04 01:00:36.721 PDT [843128] ERROR: logical replication
target relation "public.test_1" is missing "b1, d1" replicated columns
2020-09-04 01:00:46.784 PDT [843132] ERROR: logical replication
target relation "public.test_1" is missing "b1" replicated columns
2020-09-06 21:24:53.645 PDT [902945] ERROR: logical replication
target relation "public.test_1" is missing "a1, c1, d1, b1" replicated
columns
Thoughts?
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
From e923dc6ddfa9b41c70f351ba6ad7dc8cfa8dbde3 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy
Date: Fri, 4 Sep 2020 21:10:52 -0700
Subject: [PATCH v1] 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 | 50 --
1 file changed, 47 insertions(+), 3 deletions(-)
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index a60c73d74d..d34ea3ce39 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -227,6 +227,44 @@ logicalrep_rel_att_by_name(LogicalRepRelation *remoterel, const char *attname)
return -1;
}
+/*
+ * Finds the attributes that are missing in the local relation
+ * compared to remote relation.
+ */
+static void
+logicalrep_find_missing_atts(Relation localrel,
+ LogicalRepRelation *remoterel,
+ StringInfo missingatts)
+{
+ int i;
+ int j;
+ TupleDesc desc;
+
+ desc = RelationGetDescr(localrel);
+
+ for (i = 0; i < remoterel->natts; i++)
+ {
+ bool found = false;
+ for (j = 0; j < desc->natts; j++)
+ {
+ Form_pg_attribute attr = TupleDescAttr(desc, j);
+ if (strcmp(remoterel->attnames[i], NameStr(attr->attname)) == 0)
+ {
+found = true;
+break;
+ }
+ }
+
+ if (found == false)
+ {
+ if (missingatts->len == 0)
+appendStringInfoString(missingatts, remoterel->attnames[i]);
+ else if (missingatts->len > 0)
+appendStringInfo(missingatts, ", %s", remoterel->attnames[i]);
+ }
+ }
+}
+
/*
* Open the local relation associated with the remote one.
*
@@ -322,13 +360,19 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
found++;
}
- /* TODO, detail message with names of missing columns */
if (found < remoterel->natts)
+ {
+ StringInfoData missingatts;
+
+ initStringInfo();
+ logicalrep_find_missing_atts(entry->localrel, remoterel, );
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)));
+ "\"%s\" replicated columns",
+ remoterel->nspname, remoterel->relname,
+ missingatts.data)));
+ }
/*
* Check that replica identity matches. We allow for stricter replica
--
2.25.1
