public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 03/10] Make sure published XIDs are persistent
4+ messages / 4 participants
[nested] [flat]
* [PATCH 03/10] Make sure published XIDs are persistent
@ 2021-03-08 06:43 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Kyotaro Horiguchi @ 2021-03-08 06:43 UTC (permalink / raw)
pg_xact_status() premises that XIDs obtained by
pg_current_xact_id(_if_assigned)() are persistent beyond a crash. But
XIDs are not guaranteed to go beyond WAL buffers before commit and
thus XIDs may vanish if server crashes before commit. This patch
guarantees the XID shown by the functions to be flushed out to disk.
Copied from: https://www.postgresql.org/message-id/20210308.173242.463790587797836129.horikyota.ntt%40gmail.com
---
src/backend/access/transam/xact.c | 55 +++++++++++++++++++++++++------
src/backend/access/transam/xlog.c | 2 +-
src/backend/utils/adt/xid8funcs.c | 12 ++++++-
src/include/access/xact.h | 3 +-
4 files changed, 59 insertions(+), 13 deletions(-)
diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 6395a9b240..38e978d238 100644
--- a/src/backend/access/transam/xact.c
+++ b/src/backend/access/transam/xact.c
@@ -201,7 +201,7 @@ typedef struct TransactionStateData
int prevSecContext; /* previous SecurityRestrictionContext */
bool prevXactReadOnly; /* entry-time xact r/o state */
bool startedInRecovery; /* did we start in recovery? */
- bool didLogXid; /* has xid been included in WAL record? */
+ XLogRecPtr minLSN; /* LSN needed to reach to record the xid */
int parallelModeLevel; /* Enter/ExitParallelMode counter */
bool chain; /* start a new block after this one */
bool assigned; /* assigned to top-level XID */
@@ -520,14 +520,46 @@ GetCurrentFullTransactionIdIfAny(void)
* MarkCurrentTransactionIdLoggedIfAny
*
* Remember that the current xid - if it is assigned - now has been wal logged.
+ *
+ * upto is the LSN up to which we need to flush WAL to ensure the current xid
+ * to be persistent. See EnsureCurrentTransactionIdLogged().
*/
void
-MarkCurrentTransactionIdLoggedIfAny(void)
+MarkCurrentTransactionIdLoggedIfAny(XLogRecPtr upto)
{
- if (FullTransactionIdIsValid(CurrentTransactionState->fullTransactionId))
- CurrentTransactionState->didLogXid = true;
+ if (FullTransactionIdIsValid(CurrentTransactionState->fullTransactionId) &&
+ XLogRecPtrIsInvalid(CurrentTransactionState->minLSN))
+ CurrentTransactionState->minLSN = upto;
}
+/*
+ * EnsureCurrentTransactionIdLogged
+ *
+ * Make sure that the current top XID is WAL-logged.
+ */
+void
+EnsureTopTransactionIdLogged(void)
+{
+ /*
+ * We need at least one WAL record for the current top transaction to be
+ * flushed out. Write one if we don't have one yet.
+ */
+ if (XLogRecPtrIsInvalid(TopTransactionStateData.minLSN))
+ {
+ xl_xact_assignment xlrec;
+
+ xlrec.xtop = XidFromFullTransactionId(XactTopFullTransactionId);
+ Assert(TransactionIdIsValid(xlrec.xtop));
+ xlrec.nsubxacts = 0;
+
+ XLogBeginInsert();
+ XLogRegisterData((char *) &xlrec, MinSizeOfXactAssignment);
+ TopTransactionStateData.minLSN =
+ XLogInsert(RM_XACT_ID, XLOG_XACT_ASSIGNMENT);
+ }
+
+ XLogFlush(TopTransactionStateData.minLSN);
+}
/*
* GetStableLatestTransactionId
@@ -616,14 +648,14 @@ AssignTransactionId(TransactionState s)
* When wal_level=logical, guarantee that a subtransaction's xid can only
* be seen in the WAL stream if its toplevel xid has been logged before.
* If necessary we log an xact_assignment record with fewer than
- * PGPROC_MAX_CACHED_SUBXIDS. Note that it is fine if didLogXid isn't set
+ * PGPROC_MAX_CACHED_SUBXIDS. Note that it is fine if minLSN isn't set
* for a transaction even though it appears in a WAL record, we just might
* superfluously log something. That can happen when an xid is included
* somewhere inside a wal record, but not in XLogRecord->xl_xid, like in
* xl_standby_locks.
*/
if (isSubXact && XLogLogicalInfoActive() &&
- !TopTransactionStateData.didLogXid)
+ XLogRecPtrIsInvalid(TopTransactionStateData.minLSN))
log_unknown_top = true;
/*
@@ -693,6 +725,7 @@ AssignTransactionId(TransactionState s)
log_unknown_top)
{
xl_xact_assignment xlrec;
+ XLogRecPtr endptr;
/*
* xtop is always set by now because we recurse up transaction
@@ -707,11 +740,13 @@ AssignTransactionId(TransactionState s)
XLogRegisterData((char *) unreportedXids,
nUnreportedXids * sizeof(TransactionId));
- (void) XLogInsert(RM_XACT_ID, XLOG_XACT_ASSIGNMENT);
+ endptr = XLogInsert(RM_XACT_ID, XLOG_XACT_ASSIGNMENT);
nUnreportedXids = 0;
- /* mark top, not current xact as having been logged */
- TopTransactionStateData.didLogXid = true;
+
+ /* set minLSN of top, not of current xact if not yet */
+ if (XLogRecPtrIsInvalid(TopTransactionStateData.minLSN))
+ TopTransactionStateData.minLSN = endptr;
}
}
}
@@ -2022,7 +2057,7 @@ StartTransaction(void)
* initialize reported xid accounting
*/
nUnreportedXids = 0;
- s->didLogXid = false;
+ s->minLSN = InvalidXLogRecPtr;
/*
* must initialize resource-management stuff first
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 04192b7add..0183589b4d 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1171,7 +1171,7 @@ XLogInsertRecord(XLogRecData *rdata,
*/
WALInsertLockRelease();
- MarkCurrentTransactionIdLoggedIfAny();
+ MarkCurrentTransactionIdLoggedIfAny(EndPos);
END_CRIT_SECTION();
diff --git a/src/backend/utils/adt/xid8funcs.c b/src/backend/utils/adt/xid8funcs.c
index cc2b4ac797..992482f8c8 100644
--- a/src/backend/utils/adt/xid8funcs.c
+++ b/src/backend/utils/adt/xid8funcs.c
@@ -357,6 +357,8 @@ bad_format:
Datum
pg_current_xact_id(PG_FUNCTION_ARGS)
{
+ FullTransactionId xid;
+
/*
* Must prevent during recovery because if an xid is not assigned we try
* to assign one, which would fail. Programs already rely on this function
@@ -365,7 +367,12 @@ pg_current_xact_id(PG_FUNCTION_ARGS)
*/
PreventCommandDuringRecovery("pg_current_xact_id()");
- PG_RETURN_FULLTRANSACTIONID(GetTopFullTransactionId());
+ xid = GetTopFullTransactionId();
+
+ /* the XID is going to be published, make sure it is psersistent */
+ EnsureTopTransactionIdLogged();
+
+ PG_RETURN_FULLTRANSACTIONID(xid);
}
/*
@@ -380,6 +387,9 @@ pg_current_xact_id_if_assigned(PG_FUNCTION_ARGS)
if (!FullTransactionIdIsValid(topfxid))
PG_RETURN_NULL();
+ /* the XID is going to be published, make sure it is psersistent */
+ EnsureTopTransactionIdLogged();
+
PG_RETURN_FULLTRANSACTIONID(topfxid);
}
diff --git a/src/include/access/xact.h b/src/include/access/xact.h
index 34cfaf542c..a61e4d6da5 100644
--- a/src/include/access/xact.h
+++ b/src/include/access/xact.h
@@ -386,7 +386,8 @@ extern FullTransactionId GetTopFullTransactionId(void);
extern FullTransactionId GetTopFullTransactionIdIfAny(void);
extern FullTransactionId GetCurrentFullTransactionId(void);
extern FullTransactionId GetCurrentFullTransactionIdIfAny(void);
-extern void MarkCurrentTransactionIdLoggedIfAny(void);
+extern void MarkCurrentTransactionIdLoggedIfAny(XLogRecPtr upto);
+extern void EnsureTopTransactionIdLogged(void);
extern bool SubTransactionIsActive(SubTransactionId subxid);
extern CommandId GetCurrentCommandId(bool used);
extern void SetParallelStartTimestamps(TimestampTz xact_ts, TimestampTz stmt_ts);
--
2.17.0
--0qVF/w3MHQqLSynd
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0004-wal_compression_method-default-to-zlib.patch"
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);
@ 2024-02-16 13:02 David Rowley <[email protected]>
0 siblings, 2 replies; 4+ messages in thread
From: David Rowley @ 2024-02-16 13:02 UTC (permalink / raw)
To: [email protected]; [email protected]
On Sat, 17 Feb 2024 at 01:27, PG Bug reporting form
<[email protected]> wrote:
> tpch=# select extract(week from interval '3 weeks');
> ERROR: interval units "week" not supported
>
> In the documentation it's mentioned that 'week' is an ISO 8601 week, so it
> makes sense why it's not applicable to INTERVAL, which is the same for
> isoyear. However, the field is named week and not isoweek, so I expect it to
> work like the `select extract(year from interval '3 years');` does.
> Moreover, the documentation does not mention that the field cannot be
> extracted from INTERVAL, like it does for isoyear:
> https://www.postgresql.org/docs/current/functions-datetime.html#FUNCTIONS-DATETIME-EXTRACT
Maybe that table should specify which type(s) each of the items listed
is applicable to. Seems better than mentioning which types they're not
applicable to.
David
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);
@ 2024-02-17 01:47 jian he <[email protected]>
parent: David Rowley <[email protected]>
1 sibling, 0 replies; 4+ messages in thread
From: jian he @ 2024-02-17 01:47 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: [email protected]; [email protected]
in `9.9.1. EXTRACT, date_part`
EXTRACT(field FROM source)
I saw more inconsistencies with the doc when `source` is an interval.
the `minute` field
select extract(minute from interval '2011 year 16 month 35 day 48 hour
1005 min 71 sec 11 ms');
select extract(minute from interval '2011 year 16 month 35 day 48 hour
1005 min 2 sec 11 ms');
select extract(minute from interval '2011 year 16 month 35 day 48 hour
1005 min 2 sec 11 ms');
the `hour` field:
select extract(hour from interval '2011 year 16 month 35 day 48 hour
1005 min 71 sec 11 ms');
select extract(hour from interval '2011 year 16 month 35 day 48 hour
1005 min 2 sec 11 ms');
select extract(hour from interval '2011 year 16 month 35 day 48 hour
1005 min 71 sec 11111111111 ms');
the `quarter` field:
select extract(quarter from interval '2011 year 12 month 48 hour 1005
min 2 sec 11 ms');
SELECT EXTRACT(QUARTER FROM TIMESTAMP '2001-12-16 20:38:40');
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL);
@ 2024-02-17 18:14 Tom Lane <[email protected]>
parent: David Rowley <[email protected]>
1 sibling, 0 replies; 4+ messages in thread
From: Tom Lane @ 2024-02-17 18:14 UTC (permalink / raw)
To: David Rowley <[email protected]>; +Cc: [email protected]; [email protected]; Peter Eisentraut <[email protected]>
David Rowley <[email protected]> writes:
> On Sat, 17 Feb 2024 at 01:27, PG Bug reporting form
> <[email protected]> wrote:
>> Moreover, the documentation does not mention that the field cannot be
>> extracted from INTERVAL, like it does for isoyear:
>> https://www.postgresql.org/docs/current/functions-datetime.html#FUNCTIONS-DATETIME-EXTRACT
> Maybe that table should specify which type(s) each of the items listed
> is applicable to. Seems better than mentioning which types they're not
> applicable to.
The thing's not laid out as a table though, and converting it seems
like more trouble than this is worth. The rejected cases hardly seem
surprising. I propose just mentioning that not all fields apply for
all data types, as in 0001 attached.
(Parenthetically, one case that perhaps is surprising is
ERROR: unit "week" not supported for type interval
Why not just return the day field divided by 7?)
Unrelated but adjacent, the discussion of the century field seems
more than a bit flippant when I read it now. In other places we
are typically content to use examples to make similar points.
I propose doing so here too, as in 0002 attached.
Lastly, the entire page is quite schizophrenic about whether to leave
a blank line between adjacent examples. I could go either way on
whether to have that whitespace or not, but I do think it would be
better to make it uniform. Any votes on what to do there?
regards, tom lane
Attachments:
[text/x-diff] 0001-update-extract-description.patch (2.1K, ../../[email protected]/2-0001-update-extract-description.patch)
download | inline diff:
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index cf3de80394..fc8017f2f3 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -10040,13 +10040,19 @@ EXTRACT(<replaceable>field</replaceable> FROM <replaceable>source</replaceable>)
The <function>extract</function> function retrieves subfields
such as year or hour from date/time values.
<replaceable>source</replaceable> must be a value expression of
- type <type>timestamp</type>, <type>time</type>, or <type>interval</type>.
- (Expressions of type <type>date</type> are
- cast to <type>timestamp</type> and can therefore be used as
- well.) <replaceable>field</replaceable> is an identifier or
+ type <type>timestamp</type>, <type>date</type>, <type>time</type>,
+ or <type>interval</type>. (Timestamps and times can be with or
+ without time zone.)
+ <replaceable>field</replaceable> is an identifier or
string that selects what field to extract from the source value.
+ Not all fields are valid for every input data type; for example, fields
+ smaller than a day cannot be extracted from a <type>date</type>, while
+ fields of a day or more cannot be extracted from a <type>time</type>.
The <function>extract</function> function returns values of type
<type>numeric</type>.
+ </para>
+
+ <para>
The following are valid field names:
<!-- alphabetical -->
@@ -10228,7 +10228,7 @@ SELECT EXTRACT(ISODOW FROM TIMESTAMP '2001-02-18 20:38:40');
<listitem>
<para>
The <acronym>ISO</acronym> 8601 week-numbering year that the date
- falls in (not applicable to intervals)
+ falls in
</para>
<screen>
@@ -10256,7 +10256,7 @@ SELECT EXTRACT(ISOYEAR FROM DATE '2006-01-02');
<listitem>
<para>
The <firstterm>Julian Date</firstterm> corresponding to the
- date or timestamp (not applicable to intervals). Timestamps
+ date or timestamp. Timestamps
that are not local midnight result in a fractional value. See
<xref linkend="datetime-julian-dates"/> for more information.
</para>
[text/x-diff] 0002-condense-century-discussion.patch (1.2K, ../../[email protected]/3-0002-condense-century-discussion.patch)
download | inline diff:
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index cf3de80394..5d215d218c 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -10063,17 +10069,11 @@ SELECT EXTRACT(CENTURY FROM TIMESTAMP '2000-12-16 12:21:13');
<lineannotation>Result: </lineannotation><computeroutput>20</computeroutput>
SELECT EXTRACT(CENTURY FROM TIMESTAMP '2001-02-16 20:38:40');
<lineannotation>Result: </lineannotation><computeroutput>21</computeroutput>
+SELECT EXTRACT(CENTURY FROM DATE '0001-01-01 AD');
+<lineannotation>Result: </lineannotation><computeroutput>1</computeroutput>
+SELECT EXTRACT(CENTURY FROM DATE '0001-12-31 BC');
+<lineannotation>Result: </lineannotation><computeroutput>-1</computeroutput>
</screen>
-
- <para>
- The first century starts at 0001-01-01 00:00:00 AD, although
- they did not know it at the time. This definition applies to all
- Gregorian calendar countries. There is no century number 0,
- you go from -1 century to 1 century.
-
- If you disagree with this, please write your complaint to:
- Pope, Cathedral Saint-Peter of Roma, Vatican.
- </para>
</listitem>
</varlistentry>
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-02-17 18:14 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-03-08 06:43 [PATCH 03/10] Make sure published XIDs are persistent Kyotaro Horiguchi <[email protected]>
2024-02-16 13:02 Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL); David Rowley <[email protected]>
2024-02-17 01:47 ` Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL); jian he <[email protected]>
2024-02-17 18:14 ` Re: BUG #18348: Inconsistency with EXTRACT([field] from INTERVAL); Tom Lane <[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