Bug fix in vacuumdb --buffer-usage-limit xxx -Z

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 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

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 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

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