Re: Asynchronous Append on postgres_fdw nodes.

On Tue, Apr 6, 2021 at 12:01 PM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
> At Mon, 5 Apr 2021 17:15:47 +0900, Etsuro Fujita <etsuro(dot)fujita(at)gmail(dot)com> wrote in
> > On Fri, Apr 2, 2021 at 12:09 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > > The buildfarm points out that this fails under valgrind.
> > > I easily reproduced it here:
> > >
> > > ==00:00:03:42.115 3410499== Syscall param epoll_wait(events) points to unaddressable byte(s)
> > > ==00:00:03:42.115 3410499== at 0x58E926B: epoll_wait (epoll_wait.c:30)
> > > ==00:00:03:42.115 3410499== by 0x7FC903: WaitEventSetWaitBlock (latch.c:1452)
> ...
> > The reason for this would be that epoll_wait() is called with
> > maxevents exceeding the size of the input event array in the test
> > case. To fix, I adjusted the parameters to call the caller function
>
> # s/input/output/ event array? (occurrred_events)

Sorry, my explanation was not enough. I think I was in a hurry. I
mean by "the input event array" the epoll_event array given to
epoll_wait() (i.e., the epoll_ret_events array).

> # I couldn't reproduce it, so sorry in advance if the following
> # discussion is totally bogus..

I produced this failure by running the following simple query in async
mode on a valgrind-enabled build:

select * from ft1 union all select * from ft2

where ft1 and ft2 are postgres_fdw foreign tables. For this query, we
would call WaitEventSetWait() with nevents=16 in
ExecAppendAsyncEventWait() as EVENT_BUFFER_SIZE=16, and then
epoll_wait() with maxevents=16 in WaitEventSetWaitBlock(); but
maxevents would exceed the input event array as the array size is
three. I think this inconsitency would cause the valgrind failure.
I'm not 100% sure about that, but the patch fixing this inconsistency
I posted fixed the failure in my environment.

> I have nothing to say if it actually corrects the error, but the only
> restriction of maxevents is that it must be positive, and in any case
> epoll_wait returns no more than set->nevents events. So I'm a bit
> wondering if that's the reason. In the first place I'm wondering if
> valgrind is aware of that depth..

Yeah, the failure might actually be harmless, but anyway, we should
make the buildfarm green. Also, we should improve the code to avoid
the consistency mentioned above, so I'll apply the patch.

Thanks for the comments!

Best regards,
Etsuro Fujita