public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 03/10] Make sure published XIDs are persistent
6+ messages / 3 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; 6+ 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 15da91a8dd..6eb46ea8a7 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1172,7 +1172,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
--jozmn01XJZjDjM3N
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] 6+ messages in thread
* Re: libpq: PQgetCopyData() and allocation overhead
@ 2023-02-10 10:32 Jeroen Vermeulen <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Jeroen Vermeulen @ 2023-02-10 10:32 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: pgsql-hackers
Here's the patch (as a PR just to make it easy to read):
https://github.com/jtv/postgres/pull/1
I don't have an easily readable benchmark yet, since I've been timing the
potential impact on libpqxx. But can do that next.
Jeroen
On Fri, Feb 10, 2023, 11:26 Bharath Rupireddy <
[email protected]> wrote:
> On Fri, Feb 10, 2023 at 3:43 PM Jeroen Vermeulen <[email protected]> wrote:
> >
> > Would there be interest in a variant of PQgetCopyData() that re-uses the
> same buffer for a new row, rather than allocating a new buffer on each
> iteration?
> >
> > I tried it on a toy benchmark, and it reduced client-side CPU time by
> about 12%. (Less of a difference in wall-clock time of course; the client
> was only using the CPU for a bit over half the time.)
>
> Interesting. It might improve logical replication performance too as
> it uses COPY protocol.
>
> Do you mind sharing a patch, test case that you used and steps to
> verify the benefit?
>
> --
> Bharath Rupireddy
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com
>
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: libpq: PQgetCopyData() and allocation overhead
@ 2023-02-10 12:19 Jeroen Vermeulen <[email protected]>
parent: Jeroen Vermeulen <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Jeroen Vermeulen @ 2023-02-10 12:19 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: pgsql-hackers
OK, I've updated the PR with a benchmark (in the main directory).
On this benchmark I'm seeing about a 24% reduction in "user" CPU time, and
a 8% reduction in "system" CPU time. (Almost no reduction in wall-clock
time.)
Jeroen
On Fri, 10 Feb 2023 at 11:32, Jeroen Vermeulen <[email protected]> wrote:
> Here's the patch (as a PR just to make it easy to read):
> https://github.com/jtv/postgres/pull/1
>
> I don't have an easily readable benchmark yet, since I've been timing the
> potential impact on libpqxx. But can do that next.
>
>
> Jeroen
>
> On Fri, Feb 10, 2023, 11:26 Bharath Rupireddy <
> [email protected]> wrote:
>
>> On Fri, Feb 10, 2023 at 3:43 PM Jeroen Vermeulen <[email protected]>
>> wrote:
>> >
>> > Would there be interest in a variant of PQgetCopyData() that re-uses
>> the same buffer for a new row, rather than allocating a new buffer on each
>> iteration?
>> >
>> > I tried it on a toy benchmark, and it reduced client-side CPU time by
>> about 12%. (Less of a difference in wall-clock time of course; the client
>> was only using the CPU for a bit over half the time.)
>>
>> Interesting. It might improve logical replication performance too as
>> it uses COPY protocol.
>>
>> Do you mind sharing a patch, test case that you used and steps to
>> verify the benefit?
>>
>> --
>> Bharath Rupireddy
>> PostgreSQL Contributors Team
>> RDS Open Source Databases
>> Amazon Web Services: https://aws.amazon.com
>>
>
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: libpq: PQgetCopyData() and allocation overhead
@ 2023-02-27 13:48 Bharath Rupireddy <[email protected]>
parent: Jeroen Vermeulen <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Bharath Rupireddy @ 2023-02-27 13:48 UTC (permalink / raw)
To: Jeroen Vermeulen <[email protected]>; +Cc: pgsql-hackers
On Fri, Feb 10, 2023 at 5:49 PM Jeroen Vermeulen <[email protected]> wrote:
>
> OK, I've updated the PR with a benchmark (in the main directory).
>
> On this benchmark I'm seeing about a 24% reduction in "user" CPU time, and a 8% reduction in "system" CPU time. (Almost no reduction in wall-clock time.)
I can help run some logical replication performance benchmarks
tomorrow. Would you mind cleaning the PR and providing the changes
(there are multiple commits in the PR) as a single patch here for the
sake of ease of review and test?
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: libpq: PQgetCopyData() and allocation overhead
@ 2023-02-27 16:08 Jeroen Vermeulen <[email protected]>
parent: Bharath Rupireddy <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Jeroen Vermeulen @ 2023-02-27 16:08 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: pgsql-hackers
Done. Thanks for looking!
Jelte Fennema pointed out that I should probably be using PQExpBuffer for
this. I'll look into that later; this is a proof of concept, not a
production-ready API proposal.
Jeroen
On Mon, 27 Feb 2023 at 14:48, Bharath Rupireddy <
[email protected]> wrote:
> On Fri, Feb 10, 2023 at 5:49 PM Jeroen Vermeulen <[email protected]> wrote:
> >
> > OK, I've updated the PR with a benchmark (in the main directory).
> >
> > On this benchmark I'm seeing about a 24% reduction in "user" CPU time,
> and a 8% reduction in "system" CPU time. (Almost no reduction in
> wall-clock time.)
>
> I can help run some logical replication performance benchmarks
> tomorrow. Would you mind cleaning the PR and providing the changes
> (there are multiple commits in the PR) as a single patch here for the
> sake of ease of review and test?
>
> --
> Bharath Rupireddy
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com
>
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: libpq: PQgetCopyData() and allocation overhead
@ 2023-03-01 14:23 Jeroen Vermeulen <[email protected]>
parent: Jeroen Vermeulen <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Jeroen Vermeulen @ 2023-03-01 14:23 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; +Cc: pgsql-hackers
Update: in the latest iteration, I have an alternative that reduces CPU
time by more than half, compared to PQgetCopyData().
And the code is simpler, too, both in the client and in libpq itself. The
one downside is that it breaks with libpq's existing API style.
PR for easy discussion: https://github.com/jtv/postgres/pull/1
Jeroen
On Mon, 27 Feb 2023 at 17:08, Jeroen Vermeulen <[email protected]> wrote:
> Done. Thanks for looking!
>
> Jelte Fennema pointed out that I should probably be using PQExpBuffer for
> this. I'll look into that later; this is a proof of concept, not a
> production-ready API proposal.
>
>
> Jeroen
>
> On Mon, 27 Feb 2023 at 14:48, Bharath Rupireddy <
> [email protected]> wrote:
>
>> On Fri, Feb 10, 2023 at 5:49 PM Jeroen Vermeulen <[email protected]>
>> wrote:
>> >
>> > OK, I've updated the PR with a benchmark (in the main directory).
>> >
>> > On this benchmark I'm seeing about a 24% reduction in "user" CPU time,
>> and a 8% reduction in "system" CPU time. (Almost no reduction in
>> wall-clock time.)
>>
>> I can help run some logical replication performance benchmarks
>> tomorrow. Would you mind cleaning the PR and providing the changes
>> (there are multiple commits in the PR) as a single patch here for the
>> sake of ease of review and test?
>>
>> --
>> Bharath Rupireddy
>> PostgreSQL Contributors Team
>> RDS Open Source Databases
>> Amazon Web Services: https://aws.amazon.com
>>
>
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2023-03-01 14:23 UTC | newest]
Thread overview: 6+ 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]>
2023-02-10 10:32 Re: libpq: PQgetCopyData() and allocation overhead Jeroen Vermeulen <[email protected]>
2023-02-10 12:19 ` Re: libpq: PQgetCopyData() and allocation overhead Jeroen Vermeulen <[email protected]>
2023-02-27 13:48 ` Re: libpq: PQgetCopyData() and allocation overhead Bharath Rupireddy <[email protected]>
2023-02-27 16:08 ` Re: libpq: PQgetCopyData() and allocation overhead Jeroen Vermeulen <[email protected]>
2023-03-01 14:23 ` Re: libpq: PQgetCopyData() and allocation overhead Jeroen Vermeulen <[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