pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feedStartup process on a hot standby crashes with an error "invalid memory alloc request size 1073741824" while replaying "Standby/LOCK" records
11+ messages / 6 participants
[nested] [flat]
Commits from this thread:
2022-10-06 66c2922e76 Take care to de-duplicate entries in standby.c's table of locks.
* Startup process on a hot standby crashes with an error "invalid memory alloc request size 1073741824" while replaying "Standby/LOCK" records
@ 2022-09-05 10:19 Dmitriy Kuzmin <kuzmin.db4@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Dmitriy Kuzmin @ 2022-09-05 10:19 UTC (permalink / raw)
To: pgsql-bugs@lists.postgresql.org
Greetings!
One of our clients experienced a crash of startup process with an error
"invalid memory alloc request size 1073741824" on a hot standby, which
ended in replica reinit.
According to logs, startup process crashed while trying to replay
"Standby/LOCK" record with a huge list of locks(see attached
replicalog_tail.tgz):
FATAL: XX000: invalid memory alloc request size 1073741824
CONTEXT: WAL redo at 7/327F9248 for Standby/LOCK: xid 1638575 db 7550635
rel 8500880 xid 1638575 db 7550635 rel 10324499...
LOCATION: repalloc, mcxt.c:1075
BACKTRACE:
postgres: startup recovering
000000010000000700000033(repalloc+0x61) [0x8d7611]
postgres: startup recovering 000000010000000700000033() [0x691c29]
postgres: startup recovering 000000010000000700000033() [0x691c74]
postgres: startup recovering 000000010000000700000033(lappend+0x16)
[0x691e76]
postgres: startup recovering
000000010000000700000033(StandbyAcquireAccessExclusiveLock+0xdd) [0x7786bd]
postgres: startup recovering
000000010000000700000033(standby_redo+0x5d) [0x7789ed]
postgres: startup recovering
000000010000000700000033(StartupXLOG+0x1055) [0x51d7c5]
postgres: startup recovering
000000010000000700000033(StartupProcessMain+0xcd) [0x71d65d]
postgres: startup recovering
000000010000000700000033(AuxiliaryProcessMain+0x40c) [0x52c7cc]
postgres: startup recovering 000000010000000700000033() [0x71a62e]
postgres: startup recovering
000000010000000700000033(PostmasterMain+0xe74) [0x71cf74]
postgres: startup recovering 000000010000000700000033(main+0x70d)
[0x4891ad]
/lib64/libc.so.6(__libc_start_main+0xf5) [0x7f0be2db3555]
LOG: 00000: startup process (PID 2650) exited with exit code 1
Looks like startup process at some point hits the MaxAllocSize
limit(memutils.h), which forbids allocation of more than 1gb-1 bytes.
Judging by pg_waldump output, there was long running transaction on
primary, that sequentially locked and modified a lot of tables. Right
before the crash there was about 85k exclusively locked tables.
Trying to reproduce the issue, I found out that the problem is not so much
in the number of locks, but in the duration of the transaction. Replaying
"Standby/LOCK" records, the startup process eventually crashes with the
mentioned error if a long transaction holds a large number of locks long
enough.
I managed to reproduce the situation on 13.7, 14.4, 15beta3 and master
using the following steps:
1) get primary and replica with the following settings:
max_locks_per_transaction = '10000' and max_connections = '1000'
2) create 950k tables
3) lock them in AccessExclusive mode in transaction and leave it in "idle
in transaction state"
4) make some side activity with pgbench (pgbench -R 100 -P 5 -T 7200 -c 1)
In about 20-30 minutes startup process crashes with the same error.
As far as I understand, there is fixed amount of AccessExclusive locks in
this scenario. 950k exclusive locks acquired by "long running" transaction
and no additional exclusive locks held by pgbench. But startup consumes
more and more memory while replaying records, that contain exacly the same
list of locks. Could it be a memory leak? If not, is there any way to
improve this behavior?
If you're going to reproduce it, get primary and replica with enough RAM
and simultaneously run on primary:
$ *pgbench -i && pgbench -R 100 -P 5 -T 7200 -c 1* in one terminal
$ *psql -f 950k_locks.sql* in another terminal
and observe startup memory usage and replica's logs.
Best regards,
Dmitry Kuzmin
Attachments:
950k_locks.sql (application/sql, 706B) download
replicalog_tail.tgz (application/x-compressed-tar, 4.2M) download
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Startup process on a hot standby crashes with an error "invalid memory alloc request size 1073741824" while replaying "Standby/LOCK" records
@ 2022-09-05 12:13 David Rowley <dgrowleyml@gmail.com>
parent: Dmitriy Kuzmin <kuzmin.db4@gmail.com>
0 siblings, 2 replies; 11+ messages in thread
From: David Rowley @ 2022-09-05 12:13 UTC (permalink / raw)
To: Dmitriy Kuzmin <kuzmin.db4@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
On Mon, 5 Sept 2022 at 22:38, Dmitriy Kuzmin <kuzmin.db4@gmail.com> wrote:
> One of our clients experienced a crash of startup process with an error "invalid memory alloc request size 1073741824" on a hot standby, which ended in replica reinit.
>
> According to logs, startup process crashed while trying to replay "Standby/LOCK" record with a huge list of locks(see attached replicalog_tail.tgz):
>
> FATAL: XX000: invalid memory alloc request size 1073741824
> CONTEXT: WAL redo at 7/327F9248 for Standby/LOCK: xid 1638575 db 7550635 rel 8500880 xid 1638575 db 7550635 rel 10324499...
> LOCATION: repalloc, mcxt.c:1075
> BACKTRACE:
> postgres: startup recovering 000000010000000700000033(repalloc+0x61) [0x8d7611]
> postgres: startup recovering 000000010000000700000033() [0x691c29]
> postgres: startup recovering 000000010000000700000033() [0x691c74]
> postgres: startup recovering 000000010000000700000033(lappend+0x16) [0x691e76]
This must be the repalloc() in enlarge_list(). 1073741824 / 8 is
134,217,728 (2^27). That's quite a bit more than 1 lock per your 950k
tables.
I wonder why the RecoveryLockListsEntry.locks list is getting so long.
from the file you attached, I see:
$ cat replicalog_tail | grep -Eio "rel\s([0-9]+)" | wc -l
950000
So that confirms there were 950k relations in the xl_standby_locks.
The contents of that message seem to be produced by standby_desc().
That should be the same WAL record that's processed by standby_redo()
which adds the 950k locks to the RecoveryLockListsEntry.
I'm not seeing why 950k becomes 134m.
David
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Startup process on a hot standby crashes with an error "invalid memory alloc request size 1073741824" while replaying "Standby/LOCK" records
@ 2022-09-06 06:38 Dmitriy Kuzmin <kuzmin.db4@gmail.com>
parent: David Rowley <dgrowleyml@gmail.com>
1 sibling, 0 replies; 11+ messages in thread
From: Dmitriy Kuzmin @ 2022-09-06 06:38 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: pgsql-bugs@lists.postgresql.org
Thanks, David!
Let me know if there's any additional information i could provide.
Best regards,
Dmitry Kuzmin
пн, 5 сент. 2022 г. в 22:13, David Rowley <dgrowleyml@gmail.com>:
> On Mon, 5 Sept 2022 at 22:38, Dmitriy Kuzmin <kuzmin.db4@gmail.com> wrote:
> > One of our clients experienced a crash of startup process with an error
> "invalid memory alloc request size 1073741824" on a hot standby, which
> ended in replica reinit.
> >
> > According to logs, startup process crashed while trying to replay
> "Standby/LOCK" record with a huge list of locks(see attached
> replicalog_tail.tgz):
> >
> > FATAL: XX000: invalid memory alloc request size 1073741824
> > CONTEXT: WAL redo at 7/327F9248 for Standby/LOCK: xid 1638575 db
> 7550635 rel 8500880 xid 1638575 db 7550635 rel 10324499...
> > LOCATION: repalloc, mcxt.c:1075
> > BACKTRACE:
> > postgres: startup recovering
> 000000010000000700000033(repalloc+0x61) [0x8d7611]
> > postgres: startup recovering 000000010000000700000033()
> [0x691c29]
> > postgres: startup recovering 000000010000000700000033()
> [0x691c74]
> > postgres: startup recovering
> 000000010000000700000033(lappend+0x16) [0x691e76]
>
> This must be the repalloc() in enlarge_list(). 1073741824 / 8 is
> 134,217,728 (2^27). That's quite a bit more than 1 lock per your 950k
> tables.
>
> I wonder why the RecoveryLockListsEntry.locks list is getting so long.
>
> from the file you attached, I see:
> $ cat replicalog_tail | grep -Eio "rel\s([0-9]+)" | wc -l
> 950000
>
> So that confirms there were 950k relations in the xl_standby_locks.
> The contents of that message seem to be produced by standby_desc().
> That should be the same WAL record that's processed by standby_redo()
> which adds the 950k locks to the RecoveryLockListsEntry.
>
> I'm not seeing why 950k becomes 134m.
>
> David
>
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Startup process on a hot standby crashes with an error "invalid memory alloc request size 1073741824" while replaying "Standby/LOCK" records
@ 2022-10-04 22:54 Tom Lane <tgl@sss.pgh.pa.us>
parent: David Rowley <dgrowleyml@gmail.com>
1 sibling, 1 reply; 11+ messages in thread
From: Tom Lane @ 2022-10-04 22:54 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Dmitriy Kuzmin <kuzmin.db4@gmail.com>; pgsql-hackers@lists.postgresql.org
[ redirecting to -hackers because patch attached ]
David Rowley <dgrowleyml@gmail.com> writes:
> So that confirms there were 950k relations in the xl_standby_locks.
> The contents of that message seem to be produced by standby_desc().
> That should be the same WAL record that's processed by standby_redo()
> which adds the 950k locks to the RecoveryLockListsEntry.
> I'm not seeing why 950k becomes 134m.
I figured out what the problem is. The standby's startup process
retains knowledge of all these locks in standby.c's RecoveryLockLists
data structure, which *has no de-duplication capability*. It'll add
another entry to the per-XID list any time it's told about a given
exclusive lock. And checkpoints cause us to regurgitate the entire
set of currently-held exclusive locks into the WAL. So if you have
a process holding a lot of exclusive locks, and sitting on them
across multiple checkpoints, standby startup processes will bloat.
It's not a true leak, in that we know where the memory is and
we'll release it whenever we see that XID commit/abort. And I doubt
that this is a common usage pattern, which probably explains the
lack of previous complaints. Still, bloat bad.
PFA a quick-hack fix that solves this issue by making per-transaction
subsidiary hash tables. That's overkill perhaps; I'm a little worried
about whether this slows down normal cases more than it's worth.
But we ought to do something about this, because aside from the
duplication aspect the current storage of these lists seems mighty
space-inefficient.
regards, tom lane
Attachments:
fix-RecoveryLockLists-data-structure-1.patch (text/x-diff, 9.9K) download
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 9dab931990..14b86cd76a 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -42,7 +42,26 @@ int max_standby_archive_delay = 30 * 1000;
int max_standby_streaming_delay = 30 * 1000;
bool log_recovery_conflict_waits = false;
-static HTAB *RecoveryLockLists;
+/*
+ * Keep track of all the exclusive locks owned by original transactions.
+ * For each original transaction that is known to have any such locks,
+ * there is a RecoveryLockHashEntry in the RecoveryLockHash hash table,
+ * pointing to a subsidiary hash table containing RecoveryLockEntry items.
+ */
+typedef struct RecoveryLockHashEntry
+{
+ TransactionId xid; /* hash key -- must be first */
+ HTAB *hashtab; /* table of locks belonging to xid */
+} RecoveryLockHashEntry;
+
+typedef struct RecoveryLockEntry
+{
+ /* Both Oids are included in the hash key; there is no payload per se */
+ Oid dbOid; /* DB containing table */
+ Oid relOid; /* OID of table */
+} RecoveryLockEntry;
+
+static HTAB *RecoveryLockHash = NULL;
/* Flags set by timeout handlers */
static volatile sig_atomic_t got_standby_deadlock_timeout = false;
@@ -58,15 +77,6 @@ static XLogRecPtr LogCurrentRunningXacts(RunningTransactions CurrRunningXacts);
static void LogAccessExclusiveLocks(int nlocks, xl_standby_lock *locks);
static const char *get_recovery_conflict_desc(ProcSignalReason reason);
-/*
- * Keep track of all the locks owned by a given transaction.
- */
-typedef struct RecoveryLockListsEntry
-{
- TransactionId xid;
- List *locks;
-} RecoveryLockListsEntry;
-
/*
* InitRecoveryTransactionEnvironment
* Initialize tracking of our primary's in-progress transactions.
@@ -85,16 +95,18 @@ InitRecoveryTransactionEnvironment(void)
VirtualTransactionId vxid;
HASHCTL hash_ctl;
+ Assert(RecoveryLockHash == NULL); /* don't run this twice */
+
/*
* Initialize the hash table for tracking the list of locks held by each
* transaction.
*/
hash_ctl.keysize = sizeof(TransactionId);
- hash_ctl.entrysize = sizeof(RecoveryLockListsEntry);
- RecoveryLockLists = hash_create("RecoveryLockLists",
- 64,
- &hash_ctl,
- HASH_ELEM | HASH_BLOBS);
+ hash_ctl.entrysize = sizeof(RecoveryLockHashEntry);
+ RecoveryLockHash = hash_create("RecoveryLockHash",
+ 64,
+ &hash_ctl,
+ HASH_ELEM | HASH_BLOBS);
/*
* Initialize shared invalidation management for Startup process, being
@@ -140,12 +152,12 @@ void
ShutdownRecoveryTransactionEnvironment(void)
{
/*
- * Do nothing if RecoveryLockLists is NULL because which means that
- * transaction tracking has not been yet initialized or has been already
- * shutdowned. This prevents transaction tracking from being shutdowned
- * unexpectedly more than once.
+ * Do nothing if RecoveryLockHash is NULL because that means that
+ * transaction tracking has not yet been initialized or has already been
+ * shut down. This makes it safe to have possibly-redundant calls of this
+ * function during process exit.
*/
- if (RecoveryLockLists == NULL)
+ if (RecoveryLockHash == NULL)
return;
/* Mark all tracked in-progress transactions as finished. */
@@ -155,8 +167,8 @@ ShutdownRecoveryTransactionEnvironment(void)
StandbyReleaseAllLocks();
/* Destroy the hash table of locks. */
- hash_destroy(RecoveryLockLists);
- RecoveryLockLists = NULL;
+ hash_destroy(RecoveryLockHash);
+ RecoveryLockHash = NULL;
/* Cleanup our VirtualTransaction */
VirtualXactLockTableCleanup();
@@ -932,12 +944,13 @@ StandbyLockTimeoutHandler(void)
* We only keep track of AccessExclusiveLocks, which are only ever held by
* one transaction on one relation.
*
- * We keep a hash table of lists of locks in local memory keyed by xid,
- * RecoveryLockLists, so we can keep track of the various entries made by
- * the Startup process's virtual xid in the shared lock table.
- *
- * List elements use type xl_standby_lock, since the WAL record type exactly
- * matches the information that we need to keep track of.
+ * We keep a hash table of hashes of locks in local memory keyed by xid,
+ * RecoveryLockHash, so we can keep track of the various entries made by
+ * the Startup process's virtual xid in the shared lock table. This data
+ * structure allows us to efficiently find all the locks held by a given
+ * original transaction. It also lets us efficiently de-duplicate locks,
+ * which is important because checkpoints will re-report the same locks
+ * already held.
*
* We use session locks rather than normal locks so we don't need
* ResourceOwners.
@@ -947,9 +960,8 @@ StandbyLockTimeoutHandler(void)
void
StandbyAcquireAccessExclusiveLock(TransactionId xid, Oid dbOid, Oid relOid)
{
- RecoveryLockListsEntry *entry;
- xl_standby_lock *newlock;
- LOCKTAG locktag;
+ RecoveryLockHashEntry *entry;
+ RecoveryLockEntry lockent;
bool found;
/* Already processed? */
@@ -964,62 +976,84 @@ StandbyAcquireAccessExclusiveLock(TransactionId xid, Oid dbOid, Oid relOid)
/* dbOid is InvalidOid when we are locking a shared relation. */
Assert(OidIsValid(relOid));
- /* Create a new list for this xid, if we don't have one already. */
- entry = hash_search(RecoveryLockLists, &xid, HASH_ENTER, &found);
+ /* Create a new hash table for this xid, if we don't have one already. */
+ entry = hash_search(RecoveryLockHash, &xid, HASH_ENTER, &found);
if (!found)
{
- entry->xid = xid;
- entry->locks = NIL;
+ HASHCTL hash_ctl;
+ char hash_label[64];
+
+ Assert(entry->xid == xid); /* dynahash should have set this */
+
+ hash_ctl.keysize = sizeof(RecoveryLockEntry);
+ hash_ctl.entrysize = sizeof(RecoveryLockEntry);
+ snprintf(hash_label, sizeof(hash_label), "RecoveryLockHash_%u", xid);
+ entry->hashtab = hash_create(hash_label,
+ 64,
+ &hash_ctl,
+ HASH_ELEM | HASH_BLOBS);
}
- newlock = palloc(sizeof(xl_standby_lock));
- newlock->xid = xid;
- newlock->dbOid = dbOid;
- newlock->relOid = relOid;
- entry->locks = lappend(entry->locks, newlock);
+ /* Create a new hash entry for this lock, unless we have one already. */
+ lockent.dbOid = dbOid;
+ lockent.relOid = relOid;
+ (void) hash_search(entry->hashtab, &lockent, HASH_ENTER, &found);
+ if (!found)
+ {
+ /* It's new, so acquire the lock during replay. */
+ LOCKTAG locktag;
- SET_LOCKTAG_RELATION(locktag, newlock->dbOid, newlock->relOid);
+ SET_LOCKTAG_RELATION(locktag, dbOid, relOid);
- (void) LockAcquire(&locktag, AccessExclusiveLock, true, false);
+ (void) LockAcquire(&locktag, AccessExclusiveLock, true, false);
+ }
}
+/*
+ * Release all the locks recorded in this RecoveryLockHashEntry,
+ * and destroy the subsidiary hash table.
+ */
static void
-StandbyReleaseLockList(List *locks)
+StandbyReleaseLockHash(RecoveryLockHashEntry *entry)
{
- ListCell *lc;
+ HASH_SEQ_STATUS status;
+ RecoveryLockEntry *lockent;
- foreach(lc, locks)
+ hash_seq_init(&status, entry->hashtab);
+ while ((lockent = hash_seq_search(&status)))
{
- xl_standby_lock *lock = (xl_standby_lock *) lfirst(lc);
LOCKTAG locktag;
elog(trace_recovery(DEBUG4),
"releasing recovery lock: xid %u db %u rel %u",
- lock->xid, lock->dbOid, lock->relOid);
- SET_LOCKTAG_RELATION(locktag, lock->dbOid, lock->relOid);
+ entry->xid, lockent->dbOid, lockent->relOid);
+ SET_LOCKTAG_RELATION(locktag, lockent->dbOid, lockent->relOid);
if (!LockRelease(&locktag, AccessExclusiveLock, true))
{
elog(LOG,
- "RecoveryLockLists contains entry for lock no longer recorded by lock manager: xid %u database %u relation %u",
- lock->xid, lock->dbOid, lock->relOid);
+ "RecoveryLockHash contains entry for lock no longer recorded by lock manager: xid %u database %u relation %u",
+ entry->xid, lockent->dbOid, lockent->relOid);
Assert(false);
}
}
- list_free_deep(locks);
+ hash_destroy(entry->hashtab);
}
+/*
+ * Release locks for specific XID, or all locks if it's InvalidXid.
+ */
static void
StandbyReleaseLocks(TransactionId xid)
{
- RecoveryLockListsEntry *entry;
+ RecoveryLockHashEntry *entry;
if (TransactionIdIsValid(xid))
{
- if ((entry = hash_search(RecoveryLockLists, &xid, HASH_FIND, NULL)))
+ if ((entry = hash_search(RecoveryLockHash, &xid, HASH_FIND, NULL)))
{
- StandbyReleaseLockList(entry->locks);
- hash_search(RecoveryLockLists, entry, HASH_REMOVE, NULL);
+ StandbyReleaseLockHash(entry);
+ hash_search(RecoveryLockHash, entry, HASH_REMOVE, NULL);
}
}
else
@@ -1028,7 +1062,7 @@ StandbyReleaseLocks(TransactionId xid)
/*
* Release locks for a transaction tree, starting at xid down, from
- * RecoveryLockLists.
+ * RecoveryLockHash.
*
* Called during WAL replay of COMMIT/ROLLBACK when in hot standby mode,
* to remove any AccessExclusiveLocks requested by a transaction.
@@ -1051,15 +1085,15 @@ void
StandbyReleaseAllLocks(void)
{
HASH_SEQ_STATUS status;
- RecoveryLockListsEntry *entry;
+ RecoveryLockHashEntry *entry;
elog(trace_recovery(DEBUG2), "release all standby locks");
- hash_seq_init(&status, RecoveryLockLists);
+ hash_seq_init(&status, RecoveryLockHash);
while ((entry = hash_seq_search(&status)))
{
- StandbyReleaseLockList(entry->locks);
- hash_search(RecoveryLockLists, entry, HASH_REMOVE, NULL);
+ StandbyReleaseLockHash(entry);
+ hash_search(RecoveryLockHash, entry, HASH_REMOVE, NULL);
}
}
@@ -1072,9 +1106,9 @@ void
StandbyReleaseOldLocks(TransactionId oldxid)
{
HASH_SEQ_STATUS status;
- RecoveryLockListsEntry *entry;
+ RecoveryLockHashEntry *entry;
- hash_seq_init(&status, RecoveryLockLists);
+ hash_seq_init(&status, RecoveryLockHash);
while ((entry = hash_seq_search(&status)))
{
Assert(TransactionIdIsValid(entry->xid));
@@ -1088,8 +1122,8 @@ StandbyReleaseOldLocks(TransactionId oldxid)
continue;
/* Remove all locks and hash table entry. */
- StandbyReleaseLockList(entry->locks);
- hash_search(RecoveryLockLists, entry, HASH_REMOVE, NULL);
+ StandbyReleaseLockHash(entry);
+ hash_search(RecoveryLockHash, entry, HASH_REMOVE, NULL);
}
}
=
* Re: Startup process on a hot standby crashes with an error "invalid memory alloc request size 1073741824" while replaying "Standby/LOCK" records
@ 2022-10-04 23:53 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 11+ messages in thread
From: Tom Lane @ 2022-10-04 23:53 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Dmitriy Kuzmin <kuzmin.db4@gmail.com>; pgsql-hackers@lists.postgresql.org
I wrote:
> PFA a quick-hack fix that solves this issue by making per-transaction
> subsidiary hash tables. That's overkill perhaps; I'm a little worried
> about whether this slows down normal cases more than it's worth.
> But we ought to do something about this, because aside from the
> duplication aspect the current storage of these lists seems mighty
> space-inefficient.
After further thought, maybe it'd be better to do it as attached,
with one long-lived hash table for all the locks. This is a shade
less space-efficient than the current code once you account for
dynahash overhead, but the per-transaction overhead should be lower
than the previous patch since we only need to create/destroy a hash
table entry not a whole hash table.
regards, tom lane
Attachments:
fix-RecoveryLockLists-data-structure-2.patch (text/x-diff, 10.5K) download
diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c
index 9dab931990..7db86f7885 100644
--- a/src/backend/storage/ipc/standby.c
+++ b/src/backend/storage/ipc/standby.c
@@ -42,7 +42,29 @@ int max_standby_archive_delay = 30 * 1000;
int max_standby_streaming_delay = 30 * 1000;
bool log_recovery_conflict_waits = false;
-static HTAB *RecoveryLockLists;
+/*
+ * Keep track of all the exclusive locks owned by original transactions.
+ * For each known exclusive lock, there is a RecoveryLockEntry in the
+ * RecoveryLockHash hash table. All RecoveryLockEntrys belonging to a
+ * given XID are chained together so that we can find them easily.
+ * For each original transaction that is known to have any such locks,
+ * there is a RecoveryLockXidEntry in the RecoveryLockXidHash hash table,
+ * which stores the head of the chain of its locks.
+ */
+typedef struct RecoveryLockEntry
+{
+ xl_standby_lock key; /* hash key: xid, dbOid, relOid */
+ struct RecoveryLockEntry *next; /* chain link */
+} RecoveryLockEntry;
+
+typedef struct RecoveryLockXidEntry
+{
+ TransactionId xid; /* hash key -- must be first */
+ struct RecoveryLockEntry *head; /* chain head */
+} RecoveryLockXidEntry;
+
+static HTAB *RecoveryLockHash = NULL;
+static HTAB *RecoveryLockXidHash = NULL;
/* Flags set by timeout handlers */
static volatile sig_atomic_t got_standby_deadlock_timeout = false;
@@ -58,15 +80,6 @@ static XLogRecPtr LogCurrentRunningXacts(RunningTransactions CurrRunningXacts);
static void LogAccessExclusiveLocks(int nlocks, xl_standby_lock *locks);
static const char *get_recovery_conflict_desc(ProcSignalReason reason);
-/*
- * Keep track of all the locks owned by a given transaction.
- */
-typedef struct RecoveryLockListsEntry
-{
- TransactionId xid;
- List *locks;
-} RecoveryLockListsEntry;
-
/*
* InitRecoveryTransactionEnvironment
* Initialize tracking of our primary's in-progress transactions.
@@ -85,16 +98,24 @@ InitRecoveryTransactionEnvironment(void)
VirtualTransactionId vxid;
HASHCTL hash_ctl;
+ Assert(RecoveryLockHash == NULL); /* don't run this twice */
+
/*
- * Initialize the hash table for tracking the list of locks held by each
+ * Initialize the hash tables for tracking the locks held by each
* transaction.
*/
+ hash_ctl.keysize = sizeof(xl_standby_lock);
+ hash_ctl.entrysize = sizeof(RecoveryLockEntry);
+ RecoveryLockHash = hash_create("RecoveryLockHash",
+ 64,
+ &hash_ctl,
+ HASH_ELEM | HASH_BLOBS);
hash_ctl.keysize = sizeof(TransactionId);
- hash_ctl.entrysize = sizeof(RecoveryLockListsEntry);
- RecoveryLockLists = hash_create("RecoveryLockLists",
- 64,
- &hash_ctl,
- HASH_ELEM | HASH_BLOBS);
+ hash_ctl.entrysize = sizeof(RecoveryLockXidEntry);
+ RecoveryLockXidHash = hash_create("RecoveryLockXidHash",
+ 64,
+ &hash_ctl,
+ HASH_ELEM | HASH_BLOBS);
/*
* Initialize shared invalidation management for Startup process, being
@@ -140,12 +161,12 @@ void
ShutdownRecoveryTransactionEnvironment(void)
{
/*
- * Do nothing if RecoveryLockLists is NULL because which means that
- * transaction tracking has not been yet initialized or has been already
- * shutdowned. This prevents transaction tracking from being shutdowned
- * unexpectedly more than once.
+ * Do nothing if RecoveryLockHash is NULL because that means that
+ * transaction tracking has not yet been initialized or has already been
+ * shut down. This makes it safe to have possibly-redundant calls of this
+ * function during process exit.
*/
- if (RecoveryLockLists == NULL)
+ if (RecoveryLockHash == NULL)
return;
/* Mark all tracked in-progress transactions as finished. */
@@ -154,9 +175,11 @@ ShutdownRecoveryTransactionEnvironment(void)
/* Release all locks the tracked transactions were holding */
StandbyReleaseAllLocks();
- /* Destroy the hash table of locks. */
- hash_destroy(RecoveryLockLists);
- RecoveryLockLists = NULL;
+ /* Destroy the lock hash tables. */
+ hash_destroy(RecoveryLockHash);
+ hash_destroy(RecoveryLockXidHash);
+ RecoveryLockHash = NULL;
+ RecoveryLockXidHash = NULL;
/* Cleanup our VirtualTransaction */
VirtualXactLockTableCleanup();
@@ -932,12 +955,12 @@ StandbyLockTimeoutHandler(void)
* We only keep track of AccessExclusiveLocks, which are only ever held by
* one transaction on one relation.
*
- * We keep a hash table of lists of locks in local memory keyed by xid,
- * RecoveryLockLists, so we can keep track of the various entries made by
- * the Startup process's virtual xid in the shared lock table.
- *
- * List elements use type xl_standby_lock, since the WAL record type exactly
- * matches the information that we need to keep track of.
+ * We keep a table of known locks in the RecoveryLockHash hash table.
+ * The point of that table is to let us efficiently de-duplicate locks,
+ * which is important because checkpoints will re-report the same locks
+ * already held. There is also a RecoveryLockXidHash table with one entry
+ * per xid, which allows us to efficiently find all the locks held by a
+ * given original transaction.
*
* We use session locks rather than normal locks so we don't need
* ResourceOwners.
@@ -947,8 +970,9 @@ StandbyLockTimeoutHandler(void)
void
StandbyAcquireAccessExclusiveLock(TransactionId xid, Oid dbOid, Oid relOid)
{
- RecoveryLockListsEntry *entry;
- xl_standby_lock *newlock;
+ RecoveryLockXidEntry *xidentry;
+ RecoveryLockEntry *lockentry;
+ xl_standby_lock key;
LOCKTAG locktag;
bool found;
@@ -964,62 +988,79 @@ StandbyAcquireAccessExclusiveLock(TransactionId xid, Oid dbOid, Oid relOid)
/* dbOid is InvalidOid when we are locking a shared relation. */
Assert(OidIsValid(relOid));
- /* Create a new list for this xid, if we don't have one already. */
- entry = hash_search(RecoveryLockLists, &xid, HASH_ENTER, &found);
+ /* Create a hash entry for this xid, if we don't have one already. */
+ xidentry = hash_search(RecoveryLockXidHash, &xid, HASH_ENTER, &found);
if (!found)
{
- entry->xid = xid;
- entry->locks = NIL;
+ Assert(xidentry->xid == xid); /* dynahash should have set this */
+ xidentry->head = NULL;
}
- newlock = palloc(sizeof(xl_standby_lock));
- newlock->xid = xid;
- newlock->dbOid = dbOid;
- newlock->relOid = relOid;
- entry->locks = lappend(entry->locks, newlock);
+ /* Create a hash entry for this lock, unless we have one already. */
+ key.xid = xid;
+ key.dbOid = dbOid;
+ key.relOid = relOid;
+ lockentry = hash_search(RecoveryLockHash, &key, HASH_ENTER, &found);
+ if (!found)
+ {
+ /* It's new, so link it into the XID's list ... */
+ lockentry->next = xidentry->head;
+ xidentry->head = lockentry;
- SET_LOCKTAG_RELATION(locktag, newlock->dbOid, newlock->relOid);
+ /* ... and acquire the lock locally. */
+ SET_LOCKTAG_RELATION(locktag, dbOid, relOid);
- (void) LockAcquire(&locktag, AccessExclusiveLock, true, false);
+ (void) LockAcquire(&locktag, AccessExclusiveLock, true, false);
+ }
}
+/*
+ * Release all the locks associated with this RecoveryLockXidEntry.
+ */
static void
-StandbyReleaseLockList(List *locks)
+StandbyReleaseXidEntryLocks(RecoveryLockXidEntry *xidentry)
{
- ListCell *lc;
+ RecoveryLockEntry *entry;
+ RecoveryLockEntry *next;
- foreach(lc, locks)
+ for (entry = xidentry->head; entry != NULL; entry = next)
{
- xl_standby_lock *lock = (xl_standby_lock *) lfirst(lc);
LOCKTAG locktag;
elog(trace_recovery(DEBUG4),
"releasing recovery lock: xid %u db %u rel %u",
- lock->xid, lock->dbOid, lock->relOid);
- SET_LOCKTAG_RELATION(locktag, lock->dbOid, lock->relOid);
+ entry->key.xid, entry->key.dbOid, entry->key.relOid);
+ /* Release the lock ... */
+ SET_LOCKTAG_RELATION(locktag, entry->key.dbOid, entry->key.relOid);
if (!LockRelease(&locktag, AccessExclusiveLock, true))
{
elog(LOG,
- "RecoveryLockLists contains entry for lock no longer recorded by lock manager: xid %u database %u relation %u",
- lock->xid, lock->dbOid, lock->relOid);
+ "RecoveryLockHash contains entry for lock no longer recorded by lock manager: xid %u database %u relation %u",
+ entry->key.xid, entry->key.dbOid, entry->key.relOid);
Assert(false);
}
+ /* ... and remove the per-lock hash entry */
+ next = entry->next;
+ hash_search(RecoveryLockHash, entry, HASH_REMOVE, NULL);
}
- list_free_deep(locks);
+ xidentry->head = NULL; /* just for paranoia */
}
+/*
+ * Release locks for specific XID, or all locks if it's InvalidXid.
+ */
static void
StandbyReleaseLocks(TransactionId xid)
{
- RecoveryLockListsEntry *entry;
+ RecoveryLockXidEntry *entry;
if (TransactionIdIsValid(xid))
{
- if ((entry = hash_search(RecoveryLockLists, &xid, HASH_FIND, NULL)))
+ if ((entry = hash_search(RecoveryLockXidHash, &xid, HASH_FIND, NULL)))
{
- StandbyReleaseLockList(entry->locks);
- hash_search(RecoveryLockLists, entry, HASH_REMOVE, NULL);
+ StandbyReleaseXidEntryLocks(entry);
+ hash_search(RecoveryLockXidHash, entry, HASH_REMOVE, NULL);
}
}
else
@@ -1028,7 +1069,7 @@ StandbyReleaseLocks(TransactionId xid)
/*
* Release locks for a transaction tree, starting at xid down, from
- * RecoveryLockLists.
+ * RecoveryLockXidHash.
*
* Called during WAL replay of COMMIT/ROLLBACK when in hot standby mode,
* to remove any AccessExclusiveLocks requested by a transaction.
@@ -1051,15 +1092,15 @@ void
StandbyReleaseAllLocks(void)
{
HASH_SEQ_STATUS status;
- RecoveryLockListsEntry *entry;
+ RecoveryLockXidEntry *entry;
elog(trace_recovery(DEBUG2), "release all standby locks");
- hash_seq_init(&status, RecoveryLockLists);
+ hash_seq_init(&status, RecoveryLockXidHash);
while ((entry = hash_seq_search(&status)))
{
- StandbyReleaseLockList(entry->locks);
- hash_search(RecoveryLockLists, entry, HASH_REMOVE, NULL);
+ StandbyReleaseXidEntryLocks(entry);
+ hash_search(RecoveryLockXidHash, entry, HASH_REMOVE, NULL);
}
}
@@ -1072,9 +1113,9 @@ void
StandbyReleaseOldLocks(TransactionId oldxid)
{
HASH_SEQ_STATUS status;
- RecoveryLockListsEntry *entry;
+ RecoveryLockXidEntry *entry;
- hash_seq_init(&status, RecoveryLockLists);
+ hash_seq_init(&status, RecoveryLockXidHash);
while ((entry = hash_seq_search(&status)))
{
Assert(TransactionIdIsValid(entry->xid));
@@ -1088,8 +1129,8 @@ StandbyReleaseOldLocks(TransactionId oldxid)
continue;
/* Remove all locks and hash table entry. */
- StandbyReleaseLockList(entry->locks);
- hash_search(RecoveryLockLists, entry, HASH_REMOVE, NULL);
+ StandbyReleaseXidEntryLocks(entry);
+ hash_search(RecoveryLockXidHash, entry, HASH_REMOVE, NULL);
}
}
=
* Re: Startup process on a hot standby crashes with an error "invalid memory alloc request size 1073741824" while replaying "Standby/LOCK" records
@ 2022-10-05 00:15 Nathan Bossart <nathandbossart@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 11+ messages in thread
From: Nathan Bossart @ 2022-10-05 00:15 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: David Rowley <dgrowleyml@gmail.com>; Dmitriy Kuzmin <kuzmin.db4@gmail.com>; pgsql-hackers@lists.postgresql.org
On Tue, Oct 04, 2022 at 07:53:11PM -0400, Tom Lane wrote:
> I wrote:
>> PFA a quick-hack fix that solves this issue by making per-transaction
>> subsidiary hash tables. That's overkill perhaps; I'm a little worried
>> about whether this slows down normal cases more than it's worth.
>> But we ought to do something about this, because aside from the
>> duplication aspect the current storage of these lists seems mighty
>> space-inefficient.
>
> After further thought, maybe it'd be better to do it as attached,
> with one long-lived hash table for all the locks. This is a shade
> less space-efficient than the current code once you account for
> dynahash overhead, but the per-transaction overhead should be lower
> than the previous patch since we only need to create/destroy a hash
> table entry not a whole hash table.
This feels like a natural way to solve this problem. I saw several cases
of the issue that was fixed with 6301c3a, so I'm inclined to believe this
usage pattern is actually somewhat common.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Startup process on a hot standby crashes with an error "invalid memory alloc request size 1073741824" while replaying "Standby/LOCK" records
@ 2022-10-05 01:41 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: Nathan Bossart <nathandbossart@gmail.com>
0 siblings, 1 reply; 11+ messages in thread
From: Kyotaro Horiguchi @ 2022-10-05 01:41 UTC (permalink / raw)
To: nathandbossart@gmail.com; +Cc: tgl@sss.pgh.pa.us; dgrowleyml@gmail.com; kuzmin.db4@gmail.com; pgsql-hackers@lists.postgresql.org
At Tue, 4 Oct 2022 17:15:31 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in
> On Tue, Oct 04, 2022 at 07:53:11PM -0400, Tom Lane wrote:
> > I wrote:
> >> PFA a quick-hack fix that solves this issue by making per-transaction
> >> subsidiary hash tables. That's overkill perhaps; I'm a little worried
> >> about whether this slows down normal cases more than it's worth.
> >> But we ought to do something about this, because aside from the
> >> duplication aspect the current storage of these lists seems mighty
> >> space-inefficient.
> >
> > After further thought, maybe it'd be better to do it as attached,
> > with one long-lived hash table for all the locks. This is a shade
> > less space-efficient than the current code once you account for
> > dynahash overhead, but the per-transaction overhead should be lower
> > than the previous patch since we only need to create/destroy a hash
> > table entry not a whole hash table.
First one is straight forward outcome from the current implement but I
like the new one. I agree that it is natural and that the expected
overhead per (typical) transaction is lower than both the first one
and doing the same operation on a list. I don't think that space
inefficiency in that extent doesn't matter since it is the startup
process.
> This feels like a natural way to solve this problem. I saw several cases
> of the issue that was fixed with 6301c3a, so I'm inclined to believe this
> usage pattern is actually somewhat common.
So releasing locks becomes somewhat slower? But it seems to still be
far faster than massively repetitive head-removal in a list.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Startup process on a hot standby crashes with an error "invalid memory alloc request size 1073741824" while replaying "Standby/LOCK" records
@ 2022-10-05 15:30 Tom Lane <tgl@sss.pgh.pa.us>
parent: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
0 siblings, 2 replies; 11+ messages in thread
From: Tom Lane @ 2022-10-05 15:30 UTC (permalink / raw)
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; +Cc: nathandbossart@gmail.com; dgrowleyml@gmail.com; kuzmin.db4@gmail.com; pgsql-hackers@lists.postgresql.org
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Tue, 4 Oct 2022 17:15:31 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in
>> On Tue, Oct 04, 2022 at 07:53:11PM -0400, Tom Lane wrote:
>>> After further thought, maybe it'd be better to do it as attached,
>>> with one long-lived hash table for all the locks.
> First one is straight forward outcome from the current implement but I
> like the new one. I agree that it is natural and that the expected
> overhead per (typical) transaction is lower than both the first one
> and doing the same operation on a list. I don't think that space
> inefficiency in that extent doesn't matter since it is the startup
> process.
To get some hard numbers about this, I made a quick hack to collect
getrusage() numbers for the startup process (patch attached for
documentation purposes). I then ran the recovery/t/027_stream_regress.pl
test a few times and collected the stats (also attached). This seems
like a reasonably decent baseline test, since the core regression tests
certainly take lots of AccessExclusiveLocks what with all the DDL
involved, though they shouldn't ever take large numbers at once. Also
they don't run long enough for any lock list bloat to occur, so these
numbers don't reflect a case where the patches would provide benefit.
If you look hard, there's maybe about a 1% user-CPU penalty for patch 2,
although that's well below the run-to-run variation so it's hard to be
sure that it's real. The same comments apply to the max resident size
stats. So I'm comforted that there's not a significant penalty here.
I'll go ahead with patch 2 if there's not objection.
One other point to discuss: should we consider back-patching? I've
got mixed feelings about that myself. I don't think that cases where
this helps significantly are at all mainstream, so I'm kind of leaning
to "patch HEAD only".
regards, tom lane
Attachments:
startup-stats.patch (text/x-diff, 853B) download
diff --git a/src/backend/postmaster/startup.c b/src/backend/postmaster/startup.c
index f99186eab7..8fcbf898f9 100644
--- a/src/backend/postmaster/startup.c
+++ b/src/backend/postmaster/startup.c
@@ -32,6 +32,7 @@
#include "storage/pmsignal.h"
#include "storage/procsignal.h"
#include "storage/standby.h"
+#include "tcop/tcopprot.h"
#include "utils/guc.h"
#include "utils/memutils.h"
#include "utils/timeout.h"
@@ -219,6 +220,8 @@ StartupProcExit(int code, Datum arg)
/* Shutdown the recovery environment */
if (standbyState != STANDBY_DISABLED)
ShutdownRecoveryTransactionEnvironment();
+
+ ShowUsage("STARTUP PROCESS STATISTICS");
}
@@ -229,6 +232,8 @@ StartupProcExit(int code, Datum arg)
void
StartupProcessMain(void)
{
+ ResetUsage();
+
/* Arrange to clean up at startup process exit */
on_shmem_exit(StartupProcExit, 0);
=
results.txt (text/plain, 5.2K) download
Startup process stats from 027_stream_regress_standby_1.log
HEAD, --disable-cassert:
2022-10-05 10:57:15.561 EDT [2390709] LOG: STARTUP PROCESS STATISTICS
2022-10-05 10:57:15.561 EDT [2390709] DETAIL: ! system usage stats:
! 0.935814 s user, 1.019952 s system, 7.226729 s elapsed
! [0.936104 s user, 1.019952 s system total]
! 9088 kB max resident size
! 0/430608 [0/430608] filesystem blocks in/out
! 0/916 [0/1026] page faults/reclaims, 0 [0] swaps
! 0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
! 9925/180 [9925/180] voluntary/involuntary context switches
2022-10-05 11:02:58.542 EDT [2406908] LOG: STARTUP PROCESS STATISTICS
2022-10-05 11:02:58.542 EDT [2406908] DETAIL: ! system usage stats:
! 0.957075 s user, 1.029002 s system, 7.206675 s elapsed
! [0.957075 s user, 1.029260 s system total]
! 9124 kB max resident size
! 0/430512 [0/430512] filesystem blocks in/out
! 0/919 [0/1028] page faults/reclaims, 0 [0] swaps
! 0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
! 9575/174 [9575/175] voluntary/involuntary context switches
2022-10-05 11:03:35.436 EDT [2409026] LOG: STARTUP PROCESS STATISTICS
2022-10-05 11:03:35.436 EDT [2409026] DETAIL: ! system usage stats:
! 0.936716 s user, 1.117313 s system, 7.208555 s elapsed
! [0.936716 s user, 1.117606 s system total]
! 9220 kB max resident size
! 0/429712 [0/429712] filesystem blocks in/out
! 0/917 [0/1028] page faults/reclaims, 0 [0] swaps
! 0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
! 9509/178 [9509/178] voluntary/involuntary context switches
with Patch 1:
2022-10-05 10:58:50.088 EDT [2393477] LOG: STARTUP PROCESS STATISTICS
2022-10-05 10:58:50.088 EDT [2393477] DETAIL: ! system usage stats:
! 0.938248 s user, 1.103261 s system, 7.269850 s elapsed
! [0.938498 s user, 1.103261 s system total]
! 9288 kB max resident size
! 0/430912 [0/430912] filesystem blocks in/out
! 0/936 [0/1046] page faults/reclaims, 0 [0] swaps
! 0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
! 9649/176 [9649/176] voluntary/involuntary context switches
2022-10-05 10:59:29.687 EDT [2395617] LOG: STARTUP PROCESS STATISTICS
2022-10-05 10:59:29.687 EDT [2395617] DETAIL: ! system usage stats:
! 0.887786 s user, 1.147573 s system, 7.205528 s elapsed
! [0.888080 s user, 1.147573 s system total]
! 9172 kB max resident size
! 0/430704 [0/430704] filesystem blocks in/out
! 0/931 [0/1041] page faults/reclaims, 0 [0] swaps
! 0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
! 9366/188 [9366/188] voluntary/involuntary context switches
2022-10-05 10:59:54.584 EDT [2397710] LOG: STARTUP PROCESS STATISTICS
2022-10-05 10:59:54.584 EDT [2397710] DETAIL: ! system usage stats:
! 0.966489 s user, 0.903671 s system, 7.148393 s elapsed
! [0.966489 s user, 0.903919 s system total]
! 9064 kB max resident size
! 0/429376 [0/429376] filesystem blocks in/out
! 0/932 [0/1042] page faults/reclaims, 0 [0] swaps
! 0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
! 10067/164 [10067/164] voluntary/involuntary context switches
with Patch 2:
2022-10-05 11:01:10.405 EDT [2400381] LOG: STARTUP PROCESS STATISTICS
2022-10-05 11:01:10.405 EDT [2400381] DETAIL: ! system usage stats:
! 0.964738 s user, 1.111669 s system, 7.258135 s elapsed
! [0.965012 s user, 1.111669 s system total]
! 9232 kB max resident size
! 0/430752 [0/430752] filesystem blocks in/out
! 0/923 [0/1034] page faults/reclaims, 0 [0] swaps
! 0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
! 9443/206 [9443/206] voluntary/involuntary context switches
2022-10-05 11:01:41.423 EDT [2402517] LOG: STARTUP PROCESS STATISTICS
2022-10-05 11:01:41.423 EDT [2402517] DETAIL: ! system usage stats:
! 0.941057 s user, 1.069866 s system, 7.194619 s elapsed
! [0.941318 s user, 1.069866 s system total]
! 9140 kB max resident size
! 0/430240 [0/430240] filesystem blocks in/out
! 0/918 [0/1029] page faults/reclaims, 0 [0] swaps
! 0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
! 9627/194 [9627/195] voluntary/involuntary context switches
2022-10-05 11:02:11.136 EDT [2404608] LOG: STARTUP PROCESS STATISTICS
2022-10-05 11:02:11.136 EDT [2404608] DETAIL: ! system usage stats:
! 0.956157 s user, 1.041602 s system, 7.187873 s elapsed
! [0.956424 s user, 1.041602 s system total]
! 9224 kB max resident size
! 0/430448 [0/430448] filesystem blocks in/out
! 0/923 [0/1033] page faults/reclaims, 0 [0] swaps
! 0 [0] signals rcvd, 0/0 [0/0] messages rcvd/sent
! 9583/171 [9583/171] voluntary/involuntary context switches
* Re: Startup process on a hot standby crashes with an error "invalid memory alloc request size 1073741824" while replaying "Standby/LOCK" records
@ 2022-10-05 19:00 Nathan Bossart <nathandbossart@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 11+ messages in thread
From: Nathan Bossart @ 2022-10-05 19:00 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; dgrowleyml@gmail.com; kuzmin.db4@gmail.com; pgsql-hackers@lists.postgresql.org
On Wed, Oct 05, 2022 at 11:30:22AM -0400, Tom Lane wrote:
> One other point to discuss: should we consider back-patching? I've
> got mixed feelings about that myself. I don't think that cases where
> this helps significantly are at all mainstream, so I'm kind of leaning
> to "patch HEAD only".
+1. It can always be back-patched in the future if there are additional
reports.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Startup process on a hot standby crashes with an error "invalid memory alloc request size 1073741824" while replaying "Standby/LOCK" records
@ 2022-10-10 12:24 Simon Riggs <simon.riggs@enterprisedb.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 11+ messages in thread
From: Simon Riggs @ 2022-10-10 12:24 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>; nathandbossart@gmail.com; dgrowleyml@gmail.com; kuzmin.db4@gmail.com; pgsql-hackers@lists.postgresql.org
On Wed, 5 Oct 2022 at 16:30, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> > At Tue, 4 Oct 2022 17:15:31 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in
> >> On Tue, Oct 04, 2022 at 07:53:11PM -0400, Tom Lane wrote:
> >>> After further thought, maybe it'd be better to do it as attached,
> >>> with one long-lived hash table for all the locks.
>
> > First one is straight forward outcome from the current implement but I
> > like the new one. I agree that it is natural and that the expected
> > overhead per (typical) transaction is lower than both the first one
> > and doing the same operation on a list. I don't think that space
> > inefficiency in that extent doesn't matter since it is the startup
> > process.
>
> To get some hard numbers about this, I made a quick hack to collect
> getrusage() numbers for the startup process (patch attached for
> documentation purposes). I then ran the recovery/t/027_stream_regress.pl
> test a few times and collected the stats (also attached). This seems
> like a reasonably decent baseline test, since the core regression tests
> certainly take lots of AccessExclusiveLocks what with all the DDL
> involved, though they shouldn't ever take large numbers at once. Also
> they don't run long enough for any lock list bloat to occur, so these
> numbers don't reflect a case where the patches would provide benefit.
>
> If you look hard, there's maybe about a 1% user-CPU penalty for patch 2,
> although that's well below the run-to-run variation so it's hard to be
> sure that it's real. The same comments apply to the max resident size
> stats. So I'm comforted that there's not a significant penalty here.
>
> I'll go ahead with patch 2 if there's not objection.
Happy to see this change.
> One other point to discuss: should we consider back-patching? I've
> got mixed feelings about that myself. I don't think that cases where
> this helps significantly are at all mainstream, so I'm kind of leaning
> to "patch HEAD only".
It looks fine to eventually backpatch, since StandbyReleaseLockTree()
was optimized to only be called when the transaction had actually done
some AccessExclusiveLocks.
So the performance loss is minor and isolated to the users of such
locks, so I see no problems with it.
--
Simon Riggs http://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Startup process on a hot standby crashes with an error "invalid memory alloc request size 1073741824" while replaying "Standby/LOCK" records
@ 2022-10-11 06:48 Kyotaro Horiguchi <horikyota.ntt@gmail.com>
parent: Simon Riggs <simon.riggs@enterprisedb.com>
0 siblings, 0 replies; 11+ messages in thread
From: Kyotaro Horiguchi @ 2022-10-11 06:48 UTC (permalink / raw)
To: simon.riggs@enterprisedb.com; +Cc: tgl@sss.pgh.pa.us; nathandbossart@gmail.com; dgrowleyml@gmail.com; kuzmin.db4@gmail.com; pgsql-hackers@lists.postgresql.org
> On Wed, 5 Oct 2022 at 16:30, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > One other point to discuss: should we consider back-patching? I've
> > got mixed feelings about that myself. I don't think that cases where
> > this helps significantly are at all mainstream, so I'm kind of leaning
> > to "patch HEAD only".
At Mon, 10 Oct 2022 13:24:34 +0100, Simon Riggs <simon.riggs@enterprisedb.com> wrote in
> It looks fine to eventually backpatch, since StandbyReleaseLockTree()
> was optimized to only be called when the transaction had actually done
> some AccessExclusiveLocks.
>
> So the performance loss is minor and isolated to the users of such
> locks, so I see no problems with it.
At Wed, 5 Oct 2022 12:00:55 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in
> +1. It can always be back-patched in the future if there are additional
> reports.
The third +1 from me.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2022-10-11 06:48 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-09-05 10:19 Startup process on a hot standby crashes with an error "invalid memory alloc request size 1073741824" while replaying "Standby/LOCK" records Dmitriy Kuzmin <kuzmin.db4@gmail.com>
2022-09-05 12:13 ` David Rowley <dgrowleyml@gmail.com>
2022-09-06 06:38 ` Dmitriy Kuzmin <kuzmin.db4@gmail.com>
2022-10-04 22:54 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-10-04 23:53 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-10-05 00:15 ` Nathan Bossart <nathandbossart@gmail.com>
2022-10-05 01:41 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
2022-10-05 15:30 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-10-05 19:00 ` Nathan Bossart <nathandbossart@gmail.com>
2022-10-10 12:24 ` Simon Riggs <simon.riggs@enterprisedb.com>
2022-10-11 06:48 ` Kyotaro Horiguchi <horikyota.ntt@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox