[PATCH] pgbench tap tests fail if the path contains a perl special character

=?UTF-8?B?UmHDumwgTWFyw61uIFJvZHLDrWd1ZXo=?= <rmrodriguez@carto.com> writes:

As mentioned in the subject, if the $PATH where you are building postgres
contains a perl's special character [1], for example a `+`, pgbench's tap
tests will fail.

Fun. But is that really the only place that fails?

I'm attaching a patch to fix this by using quotemeta [3].

This does not look right to me: glob has a different set of special
characters than regexes, no?

regards, tom lane

Hi,

Fun. But is that really the only place that fails?

Yes, other than this it builds flawlessly.

This does not look right to me: glob has a different set of special
characters than regexes, no?

The issue itself manifests in the grep call, not in glob, but I tried
using `quotemeta` only for grep without success (probably because I didn't
know how to properly call `quotemeta` over @logs.

This is the first time I do anything in perl beyond adding some simple tap
tests, so this could be me not knowing how to deal with it properly. I used
this small script to understand perl's behaviour:

```
$ cat a.pl
#!/usr/bin/perl
use strict;
use warnings;

my @prefix = '~/postgres+XXXX/001_pgbench_log_2.20745';
my @quoted_prefix = quotemeta(@prefix);

my @found1 = grep(/^@prefix\.\d+(\.\d+)?$/, "@prefix.20745");
my @found2 = grep(/^@prefix\.\d+(\.\d+)?$/, "@quoted_prefix.20745");
my @found3 = grep(/^@quoted_prefix\.\d+(\.\d+)?$/, "@prefix.20745");
my @found4 = grep(/^@quoted_prefix\.\d+(\.\d+)?$/, "@quoted_prefix.20745");
print "1: @found1\n";
print "2: @found2\n";
print "3: @found3\n";
print "4: @found4\n";

$ perl a.pl
1:
2:
3:
4: 1.20745
```

Regards,
--
Raúl Marín Rodríguez
carto.com

On 2019-Jan-18, Raúl Marín Rodríguez wrote:

Hi,

Fun. But is that really the only place that fails?

Yes, other than this it builds flawlessly.

This does not look right to me: glob has a different set of special
characters than regexes, no?

The issue itself manifests in the grep call, not in glob, but I tried
using `quotemeta` only for grep without success (probably because I didn't
know how to properly call `quotemeta` over @logs.

Perhaps you can do
@quoted = map { quotemeta($_) } @logs;

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

This does not look right to me: glob has a different set of special
characters than regexes, no?

Perhaps you can do
@quoted = map { quotemeta($_) } @logs;

Actually, playing with it here, it seems that quotemeta() quotes
anything that's a special character for either purpose (in fact,
it looks like it quotes anything that's not alphanumeric).
So the patch as given should work.

regards, tom lane

I wrote:

So the patch as given should work.

... well, modulo the use of "@foo" where you should've used "$foo".
I don't know enough Perl to understand why that seemed to work, but
I do know it's not the right thing here.

Pushed with that correction.

regards, tom lane

Pushed with that correction.

Thanks a lot!

--
Raúl Marín Rodríguez
carto.com

=?UTF-8?B?UmHDumwgTWFyw61uIFJvZHLDrWd1ZXo=?= <rmrodriguez@carto.com> writes:

Pushed with that correction.

Thanks a lot!

Hm, so bowerbird (a Windows machine) has been failing the pgbench tests
since this went in, cf

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&amp;dt=2019-01-20%2004%3A57%3A01

I'm just guessing, but I suspect that bowerbird is using a path spec that
includes a backslash directory separator and that's somehow bollixing
things. If so, we might be able to fix it by converting backslashes to
forward slashes before applying quotemeta().

It's also possible that on Windows, "glob" handles backslashes
differently than it does elsewhere, which would be harder to fix
--- that would bring back my original fear that the appropriate
quoting rules are different for "glob" than for regexes.

Andrew, any insight?

regards, tom lane

Hello Tom,

Hm, so bowerbird (a Windows machine) has been failing the pgbench tests
since this went in, cf

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&amp;dt=2019-01-20%2004%3A57%3A01

I'm just guessing, but I suspect that bowerbird is using a path spec that
includes a backslash directory separator and that's somehow bollixing
things.

This point is unclear from the log, where plain slashes are used in the
log prefix path, and furthermore no strange characters appear in the
log prefix path:

# Running: pgbench -n -S -t 50 -c 2 --log --log-prefix=G:/prog/bf/root/HEAD/pgsql.build/src/bin/pgbench/tmp_check/t_001_pgbench_with_server_main_data/001_pgbench_log_2 --sampling-rate=0.5
ok 345 - pgbench logs status (got 0 vs expected 0)
ok 346 - pgbench logs stdout /(?^:select only)/
ok 347 - pgbench logs stdout /(?^:processed: 100/100)/
ok 348 - pgbench logs stderr /(?^:^$)/
not ok 349 - number of log files

If so, we might be able to fix it by converting backslashes to
forward slashes before applying quotemeta().

It's also possible that on Windows, "glob" handles backslashes
differently than it does elsewhere, which would be harder to fix
--- that would bring back my original fear that the appropriate
quoting rules are different for "glob" than for regexes.

I'm afraid it could indeed be due to platform-specific behavior, so
testing on the target machine to understand the actual behavior would
help.

I just tested the current version on my laptop within a directory
containing spaces and other misc chars: Pgbench TAP tests are currently
broken in this context on master, and this may be used to a collection of
issues, not just one, eg pgbench function splits parameters on spaces
which breaks if there are spaces in bdir.

I'm going to investigate, possibly over next week-end, so please be
patient.

About windows-specific issues, from File::Glob man page:

""" On DOSISH systems, backslash is a valid directory separator character.
In this case, use of backslash as a quoting character (via GLOB_QUOTE)
interferes with the use of backslash as a directory separator. ... """

It seems to suggest that quotemeta backslash-on-nearly-everything approach
is not appropriate.

--
Fabien.

On 1/21/19 4:50 AM, Fabien COELHO wrote:

Hello Tom,

Hm, so bowerbird (a Windows machine) has been failing the pgbench tests
since this went in, cf

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird&amp;dt=2019-01-20%2004%3A57%3A01

I'm just guessing, but I suspect that bowerbird is using a path spec
that
includes a backslash directory separator and that's somehow bollixing
things.

This point is unclear from the log, where plain slashes are used in
the log prefix path, and furthermore no strange characters appear in
the log prefix path:

 # Running: pgbench -n -S -t 50 -c 2 --log
--log-prefix=G:/prog/bf/root/HEAD/pgsql.build/src/bin/pgbench/tmp_check/t_001_pgbench_with_server_main_data/001_pgbench_log_2
--sampling-rate=0.5
 ok 345 - pgbench logs status (got 0 vs expected 0)
 ok 346 - pgbench logs stdout /(?^:select only)/
 ok 347 - pgbench logs stdout /(?^:processed: 100/100)/
 ok 348 - pgbench logs stderr /(?^:^$)/
 not ok 349 - number of log files

If so, we might be able to fix it by converting backslashes to
forward slashes before applying quotemeta().

It's also possible that on Windows, "glob" handles backslashes
differently than it does elsewhere, which would be harder to fix
--- that would bring back my original fear that the appropriate
quoting rules are different for "glob" than for regexes.

I'm afraid it could indeed be due to platform-specific behavior, so
testing on the target machine to understand the actual behavior would
help.

I just tested the current version on my laptop within a directory
containing spaces and other misc chars: Pgbench TAP tests are
currently broken in this context on master, and this may be used to a
collection of issues, not just one, eg pgbench function splits
parameters on spaces which breaks if there are spaces in bdir.

I'm going to investigate, possibly over next week-end, so please be
patient.

About windows-specific issues, from File::Glob man page:

""" On DOSISH systems, backslash is a valid directory separator
character. In this case, use of backslash as a quoting character (via
GLOB_QUOTE) interferes with the use of backslash as a directory
separator. ... """

It seems to suggest that quotemeta backslash-on-nearly-everything
approach is not appropriate.

File globbing is not the same thing as regex matching. For example, '.'
is a literal character in a glob pattern but a metacharacter in a regex,
while ' ' is a literal character in a regex but a globbing metacharacter
(but see below), and '*' is a metacharacter in both but has different
meanings. quotemeta is intended for regexes but being used here on an
expression used for globbing.

Perhaps it would be OK we changed back the glob line to use $prefix
instead of $qprefix, but kept the use of $qprefix in the later regex.

To deal with the issue of spaces in file names (not an issue eher ob
bowerbird), we should consider adding this:

    use File::Glob ':bsd_glob';

This removes the globbing metcharacter nature of the space character,
although it might have other effects that cause pain. See `perldoc
File::Glob`

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

Perhaps it would be OK we changed back the glob line to use $prefix
instead of $qprefix, but kept the use of $qprefix in the later regex.

Well, glob does have some metacharacters, so not doing any quoting
at all still leaves us with failure modes.

I did a little bit of googling on this subject last night, and it
seems that at least some people believe that the answer is to not
use glob, period, but read the directory for yourself. Maybe there's
a better way, but the breakage around backslashes doesn't really
leave me hopeful.

As a short-term move to un-break the buildfarm, I'm just going to
revert that patch altogether. We can reapply it once we've
figured out how to do the glob part correctly.

regards, tom lane

Hello Andrew,

About windows-specific issues, from File::Glob man page:

""" On DOSISH systems, backslash is a valid directory separator
character. In this case, use of backslash as a quoting character (via
GLOB_QUOTE) interferes with the use of backslash as a directory
separator. ... """

It seems to suggest that quotemeta backslash-on-nearly-everything
approach is not appropriate.

File globbing is not the same thing as regex matching. For example, '.'
is a literal character in a glob pattern but a metacharacter in a regex,
while ' ' is a literal character in a regex but a globbing metacharacter
(but see below), and '*' is a metacharacter in both but has different
meanings. quotemeta is intended for regexes but being used here on an
expression used for globbing.

Perhaps it would be OK we changed back the glob line to use $prefix
instead of $qprefix, but kept the use of $qprefix in the later regex.

Yep, possibly. I'll have to test, though.

To deal with the issue of spaces in file names (not an issue eher ob
bowerbird), we should consider adding this:

    use File::Glob ':bsd_glob';

I was planning that so that the behavior is the same on all systems.

This removes the globbing metcharacter nature of the space character,
although it might have other effects that cause pain. See `perldoc
File::Glob`

Yep. Thanks for doing my homework:-) I'll test starting with these
options.

--
Fabien.

Hello Tom,

Well, glob does have some metacharacters, so not doing any quoting
at all still leaves us with failure modes.

I did a little bit of googling on this subject last night, and it
seems that at least some people believe that the answer is to not
use glob, period, but read the directory for yourself.

Yep, would work, it can then be filtered with standard regexpr, although
it will take a few lines (opendir/grep on readdir/closedir).

Maybe there's a better way, but the breakage around backslashes doesn't
really leave me hopeful.

Indeed. I was thinking of defining my own quote function, but without
being convinced that it is such a good idea.

As a short-term move to un-break the buildfarm, I'm just going to
revert that patch altogether.

I was considering to suggest that, so I'm ok with that.

We can reapply it once we've figured out how to do the glob part
correctly.

Yep. No need to spend time much on that, I'll have a look at it, although
not right now, allow me a few days.

--
Fabien.

On 1/21/19 11:18 AM, Fabien COELHO wrote:

Hello Tom,

Well, glob does have some metacharacters, so not doing any quoting
at all still leaves us with failure modes.

I did a little bit of googling on this subject last night, and it
seems that at least some people believe that the answer is to not
use glob, period, but read the directory for yourself.

Yep, would work, it can then be filtered with standard regexpr,
although it will take a few lines (opendir/grep on readdir/closedir).

Sure, probably the best solution. Given that Perl has
opendir/readdir/closedir, it should be only a handful of lines.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello Tom,

I did a little bit of googling on this subject last night, and it
seems that at least some people believe that the answer is to not
use glob, period, but read the directory for yourself.

As a short-term move to un-break the buildfarm, I'm just going to
revert that patch altogether.

We can reapply it once we've figured out how to do the glob part correctly.

Here is a proposal patch which:

- works around pgbench command splitting on spaces,
if postgres sources are in a strangely named directory…
I tested within a directory named "pg .* dir".

- works aroung "glob" lack of portability by reading the directory
directly and filtering with a standard re (see list_files).

- adds a few comments

- removes a spurious i option on an empty re

--
Fabien.

Hi,

- works around pgbench command splitting on spaces,
if postgres sources are in a strangely named directory…
I tested within a directory named "pg .* dir".

I've also tested it with the initial case (including a +) and it works.

--
Raúl Marín Rodríguez
carto.com

- works around pgbench command splitting on spaces,
if postgres sources are in a strangely named directory…
I tested within a directory named "pg .* dir".

I've also tested it with the initial case (including a +) and it works.

Good, thanks for checking!

--
Fabien.

Hi,

Any way I can help to get this commited? Should we add this to the commitfest?

Regards,
--
Raúl Marín Rodríguez
carto.com

On 8 Feb 2019, at 13:33, Raúl Marín Rodríguez <rmrodriguez@carto.com> wrote:

Any way I can help to get this commited? Should we add this to the commitfest?

Please do, it’s a good way to ensure that the patch is tracked and not
forgotten about.

cheers ./daniel

The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

Tested `pgbench-tap-glob-fix-1.patch` with a "+" in the path and the tap tests now pass.

On 2/8/19 3:30 PM, Raúl Marín wrote:

The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: not tested
Spec compliant: not tested
Documentation: not tested

Tested `pgbench-tap-glob-fix-1.patch` with a "+" in the path and the tap tests now pass.

Is this now ready for committer or is there anything else Fabien needs
to do?

Marking the patch as either Ready for Committer or Waiting on Author
will help move things along.

Regards,
--
-David
david@pgmasters.net

Marking the patch as either Ready for Committer or Waiting on Author

will help move things along.

Thanks for the heads-up, I've retested it again and it applies cleanly on
top of master and addresses the issue.

I've moved it to `Ready for Committer`.

--
Raúl Marín Rodríguez
carto.com