Hi Selena,
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.
Should we avoid any doxygen tags in general in the Postgres code?
* The regression is failling, as Jeff indicated, and I didn't figure out why yet either. Hopefully will have a look closer this afternoon.
Let me know if the response I sent to Jeff works for you.
Some of the bad formatting has been left on purpose to minimize the size of the patch otherwise there would have been many tab/white spaces changes due to the indentation in the PG_TRY blocks. I suggest that whoever is going to commit the code runs pg_indent or I can fix the formatting once the reviewing is done.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."
Ok.
* 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?
Yes.
* 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."
Ok.
I was not sure what is auto-generated by SVN commit scripts. I'd be happy to add headers. Is there a specification somewhere or should I just copy/paste from another file?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
This conversion is necessary if you log in the error table the index of the row that is causing the error (this is what is logged by default in ERROR_LOGGING_KEY).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?
Yes, this is in a try/catch block that performs error logging.
I think that this was the original idea but we should probably rollback the error logging if the command has been rolled back. It might be more consistent to use the same hi_options as the copy command. Any idea what would be best?* 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?
Yes, patch can be a little bit lost when you move a big data structure like this one.* 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
Yes, queryDesc also has camel-case. I will fix readlineOk.
The advantage of calling pg_stat in InitializeErrorLogging is that it is evaluated only once for the entire copy command (and it's not going to change during the execution of the command). I am not sure to understand what your second suggestion is since currentCommand is set and initialized in Init.* 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
ok
The order of the columns does not really matter but for consistency sake we can put the same order.** Also: The error table log examples have relations shown in a different order than the actual CREATE TABLE declaration in the code.
Yes this has already been committed by Tom. The new format of options has been introduced just before this patch.* 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)
I am attaching a revised version of the patch. Emmanuel -- Emmanuel Cecchet Aster Data Systems Web: http://www.asterdata.com
aster-copy-newsyntax-patch-8.5v6context.txt.gz
Description: GNU Zip compressed data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers