Re: batch fdw insert bug (Postgres 14)

pá 7. 5. 2021 v 22:43 odesílatel Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>
napsal:

>
> On 5/7/21 2:46 PM, houzj(dot)fnst(at)fujitsu(dot)com wrote:
> >
> >> I am testing new features in Postgres 14, and I found bug
> >> EXPLAIN ANALYZE VERBOSE for insert to FDW table with batch_size 1000
> returns
> >>
> -------------------------------------------------------------------------------------------------------------------------------
> >> Insert on public.vzdalena_tabulka2 (cost=0.00..175.00 rows=0
> width=0) (actual time=30.269..30.270 rows=0 loops=1)
> >> Remote SQL:
> \x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F\x7F
> >> Batch Size: 1000
> >> -> Function Scan on pg_catalog.generate_series g
> (cost=0.00..175.00 rows=10000 width=36) (actual time=0.453..1.988 rows=10
> >> Output: g.i, ('AHOJ'::text || (g.i)::text)
> >> Function Call: generate_series(1, 10000)
> >> Planning Time: 0.075 ms
> >> Execution Time: 31.032 ms
> >> (8 rows)
> >> reproducer
> >
> > I can reproduce the issue and did some basic analysis on it.
> >
> > The "Remote SQL" is built from the following code:
> >
> > ----------------
> > char *sql = strVal(list_nth(fdw_private,
> >
> FdwModifyPrivateUpdateSql));
> >
> > ExplainPropertyText("Remote SQL", sql, es);
> > ---------------
> >
> > It use the query string stored in list fdw_private.
> > However, the "fmstate->query" will also point to the string in
> fdw_private,
> > by postgresBeginForeignModify --> create_foreign_modify -->
> "fmstate->query = query;"
> >
> > And in execute_foreign_modify(), " fmstate->query " will be freed when
> rebuild the query
> > string to do the batch insert like the following:
> >
> > ----------------
> > if (operation == CMD_INSERT && fmstate->num_slots != *numSlots)
> > {
> > ...
> > /* Build INSERT string with numSlots records in its VALUES
> clause. */
> > initStringInfo(&sql);
> > rebuildInsertSql(&sql, fmstate->orig_query, fmstate->values_end,
> > fmstate->p_nums, *numSlots - 1)
> > ** pfree(fmstate->query);
> > fmstate->query = sql.data;
> > ----------------
> >
> > So, it output the freed pointer as "Remote SQL".
> >
> > For the fix.
> > The query string could be rebuilt depending on the numSlots,
> > which query string should be output ?
> > should we just output the original query string like the attached patch ?
> > Or should we output the last one?
> >
>
> Yeah. The problem is we build fdw_private list once (which references
> the SQL string), and during execution we may pfree() it. But then
> EXPLAIN ANALYZE gets the same fdw_private list and tries to use the SQL
> string which we pfreed() already.
>
> I think the simplest fix is simply to pstrdup() the query when building
> fmstate in create_foreign_modify. We've already been doing that for
> orig_query anyway. That seems cleaner than printing the last query we
> build (which would be confusing I think).
>
> I've pushed a fix doing that. We only need that for INSERT queries, and
> we might even restrict that to cases with batching if needed.
>

Great

Thank you

Pavel

>
> regards
>
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>