Alexey Kondratov wrote: > I have rebased my branch and squashed all commits into one, since the > patch is quite small. Everything seems to be working fine now, patch > passes all regression tests.
On a *very* quick look, please use an enum to return from NextCopyFrom rather than 'int'. The chunks that change bool to int are very odd-looking. This would move the comment that explains the value from copy.c to copy.h, obviously. Also, you seem to be using non-ASCII dashes in the descriptions of those values; please don't. But those are really just minor nitpicks while paging through. After looking at the way you're handling errors, it seems that you've chosen to go precisely the way people were saying that was not admissible: use a PG_TRY block, and when things go south, simply log a WARNING and move on. This is unsafe. For example: what happens if the error being raised is out-of-memory? Failing to re-throw means you don't release current transaction memory context. What if the error is that WAL cannot be written because the WAL disk ran out of space? This would just be reported as a warning over and over until the server log disk is also full. What if your insertion fired a trigger that caused an update which got a serializability exception? You cannot just move on, because at that point session state (serializability session state) might be busted until you abort the current transaction. Or maybe I misunderstood the patch completely. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
