public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v45 2/7] Add conditional lock feature to dshash 5+ messages / 3 participants [nested] [flat]
* [PATCH v45 2/7] Add conditional lock feature to dshash @ 2020-03-13 07:58 Kyotaro Horiguchi <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Kyotaro Horiguchi @ 2020-03-13 07:58 UTC (permalink / raw) Dshash currently waits for lock unconditionally. It is inconvenient when we want to avoid being blocked by other processes. This commit adds alternative functions of dshash_find and dshash_find_or_insert that allows immediate return on lock failure. --- src/backend/lib/dshash.c | 98 +++++++++++++++++++++------------------- src/include/lib/dshash.h | 3 ++ 2 files changed, 55 insertions(+), 46 deletions(-) diff --git a/src/backend/lib/dshash.c b/src/backend/lib/dshash.c index 520bfa0979..853d78b528 100644 --- a/src/backend/lib/dshash.c +++ b/src/backend/lib/dshash.c @@ -383,6 +383,10 @@ dshash_get_hash_table_handle(dshash_table *hash_table) * the caller must take care to ensure that the entry is not left corrupted. * The lock mode is either shared or exclusive depending on 'exclusive'. * + * If found is not NULL, *found is set to true if the key is found in the hash + * table. If the key is not found, *found is set to false and a pointer to a + * newly created entry is returned. + * * The caller must not lock a lock already. * * Note that the lock held is in fact an LWLock, so interrupts will be held on @@ -392,36 +396,7 @@ dshash_get_hash_table_handle(dshash_table *hash_table) void * dshash_find(dshash_table *hash_table, const void *key, bool exclusive) { - dshash_hash hash; - size_t partition; - dshash_table_item *item; - - hash = hash_key(hash_table, key); - partition = PARTITION_FOR_HASH(hash); - - Assert(hash_table->control->magic == DSHASH_MAGIC); - Assert(!hash_table->find_locked); - - LWLockAcquire(PARTITION_LOCK(hash_table, partition), - exclusive ? LW_EXCLUSIVE : LW_SHARED); - ensure_valid_bucket_pointers(hash_table); - - /* Search the active bucket. */ - item = find_in_bucket(hash_table, key, BUCKET_FOR_HASH(hash_table, hash)); - - if (!item) - { - /* Not found. */ - LWLockRelease(PARTITION_LOCK(hash_table, partition)); - return NULL; - } - else - { - /* The caller will free the lock by calling dshash_release_lock. */ - hash_table->find_locked = true; - hash_table->find_exclusively_locked = exclusive; - return ENTRY_FROM_ITEM(item); - } + return dshash_find_extended(hash_table, key, exclusive, false, false, NULL); } /* @@ -439,31 +414,60 @@ dshash_find_or_insert(dshash_table *hash_table, const void *key, bool *found) { - dshash_hash hash; - size_t partition_index; - dshash_partition *partition; + return dshash_find_extended(hash_table, key, true, false, true, found); +} + + +/* + * Find the key in the hash table. + * + * "exclusive" is the lock mode in which the partition for the returned item + * is locked. If "nowait" is true, the function immediately returns if + * required lock was not acquired. "insert" indicates insert mode. In this + * mode new entry is inserted and set *found to false. *found is set to true if + * found. "found" must be non-null in this mode. + */ +void * +dshash_find_extended(dshash_table *hash_table, const void *key, + bool exclusive, bool nowait, bool insert, bool *found) +{ + dshash_hash hash = hash_key(hash_table, key); + size_t partidx = PARTITION_FOR_HASH(hash); + dshash_partition *partition = &hash_table->control->partitions[partidx]; + LWLockMode lockmode = exclusive ? LW_EXCLUSIVE : LW_SHARED; dshash_table_item *item; - hash = hash_key(hash_table, key); - partition_index = PARTITION_FOR_HASH(hash); - partition = &hash_table->control->partitions[partition_index]; - - Assert(hash_table->control->magic == DSHASH_MAGIC); - Assert(!hash_table->find_locked); + /* must be exclusive when insert allowed */ + Assert(!insert || (exclusive && found != NULL)); restart: - LWLockAcquire(PARTITION_LOCK(hash_table, partition_index), - LW_EXCLUSIVE); + if (!nowait) + LWLockAcquire(PARTITION_LOCK(hash_table, partidx), lockmode); + else if (!LWLockConditionalAcquire(PARTITION_LOCK(hash_table, partidx), + lockmode)) + return NULL; + ensure_valid_bucket_pointers(hash_table); /* Search the active bucket. */ item = find_in_bucket(hash_table, key, BUCKET_FOR_HASH(hash_table, hash)); if (item) - *found = true; + { + if (found) + *found = true; + } else { - *found = false; + if (found) + *found = false; + + if (!insert) + { + /* The caller didn't told to add a new entry. */ + LWLockRelease(PARTITION_LOCK(hash_table, partidx)); + return NULL; + } /* Check if we are getting too full. */ if (partition->count > MAX_COUNT_PER_PARTITION(hash_table)) @@ -479,7 +483,8 @@ restart: * Give up our existing lock first, because resizing needs to * reacquire all the locks in the right order to avoid deadlocks. */ - LWLockRelease(PARTITION_LOCK(hash_table, partition_index)); + LWLockRelease(PARTITION_LOCK(hash_table, partidx)); + resize(hash_table, hash_table->size_log2 + 1); goto restart; @@ -493,12 +498,13 @@ restart: ++partition->count; } - /* The caller must release the lock with dshash_release_lock. */ + /* The caller will free the lock by calling dshash_release_lock. */ hash_table->find_locked = true; - hash_table->find_exclusively_locked = true; + hash_table->find_exclusively_locked = exclusive; return ENTRY_FROM_ITEM(item); } + /* * Remove an entry by key. Returns true if the key was found and the * corresponding entry was removed. diff --git a/src/include/lib/dshash.h b/src/include/lib/dshash.h index a6ea377173..5b8114d041 100644 --- a/src/include/lib/dshash.h +++ b/src/include/lib/dshash.h @@ -91,6 +91,9 @@ extern void *dshash_find(dshash_table *hash_table, const void *key, bool exclusive); extern void *dshash_find_or_insert(dshash_table *hash_table, const void *key, bool *found); +extern void *dshash_find_extended(dshash_table *hash_table, const void *key, + bool exclusive, bool nowait, bool insert, + bool *found); extern bool dshash_delete_key(dshash_table *hash_table, const void *key); extern void dshash_delete_entry(dshash_table *hash_table, void *entry); extern void dshash_release_lock(dshash_table *hash_table, void *entry); -- 2.27.0 ----Next_Part(Fri_Jan__8_10_24_34_2021_185)-- Content-Type: Text/X-Patch; charset=us-ascii Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="v45-0003-Make-archiver-process-an-auxiliary-process.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pg_combinebackup does not detect missing files @ 2024-04-16 13:50 Robert Haas <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Robert Haas @ 2024-04-16 13:50 UTC (permalink / raw) To: David Steele <[email protected]>; +Cc: pgsql-hackers On Wed, Apr 10, 2024 at 9:36 PM David Steele <[email protected]> wrote: > I've been playing around with the incremental backup feature trying to > get a sense of how it can be practically used. One of the first things I > always try is to delete random files and see what happens. > > You can delete pretty much anything you want from the most recent > incremental backup (not the manifest) and it will not be detected. Sure, but you can also delete anything you want from the most recent non-incremental backup and it will also not be detected. There's no reason at all to hold incremental backup to a higher standard than we do in general. > Maybe the answer here is to update the docs to specify that > pg_verifybackup should be run on all backup directories before > pg_combinebackup is run. Right now that is not at all clear. I don't want to make those kinds of prescriptive statements. If you want to verify the backups that you use as input to pg_combinebackup, you can use pg_verifybackup to do that, but it's not a requirement. I'm not averse to having some kind of statement in the documentation along the lines of "Note that pg_combinebackup does not attempt to verify that the individual backups are intact; for that, use pg_verifybackup." But I think it should be blindingly obvious to everyone that you can't go whacking around the inputs to a program and expect to get perfectly good output. I know it isn't blindingly obvious to everyone, which is why I'm not averse to adding something like what I just mentioned, and maybe it wouldn't be a bad idea to document in a few other places that you shouldn't randomly remove files from the data directory of your live cluster, either, because people seem to keep doing it, but really, the expectation that you can't just blow files away and expect good things to happen afterward should hardly need to be stated. I think it's very easy to go overboard with warnings of this type. Weird stuff comes to me all the time because people call me when the weird stuff happens, and I'm guessing that your experience is similar. But my actual personal experience, as opposed to the cases reported to me by others, practically never features files evaporating into the ether. If I read a documentation page for PostgreSQL or any other piece of software that made it sound like that was a normal occurrence, I'd question the technical acumen of the authors. And if I saw such warnings only for one particular feature of a piece of software and not anywhere else, I'd wonder why the authors of the software were trying so hard to scare me off the use of that particular feature. I don't trust at all that incremental backup is free of bugs -- but I don't trust that all the code anyone else has written is free of bugs, either. > Overall I think it is an issue that the combine is being driven from the > most recent incremental directory rather than from the manifest. I don't. I considered that design and rejected it for various reasons that I still believe to be good. Even if I was wrong, we're not going to start rewriting the implementation a week after feature freeze. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pg_combinebackup does not detect missing files @ 2024-04-16 23:25 David Steele <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: David Steele @ 2024-04-16 23:25 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: pgsql-hackers On 4/16/24 23:50, Robert Haas wrote: > On Wed, Apr 10, 2024 at 9:36 PM David Steele <[email protected]> wrote: >> I've been playing around with the incremental backup feature trying to >> get a sense of how it can be practically used. One of the first things I >> always try is to delete random files and see what happens. >> >> You can delete pretty much anything you want from the most recent >> incremental backup (not the manifest) and it will not be detected. > > Sure, but you can also delete anything you want from the most recent > non-incremental backup and it will also not be detected. There's no > reason at all to hold incremental backup to a higher standard than we > do in general. Except that we are running pg_combinebackup on the incremental, which the user might reasonably expect to check backup integrity. It actually does a bunch of integrity checks -- but not this one. >> Maybe the answer here is to update the docs to specify that >> pg_verifybackup should be run on all backup directories before >> pg_combinebackup is run. Right now that is not at all clear. > > I don't want to make those kinds of prescriptive statements. If you > want to verify the backups that you use as input to pg_combinebackup, > you can use pg_verifybackup to do that, but it's not a requirement. > I'm not averse to having some kind of statement in the documentation > along the lines of "Note that pg_combinebackup does not attempt to > verify that the individual backups are intact; for that, use > pg_verifybackup." I think we should do this at a minimum. > But I think it should be blindingly obvious to > everyone that you can't go whacking around the inputs to a program and > expect to get perfectly good output. I know it isn't blindingly > obvious to everyone, which is why I'm not averse to adding something > like what I just mentioned, and maybe it wouldn't be a bad idea to > document in a few other places that you shouldn't randomly remove > files from the data directory of your live cluster, either, because > people seem to keep doing it, but really, the expectation that you > can't just blow files away and expect good things to happen afterward > should hardly need to be stated. And yet, we see it all the time. > I think it's very easy to go overboard with warnings of this type. > Weird stuff comes to me all the time because people call me when the > weird stuff happens, and I'm guessing that your experience is similar. > But my actual personal experience, as opposed to the cases reported to > me by others, practically never features files evaporating into the > ether. Same -- if it happens at all it is very rare. Virtually every time I am able to track down the cause of missing files it is because the user deleted them, usually to "save space" or because they "did not seem important". But given that this occurrence is pretty common in my experience, I think it is smart to mitigate against it, rather than just take it on faith that the user hasn't done anything destructive. Especially given how pg_combinebackup works, backups are going to undergo a lot of user manipulation (pushing to and pull from storage, decompressing, untaring, etc.) and I think that means we should take extra care. Regards, -David ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pg_combinebackup does not detect missing files @ 2024-04-17 15:03 Robert Haas <[email protected]> parent: David Steele <[email protected]> 0 siblings, 1 reply; 5+ messages in thread From: Robert Haas @ 2024-04-17 15:03 UTC (permalink / raw) To: David Steele <[email protected]>; +Cc: pgsql-hackers On Tue, Apr 16, 2024 at 7:25 PM David Steele <[email protected]> wrote: > Except that we are running pg_combinebackup on the incremental, which > the user might reasonably expect to check backup integrity. It actually > does a bunch of integrity checks -- but not this one. I think it's a bad idea to duplicate all of the checks from pg_verifybackup into pg_combinebackup. I did consider this exact issue during development. These are separate tools with separate purposes. I think that what a user should expect is that each tool has some job and tries to do that job well, while leaving other jobs to other tools. And I think if you think about it that way, it explains a lot about which checks pg_combinebackup does and which checks it does not do. pg_combinebackup tries to check that it is valid to combine backup A with backup B (and maybe C, D, E ...), and it checks a lot of stuff to try to make sure that such an operation appears to be sensible. Those are checks that pg_verifybackup cannot do, because pg_verifybackup only looks at one backup in isolation. If pg_combinebackup does not do those checks, nobody does. Aside from that, it will also report errors that it can't avoid noticing, even if those are things that pg_verifybackup would also have found, such as, say, the control file not existing. But it will not go out of its way to perform checks that are unrelated to its documented purpose. And I don't think it should, especially if we have another tool that already does that. > > I'm not averse to having some kind of statement in the documentation > > along the lines of "Note that pg_combinebackup does not attempt to > > verify that the individual backups are intact; for that, use > > pg_verifybackup." > > I think we should do this at a minimum. Here is a patch to do that. > Especially given how pg_combinebackup works, backups are going to > undergo a lot of user manipulation (pushing to and pull from storage, > decompressing, untaring, etc.) and I think that means we should take > extra care. We are in agreement on that point, if nothing else. I am terrified of users having problems with pg_combinebackup and me not being able to tell whether those problems are due to user error, Robert error, or something else. I put a lot of effort into detecting dumb things that I thought a user might do, and a lot of effort into debugging output so that when things do go wrong anyway, we have a reasonable chance of figuring out exactly where they went wrong. We do seem to have a philosophical difference about what the scope of those checks ought to be, and I don't really expect what I wrote above to convince you that my position is correct, but perhaps it will convince you that I have a thoughtful position, as opposed to just having done stuff at random. -- Robert Haas EDB: http://www.enterprisedb.com Attachments: [application/octet-stream] v1-0001-docs-Mention-that-pg_combinebackup-does-not-verif.patch (2.1K, ../../CA+TgmoZmuhkjGx=97kEniMRNC9Qaifjh0zMFZnHne72JGFNdKA@mail.gmail.com/2-v1-0001-docs-Mention-that-pg_combinebackup-does-not-verif.patch) download | inline diff: From 23ed43231e4d8a0a069c0d344e75e25b4969ab81 Mon Sep 17 00:00:00 2001 From: Robert Haas <[email protected]> Date: Wed, 17 Apr 2024 10:42:21 -0400 Subject: [PATCH v1] docs: Mention that pg_combinebackup does not verify backups. We don't want users to think that pg_combinebackup is trying to check the validity of individual backups, because it isn't. Adjust the wording about sanity checks to make it clear that verification of individual backups is the job of pg_verifybackup, and that the checks performed by pg_combinebackup are around the relationships between the backups. Per discussion with David Steele. Discussion: http://postgr.es/m/[email protected] --- doc/src/sgml/ref/pg_combinebackup.sgml | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/doc/src/sgml/ref/pg_combinebackup.sgml b/doc/src/sgml/ref/pg_combinebackup.sgml index 658e9a759c..c28012996b 100644 --- a/doc/src/sgml/ref/pg_combinebackup.sgml +++ b/doc/src/sgml/ref/pg_combinebackup.sgml @@ -45,12 +45,16 @@ PostgreSQL documentation </para> <para> - Although <application>pg_combinebackup</application> will attempt to verify + <application>pg_combinebackup</application> will attempt to verify that the backups you specify form a legal backup chain from which a correct - full backup can be reconstructed, it is not designed to help you keep track - of which backups depend on which other backups. If you remove the one or - more of the previous backups upon which your incremental - backup relies, you will not be able to restore it. + full backup can be reconstructed. However, it is not designed to help you + keep track of which backups depend on which other backups. If you remove + the one or more of the previous backups upon which your incremental + backup relies, you will not be able to restore it. Moreover, + <application>pg_basebackup</application> only attempts to verify that the + backups have the correct relationship to each other, not that each + individual backup is intact; for that, use + <xref linkend="app-pgverifybackup" />. </para> <para> -- 2.39.3 (Apple Git-145) ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pg_combinebackup does not detect missing files @ 2024-04-17 23:09 David Steele <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: David Steele @ 2024-04-17 23:09 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: pgsql-hackers On 4/18/24 01:03, Robert Haas wrote: > On Tue, Apr 16, 2024 at 7:25 PM David Steele <[email protected]> wrote: > > But it will not go out of its way to perform checks that are unrelated > to its documented purpose. And I don't think it should, especially if > we have another tool that already does that. > >>> I'm not averse to having some kind of statement in the documentation >>> along the lines of "Note that pg_combinebackup does not attempt to >>> verify that the individual backups are intact; for that, use >>> pg_verifybackup." >> >> I think we should do this at a minimum. > > Here is a patch to do that. I think here: + <application>pg_basebackup</application> only attempts to verify you mean: + <application>pg_combinebackup</application> only attempts to verify Otherwise this looks good to me. >> Especially given how pg_combinebackup works, backups are going to >> undergo a lot of user manipulation (pushing to and pull from storage, >> decompressing, untaring, etc.) and I think that means we should take >> extra care. > > We are in agreement on that point, if nothing else. I am terrified of > users having problems with pg_combinebackup and me not being able to > tell whether those problems are due to user error, Robert error, or > something else. I put a lot of effort into detecting dumb things that > I thought a user might do, and a lot of effort into debugging output > so that when things do go wrong anyway, we have a reasonable chance of > figuring out exactly where they went wrong. We do seem to have a > philosophical difference about what the scope of those checks ought to > be, and I don't really expect what I wrote above to convince you that > my position is correct, but perhaps it will convince you that I have a > thoughtful position, as opposed to just having done stuff at random. Fair enough. I accept that your reasoning is not random, but I'm still not very satisfied that the user needs to run a separate and rather expensive process to do the verification when pg_combinebackup already has the necessary information at hand. My guess is that most users will elect to skip verification. At least now they'll have the information they need to make an informed choice. Regards, -David ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2024-04-17 23:09 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-03-13 07:58 [PATCH v45 2/7] Add conditional lock feature to dshash Kyotaro Horiguchi <[email protected]> 2024-04-16 13:50 Re: pg_combinebackup does not detect missing files Robert Haas <[email protected]> 2024-04-16 23:25 ` Re: pg_combinebackup does not detect missing files David Steele <[email protected]> 2024-04-17 15:03 ` Re: pg_combinebackup does not detect missing files Robert Haas <[email protected]> 2024-04-17 23:09 ` Re: pg_combinebackup does not detect missing files David Steele <[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