Re: Inherited UPDATE/DELETE vs async execution

Fujita-san,

On Tue, May 11, 2021 at 5:56 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> On Mon, May 10, 2021 at 9:21 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > On Sat, May 8, 2021 at 1:21 AM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> > > I noticed this while working on the
> > > EXPLAIN-ANALYZE-for-async-capable-nodes issue:
> > >
> > > EXPLAIN (VERBOSE, COSTS OFF)
> > > DELETE FROM async_pt;
> > > QUERY PLAN
> > > ----------------------------------------------------------------
> > > Delete on public.async_pt
> > > Foreign Delete on public.async_p1 async_pt_1
> > > Foreign Delete on public.async_p2 async_pt_2
> > > Delete on public.async_p3 async_pt_3
> > > -> Append
> > > -> Async Foreign Delete on public.async_p1 async_pt_1
> > > Remote SQL: DELETE FROM public.base_tbl1
> > > -> Async Foreign Delete on public.async_p2 async_pt_2
> > > Remote SQL: DELETE FROM public.base_tbl2
> > > -> Seq Scan on public.async_p3 async_pt_3
> > > Output: async_pt_3.tableoid, async_pt_3.ctid
> > > (11 rows)
> > >
> > > DELETE FROM async_pt;
> > > server closed the connection unexpectedly
> > > This probably means the server terminated abnormally
> > > before or while processing the request.
> > > connection to server was lost
> > >
> > > The cause for this would be that direct-update plans are mistakenly
> > > treated as async-capable ones, as shown in the EXPLAIN output.
> >
> > I guess that happens because the ForeignScan nodes responsible for
> > scanning or direct-updating/deleting from child foreign tables appear
> > under an Append as of 86dc90056, whereas before they would appear as
> > child plans of a ModifyTable node. IIUC, it's the Append that causes
> > the async_capable flag to be set in those ForeignScan nodes.
>
> That's right.
>
> The inherited update/delete work is great! Thanks for that!

Thanks.

> > > To
> > > fix, I think we should modify postgresPlanDirectModify() so that it
> > > clears the async-capable flag if it is set. Attached is a patch for
> > > that. Maybe I am missing something, though.
> >
> > I see that your patch is to disable asynchronous execution in
> > ForeignScan nodes responsible for direct update/delete, but why not do
> > the same for other ForeignScan nodes too?
>
> I just thought it would be better to execute other ForeignScan nodes
> asynchronously for performance, if they are async-capable.

Okay, so I take it that making these ForeignScan nodes (that only
fetch the data) asynchronous doesn't interfere with update/delete
subsequently being performed over presumably the same connection to
the remote server.

> > Or the other way around --
> > is it because fixing the crash that occurs in the former's case would
> > be a significant undertaking for little gain?
>
> Yeah, I think it would be a good idea to support "Async Foreign
> Delete" in the former's case. And actually, I tried to do so, but I
> didn't, because it seemed to take time.

Ah I see. I guess it makes sense to prevent such cases in v14 as your
patch does, and revisit this in the future.
--
Amit Langote
EDB: http://www.enterprisedb.com