public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 02/20] Fix broken error message on unseekable input..
4+ messages / 4 participants
[nested] [flat]
* [PATCH 02/20] Fix broken error message on unseekable input..
@ 2020-12-22 21:40 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Justin Pryzby @ 2020-12-22 21:40 UTC (permalink / raw)
pg_dump -Fd -f dir.dir
cat dir.dir/toc.dat | pg_restore -l # I realize this isn't how it's intended to be used
pg_restore: error: corrupt tar header found in PGDMP (expected 0, computed 18577) file position 18446744073709551615
See also 929c69aa19
---
src/bin/pg_dump/pg_backup_tar.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c
index 54e708875c..61c9c87a9f 100644
--- a/src/bin/pg_dump/pg_backup_tar.c
+++ b/src/bin/pg_dump/pg_backup_tar.c
@@ -1280,12 +1280,14 @@ _tarGetHeader(ArchiveHandle *AH, TAR_MEMBER *th)
if (chk != sum)
{
- char posbuf[32];
+ off_t off = ftello(ctx->tarFH);
- snprintf(posbuf, sizeof(posbuf), UINT64_FORMAT,
- (uint64) ftello(ctx->tarFH));
- fatal("corrupt tar header found in %s (expected %d, computed %d) file position %s",
- tag, sum, chk, posbuf);
+ if (off == -1)
+ fatal("corrupt tar header found in %s (expected %d, computed %d)",
+ tag, sum, chk);
+ else
+ fatal("corrupt tar header found in %s (expected %d, computed %d) file position " UINT64_FORMAT,
+ tag, sum, chk, off);
}
th->targetFile = pg_strdup(tag);
--
2.17.0
--GRPZ8SYKNexpdSJ7
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="0003-Support-multiple-compression-algs-levels-opts.patch"
^ permalink raw reply [nested|flat] 4+ messages in thread
* the s_lock_stuck on perform_spin_delay
@ 2024-01-04 06:59 Andy Fan <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Andy Fan @ 2024-01-04 06:59 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>; +Cc: Thomas Munro <[email protected]>
Hi,
from src/backend/storage/lmgr/README:
"""
Spinlocks. These are intended for *very* short-term locks. If a lock
is to be held more than a few dozen instructions, or across any sort of
kernel call (or even a call to a nontrivial subroutine), don't use a
spinlock. Spinlocks are primarily used as infrastructure for lightweight
locks.
"""
I totally agree with this and IIUC spin lock is usually used with the
following functions.
#define init_local_spin_delay(status) ..
void perform_spin_delay(SpinDelayStatus *status);
void finish_spin_delay(SpinDelayStatus *status);
During the perform_spin_delay, we have the following codes:
void
perform_spin_delay(SpinDelayStatus *status)
/* Block the process every spins_per_delay tries */
if (++(status->spins) >= spins_per_delay)
{
if (++(status->delays) > NUM_DELAYS)
s_lock_stuck(status->file, status->line, status->func);
the s_lock_stuck will PAINC the entire system.
My question is if someone doesn't obey the rule by mistake (everyone
can make mistake), shall we PANIC on a production environment? IMO I
think it can be a WARNING on a production environment and be a stuck
when 'ifdef USE_ASSERT_CHECKING'.
People may think spin lock may consume too much CPU, but it is not true
in the discussed scene since perform_spin_delay have pg_usleep in it,
and the MAX_DELAY_USEC is 1 second and MIN_DELAY_USEC is 0.001s.
I notice this issue actually because of the patch "Cache relation
sizes?" from Thomas Munro [1], where the latest patch[2] still have the
following code.
+ sr = smgr_alloc_sr(); <-- HERE a spin lock is hold
+
+ /* Upgrade to exclusive lock so we can create a mapping. */
+ LWLockAcquire(mapping_lock, LW_EXCLUSIVE); <-- HERE a complex
operation is needed. it may take a long time.
Our internal testing system found more misuses on our own PG version.
I think a experienced engineer like Thomas can make this mistake and the
patch was reviewed by 3 peoples, the bug is still there. It is not easy
to say just don't do it.
the attached code show the prototype in my mind. Any feedback is welcome.
[1]
https://www.postgresql.org/message-id/CA%2BhUKGJg%2BgqCs0dgo94L%3D1J9pDp5hKkotji9A05k2nhYQhF4%2Bw%40...
[2]
https://www.postgresql.org/message-id/attachment/123659/v5-0001-WIP-Track-relation-sizes-in-shared-m...
--
Best Regards
Andy Fan
Attachments:
[text/x-diff] v1-0001-Don-t-call-s_lock_stuck-in-production-environment.patch (1.7K, ../../[email protected]/2-v1-0001-Don-t-call-s_lock_stuck-in-production-environment.patch)
download | inline diff:
From 7d7fd0f0e9b13a290bfffaec0ad40773191155f2 Mon Sep 17 00:00:00 2001
From: "yizhi.fzh" <[email protected]>
Date: Thu, 4 Jan 2024 14:33:37 +0800
Subject: [PATCH v1 1/1] Don't call s_lock_stuck in production environment
In the current implementation, if a spin lock is misused, the
s_lock_stuck in perform_spin_delay can cause the entire system to
PANIC. In order to balance fault tolerance and the ability to detect
incorrect usage, we can use WARNING to replace s_lock_stuck in the
production environment, but still use s_lock_stuck in builds with
USE_ASSERT_CHECKING.
---
src/backend/storage/lmgr/s_lock.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/src/backend/storage/lmgr/s_lock.c b/src/backend/storage/lmgr/s_lock.c
index 327ac64f7c..9446605122 100644
--- a/src/backend/storage/lmgr/s_lock.c
+++ b/src/backend/storage/lmgr/s_lock.c
@@ -67,6 +67,7 @@ slock_t dummy_spinlock;
static int spins_per_delay = DEFAULT_SPINS_PER_DELAY;
+#ifdef USE_ASSERT_CHECKING
/*
* s_lock_stuck() - complain about a stuck spinlock
*/
@@ -85,6 +86,7 @@ s_lock_stuck(const char *file, int line, const char *func)
func, file, line);
#endif
}
+#endif
/*
* s_lock(lock) - platform-independent portion of waiting for a spinlock.
@@ -132,7 +134,14 @@ perform_spin_delay(SpinDelayStatus *status)
if (++(status->spins) >= spins_per_delay)
{
if (++(status->delays) > NUM_DELAYS)
+ {
+#ifdef USE_ASSERT_CHECKING
s_lock_stuck(status->file, status->line, status->func);
+#else
+ if (status->delays % NUM_DELAYS == 0)
+ elog(WARNING, "perform spin lock on %s %d times", status->func, status->delays);
+#endif
+ }
if (status->cur_delay == 0) /* first time to delay? */
status->cur_delay = MIN_DELAY_USEC;
--
2.34.1
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: the s_lock_stuck on perform_spin_delay
@ 2024-01-04 13:35 Robert Haas <[email protected]>
parent: Andy Fan <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Robert Haas @ 2024-01-04 13:35 UTC (permalink / raw)
To: Andy Fan <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]>
On Thu, Jan 4, 2024 at 2:09 AM Andy Fan <[email protected]> wrote:
> My question is if someone doesn't obey the rule by mistake (everyone
> can make mistake), shall we PANIC on a production environment? IMO I
> think it can be a WARNING on a production environment and be a stuck
> when 'ifdef USE_ASSERT_CHECKING'.
>
> People may think spin lock may consume too much CPU, but it is not true
> in the discussed scene since perform_spin_delay have pg_usleep in it,
> and the MAX_DELAY_USEC is 1 second and MIN_DELAY_USEC is 0.001s.
>
> I notice this issue actually because of the patch "Cache relation
> sizes?" from Thomas Munro [1], where the latest patch[2] still have the
> following code.
> + sr = smgr_alloc_sr(); <-- HERE a spin lock is hold
> +
> + /* Upgrade to exclusive lock so we can create a mapping. */
> + LWLockAcquire(mapping_lock, LW_EXCLUSIVE); <-- HERE a complex
> operation is needed. it may take a long time.
I'm not sure that the approach this patch takes is correct in detail,
but I kind of agree with you about the overall point. I mean, the idea
of the PANIC is to avoid having the system just sit there in a state
from which it will never recover ... but it can also have the effect
of killing a system that wasn't really dead. I'm not sure what the
best thing to do here is, but it's worth talking about, IMHO.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: the s_lock_stuck on perform_spin_delay
@ 2024-01-04 15:22 Tom Lane <[email protected]>
parent: Robert Haas <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Tom Lane @ 2024-01-04 15:22 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Andy Fan <[email protected]>; PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]>
Robert Haas <[email protected]> writes:
> I'm not sure that the approach this patch takes is correct in detail,
> but I kind of agree with you about the overall point. I mean, the idea
> of the PANIC is to avoid having the system just sit there in a state
> from which it will never recover ... but it can also have the effect
> of killing a system that wasn't really dead. I'm not sure what the
> best thing to do here is, but it's worth talking about, IMHO.
I'm not a fan of adding overhead to such a performance-critical
thing as spinlocks in order to catch coding errors that are easily
detectable statically. IMV the correct usage of spinlocks is that
they should only be held across *short, straight line* code segments.
We should be making an effort to ban coding patterns like
"return with spinlock still held", because they're just too prone
to errors similar to this one. Note that trying to take another
lock is far from the only bad thing that can happen if you're
not very conservative about what code can execute with a spinlock
held.
regards, tom lane
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-01-04 15:22 UTC | newest]
Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-12-22 21:40 [PATCH 02/20] Fix broken error message on unseekable input.. Justin Pryzby <[email protected]>
2024-01-04 06:59 the s_lock_stuck on perform_spin_delay Andy Fan <[email protected]>
2024-01-04 13:35 ` Re: the s_lock_stuck on perform_spin_delay Robert Haas <[email protected]>
2024-01-04 15:22 ` Re: the s_lock_stuck on perform_spin_delay Tom Lane <[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