Re: Asynchronous Append on postgres_fdw nodes.

At Fri, 28 May 2021 16:30:29 +0900, Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote in
> On Wed, Mar 31, 2021 at 6:55 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> > On Tue, Mar 30, 2021 at 8:40 PM Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote:
> > > I'm happy with the patch, so I'll commit it if there are no objections.
> >
> > Pushed.
>
> I noticed that rescan of async Appends is broken when
> do_exec_prune=false, leading to incorrect results on normal builds and
> the following failure on assertion-enabled builds:
>
> TRAP: FailedAssertion("node->as_valid_asyncplans == NULL", File:
> "nodeAppend.c", Line: 1126, PID: 76644)
>
> See a test case for this added in the attached. The root cause would
> be that we call classify_matching_subplans() to re-determine
> sync/async subplans when called from the first ExecAppend() after the
> first ReScan, even if do_exec_prune=false, which is incorrect because
> in that case it is assumed to re-use sync/async subplans determined
> during the the first ExecAppend() after Init. The attached fixes this
> issue. (A previous patch also had this issue, so I fixed it, but I
> think I broke this again when simplifying the patch :-(.) I did a bit
> of cleanup, and modified ExecReScanAppend() to initialize an async
> state variable as_nasyncresults to zero, to be sure. I think the
> variable would have been set to zero before we get to that function,
> so I don't think we really need to do so, though.
>
> I will add this to the open items list for v14.

The patch drops some "= NULL" (initial) initializations when
nasyncplans == 0. AFAICS makeNode() fills the returned memory with
zeroes but I'm not sure it is our convention to omit the
intializations.

Otherwise the patch seems to make the code around cleaner.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center