Horton

Thread

Commits

  1. pg_basebackup: Try to fix some failures on Windows.

  2. In get_bc_algorithm_name, add a dummy return statement.

  3. Unbreak the build.

  4. Replace BASE_BACKUP COMPRESSION_LEVEL option with COMPRESSION_DETAIL.

  1. pgsql: Unbreak the build.

    Robert Haas <rhaas@postgresql.org> — 2022-03-23T14:24:13Z

    Unbreak the build.

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

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/607e75e8f0f84544feb879b747da1d40fed71499

Modified Files
--------------
src/backend/replication/basebackup_lz4.c  | 3 +--
src/backend/replication/basebackup_zstd.c | 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)

  2. Re: pgsql: Unbreak the build.

    Andres Freund <andres@anarazel.de> — 2022-03-23T14:46:10Z

    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.

  3. Re: pgsql: Unbreak the build.

    Andres Freund <andres@anarazel.de> — 2022-03-23T14:58:18Z

    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.

  4. Re: pgsql: Unbreak the build.

    Robert Haas <robertmhaas@gmail.com> — 2022-03-23T15:38:39Z

    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

  5. Re: pgsql: Unbreak the build.

    Andres Freund <andres@anarazel.de> — 2022-03-23T15:49:48Z

    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

  6. Re: pgsql: Unbreak the build.

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-23T15:55:37Z

    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

  7. Re: pgsql: Unbreak the build.

    Robert Haas <robertmhaas@gmail.com> — 2022-03-23T16:03:35Z

    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

  8. Re: pgsql: Unbreak the build.

    Robert Haas <robertmhaas@gmail.com> — 2022-03-23T17:05:52Z

    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