Tighten up a few overly lax regexes in pg_dump's tap tests
There are a few regexes in pg_dump's tap tests that are neglecting to
escape the dot in "schema.table" type expressions. This could result
in the test passing when it shouldn't. It seems worth putting that
right.
Patch attached.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
tighten_up_pg_dump_tap_tests.patchapplication/octet-stream; name=tighten_up_pg_dump_tap_tests.patchDownload
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 245fcbf5ce..7a66ea88d0 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -448,7 +448,7 @@ my %tests = (
},
'ALTER COLLATION test0 OWNER TO' => {
- regexp => qr/^ALTER COLLATION public.test0 OWNER TO .*;/m,
+ regexp => qr/^ALTER COLLATION public\.test0 OWNER TO .*;/m,
collation => 1,
like => { %full_runs, section_pre_data => 1, },
unlike => { %dump_test_schema_runs, no_owner => 1, },
@@ -757,7 +757,7 @@ my %tests = (
},
'ALTER TABLE test_table OWNER TO' => {
- regexp => qr/^ALTER TABLE dump_test.test_table OWNER TO .*;/m,
+ regexp => qr/^ALTER TABLE dump_test\.test_table OWNER TO .*;/m,
like => {
%full_runs,
%dump_test_schema_runs,
@@ -776,7 +776,7 @@ my %tests = (
create_sql => 'ALTER TABLE dump_test.test_table
ENABLE ROW LEVEL SECURITY;',
regexp =>
- qr/^ALTER TABLE dump_test.test_table ENABLE ROW LEVEL SECURITY;/m,
+ qr/^ALTER TABLE dump_test\.test_table ENABLE ROW LEVEL SECURITY;/m,
like => {
%full_runs,
%dump_test_schema_runs,
@@ -790,7 +790,7 @@ my %tests = (
},
'ALTER TABLE test_second_table OWNER TO' => {
- regexp => qr/^ALTER TABLE dump_test.test_second_table OWNER TO .*;/m,
+ regexp => qr/^ALTER TABLE dump_test\.test_second_table OWNER TO .*;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -800,7 +800,7 @@ my %tests = (
},
'ALTER TABLE measurement OWNER TO' => {
- regexp => qr/^ALTER TABLE dump_test.measurement OWNER TO .*;/m,
+ regexp => qr/^ALTER TABLE dump_test\.measurement OWNER TO .*;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -811,7 +811,7 @@ my %tests = (
'ALTER TABLE measurement_y2006m2 OWNER TO' => {
regexp =>
- qr/^ALTER TABLE dump_test_second_schema.measurement_y2006m2 OWNER TO .*;/m,
+ qr/^ALTER TABLE dump_test_second_schema\.measurement_y2006m2 OWNER TO .*;/m,
like => {
%full_runs,
role => 1,
@@ -822,7 +822,7 @@ my %tests = (
'ALTER FOREIGN TABLE foreign_table OWNER TO' => {
regexp =>
- qr/^ALTER FOREIGN TABLE dump_test.foreign_table OWNER TO .*;/m,
+ qr/^ALTER FOREIGN TABLE dump_test\.foreign_table OWNER TO .*;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -833,7 +833,7 @@ my %tests = (
'ALTER TEXT SEARCH CONFIGURATION alt_ts_conf1 OWNER TO' => {
regexp =>
- qr/^ALTER TEXT SEARCH CONFIGURATION dump_test.alt_ts_conf1 OWNER TO .*;/m,
+ qr/^ALTER TEXT SEARCH CONFIGURATION dump_test\.alt_ts_conf1 OWNER TO .*;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -844,7 +844,7 @@ my %tests = (
'ALTER TEXT SEARCH DICTIONARY alt_ts_dict1 OWNER TO' => {
regexp =>
- qr/^ALTER TEXT SEARCH DICTIONARY dump_test.alt_ts_dict1 OWNER TO .*;/m,
+ qr/^ALTER TEXT SEARCH DICTIONARY dump_test\.alt_ts_dict1 OWNER TO .*;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -913,7 +913,7 @@ my %tests = (
create_sql => 'COMMENT ON TABLE dump_test.test_table
IS \'comment on table\';',
regexp =>
- qr/^COMMENT ON TABLE dump_test.test_table IS 'comment on table';/m,
+ qr/^COMMENT ON TABLE dump_test\.test_table IS 'comment on table';/m,
like => {
%full_runs,
%dump_test_schema_runs,
@@ -986,7 +986,7 @@ my %tests = (
create_sql => 'COMMENT ON CONVERSION dump_test.test_conversion
IS \'comment on test conversion\';',
regexp =>
- qr/^COMMENT ON CONVERSION dump_test.test_conversion IS 'comment on test conversion';/m,
+ qr/^COMMENT ON CONVERSION dump_test\.test_conversion IS 'comment on test conversion';/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -997,7 +997,7 @@ my %tests = (
create_sql => 'COMMENT ON COLLATION test0
IS \'comment on test0 collation\';',
regexp =>
- qr/^COMMENT ON COLLATION public.test0 IS 'comment on test0 collation';/m,
+ qr/^COMMENT ON COLLATION public\.test0 IS 'comment on test0 collation';/m,
collation => 1,
like => { %full_runs, section_pre_data => 1, },
},
@@ -1051,7 +1051,7 @@ my %tests = (
'COMMENT ON TEXT SEARCH CONFIGURATION dump_test.alt_ts_conf1
IS \'comment on text search configuration\';',
regexp =>
- qr/^COMMENT ON TEXT SEARCH CONFIGURATION dump_test.alt_ts_conf1 IS 'comment on text search configuration';/m,
+ qr/^COMMENT ON TEXT SEARCH CONFIGURATION dump_test\.alt_ts_conf1 IS 'comment on text search configuration';/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -1063,7 +1063,7 @@ my %tests = (
'COMMENT ON TEXT SEARCH DICTIONARY dump_test.alt_ts_dict1
IS \'comment on text search dictionary\';',
regexp =>
- qr/^COMMENT ON TEXT SEARCH DICTIONARY dump_test.alt_ts_dict1 IS 'comment on text search dictionary';/m,
+ qr/^COMMENT ON TEXT SEARCH DICTIONARY dump_test\.alt_ts_dict1 IS 'comment on text search dictionary';/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -1074,7 +1074,7 @@ my %tests = (
create_sql => 'COMMENT ON TEXT SEARCH PARSER dump_test.alt_ts_prs1
IS \'comment on text search parser\';',
regexp =>
- qr/^COMMENT ON TEXT SEARCH PARSER dump_test.alt_ts_prs1 IS 'comment on text search parser';/m,
+ qr/^COMMENT ON TEXT SEARCH PARSER dump_test\.alt_ts_prs1 IS 'comment on text search parser';/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -1085,7 +1085,7 @@ my %tests = (
create_sql => 'COMMENT ON TEXT SEARCH TEMPLATE dump_test.alt_ts_temp1
IS \'comment on text search template\';',
regexp =>
- qr/^COMMENT ON TEXT SEARCH TEMPLATE dump_test.alt_ts_temp1 IS 'comment on text search template';/m,
+ qr/^COMMENT ON TEXT SEARCH TEMPLATE dump_test\.alt_ts_temp1 IS 'comment on text search template';/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -1096,7 +1096,7 @@ my %tests = (
create_sql => 'COMMENT ON TYPE dump_test.planets
IS \'comment on enum type\';',
regexp =>
- qr/^COMMENT ON TYPE dump_test.planets IS 'comment on enum type';/m,
+ qr/^COMMENT ON TYPE dump_test\.planets IS 'comment on enum type';/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -1107,7 +1107,7 @@ my %tests = (
create_sql => 'COMMENT ON TYPE dump_test.textrange
IS \'comment on range type\';',
regexp =>
- qr/^COMMENT ON TYPE dump_test.textrange IS 'comment on range type';/m,
+ qr/^COMMENT ON TYPE dump_test\.textrange IS 'comment on range type';/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -1118,7 +1118,7 @@ my %tests = (
create_sql => 'COMMENT ON TYPE dump_test.int42
IS \'comment on regular type\';',
regexp =>
- qr/^COMMENT ON TYPE dump_test.int42 IS 'comment on regular type';/m,
+ qr/^COMMENT ON TYPE dump_test\.int42 IS 'comment on regular type';/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -1129,7 +1129,7 @@ my %tests = (
create_sql => 'COMMENT ON TYPE dump_test.undefined
IS \'comment on undefined type\';',
regexp =>
- qr/^COMMENT ON TYPE dump_test.undefined IS 'comment on undefined type';/m,
+ qr/^COMMENT ON TYPE dump_test\.undefined IS 'comment on undefined type';/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -1282,14 +1282,14 @@ my %tests = (
'INSERT INTO test_table' => {
regexp => qr/^
- (?:INSERT\ INTO\ dump_test.test_table\ \(col1,\ col2,\ col3,\ col4\)\ VALUES\ \(\d,\ NULL,\ NULL,\ NULL\);\n){9}
+ (?:INSERT\ INTO\ dump_test\.test_table\ \(col1,\ col2,\ col3,\ col4\)\ VALUES\ \(\d,\ NULL,\ NULL,\ NULL\);\n){9}
/xm,
like => { column_inserts => 1, },
},
'INSERT INTO test_second_table' => {
regexp => qr/^
- (?:INSERT\ INTO\ dump_test.test_second_table\ \(col1,\ col2\)
+ (?:INSERT\ INTO\ dump_test\.test_second_table\ \(col1,\ col2\)
\ VALUES\ \(\d,\ '\d'\);\n){9}/xm,
like => { column_inserts => 1, },
},
@@ -1620,7 +1620,7 @@ my %tests = (
'CREATE TYPE dump_test.int42' => {
create_order => 39,
create_sql => 'CREATE TYPE dump_test.int42;',
- regexp => qr/^CREATE TYPE dump_test.int42;/m,
+ regexp => qr/^CREATE TYPE dump_test\.int42;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -1832,7 +1832,7 @@ my %tests = (
'CREATE TYPE dump_test.undefined' => {
create_order => 39,
create_sql => 'CREATE TYPE dump_test.undefined;',
- regexp => qr/^CREATE TYPE dump_test.undefined;/m,
+ regexp => qr/^CREATE TYPE dump_test\.undefined;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -2770,7 +2770,7 @@ my %tests = (
create_sql => 'GRANT SELECT ON TABLE dump_test.test_table
TO regress_dump_test_role;',
regexp =>
- qr/^GRANT SELECT ON TABLE dump_test.test_table TO regress_dump_test_role;/m,
+ qr/^GRANT SELECT ON TABLE dump_test\.test_table TO regress_dump_test_role;/m,
like => {
%full_runs,
%dump_test_schema_runs,
@@ -2790,7 +2790,7 @@ my %tests = (
TABLE dump_test.measurement
TO regress_dump_test_role;',
regexp =>
- qr/^GRANT SELECT ON TABLE dump_test.measurement TO regress_dump_test_role;/m,
+ qr/^GRANT SELECT ON TABLE dump_test\.measurement TO regress_dump_test_role;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -2805,7 +2805,7 @@ my %tests = (
TABLE dump_test_second_schema.measurement_y2006m2
TO regress_dump_test_role;',
regexp =>
- qr/^GRANT SELECT ON TABLE dump_test_second_schema.measurement_y2006m2 TO regress_dump_test_role;/m,
+ qr/^GRANT SELECT ON TABLE dump_test_second_schema\.measurement_y2006m2 TO regress_dump_test_role;/m,
like => {
%full_runs,
role => 1,
@@ -2948,7 +2948,7 @@ my %tests = (
},
'REFRESH MATERIALIZED VIEW matview' => {
- regexp => qr/^REFRESH MATERIALIZED VIEW dump_test.matview;/m,
+ regexp => qr/^REFRESH MATERIALIZED VIEW dump_test\.matview;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_post_data => 1, },
unlike => {
@@ -3015,7 +3015,7 @@ my %tests = (
create_order => 45,
create_sql => 'REVOKE SELECT ON TABLE pg_proc FROM public;',
regexp =>
- qr/^REVOKE SELECT ON TABLE pg_catalog.pg_proc FROM PUBLIC;/m,
+ qr/^REVOKE SELECT ON TABLE pg_catalog\.pg_proc FROM PUBLIC;/m,
like => { %full_runs, section_pre_data => 1, },
unlike => { no_privs => 1, },
},
On 4 Feb 2019, at 11:54, David Rowley <david.rowley@2ndquadrant.com> wrote:
There are a few regexes in pg_dump's tap tests that are neglecting to
escape the dot in "schema.table" type expressions. This could result
in the test passing when it shouldn't. It seems worth putting that
right.
+1 for tightening it up, and the patch looks good to me.
We may also want to use the + metacharacter instead of * in a few places, since
the intent is to always match something, where matching nothing should be
considered an error:
- qr/^ALTER TEXT SEARCH DICTIONARY dump_test.alt_ts_dict1 OWNER TO .*;/m,
+ qr/^ALTER TEXT SEARCH DICTIONARY dump_test\.alt_ts_dict1 OWNER TO .*;/m,cheers ./daniel
On Mon, Feb 04, 2019 at 01:12:48PM +0100, Daniel Gustafsson wrote:
+1 for tightening it up, and the patch looks good to me.
We may also want to use the + metacharacter instead of * in a few places, since
the intent is to always match something, where matching nothing should be
considered an error:- qr/^ALTER TEXT SEARCH DICTIONARY dump_test.alt_ts_dict1 OWNER TO .*;/m, + qr/^ALTER TEXT SEARCH DICTIONARY dump_test\.alt_ts_dict1 OWNER TO .*;/m,
Some tests are missing the update, and it seems to me that
tightening things up is a good thing, still we ought to do it
consistently. Some places missing the update:
- ALTER OPERATOR FAMILY
- ALTER OPERATOR CLASS
- ALTER SEQUENCE
- ALTER TABLE (ONLY, partitioned table)
- BLOB load
- COMMENT ON
- COPY
- INSERT INTO
- CREATE COLLATION
test_pg_dump's 001_base.pl needs also a refresh.
--
Michael
On Tue, 5 Feb 2019 at 13:15, Michael Paquier <michael@paquier.xyz> wrote:
Some tests are missing the update, and it seems to me that
tightening things up is a good thing, still we ought to do it
consistently. Some places missing the update:
- ALTER OPERATOR FAMILY
- ALTER OPERATOR CLASS
- ALTER SEQUENCE
- ALTER TABLE (ONLY, partitioned table)
- BLOB load
- COMMENT ON
- COPY
- INSERT INTO
- CREATE COLLATION
I think I've done all the ones that use \E. Those \Q ones don't need
any escaping. I assume that's the ones you've found. I didn't do an
exhaustive check though.
test_pg_dump's 001_base.pl needs also a refresh.
Yeah, I thought about looking, but I thought it might be a bottomless pit.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Feb 05, 2019 at 01:50:50PM +1300, David Rowley wrote:
I think I've done all the ones that use \E. Those \Q ones don't need
any escaping. I assume that's the ones you've found. I didn't do an
exhaustive check though.
Oh, good point. I didn't know that anything between \Q and \E are
interpreted as literal characters. Instead of the approach you are
proposing, perhaps it would make sense to extend this way of doing
things then? For example some tests with CREATE CONVERSION do so. It
looks much more portable than having to escape every dot.
test_pg_dump's 001_base.pl needs also a refresh.
Yeah, I thought about looking, but I thought it might be a bottomless pit.
I just double-checked this one, and the regex patterns there look
all correct.
--
Michael
On Tue, 5 Feb 2019 at 14:41, Michael Paquier <michael@paquier.xyz> wrote:
Instead of the approach you are
proposing, perhaps it would make sense to extend this way of doing
things then? For example some tests with CREATE CONVERSION do so. It
looks much more portable than having to escape every dot.
I'm not particularly excited either way, but here's a patch with it that way.
I did leave a couple untouched as there was quite a bit of escaping
going on already. I didn't think switching between \Q and \E would
have made those ones any more pleasing to the eye.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
tighten_up_pg_dump_tap_tests_v2.patchapplication/octet-stream; name=tighten_up_pg_dump_tap_tests_v2.patchDownload
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index 245fcbf5ce..073fa9c1ad 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -448,7 +448,7 @@ my %tests = (
},
'ALTER COLLATION test0 OWNER TO' => {
- regexp => qr/^ALTER COLLATION public.test0 OWNER TO .*;/m,
+ regexp => qr/^\QALTER COLLATION public.test0 OWNER TO \E.*;/m,
collation => 1,
like => { %full_runs, section_pre_data => 1, },
unlike => { %dump_test_schema_runs, no_owner => 1, },
@@ -757,7 +757,7 @@ my %tests = (
},
'ALTER TABLE test_table OWNER TO' => {
- regexp => qr/^ALTER TABLE dump_test.test_table OWNER TO .*;/m,
+ regexp => qr/^\QALTER TABLE dump_test.test_table OWNER TO \E.*;/m,
like => {
%full_runs,
%dump_test_schema_runs,
@@ -776,7 +776,7 @@ my %tests = (
create_sql => 'ALTER TABLE dump_test.test_table
ENABLE ROW LEVEL SECURITY;',
regexp =>
- qr/^ALTER TABLE dump_test.test_table ENABLE ROW LEVEL SECURITY;/m,
+ qr/^\QALTER TABLE dump_test.test_table ENABLE ROW LEVEL SECURITY;/m,
like => {
%full_runs,
%dump_test_schema_runs,
@@ -790,7 +790,7 @@ my %tests = (
},
'ALTER TABLE test_second_table OWNER TO' => {
- regexp => qr/^ALTER TABLE dump_test.test_second_table OWNER TO .*;/m,
+ regexp => qr/^\QALTER TABLE dump_test.test_second_table OWNER TO \E.*;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -800,7 +800,7 @@ my %tests = (
},
'ALTER TABLE measurement OWNER TO' => {
- regexp => qr/^ALTER TABLE dump_test.measurement OWNER TO .*;/m,
+ regexp => qr/^\QALTER TABLE dump_test.measurement OWNER TO \E.*;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -811,7 +811,7 @@ my %tests = (
'ALTER TABLE measurement_y2006m2 OWNER TO' => {
regexp =>
- qr/^ALTER TABLE dump_test_second_schema.measurement_y2006m2 OWNER TO .*;/m,
+ qr/^\QALTER TABLE dump_test_second_schema.measurement_y2006m2 OWNER TO \E.*;/m,
like => {
%full_runs,
role => 1,
@@ -822,7 +822,7 @@ my %tests = (
'ALTER FOREIGN TABLE foreign_table OWNER TO' => {
regexp =>
- qr/^ALTER FOREIGN TABLE dump_test.foreign_table OWNER TO .*;/m,
+ qr/^\QALTER FOREIGN TABLE dump_test.foreign_table OWNER TO \E.*;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -833,7 +833,7 @@ my %tests = (
'ALTER TEXT SEARCH CONFIGURATION alt_ts_conf1 OWNER TO' => {
regexp =>
- qr/^ALTER TEXT SEARCH CONFIGURATION dump_test.alt_ts_conf1 OWNER TO .*;/m,
+ qr/^\QALTER TEXT SEARCH CONFIGURATION dump_test.alt_ts_conf1 OWNER TO \E.*;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -844,7 +844,7 @@ my %tests = (
'ALTER TEXT SEARCH DICTIONARY alt_ts_dict1 OWNER TO' => {
regexp =>
- qr/^ALTER TEXT SEARCH DICTIONARY dump_test.alt_ts_dict1 OWNER TO .*;/m,
+ qr/^\QALTER TEXT SEARCH DICTIONARY dump_test.alt_ts_dict1 OWNER TO \E.*;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -913,7 +913,7 @@ my %tests = (
create_sql => 'COMMENT ON TABLE dump_test.test_table
IS \'comment on table\';',
regexp =>
- qr/^COMMENT ON TABLE dump_test.test_table IS 'comment on table';/m,
+ qr/^\QCOMMENT ON TABLE dump_test.test_table IS 'comment on table';/m,
like => {
%full_runs,
%dump_test_schema_runs,
@@ -986,7 +986,7 @@ my %tests = (
create_sql => 'COMMENT ON CONVERSION dump_test.test_conversion
IS \'comment on test conversion\';',
regexp =>
- qr/^COMMENT ON CONVERSION dump_test.test_conversion IS 'comment on test conversion';/m,
+ qr/^\QCOMMENT ON CONVERSION dump_test.test_conversion IS 'comment on test conversion';/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -997,7 +997,7 @@ my %tests = (
create_sql => 'COMMENT ON COLLATION test0
IS \'comment on test0 collation\';',
regexp =>
- qr/^COMMENT ON COLLATION public.test0 IS 'comment on test0 collation';/m,
+ qr/^\QCOMMENT ON COLLATION public.test0 IS 'comment on test0 collation';/m,
collation => 1,
like => { %full_runs, section_pre_data => 1, },
},
@@ -1051,7 +1051,7 @@ my %tests = (
'COMMENT ON TEXT SEARCH CONFIGURATION dump_test.alt_ts_conf1
IS \'comment on text search configuration\';',
regexp =>
- qr/^COMMENT ON TEXT SEARCH CONFIGURATION dump_test.alt_ts_conf1 IS 'comment on text search configuration';/m,
+ qr/^\QCOMMENT ON TEXT SEARCH CONFIGURATION dump_test.alt_ts_conf1 IS 'comment on text search configuration';/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -1063,7 +1063,7 @@ my %tests = (
'COMMENT ON TEXT SEARCH DICTIONARY dump_test.alt_ts_dict1
IS \'comment on text search dictionary\';',
regexp =>
- qr/^COMMENT ON TEXT SEARCH DICTIONARY dump_test.alt_ts_dict1 IS 'comment on text search dictionary';/m,
+ qr/^\QCOMMENT ON TEXT SEARCH DICTIONARY dump_test.alt_ts_dict1 IS 'comment on text search dictionary';/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -1074,7 +1074,7 @@ my %tests = (
create_sql => 'COMMENT ON TEXT SEARCH PARSER dump_test.alt_ts_prs1
IS \'comment on text search parser\';',
regexp =>
- qr/^COMMENT ON TEXT SEARCH PARSER dump_test.alt_ts_prs1 IS 'comment on text search parser';/m,
+ qr/^\QCOMMENT ON TEXT SEARCH PARSER dump_test.alt_ts_prs1 IS 'comment on text search parser';/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -1085,7 +1085,7 @@ my %tests = (
create_sql => 'COMMENT ON TEXT SEARCH TEMPLATE dump_test.alt_ts_temp1
IS \'comment on text search template\';',
regexp =>
- qr/^COMMENT ON TEXT SEARCH TEMPLATE dump_test.alt_ts_temp1 IS 'comment on text search template';/m,
+ qr/^\QCOMMENT ON TEXT SEARCH TEMPLATE dump_test.alt_ts_temp1 IS 'comment on text search template';/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -1096,7 +1096,7 @@ my %tests = (
create_sql => 'COMMENT ON TYPE dump_test.planets
IS \'comment on enum type\';',
regexp =>
- qr/^COMMENT ON TYPE dump_test.planets IS 'comment on enum type';/m,
+ qr/^\QCOMMENT ON TYPE dump_test.planets IS 'comment on enum type';/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -1107,7 +1107,7 @@ my %tests = (
create_sql => 'COMMENT ON TYPE dump_test.textrange
IS \'comment on range type\';',
regexp =>
- qr/^COMMENT ON TYPE dump_test.textrange IS 'comment on range type';/m,
+ qr/^\QCOMMENT ON TYPE dump_test.textrange IS 'comment on range type';/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -1118,7 +1118,7 @@ my %tests = (
create_sql => 'COMMENT ON TYPE dump_test.int42
IS \'comment on regular type\';',
regexp =>
- qr/^COMMENT ON TYPE dump_test.int42 IS 'comment on regular type';/m,
+ qr/^\QCOMMENT ON TYPE dump_test.int42 IS 'comment on regular type';/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -1129,7 +1129,7 @@ my %tests = (
create_sql => 'COMMENT ON TYPE dump_test.undefined
IS \'comment on undefined type\';',
regexp =>
- qr/^COMMENT ON TYPE dump_test.undefined IS 'comment on undefined type';/m,
+ qr/^\QCOMMENT ON TYPE dump_test.undefined IS 'comment on undefined type';/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -1282,14 +1282,14 @@ my %tests = (
'INSERT INTO test_table' => {
regexp => qr/^
- (?:INSERT\ INTO\ dump_test.test_table\ \(col1,\ col2,\ col3,\ col4\)\ VALUES\ \(\d,\ NULL,\ NULL,\ NULL\);\n){9}
+ (?:INSERT\ INTO\ dump_test\.test_table\ \(col1,\ col2,\ col3,\ col4\)\ VALUES\ \(\d,\ NULL,\ NULL,\ NULL\);\n){9}
/xm,
like => { column_inserts => 1, },
},
'INSERT INTO test_second_table' => {
regexp => qr/^
- (?:INSERT\ INTO\ dump_test.test_second_table\ \(col1,\ col2\)
+ (?:INSERT\ INTO\ dump_test\.test_second_table\ \(col1,\ col2\)
\ VALUES\ \(\d,\ '\d'\);\n){9}/xm,
like => { column_inserts => 1, },
},
@@ -1620,7 +1620,7 @@ my %tests = (
'CREATE TYPE dump_test.int42' => {
create_order => 39,
create_sql => 'CREATE TYPE dump_test.int42;',
- regexp => qr/^CREATE TYPE dump_test.int42;/m,
+ regexp => qr/^\QCREATE TYPE dump_test.int42;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -1832,7 +1832,7 @@ my %tests = (
'CREATE TYPE dump_test.undefined' => {
create_order => 39,
create_sql => 'CREATE TYPE dump_test.undefined;',
- regexp => qr/^CREATE TYPE dump_test.undefined;/m,
+ regexp => qr/^\QCREATE TYPE dump_test.undefined;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => { exclude_dump_test_schema => 1, },
@@ -2770,7 +2770,7 @@ my %tests = (
create_sql => 'GRANT SELECT ON TABLE dump_test.test_table
TO regress_dump_test_role;',
regexp =>
- qr/^GRANT SELECT ON TABLE dump_test.test_table TO regress_dump_test_role;/m,
+ qr/^\QGRANT SELECT ON TABLE dump_test.test_table TO regress_dump_test_role;/m,
like => {
%full_runs,
%dump_test_schema_runs,
@@ -2790,7 +2790,7 @@ my %tests = (
TABLE dump_test.measurement
TO regress_dump_test_role;',
regexp =>
- qr/^GRANT SELECT ON TABLE dump_test.measurement TO regress_dump_test_role;/m,
+ qr/^\QGRANT SELECT ON TABLE dump_test.measurement TO regress_dump_test_role;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -2805,7 +2805,7 @@ my %tests = (
TABLE dump_test_second_schema.measurement_y2006m2
TO regress_dump_test_role;',
regexp =>
- qr/^GRANT SELECT ON TABLE dump_test_second_schema.measurement_y2006m2 TO regress_dump_test_role;/m,
+ qr/^\QGRANT SELECT ON TABLE dump_test_second_schema.measurement_y2006m2 TO regress_dump_test_role;/m,
like => {
%full_runs,
role => 1,
@@ -2948,7 +2948,7 @@ my %tests = (
},
'REFRESH MATERIALIZED VIEW matview' => {
- regexp => qr/^REFRESH MATERIALIZED VIEW dump_test.matview;/m,
+ regexp => qr/^\QREFRESH MATERIALIZED VIEW dump_test.matview;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_post_data => 1, },
unlike => {
@@ -3015,7 +3015,7 @@ my %tests = (
create_order => 45,
create_sql => 'REVOKE SELECT ON TABLE pg_proc FROM public;',
regexp =>
- qr/^REVOKE SELECT ON TABLE pg_catalog.pg_proc FROM PUBLIC;/m,
+ qr/^\QREVOKE SELECT ON TABLE pg_catalog.pg_proc FROM PUBLIC;/m,
like => { %full_runs, section_pre_data => 1, },
unlike => { no_privs => 1, },
},
On Tue, 5 Feb 2019 at 01:12, Daniel Gustafsson <daniel@yesql.se> wrote:
We may also want to use the + metacharacter instead of * in a few places, since
the intent is to always match something, where matching nothing should be
considered an error:- qr/^ALTER TEXT SEARCH DICTIONARY dump_test.alt_ts_dict1 OWNER TO .*;/m, + qr/^ALTER TEXT SEARCH DICTIONARY dump_test\.alt_ts_dict1 OWNER TO .*;/m,
I looked for instances of * alone and didn't see any. I only saw ones
prefixed with ".", in which case, isn't that matching 1 or more chars
already?
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Monday, February 4, 2019, David Rowley <david.rowley@2ndquadrant.com>
wrote:
On Tue, 5 Feb 2019 at 01:12, Daniel Gustafsson <daniel@yesql.se> wrote:
We may also want to use the + metacharacter instead of * in a few
places, since
the intent is to always match something, where matching nothing should be
considered an error:- qr/^ALTER TEXT SEARCH DICTIONARY
dump_test.alt_ts_dict1 OWNER TO .*;/m,
+ qr/^ALTER TEXT SEARCH DICTIONARY
dump_test\.alt_ts_dict1 OWNER TO .*;/m,
I looked for instances of * alone and didn't see any. I only saw ones
prefixed with ".", in which case, isn't that matching 1 or more chars
already?
No. In Regex the following are equivalent:
.* == .{0,}
.+ == .{1,}
. == .{1}
A “*” by itself would either be an error or, assuming the preceding
character is a space (so it visually looks alone) would be zero or more
consecutive spaces.
In the above “...OWNER TO<space>;” is a valid match.
David J.
On Tue, Feb 05, 2019 at 05:46:55PM +1300, David Rowley wrote:
I did leave a couple untouched as there was quite a bit of escaping
going on already. I didn't think switching between \Q and \E would
have made those ones any more pleasing to the eye.
- qr/^ALTER TABLE dump_test.test_table ENABLE ROW LEVEL SECURITY;/m,
+ qr/^\QALTER TABLE dump_test.test_table ENABLE ROW LEVEL SECURITY;/m,
I would have just appended the \E for consistency at the end of the
strings. Except that it looks fine. No need to send an updated
version, it seems that you have all the spots. I'll do an extra pass
on it tomorrow and see if I can commit it.
--
MichaelOn 5 Feb 2019, at 06:55, David G. Johnston <david.g.johnston@gmail.com> wrote:
On Monday, February 4, 2019, David Rowley <david.rowley@2ndquadrant.com <mailto:david.rowley@2ndquadrant.com>> wrote:
On Tue, 5 Feb 2019 at 01:12, Daniel Gustafsson <daniel@yesql.se <mailto:daniel@yesql.se>> wrote:We may also want to use the + metacharacter instead of * in a few places, since
the intent is to always match something, where matching nothing should be
considered an error:- qr/^ALTER TEXT SEARCH DICTIONARY dump_test.alt_ts_dict1 OWNER TO .*;/m, + qr/^ALTER TEXT SEARCH DICTIONARY dump_test\.alt_ts_dict1 OWNER TO .*;/m,I looked for instances of * alone and didn't see any. I only saw ones
prefixed with ".", in which case, isn't that matching 1 or more chars
already?No. In Regex the following are equivalent:
.* == .{0,}
.+ == .{1,}
. == .{1}A “*” by itself would either be an error or, assuming the preceding character is a space (so it visually looks alone) would be zero or more consecutive spaces.
Sorry for being a bit unclear in my original email, it’s as David says above:
.* matches zero or more characters and .+ matches 1 or more characters.
In the above “...OWNER TO<space>;” is a valid match.
Indeed, so we should move to matching with .+ to force an owner.
cheers ./daniel
On Tue, Feb 05, 2019 at 04:26:18PM +0900, Michael Paquier wrote:
On Tue, Feb 05, 2019 at 05:46:55PM +1300, David Rowley wrote:
I did leave a couple untouched as there was quite a bit of escaping
going on already. I didn't think switching between \Q and \E would
have made those ones any more pleasing to the eye.
Indeed. I have stuck with your version here.
- qr/^ALTER TABLE dump_test.test_table ENABLE ROW LEVEL SECURITY;/m, + qr/^\QALTER TABLE dump_test.test_table ENABLE ROW LEVEL SECURITY;/m, I would have just appended the \E for consistency at the end of the strings. Except that it looks fine. No need to send an updated version, it seems that you have all the spots. I'll do an extra pass on it tomorrow and see if I can commit it.
And done after checking the whole set.
--
Michael
On 6 Feb 2019, at 09:39, Michael Paquier <michael@paquier.xyz> wrote:
And done after checking the whole set.
I still think we should enforce one-or-more matching on the OWNER part as well,
since matching zero would be a syntax error. There are more .* matches but
I’ve only touched the ones which match SQL, since there is a defined grammar to
rely on there. The attached patch does that on top of your commit.
cheers ./daniel
Attachments:
pg_dump_testregex.patchapplication/octet-stream; name=pg_dump_testregex.patch; x-unix-mode=0644Download
From 06533f9c5223be449f5a086746e16c9397603876 Mon Sep 17 00:00:00 2001
From: Daniel Gustafsson <daniel@yesql.se>
Date: Wed, 6 Feb 2019 10:42:26 +0100
Subject: [PATCH] Align test output regex with grammar
This enforces one-or-more matches in the regular expressions for pg_dump
testing on SQL syntax output where zero-or-more matches implies a syntax
error.
---
src/bin/pg_dump/t/002_pg_dump.pl | 40 ++++++++++++++++++++--------------------
1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index f70b88cab2..d54b88b9be 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -448,20 +448,20 @@ my %tests = (
},
'ALTER COLLATION test0 OWNER TO' => {
- regexp => qr/^\QALTER COLLATION public.test0 OWNER TO \E.*;/m,
+ regexp => qr/^\QALTER COLLATION public.test0 OWNER TO \E.+;/m,
collation => 1,
like => { %full_runs, section_pre_data => 1, },
unlike => { %dump_test_schema_runs, no_owner => 1, },
},
'ALTER FOREIGN DATA WRAPPER dummy OWNER TO' => {
- regexp => qr/^ALTER FOREIGN DATA WRAPPER dummy OWNER TO .*;/m,
+ regexp => qr/^ALTER FOREIGN DATA WRAPPER dummy OWNER TO .+;/m,
like => { %full_runs, section_pre_data => 1, },
unlike => { no_owner => 1, },
},
'ALTER SERVER s1 OWNER TO' => {
- regexp => qr/^ALTER SERVER s1 OWNER TO .*;/m,
+ regexp => qr/^ALTER SERVER s1 OWNER TO .+;/m,
like => { %full_runs, section_pre_data => 1, },
unlike => { no_owner => 1, },
},
@@ -470,7 +470,7 @@ my %tests = (
regexp => qr/^
\QALTER FUNCTION dump_test.pltestlang_call_handler() \E
\QOWNER TO \E
- .*;/xm,
+ .+;/xm,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -483,7 +483,7 @@ my %tests = (
regexp => qr/^
\QALTER OPERATOR FAMILY dump_test.op_family USING btree \E
\QOWNER TO \E
- .*;/xm,
+ .+;/xm,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -522,7 +522,7 @@ my %tests = (
regexp => qr/^
\QALTER OPERATOR CLASS dump_test.op_class USING btree \E
\QOWNER TO \E
- .*;/xm,
+ .+;/xm,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -532,13 +532,13 @@ my %tests = (
},
'ALTER PUBLICATION pub1 OWNER TO' => {
- regexp => qr/^ALTER PUBLICATION pub1 OWNER TO .*;/m,
+ regexp => qr/^ALTER PUBLICATION pub1 OWNER TO .+;/m,
like => { %full_runs, section_post_data => 1, },
unlike => { no_owner => 1, },
},
'ALTER LARGE OBJECT ... OWNER TO' => {
- regexp => qr/^ALTER LARGE OBJECT \d+ OWNER TO .*;/m,
+ regexp => qr/^ALTER LARGE OBJECT \d+ OWNER TO .+;/m,
like => {
%full_runs,
column_inserts => 1,
@@ -554,13 +554,13 @@ my %tests = (
},
'ALTER PROCEDURAL LANGUAGE pltestlang OWNER TO' => {
- regexp => qr/^ALTER PROCEDURAL LANGUAGE pltestlang OWNER TO .*;/m,
+ regexp => qr/^ALTER PROCEDURAL LANGUAGE pltestlang OWNER TO .+;/m,
like => { %full_runs, section_pre_data => 1, },
unlike => { no_owner => 1, },
},
'ALTER SCHEMA dump_test OWNER TO' => {
- regexp => qr/^ALTER SCHEMA dump_test OWNER TO .*;/m,
+ regexp => qr/^ALTER SCHEMA dump_test OWNER TO .+;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -570,7 +570,7 @@ my %tests = (
},
'ALTER SCHEMA dump_test_second_schema OWNER TO' => {
- regexp => qr/^ALTER SCHEMA dump_test_second_schema OWNER TO .*;/m,
+ regexp => qr/^ALTER SCHEMA dump_test_second_schema OWNER TO .+;/m,
like => {
%full_runs,
role => 1,
@@ -757,7 +757,7 @@ my %tests = (
},
'ALTER TABLE test_table OWNER TO' => {
- regexp => qr/^\QALTER TABLE dump_test.test_table OWNER TO \E.*;/m,
+ regexp => qr/^\QALTER TABLE dump_test.test_table OWNER TO \E.+;/m,
like => {
%full_runs,
%dump_test_schema_runs,
@@ -790,7 +790,7 @@ my %tests = (
},
'ALTER TABLE test_second_table OWNER TO' => {
- regexp => qr/^\QALTER TABLE dump_test.test_second_table OWNER TO \E.*;/m,
+ regexp => qr/^\QALTER TABLE dump_test.test_second_table OWNER TO \E.+;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -800,7 +800,7 @@ my %tests = (
},
'ALTER TABLE measurement OWNER TO' => {
- regexp => qr/^\QALTER TABLE dump_test.measurement OWNER TO \E.*;/m,
+ regexp => qr/^\QALTER TABLE dump_test.measurement OWNER TO \E.+;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -811,7 +811,7 @@ my %tests = (
'ALTER TABLE measurement_y2006m2 OWNER TO' => {
regexp =>
- qr/^\QALTER TABLE dump_test_second_schema.measurement_y2006m2 OWNER TO \E.*;/m,
+ qr/^\QALTER TABLE dump_test_second_schema.measurement_y2006m2 OWNER TO \E.+;/m,
like => {
%full_runs,
role => 1,
@@ -822,7 +822,7 @@ my %tests = (
'ALTER FOREIGN TABLE foreign_table OWNER TO' => {
regexp =>
- qr/^\QALTER FOREIGN TABLE dump_test.foreign_table OWNER TO \E.*;/m,
+ qr/^\QALTER FOREIGN TABLE dump_test.foreign_table OWNER TO \E.+;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -833,7 +833,7 @@ my %tests = (
'ALTER TEXT SEARCH CONFIGURATION alt_ts_conf1 OWNER TO' => {
regexp =>
- qr/^\QALTER TEXT SEARCH CONFIGURATION dump_test.alt_ts_conf1 OWNER TO \E.*;/m,
+ qr/^\QALTER TEXT SEARCH CONFIGURATION dump_test.alt_ts_conf1 OWNER TO \E.+;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -844,7 +844,7 @@ my %tests = (
'ALTER TEXT SEARCH DICTIONARY alt_ts_dict1 OWNER TO' => {
regexp =>
- qr/^\QALTER TEXT SEARCH DICTIONARY dump_test.alt_ts_dict1 OWNER TO \E.*;/m,
+ qr/^\QALTER TEXT SEARCH DICTIONARY dump_test.alt_ts_dict1 OWNER TO \E.+;/m,
like =>
{ %full_runs, %dump_test_schema_runs, section_pre_data => 1, },
unlike => {
@@ -895,14 +895,14 @@ my %tests = (
},
'COMMENT ON DATABASE postgres' => {
- regexp => qr/^COMMENT ON DATABASE postgres IS .*;/m,
+ regexp => qr/^COMMENT ON DATABASE postgres IS .+;/m,
# Should appear in the same tests as "CREATE DATABASE postgres"
like => { createdb => 1, },
},
'COMMENT ON EXTENSION plpgsql' => {
- regexp => qr/^COMMENT ON EXTENSION plpgsql IS .*;/m,
+ regexp => qr/^COMMENT ON EXTENSION plpgsql IS .+;/m,
# this shouldn't ever get emitted anymore
like => {},
--
2.14.1.145.gb3622a4ee
On Wed, 6 Feb 2019 at 21:39, Michael Paquier <michael@paquier.xyz> wrote:
And done after checking the whole set.
Thanks for pushing.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Feb 06, 2019 at 10:50:27AM +0100, Daniel Gustafsson wrote:
I still think we should enforce one-or-more matching on the OWNER part as well,
since matching zero would be a syntax error. There are more .* matches but
I’ve only touched the ones which match SQL, since there is a defined grammar to
rely on there. The attached patch does that on top of your commit.
- regexp => qr/^COMMENT ON DATABASE postgres IS .*;/m,
+ regexp => qr/^COMMENT ON DATABASE postgres IS .+;/m,
So... With what's on HEAD the current regex means that all these are
correct commands:
COMMENT ON DATABASE postgres is ;
COMMENT ON DATABASE postgres is foo;
COMMENT ON DATABASE postgres is foo;
And for the first one that's obviously wrong.So what you are suggesting is to actually make the first pattern
something to complain about, right? And ".+" this makes sure that at
least one character is present, while for ".*" it is fine to have zero
characters. What you are suggesting looks right if I understood that
right.
--
Michael
On 6 Feb 2019, at 12:37, Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Feb 06, 2019 at 10:50:27AM +0100, Daniel Gustafsson wrote:
I still think we should enforce one-or-more matching on the OWNER part as well,
since matching zero would be a syntax error. There are more .* matches but
I’ve only touched the ones which match SQL, since there is a defined grammar to
rely on there. The attached patch does that on top of your commit.- regexp => qr/^COMMENT ON DATABASE postgres IS .*;/m, + regexp => qr/^COMMENT ON DATABASE postgres IS .+;/m, So... With what's on HEAD the current regex means that all these are correct commands: COMMENT ON DATABASE postgres is ; COMMENT ON DATABASE postgres is foo; COMMENT ON DATABASE postgres is foo; And for the first one that's obviously wrong.So what you are suggesting is to actually make the first pattern
something to complain about, right? And ".+" this makes sure that at
least one character is present, while for ".*" it is fine to have zero
characters. What you are suggesting looks right if I understood that
right.
Correct. One could argue that the regex is still suboptimal since “COMMENT ON
DATABASE postgres IS ;” will be matched as well, but there I think the tradeoff
for readability wins.
cheers ./daniel
On Wed, Feb 06, 2019 at 03:41:20PM +0100, Daniel Gustafsson wrote:
Correct. One could argue that the regex is still suboptimal since “COMMENT ON
DATABASE postgres IS ;” will be matched as well, but there I think the tradeoff
for readability wins.
Okay, that looks like an improvement anyway, so committed after going
over the tests for similar problems, and there was one for CREATE
DATABASE and DROP ROLE. It is possible to have a regex which tells as
at least one non-whitespace character, but from what I can see these
don't really improve the readability.
--
Michael
Moin,
On Wed, February 6, 2019 8:10 pm, Michael Paquier wrote:
On Wed, Feb 06, 2019 at 03:41:20PM +0100, Daniel Gustafsson wrote:
Correct. One could argue that the regex is still suboptimal since
“COMMENT ON
DATABASE postgres IS ;” will be matched as well, but there I think the
tradeoff
for readability wins.Okay, that looks like an improvement anyway, so committed after going
over the tests for similar problems, and there was one for CREATE
DATABASE and DROP ROLE. It is possible to have a regex which tells as
at least one non-whitespace character, but from what I can see these
don't really improve the readability.
Sorry for being late to the party, but it just occured to me that while
".+" is definitely an improvement over ".*", it isn't foolproof either,
as it also matches ";".
Thus:
regexp => qr/^COMMENT ON DATABASE postgres IS .+;/m,
matches things like:
'COMMENT ON DATABASE postgres IS ;;'
'COMMENT ON DATABASE postgres IS ;'
'COMMENT ON DATABASE postgres IS --;'
etc.
I'm not sure it is really necessary to deal with these cases, but one
possibility would be to pre-compute regexps:
$QR_COMMENT = qr/[^ ;]+/;
$QR_IDENTIFIER = qr/[^ ;]+/; # etc
or whataver is the thing that should actually be matched here and use them
like so:
regexp => qr/^COMMENT ON DATABASE postgres IS $QR_COMMENT;/m,
That way it is easily changable and quite readable.
Oh, one more question. Shouldn't these regexps that start with "^" also
end with "$"? Or can there be output like:
'COMMENT ON DATABASE postgres IS $QR_IDENTIFIER; SELECT 1;'
?
Best regards,
Tels
On 7 Feb 2019, at 13:55, Tels <nospam-pg-abuse@bloodgate.com> wrote:
On Wed, February 6, 2019 8:10 pm, Michael Paquier wrote:On Wed, Feb 06, 2019 at 03:41:20PM +0100, Daniel Gustafsson wrote:
Correct. One could argue that the regex is still suboptimal since
“COMMENT ON
DATABASE postgres IS ;” will be matched as well, but there I think the
tradeoff
for readability wins.Okay, that looks like an improvement anyway, so committed after going
over the tests for similar problems, and there was one for CREATE
DATABASE and DROP ROLE. It is possible to have a regex which tells as
at least one non-whitespace character, but from what I can see these
don't really improve the readability.Sorry for being late to the party, but it just occured to me that while
".+" is definitely an improvement over ".*", it isn't foolproof either,
as it also matches ";”.
Correct. The idea was to maintain readability while making the regex a bit
better, without any claims to make it perfect. One can argue whether or not
that’s enough, but IMO keeping the tests as readable as possible is preferrable
when it comes to the tests in question, as they aren’t matching against user
supplied SQL but machine generated.
cheers ./daniel
On Thu, Feb 07, 2019 at 10:10:32AM +0900, Michael Paquier wrote:
On Wed, Feb 06, 2019 at 03:41:20PM +0100, Daniel Gustafsson wrote:
Correct. One could argue that the regex is still suboptimal since “COMMENT ON
DATABASE postgres IS ;” will be matched as well, but there I think the tradeoff
for readability wins.Okay, that looks like an improvement anyway, so committed after going
over the tests for similar problems, and there was one for CREATE
DATABASE and DROP ROLE. It is possible to have a regex which tells as
at least one non-whitespace character, but from what I can see these
don't really improve the readability.
Are you talking about \w+, or [^[:space:]]+, [^[:blank:]]+, or...?
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
On 7 Feb 2019, at 18:20, David Fetter <david@fetter.org> wrote:
On Thu, Feb 07, 2019 at 10:10:32AM +0900, Michael Paquier wrote:
On Wed, Feb 06, 2019 at 03:41:20PM +0100, Daniel Gustafsson wrote:
Correct. One could argue that the regex is still suboptimal since “COMMENT ON
DATABASE postgres IS ;” will be matched as well, but there I think the tradeoff
for readability wins.Okay, that looks like an improvement anyway, so committed after going
over the tests for similar problems, and there was one for CREATE
DATABASE and DROP ROLE. It is possible to have a regex which tells as
at least one non-whitespace character, but from what I can see these
don't really improve the readability.Are you talking about \w+, or [^[:space:]]+, [^[:blank:]]+, or…?
Personally I feel that expanding these test regexes to catch more low-risk
bugs, at the cost of readability, is a slippery slope towards implementing a
SQL parser in regexes. That was my $0.02 for not attempting to make these
bulletproof.
cheers ./daniel
On Thu, Feb 07, 2019 at 03:33:54PM +0100, Daniel Gustafsson wrote:
Correct. The idea was to maintain readability while making the regex a bit
better, without any claims to make it perfect.
Agreed with your position. I see no point is making all the
expressions unreadable for little gain. What Daniel proposed upthread
had a better balance in my opinion than the previous behavior, without
sacrifying the readability of anything and still improving the error
detection.
--
Michael
On Fri, 8 Feb 2019 at 13:04, Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Feb 07, 2019 at 03:33:54PM +0100, Daniel Gustafsson wrote:
Correct. The idea was to maintain readability while making the regex a bit
better, without any claims to make it perfect.Agreed with your position. I see no point is making all the
expressions unreadable for little gain. What Daniel proposed upthread
had a better balance in my opinion than the previous behavior, without
sacrifying the readability of anything and still improving the error
detection.
FWIW, this was the first time I'd really worked with TAP tests. I had
been looking into it because I needed to add a new test.
I was surprised to see it working this way and not just doing a diff
with some known good output. I'm maybe just missing the reason that's
not possible, but to me, it seems a bit less error-prone than trying
to ensure an exact match with a bunch of regular expressions.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services