On Fri, 23 Apr 2021 at 22:33, osumi.takami...@fujitsu.com <osumi.takami...@fujitsu.com> wrote: > On Saturday, April 17, 2021 4:13 PM Amit Kapila <amit.kapil...@gmail.com> > wrote: >> On Sat, Apr 17, 2021 at 12:05 PM Japin Li <japi...@hotmail.com> wrote: >> > >> > On Sat, 17 Apr 2021 at 14:09, Amit Kapila <amit.kapil...@gmail.com> >> wrote: >> > > On Thu, Apr 15, 2021 at 4:53 PM Amit Kapila <amit.kapil...@gmail.com> >> wrote: >> > >> >> > >> On Thu, Apr 15, 2021 at 4:00 PM Japin Li <japi...@hotmail.com> wrote: >> > >> > >> > >> > The RelationIdGetRelation() comment says: >> > >> > >> > >> > > Caller should eventually decrement count. (Usually, that >> > >> > > happens by calling RelationClose().) >> > >> > >> > >> > However, it doesn't do it in ReorderBufferProcessTXN(). >> > >> > I think we should close it, here is a patch that fixes it. Thoughts? >> > >> > >> > >> >> > >> +1. Your fix looks correct to me but can we test it in some way? >> > >> >> > > >> > > I have tried to find a test but not able to find one. I have tried >> > > with a foreign table but we don't log truncate for it, see >> > > ExecuteTruncate. It has a check that it will log for relids where >> > > RelationIsLogicallyLogged. If that is the case, it is not clear to >> > > me how we can ever hit this condition? Have you tried to find the test? >> > >> > I also don't find a test for this. It is introduced in 5dfd1e5a6696, >> > wrote by Simon Riggs, Marco Nenciarini and Peter Eisentraut. Maybe >> > they can explain when we can enter this condition? >> >> My guess is that this has been copied from the code a few lines above to >> handle insert/update/delete where it is required to handle some DDL ops like >> Alter Table but I think we don't need it here (for Truncate op). If that >> understanding turns out to be true then we should either have an Assert for >> this or an elog message. > In this thread, we are discussing 3 topics below... > > (1) necessity of the check for REORDER_BUFFER_CHANGE_TRUNCATE in > ReorderBufferProcessTXN() > (2) discussion of whether we disallow decoding of operations on user catalog > tables or not > (3) memory leak of maybe_send_schema() (patch already provided) > > Let's address those one by one. > In terms of (1), which was close to the motivation of this thread, > first of all, I traced the truncate processing > and I think the check is done by truncate command side as well. > I preferred Assert rather than never called elog, > but it's OK to choose elog if someone has strong opinion on it. > Attached the patch for this. >
+1, make check-world passed. -- Regrads, Japin Li. ChengDu WenWu Information Technology Co.,Ltd.
