public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v5 2/2] Do COPY FROM encoding conversion/verification in larger chunks.
2+ messages / 2 participants
[nested] [flat]
* [PATCH v5 2/2] Do COPY FROM encoding conversion/verification in larger chunks.
@ 2021-02-09 17:10 Heikki Linnakangas <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: Heikki Linnakangas @ 2021-02-09 17:10 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 | 80 ++--
src/backend/commands/copyfromparse.c | 519 +++++++++++++++++------
src/include/commands/copyfrom_internal.h | 62 +--
src/include/mb/pg_wchar.h | 22 +-
4 files changed, 500 insertions(+), 183 deletions(-)
diff --git a/src/backend/commands/copyfrom.c b/src/backend/commands/copyfrom.c
index 796ca7b3f7b..9c6d7d7f21a 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"
@@ -87,7 +94,7 @@ typedef struct CopyMultiInsertInfo
List *multiInsertBuffers; /* List of tracked CopyMultiInsertBuffers */
int bufferedTuples; /* number of tuples buffered over all buffers */
int bufferedBytes; /* number of bytes from all buffered tuples */
- CopyFromState cstate; /* Copy state for this CopyMultiInsertInfo */
+ CopyFromState cstate; /* Copy state for this CopyMultiInsertInfo */
EState *estate; /* Executor state used for COPY */
CommandId mycid; /* Command Id used for COPY */
int ti_options; /* table insert options */
@@ -107,7 +114,7 @@ static void ClosePipeFromProgram(CopyFromState cstate);
void
CopyFromErrorCallback(void *arg)
{
- CopyFromState cstate = (CopyFromState) arg;
+ CopyFromState cstate = (CopyFromState) arg;
char curlineno_str[32];
snprintf(curlineno_str, sizeof(curlineno_str), UINT64_FORMAT,
@@ -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;
@@ -300,7 +301,7 @@ CopyMultiInsertBufferFlush(CopyMultiInsertInfo *miinfo,
MemoryContext oldcontext;
int i;
uint64 save_cur_lineno;
- CopyFromState cstate = miinfo->cstate;
+ CopyFromState cstate = miinfo->cstate;
EState *estate = miinfo->estate;
CommandId mycid = miinfo->mycid;
int ti_options = miinfo->ti_options;
@@ -1187,7 +1188,7 @@ BeginCopyFrom(ParseState *pstate,
List *attnamelist,
List *options)
{
- CopyFromState cstate;
+ CopyFromState cstate;
bool pipe = (filename == NULL);
TupleDesc tupDesc;
AttrNumber num_phys_attrs,
@@ -1215,7 +1216,7 @@ BeginCopyFrom(ParseState *pstate,
oldcontext = MemoryContextSwitchTo(cstate->copycontext);
/* Extract options from the statement node tree */
- ProcessCopyOptions(pstate, &cstate->opts, true /* is_from */, options);
+ ProcessCopyOptions(pstate, &cstate->opts, true /* is_from */ , options);
/* Process the target relation */
cstate->rel = rel;
@@ -1306,15 +1307,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 */
@@ -1325,7 +1331,6 @@ BeginCopyFrom(ParseState *pstate,
oldcontext = MemoryContextSwitchTo(cstate->copycontext);
/* Initialize state variables */
- cstate->reached_eof = false;
cstate->eol_type = EOL_UNKNOWN;
cstate->cur_relname = RelationGetRelationName(cstate->rel);
cstate->cur_lineno = 0;
@@ -1333,19 +1338,36 @@ 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 raw_buf are used in both text and binary modes, but
+ * input_buf and line_buf only in text mode.
*/
- initStringInfo(&cstate->attribute_buf);
- cstate->raw_buf = (char *) palloc(RAW_BUF_SIZE + 1);
+ cstate->raw_buf = palloc(RAW_BUF_SIZE + 1);
cstate->raw_buf_index = cstate->raw_buf_len = 0;
+ cstate->raw_reached_eof = false;
+
if (!cstate->opts.binary)
{
+ /*
+ * If encoding conversion is needed, we need another buffer to hold
+ * the converted input data. Otherwise, we can just point input_buf
+ * to the same buffer as raw_buf.
+ */
+ if (cstate->need_transcoding)
+ {
+ cstate->input_buf = (char *) palloc(INPUT_BUF_SIZE + 1);
+ cstate->input_buf_index = cstate->input_buf_len = 0;
+ }
+ else
+ cstate->input_buf = cstate->raw_buf;
+ cstate->input_reached_eof = false;
+
initStringInfo(&cstate->line_buf);
- cstate->line_buf_converted = false;
}
+ initStringInfo(&cstate->attribute_buf);
+
/* Assign range table, we'll need it in CopyFrom. */
if (pstate)
cstate->range_table = pstate->p_rtable;
@@ -1564,7 +1586,7 @@ ClosePipeFromProgram(CopyFromState cstate)
* should not report that as an error. Otherwise, SIGPIPE indicates a
* problem.
*/
- if (!cstate->reached_eof &&
+ if (!cstate->raw_reached_eof &&
wait_result_is_signal(pclose_rc, SIGPIPE))
return;
diff --git a/src/backend/commands/copyfromparse.c b/src/backend/commands/copyfromparse.c
index 315b16fd7af..62164e32a3d 100644
--- a/src/backend/commands/copyfromparse.c
+++ b/src/backend/commands/copyfromparse.c
@@ -3,6 +3,49 @@
* 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. CopyConvertBuf() 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 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'. CopyConvertBuf() then merely validates that
+ * the data is valid in the current encoding.
+ *
+ * In binary mode, the pipeline is much simpler. Input is loaded into
+ * into 'raw_buf', and encoding conversion is done in the datatype-specific
+ * receive functions, if required. 'input_buf' and 'line_buf' are not used,
+ * but 'attribute_buf' is used as a temporary buffer to hold one attribute's
+ * data when it's passed the receive function.
+ *
+ * 'raw_buf' is always 64 kB in size (RAW_BUF_SIZE). 'input_buf' is also
+ * 64 kB (INPUT_BUF_SIZE), 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 +78,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 +96,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 +108,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 +120,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 +138,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 +161,7 @@ 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 void CopyLoadInputBuf(CopyFromState cstate);
static int CopyReadBinaryData(CopyFromState cstate, char *dest, int nbytes);
void
@@ -226,7 +269,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread)
(errcode_for_file_access(),
errmsg("could not read from COPY file: %m")));
if (bytesread == 0)
- cstate->reached_eof = true;
+ cstate->raw_reached_eof = true;
break;
case COPY_OLD_FE:
@@ -247,7 +290,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread)
bytesread = minread;
break;
case COPY_NEW_FE:
- while (maxread > 0 && bytesread < minread && !cstate->reached_eof)
+ while (maxread > 0 && bytesread < minread && !cstate->raw_reached_eof)
{
int avail;
@@ -275,7 +318,7 @@ CopyGetData(CopyFromState cstate, void *databuf, int minread, int maxread)
break;
case 'c': /* CopyDone */
/* COPY IN correctly terminated by frontend */
- cstate->reached_eof = true;
+ cstate->raw_reached_eof = true;
return bytesread;
case 'f': /* CopyFail */
ereport(ERROR,
@@ -361,34 +404,303 @@ CopyGetInt16(CopyFromState cstate, int16 *val)
/*
- * CopyLoadRawBuf loads some more data into raw_buf
+ * Perform encoding conversion on data in 'raw_buf', writing the converted
+ * data into 'input_buf'.
*
- * Returns true if able to obtain at least one more byte, else false.
+ * On entry, there must be some data to convert in 'raw_buf'.
+ */
+static void
+CopyConvertBuf(CopyFromState cstate)
+{
+ /*
+ * If the file and server encoding are the same, no encoding conversion is
+ * required. However, we still need to verify that the input is valid for
+ * the encoding.
+ */
+ if (!cstate->need_transcoding)
+ {
+ /*
+ * When conversion is not required, input_buf and raw_buf are the
+ * same. raw_buf_len is the total number of bytes in the buffer, and
+ * input_buf_len tracks how many of those bytes have already been
+ * verified.
+ */
+ int preverifiedlen = cstate->input_buf_len;
+ int unverifiedlen = cstate->raw_buf_len - cstate->input_buf_len;
+ int nverified;
+
+ if (unverifiedlen == 0)
+ {
+ /*
+ * If no more raw data is coming, report the EOF to the caller.
+ */
+ if (cstate->raw_reached_eof)
+ cstate->input_reached_eof = true;
+ return;
+ }
+
+ /*
+ * Verify the new data, including any residual unverified bytes from
+ * previous round.
+ */
+ nverified = pg_encoding_verifymbstr(cstate->file_encoding,
+ cstate->raw_buf + preverifiedlen,
+ unverifiedlen);
+ if (nverified == 0)
+ {
+ /*
+ * Could not verify anything.
+ *
+ * If there is no more raw input data coming, it means that there
+ * was an incomplete multi-byte sequence at the end. Also, if
+ * there's "enough" input left, we should be able to verify at
+ * least one character, and a failure to do so means that we've
+ * hit an invalid byte sequence.
+ */
+ if (cstate->raw_reached_eof || unverifiedlen >= pg_database_encoding_max_length())
+ cstate->input_reached_error = true;
+ return;
+ }
+ cstate->input_buf_len += nverified;
+ }
+ else
+ {
+ /*
+ * Encoding conversion is needed.
+ */
+ int nbytes;
+ unsigned char *src;
+ int srclen;
+ unsigned char *dst;
+ int dstlen;
+ int convertedlen;
+
+ if (RAW_BUF_BYTES(cstate) == 0)
+ {
+ /*
+ * If no more raw data is coming, report the EOF to the caller.
+ */
+ if (cstate->raw_reached_eof)
+ cstate->input_reached_eof = true;
+ return;
+ }
+
+ /*
+ * First, copy down any unprocessed data.
+ */
+ 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;
+ cstate->input_buf[nbytes] = '\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.
+ */
+ convertedlen = pg_do_encoding_conversion_buf(cstate->conversion_proc,
+ cstate->file_encoding,
+ GetDatabaseEncoding(),
+ src, srclen,
+ dst, dstlen,
+ true);
+ if (convertedlen == 0)
+ {
+ /*
+ * Could not convert anything. If there is no more raw input data
+ * coming, it means that there was an incomplete multi-byte
+ * sequence at the end. Also, if there is plenty of input left,
+ * we should be able to convert at least one character, so a
+ * failure to do so must mean that we've hit a byte sequence
+ * that's invalid.
+ */
+ if (cstate->raw_reached_eof || srclen >= MAX_CONVERSION_INPUT_LENGTH)
+ cstate->input_reached_error = true;
+ return;
+ }
+ cstate->raw_buf_index += convertedlen;
+ cstate->input_buf_len += strlen((char *) dst);
+ }
+}
+
+/*
+ * Report an encoding or conversion error.
+ */
+static void
+CopyConversionError(CopyFromState cstate)
+{
+ Assert(cstate->raw_buf_len > 0);
+ Assert(cstate->input_reached_error);
+
+ if (!cstate->need_transcoding)
+ {
+ /*
+ * Everything up to input_buf_len was successfully verified, and
+ * input_buf_len points to the invalid or incomplete character.
+ */
+ report_invalid_encoding(cstate->file_encoding,
+ cstate->raw_buf + cstate->input_buf_len,
+ cstate->raw_buf_len - cstate->input_buf_len);
+ }
+ else
+ {
+ /*
+ * raw_buf_index points to the invalid or untranslatable character. We
+ * let the conversion routine report the error, because it can provide
+ * a more specific error message than we could here. An earlier call
+ * to the conversion routine in CopyConvertBuf() detected that there
+ * is an error, now we call the conversion routine again with
+ * noError=false, to have it throw the error.
+ */
+ unsigned char *src;
+ int srclen;
+ unsigned char *dst;
+ int dstlen;
+
+ 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;
+
+ (void) pg_do_encoding_conversion_buf(cstate->conversion_proc,
+ cstate->file_encoding,
+ GetDatabaseEncoding(),
+ src, srclen,
+ dst, dstlen,
+ false);
+
+ /*
+ * The conversion routine should have reported an error, so this
+ * should not be reached.
+ */
+ elog(ERROR, "encoding conversion failed without error");
+ }
+}
+
+/*
+ * Load more data from data source to raw_buf.
*
* 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
+static void
CopyLoadRawBuf(CopyFromState cstate)
{
- int nbytes = RAW_BUF_BYTES(cstate);
+ int nbytes;
int inbytes;
- /* Copy down the unprocessed data if any. */
- if (nbytes > 0)
+ /*
+ * In text mode, if encoding conversion is not required, raw_buf and
+ * input_buf point to the same buffer. Their len/index better agree, too.
+ */
+ 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.
+ */
+ 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;
+ }
- inbytes = CopyGetData(cstate, cstate->raw_buf + nbytes,
- 1, RAW_BUF_SIZE - nbytes);
+ /* Load more data */
+ inbytes = CopyGetData(cstate, cstate->raw_buf + cstate->raw_buf_len,
+ 1, RAW_BUF_SIZE - cstate->raw_buf_len);
nbytes += inbytes;
cstate->raw_buf[nbytes] = '\0';
- cstate->raw_buf_index = 0;
cstate->raw_buf_len = nbytes;
+
cstate->bytes_processed += inbytes;
pgstat_progress_update_param(PROGRESS_COPY_BYTES_PROCESSED, cstate->bytes_processed);
- return (inbytes > 0);
+
+ if (inbytes == 0)
+ cstate->raw_reached_eof = true;
+}
+
+/*
+ * CopyLoadInputBuf loads some more data into input_buf
+ *
+ * On return, at least one more input character is loaded into
+ * input_buf, or input_reached_eof is set.
+ *
+ * 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 void
+CopyLoadInputBuf(CopyFromState cstate)
+{
+ int nbytes = INPUT_BUF_BYTES(cstate);
+
+ /*
+ * The caller has updated input_buf_index to indicate how much of the
+ * input has been consumed and isn't needed anymore. If input_buf is the
+ * same physical area as raw_buf, updated raw_buf_index accordingly.
+ */
+ if (cstate->raw_buf == cstate->input_buf)
+ {
+ Assert(!cstate->need_transcoding);
+ Assert(cstate->input_buf_index >= cstate->raw_buf_index);
+ cstate->raw_buf_index = cstate->input_buf_index;
+ }
+
+ for (;;)
+ {
+ /* If we now have some unconverted data, try to convert it. */
+ CopyConvertBuf(cstate);
+
+ /* If we now have some more input bytes ready, return them */
+ if (INPUT_BUF_BYTES(cstate) > nbytes)
+ return;
+
+ /*
+ * If we reached an invalid byte sequence, or we're at an incomplete
+ * multi-byte character but there is no more raw input data, report
+ * conversion error.
+ */
+ if (cstate->input_reached_error)
+ CopyConversionError(cstate);
+
+ /* no more input, and everything has been converted */
+ if (cstate->input_reached_eof)
+ break;
+
+ /* Try to load more raw data */
+ Assert(!cstate->raw_reached_eof);
+ CopyLoadRawBuf(cstate);
+ }
}
/*
@@ -423,7 +735,8 @@ CopyReadBinaryData(CopyFromState cstate, char *dest, int nbytes)
/* Load more data if buffer is empty. */
if (RAW_BUF_BYTES(cstate) == 0)
{
- if (!CopyLoadRawBuf(cstate))
+ CopyLoadRawBuf(cstate);
+ if (cstate->raw_reached_eof)
break; /* EOF */
}
@@ -684,8 +997,7 @@ NextCopyFrom(CopyFromState cstate, ExprContext *econtext,
}
/*
- * Read the next input line and stash it in line_buf, with conversion to
- * server encoding.
+ * Read the next input line and stash it in line_buf.
*
* Result is true if read was terminated by EOF, false if terminated
* by newline. The terminating newline or EOF marker is not included
@@ -697,10 +1009,7 @@ CopyReadLine(CopyFromState cstate)
bool result;
resetStringInfo(&cstate->line_buf);
- cstate->line_buf_valid = true;
-
- /* Mark that encoding conversion hasn't occurred yet */
- cstate->line_buf_converted = false;
+ cstate->line_buf_valid = false;
/* Parse data and transfer into line_buf */
result = CopyReadLineText(cstate);
@@ -714,10 +1023,17 @@ 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);
+ cstate->input_buf_index = 0;
+ cstate->input_buf_len = 0;
+ cstate->raw_buf_index = 0;
+ cstate->raw_buf_len = 0;
}
}
else
@@ -754,25 +1070,8 @@ 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;
+ cstate->line_buf_valid = true;
return result;
}
@@ -783,13 +1082,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 +1105,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 +1117,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
- * have been determined to be part of the line, but not yet transferred to
- * line_buf.
+ * 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 a little extra speed within the loop, we copy raw_buf and
- * raw_buf_len into local variables.
+ * 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 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,24 +1152,21 @@ 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.
- */
- if (!CopyLoadRawBuf(cstate))
- hit_eof = true;
- raw_buf_ptr = 0;
- copy_buf_len = cstate->raw_buf_len;
+ CopyLoadInputBuf(cstate);
+ /* update our local variables */
+ hit_eof = cstate->input_reached_eof;
+ input_buf_ptr = cstate->input_buf_index;
+ copy_buf_len = cstate->input_buf_len;
/*
* If we are completely out of data, break out of the loop,
* reporting EOF.
*/
- if (copy_buf_len <= 0)
+ if (INPUT_BUF_BYTES(cstate) <= 0)
{
result = true;
break;
@@ -875,8 +1175,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 +1230,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
@@ -1006,14 +1306,14 @@ CopyReadLineText(CopyFromState cstate)
/* -----
* get next character
* Note: we do not change c so if it isn't \., we can fall
- * through and continue processing for file encoding.
+ * through and continue processing.
* -----
*/
- 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 +1325,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 +1350,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 +1375,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;
}
@@ -1095,15 +1395,8 @@ CopyReadLineText(CopyFromState cstate)
* backslashes are not special, so we want to process the
* character after the backslash just like a normal character,
* so we don't increment in those cases.
- *
- * Set 'c' to skip whole character correctly in multi-byte
- * encodings. If we don't have the whole character in the
- * buffer yet, we might loop back to process it, after all,
- * but that's OK because multi-byte characters cannot have any
- * special meaning.
*/
- raw_buf_ptr++;
- c = c2;
+ input_buf_ptr++;
}
}
@@ -1114,30 +1407,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..5c110ce5b92 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
{
@@ -71,13 +60,11 @@ typedef struct CopyFromStateData
CopySource copy_src; /* type of copy source */
FILE *copy_file; /* used if copy_src == COPY_FILE */
StringInfo fe_msgbuf; /* used if copy_src == COPY_NEW_FE */
- bool reached_eof; /* true if we read to end of copy data (not
- * all copy_src types maintain this) */
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 +119,52 @@ 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
- * sufficiently to locate line boundaries, then transfers the data to
- * line_buf and converts it. 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].
+ * 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. In binary mode,
+ * CopyReadBinaryData fetches appropriate amounts of data from this
+ * buffer. In both modes, we guarantee that there is a \0 at
+ * input_buf[input_buf_len].
+ */
+#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 */
+ bool input_reached_eof; /* true if we reached EOF */
+ bool input_reached_error; /* true if a conversion error happened */
+ /* 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. Like with
+ * 'input_buf', we guarantee that there is a \0 at raw_buf[raw_buf_len].
+ *
+ * If the encoding conversion is not required, the input data is read
+ * directly into 'input_buf', and raw_buf is not used. In that case,
+ * input_buf_len tracks the number of bytes verified to be valid in the
+ * encoding, and raw_buf_len is the total # of bytes stored in the buffer.
*/
#define RAW_BUF_SIZE 65536 /* we palloc RAW_BUF_SIZE+1 bytes */
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 */
+ bool raw_reached_eof; /* true if we reached EOF */
+
/* 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/include/mb/pg_wchar.h b/src/include/mb/pg_wchar.h
index bbce9071dfc..2377934faf6 100644
--- a/src/include/mb/pg_wchar.h
+++ b/src/include/mb/pg_wchar.h
@@ -306,15 +306,33 @@ typedef enum pg_enc
/*
* When converting strings between different encodings, we assume that space
- * for converted result is 4-to-1 growth in the worst case. The rate for
+ * for converted result is 4-to-1 growth in the worst case. The rate for
* currently supported encoding pairs are within 3 (SJIS JIS X0201 half width
- * kanna -> UTF8 is the worst case). So "4" should be enough for the moment.
+ * kana -> UTF8 is the worst case). So "4" should be enough for the moment.
*
* Note that this is not the same as the maximum character width in any
* particular encoding.
*/
#define MAX_CONVERSION_GROWTH 4
+/*
+ * Maximum byte length of a string that's required in any encoding to convert
+ * at least one character to any other encoding. In other words, if you feed
+ * MAX_CONVERSION_INPUT_LENGTH bytes to any encoding conversion function, it
+ * is guaranteed to be able to convert something without needing more input
+ * (assuming the input is valid).
+ *
+ * Currently, the maximum case is the conversion UTF8 -> SJIS JIS X0201 half
+ * width kana, where a pair of UTF-8 characters is converted into a single
+ * SHIFT_JIS_2004 character (the reverse of the worst case for
+ * MAX_CONVERSION_GROWTH). It needs 6 bytes of input. In theory, a
+ * user-defined conversion function might have more complicated cases, although
+ * for the reverse mapping you would probably also need to bump up
+ * MAX_CONVERSION_GROWTH. But there is no need to be stingy here, so make it
+ * generous.
+ */
+#define MAX_CONVERSION_INPUT_LENGTH 16
+
/*
* Maximum byte length of the string equivalent to any one Unicode code point,
* in any backend encoding. The current value assumes that a 4-byte UTF-8
--
2.30.0
--------------8EEAEFC18638D25F73368AAC--
^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: PoC: VALGRIND_MAKE_MEM_NOACCESS for dynamic shared memory
@ 2026-07-08 00:31 Andreas Karlsson <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: Andreas Karlsson @ 2026-07-08 00:31 UTC (permalink / raw)
To: Tomas Vondra <[email protected]>; +Cc: Andres Freund <[email protected]>; pgsql-hackers
Hi!
This looks useful so I took a stab at continuing this work plus adding
NOACCESS sentinels to shared memory allocated in the main segment with
the ShmemAllocator.
For the ShmemAllocator I had originally planned to mark all memory as
NOACCESS and then have ShmemAllocRaw() mark only the allocated regions
as accessible but that did not work with legacy allocations (e.g.
calling ShmemAlloc() directly) and EXEC_BAKCEND as there is no way to
know where those regions were as they are not in ShmemIndex. I
personally think the solution I went with instead, of jsut adding 32
NOACESS bytes at the end of each allocation in shared memory is good
enough even if it does not cover all padding bytes like my old solution
used to do.
What do you think? Is the current solution good enough? I preferred my
original idea of starting out with everything NOACCESS but EXEC_BAKCEND
made that painful to implement.
For shm_toc I fixed the issues pointed out be Andres (the padding issue
and that we can have shm_toc_lookup() set NOACCESS) and made sure we
clear all NOACCESS sentinels on dsm_deatch(). The clearing of the
NOACCESS in dsm_detach() felt a bit ugly but I am not sure there is any
better solution.
I have also attached a third patch which contains my own extension which
I used during development in case anyone would want to use it too. It
adds three functions which can do out of bounds access to an array of 10
32-bit integers. For example the function call below triggers a Valgrind
error when called with an index > 9.
SELECT valgrind_shmem_shm_toc_fg_get(10);
The extension is just a quick and dirty hack I used to test things out,
and nothing I think should get committed as we do not have any test
cases for Valgrind for anything else either.
On 5/4/26 16:21, Tomas Vondra wrote:
>> I assume the issue is just that the workers don't have the NOACCESS markers? I
>> think you'd need to do them in every process using the shm_toc. Either by
>> doing it in shm_toc_attach() or in shm_toc().
I do not think you can do it in shm_toc_attach() because there can be
new allocations between shm_toc_attach() and shm_toc_lookup() so my
patch adds the NOACESS marker in shm_toc_lookup() which should be safe.
So NOACCESS markers are added in both shm_toc_allocate() and
shm_toc_lookup() in my patch.
Sadly it cannot take alignment correctly into account since we do not
save that in the TOC (see below).
> Right, but it'd also need to know how long the entry is. Which AFAIK it
> does not. We don't want to redo the calculation in every worker, but we
> might add a "len" field to the toc entry.
Not without changing the API and unifying shm_toc_allocate() and
shm_toc_insert() since by the time we create a TOC entry we have thrown
away the size of the allocation. I would not mind changing this API but
I think I would need to understand why it is like that before changing
it. It does not feel good to change an API just for Valgrind.
Any idea why these two functions are split and why they are not just one
function call?
>> Which means that you'll often have a up to 32byte pad at the end of the
>> (ALIGNOF_BUFFER=32) allocation already. I don't care about the waste, but the
>> ALIGNOF_BUFFER padding will often prevent detecting smaller out-of-bounds
>> accesses.
>
> Good point. I forgot about this alignment rounding. I don't care about
> the waste either (it'd be a bit silly in a valgrind build anyway), but
> not catching smaller mistakes would not be great.
My version fixes this, but is not able to fix this in shm_toc_lookup()
as I mentioned above due to the lack of a length field for the
allocation in the TOC.
--
Andreas Karlsson
Percona
Attachments:
[text/x-patch] v2-0001-valgrind-Add-NOACCESS-sentinels-for-shm_toc-entri.patch (5.0K, ../../[email protected]/2-v2-0001-valgrind-Add-NOACCESS-sentinels-for-shm_toc-entri.patch)
download | inline diff:
From 1b6436a1aaabbc4e9d50f7dd49f42ad6d8614537 Mon Sep 17 00:00:00 2001
From: Tomas Vondra <[email protected]>
Date: Sat, 2 May 2026 23:09:00 +0200
Subject: [PATCH v2 1/3] valgrind: Add NOACCESS sentinels for shm_toc entries
When built with USE_VALGRIND add a couple NOACCESS bytes after each
entry allocated in a shm_toc from the segment. Since markings are not
shared between processes they are either set when a process allocates or
when a process looks up an allocation in the toc.
But as we have not saved the size of an allocation in the toc we need
to, on lookup, base where we set the sentinel on where the start of the
following allocation is so any padding will cause a gap before the
sentinel. This is far from ideal but better than nothing.
We also need to make sure to clear all NOACCESS bytes when detaching
from a DSM segment since a shm_toc does not know when it was destroyed
or deatched.
Author: Tomas Vondra <[email protected]>
Author: Andreas Karlsson <[email protected]>
---
src/backend/storage/ipc/dsm.c | 7 ++++++
src/backend/storage/ipc/shm_toc.c | 39 +++++++++++++++++++++++++++----
2 files changed, 42 insertions(+), 4 deletions(-)
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index 8b69df4ff26..f447d1543e3 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -45,6 +45,7 @@
#include "storage/shmem.h"
#include "storage/subsystems.h"
#include "utils/freepage.h"
+#include "utils/memdebug.h"
#include "utils/memutils.h"
#include "utils/resowner.h"
@@ -846,6 +847,12 @@ dsm_detach(dsm_segment *seg)
*/
if (seg->mapped_address != NULL)
{
+ /*
+ * We need to clear up NOACCESS regions set by shm_toc as shm_tocs
+ * have no function for deatching or destroying.
+ */
+ VALGRIND_MAKE_MEM_DEFINED(seg->mapped_address, seg->mapped_size);
+
if (!is_main_region_dsm_handle(seg->handle))
dsm_impl_op(DSM_OP_DETACH, seg->handle, 0, &seg->impl_private,
&seg->mapped_address, &seg->mapped_size, WARNING);
diff --git a/src/backend/storage/ipc/shm_toc.c b/src/backend/storage/ipc/shm_toc.c
index 2f9fbb0a519..cf240c482c6 100644
--- a/src/backend/storage/ipc/shm_toc.c
+++ b/src/backend/storage/ipc/shm_toc.c
@@ -16,6 +16,7 @@
#include "port/atomics.h"
#include "storage/shm_toc.h"
#include "storage/spin.h"
+#include "utils/memdebug.h"
typedef struct shm_toc_entry
{
@@ -74,6 +75,13 @@ shm_toc_attach(uint64 magic, void *address)
return toc;
}
+/* With valgrind, we want to add a couple NOACCESS bytes */
+#ifdef USE_VALGRIND
+#define NUM_NOACCESS_BYTES 32
+#else
+#define NUM_NOACCESS_BYTES 0
+#endif
+
/*
* Allocate shared memory from a segment managed by a table of contents.
*
@@ -88,6 +96,7 @@ void *
shm_toc_allocate(shm_toc *toc, Size nbytes)
{
volatile shm_toc *vtoc = toc;
+ Size reqbytes;
Size total_bytes;
Size allocated_bytes;
Size nentry;
@@ -99,7 +108,7 @@ shm_toc_allocate(shm_toc *toc, Size nbytes)
* proper definition for the minimum to make atomic ops safe, but
* BUFFERALIGN ought to be enough.
*/
- nbytes = BUFFERALIGN(nbytes);
+ reqbytes = BUFFERALIGN(nbytes + NUM_NOACCESS_BYTES);
SpinLockAcquire(&toc->toc_mutex);
@@ -110,18 +119,24 @@ shm_toc_allocate(shm_toc *toc, Size nbytes)
+ allocated_bytes;
/* Check for memory exhaustion and overflow. */
- if (toc_bytes + nbytes > total_bytes || toc_bytes + nbytes < toc_bytes)
+ if (toc_bytes + reqbytes > total_bytes || toc_bytes + reqbytes < toc_bytes)
{
SpinLockRelease(&toc->toc_mutex);
ereport(ERROR,
(errcode(ERRCODE_OUT_OF_MEMORY),
errmsg("out of shared memory")));
}
- vtoc->toc_allocated_bytes += nbytes;
+ vtoc->toc_allocated_bytes += reqbytes;
SpinLockRelease(&toc->toc_mutex);
- return ((char *) toc) + (total_bytes - allocated_bytes - nbytes);
+#ifdef USE_VALGRIND
+ /* Make the bytes at the end no-access */
+ VALGRIND_MAKE_MEM_NOACCESS(((char *) toc) + (total_bytes - allocated_bytes - reqbytes + nbytes),
+ reqbytes - nbytes);
+#endif
+
+ return ((char *) toc) + (total_bytes - allocated_bytes - reqbytes);
}
/*
@@ -252,7 +267,20 @@ shm_toc_lookup(shm_toc *toc, uint64 key, bool noError)
for (i = 0; i < nentry; ++i)
{
if (toc->toc_entry[i].key == key)
+ {
+#ifdef USE_VALGRIND
+ /*
+ * Since we do not know the size of entries we can only use the
+ * start of the next entry for setting the no-access sentinel.
+ */
+ Size nextoffset = i == 0 ? toc->toc_total_bytes : toc->toc_entry[i - 1].offset;
+
+ VALGRIND_MAKE_MEM_NOACCESS(((char *) toc) + nextoffset - NUM_NOACCESS_BYTES,
+ NUM_NOACCESS_BYTES);
+#endif
+
return ((char *) toc) + toc->toc_entry[i].offset;
+ }
}
/* No matching entry was found. */
@@ -275,5 +303,8 @@ shm_toc_estimate(shm_toc_estimator *e)
sz = add_size(sz, mul_size(e->number_of_keys, sizeof(shm_toc_entry)));
sz = add_size(sz, e->space_for_chunks);
+ /* add space for ENOACCESS bytes, NUM_NOACCESS_BYTES per key */
+ sz = add_size(sz, mul_size(e->number_of_keys, NUM_NOACCESS_BYTES));
+
return BUFFERALIGN(sz);
}
--
2.43.0
[text/x-patch] v2-0002-valgrind-Add-NOACCESS-sentinels-to-allocations-in.patch (3.0K, ../../[email protected]/3-v2-0002-valgrind-Add-NOACCESS-sentinels-to-allocations-in.patch)
download | inline diff:
From 753e07739b4e0e906e87f24e15e2c217de28f17d Mon Sep 17 00:00:00 2001
From: Andreas Karlsson <[email protected]>
Date: Thu, 4 Jun 2026 13:54:22 +0200
Subject: [PATCH v2 2/3] valgrind: Add NOACCESS sentinels to allocations in
main shared memory
When built with USE_VALGRIND we have the ShmemAllocator add a NOACCESS
sentinel after every allocation in shared memory. On EXEC_BACKEND builds
we set up the sentinels when the backend attaches to shared memory, but
then only for things in ShmemIndex and not for allocations done directly
via legacy functions like ShmemAlloc().
Author: Andreas Karlsson <[email protected]>
---
src/backend/storage/ipc/shmem.c | 38 ++++++++++++++++++++++++++++++++-
1 file changed, 37 insertions(+), 1 deletion(-)
diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c
index f1f7cd3a4ff..a7077be097b 100644
--- a/src/backend/storage/ipc/shmem.c
+++ b/src/backend/storage/ipc/shmem.c
@@ -142,6 +142,7 @@
#include "storage/shmem_internal.h"
#include "storage/spin.h"
#include "utils/builtins.h"
+#include "utils/memdebug.h"
#include "utils/tuplestore.h"
/*
@@ -279,6 +280,13 @@ static bool AttachShmemIndexEntry(ShmemRequest *request, bool missing_ok);
Datum pg_numa_available(PG_FUNCTION_ARGS);
+/* With valgrind, we want to add a couple NOACCESS bytes */
+#ifdef USE_VALGRIND
+#define NOACCESS_BYTES 32
+#else
+#define NOACCESS_BYTES 0
+#endif
+
/*
* ShmemRequestStruct() --- request a named shared memory area
*
@@ -405,9 +413,14 @@ ShmemGetRequestedSize(void)
/* pad the start address for alignment like ShmemAllocRaw() does */
if (alignment < PG_CACHE_LINE_SIZE)
alignment = PG_CACHE_LINE_SIZE;
+
size = TYPEALIGN(alignment, size);
size = add_size(size, request->options->size);
+
+#if USE_VALGRIND
+ size = add_size(size, NOACCESS_BYTES);
+#endif
}
return size;
@@ -492,6 +505,26 @@ ShmemAttachRequested(void)
callbacks->attach_fn(callbacks->opaque_arg);
}
+#ifdef USE_VALGRIND
+ {
+ HASH_SEQ_STATUS hstat;
+ ShmemIndexEnt *ent;
+
+ hash_seq_init(&hstat, ShmemIndex);
+
+ /*
+ * When compiled with EXEC_BACKEND we need to recreate all NOACCESS
+ * regions in each backend, but we can only recreate those with index
+ * entries and not any of the regions for memory allocated directly with
+ * ShmemAlloc().
+ */
+ while ((ent = (ShmemIndexEnt *) hash_seq_search(&hstat)) != NULL)
+ {
+ VALGRIND_MAKE_MEM_NOACCESS(ent->location + ent->size, NOACCESS_BYTES);
+ }
+ }
+#endif
+
LWLockRelease(ShmemIndexLock);
shmem_request_state = SRS_DONE;
@@ -822,11 +855,14 @@ ShmemAllocRaw(Size size, Size alignment, Size *allocated_size)
rawStart = ShmemAllocator->free_offset;
newStart = TYPEALIGN(alignment, rawStart);
- newFree = newStart + size;
+ newFree = newStart + size + NOACCESS_BYTES;
if (newFree <= ShmemSegHdr->totalsize)
{
newSpace = (char *) ShmemBase + newStart;
ShmemAllocator->free_offset = newFree;
+
+ /* Make the bytes at the end no-access */
+ VALGRIND_MAKE_MEM_NOACCESS(newSpace + size, NOACCESS_BYTES);
}
else
newSpace = NULL;
--
2.43.0
[text/x-patch] v2-0003-Toy-extension-to-make-it-easier-to-test-out-of-bo.patch (8.7K, ../../[email protected]/4-v2-0003-Toy-extension-to-make-it-easier-to-test-out-of-bo.patch)
download | inline diff:
From b8e70a55251d9dd559147decaf9924eed43347f2 Mon Sep 17 00:00:00 2001
From: Andreas Karlsson <[email protected]>
Date: Sat, 4 Jul 2026 13:17:15 +0200
Subject: [PATCH v2 3/3] Toy extension to make it easier to test out of bounds
access
I built this extension just for my personal use while developing so I
can trigger out of bounds array access by accessing an array index
outside of a 10 element array in shared memory.
It can be used to test both the ShmemAllocator and shm_toc, including
access from a background worker
---
contrib/meson.build | 1 +
contrib/valgrind_shmem/meson.build | 18 ++
.../valgrind_shmem/valgrind_shmem--1.0.sql | 16 ++
contrib/valgrind_shmem/valgrind_shmem.c | 227 ++++++++++++++++++
contrib/valgrind_shmem/valgrind_shmem.control | 5 +
5 files changed, 267 insertions(+)
create mode 100644 contrib/valgrind_shmem/meson.build
create mode 100644 contrib/valgrind_shmem/valgrind_shmem--1.0.sql
create mode 100644 contrib/valgrind_shmem/valgrind_shmem.c
create mode 100644 contrib/valgrind_shmem/valgrind_shmem.control
diff --git a/contrib/meson.build b/contrib/meson.build
index ebb7f83d8c5..3159c4084b0 100644
--- a/contrib/meson.build
+++ b/contrib/meson.build
@@ -72,4 +72,5 @@ subdir('tsm_system_time')
subdir('unaccent')
subdir('uuid-ossp')
subdir('vacuumlo')
+subdir('valgrind_shmem')
subdir('xml2')
diff --git a/contrib/valgrind_shmem/meson.build b/contrib/valgrind_shmem/meson.build
new file mode 100644
index 00000000000..5b6206ed0cb
--- /dev/null
+++ b/contrib/valgrind_shmem/meson.build
@@ -0,0 +1,18 @@
+# Copyright (c) 2022-2026, PostgreSQL Global Development Group
+
+valgrind_shmem_sources = files(
+ 'valgrind_shmem.c',
+)
+
+valgrind_shmem = shared_module('valgrind_shmem',
+ valgrind_shmem_sources,
+ c_pch: pch_postgres_h,
+ kwargs: contrib_mod_args,
+)
+contrib_targets += valgrind_shmem
+
+install_data(
+ 'valgrind_shmem--1.0.sql',
+ 'valgrind_shmem.control',
+ kwargs: contrib_data_args,
+)
diff --git a/contrib/valgrind_shmem/valgrind_shmem--1.0.sql b/contrib/valgrind_shmem/valgrind_shmem--1.0.sql
new file mode 100644
index 00000000000..234be9b1b07
--- /dev/null
+++ b/contrib/valgrind_shmem/valgrind_shmem--1.0.sql
@@ -0,0 +1,16 @@
+/* contrib/valgrind_shmem/valgrind_shmem--1.1.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION valgrind_shmem" to load this file. \quit
+
+CREATE FUNCTION valgrind_shmem_main_get(int) RETURNS int
+AS 'MODULE_PATHNAME'
+LANGUAGE C STRICT IMMUTABLE PARALLEL SAFE;
+
+CREATE FUNCTION valgrind_shmem_shm_toc_fg_get(int) RETURNS int
+AS 'MODULE_PATHNAME'
+LANGUAGE C STRICT IMMUTABLE PARALLEL SAFE;
+
+CREATE FUNCTION valgrind_shmem_shm_toc_bg_get(int) RETURNS int
+AS 'MODULE_PATHNAME'
+LANGUAGE C STRICT IMMUTABLE PARALLEL SAFE;
diff --git a/contrib/valgrind_shmem/valgrind_shmem.c b/contrib/valgrind_shmem/valgrind_shmem.c
new file mode 100644
index 00000000000..62d783362bc
--- /dev/null
+++ b/contrib/valgrind_shmem/valgrind_shmem.c
@@ -0,0 +1,227 @@
+#include "postgres.h"
+
+#include "fmgr.h"
+#include "miscadmin.h"
+#include "postmaster/bgworker.h"
+#include "storage/dsm.h"
+#include "storage/ipc.h"
+#include "storage/proc.h"
+#include "storage/shm_toc.h"
+#include "storage/shmem.h"
+#include "utils/wait_event.h"
+
+PG_MODULE_MAGIC_EXT(
+ .name = "valgrind_shmem",
+ .version = PG_VERSION
+);
+
+#define PG_TEST_VALGRIND_SHMEM_MAGIC 0x74686f72
+
+PG_FUNCTION_INFO_V1(valgrind_shmem_main_get);
+PG_FUNCTION_INFO_V1(valgrind_shmem_shm_toc_fg_get);
+PG_FUNCTION_INFO_V1(valgrind_shmem_shm_toc_bg_get);
+
+pg_noreturn extern PGDLLEXPORT void valgrind_shmem_main(Datum main_arg);
+
+typedef struct
+{
+ int32 index;
+ bool done;
+ int32 result;
+} WorkerHeader;
+
+typedef struct
+{
+ int32 a[10];
+} ShmemState;
+
+static ShmemState *state;
+static int we_valdgrind_shmem_request = 0;
+
+static void
+shmem_request(void *arg)
+{
+ ShmemRequestStruct(.name = "valgrind_shmem: state",
+ .size = sizeof(ShmemState),
+ .ptr = (void **) &state,
+ );
+}
+
+static void
+shmem_init(void *arg)
+{
+ for (int i = 0; i < 10; i++)
+ state->a[i] = i * 2;
+}
+
+static const ShmemCallbacks shmem_callbacks = {
+ .request_fn = shmem_request,
+ .init_fn = shmem_init,
+};
+
+/*
+ * Gets an array element from a struct in the main shared memory.
+ */
+Datum
+valgrind_shmem_main_get(PG_FUNCTION_ARGS)
+{
+ int32 index = PG_GETARG_INT32(0);
+
+ PG_RETURN_INT32(state->a[index]);
+}
+
+/*
+ * Gets an array element from a struct in a shm_toc entry in DSM.
+ */
+Datum
+valgrind_shmem_shm_toc_fg_get(PG_FUNCTION_ARGS)
+{
+ int32 index = PG_GETARG_INT32(0);
+ shm_toc_estimator e;
+ Size segsize;
+ dsm_segment *seg;
+ shm_toc *toc;
+ ShmemState *state;
+ int32 ret;
+
+ shm_toc_initialize_estimator(&e);
+ shm_toc_estimate_chunk(&e, sizeof(ShmemState));
+ shm_toc_estimate_keys(&e, 1);
+ segsize = shm_toc_estimate(&e);
+
+ seg = dsm_create(shm_toc_estimate(&e), 0);
+ toc = shm_toc_create(PG_TEST_VALGRIND_SHMEM_MAGIC, dsm_segment_address(seg), segsize);
+
+ state = shm_toc_allocate(toc, sizeof(ShmemState));
+ for (int i = 0; i < 10; i++)
+ state->a[i] = i * 3;
+ shm_toc_insert(toc, 1, state);
+
+ ret = state->a[index];
+
+ dsm_detach(seg);
+
+ PG_RETURN_INT32(ret);
+}
+
+/*
+ * Gets an array element from a struct in a shm_toc entry in DSM in a
+ * background worker.
+ */
+Datum
+valgrind_shmem_shm_toc_bg_get(PG_FUNCTION_ARGS)
+{
+ int32 index = PG_GETARG_INT32(0);
+ shm_toc_estimator e;
+ Size segsize;
+ dsm_segment *seg;
+ shm_toc *toc;
+ WorkerHeader *header;
+ ShmemState *state;
+ BackgroundWorker worker = {
+ .bgw_flags = BGWORKER_SHMEM_ACCESS,
+ .bgw_start_time = BgWorkerStart_ConsistentState,
+ .bgw_restart_time = BGW_NEVER_RESTART,
+ .bgw_library_name = "valgrind_shmem",
+ .bgw_function_name = "valgrind_shmem_main",
+ .bgw_type = "valgrind_shmem",
+ .bgw_name = "valgrind_shmem worker",
+ .bgw_notify_pid = MyProcPid
+ };
+ BackgroundWorkerHandle *whandle;
+ int32 ret;
+
+ shm_toc_initialize_estimator(&e);
+ shm_toc_estimate_chunk(&e, sizeof(WorkerHeader));
+ shm_toc_estimate_chunk(&e, sizeof(ShmemState));
+ shm_toc_estimate_keys(&e, 2);
+ segsize = shm_toc_estimate(&e);
+
+ seg = dsm_create(shm_toc_estimate(&e), 0);
+ toc = shm_toc_create(PG_TEST_VALGRIND_SHMEM_MAGIC, dsm_segment_address(seg), segsize);
+
+ header = shm_toc_allocate(toc, sizeof(WorkerHeader));
+ header->index = index;
+ header->done = false;
+ shm_toc_insert(toc, 0, header);
+
+ state = shm_toc_allocate(toc, sizeof(ShmemState));
+ for (int i = 0; i < 10; i++)
+ state->a[i] = i * 3;
+ shm_toc_insert(toc, 1, state);
+
+ worker.bgw_main_arg = UInt32GetDatum(dsm_segment_handle(seg));
+
+ if (!RegisterDynamicBackgroundWorker(&worker, &whandle))
+ ereport(ERROR,
+ errcode(ERRCODE_INSUFFICIENT_RESOURCES),
+ errmsg("could not register background process"),
+ errhint("You may need to increase \"max_worker_processes\"."));
+
+ for (;;)
+ {
+ BgwHandleStatus status;
+ pid_t pid;
+
+ if (header->done)
+ break;
+
+ status = GetBackgroundWorkerPid(whandle, &pid);
+ if (status == BGWH_STOPPED || status == BGWH_POSTMASTER_DIED)
+ ereport(ERROR,
+ errmsg("background worker died unexpectedly"));
+
+ if (we_valdgrind_shmem_request == 0)
+ we_valdgrind_shmem_request = WaitEventExtensionNew("TestValgrindHsmemRequest");
+
+ (void) WaitLatch(MyLatch, WL_LATCH_SET | WL_EXIT_ON_PM_DEATH, 0,
+ we_valdgrind_shmem_request);
+
+ ResetLatch(MyLatch);
+
+ CHECK_FOR_INTERRUPTS();
+ }
+
+ ret = header->result;
+
+ dsm_detach(seg);
+
+ PG_RETURN_INT32(ret);
+}
+
+void
+valgrind_shmem_main(Datum main_arg)
+{
+ dsm_segment *seg;
+ shm_toc *toc;
+ WorkerHeader *header;
+ ShmemState *state;
+
+ seg = dsm_attach(DatumGetUInt32(main_arg));
+ if (seg == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("unable to map dynamic shared memory segment")));
+
+ toc = shm_toc_attach(PG_TEST_VALGRIND_SHMEM_MAGIC, dsm_segment_address(seg));
+ if (toc == NULL)
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("bad magic number in dynamic shared memory segment")));
+
+ header = shm_toc_lookup(toc, 0, false);
+ state = shm_toc_lookup(toc, 1, false);
+
+ pg_usleep(500 * 1000);
+
+ header->result = state->a[header->index];
+ header->done = true;
+
+ proc_exit(0);
+}
+
+void
+_PG_init(void)
+{
+ RegisterShmemCallbacks(&shmem_callbacks);
+}
diff --git a/contrib/valgrind_shmem/valgrind_shmem.control b/contrib/valgrind_shmem/valgrind_shmem.control
new file mode 100644
index 00000000000..b70bd0e2d10
--- /dev/null
+++ b/contrib/valgrind_shmem/valgrind_shmem.control
@@ -0,0 +1,5 @@
+# valigrind_shmem extension
+comment = 'Dummy extension testing valgrind and shared memory'
+default_version = '1.0'
+module_pathname = '$libdir/valgrind_shmem'
+relocatable = true
--
2.43.0
^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2026-07-08 00:31 UTC | newest]
Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-02-09 17:10 [PATCH v5 2/2] Do COPY FROM encoding conversion/verification in larger chunks. Heikki Linnakangas <[email protected]>
2026-07-08 00:31 Re: PoC: VALGRIND_MAKE_MEM_NOACCESS for dynamic shared memory Andreas Karlsson <[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