Re: Forget close an open relation in ReorderBufferProcessTXN()

On Wed, May 19, 2021 at 8:28 AM osumi.takamichi@fujitsu.com
<osumi.takamichi@fujitsu.com> wrote:
>
> On Wednesday, May 19, 2021 11:33 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > On Wed, May 19, 2021 at 7:59 AM Amit Kapila <amit.kapila16@gmail.com>
> > wrote:
> > >
> > > On Tue, May 18, 2021 at 5:29 PM Amit Kapila <amit.kapila16@gmail.com>
> > wrote:
> > > >
> > > > On Tue, May 18, 2021 at 1:29 PM osumi.takamichi@fujitsu.com
> > > > <osumi.takamichi@fujitsu.com> wrote:
> > > > >
> > > > > On Monday, May 17, 2021 6:45 PM Amit Kapila
> > <amit.kapila16@gmail.com> wrote:
> > > > > >
> > > > > > We allow taking locks on system catalogs, so why prohibit
> > > > > > user_catalog_tables? However, I agree that if we want plugins to
> > > > > > acquire the lock on user_catalog_tables then we should either
> > > > > > prohibit decoding of such relations or do something else to avoid
> > deadlock hazards.
> > > > > OK.
> > > > >
> > > > > Although we have not concluded the range of logical decoding of
> > > > > user_catalog_table (like we should exclude TRUNCATE command only
> > > > > or all operations on that type of table), I'm worried that
> > > > > disallowing the logical decoding of user_catalog_table produces
> > > > > the deadlock still. It's because disabling it by itself does not affect the
> > lock taken by TRUNCATE command. What I have in mind is an example
> > below.
> > > > >
> > > > > (1) plugin (e.g. pgoutput) is designed to take a lock on
> > user_catalog_table.
> > > > > (2) logical replication is set up in synchronous mode.
> > > > > (3) TRUNCATE command takes an access exclusive lock on the
> > user_catalog_table.
> > > > > (4) This time, we don't do anything for the TRUNCATE decoding.
> > > > > (5) the plugin tries to take a lock on the truncated table
> > > > >         but, it can't due to the lock by TRUNCATE command.
> > > > >
> > > >
> > > > If you skip decoding of truncate then we won't invoke plugin API so
> > > > step 5 will be skipped.
> > > >
> > >
> > > I think you were right here even if skip step-4, the plugin might take
> > > a lock on user_catalog_table for something else.
> Yes, we can't know the exact place where the user wants to use the feature
> of user_catalog_table. Even if we imagine that the user skips
> the truncate decoding (I imagined continuing and skipping a case in
> REORDER_BUFFER_CHANGE_TRUNCATE of pgoutput),
> it's possible that the user accesses it somewhere else for different purpose with lock.
>
>
> > I am not sure but I
> > > think we should prohibit truncate on user_catalog_tables as we
> > > prohibit truncate on system catalog tables (see below [1]) if we want
> > > plugin to lock them, otherwise, as you said it might lead to deadlock.
> > > For the matter, I think we should once check all other operations
> > > where we can take an exclusive lock on [user]_catalog_table, say
> > > Cluster command, and compare the behavior of same on system catalog
> > > tables.
> > >
> > > [1]
> > > postgres=# truncate pg_class;
> > > ERROR:  permission denied: "pg_class" is a system catalog postgres=#
> > > cluster pg_class;
> > > ERROR:  there is no previously clustered index for table "pg_class"
> > >
> >
> > Please ignore the cluster command as we need to use 'using index' with that
> > command to make it successful. I just want to show the truncate command
> > behavior for which you have asked the question.
> Thank you so much for clarifying the direction.
> I agree with the changing the TRUNCATE side.
> I'll make a patch based on this.
>

Isn't it a better idea to start a new thread where you can summarize
whatever we have discussed here about user_catalog_tables? We might
get the opinion from others about the behavior change you are
proposing.

-- 
With Regards,
Amit Kapila.