On Mon, Aug 9, 2021 at 1:36 AM Rahila Syed <rahilasyed90@gmail.com> wrote:
>
>> Having said that, I'm not sure I agree with this design decision; what I
>> think this is doing is hiding from the user the fact that they are
>> publishing columns that they don't want to publish. I think as a user I
>> would rather get an error in that case:
>
>
>> ERROR: invalid column list in published set
>> DETAIL: The set of published commands does not include all the replica identity columns.
>
>
>> or something like that. Avoid possible nasty surprises of security-
>> leaking nature.
>
>
> Ok, Thank you for your opinion. I agree that giving an explicit error in this case will be safer.
>
+1 for an explicit error in this case.
Can you please explain why you have the restriction for including
replica identity columns and do we want to put a similar restriction
for the primary key? As far as I understand, if we allow default
values on subscribers for replica identity, then probably updates,
deletes won't work as they need to use replica identity (or PK) to
search the required tuple. If so, shouldn't we add this restriction
only when a publication has been defined for one of these (Update,
Delete) actions?
Another point is what if someone drops the column used in one of the
publications? Do we want to drop the entire relation from publication
or just remove the column filter or something else?
Do we want to consider that the columns specified in the filter must
not have NOT NULL constraint? Because, otherwise, the subscriber will
error out inserting such rows?
Minor comments:
================
pq_sendbyte(out, flags);
-
/* attribute name */
pq_sendstring(out, NameStr(att->attname));
@@ -953,6 +1000,7 @@ logicalrep_write_attrs(StringInfo out, Relation rel)
/* attribute mode */
pq_sendint32(out, att->atttypmod);
+
}
bms_free(idattrs);
diff --git a/src/backend/replication/logical/relation.c
b/src/backend/replication/logical/relation.c
index c37e2a7e29..d7a7b00841 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -354,7 +354,6 @@ logicalrep_rel_open(LogicalRepRelId remoteid,
LOCKMODE lockmode)
attnum = logicalrep_rel_att_by_name(remoterel,
NameStr(attr->attname));
-
entry->attrmap->attnums[i] = attnum;
There are quite a few places in the patch that contains spurious line
additions or removals.
--
With Regards,
Amit Kapila.
