Re: Asynchronous Append on postgres_fdw nodes.

From: Konstantin Knizhnik <k(dot)knizhnik(at)postgrespro(dot)ru>
To: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, pryzby(at)telsasoft(dot)com
Cc: a(dot)lepikhov(at)postgrespro(dot)ru, etsuro(dot)fujita(at)gmail(dot)com, movead(dot)li(at)highgo(dot)ca, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: Asynchronous Append on postgres_fdw nodes.
Date: 2020-09-22 14:59:45
Message-ID: 39606222-5878-9ce3-7026-a44677947350@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 22.09.2020 16:40, Konstantin Knizhnik wrote:
>
>
> On 22.09.2020 15:52, Konstantin Knizhnik wrote:
>>
>>
>> On 20.08.2020 10:36, Kyotaro Horiguchi wrote:
>>> At Wed, 19 Aug 2020 23:25:36 -0500, Justin Pryzby
>>> <pryzby(at)telsasoft(dot)com> wrote in
>>>> On Thu, Jul 02, 2020 at 11:14:48AM +0900, Kyotaro Horiguchi wrote:
>>>>> As the result of a discussion with Fujita-san off-list, I'm going to
>>>>> hold off development until he decides whether mine or Thomas' is
>>>>> better.
>>>> The latest patch doesn't apply so I set as WoA.
>>>> https://commitfest.postgresql.org/29/2491/
>>> Thanks. This is rebased version.
>>>
>>> At Fri, 14 Aug 2020 13:29:16 +1200, Thomas Munro
>>> <thomas(dot)munro(at)gmail(dot)com> wrote in
>>>> Either way, we definitely need patch 0001.  One comment:
>>>>
>>>> -CreateWaitEventSet(MemoryContext context, int nevents)
>>>> +CreateWaitEventSet(MemoryContext context, ResourceOwner res, int
>>>> nevents)
>>>>
>>>> I wonder if it's better to have it receive ResourceOwner like that, or
>>>> to have it capture CurrentResourceOwner.  I think the latter is more
>>>> common in existing code.
>>> There's no existing WaitEventSets belonging to a resowner. So
>>> unconditionally capturing CurrentResourceOwner doesn't work well. I
>>> could pass a bool instead but that make things more complex.
>>>
>>> Come to think of "complex", ExecAsync stuff in this patch might be
>>> too-much for a short-term solution until executor overhaul, if it
>>> comes shortly. (the patch of mine here as a whole is like that,
>>> though..). The queueing stuff in postgres_fdw is, too.
>>>
>>> regards.
>>>
>>
>>
>> Hi,
>> Looks like current implementation of asynchronous append incorrectly
>> handle LIMIT clause:
>>
>> psql:append.sql:10: ERROR:  another command is already in progress
>> CONTEXT:  remote SQL command: CLOSE c1
>>
>>
>>
> Just FYI: the following patch fixes the problem:
>
> --- a/contrib/postgres_fdw/postgres_fdw.c
> +++ b/contrib/postgres_fdw/postgres_fdw.c
> @@ -1667,6 +1667,11 @@ remove_async_node(ForeignScanState *node)
>
>          if (cur == node)
>          {
> +            PGconn *conn = curstate->s.conn;
> +
> +            while(PQisBusy(conn))
> +                PQclear(PQgetResult(conn));
> +
>              prev_state->waiter = curstate->waiter;
>
>              /* relink to the previous node if the last node was
> removed */
>

Sorry, but it is not the only problem.
If you execute the query above and then in the same backend try to
insert more records, then backend is crashed:

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00007f5dfc59a231 in fetch_received_data (node=0x230c130) at
postgres_fdw.c:3736
3736        Assert(fsstate->s.commonstate->leader == node);
(gdb) p sstate->s.commonstate
No symbol "sstate" in current context.
(gdb) p fsstate->s.commonstate
Cannot access memory at address 0x7f7f7f7f7f7f7f87

Also my patch doesn't solve the problem for small number of records
(100) in the table.
I attach yet another patch which fix both problems.
Please notice that I did not go deep inside code of async append, so I
am not sure that my patch is complete and correct.

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Attachment Content-Type Size
async_append_limit-2.patch text/x-patch 1.5 KB

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2020-09-22 15:35:55 Re: pgindent vs dtrace on macos
Previous Message Michael Banck 2020-09-22 14:30:19 Re: [patch] Fix checksum verification in base backups for zero page headers