Hi Shubham,
Here are my review comments for patch v5-0001.
Please don't reply with a blanket "I have fixed the given comments"
because it was not true. E.g., some of my previous comments are
rejected in favour of Amit's better code suggestion, but then other
comments seem not addressed for reasons unknown.
======
Commit message.
1.
Now that the errors for the 'missing' and 'generated' columns are
separated, it means that if some subscriber table suffers both
problems at the same time then only one of those errors can be
reported. I think you should mention here that if that happens the
missing column error takes precedence.
======
src/backend/replication/logical/relation.c
get_attrs_str:
2.
+ * Generates a comma-separated string of attribute names based on the provided
+ * relation information and a bitmap indicating which attributes are included.
+ *
+ * The result is a palloc'd string.
"Generate"?
I think you can simplify the function comment a bit (also mentioning
the palloc'd string seemed overkill to me).
SUGGESTION:
Returns a comma-separated string of attribute names based on the
provided relation and bitmap indicating which attributes to include.
~
3.
+static char *
+get_attrs_str(LogicalRepRelation *remoterel, Bitmapset *atts)
All other static functions in this file have a common prefix
'logicalrep_', so it will be better for this to follow the same
pattern.
~~~~
logicalrep_report_missing_and_gen_attrs:
4.
+/*
+ * If !bms_is_empty(missingatts), report the error message as 'Missing
+ * replicated columns.' Otherwise, report the error message as
'Cannot replicate
+ * to generated columns.'
+ */
The function comment does not need to include code fragments or spell
out the actual errorS because the code is self-explanatory. Anyway,
the "Otherwise" here was not quite correct because the generated BMS
is also checked for emptiness. Finally, I think here it is better to
be explicit about the case when there are BOTH errors -- e.g. say that
the 'missing' error wins.
So the whole function comment can be simplified.
SUGGESTION:
/*
* If attempting to replicate to subscriber side missing columns or generated
* columns then report an error.
*
* (If there are both kinds of errors the 'missing' error takes precedence).
*/
~
5.
+static void
+logicalrep_report_missing_and_gen_attrs(LogicalRepRelation *remoterel,
+ Bitmapset *missingatts,
+ Bitmapset *genatts)
5a.
As I wrote in the previous review [1 - #1], because only one error can
happen at a time, IMO this function name should be
'logicalrep_report_missing_or_gen_attrs' (e.g. 'or' not 'and').
~
5b.
/genatts/generatedatts/ (that is what you called the BMS in the
caller, so better to be consistent)
~
logicalrep_rel_open:
6.
+ Bitmapset *missingatts; /* Bitmapset for missing attributes. */
+ Bitmapset *generatedattrs = NULL; /* Bitmapset for generated
+ * attributes. */
Those comments don't achieve anything because they are just saying the
same as the code. You might as well remove them.
~
7.
+ /*
+ * Report any missing and generated columns. Note, if there are both
+ * kinds then the 'missing' error takes precedence.
+ */
+ logicalrep_report_missing_and_gen_attrs(remoterel, missingatts,
+ generatedattrs);
This comment can also be removed. The function name is already
self-explanatory, and the information of the "Note" part belongs in
the function comment.
======
src/test/subscription/t/011_generated.pl
The tests LGTM.
======
Please refer to the attached diffs patch which includes most (but not
all) of the suggestions mentioned above.
======
[1]
https://www.postgresql.org/message-id/CAHut%2BPuoDsPUO1YDBOEWAsKT8dXA0PDoK6S_Yc6kO_s8yPKHfA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia.
diff --git a/src/backend/replication/logical/relation.c
b/src/backend/replication/logical/relation.c
index cdce752..7fb1604 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -220,13 +220,11 @@ logicalrep_rel_att_by_name(LogicalRepRelation *remoterel,
const char *attname)
}
/*
- * Generates a comma-separated string of attribute names based on the provided
- * relation information and a bitmap indicating which attributes are included.
- *
- * The result is a palloc'd string.
+ * Returns a comma-separated string of attribute names based on the provided
+ * relation and bitmap indicating which attributes to include.
*/
static char *
-get_attrs_str(LogicalRepRelation *remoterel, Bitmapset *atts)
+logicalrep_get_attrs_str(LogicalRepRelation *remoterel, Bitmapset *atts)
{
StringInfoData attsbuf;
int attcnt = 0;
@@ -249,12 +247,13 @@ get_attrs_str(LogicalRepRelation *remoterel, Bitmapset
*atts)
}
/*
- * If !bms_is_empty(missingatts), report the error message as 'Missing
- * replicated columns.' Otherwise, report the error message as 'Cannot
replicate
- * to generated columns.'
+ * If attempting to replicate to subscriber side missing columns or generated
+ * columns then report an error.
+ *
+ * (If both problems exist then the 'missing' error takes precedence).
*/
static void
-logicalrep_report_missing_and_gen_attrs(LogicalRepRelation *remoterel,
+logicalrep_report_missing_or_gen_attrs(LogicalRepRelation *remoterel,
Bitmapset *missingatts,
Bitmapset *genatts)
{
@@ -266,7 +265,7 @@ logicalrep_report_missing_and_gen_attrs(LogicalRepRelation
*remoterel,
bms_num_members(missingatts),
remoterel->nspname,
remoterel->relname,
-
get_attrs_str(remoterel, missingatts)));
+
logicalrep_get_attrs_str(remoterel, missingatts)));
if (!bms_is_empty(genatts))
ereport(ERROR,
@@ -276,7 +275,7 @@ logicalrep_report_missing_and_gen_attrs(LogicalRepRelation
*remoterel,
bms_num_members(genatts),
remoterel->nspname,
remoterel->relname,
-
get_attrs_str(remoterel, genatts)));
+
logicalrep_get_attrs_str(remoterel, genatts)));
}
/*
@@ -401,9 +400,8 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE
lockmode)
TupleDesc desc;
MemoryContext oldctx;
int i;
- Bitmapset *missingatts; /* Bitmapset for missing
attributes. */
- Bitmapset *generatedattrs = NULL; /* Bitmapset for
generated
-
* attributes. */
+ Bitmapset *missingatts;
+ Bitmapset *generatedattrs = NULL;
/* Release the no-longer-useful attrmap, if any. */
if (entry->attrmap)
@@ -465,11 +463,7 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE
lockmode)
}
}
- /*
- * Report any missing and generated columns. Note, if there are
both
- * kinds then the 'missing' error takes precedence.
- */
- logicalrep_report_missing_and_gen_attrs(remoterel, missingatts,
+ logicalrep_report_missing_or_gen_attrs(remoterel, missingatts,
generatedattrs);
/* be tidy */
