public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 03/18] pg_restore: "must be specified" and --list
8+ messages / 3 participants
[nested] [flat]
* [PATCH 03/18] pg_restore: "must be specified" and --list
@ 2020-12-06 04:43 Justin Pryzby <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Justin Pryzby @ 2020-12-06 04:43 UTC (permalink / raw)
This was discussed here, but the idea got lost.
https://www.postgresql.org/message-id/flat/20190612170201.GA11881%40alvherre.pgsql#2984347ab074e6f19...
---
src/bin/pg_dump/pg_restore.c | 2 +-
src/bin/pg_dump/t/001_basic.pl | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index 589b4aed53..f6e6e41329 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -305,7 +305,7 @@ main(int argc, char **argv)
/* Complain if neither -f nor -d was specified (except if dumping TOC) */
if (!opts->cparams.dbname && !opts->filename && !opts->tocSummary)
{
- pg_log_error("one of -d/--dbname and -f/--file must be specified");
+ pg_log_error("one of -d/--dbname, -f/--file, or -l/--list must be specified");
exit_nicely(1);
}
diff --git a/src/bin/pg_dump/t/001_basic.pl b/src/bin/pg_dump/t/001_basic.pl
index 083fb3ad08..8280914c2a 100644
--- a/src/bin/pg_dump/t/001_basic.pl
+++ b/src/bin/pg_dump/t/001_basic.pl
@@ -63,8 +63,8 @@ command_fails_like(
command_fails_like(
['pg_restore'],
- qr{\Qpg_restore: error: one of -d/--dbname and -f/--file must be specified\E},
- 'pg_restore: error: one of -d/--dbname and -f/--file must be specified');
+ qr{\Qpg_restore: error: one of -d/--dbname, -f/--file, or -l/--list must be specified\E},
+ 'pg_restore: error: one of -d/--dbname, -f/--file, or -l/--list must be specified');
command_fails_like(
[ 'pg_restore', '-s', '-a', '-f -' ],
--
2.17.0
--lc9FT7cWel8HagAv
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="0004-typos-in-master.patch"
^ permalink raw reply [nested|flat] 8+ messages in thread
* Doesn't pgstat_report_wal() handle the argument "force" incorrectly
@ 2023-09-22 04:58 Ryoga Yoshida <[email protected]>
2023-09-25 00:56 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Michael Paquier <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Ryoga Yoshida @ 2023-09-22 04:58 UTC (permalink / raw)
To: [email protected]
Hi,
pgstat_report_wal() calls pgstat_flush_wal() and pgstat_flush_io(). When
calling them, pgstat_report_wal() specifies its argument "force" as the
argument of them, as follows. But according to the code of
pgstat_flush_wal() and pgstat_flush_io(), their argument is "nowait" and
its meaning seems the opposite of "force". This means that, even when
checkpointer etc calls pgstat_report_wal() with force=true to forcibly
flush the statistics, pgstat_flush_wal() and pgstat_flush_io() skip
flushing the statistics if they fail to acquire the lock immediately
because they are called with nowait=true. This seems unexpected behavior
and a bug.
void
pgstat_report_wal(bool force)
{
pgstat_flush_wal(force);
pgstat_flush_io(force);
}
BTW, pgstat_report_stat() treats "nowait" and "force" as the opposite
one, as follows.
/* don't wait for lock acquisition when !force */
nowait = !force;
Ryoga Yoshida
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly
2023-09-22 04:58 Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[email protected]>
@ 2023-09-25 00:56 ` Michael Paquier <[email protected]>
2023-09-25 02:27 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2023-09-25 00:56 UTC (permalink / raw)
To: Ryoga Yoshida <[email protected]>; +Cc: [email protected]
On Fri, Sep 22, 2023 at 01:58:37PM +0900, Ryoga Yoshida wrote:
> pgstat_report_wal() calls pgstat_flush_wal() and pgstat_flush_io(). When
> calling them, pgstat_report_wal() specifies its argument "force" as the
> argument of them, as follows. But according to the code of
> pgstat_flush_wal() and pgstat_flush_io(), their argument is "nowait" and its
> meaning seems the opposite of "force". This means that, even when
> checkpointer etc calls pgstat_report_wal() with force=true to forcibly flush
> the statistics, pgstat_flush_wal() and pgstat_flush_io() skip flushing the
> statistics if they fail to acquire the lock immediately because they are
> called with nowait=true. This seems unexpected behavior and a bug.
It seems to me that you are right here. It would make sense to me to
say that force=true is equivalent to nowait=false, as in "I'm OK to
wait on the lockas I want to make sure that the stats are flushed at
this point". Currently force=true means nowait=true, as in "I'm OK to
not have the stats flushed if I cannot take the lock".
Seeing the three callers of pgstat_report_wal(), the checkpointer
wants to force its way twice, and the WAL writer does not care if they
are not flushed immediately at it loops forever in this path.
A comment at the top of pgstat_report_wal() would be nice to document
that a bit better, at least.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly
2023-09-22 04:58 Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[email protected]>
2023-09-25 00:56 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Michael Paquier <[email protected]>
@ 2023-09-25 02:27 ` Ryoga Yoshida <[email protected]>
2023-09-25 03:47 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Michael Paquier <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Ryoga Yoshida @ 2023-09-25 02:27 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: [email protected]
On 2023-09-25 09:56, Michael Paquier wrote:
> It seems to me that you are right here. It would make sense to me to
> say that force=true is equivalent to nowait=false, as in "I'm OK to
> wait on the lockas I want to make sure that the stats are flushed at
> this point". Currently force=true means nowait=true, as in "I'm OK to
> not have the stats flushed if I cannot take the lock".
>
> Seeing the three callers of pgstat_report_wal(), the checkpointer
> wants to force its way twice, and the WAL writer does not care if they
> are not flushed immediately at it loops forever in this path.
>
> A comment at the top of pgstat_report_wal() would be nice to document
> that a bit better, at least.
Thank you for the review. Certainly, adding a comments is a good idea. I
added a comment.
Ryoga Yoshida
Attachments:
[text/x-diff] v1-0002-bug-fix-in-foce-within-pgstat_report_wal.patch (804B, ../../[email protected]/2-v1-0002-bug-fix-in-foce-within-pgstat_report_wal.patch)
download | inline diff:
diff --git a/src/backend/utils/activity/pgstat_wal.c b/src/backend/utils/activity/pgstat_wal.c
index bcaed14d02..6f3bac5d0f 100644
--- a/src/backend/utils/activity/pgstat_wal.c
+++ b/src/backend/utils/activity/pgstat_wal.c
@@ -38,13 +38,18 @@ static WalUsage prevWalUsage;
*
* Must be called by processes that generate WAL, that do not call
* pgstat_report_stat(), like walwriter.
+ *
+ * force=false is the same as nowait=true, and the statistics will
+ * not be flushed if the lock cannot be acquired.
+ * force=true is the same as nowait=false, and waits until the lock
+ * is acquired and ensures that the statistics are flushed.
*/
void
pgstat_report_wal(bool force)
{
- pgstat_flush_wal(force);
+ pgstat_flush_wal(!force);
- pgstat_flush_io(force);
+ pgstat_flush_io(!force);
}
/*
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly
2023-09-22 04:58 Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[email protected]>
2023-09-25 00:56 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Michael Paquier <[email protected]>
2023-09-25 02:27 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[email protected]>
@ 2023-09-25 03:47 ` Michael Paquier <[email protected]>
2023-09-25 05:16 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2023-09-25 03:47 UTC (permalink / raw)
To: Ryoga Yoshida <[email protected]>; +Cc: [email protected]
On Mon, Sep 25, 2023 at 11:27:27AM +0900, Ryoga Yoshida wrote:
> Thank you for the review. Certainly, adding a comments is a good idea. I
> added a comment.
Hmm. How about the attached version with some tweaks?
--
Michael
Attachments:
[text/x-diff] v2-0002-bug-fix-in-foce-within-pgstat_report_wal.patch (861B, ../../[email protected]/2-v2-0002-bug-fix-in-foce-within-pgstat_report_wal.patch)
download | inline diff:
diff --git a/src/backend/utils/activity/pgstat_wal.c b/src/backend/utils/activity/pgstat_wal.c
index bcaed14d02..82feb792cf 100644
--- a/src/backend/utils/activity/pgstat_wal.c
+++ b/src/backend/utils/activity/pgstat_wal.c
@@ -38,13 +38,18 @@ static WalUsage prevWalUsage;
*
* Must be called by processes that generate WAL, that do not call
* pgstat_report_stat(), like walwriter.
+ *
+ * force set to true ensures that the statistics are flushed; note that
+ * this needs to acquire the pgstat shmem LWLock, waiting on it. When
+ * set to false, the statistics may not be flushed if the lock could not
+ * be acquired.
*/
void
pgstat_report_wal(bool force)
{
- pgstat_flush_wal(force);
-
- pgstat_flush_io(force);
+ /* like in pgstat.c, don't wait for lock acquisition when !force */
+ pgstat_flush_wal(!force);
+ pgstat_flush_io(!force);
}
/*
[application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc)
download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly
2023-09-22 04:58 Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[email protected]>
2023-09-25 00:56 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Michael Paquier <[email protected]>
2023-09-25 02:27 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[email protected]>
2023-09-25 03:47 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Michael Paquier <[email protected]>
@ 2023-09-25 05:16 ` Ryoga Yoshida <[email protected]>
2023-09-25 05:38 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Michael Paquier <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Ryoga Yoshida @ 2023-09-25 05:16 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: [email protected]
On 2023-09-25 12:47, Michael Paquier wrote:
in attached file
> + /* like in pgstat.c, don't wait for lock acquisition when !force */
Isn't it the case with force=true and !force that it doesn't wait for
the lock acquisition. In fact, force may be false.
Ryoga Yoshida
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly
2023-09-22 04:58 Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[email protected]>
2023-09-25 00:56 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Michael Paquier <[email protected]>
2023-09-25 02:27 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[email protected]>
2023-09-25 03:47 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Michael Paquier <[email protected]>
2023-09-25 05:16 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[email protected]>
@ 2023-09-25 05:38 ` Michael Paquier <[email protected]>
2023-09-25 05:49 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2023-09-25 05:38 UTC (permalink / raw)
To: Ryoga Yoshida <[email protected]>; +Cc: [email protected]
On Mon, Sep 25, 2023 at 02:16:22PM +0900, Ryoga Yoshida wrote:
> On 2023-09-25 12:47, Michael Paquier wrote:
> in attached file
>> + /* like in pgstat.c, don't wait for lock acquisition when !force */
>
> Isn't it the case with force=true and !force that it doesn't wait for the
> lock acquisition. In fact, force may be false.
We would not wait on the lock if force=false, which would do
nowait=true. And !force reads the same to me as force=false.
Anyway, I am OK to remove this part. That seems to confuse you, so
you may not be the only one who would read this comment.
Another idea would be to do like in pgstat.c by adding the following
line, then use "nowait" to call each sub-function:
nowait = !force;
pgstat_flush_wal(nowait);
pgstat_flush_io(nowait);
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly
2023-09-22 04:58 Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[email protected]>
2023-09-25 00:56 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Michael Paquier <[email protected]>
2023-09-25 02:27 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[email protected]>
2023-09-25 03:47 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Michael Paquier <[email protected]>
2023-09-25 05:16 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[email protected]>
2023-09-25 05:38 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Michael Paquier <[email protected]>
@ 2023-09-25 05:49 ` Ryoga Yoshida <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Ryoga Yoshida @ 2023-09-25 05:49 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: [email protected]
On 2023-09-25 14:38, Michael Paquier wrote:
> We would not wait on the lock if force=false, which would do
> nowait=true. And !force reads the same to me as force=false.
>
> Anyway, I am OK to remove this part. That seems to confuse you, so
> you may not be the only one who would read this comment.
When I first read it, I didn't read that !force as force=false, so
removing it might be better.
> Another idea would be to do like in pgstat.c by adding the following
> line, then use "nowait" to call each sub-function:
> nowait = !force;
> pgstat_flush_wal(nowait);
> pgstat_flush_io(nowait);
That's very clear and I think it's good.
Ryoga Yoshida
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2023-09-25 05:49 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-12-06 04:43 [PATCH 03/18] pg_restore: "must be specified" and --list Justin Pryzby <[email protected]>
2023-09-22 04:58 Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[email protected]>
2023-09-25 00:56 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Michael Paquier <[email protected]>
2023-09-25 02:27 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[email protected]>
2023-09-25 03:47 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Michael Paquier <[email protected]>
2023-09-25 05:16 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[email protected]>
2023-09-25 05:38 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Michael Paquier <[email protected]>
2023-09-25 05:49 ` Re: Doesn't pgstat_report_wal() handle the argument "force" incorrectly Ryoga Yoshida <[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