On Tue, Apr 20, 2021 at 4:22 PM Michael Paquier <mich...@paquier.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 [1] https://www.postgresql.org/message-id/CA%2BHiwqF%2Bq3MyGqLvGdC%2BJk5Xx%3DJpwpR-m5moXN%2Baf-LC-RMvdw%40mail.gmail.com
