Itagaki Takahiro wrote: > Shigeru HANADA wrote: >> Attached patch would avoid this leak by adding per-copy context to >> CopyState. This would be overkill, and ResetCopyFrom() might be >> reasonable though. > > Good catch. I merged your fix into the attached patch. Review for CF: While there is a post which suggests applying this patch in the middle of a string of fdw patches, it stands alone without depending on any of the other patches. I chose to focus on testing it alone, to isolate just issues with this particular patch. In addition to the base patch, there was a patch-on-patch which was applied to change signature of NextCopyFrom to take fewer params and return HeapTuple. This patch is in context diff format, applies cleanly, compiles without warning, passes all tests in `make check` and `make installcheck-world`. From that and the testing I did, I think that this patch could be committed before any of the others, if desired. A few minor comments on format, though -- some parts of the patch came out much more readable (for me, at least) when I regenerated the diff using the git diff --patience switch. For example, see the end of the CopyStateData structure definition each way. Also, whitespace has little differences from what pgindent uses. Most are pretty minor except for a section where the indentation wasn't changed based on a change in depth of braces, to keep the patch size down. It appears that the point of the patch is to provide a better API for accessing the implementation of COPY, to support other patches. This whole FDW effort is not an area of expertise for me, but the API looks reasonable to me, with a somewhat parallel structure to some of the other APIs used by the executor. From reading the FDW posts, it appears that other patches are successfully using this new API, and the reviewers of the other patches seem happy enough with it, which would tend to indicate that it is on-target. Other hackers seem to want it and we didn't already have it. From my reading of the posts the idea of creating an API at this level was agreed upon by the community. The only two files touched by this patch are copy.h and copy.c. The copy.h changes consisted entirely of new function prototypes and one declaration of a pointer type (CopyState) to a struct defined and used only inside copy.c (CopyStateData). That pointer is the return value from BeginCopyFrom and required as a parameter to other new functions. So the old API is still present exactly as it was, with additional functions added. I tried to read through the code to look for problems, but there are so many structures and techniques involved that I haven't had to deal with yet, that it would take me days to get up to speed enough to desk check this adequately. Since this API is a prerequisite for other patches, and already being used by them, I figured I should do what I could quickly and then worry about how to cover that. Since it doesn't appear to be intended to change any user-visible behavior, I don't see any need for docs or changes to the regression tests. I didn't see any portability problems. It seemed a little light on comments to me, but perhaps that's because of my ignorance of some of the techniques being used -- maybe things are obvious enough to anyone who would be mucking about in copy.c. I spent a few hours doing ad hoc tests of various sorts to try to break things, without success. Among those tests were checks for correct behavior on temporary tables and read only transactions -- separately and in combination. I ran millions of COPY statements inside of a single database transaction (I tried both FROM and TO) without seeing memory usage increase by more than a few kB. No assertion failures. No segfaults. Nothing unusual in the log. I plan to redo these tests with the full fdw patch set unless someone has already covered that ground. So far everything I've done has been with asserts enabled, so I haven't tried to get serious benchmarks, but it seems fast. I will rebuild without asserts and do performance tests before I change the status on the CF page. I'm wondering if it would make more sense to do the benchmarking with just this patch or the full fdw patch set? Both? -Kevin
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
