From: Antonin Houska <ah@cybertec.at>
To: pgsql-hackers@postgresql.org
Subject: Re: asynchronous execution
In-reply-to: <20170131.124548.85146517.horiguchi.kyotaro@lab.ntt.co.jp>
References: <20161115.202513.268072050.horiguchi.kyotaro@lab.ntt.co.jp>
 <20161128.192247.05277061.horiguchi.kyotaro@lab.ntt.co.jp>
 <20161221.172358.258649462.horiguchi.kyotaro@lab.ntt.co.jp>
 <20170131.124548.85146517.horiguchi.kyotaro@lab.ntt.co.jp>
Comments: In-reply-to Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>
   message dated "Tue, 31 Jan 2017 12:45:48 +0900."
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Date: Fri, 03 Feb 2017 11:04:08 +0100
Message-ID: <28186.1486116248@localhost>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote:

> I noticed that this patch is conflicting with 665d1fa (Logical
> replication) so I rebased this. Only executor/Makefile
> conflicted.

I was lucky enough to see an infinite loop when using this patch, which I
fixed by this change:

diff --git a/src/backend/executor/execAsync.c b/src/backend/executor/execAs=
ync.c
new file mode 100644
index 588ba18..9b87fbd
*** a/src/backend/executor/execAsync.c
--- b/src/backend/executor/execAsync.c
*************** ExecAsyncEventWait(EState *estate, long
*** 364,369 ****
--- 364,370 ----
=20=20
  		if ((w->events & WL_LATCH_SET) !=3D 0)
  		{
+ 			ResetLatch(MyLatch);
  			process_latch_set =3D true;
  			continue;
  		}

Actually _almost_ fixed because at some point one of the following

Assert(areq->state =3D=3D ASYNC_WAITING);

statements fired. I think it was the immediately following one, but I can
imagine the same to happen in the branch

if (process_latch_set)
...

I think the wants_process_latch field of PendingAsyncRequest is not useful
alone because the process latch can be set for reasons completely unrelated=
 to
the asynchronous processing. If the asynchronous node should use latch to
signal it's readiness, I think an additional flag is needed in the request
which tells ExecAsyncEventWait that the latch was set by the asynchronous
node.

BTW, do we really need the ASYNC_CALLBACK_PENDING state? I can imagine the
async node either to change ASYNC_WAITING directly to ASYNC_COMPLETE, or le=
ave
it ASYNC_WAITING if the data is not ready.


In addition, the following comments are based only on code review, I didn't
verify my understanding experimentally:

* Isn't it possible for AppendState.as_asyncresult to contain multiple
  responses from the same async node? Since the array stores TupleTableSlot
  instead of the actual tuple (so multiple items of as_asyncresult point to
  the same slot), I suspect the slot contents might not be defined when the
  Append node eventually tries to return it to the upper plan.

* For the WaitEvent subsystem to work, I think postgres_fdw should keep a
  separate libpq connection per node, not per user mapping. Currently the
  connections are cached by user mapping, but it's legal to locate multiple
  child postgres_fdw nodes of Append plan on the same remote server. I expe=
ct
  that these "co-located" nodes would currently use the same user mapping a=
nd
  therefore the same connection.

--=20
Antonin Houska
Cybertec Sch=C3=B6nig & Sch=C3=B6nig GmbH
Gr=C3=B6hrm=C3=BChlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at


--=20
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers