pgsql: Implement pipeline mode in libpq

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

Implement pipeline mode in libpq

So jacana and fairywren are unhappy about this, and the failure reports
are totally uninformative:

Mar 15 21:14:33 cd /home/pgrunner/bf/root/HEAD/pgsql.build/../pgsql/src/test/modules/libpq_pipeline && TESTDIR='/home/pgrunner/bf/root/HEAD/pgsql.build/src/test/modules/libpq_pipeline' PATH="/home/pgrunner/bf/root/HEAD/pgsql.build/tmp_install/home/pgrunner/bf/root/HEAD/inst/bin:$PATH" LD_LIBRARY_PATH="/home/pgrunner/bf/root/HEAD/pgsql.build/tmp_install/home/pgrunner/bf/root/HEAD/inst/lib:$LD_LIBRARY_PATH" PGPORT='66678' PG_REGRESS='/home/pgrunner/bf/root/HEAD/pgsql.build/src/test/modules/libpq_pipeline/../../../../src/test/regress/pg_regress' REGRESS_SHLIB='/home/pgrunner/bf/root/HEAD/pgsql.build/src/test/regress/regress.dll' /bin/prove -I /home/pgrunner/bf/root/HEAD/pgsql.build/../pgsql/src/test/perl/ -I /home/pgrunner/bf/root/HEAD/pgsql.build/../pgsql/src/test/modules/libpq_pipeline --timer t/*.pl
Mar 15 21:14:38 t/001_libpq_pipeline....
Mar 15 21:14:38 # Failed test 'libpq_pipeline '
Mar 15 21:14:38 # at t/001_libpq_pipeline.pl line 23.
Mar 15 21:14:38
Mar 15 21:14:38 # Failed test 'libpq_pipeline '
Mar 15 21:14:38 # at t/001_libpq_pipeline.pl line 23.
Mar 15 21:14:38
Mar 15 21:14:38 # Failed test 'libpq_pipeline '
Mar 15 21:14:38 # at t/001_libpq_pipeline.pl line 23.
Mar 15 21:14:40
Mar 15 21:14:40 # Failed test 'libpq_pipeline '
Mar 15 21:14:40 # at t/001_libpq_pipeline.pl line 23.
Mar 15 21:14:40
Mar 15 21:14:40 # Failed test 'libpq_pipeline '
Mar 15 21:14:40 # at t/001_libpq_pipeline.pl line 23.
Mar 15 21:14:40
Mar 15 21:14:40 # Failed test 'libpq_pipeline '
Mar 15 21:14:40 # at t/001_libpq_pipeline.pl line 23.
Mar 15 21:14:40
Mar 15 21:14:40 # Failed test 'libpq_pipeline '
Mar 15 21:14:40 # at t/001_libpq_pipeline.pl line 23.
Mar 15 21:14:41 # Looks like you planned 8 tests but ran 15.
Mar 15 21:14:41 # Looks like you failed 7 tests of 15 run.
Mar 15 21:14:41 dubious
Mar 15 21:14:41 Test returned status 7 (wstat 1792, 0x700)
Mar 15 21:14:41 DIED. FAILED tests 2, 4, 6, 8-15
Mar 15 21:14:41 Failed 11/8 tests, -37.50% okay
Mar 15 21:14:41 Failed Test Stat Wstat Total Fail Failed List of Failed
Mar 15 21:14:41 -------------------------------------------------------------------------------
Mar 15 21:14:41 t/001_libpq_pipeline.pl 7 1792 8 14 175.00% 2 4 6 8-15
Mar 15 21:14:41 Failed 1/1 test scripts, 0.00% okay. 0/8 subtests failed, 100.00% okay.
Mar 15 21:14:41 make: *** [check] Error 7

I suspect that the "tests" output is including \r\n newlines
and the "split" operation is not handling that right.

regards, tom lane

On 2021-Mar-15, Tom Lane wrote:

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:

Implement pipeline mode in libpq

So jacana and fairywren are unhappy about this, and the failure reports
are totally uninformative:

Yeah. This is the main clue:

Mar 15 21:14:41 # Looks like you planned 8 tests but ran 15.
Mar 15 21:14:41 # Looks like you failed 7 tests of 15 run.

We called the program seven additional times with empty string, so the
split is emitting extra results that are empty strings.

I suspect that the "tests" output is including \r\n newlines
and the "split" operation is not handling that right.

That seems the right guess. We already use split(/\s+/) in other
scripts, particularly the Windows build ones, so that seems the correct
fix.

Pushed that fix.

--
�lvaro Herrera Valdivia, Chile
"I dream about dreams about dreams", sang the nightingale
under the pale moon (Sandman)

Hi Alvaro,

On Tue, 16 Mar 2021 at 10:20, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Implement pipeline mode in libpq

src/test/modules/libpq_pipeline/libpq_pipeline.c | 1303 ++++++++++++++++++++

I'm wondering if you meant to leave the "#define DEBUG" line at line
34 in the above file?

It seems pretty strange to do:

#define DEBUG
#ifdef DEBUG
#define pg_debug(...) do { fprintf(stderr, __VA_ARGS__); } while (0)
#else
#define pg_debug(...)
#endif

pg_debug will never be an empty macro.

I noticed this when testing compiling postgres with MSVC. The compiler
is kicking out a warning: 'DEBUG': macro redefinition, which will be
because that compiler defines DEBUG when doing non-production builds.

David

On Tue, 30 Mar 2021 at 20:19, David Rowley <dgrowleyml@gmail.com> wrote:

On Tue, 16 Mar 2021 at 10:20, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Implement pipeline mode in libpq

src/test/modules/libpq_pipeline/libpq_pipeline.c | 1303 ++++++++++++++++++++

I'm wondering if you meant to leave the "#define DEBUG" line at line
34 in the above file?

It seems pretty strange to do:

#define DEBUG
#ifdef DEBUG
#define pg_debug(...) do { fprintf(stderr, __VA_ARGS__); } while (0)
#else
#define pg_debug(...)
#endif

pg_debug will never be an empty macro.

I noticed this when testing compiling postgres with MSVC. The compiler
is kicking out a warning: 'DEBUG': macro redefinition, which will be
because that compiler defines DEBUG when doing non-production builds.

I'm still not really sure why this code does #define directly followed
by an #ifdef for that define. However, just to clear up the final
compiler warning that's currently being produced on MSVC, I've
attached a proposed patch to fix it.

None of the buildfarm animals are producing this warning as they all
seem to be building as "Release" builds.

libpq_pipeline.c on my machine is compiled with the following:

C:\Program Files (x86)\Microsoft Visual
Studio\2017\Community\VC\Tools\MSVC\14.16.27023\bin\HostX86\x64\CL.exe
/c /Isrc/include /Isrc/include/port/win32
/Isrc/include/port/win32_msvc /Isrc\interfaces\libpq /Zi /nologo /W3
/WX- /diagnostics:classic /Od /D WIN32 /D _WINDOWS /D __WINDOWS__ /D
__WIN32__ /D WIN32_STACK_RLIMIT=4194304 /D _CRT_SECURE_NO_DEPRECATE /D
_CRT_NONSTDC_NO_DEPRECATE /D _DEBUG /D DEBUG=1_MBCS /GF- /Gm- /EHsc
/MDd /GS /fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline
/Fo".\Debug\libpq_pipeline\\" /Fd".\Debug\libpq_pipeline\vc141.pdb"
/Gd /TC /wd4018 /wd4244 /wd4273 /wd4102 /wd4090 /wd4267 /FC
/errorReport:queue /MP
src/test/modules/libpq_pipeline/libpq_pipeline.c
libpq_pipeline.c

Note: /D DEBUG=1_MBCS

This causes:

Build succeeded.

"L:\Projects\Postgres\e\pgsql.sln" (default target) (1) ->
"L:\Projects\Postgres\e\libpq_pipeline.vcxproj" (default target) (88) ->
(ClCompile target) ->
l:\projects\postgres\e\src\test\modules\libpq_pipeline\libpq_pipeline.c(38):
warning C4005: 'DEBUG': macro redefinition
[L:\Projects\Postgres\e\libpq_pipeline.vcxproj]

1 Warning(s)
0 Error(s)

Whereas on hamerkop, they are [1]https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=hamerkop&amp;dt=2021-04-02%2010%3A18%3A13&amp;stg=make:

C:\\Program Files (x86)\\Microsoft Visual
Studio\\2017\\Community\\VC\\Tools\\MSVC\\14.15.26726\\bin\\HostX86\\x64\\CL.exe
/c /Isrc/include /Isrc/include/port/win32
/Isrc/include/port/win32_msvc /I"c:\\pkg\\zlib-dist\\include"
/I"c:\\OpenSSL-Win64\\include" /Isrc\\interfaces\\libpq /Zi /nologo
/W3 /WX- /diagnostics:classic /Ox /D WIN32 /D _WINDOWS /D __WINDOWS__
/D __WIN32__ /D WIN32_STACK_RLIMIT=4194304 /D _CRT_SECURE_NO_DEPRECATE
/D _CRT_NONSTDC_NO_DEPRECATE /D _MBCS /GF /Gm- /EHsc /MD /GS
/fp:precise /Zc:wchar_t /Zc:forScope /Zc:inline
/Fo".\\Release\\libpq_pipeline\\\\"
/Fd".\\Release\\libpq_pipeline\\vc141.pdb" /Gd /TC /wd4018 /wd4244
/wd4273 /wd4102 /wd4090 /wd4267 /FC /errorReport:queue /MP
src/test/modules/libpq_pipeline/libpq_pipeline.c
libpq_pipeline.c

Note the mention of "Release", indicating the Release rather than Debug build.

Does anyone want to comment on the #define just before the #ifdef?

David

[1]: https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=hamerkop&amp;dt=2021-04-02%2010%3A18%3A13&amp;stg=make

On 2021-Apr-06, David Rowley wrote:

I'm still not really sure why this code does #define directly followed
by an #ifdef for that define. However, just to clear up the final
compiler warning that's currently being produced on MSVC, I've
attached a proposed patch to fix it.

Hi David, sorry for failing to reply. (I actually did write a reply,
but my mail setup was broken at the time so it was lost. Sigh)

I did not intend to have the define defined at all times. Leaving it
there was an oversight. Let's just remove the #define line; users
interested in that output can turn it on when they need it. Removing it
means less clutter in the buildfarm output. Interesting conditions that
the test should check for should be tested and dealt with by using
pg_fatal, so that the test framework correctly detects a failure. The
pg_debug lines are there just to debug the test itself.

Now about the define being called DEBUG, that was just a silly
oversight; your suggestion to change it to DEBUG_OUTPUT is good, so
let's do that.

I +1 you pushing your patch.

Thanks,

--
�lvaro Herrera 39�49'30"S 73�17'W

On Wed, 7 Apr 2021 at 01:53, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

I did not intend to have the define defined at all times. Leaving it
there was an oversight. Let's just remove the #define line; users
interested in that output can turn it on when they need it. Removing it
means less clutter in the buildfarm output. Interesting conditions that
the test should check for should be tested and dealt with by using
pg_fatal, so that the test framework correctly detects a failure. The
pg_debug lines are there just to debug the test itself.

Now about the define being called DEBUG, that was just a silly
oversight; your suggestion to change it to DEBUG_OUTPUT is good, so
let's do that.

I +1 you pushing your patch.

Thanks for having a look at this.

I've just pushed a small fix to do the rename and remove the #define.

David