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: > > > > On Tue, Jun 1, 2021 at 5:23 PM Dilip Kumar <dilipbal...@gmail.com> wrote: > > > > > > On Tue, Jun 1, 2021 at 12:25 PM Amit Kapila <amit.kapil...@gmail.com> > > > wrote: > > > > > > > > > > > > > IMHO, as I stated earlier one way to fix this problem is that we add > > > > > the spec abort operation (DELETE + XLH_DELETE_IS_SUPER flag) to the > > > > > queue, maybe with action name > > > > > "REORDER_BUFFER_CHANGE_INTERNAL_SPEC_ABORT" and as part of processing > > > > > that just cleans up the toast and specinsert tuple and nothing else. > > > > > If we think that makes sense then I can work on that patch? > > > > > > > > > > > > > I think this should solve the problem but let's first try to see if we > > > > have any other problems. Because, say, if we don't have any other > > > > problem, then maybe removing Assert might work but I guess we still > > > > need to process the tuple to find that we don't need to assemble toast > > > > for it which again seems like overkill. > > > > > > Yeah, other operation will also fail, basically, if txn->toast_hash is > > > not NULL then we assume that we need to assemble the tuple using > > > toast, but if there is next insert in another relation and if that > > > relation doesn't have a toast table then it will fail with below > > > error. And otherwise also, if it is the same relation, then the toast > > > chunks of previous tuple will be used for constructing this new tuple. > > > > > > > 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. 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. -- With Regards, Amit Kapila.
