Re: COPY enhancements

Hi!

On Fri, Sep 25, 2009 at 7:01 AM, Emmanuel Cecchet <manu(at)asterdata(dot)com> wrote:

> Here is the new version of the patch that applies to CVS HEAD as of this
> morning.

Cool features!

This is my first pass at the error logging portion of this patch. I'm
going to take a break and try to go through the partitioning logic as
well later this afternoon.

caveat: I'm not familiar with most of the code paths that are being
touched by this patch.

Overall:
* I noticed '\see' included in the comments in your code. These should
be removed.
* The regression is failling, as Jeff indicated, and I didn't figure
out why yet either. Hopefully will have a look closer this afternoon.

Comments:
* copy.c: Better formatting, maybe rewording needed for comment
starting on line 1990.
** Maybe say: "Check that errorData->sqlerrcode only logged tuples
that are malformed. This ensures that we let other errors pass
through."
* copy.c: line: 2668 -> need to fix that comment :) (/* Regular code
*/) -- this needs to be more descriptive of what is actually
happening. We fell through the partitioning logic, right, and back to
the default behavior?
* copy.c: line 2881, maybe say instead: "Mark that we have not read a
line yet. This is necessary so that we can perform error logging on
complete lines only."

Formatting:
* copy.c: whitespace is maybe a little off at line: 2004-2009
* NEW FILES: src/backend/utils/misc/errorlogging.c & errorlogging.h need headers

Code:
* copy.c: line 1990 -> cur_lineno_str[11] & related: why is this
conversion necessary? (sorry if that is a dumb question)
* copy.c: line 2660 -> what if we are error logging for copy? Does
this get logged properly?
* errorlogging.c: Noticed the comment at 503 -> 'note that we always
enable wal logging'.. Thinking this through - the reasoning is that
you won't be rolling back the error logging no matter what, right?
* src/include/commands/copy.c and copy.h: struct CopyStateData was
moved from copy.c to copy.h; this made sense to me, just noting. That
move made it a little tricky to find the changes that were made. There
were 10 items added.
** super nit pick: 'readlineOk' uses camel-case instead of underscores
like the rest of the new variables
* errorlogging.c: could move currentCommand pg_stat call in Init
routine into MalformedTupleIs, or maybe move the setting of that
parameter into the Init routine for consistency's sake.

Documentation:
* doc/src/sgml/ref/copy.sgml: line 313: 'failif' needs a space
** Also: The error table log examples have relations shown in a
different order than the actual CREATE TABLE declaration in the code.
* src/test/regress/sql/copyselect.sql: Format of options changed..
added parenthesis. Is this change documented elsewhere? (sorry if I
just missed this in the rest of the thread/patch)

--
http://chesnok.com/daily - me
http://endpoint.com - work