public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 03/10] Make sure published XIDs are persistent 3+ 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; 3+ 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] 3+ messages in thread
* Re: pgbench with partitioned tables @ 2025-02-03 12:23 Sergey Tatarintsev <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Sergey Tatarintsev @ 2025-02-03 12:23 UTC (permalink / raw) To: Álvaro Herrera <[email protected]>; +Cc: Melanie Plageman <[email protected]>; [email protected] <[email protected]> 03.02.2025 14:57, Álvaro Herrera пишет: > On 2025-Feb-03, Sergey Tatarintsev wrote: > >> Thanks for the note. I changed the query in the patch (v2 patch attached) >> >> Btw, an additional benefit from the patch is that we can use foreign tables >> (for example, to test postgres_fdw optimizations) > Good thought, and maybe it would be better if the new function was > "get_table_relkind" which just returns the char, and this check > >> + /* Use COPY with FREEZE on v14 and later for all regular tables */ >> + if ((PQserverVersion(con) >= 140000) && is_regular_table(con, table)) >> copy_statement_fmt = "copy %s from stdin with (freeze on)"; > can be "&& get_table_relkind(con, table) == RELKIND_RELATION" > > which I think is more natural. > sounds reasonable. patch v3 attached -- With best regards, Sergey Tatarintsev, PostgresPro Attachments: [text/x-patch] v3-0001-Fix-pgbench-client-side-data-generation-for.patch (2.3K, ../../[email protected]/2-v3-0001-Fix-pgbench-client-side-data-generation-for.patch) download | inline diff: From cdda0dcb2c0fd8d14cddd185020fd0b1ecea59be Mon Sep 17 00:00:00 2001 From: Sergey Tatarintsev <[email protected]> Date: Mon, 3 Feb 2025 19:18:06 +0700 Subject: [PATCH] [PATCH-v3] Fix pgbench client-side data generation for partitioned tables Client-side data generation cannot perform COPY WITH (FREEZE = ON) on partitioned tables except pgbench_accounts. Patch disables FREEZE for any non-regular tables --- src/bin/pgbench/pgbench.c | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index 40592e6260..d003b407bf 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -53,6 +53,7 @@ #include <sys/select.h> #endif +#include "catalog/pg_class.h" #include "common/int.h" #include "common/logging.h" #include "common/pg_prng.h" @@ -848,6 +849,29 @@ static const PsqlScanCallbacks pgbench_callbacks = { NULL, /* don't need get_variable functionality */ }; +static char +get_table_relkind(PGconn *con, const char *table) +{ + PGresult *res; + char *sql = psprintf("SELECT relkind FROM pg_catalog.pg_class WHERE oid='%s'::pg_catalog.regclass", table); + char *val; + char relkind = RELKIND_RELATION; + + res = PQexec(con, sql); + if (PQresultStatus(res) != PGRES_TUPLES_OK) + { + pg_log_error("query failed: %s", PQerrorMessage(con)); + pg_log_error_detail("Query was: %s", sql); + exit(1); + } + val = PQgetvalue(res, 0, 0); + Assert(strlen(val) == 1); + relkind = val[0]; + PQclear(res); + pfree(sql); + return relkind; +} + static inline pg_time_usec_t pg_time_now(void) { @@ -4962,16 +4986,10 @@ initPopulateTable(PGconn *con, const char *table, int64 base, initPQExpBuffer(&sql); - /* - * Use COPY with FREEZE on v14 and later for all the tables except - * pgbench_accounts when it is partitioned. - */ - if (PQserverVersion(con) >= 140000) - { - if (strcmp(table, "pgbench_accounts") != 0 || - partitions == 0) + /* Use COPY with FREEZE on v14 and later for all regular tables */ + if ((PQserverVersion(con) >= 140000) && get_table_relkind(con, table) == RELKIND_RELATION) copy_statement_fmt = "copy %s from stdin with (freeze on)"; - } + n = pg_snprintf(copy_statement, sizeof(copy_statement), copy_statement_fmt, table); if (n >= sizeof(copy_statement)) -- 2.43.0 ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: pgbench with partitioned tables @ 2025-02-07 23:15 Melanie Plageman <[email protected]> parent: Sergey Tatarintsev <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Melanie Plageman @ 2025-02-07 23:15 UTC (permalink / raw) To: Sergey Tatarintsev <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; [email protected] <[email protected]> On Mon, Feb 3, 2025 at 7:23 AM Sergey Tatarintsev <[email protected]> wrote: > > 03.02.2025 14:57, Álvaro Herrera пишет: > > On 2025-Feb-03, Sergey Tatarintsev wrote: > > > >> Thanks for the note. I changed the query in the patch (v2 patch attached) > >> > >> Btw, an additional benefit from the patch is that we can use foreign tables > >> (for example, to test postgres_fdw optimizations) > > Good thought, and maybe it would be better if the new function was > > "get_table_relkind" which just returns the char, and this check > > > >> + /* Use COPY with FREEZE on v14 and later for all regular tables */ > >> + if ((PQserverVersion(con) >= 140000) && is_regular_table(con, table)) > >> copy_statement_fmt = "copy %s from stdin with (freeze on)"; > > can be "&& get_table_relkind(con, table) == RELKIND_RELATION" > > > > which I think is more natural. > > > sounds reasonable. > > patch v3 attached Okay, I've stared at this a bit, and it seems basically fine the way it is (I might add a bit more whitespace, clean up the commit message, etc). So I'm interested in committing it. I will admit that having never committed anything to pgbench, I'm a bit nervous. So, give me a couple days to work up the nerve. - Melanie ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2025-02-07 23:15 UTC | newest] Thread overview: 3+ 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]> 2025-02-03 12:23 Re: pgbench with partitioned tables Sergey Tatarintsev <[email protected]> 2025-02-07 23:15 ` Re: pgbench with partitioned tables Melanie Plageman <[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