Hi, On 2023-10-12 11:44:09 -0400, Tom Lane wrote: > Andres Freund <and...@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
