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]>
2023-09-23 18:00 ` Re: Should rolpassword be toastable? Alexander Lakhin <[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 14:39 Re: Should rolpassword be toastable? Tom Lane <[email protected]>
@ 2023-09-23 18:00 ` Alexander Lakhin <[email protected]>
2024-09-19 03:00 ` Re: Should rolpassword be toastable? Alexander Lakhin <[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?
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 ` Alexander Lakhin <[email protected]>
2024-09-19 14:22 ` Re: Should rolpassword be toastable? Nathan Bossart <[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?
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 ` Nathan Bossart <[email protected]>
2024-09-19 14:31 ` Re: Should rolpassword be toastable? Tom Lane <[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?
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 ` Tom Lane <[email protected]>
2024-09-19 17:44 ` Re: Should rolpassword be toastable? 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?
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 ` Nathan Bossart <[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