Re: [HACKERS] Fixing warnings in back branches?

[Robert Haas]

On Tue, Dec 15, 2015 at 9:17 AM, Andres Freund  wrote:
> On 2015-12-15 09:09:39 -0500, Tom Lane wrote:
>> In the end, if you're building an old branch, you should be doing it with
>> old tools.
>
> That I don't buy for even one second. Old branches are used in up2date
> environments in production. Absolutely regularly. apt.pg.o, yum.pg.o et
> al do provide them for that.

Really?  I'm kind of with Tom; I don't expect that keeping old
branches warning-free on new compilers is really workable.  I think
the situation today is actually better than it was a few years ago, at
least for me.  I get some warnings on older branches, but with other
toolchains I've used for PG hacking at other times, it was much worse.
I think that it might be worth back-patching some of the warning fixes
we've done would be a good idea.  Like this one:

-   if (!res || !res->cmdStatus || strncmp(res->cmdStatus, "INSERT
", 7) != 0)
+   if (!res || strncmp(res->cmdStatus, "INSERT ", 7) != 0)
return "";

I really don't see how back-patching that can hurt anything, and it
gets rid of a warning, so great.  But not all cases are going to be so
clear cut, and getting all supported back-branches to compile warning
free on every contributor's current toolchain sounds like a treadmill
I don't want to get on.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fixing warnings in back branches?

[Robert Haas]

On Tue, Dec 15, 2015 at 7:59 AM, Andres Freund  wrote:
> On 2015-12-14 11:00:32 -0500, Robert Haas wrote:
>> On Mon, Dec 14, 2015 at 10:06 AM, Andres Freund  wrote:
>> > to compile 9.1 without warnings. -Wincompatible-pointer-types is quite
>> > useful to detect problems. The rest indeed is pretty 'Meh'.
>>
>> IIUC, the main thing that causes incompatible pointer type warnings on
>> 9.1 is the conflation of FILE with gzFile in pg_dump and
>> pg_basebackup.
>
> Right.
>
>> Not sure exactly which commits fixed that offhand.
>
> d923125b77c5d698bb8107a533a21627582baa43 , but that doesn't fix the
> 'files' output format, which was removed in
> 19f45565f581ce605956c29586bfd277f6012eec

Well, we're not going to back-patch the removal of the files format, right?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fixing warnings in back branches?

[Robert Haas]

On Tue, Dec 15, 2015 at 8:17 AM, Andres Freund  wrote:
> On 2015-12-15 08:13:06 -0500, Robert Haas wrote:
>> On Tue, Dec 15, 2015 at 7:59 AM, Andres Freund  wrote:
>> > On 2015-12-14 11:00:32 -0500, Robert Haas wrote:
>> >> IIUC, the main thing that causes incompatible pointer type warnings on
>> >> 9.1 is the conflation of FILE with gzFile in pg_dump and
>> >> pg_basebackup.
>> >
>> > Right.
>> >
>> >> Not sure exactly which commits fixed that offhand.
>> >
>> > d923125b77c5d698bb8107a533a21627582baa43 , but that doesn't fix the
>> > 'files' output format, which was removed in
>> > 19f45565f581ce605956c29586bfd277f6012eec
>>
>> Well, we're not going to back-patch the removal of the files format, right?
>
> Obviously not.

So... that means we can't really get rid of these warnings on 9.1,
IIUC.  I agree it would be nice to do if this were no issue, but as it
is I'm inclined to think we should just live with it for the next ~10
months.  After that 9.1 will no longer be a supported branch.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fixing warnings in back branches?

[Andres Freund]

On 2015-12-15 08:53:25 -0500, Robert Haas wrote:
> So... that means we can't really get rid of these warnings on 9.1,
> IIUC.

Well, we could fix them. Or, as proposed here, just silence that
category.


> I agree it would be nice to do if this were no issue, but as it
> is I'm inclined to think we should just live with it for the next ~10
> months.  After that 9.1 will no longer be a supported branch.

I think that's an ok one-off policy. But looking back it was pretty much
always the case that the release -3 or so started to look pretty
horrible, warning wise.

Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fixing warnings in back branches?

[Andres Freund]

On 2015-12-14 11:00:32 -0500, Robert Haas wrote:
> On Mon, Dec 14, 2015 at 10:06 AM, Andres Freund  wrote:
> > to compile 9.1 without warnings. -Wincompatible-pointer-types is quite
> > useful to detect problems. The rest indeed is pretty 'Meh'.
>
> IIUC, the main thing that causes incompatible pointer type warnings on
> 9.1 is the conflation of FILE with gzFile in pg_dump and
> pg_basebackup.

Right.

> Not sure exactly which commits fixed that offhand.

d923125b77c5d698bb8107a533a21627582baa43 , but that doesn't fix the
'files' output format, which was removed in
19f45565f581ce605956c29586bfd277f6012eec

Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fixing warnings in back branches?

[Andres Freund]

On 2015-12-15 08:13:06 -0500, Robert Haas wrote:
> On Tue, Dec 15, 2015 at 7:59 AM, Andres Freund  wrote:
> > On 2015-12-14 11:00:32 -0500, Robert Haas wrote:
> >> IIUC, the main thing that causes incompatible pointer type warnings on
> >> 9.1 is the conflation of FILE with gzFile in pg_dump and
> >> pg_basebackup.
> >
> > Right.
> >
> >> Not sure exactly which commits fixed that offhand.
> >
> > d923125b77c5d698bb8107a533a21627582baa43 , but that doesn't fix the
> > 'files' output format, which was removed in
> > 19f45565f581ce605956c29586bfd277f6012eec
> 
> Well, we're not going to back-patch the removal of the files format, right?

Obviously not.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fixing warnings in back branches?

[Tom Lane]

Andres Freund  writes:
> I think that's an ok one-off policy. But looking back it was pretty much
> always the case that the release -3 or so started to look pretty
> horrible, warning wise.

I think that's a condition of life.  The compilers are moving targets,
no matter that they allegedly implement standards.  We endeavor to keep
HEAD able to compile warning-free on recent compilers, but I don't think
we can make such a promise for back branches.  This thread offers a great
example of why not: the changes required would sometimes be too invasive
to be justifiable.

In the end, if you're building an old branch, you should be doing it with
old tools.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fixing warnings in back branches?

[Andres Freund]

On 2015-12-15 09:09:39 -0500, Tom Lane wrote:
> In the end, if you're building an old branch, you should be doing it with
> old tools.

That I don't buy for even one second. Old branches are used in up2date
environments in production. Absolutely regularly. apt.pg.o, yum.pg.o et
al do provide them for that.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fixing warnings in back branches?

[Noah Misch]

On Tue, Dec 15, 2015 at 11:04:07AM -0500, Robert Haas wrote:
> On Tue, Dec 15, 2015 at 9:17 AM, Andres Freund  wrote:
> > On 2015-12-15 09:09:39 -0500, Tom Lane wrote:
> >> In the end, if you're building an old branch, you should be doing it with
> >> old tools.

I grant that's the most risk-averse choice, because each release probably does
get the most testing with compilers contemporary to its dot-zero release date.
Your statement takes a step beyond neglecting warnings.  It is sound advice
for packagers, but it is overkill for the average end user.  We don't
emphasize the similar risks that apply to new libc, Perl, kernels, etc.  You
have back-patched changes to let new compilers build working/optimal code,
e.g. 649839d and e709807; that's great to continue doing.  Acquiring an old
compiler is tedious, and the risk of being the first to encounter a new
miscompilation is low.

> I think that it might be worth back-patching some of the warning fixes
> we've done would be a good idea.  Like this one:
> 
> -   if (!res || !res->cmdStatus || strncmp(res->cmdStatus, "INSERT
> ", 7) != 0)
> +   if (!res || strncmp(res->cmdStatus, "INSERT ", 7) != 0)
> return "";
> 
> I really don't see how back-patching that can hurt anything, and it
> gets rid of a warning, so great.  But not all cases are going to be so
> clear cut, and getting all supported back-branches to compile warning
> free on every contributor's current toolchain sounds like a treadmill
> I don't want to get on.

Agreed.  We don't have or need a firm ban on back-branch warning fixes, but
let's continue to cite the low value of contributions having that aim.

nm


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Fixing warnings in back branches?

[Andres Freund]

Hi,

While newer branches are at the moment mostly free of warnings for me,
the picture is entirely different in the older back branches, especially
9.1. That has several hundred lines of warnings.

I personally am not bothered by a handful of spurious warnings in the
back branches, but at this point you're very unlikely to see a new
warning introduced into 9.1 while backpatching.

Should we perhaps fix the worst offenders?

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fixing warnings in back branches?

[Greg Stark]

On Mon, Dec 14, 2015 at 10:20 AM, Andres Freund  wrote:
> I personally am not bothered by a handful of spurious warnings in the
> back branches, but at this point you're very unlikely to see a new
> warning introduced into 9.1 while backpatching.

These are new warnings older compilers didn't detect? Perhaps just
adding some -Wno-* flags would make more sense than changing code and
possibly introducing bugs.

-- 
greg


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fixing warnings in back branches?

[Andres Freund]

On 2015-12-14 10:55:05 +, Greg Stark wrote:
> On Mon, Dec 14, 2015 at 10:20 AM, Andres Freund  wrote:
> > I personally am not bothered by a handful of spurious warnings in the
> > back branches, but at this point you're very unlikely to see a new
> > warning introduced into 9.1 while backpatching.
> 
> These are new warnings older compilers didn't detect?

Yes. We fixed most of them since, but that doesn't help much when
compiling 9.1.

> Perhaps just adding some -Wno-* flags would make more sense than
> changing code and possibly introducing bugs.

I think that's a case-by-case decision. Just verbatimly backpatching
something that stewed in master for a year or two seems fine. That's imo
often preferrable because often it's just that existing warning
categories grew more "vigilant", or however you want to describe it. So
if you disable those, you also remove coverage...

Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fixing warnings in back branches?

[Andres Freund]

On 2015-12-14 09:43:07 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > On 2015-12-14 10:55:05 +, Greg Stark wrote:
> >> Perhaps just adding some -Wno-* flags would make more sense than
> >> changing code and possibly introducing bugs.
> 
> > I think that's a case-by-case decision. Just verbatimly backpatching
> > something that stewed in master for a year or two seems fine. That's imo
> > often preferrable because often it's just that existing warning
> > categories grew more "vigilant", or however you want to describe it. So
> > if you disable those, you also remove coverage...
> 
> Meh.  If we thought that anything like that was an actual bug, we should
> have back-patched the fix when removing the warning in HEAD.  So I would
> expect that all remaining warnings are just compiler nannyism, and thus
> that fixing them is more likely to introduce bugs than do anything very
> useful.

I'm more concerned about removing warnings that help detect problems
when backpatching. Right now I need
  -Wno-incompatible-pointer-types \
  -Wno-type-limits \
  -Wno-unused-but-set-variable \
  -Wno-empty-body \
  -Wno-address

to compile 9.1 without warnings. -Wincompatible-pointer-types is quite
useful to detect problems. The rest indeed is pretty 'Meh'.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fixing warnings in back branches?

[Robert Haas]

On Mon, Dec 14, 2015 at 10:06 AM, Andres Freund  wrote:
> On 2015-12-14 09:43:07 -0500, Tom Lane wrote:
>> Andres Freund  writes:
>> > On 2015-12-14 10:55:05 +, Greg Stark wrote:
>> >> Perhaps just adding some -Wno-* flags would make more sense than
>> >> changing code and possibly introducing bugs.
>>
>> > I think that's a case-by-case decision. Just verbatimly backpatching
>> > something that stewed in master for a year or two seems fine. That's imo
>> > often preferrable because often it's just that existing warning
>> > categories grew more "vigilant", or however you want to describe it. So
>> > if you disable those, you also remove coverage...
>>
>> Meh.  If we thought that anything like that was an actual bug, we should
>> have back-patched the fix when removing the warning in HEAD.  So I would
>> expect that all remaining warnings are just compiler nannyism, and thus
>> that fixing them is more likely to introduce bugs than do anything very
>> useful.
>
> I'm more concerned about removing warnings that help detect problems
> when backpatching. Right now I need
>   -Wno-incompatible-pointer-types \
>   -Wno-type-limits \
>   -Wno-unused-but-set-variable \
>   -Wno-empty-body \
>   -Wno-address
>
> to compile 9.1 without warnings. -Wincompatible-pointer-types is quite
> useful to detect problems. The rest indeed is pretty 'Meh'.

IIUC, the main thing that causes incompatible pointer type warnings on
9.1 is the conflation of FILE with gzFile in pg_dump and
pg_basebackup.  Not sure exactly which commits fixed that offhand.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Fixing warnings in back branches?

[Tom Lane]

Andres Freund  writes:
> On 2015-12-14 10:55:05 +, Greg Stark wrote:
>> Perhaps just adding some -Wno-* flags would make more sense than
>> changing code and possibly introducing bugs.

> I think that's a case-by-case decision. Just verbatimly backpatching
> something that stewed in master for a year or two seems fine. That's imo
> often preferrable because often it's just that existing warning
> categories grew more "vigilant", or however you want to describe it. So
> if you disable those, you also remove coverage...

Meh.  If we thought that anything like that was an actual bug, we should
have back-patched the fix when removing the warning in HEAD.  So I would
expect that all remaining warnings are just compiler nannyism, and thus
that fixing them is more likely to introduce bugs than do anything very
useful.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers