Обсуждение: pgsql: Establish conventions about global object names used in regressi
Establish conventions about global object names used in regression tests. To ensure that "make installcheck" can be used safely against an existing installation, we need to be careful about what global object names (database, role, and tablespace names) we use; otherwise we might accidentally clobber important objects. There's been a weak consensus that test databases should have names including "regression", and that test role names should start with "regress_", but we didn't have any particular rule about tablespace names; and neither of the other rules was followed with any consistency either. This commit moves us a long way towards having a hard-and-fast rule that regression test databases must have names including "regression", and that test role and tablespace names must start with "regress_". It's not completely there because I did not touch some test cases in rolenames.sql that test creation of special role names like "session_user". That will require some rethinking of exactly what we want to test, whereas the intent of this patch is just to hit all the cases in which the needed renamings are cosmetic. There is no enforcement mechanism in this patch either, but if we don't add one we can expect that the tests will soon be violating the convention again. Again, that's not such a cosmetic change and it will require discussion. (But I did use a quick-hack enforcement patch to find these cases.) Discussion: <16638.1468620817@sss.pgh.pa.us> Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/18555b1323bd225c7882e80723c52f25ce60afed Modified Files -------------- contrib/dblink/expected/dblink.out | 14 +- contrib/dblink/sql/dblink.sql | 14 +- contrib/file_fdw/input/file_fdw.source | 54 +- contrib/file_fdw/output/file_fdw.source | 58 +- contrib/sepgsql/expected/alter.out | 52 +- contrib/sepgsql/expected/ddl.out | 22 +- contrib/sepgsql/sql/alter.sql | 44 +- contrib/sepgsql/sql/ddl.sql | 18 +- contrib/test_decoding/expected/permissions.out | 32 +- contrib/test_decoding/sql/permissions.sql | 32 +- doc/src/sgml/dblink.sgml | 18 +- doc/src/sgml/regress.sgml | 12 +- src/bin/pg_dump/t/002_pg_dump.pl | 74 +-- src/bin/scripts/t/040_createuser.pl | 18 +- src/bin/scripts/t/070_dropuser.pl | 8 +- src/interfaces/ecpg/test/Makefile | 2 +- .../ecpg/test/compat_informix/test_informix2.pgc | 2 +- src/interfaces/ecpg/test/connect/test1.pgc | 28 +- src/interfaces/ecpg/test/connect/test2.pgc | 2 +- src/interfaces/ecpg/test/connect/test3.pgc | 4 +- src/interfaces/ecpg/test/connect/test5.pgc | 36 +- .../ecpg/test/expected/compat_informix-describe.c | 2 +- .../test/expected/compat_informix-describe.stderr | 22 +- .../ecpg/test/expected/compat_informix-rnull.c | 2 +- .../test/expected/compat_informix-rnull.stderr | 24 +- .../ecpg/test/expected/compat_informix-sqlda.c | 4 +- .../test/expected/compat_informix-sqlda.stderr | 4 +- .../test/expected/compat_informix-test_informix.c | 2 +- .../expected/compat_informix-test_informix.stderr | 44 +- .../test/expected/compat_informix-test_informix2.c | 2 +- .../expected/compat_informix-test_informix2.stderr | 22 +- .../test/expected/connect-test1-minGW32.stderr | 54 +- src/interfaces/ecpg/test/expected/connect-test1.c | 28 +- .../ecpg/test/expected/connect-test1.stderr | 50 +- src/interfaces/ecpg/test/expected/connect-test2.c | 4 +- .../ecpg/test/expected/connect-test2.stderr | 14 +- src/interfaces/ecpg/test/expected/connect-test3.c | 8 +- .../ecpg/test/expected/connect-test3.stderr | 10 +- src/interfaces/ecpg/test/expected/connect-test4.c | 2 +- .../ecpg/test/expected/connect-test4.stderr | 2 +- src/interfaces/ecpg/test/expected/connect-test5.c | 36 +- .../ecpg/test/expected/connect-test5.stderr | 46 +- .../ecpg/test/expected/pgtypeslib-dt_test.c | 2 +- .../ecpg/test/expected/pgtypeslib-dt_test.stderr | 16 +- .../ecpg/test/expected/pgtypeslib-nan_test.c | 2 +- .../ecpg/test/expected/pgtypeslib-nan_test.stderr | 80 +-- .../ecpg/test/expected/pgtypeslib-num_test.c | 2 +- .../ecpg/test/expected/pgtypeslib-num_test.stderr | 14 +- .../ecpg/test/expected/preproc-array_of_struct.c | 2 +- .../test/expected/preproc-array_of_struct.stderr | 20 +- .../ecpg/test/expected/preproc-autoprep.c | 2 +- .../ecpg/test/expected/preproc-autoprep.stderr | 76 +-- .../ecpg/test/expected/preproc-comment.c | 2 +- .../ecpg/test/expected/preproc-comment.stderr | 4 +- src/interfaces/ecpg/test/expected/preproc-cursor.c | 4 +- .../ecpg/test/expected/preproc-cursor.stderr | 4 +- src/interfaces/ecpg/test/expected/preproc-define.c | 2 +- .../ecpg/test/expected/preproc-define.stderr | 20 +- .../ecpg/test/expected/preproc-describe.c | 2 +- .../ecpg/test/expected/preproc-describe.stderr | 22 +- .../ecpg/test/expected/preproc-outofscope.c | 2 +- .../ecpg/test/expected/preproc-outofscope.stderr | 32 +- .../ecpg/test/expected/preproc-pointer_to_struct.c | 2 +- .../test/expected/preproc-pointer_to_struct.stderr | 20 +- .../ecpg/test/expected/preproc-strings.c | 2 +- .../ecpg/test/expected/preproc-strings.stderr | 8 +- src/interfaces/ecpg/test/expected/preproc-type.c | 2 +- .../ecpg/test/expected/preproc-type.stderr | 10 +- .../ecpg/test/expected/preproc-variable.c | 2 +- .../ecpg/test/expected/preproc-variable.stderr | 40 +- .../ecpg/test/expected/preproc-whenever.c | 2 +- .../ecpg/test/expected/preproc-whenever.stderr | 34 +- src/interfaces/ecpg/test/expected/sql-array.c | 2 +- src/interfaces/ecpg/test/expected/sql-array.stderr | 30 +- src/interfaces/ecpg/test/expected/sql-binary.c | 2 +- .../ecpg/test/expected/sql-binary.stderr | 28 +- src/interfaces/ecpg/test/expected/sql-code100.c | 2 +- .../ecpg/test/expected/sql-code100.stderr | 40 +- src/interfaces/ecpg/test/expected/sql-copystdout.c | 2 +- .../ecpg/test/expected/sql-copystdout.stderr | 14 +- src/interfaces/ecpg/test/expected/sql-define.c | 2 +- .../ecpg/test/expected/sql-define.stderr | 18 +- src/interfaces/ecpg/test/expected/sql-desc.c | 2 +- src/interfaces/ecpg/test/expected/sql-desc.stderr | 30 +- src/interfaces/ecpg/test/expected/sql-describe.c | 2 +- .../ecpg/test/expected/sql-describe.stderr | 22 +- src/interfaces/ecpg/test/expected/sql-dynalloc.c | 2 +- .../ecpg/test/expected/sql-dynalloc.stderr | 14 +- src/interfaces/ecpg/test/expected/sql-dynalloc2.c | 2 +- .../ecpg/test/expected/sql-dynalloc2.stderr | 24 +- src/interfaces/ecpg/test/expected/sql-dyntest.c | 2 +- .../ecpg/test/expected/sql-dyntest.stderr | 20 +- src/interfaces/ecpg/test/expected/sql-execute.c | 2 +- .../ecpg/test/expected/sql-execute.stderr | 2 +- src/interfaces/ecpg/test/expected/sql-fetch.c | 2 +- src/interfaces/ecpg/test/expected/sql-fetch.stderr | 42 +- src/interfaces/ecpg/test/expected/sql-func.c | 2 +- src/interfaces/ecpg/test/expected/sql-func.stderr | 28 +- src/interfaces/ecpg/test/expected/sql-indicators.c | 2 +- .../ecpg/test/expected/sql-indicators.stderr | 32 +- src/interfaces/ecpg/test/expected/sql-insupd.c | 2 +- .../ecpg/test/expected/sql-insupd.stderr | 20 +- src/interfaces/ecpg/test/expected/sql-oldexec.c | 2 +- .../ecpg/test/expected/sql-oldexec.stderr | 2 +- src/interfaces/ecpg/test/expected/sql-parser.c | 2 +- .../ecpg/test/expected/sql-parser.stderr | 18 +- src/interfaces/ecpg/test/expected/sql-quote.c | 2 +- src/interfaces/ecpg/test/expected/sql-quote.stderr | 42 +- src/interfaces/ecpg/test/expected/sql-show.c | 2 +- src/interfaces/ecpg/test/expected/sql-show.stderr | 24 +- src/interfaces/ecpg/test/expected/sql-sqlda.c | 4 +- src/interfaces/ecpg/test/expected/sql-sqlda.stderr | 4 +- src/interfaces/ecpg/test/expected/thread-alloc.c | 2 +- src/interfaces/ecpg/test/expected/thread-prep.c | 4 +- src/interfaces/ecpg/test/expected/thread-thread.c | 6 +- .../ecpg/test/expected/thread-thread_implicit.c | 6 +- src/interfaces/ecpg/test/regression.h | 8 +- src/test/isolation/isolation_main.c | 2 +- .../dummy_seclabel/expected/dummy_seclabel.out | 68 +-- .../modules/dummy_seclabel/sql/dummy_seclabel.sql | 47 +- .../test_ddl_deparse/expected/alter_function.out | 8 +- .../test_ddl_deparse/sql/alter_function.sql | 8 +- src/test/modules/test_pg_dump/t/001_base.pl | 14 +- .../modules/test_pg_dump/test_pg_dump--1.0.sql | 6 +- .../test_rls_hooks/expected/test_rls_hooks.out | 112 ++-- .../modules/test_rls_hooks/sql/test_rls_hooks.sql | 84 +-- src/test/regress/expected/alter_generic.out | 246 ++++---- src/test/regress/expected/alter_operator.out | 6 +- src/test/regress/expected/cluster.out | 12 +- src/test/regress/expected/conversion.out | 6 +- src/test/regress/expected/create_function_3.out | 10 +- src/test/regress/expected/create_index.out | 6 +- src/test/regress/expected/dependency.out | 130 ++--- src/test/regress/expected/drop_if_exists.out | 42 +- src/test/regress/expected/event_trigger.out | 38 +- src/test/regress/expected/foreign_data.out | 648 ++++++++++----------- src/test/regress/expected/guc.out | 10 +- src/test/regress/expected/object_address.out | 32 +- src/test/regress/expected/privileges.out | 568 +++++++++--------- src/test/regress/expected/regproc.out | 52 +- src/test/regress/expected/roleattributes.out | 352 +++++------ src/test/regress/expected/rolenames.out | 608 +++++++++---------- src/test/regress/expected/security_label.out | 29 +- src/test/regress/expected/select_into.out | 16 +- src/test/regress/expected/sequence.out | 58 +- src/test/regress/expected/sequence_1.out | 58 +- src/test/regress/expected/updatable_views.out | 34 +- src/test/regress/input/largeobject.source | 6 +- src/test/regress/input/tablespace.source | 60 +- src/test/regress/output/largeobject.source | 12 +- src/test/regress/output/largeobject_1.source | 12 +- src/test/regress/output/tablespace.source | 92 +-- src/test/regress/sql/alter_generic.sql | 146 ++--- src/test/regress/sql/alter_operator.sql | 6 +- src/test/regress/sql/cluster.sql | 12 +- src/test/regress/sql/conversion.sql | 6 +- src/test/regress/sql/create_function_3.sql | 10 +- src/test/regress/sql/create_index.sql | 6 +- src/test/regress/sql/dependency.sql | 90 +-- src/test/regress/sql/drop_if_exists.sql | 24 +- src/test/regress/sql/event_trigger.sql | 32 +- src/test/regress/sql/foreign_data.sql | 40 +- src/test/regress/sql/guc.sql | 10 +- src/test/regress/sql/object_address.sql | 26 +- src/test/regress/sql/privileges.sql | 450 +++++++------- src/test/regress/sql/regproc.sql | 20 +- src/test/regress/sql/roleattributes.sql | 160 ++--- src/test/regress/sql/rolenames.sql | 74 +-- src/test/regress/sql/security_label.sql | 30 +- src/test/regress/sql/select_into.sql | 16 +- src/test/regress/sql/sequence.sql | 58 +- src/test/regress/sql/updatable_views.sql | 34 +- src/tools/msvc/vcregress.pl | 4 +- 173 files changed, 3338 insertions(+), 3288 deletions(-)
Re: [COMMITTERS] pgsql: Establish conventions about global objectnames used in regressi
От
Peter Geoghegan
Дата:
On Sun, Jul 17, 2016 at 3:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Establish conventions about global object names used in regression tests. > require some rethinking of exactly what we want to test, whereas the intent > of this patch is just to hit all the cases in which the needed renamings > are cosmetic. I gather that this patch was intended to be cosmetic and mechanical. Why, then, did it result in a certain amount of seemingly more-than-cosmetic test output changes for certain ecpg tests? For example: -[NO_PID]: ECPGconnect: opening database <DEFAULT> on localhost port <DEFAULT> for user connectdb +[NO_PID]: ECPGconnect: opening database <DEFAULT> on localhost port <DEFAULT> for user regress_ecpg_user2 +[NO_PID]: sqlca: code: 0, state: 00000 +[NO_PID]: ECPGconnect: could not open database: FATAL: database "regress_ecpg_user2" does not exist I'm asking about this because I find that "make installcheck-world" sometimes fails at this point when run against my local installation -- the test "test connect/test" reliably fails. This is with Valgrind, though that's probably not relevant. My installation outputs more-or-less the *original* expected output prior to this commit. In other words, the two extra lines added by the patch no longer appear, plus there is a very similar difference a bit later in the same diffs file (we also fail to fail to connect to this database, if the test output is to be believed). It's as if there really was a "regress_ecpg_user2" database, even though I have no reason to believe that there should be one, and see no evidence that there ever was one beyond the failing test output. I am not the first person to notice this exact "regress_ecpg_user2" database issue [1]. Theoretically this could be a bug in the patch that I'm testing, but that seems rather unlikely. Some kind of problem with the test or some kind of environmental issue seem much more likely. [1] https://www.postgresql.org/message-id/1607.1503920376%40sss.pgh.pa.us -- Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
> On Sun, Jul 17, 2016 at 3:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Establish conventions about global object names used in regression tests.
> I gather that this patch was intended to be cosmetic and mechanical.
> Why, then, did it result in a certain amount of seemingly
> more-than-cosmetic test output changes for certain ecpg tests?
Hm ... it's been awhile, but I think what was happening there is
that the previous coding created both a user and a database named
"connectdb", and then checked if
exec sql connect to @localhost as main user connectdb;
would work. The naming convention I was putting in didn't allow for
identical DB and user names, and it didn't seem to me that it was
worth overriding that convention to keep this particular test
result the same. The error output still proves that ecpg *tried*
to connect to the database named the same as the user, which is
the only real point of this test case as compared to its siblings.
And why not test the error path, anyway?
> I'm asking about this because I find that "make installcheck-world"
> sometimes fails at this point when run against my local installation
> -- the test "test connect/test" reliably fails.
Um, you're confusing me ... is it "sometimes", or is it "reliably"?
It does seem like there might be something to poke at here. We never
did figure out quite what the other reporter was seeing.
Given the lack of overlap of user and database names, we can be
pretty darn certain that the test case shouldn't accidentally
succeed due to timing issues. I wonder if there could be some kind
of uninitialized-variable bug inside ecpg that lets it sometimes
connect to the wrong DB name (perhaps one it'd previously connected
to) if the DB name is omitted from the command?
regards, tom lane
Re: [COMMITTERS] pgsql: Establish conventions about global objectnames used in regressi
От
Peter Geoghegan
Дата:
On Sat, Sep 21, 2019 at 10:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > would work. The naming convention I was putting in didn't allow for > identical DB and user names, and it didn't seem to me that it was > worth overriding that convention to keep this particular test > result the same. The error output still proves that ecpg *tried* > to connect to the database named the same as the user, which is > the only real point of this test case as compared to its siblings. > And why not test the error path, anyway? Seems reasonable to me. > > I'm asking about this because I find that "make installcheck-world" > > sometimes fails at this point when run against my local installation > > -- the test "test connect/test" reliably fails. > > Um, you're confusing me ... is it "sometimes", or is it "reliably"? It's reliable. Doesn't have to be a Valgrind build, or even a debug build. > It does seem like there might be something to poke at here. We never > did figure out quite what the other reporter was seeing. > > Given the lack of overlap of user and database names, we can be > pretty darn certain that the test case shouldn't accidentally > succeed due to timing issues. I wonder if there could be some kind > of uninitialized-variable bug inside ecpg that lets it sometimes > connect to the wrong DB name (perhaps one it'd previously connected > to) if the DB name is omitted from the command? That seems possible. IIRC Valgrind has certain limitations when it comes to detecting stack corruption. -- Peter Geoghegan
Re: [COMMITTERS] pgsql: Establish conventions about global objectnames used in regressi
От
Peter Geoghegan
Дата:
On Sat, Sep 21, 2019 at 11:02 PM Peter Geoghegan <pg@bowt.ie> wrote: > It's reliable. Doesn't have to be a Valgrind build, or even a debug build. I can also confirm that the problem can't have anything to do with the patch that I happened to be testing -- the master branch is also affected. -- Peter Geoghegan
Peter Geoghegan <pg@bowt.ie> writes:
> On Sat, Sep 21, 2019 at 11:02 PM Peter Geoghegan <pg@bowt.ie> wrote:
>> It's reliable. Doesn't have to be a Valgrind build, or even a debug build.
> I can also confirm that the problem can't have anything to do with the
> patch that I happened to be testing -- the master branch is also
> affected.
Hm. So ... how come the buildfarm hasn't been red for the last three
years? We need to figure out what you're doing that's different.
regards, tom lane
Re: [COMMITTERS] pgsql: Establish conventions about global objectnames used in regressi
От
Peter Geoghegan
Дата:
On Sat, Sep 21, 2019 at 11:36 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Hm. So ... how come the buildfarm hasn't been red for the last three > years? We need to figure out what you're doing that's different. I'll try to debug the problem soon. The exact regressions.diffs I get is attached, just in case the exact details are interesting to somebody. -- Peter Geoghegan
Вложения
Re: [COMMITTERS] pgsql: Establish conventions about global objectnames used in regressi
От
Andres Freund
Дата:
Hi, On 2019-09-21 23:45:34 -0700, Peter Geoghegan wrote: > On Sat, Sep 21, 2019 at 11:36 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Hm. So ... how come the buildfarm hasn't been red for the last three > > years? We need to figure out what you're doing that's different. > > I'll try to debug the problem soon. > > The exact regressions.diffs I get is attached, just in case the exact > details are interesting to somebody. https://www.postgresql.org/message-id/20180318205548.2akxjqvo7hrk5wbc%40alap3.anarazel.de Greetings, Andres Freund