Re: Replica Identity check of partition table on subscriber

On Tue, Jun 14, 2022 at 3:31 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Mon, Jun 13, 2022 at 6:14 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > I think we can do that way as well but do you see any benefit in it?
> > The way I am suggesting will avoid the effort of updating the remote
> > rel copy till we try to access that particular partition.
>
> I don't see any benefit as such to doing it the way the patch does,
> it's just that that seems to be the only way to go given the way
> things are.

Oh, I see that v4-0002 has this:

+/*
+ * Reset the entries in the partition map that refer to remoterel
+ *
+ * Called when new relation mapping is sent by the publisher to update our
+ * expected view of incoming data from said publisher.
+ *
+ * Note that we don't update the remoterel information in the entry here,
+ * we will update the information in logicalrep_partition_open to avoid
+ * unnecessary work.
+ */
+void
+logicalrep_partmap_reset_relmap(LogicalRepRelation *remoterel)
+{
+ HASH_SEQ_STATUS status;
+ LogicalRepPartMapEntry *part_entry;
+ LogicalRepRelMapEntry *entry;
+
+ if (LogicalRepPartMap == NULL)
+ return;
+
+ hash_seq_init(&status, LogicalRepPartMap);
+ while ((part_entry = (LogicalRepPartMapEntry *)
hash_seq_search(&status)) != NULL)
+ {
+ entry = &part_entry->relmapentry;
+
+ if (entry->remoterel.remoteid != remoterel->remoteid)
+ continue;
+
+ logicalrep_relmap_free_entry(entry);
+
+ memset(entry, 0, sizeof(LogicalRepRelMapEntry));
+ }
+}

The previous versions would also call logicalrep_relmap_update() on
the entry after the memset, which is no longer done, so that is indeed
saving useless work. I also see that both logicalrep_relmap_update()
and the above function basically invalidate the whole
LogicalRepRelMapEntry before setting the new remote relation info so
that the next logicaprep_rel_open() or logicalrep_partition_open()
have to refill the other members too.

Though, I thought maybe you were saying that we shouldn't need this
function for resetting partitions in the first place, which I guess
you weren't.

v4-0002 looks good btw, except the bitpick about test comment similar
to my earlier comment regarding v5-0001:

+# Change the column order of table on publisher

I think it might be better to say something specific to describe the
test intent, like:

Test that replication into partitioned target table continues to works
correctly when the published table is altered

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com