public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v3 5/5] Do COPY FROM encoding conversion/verification in larger chunks.
3+ messages / 3 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; 3+ 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] 3+ messages in thread
* Re: Avoiding superfluous buffer locking during nbtree backwards scans
@ 2024-10-11 23:29 Peter Geoghegan <[email protected]>
2024-11-07 10:44 ` Re: Avoiding superfluous buffer locking during nbtree backwards scans Masahiro Ikeda <[email protected]>
0 siblings, 1 reply; 3+ messages in thread
From: Peter Geoghegan @ 2024-10-11 23:29 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>; +Cc: Matthias van de Meent <[email protected]>; Tomas Vondra <[email protected]>
On Thu, Oct 10, 2024 at 1:41 PM Peter Geoghegan <[email protected]> wrote:
> The main simplification new to v4 is that v4 isolates the need to call
> _bt_drop_lock_and_maybe_pin to only 2 functions: _bt_readnextpage, and
> its new _bt_readfirstpage "sibling" function. The functions have
> similar preconditions, and identical postconditions -- all of which
> are now clearly documented.
Worked on this a little more today. Attached is v5, which goes even
further than v4 did. Highlights:
* Completely gets rid of _bt_initialize_more_data by moving its code
into the new _bt_readfirstpage function.
* Adds similar "initialize moreLeft/moreRight" code to the
corresponding point within _bt_readnextpage (that is, at the start of
_bt_readnextpage) -- meaning I moved a bit more code from _bt_steppage
into _bt_readnextpage.
Again, _bt_readnextpage and the new _bt_readfirstpage function are
intended to be "sibling" functions (while _bt_steppage is demoted to a
helper that sets up a call to _bt_readfirstpage). The matching
handling of currPos initialization within _bt_readnextpage and the
_bt_readfirstpage pushes this further than in v4.
* We now reset currPos state (including even its moreLeft/moreRight
fields) within _bt_parallel_seize, automatically and regardless of any
other details.
It doesn't really make sense that (up until now) we've trusted the
shared state that we seized from the parallel scan to kinda be in sync
with the local currPos state (at least its moreLeft/moreRight need to
be in sync, to avoid confusion within _bt_readnextpage). This allowed
me to remove several parallel-only BTScanPosInvalidate calls from
_bt_readnextpage.
This "currPos must be kept in sync with parallel scan state" confusion
is exemplified by the following code snippet (which FWIW was already
removed by earlier revisions of the patch), that appears in the
backwards scan block of _bt_readnextpage on master:
"""
/*
* Should only happen in parallel cases, when some other backend
* advanced the scan.
*/
if (so->currPos.currPage != blkno)
{
BTScanPosUnpinIfPinned(so->currPos);
so->currPos.currPage = blkno;
}
"""
Why should the parallel scan have any concern for what currPos is, in general?
* Now nbtree has only one PredicateLockPage call, inside _bt_readpage.
This saves us an extra BufferGetBlockNumber call. (v4 pushed
PredicateLockPage calls down one layer, v5 pushes them down another
layer still.)
* Improved precondition/postcondition comments, and generally cleaner
separation between _bt_steppage and _bt_readnextpage.
--
Peter Geoghegan
Attachments:
[application/x-patch] v5-0001-Optimize-and-simplify-nbtree-backwards-scans.patch (41.6K, ../../CAH2-Wzk003jELmOQ5NCgCqHuE85q_39LFdi0FUvbFAv1KH9ktQ@mail.gmail.com/2-v5-0001-Optimize-and-simplify-nbtree-backwards-scans.patch)
download | inline diff:
From b3c892d428688057378c5a74e2bb313c1a6b4c2e Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <[email protected]>
Date: Tue, 8 Oct 2024 11:40:54 -0400
Subject: [PATCH v5] Optimize and simplify nbtree backwards scans.
Author: Matthias van de Meent <[email protected]>
Author: Peter Geoghegan <[email protected]>
Discussion: https://postgr.es/m/CAEze2WgpBGRgTTxTWVPXc9+PB6fc1a7t+VyGXHzfnrFXcQVxnA@mail.gmail.com
Discussion: https://postgr.es/m/CAH2-WzkBTuFv7W2+84jJT8mWZLXVL0GHq2hMUTn6c9Vw=eYrCw@mail.gmail.com
---
src/include/access/nbtree.h | 21 +-
src/backend/access/nbtree/nbtree.c | 49 ++-
src/backend/access/nbtree/nbtsearch.c | 603 +++++++++++++-------------
src/backend/access/nbtree/nbtutils.c | 2 +-
4 files changed, 347 insertions(+), 328 deletions(-)
diff --git a/src/include/access/nbtree.h b/src/include/access/nbtree.h
index d64300fb9..594eddaec 100644
--- a/src/include/access/nbtree.h
+++ b/src/include/access/nbtree.h
@@ -954,6 +954,7 @@ typedef struct BTScanPosData
XLogRecPtr lsn; /* pos in the WAL stream when page was read */
BlockNumber currPage; /* page referenced by items array */
+ BlockNumber prevPage; /* page's left link when we scanned it */
BlockNumber nextPage; /* page's right link when we scanned it */
/*
@@ -965,15 +966,6 @@ typedef struct BTScanPosData
bool moreLeft;
bool moreRight;
- /*
- * Direction of the scan at the time that _bt_readpage was called.
- *
- * Used by btrestrpos to "restore" the scan's array keys by resetting each
- * array to its first element's value (first in this scan direction). This
- * avoids the need to directly track the array keys in btmarkpos.
- */
- ScanDirection dir;
-
/*
* If we are doing an index-only scan, nextTupleOffset is the first free
* location in the associated tuple storage workspace.
@@ -1022,6 +1014,7 @@ typedef BTScanPosData *BTScanPos;
#define BTScanPosInvalidate(scanpos) \
do { \
(scanpos).currPage = InvalidBlockNumber; \
+ (scanpos).prevPage = InvalidBlockNumber; \
(scanpos).nextPage = InvalidBlockNumber; \
(scanpos).buf = InvalidBuffer; \
(scanpos).lsn = InvalidXLogRecPtr; \
@@ -1071,6 +1064,7 @@ typedef struct BTScanOpaqueData
* determine if there is a mark, first look at markItemIndex, then at
* markPos.
*/
+ ScanDirection markDir; /* direction of scan with mark */
int markItemIndex; /* itemIndex, or -1 if not valid */
/* keep these last in struct for efficiency */
@@ -1090,7 +1084,6 @@ typedef struct BTReadPageState
OffsetNumber minoff; /* Lowest non-pivot tuple's offset */
OffsetNumber maxoff; /* Highest non-pivot tuple's offset */
IndexTuple finaltup; /* Needed by scans with array keys */
- BlockNumber prev_scan_page; /* previous _bt_parallel_release block */
Page page; /* Page being read */
/* Per-tuple input parameters, set by _bt_readpage for _bt_checkkeys */
@@ -1192,11 +1185,13 @@ extern int btgettreeheight(Relation rel);
* prototypes for internal functions in nbtree.c
*/
extern bool _bt_parallel_seize(IndexScanDesc scan, BlockNumber *pageno,
- bool first);
-extern void _bt_parallel_release(IndexScanDesc scan, BlockNumber scan_page);
+ BlockNumber *currblkno, bool first);
+extern void _bt_parallel_release(IndexScanDesc scan,
+ BlockNumber next_scan_page,
+ BlockNumber curr_page);
extern void _bt_parallel_done(IndexScanDesc scan);
extern void _bt_parallel_primscan_schedule(IndexScanDesc scan,
- BlockNumber prev_scan_page);
+ BlockNumber curr_page);
/*
* prototypes for functions in nbtdedup.c
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 56e502c4f..db20b1138 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -66,7 +66,9 @@ typedef enum
*/
typedef struct BTParallelScanDescData
{
- BlockNumber btps_scanPage; /* latest or next page to be scanned */
+ BlockNumber btps_nextScanPage; /* next page to be scanned */
+ BlockNumber btps_lastCurrPage; /* page whose sibling link was copied into
+ * btps_scanPage */
BTPS_State btps_pageStatus; /* indicates whether next page is
* available for scan. see above for
* possible states of parallel scan. */
@@ -520,7 +522,7 @@ btrestrpos(IndexScanDesc scan)
/* Reset the scan's array keys (see _bt_steppage for why) */
if (so->numArrayKeys)
{
- _bt_start_array_keys(scan, so->currPos.dir);
+ _bt_start_array_keys(scan, so->markDir);
so->needPrimScan = false;
}
}
@@ -548,7 +550,8 @@ btinitparallelscan(void *target)
BTParallelScanDesc bt_target = (BTParallelScanDesc) target;
SpinLockInit(&bt_target->btps_mutex);
- bt_target->btps_scanPage = InvalidBlockNumber;
+ bt_target->btps_nextScanPage = InvalidBlockNumber;
+ bt_target->btps_lastCurrPage = InvalidBlockNumber;
bt_target->btps_pageStatus = BTPARALLEL_NOT_INITIALIZED;
ConditionVariableInit(&bt_target->btps_cv);
}
@@ -573,7 +576,8 @@ btparallelrescan(IndexScanDesc scan)
* consistency.
*/
SpinLockAcquire(&btscan->btps_mutex);
- btscan->btps_scanPage = InvalidBlockNumber;
+ btscan->btps_nextScanPage = InvalidBlockNumber;
+ btscan->btps_lastCurrPage = InvalidBlockNumber;
btscan->btps_pageStatus = BTPARALLEL_NOT_INITIALIZED;
SpinLockRelease(&btscan->btps_mutex);
}
@@ -600,7 +604,8 @@ btparallelrescan(IndexScanDesc scan)
* Callers should ignore the value of pageno if the return value is false.
*/
bool
-_bt_parallel_seize(IndexScanDesc scan, BlockNumber *pageno, bool first)
+_bt_parallel_seize(IndexScanDesc scan, BlockNumber *pageno,
+ BlockNumber *currblkno, bool first)
{
BTScanOpaque so = (BTScanOpaque) scan->opaque;
bool exit_loop = false;
@@ -609,6 +614,14 @@ _bt_parallel_seize(IndexScanDesc scan, BlockNumber *pageno, bool first)
BTParallelScanDesc btscan;
*pageno = P_NONE;
+ *currblkno = InvalidBlockNumber;
+
+ /*
+ * Reset moreLeft/moreRight. We don't want to retain information from the
+ * last time the backend seized the scan.
+ */
+ BTScanPosInvalidate(so->currPos);
+ so->currPos.moreLeft = so->currPos.moreRight = true;
if (first)
{
@@ -684,7 +697,8 @@ _bt_parallel_seize(IndexScanDesc scan, BlockNumber *pageno, bool first)
* of advancing it to a new page!
*/
btscan->btps_pageStatus = BTPARALLEL_ADVANCING;
- *pageno = btscan->btps_scanPage;
+ *pageno = btscan->btps_nextScanPage;
+ *currblkno = btscan->btps_lastCurrPage;
exit_loop = true;
}
SpinLockRelease(&btscan->btps_mutex);
@@ -699,17 +713,22 @@ _bt_parallel_seize(IndexScanDesc scan, BlockNumber *pageno, bool first)
/*
* _bt_parallel_release() -- Complete the process of advancing the scan to a
- * new page. We now have the new value btps_scanPage; some other backend
+ * new page. We now have the new value btps_nextScanPage; another backend
* can now begin advancing the scan.
*
- * Callers whose scan uses array keys must save their scan_page argument so
+ * Callers whose scan uses array keys must save their curr_page argument so
* that it can be passed to _bt_parallel_primscan_schedule, should caller
* determine that another primitive index scan is required. If that happens,
* scan_page won't be scanned by any backend (unless the next primitive index
* scan lands on scan_page).
+ *
+ * Note: unlike the serial case, parallel scans don't need to remember both
+ * sibling links. Caller can just pass us whichever link is next for the scan
+ * direction because the direction cannot change when parallelism is in use.
*/
void
-_bt_parallel_release(IndexScanDesc scan, BlockNumber scan_page)
+_bt_parallel_release(IndexScanDesc scan, BlockNumber next_scan_page,
+ BlockNumber curr_page)
{
ParallelIndexScanDesc parallel_scan = scan->parallel_scan;
BTParallelScanDesc btscan;
@@ -718,7 +737,8 @@ _bt_parallel_release(IndexScanDesc scan, BlockNumber scan_page)
parallel_scan->ps_offset);
SpinLockAcquire(&btscan->btps_mutex);
- btscan->btps_scanPage = scan_page;
+ btscan->btps_nextScanPage = next_scan_page;
+ btscan->btps_lastCurrPage = curr_page;
btscan->btps_pageStatus = BTPARALLEL_IDLE;
SpinLockRelease(&btscan->btps_mutex);
ConditionVariableSignal(&btscan->btps_cv);
@@ -774,13 +794,13 @@ _bt_parallel_done(IndexScanDesc scan)
/*
* _bt_parallel_primscan_schedule() -- Schedule another primitive index scan.
*
- * Caller passes the block number most recently passed to _bt_parallel_release
+ * Caller passes the curr_page most recently passed to _bt_parallel_release
* by its backend. Caller successfully schedules the next primitive index scan
* if the shared parallel state hasn't been seized since caller's backend last
* advanced the scan.
*/
void
-_bt_parallel_primscan_schedule(IndexScanDesc scan, BlockNumber prev_scan_page)
+_bt_parallel_primscan_schedule(IndexScanDesc scan, BlockNumber curr_page)
{
BTScanOpaque so = (BTScanOpaque) scan->opaque;
ParallelIndexScanDesc parallel_scan = scan->parallel_scan;
@@ -792,10 +812,11 @@ _bt_parallel_primscan_schedule(IndexScanDesc scan, BlockNumber prev_scan_page)
parallel_scan->ps_offset);
SpinLockAcquire(&btscan->btps_mutex);
- if (btscan->btps_scanPage == prev_scan_page &&
+ if (btscan->btps_lastCurrPage == curr_page &&
btscan->btps_pageStatus == BTPARALLEL_IDLE)
{
- btscan->btps_scanPage = InvalidBlockNumber;
+ btscan->btps_nextScanPage = InvalidBlockNumber;
+ btscan->btps_lastCurrPage = InvalidBlockNumber;
btscan->btps_pageStatus = BTPARALLEL_NEED_PRIMSCAN;
/* Serialize scan's current array keys */
diff --git a/src/backend/access/nbtree/nbtsearch.c b/src/backend/access/nbtree/nbtsearch.c
index fff7c89ea..cd9092130 100644
--- a/src/backend/access/nbtree/nbtsearch.c
+++ b/src/backend/access/nbtree/nbtsearch.c
@@ -43,12 +43,13 @@ static inline void _bt_savepostingitem(BTScanOpaque so, int itemIndex,
OffsetNumber offnum,
ItemPointer heapTid, int tupleOffset);
static bool _bt_steppage(IndexScanDesc scan, ScanDirection dir);
-static bool _bt_readnextpage(IndexScanDesc scan, BlockNumber blkno, ScanDirection dir);
-static bool _bt_parallel_readpage(IndexScanDesc scan, BlockNumber blkno,
- ScanDirection dir);
-static Buffer _bt_walk_left(Relation rel, Buffer buf);
+static bool _bt_readfirstpage(IndexScanDesc scan, OffsetNumber offnum,
+ ScanDirection dir);
+static bool _bt_readnextpage(IndexScanDesc scan, BlockNumber blkno,
+ BlockNumber lastcurrblkno, ScanDirection dir);
+static Buffer _bt_lock_and_validate_left(Relation rel, BlockNumber *blkno,
+ BlockNumber lastcurrblkno);
static bool _bt_endpoint(IndexScanDesc scan, ScanDirection dir);
-static inline void _bt_initialize_more_data(BTScanOpaque so, ScanDirection dir);
/*
@@ -889,7 +890,6 @@ _bt_first(IndexScanDesc scan, ScanDirection dir)
ScanKeyData notnullkeys[INDEX_MAX_KEYS];
int keysz = 0;
int i;
- bool status;
StrategyNumber strat_total;
BTScanPosItem *currItem;
BlockNumber blkno;
@@ -924,7 +924,10 @@ _bt_first(IndexScanDesc scan, ScanDirection dir)
*/
if (scan->parallel_scan != NULL)
{
- status = _bt_parallel_seize(scan, &blkno, true);
+ bool status;
+ BlockNumber lastcurrblkno;
+
+ status = _bt_parallel_seize(scan, &blkno, &lastcurrblkno, true);
/*
* Initialize arrays (when _bt_parallel_seize didn't already set up
@@ -942,7 +945,14 @@ _bt_first(IndexScanDesc scan, ScanDirection dir)
}
else if (blkno != InvalidBlockNumber)
{
- if (!_bt_parallel_readpage(scan, blkno, dir))
+ Assert(!so->needPrimScan);
+
+ /*
+ * We anticipated starting another primitive scan, but some other
+ * worker bet us to it. Bypass _bt_readfirstpage by calling
+ * _bt_readnextpage directly.
+ */
+ if (!_bt_readnextpage(scan, blkno, lastcurrblkno, dir))
return false;
goto readcomplete;
}
@@ -1427,10 +1437,6 @@ _bt_first(IndexScanDesc scan, ScanDirection dir)
}
}
- PredicateLockPage(rel, BufferGetBlockNumber(buf), scan->xs_snapshot);
-
- _bt_initialize_more_data(so, dir);
-
/* position to the precise item on the page */
offnum = _bt_binsrch(rel, &inskey, buf);
Assert(!BTScanPosIsValid(so->currPos));
@@ -1455,21 +1461,8 @@ _bt_first(IndexScanDesc scan, ScanDirection dir)
* for the page. For example, when inskey is both < the leaf page's high
* key and > all of its non-pivot tuples, offnum will be "maxoff + 1".
*/
- if (!_bt_readpage(scan, dir, offnum, true))
- {
- /*
- * There's no actually-matching data on this page. Try to advance to
- * the next page. Return false if there's no matching data at all.
- */
- _bt_unlockbuf(scan->indexRelation, so->currPos.buf);
- if (!_bt_steppage(scan, dir))
- return false;
- }
- else
- {
- /* We have at least one item to return as scan's first item */
- _bt_drop_lock_and_maybe_pin(scan, &so->currPos);
- }
+ if (!_bt_readfirstpage(scan, offnum, dir))
+ return false;
readcomplete:
/* OK, itemIndex says what to return */
@@ -1545,14 +1538,6 @@ _bt_next(IndexScanDesc scan, ScanDirection dir)
* that there can be no more matching tuples in the current scan direction
* (could just be for the current primitive index scan when scan has arrays).
*
- * _bt_first caller passes us an offnum returned by _bt_binsrch, which might
- * be an out of bounds offnum such as "maxoff + 1" in certain corner cases.
- * _bt_checkkeys will stop the scan as soon as an equality qual fails (when
- * its scan key was marked required), so _bt_first _must_ pass us an offnum
- * exactly at the beginning of where equal tuples are to be found. When we're
- * passed an offnum past the end of the page, we might still manage to stop
- * the scan on this page by calling _bt_checkkeys against the high key.
- *
* In the case of a parallel scan, caller must have called _bt_parallel_seize
* prior to calling this function; this function will invoke
* _bt_parallel_release before returning.
@@ -1563,6 +1548,7 @@ static bool
_bt_readpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum,
bool firstPage)
{
+ Relation rel = scan->indexRelation;
BTScanOpaque so = (BTScanOpaque) scan->opaque;
Page page;
BTPageOpaque opaque;
@@ -1582,18 +1568,7 @@ _bt_readpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum,
page = BufferGetPage(so->currPos.buf);
opaque = BTPageGetOpaque(page);
- /* allow next page be processed by parallel worker */
- if (scan->parallel_scan)
- {
- if (ScanDirectionIsForward(dir))
- pstate.prev_scan_page = opaque->btpo_next;
- else
- pstate.prev_scan_page = BufferGetBlockNumber(so->currPos.buf);
-
- _bt_parallel_release(scan, pstate.prev_scan_page);
- }
-
- indnatts = IndexRelationGetNumberOfAttributes(scan->indexRelation);
+ indnatts = IndexRelationGetNumberOfAttributes(rel);
arrayKeys = so->numArrayKeys != 0;
minoff = P_FIRSTDATAKEY(opaque);
maxoff = PageGetMaxOffsetNumber(page);
@@ -1615,8 +1590,12 @@ _bt_readpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum,
/*
* We note the buffer's block number so that we can release the pin later.
* This allows us to re-read the buffer if it is needed again for hinting.
+ * Also save the page's sibling links; this tells us where to step
+ * right/left to after we're done reading items from this page.
*/
so->currPos.currPage = BufferGetBlockNumber(so->currPos.buf);
+ so->currPos.prevPage = opaque->btpo_prev;
+ so->currPos.nextPage = opaque->btpo_next;
/*
* We save the LSN of the page as we read it, so that we know whether it
@@ -1625,13 +1604,6 @@ _bt_readpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum,
*/
so->currPos.lsn = BufferGetLSNAtomic(so->currPos.buf);
- /*
- * we must save the page's right-link while scanning it; this tells us
- * where to step right to after we're done with these items. There is no
- * corresponding need for the left-link, since splits always go right.
- */
- so->currPos.nextPage = opaque->btpo_next;
-
/* initialize tuple workspace to empty */
so->currPos.nextTupleOffset = 0;
@@ -1640,6 +1612,20 @@ _bt_readpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum,
* good.
*/
Assert(BTScanPosIsPinned(so->currPos));
+ Assert(!P_IGNORE(opaque));
+
+ /* allow next page to be processed by parallel worker */
+ if (scan->parallel_scan)
+ {
+ if (ScanDirectionIsForward(dir))
+ _bt_parallel_release(scan, so->currPos.nextPage,
+ so->currPos.currPage);
+ else
+ _bt_parallel_release(scan, so->currPos.prevPage,
+ so->currPos.currPage);
+ }
+
+ PredicateLockPage(rel, so->currPos.currPage, scan->xs_snapshot);
/*
* Prechecking the value of the continuescan flag for the last item on the
@@ -1804,7 +1790,7 @@ _bt_readpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum,
IndexTuple itup = (IndexTuple) PageGetItem(page, iid);
int truncatt;
- truncatt = BTreeTupleGetNAtts(itup, scan->indexRelation);
+ truncatt = BTreeTupleGetNAtts(itup, rel);
pstate.prechecked = false; /* precheck didn't cover HIKEY */
_bt_checkkeys(scan, &pstate, arrayKeys, itup, truncatt);
}
@@ -1934,7 +1920,7 @@ _bt_readpage(IndexScanDesc scan, ScanDirection dir, OffsetNumber offnum,
* We don't need to visit page to the left when no more matches will
* be found there
*/
- if (!pstate.continuescan || P_LEFTMOST(opaque))
+ if (!pstate.continuescan)
so->currPos.moreLeft = false;
Assert(itemIndex >= 0);
@@ -2035,20 +2021,21 @@ _bt_savepostingitem(BTScanOpaque so, int itemIndex, OffsetNumber offnum,
/*
* _bt_steppage() -- Step to next page containing valid data for scan
*
- * On entry, if so->currPos.buf is valid the buffer is pinned but not locked;
- * if pinned, we'll drop the pin before moving to next page. The buffer is
- * not locked on entry.
+ * On entry, if so->currPos.buf is valid the buffer is pinned but not locked.
+ * This is a wrapper on _bt_readnextpage that performs final steps for the
+ * current page (including dropping its pin). It sets up the _bt_readnextpage
+ * call using either local state saved by the last _bt_readpage call, or using
+ * shared parallel scan state (by seizing the parallel scan).
*
- * For success on a scan using a non-MVCC snapshot we hold a pin, but not a
- * read lock, on that page. If we do not hold the pin, we set so->currPos.buf
- * to InvalidBuffer. We return true to indicate success.
+ * Parallel scan callers that have already seized the scan should directly
+ * call _bt_readnextpage, rather than calling here.
*/
static bool
_bt_steppage(IndexScanDesc scan, ScanDirection dir)
{
BTScanOpaque so = (BTScanOpaque) scan->opaque;
- BlockNumber blkno = InvalidBlockNumber;
- bool status;
+ BlockNumber blkno,
+ lastcurrblkno;
Assert(BTScanPosIsValid(so->currPos));
@@ -2090,7 +2077,7 @@ _bt_steppage(IndexScanDesc scan, ScanDirection dir)
* In effect, btrestpos leaves advancing the arrays up to the first
* _bt_readpage call (that takes place after it has restored markPos).
*/
- Assert(so->markPos.dir == dir);
+ Assert(so->markDir == dir);
if (so->needPrimScan)
{
if (ScanDirectionIsForward(dir))
@@ -2098,93 +2085,193 @@ _bt_steppage(IndexScanDesc scan, ScanDirection dir)
else
so->markPos.moreLeft = true;
}
+
+ /* mark/restore not supported by parallel scans */
+ Assert(!scan->parallel_scan);
}
- if (ScanDirectionIsForward(dir))
+ /* release the previously pinned leaf page buffer, if any */
+ BTScanPosUnpinIfPinned(so->currPos);
+
+ /* Walk to the next page with data */
+ if (!scan->parallel_scan)
{
- /* Walk right to the next page with data */
- if (scan->parallel_scan != NULL)
- {
- /*
- * Seize the scan to get the next block number; if the scan has
- * ended already, bail out.
- */
- status = _bt_parallel_seize(scan, &blkno, false);
- if (!status)
- {
- /* release the previous buffer, if pinned */
- BTScanPosUnpinIfPinned(so->currPos);
- BTScanPosInvalidate(so->currPos);
- return false;
- }
- }
- else
- {
- /* Not parallel, so use the previously-saved nextPage link. */
+ /* Not parallel, so use nextPage/prevPage from local scan state */
+ if (ScanDirectionIsForward(dir))
blkno = so->currPos.nextPage;
- }
-
- /* Remember we left a page with data */
- so->currPos.moreLeft = true;
-
- /* release the previous buffer, if pinned */
- BTScanPosUnpinIfPinned(so->currPos);
+ else
+ blkno = so->currPos.prevPage;
+ lastcurrblkno = so->currPos.currPage;
}
else
{
- /* Remember we left a page with data */
- so->currPos.moreRight = true;
-
- if (scan->parallel_scan != NULL)
+ /*
+ * Seize the scan to get the nextPage and currPage from shared
+ * parallel state
+ */
+ if (!_bt_parallel_seize(scan, &blkno, &lastcurrblkno, false))
{
- /*
- * Seize the scan to get the current block number; if the scan has
- * ended already, bail out.
- */
- status = _bt_parallel_seize(scan, &blkno, false);
- BTScanPosUnpinIfPinned(so->currPos);
- if (!status)
- {
- BTScanPosInvalidate(so->currPos);
- return false;
- }
- }
- else
- {
- /* Not parallel, so just use our own notion of the current page */
- blkno = so->currPos.currPage;
+ /* don't call _bt_parallel_done */
+ return false;
}
}
- if (!_bt_readnextpage(scan, blkno, dir))
- return false;
+ /*
+ * _bt_readnextpage just works off of blkno/lastcurrblkno now
+ *
+ * (It does still expect so->currPos.moreLeft/so->currPos.moreLeft to
+ * remain however the last _bt_readpage call for lastcurrblkno set them.)
+ */
+ BTScanPosInvalidate(so->currPos);
- /* We have at least one item to return as scan's next item */
- _bt_drop_lock_and_maybe_pin(scan, &so->currPos);
+ if (!_bt_readnextpage(scan, blkno, lastcurrblkno, dir))
+ return false;
return true;
}
/*
- * _bt_readnextpage() -- Read next page containing valid data for scan
+ * _bt_readfirstpage() -- Read first page containing valid data for _bt_first
+ *
+ * _bt_first caller passes us an offnum returned by _bt_binsrch, which might
+ * be an out of bounds offnum such as "maxoff + 1" in certain corner cases.
+ * _bt_checkkeys will stop the scan as soon as an equality qual fails (when
+ * its scan key was marked required), so _bt_first _must_ pass us an offnum
+ * exactly at the beginning of where equal tuples are to be found. When we're
+ * passed an offnum past the end of the page, we might still manage to stop
+ * the scan on this page by calling _bt_checkkeys against the high key. See
+ * _bt_readpage for full details.
+ *
+ * On entry, so->currPos.buf must be pinned and locked. Also, parallel scan
+ * callers must have seized the scan before calling here.
*
* On success exit, so->currPos is updated to contain data from the next
- * interesting page, and we return true. Caller must release the lock (and
- * maybe the pin) on the buffer on success exit.
+ * interesting page, and we return true. We hold a pin on the buffer on
+ * success exit, except for scans that _bt_drop_lock_and_maybe_pin indicates
+ * can safely have their pin dropped to avoid blocking progress by VACUUM.
*
- * If there are no more matching records in the given direction, we drop all
- * locks and pins, set so->currPos.buf to InvalidBuffer, and return false.
+ * If there are no more matching records in the given direction, we set
+ * so->currPos.buf to InvalidBuffer, and return false. Note that this could
+ * just signal the end of the current primitive index scan.
+ *
+ * We always release the scan for a parallel scan caller, regardless of
+ * success or failure; we do this by calling _bt_parallel_release at the
+ * earliest opportunity.
*/
static bool
-_bt_readnextpage(IndexScanDesc scan, BlockNumber blkno, ScanDirection dir)
+_bt_readfirstpage(IndexScanDesc scan, OffsetNumber offnum, ScanDirection dir)
{
BTScanOpaque so = (BTScanOpaque) scan->opaque;
- Relation rel;
+
+ Assert(BufferIsValid(so->currPos.buf));
+
+ /* Initialize currPos for so->currPos.buf */
+ if (so->needPrimScan)
+ {
+ Assert(so->numArrayKeys);
+
+ so->currPos.moreLeft = true;
+ so->currPos.moreRight = true;
+ so->needPrimScan = false;
+ }
+ else if (ScanDirectionIsForward(dir))
+ {
+ so->currPos.moreLeft = false;
+ so->currPos.moreRight = true;
+ }
+ else
+ {
+ so->currPos.moreLeft = true;
+ so->currPos.moreRight = false;
+ }
+ so->markDir = dir;
+ so->numKilled = 0; /* just paranoia */
+ so->markItemIndex = -1; /* ditto */
+
+ if (_bt_readpage(scan, dir, offnum, true))
+ {
+ /*
+ * _bt_readpage succeeded. Drop the lock (and maybe the pin) on
+ * so->currPos.buf in preparation for btgettuple returning tuples.
+ */
+ Assert(BTScanPosIsPinned(so->currPos));
+ _bt_drop_lock_and_maybe_pin(scan, &so->currPos);
+ return true;
+ }
+
+ /* There's no actually-matching data on the page in so->currPos.buf */
+ _bt_unlockbuf(scan->indexRelation, so->currPos.buf);
+
+ /* Call _bt_readnextpage through its _bt_steppage wrapper function */
+ if (!_bt_steppage(scan, dir))
+ return false;
+
+ /*
+ * _bt_readpage for a later page succeeded. There's no need to call
+ * _bt_drop_lock_and_maybe_pin here, though; _bt_readnextpage already did
+ * that for us.
+ */
+ return true;
+}
+
+/*
+ * _bt_readnextpage() -- Read next page containing valid data for _bt_next
+ *
+ * Caller's blkno is the next interesting page's link, taken from either the
+ * previously-saved right link or left link. lastcurrblkno is the page that
+ * was current at the point where the blkno link was saved, which we use to
+ * reason about concurrent page splits/page deletions during backwards scans
+ * (_bt_parallel_seize also requires it, regardless of scan direction).
+ *
+ * We work off of blkno and lastcurrblkno. On entry, so->currPos must be
+ * invalid; caller shouldn't hold any locks or pins on any page. Parallel
+ * scan callers must have seized the scan before calling here (and must pass
+ * us the blkno + lastcurrblkno they obtained as part of seizing the scan).
+ * However, we still expect so->currPos.moreLeft/so->currPos.moreLeft to
+ * remain however the last _bt_readpage call (for lastcurrblkno) set them.
+ *
+ * On success exit, so->currPos is updated to contain data from the next
+ * interesting page, and we return true. We hold a pin on the buffer on
+ * success exit, except for scans that _bt_drop_lock_and_maybe_pin indicates
+ * can safely have their pin dropped to avoid blocking progress by VACUUM.
+ *
+ * If there are no more matching records in the given direction, we leave
+ * so->currPos.buf set to InvalidBuffer, and return false. No locks or pins
+ * are retained. Note that this could just signal the end of the current
+ * primitive index scan (in which case another call to _bt_first will be
+ * required to continue the scan in the current scan direction).
+ *
+ * On exit, we'll always have released the seized scan for parallel scan
+ * callers (regardless of whether we were successful or not).
+ */
+static bool
+_bt_readnextpage(IndexScanDesc scan, BlockNumber blkno,
+ BlockNumber lastcurrblkno, ScanDirection dir)
+{
+ Relation rel = scan->indexRelation;
+ BTScanOpaque so = (BTScanOpaque) scan->opaque;
Page page;
BTPageOpaque opaque;
- bool status;
- rel = scan->indexRelation;
+ Assert(!BTScanPosIsValid(so->currPos));
+
+ /*
+ * Initialize currPos for blkno.
+ *
+ * We know that the scan just read a page (lastcurrblkno), so remember
+ * that we left a place with potentially matching tuples to blkno's left
+ * (or to blkno's right, when we're scanning backwards).
+ *
+ * Don't reset moreRight (or moreLeft if this is a backwards scan) here.
+ * If there is no more data to the right of lastcurrblkno, then there
+ * certainly cannot be any more to the right of its blkno right sibling
+ * (when we're scanning backwards then moreLeft similarly applies to both
+ * lastcurrblkno and blkno).
+ */
+ if (ScanDirectionIsForward(dir))
+ so->currPos.moreLeft = true;
+ else
+ so->currPos.moreRight = true;
if (ScanDirectionIsForward(dir))
{
@@ -2197,7 +2284,6 @@ _bt_readnextpage(IndexScanDesc scan, BlockNumber blkno, ScanDirection dir)
if (blkno == P_NONE || !so->currPos.moreRight)
{
_bt_parallel_done(scan);
- BTScanPosInvalidate(so->currPos);
return false;
}
/* check for interrupts while we're not holding any buffer lock */
@@ -2209,7 +2295,6 @@ _bt_readnextpage(IndexScanDesc scan, BlockNumber blkno, ScanDirection dir)
/* check for deleted page */
if (!P_IGNORE(opaque))
{
- PredicateLockPage(rel, blkno, scan->xs_snapshot);
/* see if there are any matches on this page */
/* note that this will clear moreRight if we can stop */
if (_bt_readpage(scan, dir, P_FIRSTDATAKEY(opaque), false))
@@ -2217,86 +2302,54 @@ _bt_readnextpage(IndexScanDesc scan, BlockNumber blkno, ScanDirection dir)
}
else if (scan->parallel_scan != NULL)
{
- /* allow next page be processed by parallel worker */
- _bt_parallel_release(scan, opaque->btpo_next);
+ /* _bt_readpage not called, so do this for ourselves */
+ _bt_parallel_release(scan, opaque->btpo_next, blkno);
}
- /* nope, keep going */
if (scan->parallel_scan != NULL)
{
+ /* no matching tuples, so seize a page to the right */
_bt_relbuf(rel, so->currPos.buf);
- status = _bt_parallel_seize(scan, &blkno, false);
- if (!status)
+
+ if (!_bt_parallel_seize(scan, &blkno, &lastcurrblkno, false))
{
- BTScanPosInvalidate(so->currPos);
+ /* don't call _bt_parallel_done */
return false;
}
}
else
{
+ /* no matching tuples, so consider following right link */
+ lastcurrblkno = blkno;
blkno = opaque->btpo_next;
_bt_relbuf(rel, so->currPos.buf);
}
+
+ BTScanPosInvalidate(so->currPos); /* working off new blkno now */
}
}
else
{
- /*
- * Should only happen in parallel cases, when some other backend
- * advanced the scan.
- */
- if (so->currPos.currPage != blkno)
- {
- BTScanPosUnpinIfPinned(so->currPos);
- so->currPos.currPage = blkno;
- }
-
- /* Done if we know that the left sibling link isn't of interest */
- if (!so->currPos.moreLeft)
- {
- BTScanPosUnpinIfPinned(so->currPos);
- _bt_parallel_done(scan);
- BTScanPosInvalidate(so->currPos);
- return false;
- }
-
- /*
- * Walk left to the next page with data. This is much more complex
- * than the walk-right case because of the possibility that the page
- * to our left splits while we are in flight to it, plus the
- * possibility that the page we were on gets deleted after we leave
- * it. See nbtree/README for details.
- *
- * It might be possible to rearrange this code to have less overhead
- * in pinning and locking, but that would require capturing the left
- * sibling block number when the page is initially read, and then
- * optimistically starting there (rather than pinning the page twice).
- * It is not clear that this would be worth the complexity.
- */
- if (BTScanPosIsPinned(so->currPos))
- _bt_lockbuf(rel, so->currPos.buf, BT_READ);
- else
- so->currPos.buf = _bt_getbuf(rel, so->currPos.currPage, BT_READ);
-
for (;;)
{
- /* Done if we know that the left sibling link isn't of interest */
- if (!so->currPos.moreLeft)
+ /*
+ * if we're at end of scan, give up and mark parallel scan as
+ * done, so that all the workers can finish their scan
+ */
+ if (blkno == P_NONE || !so->currPos.moreLeft)
{
- _bt_relbuf(rel, so->currPos.buf);
_bt_parallel_done(scan);
- BTScanPosInvalidate(so->currPos);
return false;
}
- /* Step to next physical page */
- so->currPos.buf = _bt_walk_left(rel, so->currPos.buf);
+ /* move left at least one page (checks for interrupts, too) */
+ so->currPos.buf = _bt_lock_and_validate_left(rel, &blkno,
+ lastcurrblkno);
- /* if we're physically at end of index, return failure */
if (so->currPos.buf == InvalidBuffer)
{
+ /* must have been a concurrent deletion of leftmost page */
_bt_parallel_done(scan);
- BTScanPosInvalidate(so->currPos);
return false;
}
@@ -2309,7 +2362,6 @@ _bt_readnextpage(IndexScanDesc scan, BlockNumber blkno, ScanDirection dir)
opaque = BTPageGetOpaque(page);
if (!P_IGNORE(opaque))
{
- PredicateLockPage(rel, BufferGetBlockNumber(so->currPos.buf), scan->xs_snapshot);
/* see if there are any matches on this page */
/* note that this will clear moreLeft if we can stop */
if (_bt_readpage(scan, dir, PageGetMaxOffsetNumber(page), false))
@@ -2317,100 +2369,80 @@ _bt_readnextpage(IndexScanDesc scan, BlockNumber blkno, ScanDirection dir)
}
else if (scan->parallel_scan != NULL)
{
- /* allow next page be processed by parallel worker */
- _bt_parallel_release(scan, BufferGetBlockNumber(so->currPos.buf));
+ /* _bt_readpage not called, so do this for ourselves */
+ Assert(P_ISHALFDEAD(opaque));
+ _bt_parallel_release(scan, opaque->btpo_prev, blkno);
}
- /*
- * For parallel scans, get the last page scanned as it is quite
- * possible that by the time we try to seize the scan, some other
- * worker has already advanced the scan to a different page. We
- * must continue based on the latest page scanned by any worker.
- */
if (scan->parallel_scan != NULL)
{
+ /* no matching tuples, so seize a page to the left */
_bt_relbuf(rel, so->currPos.buf);
- status = _bt_parallel_seize(scan, &blkno, false);
- if (!status)
+
+ if (!_bt_parallel_seize(scan, &blkno, &lastcurrblkno, false))
{
- BTScanPosInvalidate(so->currPos);
+ /* don't call _bt_parallel_done */
return false;
}
- so->currPos.buf = _bt_getbuf(rel, blkno, BT_READ);
}
+ else
+ {
+ /* no matching tuples, so consider following left link */
+ lastcurrblkno = blkno;
+ blkno = opaque->btpo_prev;
+ _bt_relbuf(rel, so->currPos.buf);
+ }
+
+ BTScanPosInvalidate(so->currPos); /* working off new blkno now */
}
}
- return true;
-}
-
-/*
- * _bt_parallel_readpage() -- Read current page containing valid data for scan
- *
- * On success, release lock and maybe pin on buffer. We return true to
- * indicate success.
- */
-static bool
-_bt_parallel_readpage(IndexScanDesc scan, BlockNumber blkno, ScanDirection dir)
-{
- BTScanOpaque so = (BTScanOpaque) scan->opaque;
-
- Assert(!so->needPrimScan);
-
- _bt_initialize_more_data(so, dir);
-
- if (!_bt_readnextpage(scan, blkno, dir))
- return false;
-
- /* We have at least one item to return as scan's next item */
+ /*
+ * _bt_readpage succeeded. Drop the lock (and maybe the pin) on
+ * so->currPos.buf in preparation for btgettuple returning tuples.
+ */
+ Assert(BTScanPosIsPinned(so->currPos));
_bt_drop_lock_and_maybe_pin(scan, &so->currPos);
return true;
}
/*
- * _bt_walk_left() -- step left one page, if possible
+ * _bt_lock_and_validate_left() -- lock caller's left sibling blkno,
+ * recovering from concurrent page splits/page deletions when necessary
*
- * The given buffer must be pinned and read-locked. This will be dropped
- * before stepping left. On return, we have pin and read lock on the
- * returned page, instead.
+ * Called during backwards scans, to deal with their unique concurrency rules.
*
- * Returns InvalidBuffer if there is no page to the left (no lock is held
- * in that case).
+ * blkno points to the block number of the page that we expect to move the
+ * scan to. We'll successfully move the scan there when we find that its
+ * right sibling link points to lastcurrblkno, the page that we just finished
+ * reading. Otherwise, we have to figure out which page is the correct one to
+ * visit next the hard way, which involves reasoning about both concurrent
+ * page splits and concurrent page deletions. See nbtree/README for details.
+ *
+ * On return, we have both a pin and a read lock on the returned page, whose
+ * block number will be set in *blkno. Returns InvalidBuffer if there is no
+ * page to the left (no lock is held in that case).
*
* It is possible for the returned leaf page to be half-dead; caller must
* check that condition and step left again when required.
*/
static Buffer
-_bt_walk_left(Relation rel, Buffer buf)
+_bt_lock_and_validate_left(Relation rel, BlockNumber *blkno,
+ BlockNumber lastcurrblkno)
{
- Page page;
- BTPageOpaque opaque;
-
- page = BufferGetPage(buf);
- opaque = BTPageGetOpaque(page);
+ BlockNumber origblkno = *blkno; /* detects circular links */
for (;;)
{
- BlockNumber obknum;
- BlockNumber lblkno;
- BlockNumber blkno;
+ Buffer buf;
+ Page page;
+ BTPageOpaque opaque;
int tries;
- /* if we're at end of tree, release buf and return failure */
- if (P_LEFTMOST(opaque))
- {
- _bt_relbuf(rel, buf);
- break;
- }
- /* remember original page we are stepping left from */
- obknum = BufferGetBlockNumber(buf);
- /* step left */
- blkno = lblkno = opaque->btpo_prev;
- _bt_relbuf(rel, buf);
/* check for interrupts while we're not holding any buffer lock */
CHECK_FOR_INTERRUPTS();
- buf = _bt_getbuf(rel, blkno, BT_READ);
+ buf = _bt_getbuf(rel, *blkno, BT_READ);
page = BufferGetPage(buf);
opaque = BTPageGetOpaque(page);
@@ -2427,21 +2459,26 @@ _bt_walk_left(Relation rel, Buffer buf)
tries = 0;
for (;;)
{
- if (!P_ISDELETED(opaque) && opaque->btpo_next == obknum)
+ if (!P_ISDELETED(opaque) && opaque->btpo_next == lastcurrblkno)
{
/* Found desired page, return it */
return buf;
}
if (P_RIGHTMOST(opaque) || ++tries > 4)
break;
- blkno = opaque->btpo_next;
- buf = _bt_relandgetbuf(rel, buf, blkno, BT_READ);
+ /* step right */
+ *blkno = opaque->btpo_next;
+ buf = _bt_relandgetbuf(rel, buf, *blkno, BT_READ);
page = BufferGetPage(buf);
opaque = BTPageGetOpaque(page);
}
- /* Return to the original page to see what's up */
- buf = _bt_relandgetbuf(rel, buf, obknum, BT_READ);
+ /*
+ * Return to the original page (usually the page most recently read by
+ * _bt_readpage, which is passed by caller as lastcurrblkno) to see
+ * what's up with sibling links
+ */
+ buf = _bt_relandgetbuf(rel, buf, lastcurrblkno, BT_READ);
page = BufferGetPage(buf);
opaque = BTPageGetOpaque(page);
if (P_ISDELETED(opaque))
@@ -2457,8 +2494,8 @@ _bt_walk_left(Relation rel, Buffer buf)
if (P_RIGHTMOST(opaque))
elog(ERROR, "fell off the end of index \"%s\"",
RelationGetRelationName(rel));
- blkno = opaque->btpo_next;
- buf = _bt_relandgetbuf(rel, buf, blkno, BT_READ);
+ *blkno = opaque->btpo_next;
+ buf = _bt_relandgetbuf(rel, buf, *blkno, BT_READ);
page = BufferGetPage(buf);
opaque = BTPageGetOpaque(page);
if (!P_ISDELETED(opaque))
@@ -2466,9 +2503,10 @@ _bt_walk_left(Relation rel, Buffer buf)
}
/*
- * Now return to top of loop, resetting obknum to point to this
- * nondeleted page, and try again.
+ * remember that this page is actually now the leftmost one whose
+ * key space we've already read all tuples from
*/
+ lastcurrblkno = BufferGetBlockNumber(buf);
}
else
{
@@ -2477,11 +2515,22 @@ _bt_walk_left(Relation rel, Buffer buf)
* to the left got split or deleted. Without this check, we'd go
* into an infinite loop if there's anything wrong.
*/
- if (opaque->btpo_prev == lblkno)
+ if (opaque->btpo_prev == origblkno)
elog(ERROR, "could not find left sibling of block %u in index \"%s\"",
- obknum, RelationGetRelationName(rel));
- /* Okay to try again with new lblkno value */
+ lastcurrblkno, RelationGetRelationName(rel));
+ /* Okay to try again, since left sibling link changed */
}
+
+ if (P_LEFTMOST(opaque))
+ {
+ /* previous leftmost page must have been concurrently deleted */
+ _bt_relbuf(rel, buf);
+ break;
+ }
+
+ /* next loop iteration will start over with a clean slate */
+ *blkno = origblkno = opaque->btpo_prev;
+ _bt_relbuf(rel, buf);
}
return InvalidBuffer;
@@ -2605,7 +2654,6 @@ _bt_endpoint(IndexScanDesc scan, ScanDirection dir)
return false;
}
- PredicateLockPage(rel, BufferGetBlockNumber(buf), scan->xs_snapshot);
page = BufferGetPage(buf);
opaque = BTPageGetOpaque(page);
Assert(P_ISLEAF(opaque));
@@ -2632,26 +2680,11 @@ _bt_endpoint(IndexScanDesc scan, ScanDirection dir)
/* remember which buffer we have pinned */
so->currPos.buf = buf;
- _bt_initialize_more_data(so, dir);
-
/*
* Now load data from the first page of the scan.
*/
- if (!_bt_readpage(scan, dir, start, true))
- {
- /*
- * There's no actually-matching data on this page. Try to advance to
- * the next page. Return false if there's no matching data at all.
- */
- _bt_unlockbuf(scan->indexRelation, so->currPos.buf);
- if (!_bt_steppage(scan, dir))
- return false;
- }
- else
- {
- /* We have at least one item to return as scan's first item */
- _bt_drop_lock_and_maybe_pin(scan, &so->currPos);
- }
+ if (!_bt_readfirstpage(scan, start, dir))
+ return false;
/* OK, itemIndex says what to return */
currItem = &so->currPos.items[so->currPos.itemIndex];
@@ -2661,33 +2694,3 @@ _bt_endpoint(IndexScanDesc scan, ScanDirection dir)
return true;
}
-
-/*
- * _bt_initialize_more_data() -- initialize moreLeft, moreRight and scan dir
- * from currPos
- */
-static inline void
-_bt_initialize_more_data(BTScanOpaque so, ScanDirection dir)
-{
- so->currPos.dir = dir;
- if (so->needPrimScan)
- {
- Assert(so->numArrayKeys);
-
- so->currPos.moreLeft = true;
- so->currPos.moreRight = true;
- so->needPrimScan = false;
- }
- else if (ScanDirectionIsForward(dir))
- {
- so->currPos.moreLeft = false;
- so->currPos.moreRight = true;
- }
- else
- {
- so->currPos.moreLeft = true;
- so->currPos.moreRight = false;
- }
- so->numKilled = 0; /* just paranoia */
- so->markItemIndex = -1; /* ditto */
-}
diff --git a/src/backend/access/nbtree/nbtutils.c b/src/backend/access/nbtree/nbtutils.c
index b4ba51357..03d5c671f 100644
--- a/src/backend/access/nbtree/nbtutils.c
+++ b/src/backend/access/nbtree/nbtutils.c
@@ -2442,7 +2442,7 @@ new_prim_scan:
so->needPrimScan = true; /* ...but call _bt_first again */
if (scan->parallel_scan)
- _bt_parallel_primscan_schedule(scan, pstate->prev_scan_page);
+ _bt_parallel_primscan_schedule(scan, so->currPos.currPage);
/* Caller's tuple doesn't match the new qual */
return false;
--
2.45.2
^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: Avoiding superfluous buffer locking during nbtree backwards scans
2024-10-11 23:29 Re: Avoiding superfluous buffer locking during nbtree backwards scans Peter Geoghegan <[email protected]>
@ 2024-11-07 10:44 ` Masahiro Ikeda <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Masahiro Ikeda @ 2024-11-07 10:44 UTC (permalink / raw)
To: Peter Geoghegan <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Matthias van de Meent <[email protected]>; Tomas Vondra <[email protected]>
Hi, thanks for working on these improvements.
I noticed an unexpected behavior where the index scan continues instead
of
stopping, even when it detects that there are no tuples that match the
conditions.
(I observed this while reviewing the skip scan patch, though it isn't
directly
related to this issue.)
On 2024-10-12 08:29, Peter Geoghegan wrote:
> On Thu, Oct 10, 2024 at 1:41 PM Peter Geoghegan <[email protected]> wrote:
> * We now reset currPos state (including even its moreLeft/moreRight
> fields) within _bt_parallel_seize, automatically and regardless of any
> other details.
IIUC, the above change is the root cause. The commit 1bd4bc8 adds a
reset of
the currPos state in _bt_parallel_seize(). However, this change can
overwrite
currPos.moreRight which should be preserved before calling
_bt_readnextpage().
* Test case
-- Prepare
DROP TABLE IF EXISTS test;
CREATE TABLE test (smallint smallint, bool bool);
INSERT INTO test (SELECT -20000+i%40000, random()>0.5 FROM
generate_series(1, 1_000_000) s(i));
CREATE INDEX test_smallint ON test (smallint);
VACUUM ANALYZE test;
-- Check the number of pages of the index
=# SELECT relpages FROM pg_class WHERE relname = 'test_smallint';
relpages
----------
937
(1 row)
-- Test
=# SET max_parallel_workers = 0;
=# EXPLAIN (ANALYZE, BUFFERS, VERBOSE) SELECT COUNT(*) FROM test WHERE
smallint < -10000;
QUERY PLAN
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Finalize Aggregate (cost=5170.23..5170.24 rows=1 width=8) (actual
time=71.352..71.402 rows=1 loops=1)
Output: count(*)
Buffers: shared hit=934
-> Gather (cost=5170.01..5170.22 rows=2 width=8) (actual
time=71.344..71.395 rows=1 loops=1)
Output: (PARTIAL count(*))
Workers Planned: 2
Workers Launched: 0
Buffers: shared hit=934
-> Partial Aggregate (cost=4170.01..4170.02 rows=1 width=8)
(actual time=71.199..71.199 rows=1 loops=1)
Output: PARTIAL count(*)
Buffers: shared hit=934
-> Parallel Index Only Scan using test_smallint on
public.test (cost=0.42..3906.27 rows=105495 width=0) (actual
time=0.062..49.137 rows=250000 loops=1)
Output: "smallint"
Index Cond: (test."smallint" < '-10000'::integer)
Heap Fetches: 0
Buffers: shared hit=934 -- This is not the result
I expected. Almost all relpages are being read to retrieve only 25% of
the tuples.
-- Without commit 1bd4bc8,
the number was '236' in my environment.
Planning Time: 0.105 ms
Execution Time: 71.454 ms
(18 rows)
Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2024-11-07 10:44 UTC | newest]
Thread overview: 3+ 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]>
2024-10-11 23:29 Re: Avoiding superfluous buffer locking during nbtree backwards scans Peter Geoghegan <[email protected]>
2024-11-07 10:44 ` Re: Avoiding superfluous buffer locking during nbtree backwards scans Masahiro Ikeda <[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