RE: Forget close an open relation in ReorderBufferProcessTXN()

On Monday, April 26, 2021 2:05 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, Apr 23, 2021 at 8:03 PM osumi(dot)takamichi(at)fujitsu(dot)com
> <osumi(dot)takamichi(at)fujitsu(dot)com> wrote:
> >
> > On Saturday, April 17, 2021 4:13 PM Amit Kapila
> <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > > > 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,
> >
>
> I think (1) and (2) are related because if we need (2) then the check removed
> by (1) needs to be replaced with another check. So, I am not sure how to
> make this decision.
Yeah, you are right.

On Monday, April 19, 2021 9:23 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Sat, Apr 17, 2021 at 12:01 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
> > On Fri, Apr 16, 2021 at 11:24 PM Andres Freund <andres(at)anarazel(dot)de>
> wrote:
> > >
> > > > I think it is also important to *not* acquire any lock on relation
> > > > otherwise it can lead to some sort of deadlock or infinite wait in
> > > > the decoding process. Consider a case for operations like Truncate
> > > > (or if the user has acquired an exclusive lock on the relation in
> > > > some other way say via Lock command) which acquires an exclusive
> > > > lock on relation, it won't get replicated in synchronous mode
> > > > (when synchronous_standby_name is configured). The truncate
> > > > operation will wait for the transaction to be replicated to the
> > > > subscriber and the decoding process will wait for the Truncate
> operation to finish.
> > >
> > > However, this cannot be really relied upon for catalog tables. An
> > > output function might acquire locks or such. But for those we do not
> > > need to decode contents...
> > >
> >
> > I see that if we define a user_catalog_table (create table t1_cat(c1
> > int) WITH(user_catalog_table = true);), we are able to decode
> > operations like (insert, truncate) on such a table. What do you mean
> > by "But for those we do not need to decode contents"?
> >
>
> I think we are allowed to decode the operations on user catalog tables
> because we are using RelationIsLogicallyLogged() instead of
> RelationIsAccessibleInLogicalDecoding() in ReorderBufferProcessTXN().
> Based on this discussion, I think we should not be allowing decoding of
> operations on user catalog tables, so we should use
> RelationIsAccessibleInLogicalDecoding to skip such ops in
> ReorderBufferProcessTXN(). Am, I missing something?
>
> Can you please clarify?
I don't understand that point, either.

I read the context where the user_catalog_table was introduced - [1].
There, I couldn't find any discussion if we should skip decode operations
on that kind of tables or not. Accordingly, we just did not conclude it, I suppose.

What surprised me a bit is to decode operations of system catalog table are considered like [2]
somehow at the time. I cannot find any concrete description of such use cases in the thread, though.

Anyway, I felt disallowing decoding of operations on user catalog tables
doesn't spoil the feature's purpose. So, I'm OK to do so. What do you think ?

[1] - https://www.postgresql.org/message-id/flat/20130914204913.GA4071%40awork2.anarazel.de

Note that in this discussion, user_catalog_table was renamed from
treat_as_catalog_table in the middle of the thread. Searching it might help you to shorten your time to have a look at it.

[2] - https://www.postgresql.org/message-id/CA%2BTgmobhDCHuckL_86wRDWJ31Gw3Y1HrQ4yUKEn7U1_hTbeVqQ%40mail.gmail.com

Best Regards,
Takamichi Osumi