public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 03/10] Make sure published XIDs are persistent 7+ 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; 7+ 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] 7+ messages in thread
* Re: Should rolpassword be toastable? @ 2023-09-23 14:39 Tom Lane <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Tom Lane @ 2023-09-23 14:39 UTC (permalink / raw) To: Alexander Lakhin <[email protected]>; +Cc: pgsql-hackers Alexander Lakhin <[email protected]> writes: > When playing with oversized tuples, I've found that it's possible to set > such oversized password for a user, that could not be validated. > For example: > ... > psql -U "test_user" -c "SELECT 1" > psql: error: connection to server on socket "/tmp/.s.PGSQL.5432" failed: FATAL: cannot read pg_class without having > selected a database My inclination is to fix this by removing pg_authid's toast table. I was not in favor of "let's attach a toast table to every catalog" to begin with, and I think this failure graphically illustrates why that was not as great an idea as some people thought. I also don't think it's worth trying to make it really work. I'm also now more than just slightly skeptical about whether pg_database should have a toast table. Has anybody tried, say, storing a daticurules field wide enough to end up out-of-line? regards, tom lane ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Should rolpassword be toastable? @ 2023-09-23 18:00 Alexander Lakhin <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Alexander Lakhin @ 2023-09-23 18:00 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: pgsql-hackers 23.09.2023 17:39, Tom Lane wrote: > I'm also now more than just slightly skeptical about whether > pg_database should have a toast table. Has anybody tried, > say, storing a daticurules field wide enough to end up > out-of-line? I tried, but failed, because pg_database accessed in InitPostgres() before assigning MyDatabaseId only via the function GetDatabaseTupleByOid(), which doesn't unpack the database tuple. Another access to a system catalog with unassigned MyDatabaseId might occur in the has_privs_of_role() call, but pg_auth_members contains no toastable attributes. So for now only pg_authid is worthy of condemnation, AFAICS. Best regards, Alexander ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Should rolpassword be toastable? @ 2024-09-19 03:00 Alexander Lakhin <[email protected]> parent: Alexander Lakhin <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Alexander Lakhin @ 2024-09-19 03:00 UTC (permalink / raw) To: Tom Lane <[email protected]>; Nathan Bossart <[email protected]>; +Cc: pgsql-hackers Hello Tom and Nathan, 23.09.2023 21:00, Alexander Lakhin wrote: > 23.09.2023 17:39, Tom Lane wrote: >> I'm also now more than just slightly skeptical about whether >> pg_database should have a toast table. Has anybody tried, >> say, storing a daticurules field wide enough to end up >> out-of-line? > > So for now only pg_authid is worthy of condemnation, AFAICS. > Let me remind you of this issue in light of b52c4fc3c. Yes, it's opposite, but maybe it makes sense to fix it now in the hope that ~1 year of testing will bring something helpful for both changes. Best regards, Alexander ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Should rolpassword be toastable? @ 2024-09-19 14:22 Nathan Bossart <[email protected]> parent: Alexander Lakhin <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Nathan Bossart @ 2024-09-19 14:22 UTC (permalink / raw) To: Alexander Lakhin <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers On Thu, Sep 19, 2024 at 06:00:00AM +0300, Alexander Lakhin wrote: > 23.09.2023 21:00, Alexander Lakhin wrote: >> So for now only pg_authid is worthy of condemnation, AFAICS. > > Let me remind you of this issue in light of b52c4fc3c. > Yes, it's opposite, but maybe it makes sense to fix it now in the hope that > ~1 year of testing will bring something helpful for both changes. Hm. It does seem like there's little point in giving pg_authid a TOAST table, as rolpassword is the only varlena column, and it obviously has problems. But wouldn't removing it just trade one unhelpful internal error when trying to log in for another when trying to add a really long password hash (which hopefully nobody is really trying to do in practice)? I wonder if we could make this a little more user-friendly. -- nathan ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Should rolpassword be toastable? @ 2024-09-19 14:31 Tom Lane <[email protected]> parent: Nathan Bossart <[email protected]> 0 siblings, 1 reply; 7+ messages in thread From: Tom Lane @ 2024-09-19 14:31 UTC (permalink / raw) To: Nathan Bossart <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; pgsql-hackers Nathan Bossart <[email protected]> writes: > Hm. It does seem like there's little point in giving pg_authid a TOAST > table, as rolpassword is the only varlena column, and it obviously has > problems. But wouldn't removing it just trade one unhelpful internal error > when trying to log in for another when trying to add a really long password > hash (which hopefully nobody is really trying to do in practice)? I wonder > if we could make this a little more user-friendly. We could put an arbitrary limit (say, half of BLCKSZ) on the length of passwords. regards, tom lane ^ permalink raw reply [nested|flat] 7+ messages in thread
* Re: Should rolpassword be toastable? @ 2024-09-19 17:44 Nathan Bossart <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 7+ messages in thread From: Nathan Bossart @ 2024-09-19 17:44 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Alexander Lakhin <[email protected]>; pgsql-hackers On Thu, Sep 19, 2024 at 10:31:15AM -0400, Tom Lane wrote: > Nathan Bossart <[email protected]> writes: >> Hm. It does seem like there's little point in giving pg_authid a TOAST >> table, as rolpassword is the only varlena column, and it obviously has >> problems. But wouldn't removing it just trade one unhelpful internal error >> when trying to log in for another when trying to add a really long password >> hash (which hopefully nobody is really trying to do in practice)? I wonder >> if we could make this a little more user-friendly. > > We could put an arbitrary limit (say, half of BLCKSZ) on the length of > passwords. Something like that could be good enough. I was thinking about actually validating that the hash had the correct form, but that might be a little more complex than is warranted here. -- nathan ^ permalink raw reply [nested|flat] 7+ messages in thread
end of thread, other threads:[~2024-09-19 17:44 UTC | newest] Thread overview: 7+ 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-09-23 14:39 Re: Should rolpassword be toastable? Tom Lane <[email protected]> 2023-09-23 18:00 ` Re: Should rolpassword be toastable? Alexander Lakhin <[email protected]> 2024-09-19 03:00 ` Re: Should rolpassword be toastable? Alexander Lakhin <[email protected]> 2024-09-19 14:22 ` Re: Should rolpassword be toastable? Nathan Bossart <[email protected]> 2024-09-19 14:31 ` Re: Should rolpassword be toastable? Tom Lane <[email protected]> 2024-09-19 17:44 ` Re: Should rolpassword be toastable? Nathan Bossart <[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