Re: Performance degradation on concurrent COPY into a single relation in PG16.
Andres Freund <andres@anarazel.de>
From: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Heikki Linnakangas <hlinnaka@iki.fi>, Alvaro Herrera <alvherre@alvh.no-ip.org>, Masahiko Sawada <sawada.mshk@gmail.com>, David Rowley <dgrowleyml@gmail.com>, Dean Rasheed <dean.a.rasheed@gmail.com>, Jakub Wartak <jakub.wartak@enterprisedb.com>, Andrew Dunstan <andrew@dunslane.net>, Peter Eisentraut <peter@eisentraut.org>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2023-10-12T16:24:19Z
Lists: pgsql-hackers
Hi, On 2023-10-12 11:44:09 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > >> On 2023-09-25 15:42:26 -0400, Tom Lane wrote: > >>> I just did a git bisect run to discover when the failure documented > >>> in bug #18130 [1] started. And the answer is commit 82a4edabd. > > > Uh, huh. The problem is that COPY uses a single BulkInsertState for multiple > > partitions. Which to me seems to run counter to the following comment: > > * The caller can also provide a BulkInsertState object to optimize many > > * insertions into the same relation. This keeps a pin on the current > > * insertion target page (to save pin/unpin cycles) and also passes a > > * BULKWRITE buffer selection strategy object to the buffer manager. > > * Passing NULL for bistate selects the default behavior. > > > The reason this doesn't cause straight up corruption due to reusing a pin from > > another relation is that b1ecb9b3fcfb added ReleaseBulkInsertStatePin() and a > > call to it. But I didn't make ReleaseBulkInsertStatePin() reset the bulk > > insertion state, which is what leads to the errors from the bug report. > > > Resetting the relevant BulkInsertState fields fixes the problem. But I'm not > > sure that's the right fix. ISTM that independent of whether we fix this via > > ReleaseBulkInsertStatePin() resetting the fields or via not reusing > > BulkInsertState, we should add assertions defending against future issues like > > this (e.g. by adding a relation field to BulkInsertState in cassert builds, > > and asserting that the relation is the same as in prior calls unless > > ReleaseBulkInsertStatePin() has been called). > > Ping? We really ought to have a fix for this committed in time for > 16.1. I kind of had hoped somebody would comment on the approach. Given that nobody has, I'll push the minimal fix of resetting the state in ReleaseBulkInsertStatePin(), even though I think architecturally that's not great. Greetings, Andres Freund
Commits
-
Avoid edge case in pg_visibility test with small shared_buffers
- 408209d6a9ae 16.3 landed
- 3a4837fc809a 17.0 landed
-
Fix bulk table extension when copying into multiple partitions
- 0002feb82096 16.1 landed
- 22655aa23132 17.0 landed
-
hio: Take number of prior relation extensions into account
- 82a4edabd272 17.0 landed
- d37ab378b6e7 16.0 landed
-
Fix performance regression in pg_strtointNN_safe functions
- 4e2e75cd29eb 16.0 landed
- 3845577cb55e 17.0 landed
-
Fix performance problem with new COPY DEFAULT code
- c1308ce2d922 16.0 landed
- b635ac03e802 17.0 landed
-
hio: Use ExtendBufferedRelBy() to extend tables more efficiently
- 00d1e02be249 16.0 cited
-
Add VACUUM/ANALYZE BUFFER_USAGE_LIMIT option
- 1cbbee033857 16.0 cited