public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v4 1/4] Implement FIRST_IS_ABORTED_CONTRECORD
4+ messages / 3 participants
[nested] [flat]
* [PATCH v4 1/4] Implement FIRST_IS_ABORTED_CONTRECORD
@ 2021-09-02 21:21 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Alvaro Herrera @ 2021-09-02 21:21 UTC (permalink / raw)
---
src/backend/access/transam/xlog.c | 55 +++++++++++++++++++++++--
src/backend/access/transam/xlogreader.c | 39 +++++++++++++++++-
src/include/access/xlog_internal.h | 14 ++++++-
src/include/access/xlogreader.h | 3 ++
4 files changed, 105 insertions(+), 6 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index e51a7a749d..49912483d5 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -586,6 +586,8 @@ typedef struct XLogCtlData
XLogRecPtr replicationSlotMinLSN; /* oldest LSN needed by any slot */
XLogSegNo lastRemovedSegNo; /* latest removed/recycled XLOG segment */
+ XLogRecPtr abortedContrecordPtr; /* LSN of incomplete record at end of
+ * WAL */
/* Fake LSN counter, for unlogged relations. Protected by ulsn_lck. */
XLogRecPtr unloggedLSN;
@@ -848,6 +850,7 @@ static XLogSource XLogReceiptSource = XLOG_FROM_ANY;
/* State information for XLOG reading */
static XLogRecPtr ReadRecPtr; /* start of last record read */
static XLogRecPtr EndRecPtr; /* end+1 of last record read */
+static XLogRecPtr abortedContrecordPtr; /* end+1 of incomplete record */
/*
* Local copies of equivalent fields in the control file. When running
@@ -2246,6 +2249,31 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, bool opportunistic)
if (!Insert->forcePageWrites)
NewPage->xlp_info |= XLP_BKP_REMOVABLE;
+ /*
+ * If the last page ended with an aborted partial continuation record,
+ * mark the new page to indicate that the partial record can be
+ * omitted.
+ *
+ * This happens only once at the end of recovery, so there's no race
+ * condition here.
+ */
+ if (XLogCtl->abortedContrecordPtr >= NewPageBeginPtr)
+ {
+#ifdef WAL_DEBUG
+ if (XLogCtl->abortedContrecordPtr != NewPageBeginPtr)
+ elog(PANIC, "inconsistent aborted contrecord location %X/%X, expected %X/%X",
+ LSN_FORMAT_ARGS(XLogCtl->abortedContrecordPtr),
+ LSN_FORMAT_ARGS(NewPageBeginPtr));
+ if (XLOG_DEBUG)
+ ereport(LOG,
+ (errmsg_internal("setting XLP_FIRST_IS_ABORTED_PARTIAL flag at %X/%X",
+ LSN_FORMAT_ARGS(NewPageBeginPtr))));
+#endif
+ NewPage->xlp_info |= XLP_FIRST_IS_ABORTED_PARTIAL;
+
+ XLogCtl->abortedContrecordPtr = InvalidXLogRecPtr;
+ }
+
/*
* If first page of an XLOG segment file, make it a long header.
*/
@@ -4392,6 +4420,7 @@ ReadRecord(XLogReaderState *xlogreader, int emode,
record = XLogReadRecord(xlogreader, &errormsg);
ReadRecPtr = xlogreader->ReadRecPtr;
EndRecPtr = xlogreader->EndRecPtr;
+ abortedContrecordPtr = xlogreader->abortedContrecordPtr;
if (record == NULL)
{
if (readFile >= 0)
@@ -7691,10 +7720,30 @@ StartupXLOG(void)
/*
* Re-fetch the last valid or last applied record, so we can identify the
* exact endpoint of what we consider the valid portion of WAL.
+ *
+ * When recovery ended in an incomplete record, continue writing from the
+ * point where it went missing. This leaves behind an initial part of
+ * broken record, which rescues downstream which have already received
+ * that first part.
*/
- XLogBeginRead(xlogreader, LastRec);
- record = ReadRecord(xlogreader, PANIC, false);
- EndOfLog = EndRecPtr;
+ if (XLogRecPtrIsInvalid(abortedContrecordPtr))
+ {
+ XLogBeginRead(xlogreader, LastRec);
+ record = ReadRecord(xlogreader, PANIC, false);
+ EndOfLog = EndRecPtr;
+ }
+ else
+ {
+#ifdef WAL_DEBUG
+ if (XLOG_DEBUG)
+ ereport(LOG,
+ (errmsg_internal("recovery overwriting broken contrecord at %X/%X (EndRecPtr: %X/%X)",
+ LSN_FORMAT_ARGS(abortedContrecordPtr),
+ LSN_FORMAT_ARGS(EndRecPtr))));
+#endif
+ EndOfLog = abortedContrecordPtr;
+ XLogCtl->abortedContrecordPtr = abortedContrecordPtr;
+ }
/*
* EndOfLogTLI is the TLI in the filename of the XLOG segment containing
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 5cf74e181a..dbfa6d3562 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -278,6 +278,7 @@ XLogReadRecord(XLogReaderState *state, char **errormsg)
total_len;
uint32 targetRecOff;
uint32 pageHeaderSize;
+ bool assembled;
bool gotheader;
int readOff;
@@ -293,6 +294,7 @@ XLogReadRecord(XLogReaderState *state, char **errormsg)
state->errormsg_buf[0] = '\0';
ResetDecoder(state);
+ state->abortedContrecordPtr = InvalidXLogRecPtr;
RecPtr = state->EndRecPtr;
@@ -319,7 +321,9 @@ XLogReadRecord(XLogReaderState *state, char **errormsg)
randAccess = true;
}
+restart:
state->currRecPtr = RecPtr;
+ assembled = false;
targetPagePtr = RecPtr - (RecPtr % XLOG_BLCKSZ);
targetRecOff = RecPtr % XLOG_BLCKSZ;
@@ -415,6 +419,8 @@ XLogReadRecord(XLogReaderState *state, char **errormsg)
char *buffer;
uint32 gotlen;
+ assembled = true;
+
/*
* Enlarge readRecordBuf as needed.
*/
@@ -442,14 +448,28 @@ XLogReadRecord(XLogReaderState *state, char **errormsg)
readOff = ReadPageInternal(state, targetPagePtr,
Min(total_len - gotlen + SizeOfXLogShortPHD,
XLOG_BLCKSZ));
-
if (readOff < 0)
goto err;
Assert(SizeOfXLogShortPHD <= readOff);
- /* Check that the continuation on next page looks valid */
pageHeader = (XLogPageHeader) state->readBuf;
+
+ /*
+ * If we were expecting a continuation record and got an "aborted
+ * partial" flag, that means the continuation record was lost.
+ * Ignore the record we were reading, since we now know it's broken
+ * and lost forever, and restart the read by assuming the address
+ * to read is the location where we found this flag.
+ */
+ if (pageHeader->xlp_info & XLP_FIRST_IS_ABORTED_PARTIAL)
+ {
+ ResetDecoder(state);
+ RecPtr = targetPagePtr;
+ goto restart;
+ }
+
+ /* Check that the continuation on next page looks valid */
if (!(pageHeader->xlp_info & XLP_FIRST_IS_CONTRECORD))
{
report_invalid_record(state,
@@ -551,6 +571,21 @@ XLogReadRecord(XLogReaderState *state, char **errormsg)
return NULL;
err:
+ if (assembled)
+ {
+ /*
+ * We get here when a record that spans multiple pages needs to be
+ * assembled, but something went wrong -- perhaps a contrecord piece
+ * was lost. We deal with this by setting abortedContrecordPtr to the
+ * location of the piece we failed to read, or the start of the page
+ * we read where validation failed. If caller is WAL replay, it will
+ * know that recovery ended and that this is where to start writing
+ * future WAL marking the next piece with XLP_FIRST_IS_ABORTED_PARTIAL,
+ * which will in turn signal downstream WAL consumers that the broken
+ * WAL record here is to be ignored.
+ */
+ state->abortedContrecordPtr = targetPagePtr;
+ }
/*
* Invalidate the read state. We might read from a different source after
diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index 3b5eceff65..9bc72b4c95 100644
--- a/src/include/access/xlog_internal.h
+++ b/src/include/access/xlog_internal.h
@@ -76,8 +76,20 @@ typedef XLogLongPageHeaderData *XLogLongPageHeader;
#define XLP_LONG_HEADER 0x0002
/* This flag indicates backup blocks starting in this page are optional */
#define XLP_BKP_REMOVABLE 0x0004
+/*
+ * This flag marks a record that replaces a missing contrecord.
+ * When on WAL replay we expect a continuation record at the start of
+ * a page that is not there, recovery ends but the checkpoint record
+ * that follows is marked with this flag, which indicates WAL readers
+ * that the incomplete record is to be skipped, and that WAL reading
+ * is to be resumed here. This is useful for downstream consumers of
+ * WAL which have already received (the first half of) the original
+ * broken WAL record, such as via archive_command or physical streaming
+ * replication, which we cannot "rewind".
+ */
+#define XLP_FIRST_IS_ABORTED_PARTIAL 0x0008
/* All defined flag bits in xlp_info (used for validity checking of header) */
-#define XLP_ALL_FLAGS 0x0007
+#define XLP_ALL_FLAGS 0x000F
#define XLogPageHeaderSize(hdr) \
(((hdr)->xlp_info & XLP_LONG_HEADER) ? SizeOfXLogLongPHD : SizeOfXLogShortPHD)
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index 21d200d3df..96e5eab1c9 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -175,6 +175,9 @@ struct XLogReaderState
XLogRecPtr ReadRecPtr; /* start of last record read */
XLogRecPtr EndRecPtr; /* end+1 of last record read */
+ /* end+1 of incomplete record at end of WAL */
+ XLogRecPtr abortedContrecordPtr;
+
/* ----------------------------------------
* Decoded representation of current record
--
2.30.2
--3fun6kp47uctezw6
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
filename="v4-0002-crosscheck-that-FIRST_IS_CONTRECORD-is-not-togeth.patch"
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: SQL:2011 application time
@ 2022-01-06 05:44 Paul A Jungwirth <[email protected]>
2022-01-10 08:53 ` Re: SQL:2011 application time Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Paul A Jungwirth @ 2022-01-06 05:44 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Corey Huinker <[email protected]>; PostgreSQL Hackers <[email protected]>; Jaime Casanova <[email protected]>
On Wed, Jan 5, 2022 at 8:07 AM Peter Eisentraut
<[email protected]> wrote:
>
> This patch set looks very interesting.
Thank you for the review!
I'll work on your feedback but in the meantime here are replies to
your questions:
> I'm confused about how to query tables based on application time
> periods. Online, I see examples using AS OF, but in the SQL standard
> I only see this used for system time, which we are not doing here.
Correct, the standard only gives it for system time. I think
application time is intended to be more "in user space" so it's fine
to use regular operators in your WHERE condition against the time
columns, whereas system time is more of a managed thing---automatic,
read-only, possibly stored in a separate table. Having a special
syntax cue lets the RDBMS know it needs to involve the historical
records.
> validate_period(): Could use an explanatory comment. There are a
> bunch of output arguments, and it's not clear what all of this is
> supposed to do, and what "validating" is in this context.
I'm not too happy with that function, but a previous reviewer asked me
to factor out what was shared between the CREATE TABLE and ALTER TABLE
cases. It does some sanity checks on the columns you've chosen, and
along the way it collects info about those columns that we'll need
later. But yeah all those out parameters are pretty ugly. I'll see if
I can come up with a stronger abstraction for it, and at the very
least I'll add some comments.
> MergeAttributes(): I would perhaps initially just prohibit inheritance
> situations that involve periods on either side. (It should work for
> partitioning, IMO, but that should be easier to arrange.)
Okay. I'm glad to hear you think partitioning won't be too hard. It is
one of the last things, but to me it's a bit intimidating.
> I didn't follow why indexes would have periods, for example, the new
> period field in IndexStmt. Is that explained anywhere?
When you create a primary key or a unique constraint (which are backed
by a unique index), you can give a period name to make it a temporal
constraint. We create the index first and then create the constraint
as a side-effect of that (e.g. index_create calls
index_constraint_create). The analysis phase generates an IndexStmt.
So I think this was mostly a way to pass the period info down to the
constraint. It probably doesn't actually need to be stored on pg_index
though. Maybe it does for index_concurrently_create_copy. I'll add
some comments, but if you think it's the wrong approach let me know.
> While reading this patch I kept wondering whether it would be possible
> to fold periods into pg_attribute, perhaps with negative attribute
> numbers. Have you looked into something like that? No doubt it's
> also complicated, but it might simplify some things, like the name
> conflict checking.
Hmm, I thought that sort of thing would be frowned upon. :-) But also
it seems like periods really do have a bunch of details they need
beyond what other attributes have (e.g. the two source attributes, the
matching range type, the period type (application-vs-system), maybe
some extra things for table inheritance.
Also are you sure we aren't already using negative attnums somewhere
already? I thought I saw something like that.
> Of course, the main problem in this patch is that for most uses it
> requires btree_gist. I think we should consider moving that into
> core, or at least the support for types that are most relevant to this
> functionality, specifically the date/time types. Aside from user
> convenience, this would also allow writing more realistic test cases.
I think this would be great too. How realistic do you think it is? I
figured since exclusion constraints are also pretty useless without
btree_gist, it wasn't asking too much to have people install the
extension, but still it'd be better if it were all built in.
> src/backend/access/brin/brin_minmax_multi.c
>
> These renaming changes seem unrelated (but still seem like a good
> idea). Should they be progressed separately?
I can pull this out into a separate patch. I needed to do it because
when I added an `#include <rangetypes.h>` somewhere, these conflicted
with the range_{de,}serialize functions declared there.
> I don't understand why a temporal primary key is required for doing
> UPDATE FOR PORTION OF. I don't see this in the standard.
You're right, it's not in the standard. I'm doing that because
creating the PK is when we add the triggers to implement UPDATE FOR
PORTION OF. I thought it was acceptable since we also require a
PK/unique constraint as the referent of a foreign key. But we could
avoid it if I went back to the executor-based FOR PORTION OF
implementation, since that doesn't depend on triggers. What do you
think?
Also: I noticed recently that you can't use FOR PORTION OF against an
updatable view. I'm working on a new patch set to fix that. But the
main reason is this PK check. So that's maybe another reason to go
back to the executor implementation.
> How to proceed. I suppose we could focus on committing 0001 and 0002
> first.
That would be great! I don't think either is likely to conflict with
future system-time work.
> Is there anything else you think we can do as
> preparatory work to make the main patches more manageable?
I think it would be smart to have a rough plan for how this work will
be compatible with system-time support. Corey & I have talked about
that a lot, and In general they are orthogonal, but it would be nice
to have details written down somewhere.
Yours,
Paul
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: SQL:2011 application time
2022-01-06 05:44 Re: SQL:2011 application time Paul A Jungwirth <[email protected]>
@ 2022-01-10 08:53 ` Peter Eisentraut <[email protected]>
2022-01-19 08:32 ` Re: SQL:2011 application time Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Peter Eisentraut @ 2022-01-10 08:53 UTC (permalink / raw)
To: Paul A Jungwirth <[email protected]>; +Cc: Corey Huinker <[email protected]>; PostgreSQL Hackers <[email protected]>; Jaime Casanova <[email protected]>
On 06.01.22 06:44, Paul A Jungwirth wrote:
>> I didn't follow why indexes would have periods, for example, the new
>> period field in IndexStmt. Is that explained anywhere?
>
> When you create a primary key or a unique constraint (which are backed
> by a unique index), you can give a period name to make it a temporal
> constraint. We create the index first and then create the constraint
> as a side-effect of that (e.g. index_create calls
> index_constraint_create). The analysis phase generates an IndexStmt.
> So I think this was mostly a way to pass the period info down to the
> constraint. It probably doesn't actually need to be stored on pg_index
> though. Maybe it does for index_concurrently_create_copy. I'll add
> some comments, but if you think it's the wrong approach let me know.
This seems backwards. Currently, when you create a constraint, the
index is created as a side effect and is owned, so to speak, by the
constraint. What you are describing here sounds like the index owns the
constraint. This needs to be reconsidered, I think.
>> Of course, the main problem in this patch is that for most uses it
>> requires btree_gist. I think we should consider moving that into
>> core, or at least the support for types that are most relevant to this
>> functionality, specifically the date/time types. Aside from user
>> convenience, this would also allow writing more realistic test cases.
>
> I think this would be great too. How realistic do you think it is? I
> figured since exclusion constraints are also pretty useless without
> btree_gist, it wasn't asking too much to have people install the
> extension, but still it'd be better if it were all built in.
IMO, if this temporal feature is to happen, btree_gist needs to be moved
into core first. Having to install an extension in order to use an
in-core feature like this isn't going to be an acceptable experience.
>> src/backend/access/brin/brin_minmax_multi.c
>>
>> These renaming changes seem unrelated (but still seem like a good
>> idea). Should they be progressed separately?
>
> I can pull this out into a separate patch. I needed to do it because
> when I added an `#include <rangetypes.h>` somewhere, these conflicted
> with the range_{de,}serialize functions declared there.
OK, I have committed this separately.
>> I don't understand why a temporal primary key is required for doing
>> UPDATE FOR PORTION OF. I don't see this in the standard.
>
> You're right, it's not in the standard. I'm doing that because
> creating the PK is when we add the triggers to implement UPDATE FOR
> PORTION OF. I thought it was acceptable since we also require a
> PK/unique constraint as the referent of a foreign key.
That part *is* in the standard.
> But we could
> avoid it if I went back to the executor-based FOR PORTION OF
> implementation, since that doesn't depend on triggers. What do you
> think?
I think it's worth trying to do this without triggers.
But if you are just looking for a way to create the triggers, why are
they not just created when the table is created?
> I think it would be smart to have a rough plan for how this work will
> be compatible with system-time support. Corey & I have talked about
> that a lot, and In general they are orthogonal, but it would be nice
> to have details written down somewhere.
I personally don't see why we need to worry about system time now.
System time seems quite a complicated feature, since you have to figure
out a system to store and clean the old data, whereas this application
time feature is ultimately mostly syntax sugar around ranges and
exclusion constraints. As long as we keep the standard syntax for
system time available for future use (which is what your patch does), I
don't see a need to go deeper right now.
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: SQL:2011 application time
2022-01-06 05:44 Re: SQL:2011 application time Paul A Jungwirth <[email protected]>
2022-01-10 08:53 ` Re: SQL:2011 application time Peter Eisentraut <[email protected]>
@ 2022-01-19 08:32 ` Peter Eisentraut <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Peter Eisentraut @ 2022-01-19 08:32 UTC (permalink / raw)
To: Paul A Jungwirth <[email protected]>; +Cc: Corey Huinker <[email protected]>; PostgreSQL Hackers <[email protected]>; Jaime Casanova <[email protected]>
On 10.01.22 09:53, Peter Eisentraut wrote:
>>> Of course, the main problem in this patch is that for most uses it
>>> requires btree_gist. I think we should consider moving that into
>>> core, or at least the support for types that are most relevant to this
>>> functionality, specifically the date/time types. Aside from user
>>> convenience, this would also allow writing more realistic test cases.
>>
>> I think this would be great too. How realistic do you think it is? I
>> figured since exclusion constraints are also pretty useless without
>> btree_gist, it wasn't asking too much to have people install the
>> extension, but still it'd be better if it were all built in.
>
> IMO, if this temporal feature is to happen, btree_gist needs to be moved
> into core first. Having to install an extension in order to use an
> in-core feature like this isn't going to be an acceptable experience.
I have started a separate thread about this question.
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2022-01-19 08:32 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-09-02 21:21 [PATCH v4 1/4] Implement FIRST_IS_ABORTED_CONTRECORD Alvaro Herrera <[email protected]>
2022-01-06 05:44 Re: SQL:2011 application time Paul A Jungwirth <[email protected]>
2022-01-10 08:53 ` Re: SQL:2011 application time Peter Eisentraut <[email protected]>
2022-01-19 08:32 ` Re: SQL:2011 application time Peter Eisentraut <[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