Bug fix in vacuumdb --buffer-usage-limit xxx -Z
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.patchtext/x-diff; name=v1-0001-bug-fix-in-vacuumdb-buffer-usage-limit-Z.patchDownload
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, ')');
}
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
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
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 blockseems 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
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
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
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 5cfba1ad6Thanks 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
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.patchtext/x-diff; charset=us-asciiDownload
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');