public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 1/2] fix WALRead API to take seg/segcxt from XLogReaderState
2+ messages / 2 participants
[nested] [flat]
* [PATCH 1/2] fix WALRead API to take seg/segcxt from XLogReaderState
@ 2020-05-08 20:40 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: Alvaro Herrera @ 2020-05-08 20:40 UTC (permalink / raw)
---
src/backend/access/transam/xlogreader.c | 31 +++++++++++--------------
src/backend/access/transam/xlogutils.c | 1 -
src/backend/replication/walsender.c | 6 ++---
src/bin/pg_waldump/pg_waldump.c | 1 -
src/include/access/xlogreader.h | 4 +---
5 files changed, 17 insertions(+), 26 deletions(-)
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 7cee8b92c9..f42dee2640 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -1050,8 +1050,6 @@ err:
* Read 'count' bytes into 'buf', starting at location 'startptr', from WAL
* fetched from timeline 'tli'.
*
- * 'seg/segcxt' identify the last segment used.
- *
* Returns true if succeeded, false if an error occurs, in which case
* 'errinfo' receives error details.
*
@@ -1061,7 +1059,6 @@ err:
bool
WALRead(XLogReaderState *state,
char *buf, XLogRecPtr startptr, Size count, TimeLineID tli,
- WALOpenSegment *seg, WALSegmentContext *segcxt,
WALReadError *errinfo)
{
char *p;
@@ -1078,34 +1075,34 @@ WALRead(XLogReaderState *state,
int segbytes;
int readbytes;
- startoff = XLogSegmentOffset(recptr, segcxt->ws_segsize);
+ startoff = XLogSegmentOffset(recptr, state->segcxt.ws_segsize);
/*
* If the data we want is not in a segment we have open, close what we
* have (if anything) and open the next one, using the caller's
* provided openSegment callback.
*/
- if (seg->ws_file < 0 ||
- !XLByteInSeg(recptr, seg->ws_segno, segcxt->ws_segsize) ||
- tli != seg->ws_tli)
+ if (state->seg.ws_file < 0 ||
+ !XLByteInSeg(recptr, state->seg.ws_segno, state->segcxt.ws_segsize) ||
+ tli != state->seg.ws_tli)
{
XLogSegNo nextSegNo;
- if (seg->ws_file >= 0)
+ if (state->seg.ws_file >= 0)
state->routine.segment_close(state);
- XLByteToSeg(recptr, nextSegNo, segcxt->ws_segsize);
- seg->ws_file = state->routine.segment_open(state, nextSegNo,
- segcxt, &tli);
+ XLByteToSeg(recptr, nextSegNo, state->segcxt.ws_segsize);
+ state->seg.ws_file = state->routine.segment_open(state, nextSegNo,
+ &state->segcxt, &tli);
/* Update the current segment info. */
- seg->ws_tli = tli;
- seg->ws_segno = nextSegNo;
+ state->seg.ws_tli = tli;
+ state->seg.ws_segno = nextSegNo;
}
/* How many bytes are within this segment? */
- if (nbytes > (segcxt->ws_segsize - startoff))
- segbytes = segcxt->ws_segsize - startoff;
+ if (nbytes > (state->segcxt.ws_segsize - startoff))
+ segbytes = state->segcxt.ws_segsize - startoff;
else
segbytes = nbytes;
@@ -1115,7 +1112,7 @@ WALRead(XLogReaderState *state,
/* Reset errno first; eases reporting non-errno-affecting errors */
errno = 0;
- readbytes = pg_pread(seg->ws_file, p, segbytes, (off_t) startoff);
+ readbytes = pg_pread(state->seg.ws_file, p, segbytes, (off_t) startoff);
#ifndef FRONTEND
pgstat_report_wait_end();
@@ -1127,7 +1124,7 @@ WALRead(XLogReaderState *state,
errinfo->wre_req = segbytes;
errinfo->wre_read = readbytes;
errinfo->wre_off = startoff;
- errinfo->wre_seg = *seg;
+ errinfo->wre_seg = state->seg;
return false;
}
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index bbd801513a..fc0bb7d059 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -947,7 +947,6 @@ read_local_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr,
* zero-padded up to the page boundary if it's incomplete.
*/
if (!WALRead(state, cur_page, targetPagePtr, XLOG_BLCKSZ, tli,
- &state->seg, &state->segcxt,
&errinfo))
WALReadRaiseError(&errinfo);
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index d18475b854..ed8c08cb6a 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -840,8 +840,6 @@ logical_read_xlog_page(XLogReaderState *state, XLogRecPtr targetPagePtr, int req
sendSeg->ws_tli, /* Pass the current TLI because only
* WalSndSegmentOpen controls whether new
* TLI is needed. */
- sendSeg,
- sendCxt,
&errinfo))
WALReadRaiseError(&errinfo);
@@ -2760,6 +2758,8 @@ XLogSendPhysical(void)
enlargeStringInfo(&output_message, nbytes);
retry:
+ fake_xlogreader.seg = *sendSeg;
+ fake_xlogreader.segcxt = *sendCxt;
if (!WALRead(&fake_xlogreader,
&output_message.data[output_message.len],
startptr,
@@ -2767,8 +2767,6 @@ retry:
sendSeg->ws_tli, /* Pass the current TLI because only
* WalSndSegmentOpen controls whether new
* TLI is needed. */
- sendSeg,
- sendCxt,
&errinfo))
WALReadRaiseError(&errinfo);
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index e29f65500f..46734914b7 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -356,7 +356,6 @@ WALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
}
if (!WALRead(state, readBuff, targetPagePtr, count, private->timeline,
- &state->seg, &state->segcxt,
&errinfo))
{
WALOpenSegment *seg = &errinfo.wre_seg;
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index 81af200f5e..e77f478d68 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -301,9 +301,7 @@ typedef struct WALReadError
extern bool WALRead(XLogReaderState *state,
char *buf, XLogRecPtr startptr, Size count,
- TimeLineID tli, WALOpenSegment *seg,
- WALSegmentContext *segcxt,
- WALReadError *errinfo);
+ TimeLineID tli, WALReadError *errinfo);
/* Functions for decoding an XLogRecord */
--
2.20.1
--/04w6evG8XlLl3ft
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0002-Simplify-XLogReader-s-open_segment-API.patch"
^ permalink raw reply [nested|flat] 2+ messages in thread
* Re: [PATCHES] Post-special page storage TDE support
@ 2023-11-08 00:20 Andres Freund <[email protected]>
0 siblings, 0 replies; 2+ messages in thread
From: Andres Freund @ 2023-11-08 00:20 UTC (permalink / raw)
To: David Christensen <[email protected]>; +Cc: Matthias van de Meent <[email protected]>; pgsql-hackers; Stephen Frost <[email protected]>
Hi,
On 2023-05-09 17:08:26 -0500, David Christensen wrote:
> From 965309ea3517fa734c4bc89c144e2031cdf6c0c3 Mon Sep 17 00:00:00 2001
> From: David Christensen <[email protected]>
> Date: Tue, 9 May 2023 16:56:15 -0500
> Subject: [PATCH v4 1/3] Add reserved_page_space to Page structure
>
> This space is reserved for extended data on the Page structure which will be ultimately used for
> encrypted data, extended checksums, and potentially other things. This data appears at the end of
> the Page, after any `pd_special` area, and will be calculated at runtime based on specific
> ControlFile features.
>
> No effort is made to ensure this is backwards-compatible with existing clusters for `pg_upgrade`, as
> we will require logical replication to move data into a cluster with
> different settings here.
The first part of the last paragraph makes it sound like pg_upgrade won't be
supported across this commit, rather than just between different settings...
I think as a whole this is not an insane idea. A few comments:
- IMO the patch touches many places it shouldn't need to touch, because of
essentially renaming a lot of existing macro names to *Limit,
necessitating modifying a lot of users. I think instead the few places that
care about the runtime limit should be modified.
As-is the patch would cause a lot of fallout in extensions that just do
things like defining an on-stack array of Datums or such - even though all
they'd need is to change the define to the *Limit one.
Even leaving extensions aside, it must makes reviewing (and I'm sure
maintaining) the patch very tedious.
- I'm a bit worried about how the extra special page will be managed - if
there are multiple features that want to use it, who gets to put their data
at what offset?
After writing this I saw that 0002 tries to address this - but I don't like
the design. It introduces runtime overhead that seems likely to be visible.
- Checking for features using PageGetFeatureOffset() seems the wrong design to
me - instead of a branch for some feature being disabled, perfectly
predictable for the CPU, we need to do an external function call every time
to figure out that yet, checksums are *still* disabled.
- Recomputing offsets every time in PageGetFeatureOffset() seems too
expensive. The offsets can't change while running as PageGetFeatureOffset()
have enough information to distinguish between different kinds of relations
- so why do we need to recompute offsets on every single page? I'd instead
add a distinct offset variable for each feature.
- Modifying every single PageInit() call doesn't make sense to me. That'll
just create a lot of breakage for - as far as I can tell - no win.
- Why is it worth sacrificing space on every page to indicate which features
were enabled? I think there'd need to be some convincing reasons for
introducing such overhead.
- Is it really useful to encode the set of features enabled in a cluster with
a bitmask? That pretty much precludes utilizing extra page space in
extensions. We could instead just have an extra cluster-wide file that
defines a mapping of offset to feature.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 2+ messages in thread
end of thread, other threads:[~2023-11-08 00:20 UTC | newest]
Thread overview: 2+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-05-08 20:40 [PATCH 1/2] fix WALRead API to take seg/segcxt from XLogReaderState Alvaro Herrera <[email protected]>
2023-11-08 00:20 Re: [PATCHES] Post-special page storage TDE support Andres Freund <[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