public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v3 5/5] Do COPY FROM encoding conversion/verification in larger chunks.
24+ messages / 8 participants
[nested] [flat]
* [PATCH v3 5/5] Do COPY FROM encoding conversion/verification in larger chunks.
@ 2021-01-28 16:42 Heikki Linnakangas <[email protected]>
0 siblings, 0 replies; 24+ messages in thread
From: Heikki Linnakangas @ 2021-01-28 16:42 UTC (permalink / raw)
NOTE: This changes behavior in one corner-case: if client and server
encodings are the same single-byte encoding (e.g. latin1), previously the
input would not be checked for zero bytes ('\0'). Any fields containing
zero bytes would be truncated at the zero. But if encoding conversion was
needed, the conversion routine would throw an error on the zero. After
this commit, the input is always checked for zeros.
---
src/backend/commands/copyfrom.c | 58 +--
src/backend/commands/copyfromparse.c | 473 ++++++++++++++++------
src/include/commands/copyfrom_internal.h | 53 +--
src/test/regress/expected/copycorners.out | 202 +++++++++
src/test/regress/sql/copycorners.sql | 90 ++++
5 files changed, 715 insertions(+), 161 deletions(-)
create mode 100644 src/test/regress/expected/copycorners.out
create mode 100644 src/test/regress/sql/copycorners.sql
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index c39cc736ed2..3f787b885ae 100644
--- a/src/backend/commands/copyfrom.c
+++ b/src/backend/commands/copyfrom.c
@@ -3,6 +3,12 @@
* copyfrom.c
* COPY <table> FROM file/program/client
*
+ * This file contains routines needed to efficiently load tuples into a
+ * table. That includes looking up the correct partition, firing triggers,
+ * calling the table AM function to insert the data, and updating indexes.
+ * Reading data from the input file or client and parsing it into Datums
+ * is handled in copyfromparse.c.
+ *
* Portions Copyright (c) 1996-2021, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
@@ -23,6 +29,7 @@
#include "access/tableam.h"
#include "access/xact.h"
#include "access/xlog.h"
+#include "catalog/namespace.h"
#include "commands/copy.h"
#include "commands/copyfrom_internal.h"
#include "commands/progress.h"
@@ -149,15 +156,9 @@ CopyFromErrorCallback(void *arg)
/*
* Error is relevant to a particular line.
*
- * If line_buf still contains the correct line, and it's already
- * transcoded, print it. If it's still in a foreign encoding, it's
- * quite likely that the error is precisely a failure to do
- * encoding conversion (ie, bad data). We dare not try to convert
- * it, and at present there's no way to regurgitate it without
- * conversion. So we have to punt and just report the line number.
+ * If line_buf still contains the correct line, print it.
*/
- if (cstate->line_buf_valid &&
- (cstate->line_buf_converted || !cstate->need_transcoding))
+ if (cstate->line_buf_valid)
{
char *lineval;
@@ -1305,15 +1306,20 @@ BeginCopyFrom(ParseState *pstate,
cstate->file_encoding = cstate->opts.file_encoding;
/*
- * Set up encoding conversion info. Even if the file and server encodings
- * are the same, we must apply pg_any_to_server() to validate data in
- * multibyte encodings.
+ * Look up encoding conversion function.
*/
- cstate->need_transcoding =
- (cstate->file_encoding != GetDatabaseEncoding() ||
- pg_database_encoding_max_length() > 1);
- /* See Multibyte encoding comment above */
- cstate->encoding_embeds_ascii = PG_ENCODING_IS_CLIENT_ONLY(cstate->file_encoding);
+ if (cstate->file_encoding == GetDatabaseEncoding() ||
+ cstate->file_encoding == PG_SQL_ASCII ||
+ GetDatabaseEncoding() == PG_SQL_ASCII)
+ {
+ cstate->need_transcoding = false;
+ }
+ else
+ {
+ cstate->need_transcoding = true;
+ cstate->conversion_proc = FindDefaultConversionProc(cstate->file_encoding,
+ GetDatabaseEncoding());
+ }
cstate->copy_src = COPY_FILE; /* default */
@@ -1332,17 +1338,23 @@ BeginCopyFrom(ParseState *pstate,
cstate->cur_attval = NULL;
/*
- * Set up variables to avoid per-attribute overhead. attribute_buf and
- * raw_buf are used in both text and binary modes, but we use line_buf
- * only in text mode.
+ * Allocate buffers for the input pipeline.
+ *
+ * attribute_buf and input_buf are used in both text and binary modes, but
+ * we use line_buf only in text mode.
*/
initStringInfo(&cstate->attribute_buf);
- cstate->raw_buf = (char *) palloc(RAW_BUF_SIZE + 1);
- cstate->raw_buf_index = cstate->raw_buf_len = 0;
+
if (!cstate->opts.binary)
- {
initStringInfo(&cstate->line_buf);
- cstate->line_buf_converted = false;
+
+ cstate->input_buf = (char *) palloc(INPUT_BUF_SIZE + 1);
+ cstate->input_buf_index = cstate->input_buf_len = 0;
+
+ if (!cstate->opts.binary && cstate->need_transcoding)
+ {
+ cstate->raw_buf = palloc(RAW_BUF_SIZE);
+ cstate->raw_buf_index = cstate->raw_buf_len = 0;
}
/* Assign range table, we'll need it in CopyFrom. */
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index 4c74067f849..03d1c621792 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -3,6 +3,48 @@
* copyfromparse.c
* Parse CSV/text/binary format for COPY FROM.
*
+ * This file contains routines to parse the text, CSV and binary input
+ * formats. The main entry point is NextCopyFrom(), which parses the
+ * next input line and returns it as Datums.
+ *
+ * In text/CSV mode, the parsing happens in multiple stages:
+ *
+ * [data source] --> raw_buf --> input_buf --> line_buf --> attribute_buf
+ * 1. 2. 3. 4.
+ *
+ * 1. CopyLoadRawBuf() reads raw data from the input file or client, and
+ * places it into 'raw_buf'.
+ *
+ * 2. CopyLoadInputBufText() calls the encoding conversion function to
+ * convert the data in 'raw_buf' from client to server encoding, placing the
+ * converted result in 'input_buf'.
+ *
+ * 3. CopyReadLine() parses the data in 'input_buf', one line at a time.
+ * It is responsible for finding the next newline marker, taking quote and
+ * escape characters into account according to the COPY options. The line
+ * is copied into 'line_buf', with quotes and escape characters still intact.
+ *
+ * 4. CopyReadAttributesText/CSV() function takes the input line from
+ * 'line_buf', and splits it into fields, unescaping the data as required.
+ * The fields are stored in 'attribute_buf', and 'raw_fields' array holds
+ * pointers to each field.
+ *
+ * If encoding conversion is not required, a shortcut is taken in step 2
+ * to avoid copying the raw data unnecessarily. The 'input_buf' pointer is
+ * set to point directly to 'raw_buf', so that CopyLoadRawBuf() loads the
+ * raw data directly into 'input_buf'. CopyLoadInputBufText() then merely
+ * validates that the data is valid in the current encoding.
+ *
+ * In binary mode, the pipeline is much simpler. Input is loaded directly
+ * into 'input_buf', and encoding conversion is done in the datatype-specific
+ * receive functions, if required. 'line_buf' is not used, but
+ * 'attribute_buf' is used as a temporary buffer to hold one attribute's data
+ * when it's passed the receive function.
+ *
+ * input_buf is always 64 kB in size. 'raw_buf' is also 64 kB, if encoding
+ * conversion is required. 'line_buf' and 'attribute_buf' are expanded on
+ * demand, to hold the longest line encountered so far.
+ *
* Portions Copyright (c) 1996-2021, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
@@ -35,7 +77,7 @@
#define OCTVALUE(c) ((c) - '0')
/*
- * These macros centralize code used to process line_buf and raw_buf buffers.
+ * These macros centralize code used to process line_buf and input_buf buffers.
* They are macros because they often do continue/break control and to avoid
* function call overhead in tight COPY loops.
*
@@ -53,9 +95,9 @@
#define IF_NEED_REFILL_AND_NOT_EOF_CONTINUE(extralen) \
if (1) \
{ \
- if (raw_buf_ptr + (extralen) >= copy_buf_len && !hit_eof) \
+ if (input_buf_ptr + (extralen) >= copy_buf_len && !hit_eof) \
{ \
- raw_buf_ptr = prev_raw_ptr; /* undo fetch */ \
+ input_buf_ptr = prev_raw_ptr; /* undo fetch */ \
need_data = true; \
continue; \
} \
@@ -65,10 +107,10 @@ if (1) \
#define IF_NEED_REFILL_AND_EOF_BREAK(extralen) \
if (1) \
{ \
- if (raw_buf_ptr + (extralen) >= copy_buf_len && hit_eof) \
+ if (input_buf_ptr + (extralen) >= copy_buf_len && hit_eof) \
{ \
if (extralen) \
- raw_buf_ptr = copy_buf_len; /* consume the partial character */ \
+ input_buf_ptr = copy_buf_len; /* consume the partial character */ \
/* backslash just before EOF, treat as data char */ \
result = true; \
break; \
@@ -77,17 +119,17 @@ if (1) \
/*
* Transfer any approved data to line_buf; must do this to be sure
- * there is some room in raw_buf.
+ * there is some room in input_buf.
*/
#define REFILL_LINEBUF \
if (1) \
{ \
- if (raw_buf_ptr > cstate->raw_buf_index) \
+ if (input_buf_ptr > cstate->input_buf_index) \
{ \
appendBinaryStringInfo(&cstate->line_buf, \
- cstate->raw_buf + cstate->raw_buf_index, \
- raw_buf_ptr - cstate->raw_buf_index); \
- cstate->raw_buf_index = raw_buf_ptr; \
+ cstate->input_buf + cstate->input_buf_index, \
+ input_buf_ptr - cstate->input_buf_index); \
+ cstate->input_buf_index = input_buf_ptr; \
} \
} else ((void) 0)
@@ -95,7 +137,7 @@ if (1) \
#define NO_END_OF_COPY_GOTO \
if (1) \
{ \
- raw_buf_ptr = prev_raw_ptr + 1; \
+ input_buf_ptr = prev_raw_ptr + 1; \
goto not_end_of_copy; \
} else ((void) 0)
@@ -118,7 +160,8 @@ static int CopyGetData(CopyFromState cstate, void *databuf,
int minread, int maxread);
static inline bool CopyGetInt32(CopyFromState cstate, int32 *val);
static inline bool CopyGetInt16(CopyFromState cstate, int16 *val);
-static bool CopyLoadRawBuf(CopyFromState cstate);
+static bool CopyLoadInputBufText(CopyFromState cstate);
+static bool CopyLoadInputBufBinary(CopyFromState cstate);
static int CopyReadBinaryData(CopyFromState cstate, char *dest, int nbytes);
void
@@ -359,42 +402,286 @@ CopyGetInt16(CopyFromState cstate, int16 *val)
return true;
}
+/*
+ * Perform encoding conversion on data in 'raw_buf', writing the converted
+ * data into 'input_buf'.
+ *
+ * On entry, there must be some data to convert in 'raw_buf'.
+ */
+static void
+CopyConvertBuf(CopyFromState cstate)
+{
+ int convertedbytes;
+ unsigned char *src;
+ int srclen;
+ unsigned char *dst;
+ int dstlen;
+
+ Assert(cstate->raw_buf_len > 0);
+ /*
+ * we assume that the caller has moved any remaining data in the
+ * buffer to the beginning.
+ */
+ Assert(cstate->input_buf_index == 0);
+
+ src = (unsigned char *) cstate->raw_buf + cstate->raw_buf_index;
+ srclen = cstate->raw_buf_len - cstate->raw_buf_index;
+ dst = (unsigned char *) cstate->input_buf + cstate->input_buf_len;
+ dstlen = INPUT_BUF_SIZE - cstate->input_buf_len + 1;
+
+ /*
+ * Do the conversion. This might stop short, if there is an invalid byte
+ * sequence in the input. We'll convert as much as we can in that case.
+ *
+ * Note: Even if we hit an invalid byte sequence, we don't report the error
+ * until all the valid bytes have been consumed. The input might contain
+ * an end-of-input marker (\.), and we don't want to report an error if
+ * the invalid byte sequence is after the end-of-input marker. We might
+ * unnecessarily convert some data after the end-of-input marker as long
+ * as it's valid for the encoding, but that's harmless.
+ */
+ convertedbytes = pg_do_encoding_conversion_buf(cstate->conversion_proc,
+ cstate->file_encoding,
+ GetDatabaseEncoding(),
+ src, srclen,
+ dst, dstlen,
+ true);
+ if (convertedbytes == 0)
+ {
+ /*
+ * No more valid input in the buffer, and we have hit an invalid or
+ * untranslatable byte sequence. Call the conversion function again,
+ * with noError=false, to let it throw an appropriate error.
+ */
+ (void) pg_do_encoding_conversion_buf(cstate->conversion_proc,
+ cstate->file_encoding,
+ GetDatabaseEncoding(),
+ src, srclen,
+ dst, dstlen,
+ false);
+ /*
+ * Should not get here, because if the input contained invalid data on the
+ * first call, the second pg_do_encoding_conversion_buf with noError = false
+ * should've reported an error. But just in case the conversion function
+ * messsed up.
+ */
+ elog(ERROR, "encoding conversion failed without error");
+ }
+ cstate->raw_buf_index += convertedbytes;
+ cstate->input_buf_len += strlen((char *) dst);
+}
/*
- * CopyLoadRawBuf loads some more data into raw_buf
+ * Load more data from data source to raw_buf.
*
- * Returns true if able to obtain at least one more byte, else false.
+ * Returns true if at least one more byte was loaded, false means EOF was reached.
*
* If RAW_BUF_BYTES(cstate) > 0, the unprocessed bytes are moved to the start
- * of the buffer and then we load more data after that. This case occurs only
- * when a multibyte character crosses a bufferload boundary.
+ * of the buffer and then we load more data after that.
*/
static bool
CopyLoadRawBuf(CopyFromState cstate)
{
- int nbytes = RAW_BUF_BYTES(cstate);
+ int nbytes;
int inbytes;
+ /*
+ * If encoding conversion is not required, raw_buf and input_buf point
+ * to the same buffer. Their len/index should agree, too, otherwise
+ * we are confused.
+ */
+ if (cstate->raw_buf == cstate->input_buf)
+ {
+ Assert(!cstate->need_transcoding);
+ Assert(cstate->raw_buf_index == cstate->input_buf_index);
+ Assert(cstate->input_buf_len <= cstate->raw_buf_len);
+ }
+
/* Copy down the unprocessed data if any. */
- if (nbytes > 0)
+ nbytes = RAW_BUF_BYTES(cstate);
+ if (nbytes > 0 && cstate->raw_buf_index > 0)
memmove(cstate->raw_buf, cstate->raw_buf + cstate->raw_buf_index,
nbytes);
+ cstate->raw_buf_len -= cstate->raw_buf_index;
+ cstate->raw_buf_index = 0;
+
+ /*
+ * If raw_buf and input_buf are in fact the same buffer, adjust the
+ * input_buf variables, too.
+ */
+ if (cstate->raw_buf == cstate->input_buf)
+ {
+ cstate->input_buf_len -= cstate->input_buf_index;
+ cstate->input_buf_index = 0;
+ }
+
+ /* Load more data */
+ inbytes = CopyGetData(cstate, cstate->raw_buf + cstate->raw_buf_len,
+ 1, RAW_BUF_SIZE - cstate->raw_buf_len);
+ cstate->raw_buf_len += inbytes;
+
+ cstate->bytes_processed += inbytes;
+ pgstat_progress_update_param(PROGRESS_COPY_BYTES_PROCESSED, cstate->bytes_processed);
+
+ return (inbytes > 0);
+}
+
+/*
+ * CopyLoadInputBuf loads some more data into input_buf
+ *
+ * Returns true if able to obtain at least one more byte, else false.
+ *
+ * If INPUT_BUF_BYTES(cstate) > 0, the unprocessed bytes are moved to the start
+ * of the buffer and then we load more data after that.
+ */
+static bool
+CopyLoadInputBufText(CopyFromState cstate)
+{
+ int nbytes;
+
+ if (!cstate->need_transcoding)
+ {
+ /*
+ * If the file and server encoding are the same, no encoding conversion
+ * is required, and we can load the input data directly into 'input_buf'.
+ * However, we still need to verify that the input is valid for the encoding.
+ *
+ * FIXME: for single-byte encoding, the verification only needs to check
+ * that the input doesn't contain any zero bytes. Could we skip that
+ * altogether?
+ *
+ * On entry, input_buf_len indicates how many bytes in the buffer have
+ * already been validated. raw_buf_len can be larger, if there was an
+ * incomplete multi-byte character at the bufferload boundary, or if the
+ * input contains an invalid character.
+ */
+ for (;;)
+ {
+ int verified_bytes = INPUT_BUF_BYTES(cstate);
+ int unverified_bytes = cstate->raw_buf_len - cstate->input_buf_len;
+ int nvalidated;
+
+ /* Load more bytes to the buffer */
+ cstate->raw_buf_index = cstate->input_buf_index;
+ cstate->raw_buf = cstate->input_buf;
+ if (!CopyLoadRawBuf(cstate))
+ {
+ /*
+ * EOF reached. If we have any unverified bytes left, it means
+ * that there was an incomplete multi-byte character at the end.
+ */
+ if (unverified_bytes > 0)
+ report_invalid_encoding(cstate->file_encoding,
+ cstate->raw_buf + verified_bytes,
+ unverified_bytes);
- inbytes = CopyGetData(cstate, cstate->raw_buf + nbytes,
- 1, RAW_BUF_SIZE - nbytes);
+ /* truly hit EOF */
+ return false;
+ }
+ Assert(cstate->raw_buf_index == 0);
+ Assert(cstate->input_buf_index == 0);
+ unverified_bytes = cstate->raw_buf_len - verified_bytes;
+ Assert(unverified_bytes > 0);
+
+ /* Verify the new data (including any unverified bytes from previous round) */
+ nvalidated = pg_encoding_verifymbstr(cstate->file_encoding,
+ cstate->raw_buf + verified_bytes,
+ unverified_bytes);
+ if (nvalidated == 0)
+ {
+ /*
+ * No valid characters in the buffer. It could be because
+ * there are only few bytes in the buffer, and they don't form
+ * any whole characters. In that case, load more data. But if
+ * we have enough data, then it must be an invalid byte
+ * sequence.
+ */
+ if (unverified_bytes < pg_database_encoding_max_length())
+ continue;
+ else
+ report_invalid_encoding(cstate->file_encoding, cstate->raw_buf + verified_bytes,
+ unverified_bytes);
+ }
+ verified_bytes += nvalidated;
+
+ cstate->input_buf_index = 0;
+ cstate->input_buf_len = verified_bytes;
+ return true;
+ }
+ }
+ else
+ {
+ /*
+ * Encoding conversion is needed. First, copy down the unprocessed data
+ * if any.
+ */
+ nbytes = INPUT_BUF_BYTES(cstate);
+ if (nbytes > 0 && cstate->input_buf_index > 0)
+ memmove(cstate->input_buf, cstate->input_buf + cstate->input_buf_index,
+ nbytes);
+ cstate->input_buf_index = 0;
+ cstate->input_buf_len = nbytes;
+
+ for (;;)
+ {
+ /* If we still have a good amount of unconverted data left, convert it. */
+ nbytes = cstate->raw_buf_len - cstate->raw_buf_index;
+ if (nbytes >= MAX_CONVERSION_GROWTH)
+ {
+ CopyConvertBuf(cstate);
+ break;
+ }
+
+ /* Load more bytes to the raw buffer */
+ if (!CopyLoadRawBuf(cstate))
+ {
+ /* Hit EOF. If we have any unconverted bytes left, convert them now */
+ if (cstate->raw_buf_index < cstate->raw_buf_len)
+ {
+ CopyConvertBuf(cstate);
+ break;
+ }
+
+ /*
+ * No more input data, and no unconverted data remain in raw_buf. Report
+ * the EOF to the caller
+ */
+ return false;
+ }
+ }
+ }
+
+ return true;
+}
+
+static bool
+CopyLoadInputBufBinary(CopyFromState cstate)
+{
+ int nbytes = INPUT_BUF_BYTES(cstate);
+ int inbytes;
+
+ /* Copy down the unprocessed data if any. */
+ if (nbytes > 0)
+ memmove(cstate->input_buf, cstate->input_buf + cstate->input_buf_index,
+ nbytes);
+
+ inbytes = CopyGetData(cstate, cstate->input_buf + nbytes,
+ 1, INPUT_BUF_SIZE - nbytes);
nbytes += inbytes;
- cstate->raw_buf[nbytes] = '\0';
- cstate->raw_buf_index = 0;
- cstate->raw_buf_len = nbytes;
+ cstate->input_buf[nbytes] = '\0';
+ cstate->input_buf_index = 0;
+ cstate->input_buf_len = nbytes;
+
cstate->bytes_processed += nbytes;
pgstat_progress_update_param(PROGRESS_COPY_BYTES_PROCESSED, cstate->bytes_processed);
+
return (inbytes > 0);
}
/*
* CopyReadBinaryData
*
- * Reads up to 'nbytes' bytes from cstate->copy_file via cstate->raw_buf
+ * Reads up to 'nbytes' bytes from cstate->copy_file via cstate->input_buf
* and writes them to 'dest'. Returns the number of bytes read (which
* would be less than 'nbytes' only if we reach EOF).
*/
@@ -403,11 +690,11 @@ CopyReadBinaryData(CopyFromState cstate, char *dest, int nbytes)
{
int copied_bytes = 0;
- if (RAW_BUF_BYTES(cstate) >= nbytes)
+ if (INPUT_BUF_BYTES(cstate) >= nbytes)
{
/* Enough bytes are present in the buffer. */
- memcpy(dest, cstate->raw_buf + cstate->raw_buf_index, nbytes);
- cstate->raw_buf_index += nbytes;
+ memcpy(dest, cstate->input_buf + cstate->input_buf_index, nbytes);
+ cstate->input_buf_index += nbytes;
copied_bytes = nbytes;
}
else
@@ -421,16 +708,16 @@ CopyReadBinaryData(CopyFromState cstate, char *dest, int nbytes)
int copy_bytes;
/* Load more data if buffer is empty. */
- if (RAW_BUF_BYTES(cstate) == 0)
+ if (INPUT_BUF_BYTES(cstate) == 0)
{
- if (!CopyLoadRawBuf(cstate))
+ if (!CopyLoadInputBufBinary(cstate))
break; /* EOF */
}
/* Transfer some bytes. */
- copy_bytes = Min(nbytes - copied_bytes, RAW_BUF_BYTES(cstate));
- memcpy(dest, cstate->raw_buf + cstate->raw_buf_index, copy_bytes);
- cstate->raw_buf_index += copy_bytes;
+ copy_bytes = Min(nbytes - copied_bytes, INPUT_BUF_BYTES(cstate));
+ memcpy(dest, cstate->input_buf + cstate->input_buf_index, copy_bytes);
+ cstate->input_buf_index += copy_bytes;
dest += copy_bytes;
copied_bytes += copy_bytes;
} while (copied_bytes < nbytes);
@@ -699,9 +986,6 @@ CopyReadLine(CopyFromState cstate)
resetStringInfo(&cstate->line_buf);
cstate->line_buf_valid = true;
- /* Mark that encoding conversion hasn't occurred yet */
- cstate->line_buf_converted = false;
-
/* Parse data and transfer into line_buf */
result = CopyReadLineText(cstate);
@@ -714,10 +998,13 @@ CopyReadLine(CopyFromState cstate)
*/
if (cstate->copy_src == COPY_NEW_FE)
{
+ int inbytes;
+
do
{
- cstate->raw_buf_index = cstate->raw_buf_len;
- } while (CopyLoadRawBuf(cstate));
+ inbytes = CopyGetData(cstate, cstate->input_buf,
+ 1, INPUT_BUF_SIZE);
+ } while (inbytes > 0);
}
}
else
@@ -754,26 +1041,6 @@ CopyReadLine(CopyFromState cstate)
}
}
- /* Done reading the line. Convert it to server encoding. */
- if (cstate->need_transcoding)
- {
- char *cvt;
-
- cvt = pg_any_to_server(cstate->line_buf.data,
- cstate->line_buf.len,
- cstate->file_encoding);
- if (cvt != cstate->line_buf.data)
- {
- /* transfer converted data back to line_buf */
- resetStringInfo(&cstate->line_buf);
- appendBinaryStringInfo(&cstate->line_buf, cvt, strlen(cvt));
- pfree(cvt);
- }
- }
-
- /* Now it's safe to use the buffer in error messages */
- cstate->line_buf_converted = true;
-
return result;
}
@@ -783,13 +1050,12 @@ CopyReadLine(CopyFromState cstate)
static bool
CopyReadLineText(CopyFromState cstate)
{
- char *copy_raw_buf;
- int raw_buf_ptr;
+ char *copy_input_buf;
+ int input_buf_ptr;
int copy_buf_len;
bool need_data = false;
bool hit_eof = false;
bool result = false;
- char mblen_str[2];
/* CSV variables */
bool first_char_in_line = true;
@@ -807,8 +1073,6 @@ CopyReadLineText(CopyFromState cstate)
escapec = '\0';
}
- mblen_str[1] = '\0';
-
/*
* The objective of this loop is to transfer the entire next input line
* into line_buf. Hence, we only care for detecting newlines (\r and/or
@@ -821,18 +1085,25 @@ CopyReadLineText(CopyFromState cstate)
* These four characters, and the CSV escape and quote characters, are
* assumed the same in frontend and backend encodings.
*
- * For speed, we try to move data from raw_buf to line_buf in chunks
- * rather than one character at a time. raw_buf_ptr points to the next
- * character to examine; any characters from raw_buf_index to raw_buf_ptr
+ * The input has already been converted to the database encoding. All
+ * supported server encodings have the property that all bytes in a
+ * multi-byte sequence have the high bit set, so a multibyte character
+ * cannot contain any newline or escape characters embedded in the
+ * multibyte sequence. Therefore, we can process the input byte-by-byte,
+ * regardless of the encoding.
+ *
+ * For speed, we try to move data from input_buf to line_buf in chunks
+ * rather than one character at a time. input_buf_ptr points to the next
+ * character to examine; any characters from input_buf_index to input_buf_ptr
* have been determined to be part of the line, but not yet transferred to
* line_buf.
*
- * For a little extra speed within the loop, we copy raw_buf and
- * raw_buf_len into local variables.
+ * For a little extra speed within the loop, we copy input_buf and
+ * input_buf_len into local variables.
*/
- copy_raw_buf = cstate->raw_buf;
- raw_buf_ptr = cstate->raw_buf_index;
- copy_buf_len = cstate->raw_buf_len;
+ copy_input_buf = cstate->input_buf;
+ input_buf_ptr = cstate->input_buf_index;
+ copy_buf_len = cstate->input_buf_len;
for (;;)
{
@@ -849,18 +1120,18 @@ CopyReadLineText(CopyFromState cstate)
* cstate->copy_src != COPY_OLD_FE, but it hardly seems worth it,
* considering the size of the buffer.
*/
- if (raw_buf_ptr >= copy_buf_len || need_data)
+ if (input_buf_ptr >= copy_buf_len || need_data)
{
REFILL_LINEBUF;
/*
* Try to read some more data. This will certainly reset
- * raw_buf_index to zero, and raw_buf_ptr must go with it.
+ * input_buf_index to zero, and input_buf_ptr must go with it.
*/
- if (!CopyLoadRawBuf(cstate))
+ if (!CopyLoadInputBufText(cstate))
hit_eof = true;
- raw_buf_ptr = 0;
- copy_buf_len = cstate->raw_buf_len;
+ input_buf_ptr = 0;
+ copy_buf_len = cstate->input_buf_len;
/*
* If we are completely out of data, break out of the loop,
@@ -875,8 +1146,8 @@ CopyReadLineText(CopyFromState cstate)
}
/* OK to fetch a character */
- prev_raw_ptr = raw_buf_ptr;
- c = copy_raw_buf[raw_buf_ptr++];
+ prev_raw_ptr = input_buf_ptr;
+ c = copy_input_buf[input_buf_ptr++];
if (cstate->opts.csv_mode)
{
@@ -930,16 +1201,16 @@ CopyReadLineText(CopyFromState cstate)
* If need more data, go back to loop top to load it.
*
* Note that if we are at EOF, c will wind up as '\0' because
- * of the guaranteed pad of raw_buf.
+ * of the guaranteed pad of input_buf.
*/
IF_NEED_REFILL_AND_NOT_EOF_CONTINUE(0);
/* get next char */
- c = copy_raw_buf[raw_buf_ptr];
+ c = copy_input_buf[input_buf_ptr];
if (c == '\n')
{
- raw_buf_ptr++; /* eat newline */
+ input_buf_ptr++; /* eat newline */
cstate->eol_type = EOL_CRNL; /* in case not set yet */
}
else
@@ -1009,11 +1280,11 @@ CopyReadLineText(CopyFromState cstate)
* through and continue processing for file encoding.
* -----
*/
- c2 = copy_raw_buf[raw_buf_ptr];
+ c2 = copy_input_buf[input_buf_ptr];
if (c2 == '.')
{
- raw_buf_ptr++; /* consume the '.' */
+ input_buf_ptr++; /* consume the '.' */
/*
* Note: if we loop back for more data here, it does not
@@ -1025,7 +1296,7 @@ CopyReadLineText(CopyFromState cstate)
/* Get the next character */
IF_NEED_REFILL_AND_NOT_EOF_CONTINUE(0);
/* if hit_eof, c2 will become '\0' */
- c2 = copy_raw_buf[raw_buf_ptr++];
+ c2 = copy_input_buf[input_buf_ptr++];
if (c2 == '\n')
{
@@ -1050,7 +1321,7 @@ CopyReadLineText(CopyFromState cstate)
/* Get the next character */
IF_NEED_REFILL_AND_NOT_EOF_CONTINUE(0);
/* if hit_eof, c2 will become '\0' */
- c2 = copy_raw_buf[raw_buf_ptr++];
+ c2 = copy_input_buf[input_buf_ptr++];
if (c2 != '\r' && c2 != '\n')
{
@@ -1075,11 +1346,11 @@ CopyReadLineText(CopyFromState cstate)
* Transfer only the data before the \. into line_buf, then
* discard the data and the \. sequence.
*/
- if (prev_raw_ptr > cstate->raw_buf_index)
+ if (prev_raw_ptr > cstate->input_buf_index)
appendBinaryStringInfo(&cstate->line_buf,
- cstate->raw_buf + cstate->raw_buf_index,
- prev_raw_ptr - cstate->raw_buf_index);
- cstate->raw_buf_index = raw_buf_ptr;
+ cstate->input_buf + cstate->input_buf_index,
+ prev_raw_ptr - cstate->input_buf_index);
+ cstate->input_buf_index = input_buf_ptr;
result = true; /* report EOF */
break;
}
@@ -1096,7 +1367,7 @@ CopyReadLineText(CopyFromState cstate)
* character after the backslash just like a normal character,
* so we don't increment in those cases.
*/
- raw_buf_ptr++;
+ input_buf_ptr++;
}
/*
@@ -1106,30 +1377,6 @@ CopyReadLineText(CopyFromState cstate)
* value, while in non-CSV mode, \. cannot be a data value.
*/
not_end_of_copy:
-
- /*
- * Process all bytes of a multi-byte character as a group.
- *
- * We only support multi-byte sequences where the first byte has the
- * high-bit set, so as an optimization we can avoid this block
- * entirely if it is not set.
- */
- if (cstate->encoding_embeds_ascii && IS_HIGHBIT_SET(c))
- {
- int mblen;
-
- /*
- * It is enough to look at the first byte in all our encodings, to
- * get the length. (GB18030 is a bit special, but still works for
- * our purposes; see comment in pg_gb18030_mblen())
- */
- mblen_str[0] = c;
- mblen = pg_encoding_mblen(cstate->file_encoding, mblen_str);
-
- IF_NEED_REFILL_AND_NOT_EOF_CONTINUE(mblen - 1);
- IF_NEED_REFILL_AND_EOF_BREAK(mblen - 1);
- raw_buf_ptr += mblen - 1;
- }
first_char_in_line = false;
} /* end of outer loop */
diff --git a/src/include/commands/copyfrom_internal.h b/src/include/commands/copyfrom_internal.h
index e37942df391..86c92394a09 100644
--- a/src/include/commands/copyfrom_internal.h
+++ b/src/include/commands/copyfrom_internal.h
@@ -53,17 +53,6 @@ typedef enum CopyInsertMethod
/*
* This struct contains all the state variables used throughout a COPY FROM
* operation.
- *
- * Multi-byte encodings: all supported client-side encodings encode multi-byte
- * characters by having the first byte's high bit set. Subsequent bytes of the
- * character can have the high bit not set. When scanning data in such an
- * encoding to look for a match to a single-byte (ie ASCII) character, we must
- * use the full pg_encoding_mblen() machinery to skip over multibyte
- * characters, else we might find a false match to a trailing byte. In
- * supported server encodings, there is no possibility of a false match, and
- * it's faster to make useless comparisons to trailing bytes than it is to
- * invoke pg_encoding_mblen() to skip over them. encoding_embeds_ascii is true
- * when we have to do it the hard way.
*/
typedef struct CopyFromStateData
{
@@ -77,7 +66,7 @@ typedef struct CopyFromStateData
EolType eol_type; /* EOL type of input */
int file_encoding; /* file or remote side's character encoding */
bool need_transcoding; /* file encoding diff from server? */
- bool encoding_embeds_ascii; /* ASCII can be non-first byte? */
+ Oid conversion_proc;
/* parameters from the COPY command */
Relation rel; /* relation to copy from */
@@ -132,31 +121,45 @@ typedef struct CopyFromStateData
/*
* Similarly, line_buf holds the whole input line being processed. The
- * input cycle is first to read the whole line into line_buf, convert it
- * to server encoding there, and then extract the individual attribute
- * fields into attribute_buf. line_buf is preserved unmodified so that we
- * can display it in error messages if appropriate. (In binary mode,
- * line_buf is not used.)
+ * input cycle is first to read the whole line into line_buf, and then
+ * extract the individual attribute fields into attribute_buf. line_buf
+ * is preserved unmodified so that we can display it in error messages
+ * if appropriate. (In binary mode, line_buf is not used.)
*/
StringInfoData line_buf;
- bool line_buf_converted; /* converted to server encoding? */
bool line_buf_valid; /* contains the row being processed? */
/*
- * Finally, raw_buf holds raw data read from the data source (file or
- * client connection). In text mode, CopyReadLine parses this data
+ * input_buf holds input data, already converted to database encoding.
+ *
+ * In text mode, CopyReadLine parses this data
* sufficiently to locate line boundaries, then transfers the data to
- * line_buf and converts it. In binary mode, CopyReadBinaryData fetches
+ * line_buf. In binary mode, CopyReadBinaryData fetches
* appropriate amounts of data from this buffer. In both modes, we
- * guarantee that there is a \0 at raw_buf[raw_buf_len].
+ * guarantee that there is a \0 at input_buf[input_buf_len]. FIXME: do we still?
*/
-#define RAW_BUF_SIZE 65536 /* we palloc RAW_BUF_SIZE+1 bytes */
+#define INPUT_BUF_SIZE 65536 /* we palloc INPUT_BUF_SIZE+1 bytes */
+ char *input_buf;
+ int input_buf_index; /* next byte to process */
+ int input_buf_len; /* total # of bytes stored */
+ /* Shorthand for number of unconsumed bytes available in input_buf */
+#define INPUT_BUF_BYTES(cstate) ((cstate)->input_buf_len - (cstate)->input_buf_index)
+
+ /*
+ * raw_buf holds raw input data read from the data source (file or
+ * client connection), not yet converted to the database encoding.
+ *
+ * If the encoding conversion is not required, the input data is read
+ * directly into 'input_buf', and raw_buf is not used.
+ */
+#define RAW_BUF_SIZE 65536 /* allocated size of the buffer */
char *raw_buf;
int raw_buf_index; /* next byte to process */
- int raw_buf_len; /* total # of bytes stored */
- uint64 bytes_processed;/* number of bytes processed so far */
+ int raw_buf_len; /* total # of bytes stored */
/* Shorthand for number of unconsumed bytes available in raw_buf */
#define RAW_BUF_BYTES(cstate) ((cstate)->raw_buf_len - (cstate)->raw_buf_index)
+
+ uint64 bytes_processed; /* number of bytes processed so far */
} CopyFromStateData;
extern void ReceiveCopyBegin(CopyFromState cstate);
diff --git a/src/test/regress/expected/copycorners.out b/src/test/regress/expected/copycorners.out
new file mode 100644
index 00000000000..ac3a6fe022b
--- /dev/null
+++ b/src/test/regress/expected/copycorners.out
@@ -0,0 +1,202 @@
+create temp table copytest (
+ a text,
+ b text,
+ c text);
+create extension plperlu;
+create function write_test_file(content text) returns void language plperlu as
+$$
+use strict;
+use warnings;
+
+open(FH, '>', '/tmp/copycorners.data') or die $!;
+print FH $_[0];
+close(FH);
+
+$$;
+create function copytest(copyoptions text, content text) returns setof copytest language plpgsql as
+$$
+begin
+ truncate copytest;
+ perform write_test_file($2);
+
+ execute 'copy copytest from ''/tmp/copycorners.data''' || copyoptions;
+ return query select * from copytest;
+end;
+$$;
+-- Basic tests. Not very interesting but see that write_test_file() works.
+select * from copytest('',
+$$a b c$$
+);
+ a | b | c
+---+---+---
+ a | b | c
+(1 row)
+
+select * from copytest('',
+$$a b c
+$$
+);
+ a | b | c
+---+---+---
+ a | b | c
+(1 row)
+
+--
+-- Test EOL detection
+--
+select * from copytest('', E'a b c\nd e f\n'); -- ok
+ a | b | c
+---+---+---
+ a | b | c
+ d | e | f
+(2 rows)
+
+select * from copytest('', E'a b c\rd e f\r'); -- ok
+ a | b | c
+---+---+---
+ a | b | c
+ d | e | f
+(2 rows)
+
+select * from copytest('', E'a b c\r\nd e f\r\n'); -- ok
+ a | b | c
+---+---+---
+ a | b | c
+ d | e | f
+(2 rows)
+
+select * from copytest('', E'a b c\nd e f\r'); -- mismatch
+ERROR: literal carriage return found in data
+HINT: Use "\r" to represent carriage return.
+CONTEXT: COPY copytest, line 2: ""
+SQL statement "copy copytest from '/tmp/copycorners.data'"
+PL/pgSQL function copytest(text,text) line 6 at EXECUTE
+select * from copytest('', E'a b c\rd e f\n'); -- mismatch
+ERROR: literal newline found in data
+HINT: Use "\n" to represent newline.
+CONTEXT: COPY copytest, line 2: ""
+SQL statement "copy copytest from '/tmp/copycorners.data'"
+PL/pgSQL function copytest(text,text) line 6 at EXECUTE
+select * from copytest('', E'a b c\r\nd e f\n'); -- mismatch
+ERROR: literal newline found in data
+HINT: Use "\n" to represent newline.
+CONTEXT: COPY copytest, line 2: ""
+SQL statement "copy copytest from '/tmp/copycorners.data'"
+PL/pgSQL function copytest(text,text) line 6 at EXECUTE
+select * from copytest('', E'a b c\r\nd e f\r'); -- mismatch
+ERROR: literal carriage return found in data
+HINT: Use "\r" to represent carriage return.
+CONTEXT: COPY copytest, line 2: "d e f"
+SQL statement "copy copytest from '/tmp/copycorners.data'"
+PL/pgSQL function copytest(text,text) line 6 at EXECUTE
+--
+-- Test end-of-copy markers at different locations.
+--
+select * from copytest('', E'a b c\\.');
+ERROR: end-of-copy marker corrupt
+CONTEXT: COPY copytest, line 1: "a b c"
+SQL statement "copy copytest from '/tmp/copycorners.data'"
+PL/pgSQL function copytest(text,text) line 6 at EXECUTE
+select * from copytest('', E'a b c\\.\n');
+ a | b | c
+---+---+---
+ a | b | c
+(1 row)
+
+select * from copytest('', E'a b c\n\n\\.');
+ERROR: missing data for column "b"
+CONTEXT: COPY copytest, line 2: ""
+SQL statement "copy copytest from '/tmp/copycorners.data'"
+PL/pgSQL function copytest(text,text) line 6 at EXECUTE
+select * from copytest('', E'a b c\n\n\\.\n');
+ERROR: missing data for column "b"
+CONTEXT: COPY copytest, line 2: ""
+SQL statement "copy copytest from '/tmp/copycorners.data'"
+PL/pgSQL function copytest(text,text) line 6 at EXECUTE
+-- \. on a line of its own, with garbage after it
+select * from copytest('', E'a b c\n\\.\ngarbage');
+ a | b | c
+---+---+---
+ a | b | c
+(1 row)
+
+-- \. at beginning of line, with garbage after it
+select * from copytest('', E'a b c\n\\.garbage');
+ERROR: end-of-copy marker corrupt
+CONTEXT: COPY copytest, line 2: ""
+SQL statement "copy copytest from '/tmp/copycorners.data'"
+PL/pgSQL function copytest(text,text) line 6 at EXECUTE
+-- \. in the middle of file, and garbage after it.
+select * from copytest('', E'a b\\.garbage');
+ERROR: end-of-copy marker corrupt
+CONTEXT: COPY copytest, line 1: ""
+SQL statement "copy copytest from '/tmp/copycorners.data'"
+PL/pgSQL function copytest(text,text) line 6 at EXECUTE
+--
+-- Test end-of-copy markers with different EOLs
+--
+select * from copytest('', E'a b c\nd e f\\.\n');
+ a | b | c
+---+---+---
+ a | b | c
+ d | e | f
+(2 rows)
+
+select * from copytest('', E'a b c\rd e f\\.\r');
+ a | b | c
+---+---+---
+ a | b | c
+ d | e | f
+(2 rows)
+
+select * from copytest('', E'a b c\r\nd e f\\.\r\n');
+ a | b | c
+---+---+---
+ a | b | c
+ d | e | f
+(2 rows)
+
+-- mismatch between EOL style and EOL after \.
+select * from copytest('', E'a b c\na b c\\.\r');
+ERROR: end-of-copy marker does not match previous newline style
+CONTEXT: COPY copytest, line 2: ""
+SQL statement "copy copytest from '/tmp/copycorners.data'"
+PL/pgSQL function copytest(text,text) line 6 at EXECUTE
+select * from copytest('', E'a b c\ra b c\\.\n');
+ERROR: end-of-copy marker does not match previous newline style
+CONTEXT: COPY copytest, line 2: ""
+SQL statement "copy copytest from '/tmp/copycorners.data'"
+PL/pgSQL function copytest(text,text) line 6 at EXECUTE
+select * from copytest('', E'a b c\r\na b c\\.\n');
+ERROR: end-of-copy marker does not match previous newline style
+CONTEXT: COPY copytest, line 2: ""
+SQL statement "copy copytest from '/tmp/copycorners.data'"
+PL/pgSQL function copytest(text,text) line 6 at EXECUTE
+select * from copytest('', E'a b c\na b c\\.\r\n');
+ERROR: end-of-copy marker does not match previous newline style
+CONTEXT: COPY copytest, line 2: ""
+SQL statement "copy copytest from '/tmp/copycorners.data'"
+PL/pgSQL function copytest(text,text) line 6 at EXECUTE
+-- end-of-copy marker on first line, with different EOL styles
+select * from copytest('', E'a b c\\.');
+ERROR: end-of-copy marker corrupt
+CONTEXT: COPY copytest, line 1: "a b c"
+SQL statement "copy copytest from '/tmp/copycorners.data'"
+PL/pgSQL function copytest(text,text) line 6 at EXECUTE
+select * from copytest('', E'a b c\\.\n');
+ a | b | c
+---+---+---
+ a | b | c
+(1 row)
+
+select * from copytest('', E'a b c\\.\r');
+ a | b | c
+---+---+---
+ a | b | c
+(1 row)
+
+select * from copytest('', E'a b c\\.\r\n');
+ERROR: missing data for column "b"
+CONTEXT: COPY copytest, line 2: ""
+SQL statement "copy copytest from '/tmp/copycorners.data'"
+PL/pgSQL function copytest(text,text) line 6 at EXECUTE
diff --git a/src/test/regress/sql/copycorners.sql b/src/test/regress/sql/copycorners.sql
new file mode 100644
index 00000000000..c5960bdceab
--- /dev/null
+++ b/src/test/regress/sql/copycorners.sql
@@ -0,0 +1,90 @@
+create temp table copytest (
+ a text,
+ b text,
+ c text);
+
+create extension plperlu;
+
+create function write_test_file(content text) returns void language plperlu as
+$$
+use strict;
+use warnings;
+
+open(FH, '>', '/tmp/copycorners.data') or die $!;
+print FH $_[0];
+close(FH);
+
+$$;
+
+create function copytest(copyoptions text, content text) returns setof copytest language plpgsql as
+$$
+begin
+ truncate copytest;
+ perform write_test_file($2);
+
+ execute 'copy copytest from ''/tmp/copycorners.data''' || copyoptions;
+ return query select * from copytest;
+end;
+$$;
+
+-- Basic tests. Not very interesting but see that write_test_file() works.
+select * from copytest('',
+$$a b c$$
+);
+
+select * from copytest('',
+$$a b c
+$$
+);
+
+--
+-- Test EOL detection
+--
+select * from copytest('', E'a b c\nd e f\n'); -- ok
+select * from copytest('', E'a b c\rd e f\r'); -- ok
+select * from copytest('', E'a b c\r\nd e f\r\n'); -- ok
+select * from copytest('', E'a b c\nd e f\r'); -- mismatch
+select * from copytest('', E'a b c\rd e f\n'); -- mismatch
+select * from copytest('', E'a b c\r\nd e f\n'); -- mismatch
+select * from copytest('', E'a b c\r\nd e f\r'); -- mismatch
+
+--
+-- Test end-of-copy markers at different locations.
+--
+
+select * from copytest('', E'a b c\\.');
+
+select * from copytest('', E'a b c\\.\n');
+
+select * from copytest('', E'a b c\n\n\\.');
+
+select * from copytest('', E'a b c\n\n\\.\n');
+
+-- \. on a line of its own, with garbage after it
+select * from copytest('', E'a b c\n\\.\ngarbage');
+
+-- \. at beginning of line, with garbage after it
+select * from copytest('', E'a b c\n\\.garbage');
+
+-- \. in the middle of file, and garbage after it.
+select * from copytest('', E'a b\\.garbage');
+
+
+--
+-- Test end-of-copy markers with different EOLs
+--
+select * from copytest('', E'a b c\nd e f\\.\n');
+select * from copytest('', E'a b c\rd e f\\.\r');
+select * from copytest('', E'a b c\r\nd e f\\.\r\n');
+
+-- mismatch between EOL style and EOL after \.
+select * from copytest('', E'a b c\na b c\\.\r');
+select * from copytest('', E'a b c\ra b c\\.\n');
+select * from copytest('', E'a b c\r\na b c\\.\n');
+select * from copytest('', E'a b c\na b c\\.\r\n');
+
+-- end-of-copy marker on first line, with different EOL styles
+select * from copytest('', E'a b c\\.');
+select * from copytest('', E'a b c\\.\n');
+select * from copytest('', E'a b c\\.\r');
+select * from copytest('', E'a b c\\.\r\n');
--
2.29.2
--------------39E46EF15EBFAA37D53332E5--
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-01-28 08:57 Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 24+ messages in thread
From: Peter Eisentraut @ 2022-01-28 08:57 UTC (permalink / raw)
To: Rémi Lapeyre <[email protected]>; Zhihong Yu <[email protected]>; +Cc: David Steele <[email protected]>; Michael Paquier <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Daniel Verite <[email protected]>
On 31.12.21 18:36, Rémi Lapeyre wrote:
> Here’s an updated version of the patch that takes into account the changes in d1029bb5a2. The actual code is the same as v10 which was already marked as ready for committer.
I have committed the 0001 patch. I will work on the 0002 patch next.
I notice in the 0002 patch that there is no test case for the error
"wrong header for column \"%s\": got \"%s\"", which I think is really
the core functionality of this patch. So please add that.
I wonder whether the header matching should be a separate option from
the HEADER option. The option parsing in this patch is quite
complicated and could be simpler if there were two separate options. It
appears this has been mentioned in the thread but not fully discussed.
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-01-30 22:56 Rémi Lapeyre <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 24+ messages in thread
From: Rémi Lapeyre @ 2022-01-30 22:56 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Zhihong Yu <[email protected]>; David Steele <[email protected]>; Michael Paquier <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Daniel Verite <[email protected]>
> On 28 Jan 2022, at 09:57, Peter Eisentraut <[email protected]> wrote:
>
> On 31.12.21 18:36, Rémi Lapeyre wrote:
>> Here’s an updated version of the patch that takes into account the changes in d1029bb5a2. The actual code is the same as v10 which was already marked as ready for committer.
>
> I have committed the 0001 patch. I will work on the 0002 patch next.
>
Thanks!
> I notice in the 0002 patch that there is no test case for the error "wrong header for column \"%s\": got \"%s\"", which I think is really the core functionality of this patch. So please add that.
>
I added a test for it in this new version of the patch.
> I wonder whether the header matching should be a separate option from the HEADER option. The option parsing in this patch is quite complicated and could be simpler if there were two separate options. It appears this has been mentioned in the thread but not fully discussed.
I suppose a new option could be added but I’m not sure it would simplify things much with regard to the code and in my opinion it would be a bit weirder for users, right now it is just:
copy my_table from stdin with (header match);
with an additional option it could be:
copy my_table from stdin with (header true, match);
with potentially “header true” being implicit when “match” is given:
copy my_table from stdin with (match);
But I think we would still have to check for and return an error if the user inputs:
copy my_table from stdin with (header off, match);
Rather than complicating things, the current implementation seemed to be the best but I will update the patch if you think I should change it.
Best regards,
Rémi
Attachments:
[application/octet-stream] v12-0001-Add-header-matching-mode-to-COPY-FROM.patch (14.7K, ../../[email protected]/2-v12-0001-Add-header-matching-mode-to-COPY-FROM.patch)
download | inline diff:
From 1fbfd87a4070b91a4ac3630b36a1ff1dadecaa5d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9mi=20Lapeyre?= <[email protected]>
Date: Tue, 13 Oct 2020 14:45:56 +0200
Subject: [PATCH v12] Add header matching mode to "COPY FROM"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.35.0"
This is a multi-part message in MIME format.
--------------2.35.0
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
COPY FROM supports the HEADER option to silently discard the header from
a CSV or text file. It is possible to load by mistake a file that
matches the expected format, for example if two text columns have been
swapped, resulting in garbage in the database.
This option adds the possibility to actually check the header to make
sure it matches what is expected and exit immediatly if it does not.
Discussion: https://www.postgresql.org/message-id/flat/CAF1-J-0PtCWMeLtswwGV2M70U26n4g33gpe1rcKQqe6wVQDrFA@mail.gmail.com
---
contrib/file_fdw/expected/file_fdw.out | 10 +++-
contrib/file_fdw/sql/file_fdw.sql | 7 +++
doc/src/sgml/ref/copy.sgml | 11 +++--
src/backend/commands/copy.c | 65 ++++++++++++++++++++++++--
src/backend/commands/copyfromparse.c | 52 +++++++++++++++++++--
src/backend/commands/copyto.c | 2 +-
src/include/commands/copy.h | 12 ++++-
src/test/regress/expected/copy.out | 20 ++++++++
src/test/regress/sql/copy.sql | 29 ++++++++++++
9 files changed, 195 insertions(+), 13 deletions(-)
--------------2.35.0
Content-Type: text/x-patch; name="v12-0001-Add-header-matching-mode-to-COPY-FROM.patch"
Content-Transfer-Encoding: 8bit
Content-Disposition: attachment; filename="v12-0001-Add-header-matching-mode-to-COPY-FROM.patch"
diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out
index 0ac6e4e0d7..b1617640d8 100644
--- a/contrib/file_fdw/expected/file_fdw.out
+++ b/contrib/file_fdw/expected/file_fdw.out
@@ -113,6 +113,12 @@ CREATE FOREIGN TABLE agg_bad (
) SERVER file_server
OPTIONS (format 'csv', filename :'filename', header 'true', delimiter ';', quote '@', escape '"', null '');
ALTER FOREIGN TABLE agg_bad ADD CHECK (a >= 0);
+-- test header matching
+\set filename :abs_srcdir '/data/list1.csv'
+CREATE FOREIGN TABLE header_match ("1" int, foo text) SERVER file_server
+OPTIONS (format 'csv', filename :'filename', delimiter ',', header 'match');
+CREATE FOREIGN TABLE header_doesnt_match (a int, foo text) SERVER file_server
+OPTIONS (format 'csv', filename :'filename', delimiter ',', header 'match'); -- ERROR
-- per-column options tests
\set filename :abs_srcdir '/data/text.csv'
CREATE FOREIGN TABLE text_csv (
@@ -464,12 +470,14 @@ SET ROLE regress_file_fdw_superuser;
-- cleanup
RESET ROLE;
DROP EXTENSION file_fdw CASCADE;
-NOTICE: drop cascades to 7 other objects
+NOTICE: drop cascades to 9 other objects
DETAIL: drop cascades to server file_server
drop cascades to user mapping for regress_file_fdw_superuser on server file_server
drop cascades to user mapping for regress_no_priv_user on server file_server
drop cascades to foreign table agg_text
drop cascades to foreign table agg_csv
drop cascades to foreign table agg_bad
+drop cascades to foreign table header_match
+drop cascades to foreign table header_doesnt_match
drop cascades to foreign table text_csv
DROP ROLE regress_file_fdw_superuser, regress_file_fdw_user, regress_no_priv_user;
diff --git a/contrib/file_fdw/sql/file_fdw.sql b/contrib/file_fdw/sql/file_fdw.sql
index 86f876d565..9f3c7219d0 100644
--- a/contrib/file_fdw/sql/file_fdw.sql
+++ b/contrib/file_fdw/sql/file_fdw.sql
@@ -103,6 +103,13 @@ CREATE FOREIGN TABLE agg_bad (
OPTIONS (format 'csv', filename :'filename', header 'true', delimiter ';', quote '@', escape '"', null '');
ALTER FOREIGN TABLE agg_bad ADD CHECK (a >= 0);
+-- test header matching
+\set filename :abs_srcdir '/data/list1.csv'
+CREATE FOREIGN TABLE header_match ("1" int, foo text) SERVER file_server
+OPTIONS (format 'csv', filename :'filename', delimiter ',', header 'match');
+CREATE FOREIGN TABLE header_doesnt_match (a int, foo text) SERVER file_server
+OPTIONS (format 'csv', filename :'filename', delimiter ',', header 'match'); -- ERROR
+
-- per-column options tests
\set filename :abs_srcdir '/data/text.csv'
CREATE FOREIGN TABLE text_csv (
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 1b7d001963..f36eca02fb 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -36,7 +36,7 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
FREEZE [ <replaceable class="parameter">boolean</replaceable> ]
DELIMITER '<replaceable class="parameter">delimiter_character</replaceable>'
NULL '<replaceable class="parameter">null_string</replaceable>'
- HEADER [ <replaceable class="parameter">boolean</replaceable> ]
+ HEADER [ <literal>match</literal> | <literal>true</literal> | <literal>false</literal> ]
QUOTE '<replaceable class="parameter">quote_character</replaceable>'
ESCAPE '<replaceable class="parameter">escape_character</replaceable>'
FORCE_QUOTE { ( <replaceable class="parameter">column_name</replaceable> [, ...] ) | * }
@@ -276,8 +276,13 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
<para>
Specifies that the file contains a header line with the names of each
column in the file. On output, the first line contains the column
- names from the table, and on input, the first line is ignored.
- This option is not allowed when using <literal>binary</literal> format.
+ names from the table. On input, the first line is discarded when
+ <literal>header</literal> is set to <literal>true</literal> or required
+ to match the column names if set to <literal>match</literal>. If the
+ number of columns in the header is not correct, their order differs
+ from the one expected, or the name or case doesn't match, the copy will
+ be aborted with an error. This option is allowed only when using
+ <literal>CSV</literal> or <literal>text</literal> format.
</para>
</listitem>
</varlistentry>
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 7da7105d44..830bd9f762 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -314,7 +314,66 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
}
/*
- * Process the statement option list for COPY.
+* Extract a CopyHeader value from a DefElem.
+*/
+static CopyHeader
+DefGetCopyHeader(DefElem *def)
+{
+ /*
+ * If no parameter given, assume "true" is meant.
+ */
+ if (def->arg == NULL)
+ return COPY_HEADER_PRESENT;
+
+ /*
+ * Allow 0, 1, "true", "false", "on", "off" or "match".
+ */
+ switch (nodeTag(def->arg))
+ {
+ case T_Integer:
+ switch (intVal(def->arg))
+ {
+ case 0:
+ return COPY_HEADER_ABSENT;
+ case 1:
+ return COPY_HEADER_PRESENT;
+ default:
+ /* otherwise, error out below */
+ break;
+ }
+ break;
+ default:
+ {
+ char *sval = defGetString(def);
+
+ /*
+ * The set of strings accepted here should match up with the
+ * grammar's opt_boolean_or_string production.
+ */
+ if (pg_strcasecmp(sval, "true") == 0)
+ return COPY_HEADER_PRESENT;
+ if (pg_strcasecmp(sval, "false") == 0)
+ return COPY_HEADER_ABSENT;
+ if (pg_strcasecmp(sval, "on") == 0)
+ return COPY_HEADER_PRESENT;
+ if (pg_strcasecmp(sval, "off") == 0)
+ return COPY_HEADER_ABSENT;
+ if (pg_strcasecmp(sval, "match") == 0)
+ return COPY_HEADER_MATCH;
+
+ }
+ break;
+ }
+
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("%s requires a boolean or \"match\"",
+ def->defname)));
+ return COPY_HEADER_ABSENT; /* keep compiler quiet */
+}
+
+/*
+* Process the statement option list for COPY.
*
* Scan the options list (a list of DefElem) and transpose the information
* into *opts_out, applying appropriate error checking.
@@ -394,7 +453,7 @@ ProcessCopyOptions(ParseState *pstate,
if (header_specified)
errorConflictingDefElem(defel, pstate);
header_specified = true;
- opts_out->header_line = defGetBoolean(defel);
+ opts_out->header_line = DefGetCopyHeader(defel);
}
else if (strcmp(defel->defname, "quote") == 0)
{
@@ -555,7 +614,7 @@ ProcessCopyOptions(ParseState *pstate,
errmsg("COPY delimiter cannot be \"%s\"", opts_out->delim)));
/* Check header */
- if (opts_out->binary && opts_out->header_line)
+ if (opts_out->binary && opts_out->header_line != COPY_HEADER_ABSENT)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("cannot specify HEADER in BINARY mode")));
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index baf328b620..5ab59c0631 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -72,6 +72,7 @@
#include "miscadmin.h"
#include "pgstat.h"
#include "port/pg_bswap.h"
+#include "utils/builtins.h"
#include "utils/memutils.h"
#include "utils/rel.h"
@@ -758,12 +759,55 @@ NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields)
/* only available for text or csv input */
Assert(!cstate->opts.binary);
- /* on input just throw the header line away */
- if (cstate->cur_lineno == 0 && cstate->opts.header_line)
+ /* on input check that the header line is correct if needed */
+ if (cstate->cur_lineno == 0 && cstate->opts.header_line != COPY_HEADER_ABSENT)
{
+ ListCell *cur;
+ TupleDesc tupDesc;
+
+ tupDesc = RelationGetDescr(cstate->rel);
+
cstate->cur_lineno++;
- if (CopyReadLine(cstate))
- return false; /* done */
+ done = CopyReadLine(cstate);
+
+ if (cstate->opts.header_line == COPY_HEADER_MATCH)
+ {
+ if (cstate->opts.csv_mode)
+ fldct = CopyReadAttributesCSV(cstate);
+ else
+ fldct = CopyReadAttributesText(cstate);
+
+ if (fldct < list_length(cstate->attnumlist))
+ ereport(ERROR,
+ (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
+ errmsg("incomplete header, expected %d columns but got %d",
+ list_length(cstate->attnumlist), fldct)));
+ else if (fldct > list_length(cstate->attnumlist))
+ ereport(ERROR,
+ (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
+ errmsg("extra data after last expected header, expected %d columns but got %d",
+ list_length(cstate->attnumlist), fldct)));
+
+ foreach(cur, cstate->attnumlist)
+ {
+ int attnum = lfirst_int(cur);
+ char *colName = cstate->raw_fields[attnum - 1];
+ Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
+
+ if (colName == NULL)
+ colName = cstate->opts.null_print;
+
+ if (namestrcmp(&attr->attname, colName) != 0) {
+ ereport(ERROR,
+ (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
+ errmsg("wrong header for column \"%s\": got \"%s\"",
+ NameStr(attr->attname), colName)));
+ }
+ }
+ }
+
+ if (done)
+ return false;
}
cstate->cur_lineno++;
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index 3283ef50d0..6c611c0277 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -846,7 +846,7 @@ DoCopyTo(CopyToState cstate)
cstate->file_encoding);
/* if a header has been requested send the line */
- if (cstate->opts.header_line)
+ if (cstate->opts.header_line != COPY_HEADER_ABSENT)
{
bool hdr_delim = false;
diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h
index 8694da5004..6fb2cade6b 100644
--- a/src/include/commands/copy.h
+++ b/src/include/commands/copy.h
@@ -19,6 +19,16 @@
#include "parser/parse_node.h"
#include "tcop/dest.h"
+/*
+ * Represents whether the header must match, be absent or be present.
+ */
+typedef enum CopyHeader
+{
+ COPY_HEADER_ABSENT,
+ COPY_HEADER_PRESENT,
+ COPY_HEADER_MATCH
+} CopyHeader;
+
/*
* A struct to hold COPY options, in a parsed form. All of these are related
* to formatting, except for 'freeze', which doesn't really belong here, but
@@ -32,7 +42,7 @@ typedef struct CopyFormatOptions
bool binary; /* binary format? */
bool freeze; /* freeze rows on loading? */
bool csv_mode; /* Comma Separated Value format? */
- bool header_line; /* header line? */
+ CopyHeader header_line; /* CSV or text header line? */
char *null_print; /* NULL marker string (server encoding!) */
int null_print_len; /* length of same */
char *null_print_client; /* same converted to file encoding */
diff --git a/src/test/regress/expected/copy.out b/src/test/regress/expected/copy.out
index 851b9a4a2d..77b9a70b74 100644
--- a/src/test/regress/expected/copy.out
+++ b/src/test/regress/expected/copy.out
@@ -254,3 +254,23 @@ INFO: progress: {"type": "FILE", "command": "COPY FROM", "relname": "tab_progre
drop trigger check_after_tab_progress_reporting on tab_progress_reporting;
drop function notice_after_tab_progress_reporting();
drop table tab_progress_reporting;
+-- Test header matching feature
+create table header_copytest (
+ a int,
+ b int,
+ c text
+);
+copy header_copytest from stdin with (header wrong_choice);
+ERROR: header requires a boolean or "match"
+copy header_copytest from stdin with (header match);
+copy header_copytest from stdin with (header match);
+ERROR: incomplete header, expected 3 columns but got 2
+CONTEXT: COPY header_copytest, line 1: "a b"
+copy header_copytest from stdin with (header match);
+ERROR: extra data after last expected header, expected 3 columns but got 4
+CONTEXT: COPY header_copytest, line 1: "a b c d"
+copy header_copytest from stdin with (header match);
+ERROR: wrong header for column "c": got "d"
+CONTEXT: COPY header_copytest, line 1: "a b d"
+copy header_copytest from stdin with (header match, format csv);
+drop table header_copytest;
diff --git a/src/test/regress/sql/copy.sql b/src/test/regress/sql/copy.sql
index 016fedf675..5e192428a3 100644
--- a/src/test/regress/sql/copy.sql
+++ b/src/test/regress/sql/copy.sql
@@ -303,3 +303,32 @@ copy tab_progress_reporting from :'filename'
drop trigger check_after_tab_progress_reporting on tab_progress_reporting;
drop function notice_after_tab_progress_reporting();
drop table tab_progress_reporting;
+
+-- Test header matching feature
+create table header_copytest (
+ a int,
+ b int,
+ c text
+);
+copy header_copytest from stdin with (header wrong_choice);
+copy header_copytest from stdin with (header match);
+a b c
+1 2 foo
+\.
+copy header_copytest from stdin with (header match);
+a b
+1 2
+\.
+copy header_copytest from stdin with (header match);
+a b c d
+1 2 foo bar
+\.
+copy header_copytest from stdin with (header match);
+a b d
+1 2 foo
+\.
+copy header_copytest from stdin with (header match, format csv);
+a,b,c
+1,2,foo
+\.
+drop table header_copytest;
--------------2.35.0--
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-01-31 06:54 Peter Eisentraut <[email protected]>
parent: Rémi Lapeyre <[email protected]>
0 siblings, 1 reply; 24+ messages in thread
From: Peter Eisentraut @ 2022-01-31 06:54 UTC (permalink / raw)
To: Rémi Lapeyre <[email protected]>; +Cc: Zhihong Yu <[email protected]>; David Steele <[email protected]>; Michael Paquier <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Daniel Verite <[email protected]>
On 30.01.22 23:56, Rémi Lapeyre wrote:
>> I notice in the 0002 patch that there is no test case for the error "wrong header for column \"%s\": got \"%s\"", which I think is really the core functionality of this patch. So please add that.
>>
>
> I added a test for it in this new version of the patch.
The file_fdw.sql tests contain this
+CREATE FOREIGN TABLE header_doesnt_match (a int, foo text) SERVER
file_server
+OPTIONS (format 'csv', filename :'filename', delimiter ',', header
'match'); -- ERROR
but no actual error is generated. Please review the additions on the
file_fdw tests to see that they make sense.
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-02-03 06:37 Peter Eisentraut <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 2 replies; 24+ messages in thread
From: Peter Eisentraut @ 2022-02-03 06:37 UTC (permalink / raw)
To: Rémi Lapeyre <[email protected]>; +Cc: Zhihong Yu <[email protected]>; David Steele <[email protected]>; Michael Paquier <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Daniel Verite <[email protected]>
On 31.01.22 07:54, Peter Eisentraut wrote:
> On 30.01.22 23:56, Rémi Lapeyre wrote:
>>> I notice in the 0002 patch that there is no test case for the error
>>> "wrong header for column \"%s\": got \"%s\"", which I think is really
>>> the core functionality of this patch. So please add that.
>>>
>>
>> I added a test for it in this new version of the patch.
>
> The file_fdw.sql tests contain this
>
> +CREATE FOREIGN TABLE header_doesnt_match (a int, foo text) SERVER
> file_server
> +OPTIONS (format 'csv', filename :'filename', delimiter ',', header
> 'match'); -- ERROR
>
> but no actual error is generated. Please review the additions on the
> file_fdw tests to see that they make sense.
A few more comments on your latest patch:
- The DefGetCopyHeader() function seems very bulky and might not be
necessary. I think you can just check for the string "match" first and
then use defGetBoolean() as before if it didn't match.
- If you define COPY_HEADER_ABSENT = 0 in the enum, then most of the
existing truth checks don't need to be changed.
- In NextCopyFromRawFields(), it looks like you have code that replaces
the null_print value if the supplied column name is null. I don't think
we should allow null column values. Someone, this should be an error.
(Do we use null_print on output and make the column name null if it
matches?)
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-03-25 04:21 Greg Stark <[email protected]>
parent: Peter Eisentraut <[email protected]>
1 sibling, 1 reply; 24+ messages in thread
From: Greg Stark @ 2022-03-25 04:21 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Rémi Lapeyre <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; Michael Paquier <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Daniel Verite <[email protected]>
Great to see the first of the two patches committed.
It looks like the second patch got some feedback from Peter in
February and has been marked "Waiting on author" ever since.
Remi, will you have a chance to look at this this month?
Peter, are these comments blocking if Remi doesn't have a chance to
work on it should I move it to the next release or could it be fixed
up and committed?
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-03-25 12:02 Daniel Verite <[email protected]>
parent: Peter Eisentraut <[email protected]>
1 sibling, 0 replies; 24+ messages in thread
From: Daniel Verite @ 2022-03-25 12:02 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: =?ISO-8859-15?Q?R=E9mi_Lapeyre?= <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; Michael Paquier <[email protected]>; vignesh C <[email protected]>; pgsql-hackers
Peter Eisentraut wrote:
> - The DefGetCopyHeader() function seems very bulky and might not be
> necessary. I think you can just check for the string "match" first and
> then use defGetBoolean() as before if it didn't match.
The problem is that defGetBoolean() ends like this in the non-matching
case:
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s requires a Boolean value",
def->defname)));
We don't want this error message when the string does not match
since it's really a tri-state option, not a boolean.
To avoid duplicating the code that recognizes true/false/on/off/0/1,
probably defGetBoolean()'s guts should be moved into another function
that does the matching and report to the caller instead of throwing an
error. Then DefGetCopyHeader() could call that non-throwing function.
> - If you define COPY_HEADER_ABSENT = 0 in the enum, then most of the
> existing truth checks don't need to be changed.
It's already 0 by default. Not changing some truth checks does work, but
then we get some code that treat CopyFromState.header_line like
a boolean and some other code like an enum, which I found not
ideal wrt clarity in an earlier round of review [1]
It's not a major issue though, as it concerns only 3 lines of code in the
v12
patch:
- if (opts_out->binary && opts_out->header_line)
+ if (opts_out->binary && opts_out->header_line != COPY_HEADER_ABSENT)
+ /* on input check that the header line is correct if needed */
+ if (cstate->cur_lineno == 0 && cstate->opts.header_line !=
COPY_HEADER_ABSENT)
- if (cstate->opts.header_line)
+ if (cstate->opts.header_line != COPY_HEADER_ABSENT)
> - In NextCopyFromRawFields(), it looks like you have code that replaces
> the null_print value if the supplied column name is null. I don't think
> we should allow null column values. Someone, this should be an error.
+1
[1]
https://www.postgresql.org/message-id/80a9b594-01d6-4ee4-a612-9ae9fd3c1194%40manitou-mail.org
Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-03-29 15:02 Peter Eisentraut <[email protected]>
parent: Greg Stark <[email protected]>
0 siblings, 1 reply; 24+ messages in thread
From: Peter Eisentraut @ 2022-03-29 15:02 UTC (permalink / raw)
To: Greg Stark <[email protected]>; +Cc: Rémi Lapeyre <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; Michael Paquier <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Daniel Verite <[email protected]>
On 25.03.22 05:21, Greg Stark wrote:
> Great to see the first of the two patches committed.
>
> It looks like the second patch got some feedback from Peter in
> February and has been marked "Waiting on author" ever since.
>
> Remi, will you have a chance to look at this this month?
>
> Peter, are these comments blocking if Remi doesn't have a chance to
> work on it should I move it to the next release or could it be fixed
> up and committed?
I will try to finish up this patch.
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-03-30 07:11 Peter Eisentraut <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 24+ messages in thread
From: Peter Eisentraut @ 2022-03-30 07:11 UTC (permalink / raw)
To: Greg Stark <[email protected]>; +Cc: Rémi Lapeyre <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; Michael Paquier <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Daniel Verite <[email protected]>
On 29.03.22 17:02, Peter Eisentraut wrote:
> On 25.03.22 05:21, Greg Stark wrote:
>> Great to see the first of the two patches committed.
>>
>> It looks like the second patch got some feedback from Peter in
>> February and has been marked "Waiting on author" ever since.
>>
>> Remi, will you have a chance to look at this this month?
>>
>> Peter, are these comments blocking if Remi doesn't have a chance to
>> work on it should I move it to the next release or could it be fixed
>> up and committed?
>
> I will try to finish up this patch.
Committed, after some further refinements as discussed.
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-06-07 15:47 Julien Rouhaud <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 24+ messages in thread
From: Julien Rouhaud @ 2022-06-07 15:47 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Greg Stark <[email protected]>; Rémi Lapeyre <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; Michael Paquier <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Daniel Verite <[email protected]>
Hi,
On Wed, Mar 30, 2022 at 09:11:09AM +0200, Peter Eisentraut wrote:
>
> Committed, after some further refinements as discussed.
While working on nearby code, I found some problems with this feature.
First, probably nitpicking, the HEADER MATCH is allowed for COPY TO, is that
expected? The documentation isn't really explicit about it, but there's
nothing to match when exporting data it's a bit surprising. I'm not opposed to
have HEADER MATCH means HEADER ON for COPY TO, as as-is one can easily reuse
the commands history, but maybe it should be clearly documented?
Then, apparently HEADER MATCH doesn't let you do sanity checks against a custom
column list. This one looks like a clear oversight, as something like that
should be entirely valid IMHO:
CREATE TABLE tbl(col1 int, col2 int);
COPY tbl (col2, col1) TO '/path/to/file' WITH (HEADER MATCH);
COPY tbl (col2, col1) FROM '/path/to/file' WITH (HEADER MATCH);
but right now it errors out with:
ERROR: column name mismatch in header line field 1: got "col1", expected "col2"
Note that the error message is bogus if you specify attributes in a
different order from the relation, as the code is mixing access to the tuple
desc and access to the raw fields with the same offset.
This also means that it will actually fail to detect a mismatch in the provided
column list and let you import data in the wrong position as long as the
datatypes are compatible and the column header in the file are in the correct
order. For instance:
CREATE TABLE abc (a text, b text, c text);
INSERT INTO abc SELECT 'a', 'b', 'c';
COPY abc TO '/path/to/file' WITH (HEADER MATCH);
You can then import the data with any of those:
COPY abc(c, b, a) TO '/path/to/file' WITH (HEADER MATCH);
COPY abc(c, a, b) TO '/path/to/file' WITH (HEADER MATCH);
[...]
SELECT * FROM abc;
Even worse, if you try to do a COPY ... FROM ... WITH (HEADER ON) on a table
that has some dropped attribute(s). The current code will access random memory
as there's no exact attnum / raw field mapping anymore.
I can work on a fix if needed (with some additional regression test to cover
those cases), but I'm still not sure that having a user provided column list is
supposed to be accepted or not for the HEADER MATCH. In the meantime I will
add an open item.
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-06-12 13:36 Andrew Dunstan <[email protected]>
parent: Julien Rouhaud <[email protected]>
0 siblings, 1 reply; 24+ messages in thread
From: Andrew Dunstan @ 2022-06-12 13:36 UTC (permalink / raw)
To: Julien Rouhaud <[email protected]>; Peter Eisentraut <[email protected]>; +Cc: Greg Stark <[email protected]>; Rémi Lapeyre <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; Michael Paquier <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Daniel Verite <[email protected]>
On 2022-06-07 Tu 11:47, Julien Rouhaud wrote:
> Hi,
>
> On Wed, Mar 30, 2022 at 09:11:09AM +0200, Peter Eisentraut wrote:
>> Committed, after some further refinements as discussed.
> While working on nearby code, I found some problems with this feature.
>
> First, probably nitpicking, the HEADER MATCH is allowed for COPY TO, is that
> expected? The documentation isn't really explicit about it, but there's
> nothing to match when exporting data it's a bit surprising. I'm not opposed to
> have HEADER MATCH means HEADER ON for COPY TO, as as-is one can easily reuse
> the commands history, but maybe it should be clearly documented?
I think it makes more sense to have a sanity check to prevent HEADER
MATCH with COPY TO.
>
> Then, apparently HEADER MATCH doesn't let you do sanity checks against a custom
> column list. This one looks like a clear oversight, as something like that
> should be entirely valid IMHO:
>
> CREATE TABLE tbl(col1 int, col2 int);
> COPY tbl (col2, col1) TO '/path/to/file' WITH (HEADER MATCH);
> COPY tbl (col2, col1) FROM '/path/to/file' WITH (HEADER MATCH);
>
> but right now it errors out with:
>
> ERROR: column name mismatch in header line field 1: got "col1", expected "col2"
>
> Note that the error message is bogus if you specify attributes in a
> different order from the relation, as the code is mixing access to the tuple
> desc and access to the raw fields with the same offset.
>
> This also means that it will actually fail to detect a mismatch in the provided
> column list and let you import data in the wrong position as long as the
> datatypes are compatible and the column header in the file are in the correct
> order. For instance:
>
> CREATE TABLE abc (a text, b text, c text);
> INSERT INTO abc SELECT 'a', 'b', 'c';
> COPY abc TO '/path/to/file' WITH (HEADER MATCH);
>
> You can then import the data with any of those:
> COPY abc(c, b, a) TO '/path/to/file' WITH (HEADER MATCH);
> COPY abc(c, a, b) TO '/path/to/file' WITH (HEADER MATCH);
> [...]
> SELECT * FROM abc;
>
> Even worse, if you try to do a COPY ... FROM ... WITH (HEADER ON) on a table
> that has some dropped attribute(s). The current code will access random memory
> as there's no exact attnum / raw field mapping anymore.
Ouch! That certainly needs to be fixed.
>
> I can work on a fix if needed (with some additional regression test to cover
> those cases), but I'm still not sure that having a user provided column list is
> supposed to be accepted or not for the HEADER MATCH. In the meantime I will
> add an open item.
>
>
I think it should, but a temporary alternative would be to forbid HEADER
MATCH with explicit column lists until we can make it work right.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-06-13 02:32 Julien Rouhaud <[email protected]>
parent: Andrew Dunstan <[email protected]>
0 siblings, 2 replies; 24+ messages in thread
From: Julien Rouhaud @ 2022-06-13 02:32 UTC (permalink / raw)
To: Andrew Dunstan <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Greg Stark <[email protected]>; Rémi Lapeyre <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; Michael Paquier <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Daniel Verite <[email protected]>
Hi,
On Sun, Jun 12, 2022 at 09:36:13AM -0400, Andrew Dunstan wrote:
>
> On 2022-06-07 Tu 11:47, Julien Rouhaud wrote:
> >
> > First, probably nitpicking, the HEADER MATCH is allowed for COPY TO, is that
> > expected? The documentation isn't really explicit about it, but there's
> > nothing to match when exporting data it's a bit surprising. I'm not opposed to
> > have HEADER MATCH means HEADER ON for COPY TO, as as-is one can easily reuse
> > the commands history, but maybe it should be clearly documented?
>
>
> I think it makes more sense to have a sanity check to prevent HEADER
> MATCH with COPY TO.
I'm fine with it. I added such a check and mentioned it in the documentation.
> > Then, apparently HEADER MATCH doesn't let you do sanity checks against a custom
> > column list. This one looks like a clear oversight, as something like that
> > should be entirely valid IMHO:
> >
> > CREATE TABLE tbl(col1 int, col2 int);
> > COPY tbl (col2, col1) TO '/path/to/file' WITH (HEADER MATCH);
> > COPY tbl (col2, col1) FROM '/path/to/file' WITH (HEADER MATCH);
> >
> > but right now it errors out with:
> >
> > ERROR: column name mismatch in header line field 1: got "col1", expected "col2"
> >
> > Note that the error message is bogus if you specify attributes in a
> > different order from the relation, as the code is mixing access to the tuple
> > desc and access to the raw fields with the same offset.
> > [...]
> I think it should, but a temporary alternative would be to forbid HEADER
> MATCH with explicit column lists until we can make it work right.
I think it would still be problematic if the target table has dropped columns.
Fortunately, as I initially thought the problem is only due to a thinko in the
original commit which used a wrong variable for the raw_fields offset. Once
fixed (attached v1) I didn't see any other problem in the rest of the logic and
all the added regression tests work as expected.
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-06-13 07:35 Peter Eisentraut <[email protected]>
parent: Julien Rouhaud <[email protected]>
1 sibling, 0 replies; 24+ messages in thread
From: Peter Eisentraut @ 2022-06-13 07:35 UTC (permalink / raw)
To: Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; +Cc: Greg Stark <[email protected]>; Rémi Lapeyre <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; Michael Paquier <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Daniel Verite <[email protected]>
On 13.06.22 04:32, Julien Rouhaud wrote:
>> I think it makes more sense to have a sanity check to prevent HEADER
>> MATCH with COPY TO.
>
> I'm fine with it. I added such a check and mentioned it in the documentation.
> I think it would still be problematic if the target table has dropped columns.
> Fortunately, as I initially thought the problem is only due to a thinko in the
> original commit which used a wrong variable for the raw_fields offset. Once
> fixed (attached v1) I didn't see any other problem in the rest of the logic and
> all the added regression tests work as expected.
Thanks for this patch. I'll check it in detail in a bit. It looks good
to me at first glance.
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-06-13 07:46 Michael Paquier <[email protected]>
parent: Julien Rouhaud <[email protected]>
1 sibling, 1 reply; 24+ messages in thread
From: Michael Paquier @ 2022-06-13 07:46 UTC (permalink / raw)
To: Julien Rouhaud <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Peter Eisentraut <[email protected]>; Greg Stark <[email protected]>; Rémi Lapeyre <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Daniel Verite <[email protected]>
On Mon, Jun 13, 2022 at 10:32:13AM +0800, Julien Rouhaud wrote:
> On Sun, Jun 12, 2022 at 09:36:13AM -0400, Andrew Dunstan wrote:
> I'm fine with it. I added such a check and mentioned it in the documentation.
An error looks like the right call at this stage of the game. I am
not sure what the combination of MATCH with COPY TO would mean,
actually. And with the concept of SELECT queries on top of it, the
whole idea gets blurrier.
> I think it would still be problematic if the target table has dropped columns.
> Fortunately, as I initially thought the problem is only due to a thinko in the
> original commit which used a wrong variable for the raw_fields offset. Once
> fixed (attached v1) I didn't see any other problem in the rest of the logic and
> all the added regression tests work as expected.
Interesting catch. One thing that I've always found useful when it
comes to tests that stress dropped columns is to have tests where we
reduce the number of total columns that still exist. An extra thing
is to look after ........pg.dropped.N........ a bit, and I would put
something in one of the headers.
> if (pg_strcasecmp(sval, "match") == 0)
> + {
> + /* match is only valid for COPY FROM */
> + if (!is_from)
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("%s match is only valid for COPY FROM",
> + def->defname)));
Some nits. I would suggest to reword this error message, like "cannot
use \"match\" with HEADER in COPY TO". There is no need for the extra
comment, and the error code should be ERRCODE_FEATURE_NOT_SUPPORTED.
--
Michael
Attachments:
[text/x-diff] v2-0001-Fix-processing-of-header-match-option-for-COPY.patch (7.6K, ../../[email protected]/2-v2-0001-Fix-processing-of-header-match-option-for-COPY.patch)
download | inline diff:
From af4e06ca5f0a13ad5922abae446bd6716bbdf3c1 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud <[email protected]>
Date: Mon, 13 Jun 2022 09:49:23 +0800
Subject: [PATCH v2] Fix processing of header match option for COPY.
Thinko in 072132f04en which used the attnum offset to access the raw_fields
array, leading to incorrect results of crash. Use the correct variable, and
add some regression tests to cover a bit more scenario for the HEADER MATCH
option.
While at it, disallow HEADER MATCH in COPY TO as there is no validation that
can be done in that case.
Author: Julien Rouhaud
Discussion: https://postgr.es/m/20220607154744.vvmitnqhyxrne5ms@jrouhaud
---
src/backend/commands/copy.c | 11 +++++--
src/backend/commands/copyfromparse.c | 5 ++--
src/test/regress/expected/copy.out | 43 ++++++++++++++++++++++++++-
src/test/regress/sql/copy.sql | 44 ++++++++++++++++++++++++++--
doc/src/sgml/ref/copy.sgml | 2 ++
5 files changed, 97 insertions(+), 8 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index f448d39c7e..e2870e3c11 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -318,7 +318,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
* defGetBoolean() but also accepts the special value "match".
*/
static CopyHeaderChoice
-defGetCopyHeaderChoice(DefElem *def)
+defGetCopyHeaderChoice(DefElem *def, bool is_from)
{
/*
* If no parameter given, assume "true" is meant.
@@ -360,7 +360,14 @@ defGetCopyHeaderChoice(DefElem *def)
if (pg_strcasecmp(sval, "off") == 0)
return COPY_HEADER_FALSE;
if (pg_strcasecmp(sval, "match") == 0)
+ {
+ if (!is_from)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot use \"%s\" with HEADER in COPY TO",
+ sval)));
return COPY_HEADER_MATCH;
+ }
}
break;
}
@@ -452,7 +459,7 @@ ProcessCopyOptions(ParseState *pstate,
if (header_specified)
errorConflictingDefElem(defel, pstate);
header_specified = true;
- opts_out->header_line = defGetCopyHeaderChoice(defel);
+ opts_out->header_line = defGetCopyHeaderChoice(defel, is_from);
}
else if (strcmp(defel->defname, "quote") == 0)
{
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index e06534943f..57813b3458 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -789,11 +789,12 @@ NextCopyFromRawFields(CopyFromState cstate, char ***fields, int *nfields)
foreach(cur, cstate->attnumlist)
{
int attnum = lfirst_int(cur);
- char *colName = cstate->raw_fields[attnum - 1];
+ char *colName;
Form_pg_attribute attr = TupleDescAttr(tupDesc, attnum - 1);
- fldnum++;
+ Assert(fldnum < cstate->max_fields);
+ colName = cstate->raw_fields[fldnum++];
if (colName == NULL)
ereport(ERROR,
(errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
diff --git a/src/test/regress/expected/copy.out b/src/test/regress/expected/copy.out
index e8d6b4fc13..7f2f4ae4ae 100644
--- a/src/test/regress/expected/copy.out
+++ b/src/test/regress/expected/copy.out
@@ -182,9 +182,21 @@ create table header_copytest (
b int,
c text
);
+-- Make sure it works with with dropped columns
+alter table header_copytest drop column c;
+alter table header_copytest add column c text;
+copy header_copytest to stdout with (header match);
+ERROR: cannot use "match" with HEADER in COPY TO
copy header_copytest from stdin with (header wrong_choice);
ERROR: header requires a Boolean value or "match"
+-- works
copy header_copytest from stdin with (header match);
+copy header_copytest (c, a, b) from stdin with (header match);
+copy header_copytest from stdin with (header match, format csv);
+-- errors
+copy header_copytest (c, b, a) from stdin with (header match);
+ERROR: column name mismatch in header line field 1: got "a", expected "c"
+CONTEXT: COPY header_copytest, line 1: "a b c"
copy header_copytest from stdin with (header match);
ERROR: column name mismatch in header line field 3: got null value ("\N"), expected "c"
CONTEXT: COPY header_copytest, line 1: "a b \N"
@@ -197,5 +209,34 @@ CONTEXT: COPY header_copytest, line 1: "a b c d"
copy header_copytest from stdin with (header match);
ERROR: column name mismatch in header line field 3: got "d", expected "c"
CONTEXT: COPY header_copytest, line 1: "a b d"
-copy header_copytest from stdin with (header match, format csv);
+SELECT * FROM header_copytest ORDER BY a;
+ a | b | c
+---+---+-----
+ 1 | 2 | foo
+ 3 | 4 | bar
+ 5 | 6 | baz
+(3 rows)
+
+-- Drop an extra column, in the middle of the existing set.
+alter table header_copytest drop column b;
+-- works
+copy header_copytest (c, a) from stdin with (header match);
+copy header_copytest (a, c) from stdin with (header match);
+-- errors
+copy header_copytest from stdin with (header match);
+ERROR: wrong number of fields in header line: field count is 3, expected 2
+CONTEXT: COPY header_copytest, line 1: "a ........pg.dropped.2........ c"
+copy header_copytest (a, c) from stdin with (header match);
+ERROR: wrong number of fields in header line: field count is 3, expected 2
+CONTEXT: COPY header_copytest, line 1: "a c b"
+SELECT * FROM header_copytest ORDER BY a;
+ a | c
+---+-----
+ 1 | foo
+ 3 | bar
+ 5 | baz
+ 7 | foo
+ 8 | foo
+(5 rows)
+
drop table header_copytest;
diff --git a/src/test/regress/sql/copy.sql b/src/test/regress/sql/copy.sql
index d72d226f34..285022e07c 100644
--- a/src/test/regress/sql/copy.sql
+++ b/src/test/regress/sql/copy.sql
@@ -204,11 +204,29 @@ create table header_copytest (
b int,
c text
);
+-- Make sure it works with with dropped columns
+alter table header_copytest drop column c;
+alter table header_copytest add column c text;
+copy header_copytest to stdout with (header match);
copy header_copytest from stdin with (header wrong_choice);
+-- works
copy header_copytest from stdin with (header match);
a b c
1 2 foo
\.
+copy header_copytest (c, a, b) from stdin with (header match);
+c a b
+bar 3 4
+\.
+copy header_copytest from stdin with (header match, format csv);
+a,b,c
+5,6,baz
+\.
+-- errors
+copy header_copytest (c, b, a) from stdin with (header match);
+a b c
+1 2 foo
+\.
copy header_copytest from stdin with (header match);
a b \N
1 2 foo
@@ -225,8 +243,28 @@ copy header_copytest from stdin with (header match);
a b d
1 2 foo
\.
-copy header_copytest from stdin with (header match, format csv);
-a,b,c
-1,2,foo
+SELECT * FROM header_copytest ORDER BY a;
+
+-- Drop an extra column, in the middle of the existing set.
+alter table header_copytest drop column b;
+-- works
+copy header_copytest (c, a) from stdin with (header match);
+c a
+foo 7
\.
+copy header_copytest (a, c) from stdin with (header match);
+a c
+8 foo
+\.
+-- errors
+copy header_copytest from stdin with (header match);
+a ........pg.dropped.2........ c
+1 2 foo
+\.
+copy header_copytest (a, c) from stdin with (header match);
+a c b
+1 foo 2
+\.
+
+SELECT * FROM header_copytest ORDER BY a;
drop table header_copytest;
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 40af423ccf..8aae711b3b 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -282,6 +282,8 @@ COPY { <replaceable class="parameter">table_name</replaceable> [ ( <replaceable
of the columns in the header line must match the actual column names of
the table, otherwise an error is raised.
This option is not allowed when using <literal>binary</literal> format.
+ The <literal>MATCH</literal> option is only valid for <command>COPY
+ FROM</command> commands.
</para>
</listitem>
</varlistentry>
--
2.36.1
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-06-14 09:13 Julien Rouhaud <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 2 replies; 24+ messages in thread
From: Julien Rouhaud @ 2022-06-14 09:13 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Peter Eisentraut <[email protected]>; Greg Stark <[email protected]>; Rémi Lapeyre <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Daniel Verite <[email protected]>
On Mon, Jun 13, 2022 at 04:46:46PM +0900, Michael Paquier wrote:
>
> Some nits. I would suggest to reword this error message, like "cannot
> use \"match\" with HEADER in COPY TO".
Agreed.
> There is no need for the extra
> comment, and the error code should be ERRCODE_FEATURE_NOT_SUPPORTED.
Is there any rule for what error code should be used?
Maybe that's just me but I understand "not supported" as "this makes sense, but
this is currently a limitation that might be lifted later".
Here I don't think it can ever make to use MATCH for a COPY TO, apart from
ignoring its meaning and accept it as an alias for HEADER ON. But if we don't
allow this loose alias now it would just cause trouble to later allow it so
having an invalid syntax or something like that sounds more suited.
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-06-15 09:14 Peter Eisentraut <[email protected]>
parent: Julien Rouhaud <[email protected]>
1 sibling, 0 replies; 24+ messages in thread
From: Peter Eisentraut @ 2022-06-15 09:14 UTC (permalink / raw)
To: Julien Rouhaud <[email protected]>; Michael Paquier <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Greg Stark <[email protected]>; Rémi Lapeyre <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; vignesh C <[email protected]>; pgsql-hackers; Daniel Verite <[email protected]>
On 14.06.22 11:13, Julien Rouhaud wrote:
>> There is no need for the extra
>> comment, and the error code should be ERRCODE_FEATURE_NOT_SUPPORTED.
> Is there any rule for what error code should be used?
>
> Maybe that's just me but I understand "not supported" as "this makes sense, but
> this is currently a limitation that might be lifted later".
I tend to agree with that interpretation.
Also, when you consider the way SQL rules and error codes are set up,
errors that are detected during parse analysis should be a subclass of
"syntax error or access rule violation".
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-06-15 11:50 Daniel Verite <[email protected]>
parent: Julien Rouhaud <[email protected]>
1 sibling, 1 reply; 24+ messages in thread
From: Daniel Verite @ 2022-06-15 11:50 UTC (permalink / raw)
To: Julien Rouhaud <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andrew Dunstan <[email protected]>; Peter Eisentraut <[email protected]>; Greg Stark <[email protected]>; =?ISO-8859-15?Q?R=E9mi_Lapeyre?= <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; vignesh C <[email protected]>; pgsql-hackers
Julien Rouhaud wrote:
> Maybe that's just me but I understand "not supported" as "this makes
> sense, but this is currently a limitation that might be lifted
> later".
Looking at ProcessCopyOptions(), there are quite a few invalid
combinations of options that produce
ERRCODE_FEATURE_NOT_SUPPORTED currently:
- HEADER in binary mode
- FORCE_QUOTE outside of csv
- FORCE_QUOTE outside of COPY TO
- FORCE_NOT_NULL outside of csv
- FORCE_NOT_NULL outside of COPY FROM
- ESCAPE outside of csv
- delimiter appearing in the NULL specification
- csv quote appearing in the NULL specification
FORCE_QUOTE and FORCE_NOT_NULL are options that only make sense in one
direction, so the errors when using these in the wrong direction are
comparable to the "HEADER MATCH outside of COPY FROM" error that we
want to add. In that sense, ERRCODE_FEATURE_NOT_SUPPORTED would be
consistent.
The other errors in the list above are more about the format itself,
with options that make sense only for one format. So the way we're
supposed to understand ERRCODE_FEATURE_NOT_SUPPORTED in these
other cases is that such format does not support such feature,
but without implying that it's a limitation.
Best regards,
--
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-06-16 07:24 Peter Eisentraut <[email protected]>
parent: Daniel Verite <[email protected]>
0 siblings, 1 reply; 24+ messages in thread
From: Peter Eisentraut @ 2022-06-16 07:24 UTC (permalink / raw)
To: Daniel Verite <[email protected]>; Julien Rouhaud <[email protected]>; +Cc: Michael Paquier <[email protected]>; Andrew Dunstan <[email protected]>; Greg Stark <[email protected]>; Rémi Lapeyre <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; vignesh C <[email protected]>; pgsql-hackers
On 15.06.22 13:50, Daniel Verite wrote:
> The other errors in the list above are more about the format itself,
> with options that make sense only for one format. So the way we're
> supposed to understand ERRCODE_FEATURE_NOT_SUPPORTED in these
> other cases is that such format does not support such feature,
> but without implying that it's a limitation.
I don't feel very strongly about this. It makes sense to stay
consistent with the existing COPY code.
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-06-20 00:03 Michael Paquier <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 24+ messages in thread
From: Michael Paquier @ 2022-06-20 00:03 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Daniel Verite <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Greg Stark <[email protected]>; Rémi Lapeyre <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; vignesh C <[email protected]>; pgsql-hackers
On Thu, Jun 16, 2022 at 09:24:56AM +0200, Peter Eisentraut wrote:
> I don't feel very strongly about this. It makes sense to stay consistent
> with the existing COPY code.
Yes, my previous argument is based on consistency with the
surroundings. I am not saying that this could not be made better, it
surely can, but I would recommend to tackle that in a separate patch,
and apply that to more areas than this specific one.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-06-21 23:34 Michael Paquier <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 24+ messages in thread
From: Michael Paquier @ 2022-06-21 23:34 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Daniel Verite <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Greg Stark <[email protected]>; Rémi Lapeyre <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; vignesh C <[email protected]>; pgsql-hackers
On Mon, Jun 20, 2022 at 09:03:23AM +0900, Michael Paquier wrote:
> On Thu, Jun 16, 2022 at 09:24:56AM +0200, Peter Eisentraut wrote:
>> I don't feel very strongly about this. It makes sense to stay consistent
>> with the existing COPY code.
>
> Yes, my previous argument is based on consistency with the
> surroundings. I am not saying that this could not be made better, it
> surely can, but I would recommend to tackle that in a separate patch,
> and apply that to more areas than this specific one.
Peter, beta2 is planned for next week. Do you think that you would be
able to address this open item by the end of this week? If not, and I
have already looked at the proposed patch, I can jump in and help.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-06-22 10:22 Peter Eisentraut <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 24+ messages in thread
From: Peter Eisentraut @ 2022-06-22 10:22 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Daniel Verite <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Greg Stark <[email protected]>; Rémi Lapeyre <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; vignesh C <[email protected]>; pgsql-hackers
On 22.06.22 01:34, Michael Paquier wrote:
> On Mon, Jun 20, 2022 at 09:03:23AM +0900, Michael Paquier wrote:
>> On Thu, Jun 16, 2022 at 09:24:56AM +0200, Peter Eisentraut wrote:
>>> I don't feel very strongly about this. It makes sense to stay consistent
>>> with the existing COPY code.
>>
>> Yes, my previous argument is based on consistency with the
>> surroundings. I am not saying that this could not be made better, it
>> surely can, but I would recommend to tackle that in a separate patch,
>> and apply that to more areas than this specific one.
>
> Peter, beta2 is planned for next week. Do you think that you would be
> able to address this open item by the end of this week? If not, and I
> have already looked at the proposed patch, I can jump in and help.
The latest patch was posted by you, so I was deferring to you to commit
it. Would you like me to do it?
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-06-22 11:00 Michael Paquier <[email protected]>
parent: Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 24+ messages in thread
From: Michael Paquier @ 2022-06-22 11:00 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Daniel Verite <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Greg Stark <[email protected]>; Rémi Lapeyre <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; vignesh C <[email protected]>; pgsql-hackers
On Wed, Jun 22, 2022 at 12:22:01PM +0200, Peter Eisentraut wrote:
> The latest patch was posted by you, so I was deferring to you to commit it.
> Would you like me to do it?
OK. As this is originally a feature you have committed, I originally
thought that you would take care of it, even if I sent a patch. I'll
handle that tomorrow then, if that's fine for you, of course. Happy
to help.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-06-23 04:26 Michael Paquier <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 1 reply; 24+ messages in thread
From: Michael Paquier @ 2022-06-23 04:26 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Daniel Verite <[email protected]>; Julien Rouhaud <[email protected]>; Andrew Dunstan <[email protected]>; Greg Stark <[email protected]>; Rémi Lapeyre <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; vignesh C <[email protected]>; pgsql-hackers
On Wed, Jun 22, 2022 at 08:00:15PM +0900, Michael Paquier wrote:
> OK. As this is originally a feature you have committed, I originally
> thought that you would take care of it, even if I sent a patch. I'll
> handle that tomorrow then, if that's fine for you, of course. Happy
> to help.
And done. Thanks.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 24+ messages in thread
* Re: Add header support to text format and matching feature
@ 2022-06-23 04:46 Julien Rouhaud <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 0 replies; 24+ messages in thread
From: Julien Rouhaud @ 2022-06-23 04:46 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Daniel Verite <[email protected]>; Andrew Dunstan <[email protected]>; Greg Stark <[email protected]>; Rémi Lapeyre <[email protected]>; Zhihong Yu <[email protected]>; David Steele <[email protected]>; vignesh C <[email protected]>; pgsql-hackers
On Thu, Jun 23, 2022 at 01:26:29PM +0900, Michael Paquier wrote:
> On Wed, Jun 22, 2022 at 08:00:15PM +0900, Michael Paquier wrote:
> > OK. As this is originally a feature you have committed, I originally
> > thought that you would take care of it, even if I sent a patch. I'll
> > handle that tomorrow then, if that's fine for you, of course. Happy
> > to help.
>
> And done. Thanks.
Thanks!
^ permalink raw reply [nested|flat] 24+ messages in thread
end of thread, other threads:[~2022-06-23 04:46 UTC | newest]
Thread overview: 24+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-01-28 16:42 [PATCH v3 5/5] Do COPY FROM encoding conversion/verification in larger chunks. Heikki Linnakangas <[email protected]>
2022-01-28 08:57 Re: Add header support to text format and matching feature Peter Eisentraut <[email protected]>
2022-01-30 22:56 ` Re: Add header support to text format and matching feature Rémi Lapeyre <[email protected]>
2022-01-31 06:54 ` Re: Add header support to text format and matching feature Peter Eisentraut <[email protected]>
2022-02-03 06:37 ` Re: Add header support to text format and matching feature Peter Eisentraut <[email protected]>
2022-03-25 04:21 ` Re: Add header support to text format and matching feature Greg Stark <[email protected]>
2022-03-29 15:02 ` Re: Add header support to text format and matching feature Peter Eisentraut <[email protected]>
2022-03-30 07:11 ` Re: Add header support to text format and matching feature Peter Eisentraut <[email protected]>
2022-06-07 15:47 ` Re: Add header support to text format and matching feature Julien Rouhaud <[email protected]>
2022-06-12 13:36 ` Re: Add header support to text format and matching feature Andrew Dunstan <[email protected]>
2022-06-13 02:32 ` Re: Add header support to text format and matching feature Julien Rouhaud <[email protected]>
2022-06-13 07:35 ` Re: Add header support to text format and matching feature Peter Eisentraut <[email protected]>
2022-06-13 07:46 ` Re: Add header support to text format and matching feature Michael Paquier <[email protected]>
2022-06-14 09:13 ` Re: Add header support to text format and matching feature Julien Rouhaud <[email protected]>
2022-06-15 09:14 ` Re: Add header support to text format and matching feature Peter Eisentraut <[email protected]>
2022-06-15 11:50 ` Re: Add header support to text format and matching feature Daniel Verite <[email protected]>
2022-06-16 07:24 ` Re: Add header support to text format and matching feature Peter Eisentraut <[email protected]>
2022-06-20 00:03 ` Re: Add header support to text format and matching feature Michael Paquier <[email protected]>
2022-06-21 23:34 ` Re: Add header support to text format and matching feature Michael Paquier <[email protected]>
2022-06-22 10:22 ` Re: Add header support to text format and matching feature Peter Eisentraut <[email protected]>
2022-06-22 11:00 ` Re: Add header support to text format and matching feature Michael Paquier <[email protected]>
2022-06-23 04:26 ` Re: Add header support to text format and matching feature Michael Paquier <[email protected]>
2022-06-23 04:46 ` Re: Add header support to text format and matching feature Julien Rouhaud <[email protected]>
2022-03-25 12:02 ` Re: Add header support to text format and matching feature Daniel Verite <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox