pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedBug fix in vacuumdb --buffer-usage-limit xxx -Z
8+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2023-09-21 f7dbdab051 5cfba1ad69 Fix vacuumdb to pass buffer-usage-limit with analyze-only mode
* Bug fix in vacuumdb --buffer-usage-limit xxx -Z
@ 2023-09-21 01:44 Ryoga Yoshida <bt23yoshidar@oss.nttdata.com>
0 siblings, 2 replies; 8+ messages in thread
From: Ryoga Yoshida @ 2023-09-21 01:44 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
Hi,
When --buffer-usage-limit option is specified, vacuumdb issues VACUUM or
VACUUM ANALYZE command with BUFFER_USAGE_LIMIT option. Also if
--buffer-usage-limit and -Z options are specified, vacuumdb should issue
ANALYZE command with BUFFER_USAGE_LIMIT option. But it does not. That
is, vacuumdb -Z seems to fail to handle --buffer-usage-limit option.
This seems a bug.
You can see my patch in the attached file and how it works by adding -e
option in vacuumdb.
Ryoga Yoshida
Attachments:
v1-0001-bug-fix-in-vacuumdb-buffer-usage-limit-Z.patch (text/x-diff, 593B) download
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index f03d5b1c6c..57355639c0 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -964,6 +964,13 @@ prepare_vacuum_command(PQExpBuffer sql, int serverVersion,
appendPQExpBuffer(sql, "%sVERBOSE", sep);
sep = comma;
}
+ if (vacopts->buffer_usage_limit)
+ {
+ Assert(serverVersion >= 160000);
+ appendPQExpBuffer(sql, "%sBUFFER_USAGE_LIMIT '%s'", sep,
+ vacopts->buffer_usage_limit);
+ sep = comma;
+ }
if (sep != paren)
appendPQExpBufferChar(sql, ')');
}
* Re: Bug fix in vacuumdb --buffer-usage-limit xxx -Z
@ 2023-09-21 02:32 Michael Paquier <michael@paquier.xyz>
parent: Ryoga Yoshida <bt23yoshidar@oss.nttdata.com>
1 sibling, 0 replies; 8+ messages in thread
From: Michael Paquier @ 2023-09-21 02:32 UTC (permalink / raw)
To: Ryoga Yoshida <bt23yoshidar@oss.nttdata.com>; +Cc: pgsql-hackers@lists.postgresql.org
On Thu, Sep 21, 2023 at 10:44:49AM +0900, Ryoga Yoshida wrote:
> When --buffer-usage-limit option is specified, vacuumdb issues VACUUM or
> VACUUM ANALYZE command with BUFFER_USAGE_LIMIT option. Also if
> --buffer-usage-limit and -Z options are specified, vacuumdb should issue
> ANALYZE command with BUFFER_USAGE_LIMIT option. But it does not. That is,
> vacuumdb -Z seems to fail to handle --buffer-usage-limit option. This seems
> a bug.
>
> You can see my patch in the attached file and how it works by adding -e
> option in vacuumdb.
Good catch, indeed the option is missing from the ANALYZE commands
built under analyze_only. I can also notice that we have no tests for
this option in src/bin/scripts/t checking the shape of the commands
generated. Could you add something for ANALYZE and VACUUM? The
option could just be appended in one of the existing cases, for
instance.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Bug fix in vacuumdb --buffer-usage-limit xxx -Z
@ 2023-09-21 04:18 David Rowley <dgrowleyml@gmail.com>
parent: Ryoga Yoshida <bt23yoshidar@oss.nttdata.com>
1 sibling, 1 reply; 8+ messages in thread
From: David Rowley @ 2023-09-21 04:18 UTC (permalink / raw)
To: Ryoga Yoshida <bt23yoshidar@oss.nttdata.com>; +Cc: pgsql-hackers@lists.postgresql.org
On Thu, 21 Sept 2023 at 13:45, Ryoga Yoshida
<bt23yoshidar@oss.nttdata.com> wrote:
> When --buffer-usage-limit option is specified, vacuumdb issues VACUUM or
> VACUUM ANALYZE command with BUFFER_USAGE_LIMIT option. Also if
> --buffer-usage-limit and -Z options are specified, vacuumdb should issue
> ANALYZE command with BUFFER_USAGE_LIMIT option. But it does not. That
> is, vacuumdb -Z seems to fail to handle --buffer-usage-limit option.
> This seems a bug.
>
> You can see my patch in the attached file and how it works by adding -e
> option in vacuumdb.
Thanks for the report and the patch. I agree this has been overlooked.
I also wonder if we should be escaping the buffer-usage-limit string
sent in the comment. It seems quite an unlikely attack vector, as the
user would have command line access and could likely just use psql
anyway, but I had thought about something along the lines of:
$ vacuumdb --buffer-usage-limit "1MB'); drop database postgres;--" postgres
vacuumdb: vacuuming database "postgres"
vacuumdb: error: processing of database "postgres" failed: ERROR:
VACUUM cannot run inside a transaction block
seems that won't work, due to sending multiple commands at once, but I
think we should fix it anyway.
David
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Bug fix in vacuumdb --buffer-usage-limit xxx -Z
@ 2023-09-21 05:50 David Rowley <dgrowleyml@gmail.com>
parent: David Rowley <dgrowleyml@gmail.com>
0 siblings, 2 replies; 8+ messages in thread
From: David Rowley @ 2023-09-21 05:50 UTC (permalink / raw)
To: Ryoga Yoshida <bt23yoshidar@oss.nttdata.com>; +Cc: pgsql-hackers@lists.postgresql.org
On Thu, 21 Sept 2023 at 16:18, David Rowley <dgrowleyml@gmail.com> wrote:
> Thanks for the report and the patch. I agree this has been overlooked.
>
> I also wonder if we should be escaping the buffer-usage-limit string
> sent in the comment. It seems quite an unlikely attack vector, as the
> user would have command line access and could likely just use psql
> anyway, but I had thought about something along the lines of:
>
> $ vacuumdb --buffer-usage-limit "1MB'); drop database postgres;--" postgres
> vacuumdb: vacuuming database "postgres"
> vacuumdb: error: processing of database "postgres" failed: ERROR:
> VACUUM cannot run inside a transaction block
>
> seems that won't work, due to sending multiple commands at once, but I
> think we should fix it anyway.
I've pushed your patch plus some additional code to escape the text
specified in --buffer-usage-limit before passing it to the server in
commit 5cfba1ad6
Thanks again for the report.
David
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Bug fix in vacuumdb --buffer-usage-limit xxx -Z
@ 2023-09-21 05:59 Michael Paquier <michael@paquier.xyz>
parent: David Rowley <dgrowleyml@gmail.com>
1 sibling, 1 reply; 8+ messages in thread
From: Michael Paquier @ 2023-09-21 05:59 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Ryoga Yoshida <bt23yoshidar@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
On Thu, Sep 21, 2023 at 05:50:26PM +1200, David Rowley wrote:
> I've pushed your patch plus some additional code to escape the text
> specified in --buffer-usage-limit before passing it to the server in
> commit 5cfba1ad6
That was fast. If I may ask, why don't you have some regression tests
for the two code paths of vacuumdb that append this option to the
commands generated for VACUUM and ANALYZE?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Bug fix in vacuumdb --buffer-usage-limit xxx -Z
@ 2023-09-21 06:56 David Rowley <dgrowleyml@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 8+ messages in thread
From: David Rowley @ 2023-09-21 06:56 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Ryoga Yoshida <bt23yoshidar@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
On Thu, 21 Sept 2023 at 17:59, Michael Paquier <michael@paquier.xyz> wrote:
> That was fast. If I may ask, why don't you have some regression tests
> for the two code paths of vacuumdb that append this option to the
> commands generated for VACUUM and ANALYZE?
I think we have differing standards for what constitutes as a useful
test. For me, there would have to be a much higher likelihood of this
ever getting broken again.
I deem it pretty unlikely that someone will accidentally remove the
code that I just committed and a test to test that vacuumdb -Z
--buffer-usage-limit ... passes the BUFFER_USAGE_LIMIT option would
likely just forever mark that we once had a trivial bug that forgot to
include the --buffer-usage-limit when -Z was specified.
If others feel strongly that a test is worthwhile, then I'll reconsider.
David
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Bug fix in vacuumdb --buffer-usage-limit xxx -Z
@ 2023-09-21 08:06 Ryoga Yoshida <bt23yoshidar@oss.nttdata.com>
parent: David Rowley <dgrowleyml@gmail.com>
1 sibling, 0 replies; 8+ messages in thread
From: Ryoga Yoshida @ 2023-09-21 08:06 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org
On 2023-09-21 14:50, David Rowley wrote:
> I've pushed your patch plus some additional code to escape the text
> specified in --buffer-usage-limit before passing it to the server in
> commit 5cfba1ad6
>
> Thanks again for the report.
Thank you for the commit. I didn't notice about the escaping and it
seemed like it would be difficult for me to fix, so I appreciate your
help.
Ryoga Yoshida
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: Bug fix in vacuumdb --buffer-usage-limit xxx -Z
@ 2023-09-22 05:21 Michael Paquier <michael@paquier.xyz>
parent: David Rowley <dgrowleyml@gmail.com>
0 siblings, 0 replies; 8+ messages in thread
From: Michael Paquier @ 2023-09-22 05:21 UTC (permalink / raw)
To: David Rowley <dgrowleyml@gmail.com>; +Cc: Ryoga Yoshida <bt23yoshidar@oss.nttdata.com>; pgsql-hackers@lists.postgresql.org
On Thu, Sep 21, 2023 at 06:56:29PM +1200, David Rowley wrote:
> I deem it pretty unlikely that someone will accidentally remove the
> code that I just committed and a test to test that vacuumdb -Z
> --buffer-usage-limit ... passes the BUFFER_USAGE_LIMIT option would
> likely just forever mark that we once had a trivial bug that forgot to
> include the --buffer-usage-limit when -Z was specified.
Perhaps so.
> If others feel strongly that a test is worthwhile, then I'll reconsider.
I don't know if you would like that, but the addition is as simple as
the attached, FYI.
--
Michael
Attachments:
vacuumdb-tests.patch (text/x-diff, 1.3K) download
diff --git a/src/bin/scripts/t/100_vacuumdb.pl b/src/bin/scripts/t/100_vacuumdb.pl
index eccfcc54a1..597bbd8f54 100644
--- a/src/bin/scripts/t/100_vacuumdb.pl
+++ b/src/bin/scripts/t/100_vacuumdb.pl
@@ -40,6 +40,10 @@ $node->issues_sql_like(
[ 'vacuumdb', '--disable-page-skipping', 'postgres' ],
qr/statement: VACUUM \(DISABLE_PAGE_SKIPPING, SKIP_DATABASE_STATS\).*;/,
'vacuumdb --disable-page-skipping');
+$node->issues_sql_like(
+ [ 'vacuumdb', '--buffer-usage-limit', '0', 'postgres' ],
+ qr/statement: VACUUM \(SKIP_DATABASE_STATS, BUFFER_USAGE_LIMIT '0'\).*;/,
+ 'vacuumdb --buffer-usage-limit');
$node->issues_sql_like(
[ 'vacuumdb', '--skip-locked', 'postgres' ],
qr/statement: VACUUM \(SKIP_DATABASE_STATS, SKIP_LOCKED\).*;/,
@@ -48,6 +52,10 @@ $node->issues_sql_like(
[ 'vacuumdb', '--skip-locked', '--analyze-only', 'postgres' ],
qr/statement: ANALYZE \(SKIP_LOCKED\).*;/,
'vacuumdb --skip-locked --analyze-only');
+$node->issues_sql_like(
+ [ 'vacuumdb', '--buffer-usage-limit', '0', '--analyze-only', 'postgres' ],
+ qr/statement: ANALYZE \(BUFFER_USAGE_LIMIT '0'\).*;/,
+ 'vacuumdb --buffer-usage-limit --analyze-only');
$node->command_fails(
[ 'vacuumdb', '--analyze-only', '--disable-page-skipping', 'postgres' ],
'--analyze-only and --disable-page-skipping specified together');
end of thread, other threads:[~2023-09-22 05:21 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-09-21 01:44 Bug fix in vacuumdb --buffer-usage-limit xxx -Z Ryoga Yoshida <bt23yoshidar@oss.nttdata.com>
2023-09-21 02:32 ` Michael Paquier <michael@paquier.xyz>
2023-09-21 04:18 ` David Rowley <dgrowleyml@gmail.com>
2023-09-21 05:50 ` David Rowley <dgrowleyml@gmail.com>
2023-09-21 05:59 ` Michael Paquier <michael@paquier.xyz>
2023-09-21 06:56 ` David Rowley <dgrowleyml@gmail.com>
2023-09-22 05:21 ` Michael Paquier <michael@paquier.xyz>
2023-09-21 08:06 ` Ryoga Yoshida <bt23yoshidar@oss.nttdata.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