From: Tomas Vondra <tomas@vondra.me>
To: git@jasonk.me
To: pgsql-bugs@lists.postgresql.org
Subject: Re: FDW INSERT batching can change behavior
Date: Tue, 20 Aug 2024 23:47:30 +0200
Message-ID: <148fcd22-d9d8-4363-896f-e5a5cc233023@vondra.me> (raw)
In-Reply-To: <1ee2d452-45c8-4512-bc4b-773b0ec5d0a0@vondra.me>
References: <20240809030755.jubqv6f6vpxkfkzv@jasonk.me>
<900ad3fe-151e-4876-a399-fa12faa2c653@vondra.me>
<1ee2d452-45c8-4512-bc4b-773b0ec5d0a0@vondra.me>
On 8/14/24 13:31, Tomas Vondra wrote:
> On 8/13/24 20:31, Tomas Vondra wrote:
>> Hi,
>>
>> I took a closer look at this, planning to way to fix this, but I think
>> it's actually a bit worse than reported - both in impact and ways how to
>> fix that.
>>
>> The problem is it's not really specific to DEFAULT values. The exact
>> same issue exists whenever the insert uses the expressions directly.
>> That is, if you do this:
>>
>>
>> insert into grem1 (a) values (counter()), (counter()),
>> (counter()), (counter()),
>> (counter());
>>
>> it will misbehave in exactly the same way as with the default values. Of
>> course, this also means that my original idea to disable batching if the
>> foreign table has (volatile) expression in the DEFAULT value won't fly.
>>
>> This can happen whenever the to-be-inserted rows have any expressions.
>> But those expressions are evaluated *outside* ModifyTable - in the nodes
>> that produce the rows. In the above example it's ValueScan. But it could
>> be any other node. For example:
>>
>> insert into grem1 (a) select counter() from generate_series(1,5);
>>
>> does that in a subquery. But AFAICS it could be any other node.
>>
>> Ideally we'd simply set batch_size=1 for those cases, but at this point
>> I have no idea how to check this from ModifyTable :-(
>>
>> In retrospect the issue is pretty obvious, yet I haven't thought about
>> this while working on the batching. This is embarrassing.
>>
>
> I've been thinking about this a bit more, and I'm not really sure using
> counter() as a default value can ever be "correct". The problem is it's
> inherently broken with concurrency - even with no batching, it'll fail
> if two of these inserts run at the same time. The batching only makes
> that more obvious / easier to hit, but it's not really the root cause.
>
FWIW I've brought this up on pgsql-hackers, just to get more opinions if
this should be considered a bug, and I think the conclusion is from that
thread is "not a bug".
I do agree the behavior change is a bit surprising, but even ignoring
the concurrency issues this INSERT/SELECT is inherently unsafe. There's
no guarantee the INSERT / SELECT will proceed in lockstep row by row,
which is what the counter() function relies on. We'd be well within our
right to e.g. run the SELECT to completion first, stash the results into
a tuplestore (in a Materialize node or something like that), and only
then to actually start processing the INSERT.
While that currently does not happen, it's not unthinkable we might do
something like that in a future reease (e.g. by doing a bit of batching
or vectorization in the executor).
regards
--
Tomas Vondra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tomas@vondra.me, git@jasonk.me, pgsql-bugs@lists.postgresql.org
Subject: Re: FDW INSERT batching can change behavior
In-Reply-To: <148fcd22-d9d8-4363-896f-e5a5cc233023@vondra.me>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox