Обсуждение: 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

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