Re: Inherited UPDATE/DELETE vs async execution

Amit-san,

On Mon, May 10, 2021 at 9:21 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Sat, May 8, 2021 at 1:21 AM Etsuro Fujita <etsuro.fujita@gmail.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!

> >  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.

> 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.  I might be missing something,
though.

Thanks!

Best regards,
Etsuro Fujita