Re: Forget close an open relation in ReorderBufferProcessTXN()

On Sat, Apr 17, 2021 at 12:05 PM Japin Li <japinli(at)hotmail(dot)com> wrote:
>
> On Sat, 17 Apr 2021 at 14:09, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > On Thu, Apr 15, 2021 at 4:53 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >>
> >> On Thu, Apr 15, 2021 at 4:00 PM Japin Li <japinli(at)hotmail(dot)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.

--
With Regards,
Amit Kapila.