Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 75da2be

Browse files
committed
Fix read_stream.c for changing io_combine_limit.
In a couple of places, read_stream.c assumed that io_combine_limit would be stable during the lifetime of a stream. That is not true in at least one unusual case: streams held by CURSORs where you could change the GUC between FETCH commands, with unpredictable results. Fix, by storing stream->io_combine_limit and referring only to that after construction. This mirrors the treatment of the other important setting {effective,maintenance}_io_concurrency, which is stored in stream->max_ios. One of the cases was the queue overflow space, which was sized for io_combine_limit and could be overrun if the GUC was increased. Since that coding was a little hard to follow, also introduce a variable for better readability instead of open-coding the arithmetic. Doing so revealed an off-by-one thinko while clamping max_pinned_buffers to INT16_MAX, though that wasn't a live bug due to the current limits on GUC values. Back-patch to 17. Discussion: https://postgr.es/m/CA%2BhUKG%2B2T9p-%2BzM6Eeou-RAJjTML6eit1qn26f9twznX59qtCA%40mail.gmail.com
1 parent d4f7986 commit 75da2be

File tree

1 file changed

+27
-11
lines changed

1 file changed

+27
-11
lines changed

src/backend/storage/aio/read_stream.c

+27-11
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,7 @@ typedef struct InProgressIO
109109
struct ReadStream
110110
{
111111
int16 max_ios;
112+
int16 io_combine_limit;
112113
int16 ios_in_progress;
113114
int16 queue_size;
114115
int16 max_pinned_buffers;
@@ -236,7 +237,7 @@ read_stream_start_pending_read(ReadStream *stream, bool suppress_advice)
236237

237238
/* This should only be called with a pending read. */
238239
Assert(stream->pending_read_nblocks > 0);
239-
Assert(stream->pending_read_nblocks <= io_combine_limit);
240+
Assert(stream->pending_read_nblocks <= stream->io_combine_limit);
240241

241242
/* We had better not exceed the pin limit by starting this read. */
242243
Assert(stream->pinned_buffers + stream->pending_read_nblocks <=
@@ -324,7 +325,7 @@ read_stream_look_ahead(ReadStream *stream, bool suppress_advice)
324325
int16 buffer_index;
325326
void *per_buffer_data;
326327

327-
if (stream->pending_read_nblocks == io_combine_limit)
328+
if (stream->pending_read_nblocks == stream->io_combine_limit)
328329
{
329330
read_stream_start_pending_read(stream, suppress_advice);
330331
suppress_advice = false;
@@ -384,7 +385,7 @@ read_stream_look_ahead(ReadStream *stream, bool suppress_advice)
384385
* signaled end-of-stream, we start the read immediately.
385386
*/
386387
if (stream->pending_read_nblocks > 0 &&
387-
(stream->pending_read_nblocks == io_combine_limit ||
388+
(stream->pending_read_nblocks == stream->io_combine_limit ||
388389
(stream->pending_read_nblocks == stream->distance &&
389390
stream->pinned_buffers == 0) ||
390391
stream->distance == 0) &&
@@ -415,6 +416,7 @@ read_stream_begin_impl(int flags,
415416
ReadStream *stream;
416417
size_t size;
417418
int16 queue_size;
419+
int16 queue_overflow;
418420
int max_ios;
419421
int strategy_pin_limit;
420422
uint32 max_pinned_buffers;
@@ -445,6 +447,14 @@ read_stream_begin_impl(int flags,
445447
/* Cap to INT16_MAX to avoid overflowing below */
446448
max_ios = Min(max_ios, PG_INT16_MAX);
447449

450+
/*
451+
* If starting a multi-block I/O near the end of the queue, we might
452+
* temporarily need extra space for overflowing buffers before they are
453+
* moved to regular circular position. This is the maximum extra space we
454+
* could need.
455+
*/
456+
queue_overflow = io_combine_limit - 1;
457+
448458
/*
449459
* Choose the maximum number of buffers we're prepared to pin. We try to
450460
* pin fewer if we can, though. We add one so that we can make progress
@@ -459,7 +469,7 @@ read_stream_begin_impl(int flags,
459469
*/
460470
max_pinned_buffers = (max_ios + 1) * io_combine_limit;
461471
max_pinned_buffers = Min(max_pinned_buffers,
462-
PG_INT16_MAX - io_combine_limit - 1);
472+
PG_INT16_MAX - queue_overflow - 1);
463473

464474
/* Give the strategy a chance to limit the number of buffers we pin. */
465475
strategy_pin_limit = GetAccessStrategyPinLimit(strategy);
@@ -485,18 +495,17 @@ read_stream_begin_impl(int flags,
485495
* one big chunk. Though we have queue_size buffers, we want to be able
486496
* to assume that all the buffers for a single read are contiguous (i.e.
487497
* don't wrap around halfway through), so we allow temporary overflows of
488-
* up to the maximum possible read size by allocating an extra
489-
* io_combine_limit - 1 elements.
498+
* up to the maximum possible overflow size.
490499
*/
491500
size = offsetof(ReadStream, buffers);
492-
size += sizeof(Buffer) * (queue_size + io_combine_limit - 1);
501+
size += sizeof(Buffer) * (queue_size + queue_overflow);
493502
size += sizeof(InProgressIO) * Max(1, max_ios);
494503
size += per_buffer_data_size * queue_size;
495504
size += MAXIMUM_ALIGNOF * 2;
496505
stream = (ReadStream *) palloc(size);
497506
memset(stream, 0, offsetof(ReadStream, buffers));
498507
stream->ios = (InProgressIO *)
499-
MAXALIGN(&stream->buffers[queue_size + io_combine_limit - 1]);
508+
MAXALIGN(&stream->buffers[queue_size + queue_overflow]);
500509
if (per_buffer_data_size > 0)
501510
stream->per_buffer_data = (void *)
502511
MAXALIGN(&stream->ios[Max(1, max_ios)]);
@@ -523,7 +532,14 @@ read_stream_begin_impl(int flags,
523532
if (max_ios == 0)
524533
max_ios = 1;
525534

535+
/*
536+
* Capture stable values for these two GUC-derived numbers for the
537+
* lifetime of this stream, so we don't have to worry about the GUCs
538+
* changing underneath us beyond this point.
539+
*/
526540
stream->max_ios = max_ios;
541+
stream->io_combine_limit = io_combine_limit;
542+
527543
stream->per_buffer_data_size = per_buffer_data_size;
528544
stream->max_pinned_buffers = max_pinned_buffers;
529545
stream->queue_size = queue_size;
@@ -537,7 +553,7 @@ read_stream_begin_impl(int flags,
537553
* doing full io_combine_limit sized reads (behavior B).
538554
*/
539555
if (flags & READ_STREAM_FULL)
540-
stream->distance = Min(max_pinned_buffers, io_combine_limit);
556+
stream->distance = Min(max_pinned_buffers, stream->io_combine_limit);
541557
else
542558
stream->distance = 1;
543559

@@ -752,14 +768,14 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data)
752768
else
753769
{
754770
/* No advice; move towards io_combine_limit (behavior B). */
755-
if (stream->distance > io_combine_limit)
771+
if (stream->distance > stream->io_combine_limit)
756772
{
757773
stream->distance--;
758774
}
759775
else
760776
{
761777
distance = stream->distance * 2;
762-
distance = Min(distance, io_combine_limit);
778+
distance = Min(distance, stream->io_combine_limit);
763779
distance = Min(distance, stream->max_pinned_buffers);
764780
stream->distance = distance;
765781
}

0 commit comments

Comments
 (0)