On Tue, Jun 14, 2022 at 1:02 PM Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Tue, Jun 14, 2022 at 3:31 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Mon, Jun 13, 2022 at 6:14 PM Amit Kapila <amit.kapila16@gmail.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.
>
Right.
> 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
>
Okay changed this and slightly modify the comments and commit message.
I am just attaching the HEAD patches for the first two issues.
--
With Regards,
Amit Kapila.