On Tue, Jan 28, 2020 at 11:28 AM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Wed, Jan 22, 2020 at 10:30 AM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > On Tue, Jan 14, 2020 at 10:44 AM Amit Kapila <amit.kapil...@gmail.com> > > wrote: > > > > > > > > > Hmm, I think this can turn out to be inefficient because we can easily > > > end up spilling the data even when we don't need to so. Consider > > > cases, where part of the streamed changes are for toast, and remaining > > > are the changes which we would have streamed and hence can be removed. > > > In such cases, we could have easily consumed remaining changes for > > > toast without spilling. Also, I am not sure if spilling changes from > > > the hash table is a good idea as they are no more in the same order as > > > they were in ReorderBuffer which means the order in which we serialize > > > the changes normally would change and that might have some impact, so > > > we would need some more study if we want to pursue this idea. > > I have fixed this bug and attached it as a separate patch. I will > > merge it to the main patch after we agree with the idea and after some > > more testing. > > > > The idea is that whenever we get the toasted chunk instead of directly > > inserting it into the toast hash I am inserting it into some local > > list so that if we don't get the change for the main table then we can > > insert these changes back to the txn->changes. So once we get the > > change for the main table at that time I am preparing the hash table > > to merge the chunks. > > > > > I think this idea will work but appears to be quite costly because (a) > you might need to serialize/deserialize the changes multiple times and > might attempt streaming multiple times even though you can't do (b) > you need to remove/add the same set of changes from the main list > multiple times. I agree with this. > > It seems to me that we need to add all of this new handling because > while taking the decision whether to stream or not we don't know > whether the txn has changes that can't be streamed. One idea to make > it work is that we identify it while decoding the WAL. I think we > need to set a bit in the insert/delete WAL record to identify if the > tuple belongs to a toast relation. This won't add any additional > overhead in WAL and reduce a lot of complexity in the logical decoding > and also decoding will be efficient. If this is feasible, then we can > do the same for speculative insertions. The Idea looks good to me. I will work on this.
> > In patch v8-0013-Bugfix-handling-of-incomplete-toast-tuple, why is > below change required? > > --- a/contrib/test_decoding/logical.conf > +++ b/contrib/test_decoding/logical.conf > @@ -1,3 +1,4 @@ > wal_level = logical > max_replication_slots = 4 > logical_decoding_work_mem = 64kB > +logging_collector=on Sorry, these are some local changes which got included in the patch. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
