From: | Amit Langote <amitlangote09(at)gmail(dot)com> |
---|---|
To: | Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, "houzj(dot)fnst(at)fujitsu(dot)com" <houzj(dot)fnst(at)fujitsu(dot)com>, "tanghy(dot)fnst(at)fujitsu(dot)com" <tanghy(dot)fnst(at)fujitsu(dot)com>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "shiy(dot)fnst(at)fujitsu(dot)com" <shiy(dot)fnst(at)fujitsu(dot)com> |
Subject: | Re: Table refer leak in logical replication |
Date: | 2021-04-20 08:51:58 |
Message-ID: | CA+HiwqG0tHSHr98SArku5bHgxEQSPKXGhRyoutY0aXRGtPovgA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Apr 20, 2021 at 4:22 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> On Tue, Apr 20, 2021 at 02:48:35PM +0900, Amit Langote wrote:
> > Manipulating the contents of es_opened_result_relations directly in
> > worker.c is admittedly a "hack", which I am reluctant to have other
> > places participating in. As originally designed, that list is to
> > speed up ExecCloseResultRelations(), not as a place to access result
> > relations from. The result relations targeted over the course of
> > execution of a query (update/delete) or a (possibly multi-tuple in the
> > future) replication apply operation will not be guaranteed to be added
> > to the list in any particular order, so assuming where a result
> > relation of interest can be found in the list is bound to be unstable.
>
> I really hope that this code gets heavily reorganized before
> considering more features or more manipulations of dependencies within
> the relations used for any apply operations. From what I can
> understand, I think that it would be really nicer and less bug prone
> to have a logic like COPY FROM, where we'd rely on a set of
> ExecInitResultRelation() with one final ExecCloseResultRelations(),
> and as bonus it should be possible to not have to do any business with
> ExecOpenIndices() or ExecCloseIndices() as part of worker.c.
As pointed out by Amit K, a problem with using
ExecInitResultRelation() in both copyfrom.c and worker.c is that it
effectively ignores the Relation pointer that's already been acquired
by other parts of the code. Upthread [1], I proposed that we add a
Relation pointer argument to ExecInitResultRelation() so that the
callers that are not interested in setting up es_range_table, but only
es_result_relations, can do so.
BTW, I tend to agree that ExecCloseIndices() is better only done in
ExecCloseResultRelations(), but...
> Anyway,
> I also understand that we do with what we have now in this code, so I
> am fine to live with this patch as of 14.
...IIUC, Amit K prefers starting another thread for other improvements
on top of 1375422c.
--
Amit Langote
EDB: http://www.enterprisedb.com
From | Date | Subject | |
---|---|---|---|
Next Message | tsunakawa.takay@fujitsu.com | 2021-04-20 08:52:46 | [bug?] Missed parallel safety checks, and wrong parallel safety |
Previous Message | Aleksander Alekseev | 2021-04-20 08:35:13 | Re: An omission of automatic completion in tab-complete.c |