On Mon, 7 Jun 2021 at 8:30 AM, Amit Kapila <amit.kapil...@gmail.com> wrote:
> On Wed, Jun 2, 2021 at 11:52 AM Amit Kapila <amit.kapil...@gmail.com> > wrote: > > > > On Wed, Jun 2, 2021 at 11:38 AM Dilip Kumar <dilipbal...@gmail.com> > wrote: > > > > > > On Wed, Jun 2, 2021 at 11:25 AM Amit Kapila <amit.kapil...@gmail.com> > wrote: > > > > > > > > I think the same relation case might not create a problem because it > > > > won't find the entry for it in the toast_hash, so it will return from > > > > there but the other two problems will be there. > > > > > > Right > > > > > > So, one idea could be > > > > to just avoid those two cases (by simply adding return for those > > > > cases) and still we can rely on toast clean up on the next > > > > insert/update/delete. However, your approach looks more natural to me > > > > as that will allow us to clean up memory in all cases instead of > > > > waiting till the transaction end. So, I still vote for going with > your > > > > patch's idea of cleaning at spec_abort but I am fine if you and > others > > > > decide not to process spec_abort message. What do you think? Tomas, > do > > > > you have any opinion on this matter? > > > > > > I agree that processing with spec abort looks more natural and ideally > > > the current code expects it to be getting cleaned after the change, > > > that's the reason we have those assertions and errors. > > > > > Okay, so, let's go with that approach. I have thought about whether it > creates any problem in back-branches but couldn't think of any > primarily because we are not going to send anything additional to > plugin/subscriber. Do you see any problems with back branches if we go > with this approach? I will check this and let you know. > > > OTOH I agree > > > that we can just return from those conditions because now we know that > > > with the current code those situations are possible. My vote is with > > > handling the spec abort option (Option1) because that looks more > > > natural way of handling these issues and we also don't have to clean > > > up the hash in "ReorderBufferReturnTXN" if no followup change after > > > spec abort. > > > > > > > Even, if we decide to go with spec_abort approach, it might be better > > to still keep the toastreset call in ReorderBufferReturnTXN so that it > > can be freed in case of error. > > > > Please take care of this as well. Ok > -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
