pgsql: Unbreak the build.

Hi,

On March 23, 2022 7:24:13 AM PDT, Robert Haas <rhaas@postgresql.org> wrote:

Unbreak the build.

Commit ffd53659c46a54a6978bcb8c4424c1e157a2c0f1 broke the build for
anyone not compiling with LZ4 and ZSTD enabled. Woops.

There's new warnings that sound reasonable introduced in the prior commit that didn't get removed in this one:

https://cirrus-ci.com/task/5259487073271808?logs=mingw_cross_warning#L392

- Andres

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Hi,

On March 23, 2022 7:46:10 AM PDT, Andres Freund <andres@anarazel.de> wrote:

Hi,

On March 23, 2022 7:24:13 AM PDT, Robert Haas <rhaas@postgresql.org> wrote:

Unbreak the build.

Commit ffd53659c46a54a6978bcb8c4424c1e157a2c0f1 broke the build for
anyone not compiling with LZ4 and ZSTD enabled. Woops.

There's new warnings that sound reasonable introduced in the prior commit that didn't get removed in this one:

https://cirrus-ci.com/task/5259487073271808?logs=mingw_cross_warning#L392

And windows still fails tests after this commit: https://cirrus-ci.com/task/6424123323711488?logs=test_bin#L22

Andres

--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

On Wed, Mar 23, 2022 at 10:46 AM Andres Freund <andres@anarazel.de> wrote:

There's new warnings that sound reasonable introduced in the prior commit that didn't get removed in this one:

https://cirrus-ci.com/task/5259487073271808?logs=mingw_cross_warning#L392

That link takes me to a screen that shows no warnings. Scrolling
around the only thing I see that doesn't seem to be addressed by this
commit is a complaint about get_bc_algorithm_name falling off the end.
I've added a dummy return statement to hopefully address that.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 2022-03-23 11:38:39 -0400, Robert Haas wrote:

On Wed, Mar 23, 2022 at 10:46 AM Andres Freund <andres@anarazel.de> wrote:

There's new warnings that sound reasonable introduced in the prior commit that didn't get removed in this one:

https://cirrus-ci.com/task/5259487073271808?logs=mingw_cross_warning#L392

That link takes me to a screen that shows no warnings.

Hm, apparently copied the link with slightly off line numbers. Odd.

Scrolling around the only thing I see that doesn't seem to be addressed by
this commit is a complaint about get_bc_algorithm_name falling off the end.

Well, there's also the test failure on windows...

I've added a dummy return statement to hopefully address that.

Assert(false); won't help a compiler to see the path is unreachable when
building without assertions. Might be nicer to use pg_unreachable().

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

And windows still fails tests after this commit: https://cirrus-ci.com/task/6424123323711488?logs=test_bin#L22

Yeah. drongo is reporting

# Running: pg_basebackup --no-sync -cfast -D C:\\prog\\bf\\root\\HEAD\\pgsql.build\\src\\bin\\pg_basebackup\\tmp_check\\tmp_test_vv4i/tarbackup -Ft
Assertion failed: 0, file c:\\prog\\bf\\root\\HEAD\\pgsql.build\\src\\bin\\pg_basebackup\\walmethods.c, line 953
not ok 82 - tar format

# Failed test 'tar format'
# at t/010_pg_basebackup.pl line 261.

which is pointing at

/* not reachable */
Assert(false);

so it's not so unreachable after all.

regards, tom lane

On Wed, Mar 23, 2022 at 11:55 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andres Freund <andres@anarazel.de> writes:

And windows still fails tests after this commit: https://cirrus-ci.com/task/6424123323711488?logs=test_bin#L22

Yeah. drongo is reporting

# Running: pg_basebackup --no-sync -cfast -D C:\\prog\\bf\\root\\HEAD\\pgsql.build\\src\\bin\\pg_basebackup\\tmp_check\\tmp_test_vv4i/tarbackup -Ft
Assertion failed: 0, file c:\\prog\\bf\\root\\HEAD\\pgsql.build\\src\\bin\\pg_basebackup\\walmethods.c, line 953
not ok 82 - tar format

# Failed test 'tar format'
# at t/010_pg_basebackup.pl line 261.

which is pointing at

/* not reachable */
Assert(false);

so it's not so unreachable after all.

I'm looking into this now, but that's not the same Assert(false). The
one Andres is talking about is at the end of get_bc_algorithm_name().
This one is in tar_open_for_write(). AFAIK, the first of these is
actually unreachable or at least I see no evidence that we are
reaching it. The second is clearly reachable because we're failing the
assertion. I thought that might be because I didn't test
--without-zlib locally, and indeed in testing that just now, I found
another unused variable warning which I need to fix. But, that doesn't
account for this failure, because when I correct the problem with the
unused variable, all the tests pass.

I think what likely happened here is that in reorganizing some of the
logic in basebackup.c, I caused COMPRESSION_GZIP to get passed to
tar_open_for_write() even when HAVE_LIBZ is not defined. But I don't
yet understand why it only happens on Windows. I am suspicious that
the problem is in basebackup.c's main() function, but I haven't
pinpointed it yet.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Wed, Mar 23, 2022 at 12:03 PM Robert Haas <robertmhaas@gmail.com> wrote:

I'm looking into this now, but that's not the same Assert(false). The
one Andres is talking about is at the end of get_bc_algorithm_name().
This one is in tar_open_for_write(). AFAIK, the first of these is
actually unreachable or at least I see no evidence that we are
reaching it. The second is clearly reachable because we're failing the
assertion. I thought that might be because I didn't test
--without-zlib locally, and indeed in testing that just now, I found
another unused variable warning which I need to fix. But, that doesn't
account for this failure, because when I correct the problem with the
unused variable, all the tests pass.

I think what likely happened here is that in reorganizing some of the
logic in basebackup.c, I caused COMPRESSION_GZIP to get passed to
tar_open_for_write() even when HAVE_LIBZ is not defined. But I don't
yet understand why it only happens on Windows. I am suspicious that
the problem is in basebackup.c's main() function, but I haven't
pinpointed it yet.

Oh, it's not that: it's that Windows is using threads, and therefore
LogStreamerMain() is getting called with the wrong arguments. I guess
I just need to move the additional parameters that I added to
LogStreamerMain() into members in the logstreamer_param struct.

--
Robert Haas
EDB: http://www.enterprisedb.com