From: | "osumi(dot)takamichi(at)fujitsu(dot)com" <osumi(dot)takamichi(at)fujitsu(dot)com> |
---|---|
To: | 'Amit Kapila' <amit(dot)kapila16(at)gmail(dot)com>, Japin Li <japinli(at)hotmail(dot)com> |
Cc: | "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Marco Nenciarini <marco(dot)nenciarini(at)2ndquadrant(dot)it> |
Subject: | RE: Forget close an open relation in ReorderBufferProcessTXN() |
Date: | 2021-04-23 14:33:53 |
Message-ID: | OSBPR01MB4888FCC8BE49FDFDA212F436ED459@OSBPR01MB4888.jpnprd01.prod.outlook.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Saturday, April 17, 2021 4:13 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> 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.
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.
Best Regards,
Takamichi Osumi
Attachment | Content-Type | Size |
---|---|---|
replace_check_of_ReorderBufferProcessTXN_v01.patch | application/octet-stream | 789 bytes |
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2021-04-23 15:20:31 | Re: A test for replay of regression tests |
Previous Message | Tom Lane | 2021-04-23 13:55:09 | Re: How to test Postgres for any unaligned memory accesses? |