Обсуждение: Fixing warnings in back branches?
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
On Mon, Dec 14, 2015 at 10:20 AM, Andres Freund <andres@anarazel.de> 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
On 2015-12-14 10:55:05 +0000, Greg Stark wrote: > On Mon, Dec 14, 2015 at 10:20 AM, Andres Freund <andres@anarazel.de> 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
Andres Freund <andres@anarazel.de> writes: > On 2015-12-14 10:55:05 +0000, 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
On 2015-12-14 09:43:07 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2015-12-14 10:55:05 +0000, 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'.
On Mon, Dec 14, 2015 at 10:06 AM, Andres Freund <andres@anarazel.de> wrote: > On 2015-12-14 09:43:07 -0500, Tom Lane wrote: >> Andres Freund <andres@anarazel.de> writes: >> > On 2015-12-14 10:55:05 +0000, 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
On 2015-12-14 11:00:32 -0500, Robert Haas wrote: > On Mon, Dec 14, 2015 at 10:06 AM, Andres Freund <andres@anarazel.de> 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
On Tue, Dec 15, 2015 at 7:59 AM, Andres Freund <andres@anarazel.de> wrote: > On 2015-12-14 11:00:32 -0500, Robert Haas wrote: >> On Mon, Dec 14, 2015 at 10:06 AM, Andres Freund <andres@anarazel.de> 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
On 2015-12-15 08:13:06 -0500, Robert Haas wrote: > On Tue, Dec 15, 2015 at 7:59 AM, Andres Freund <andres@anarazel.de> 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.
On Tue, Dec 15, 2015 at 8:17 AM, Andres Freund <andres@anarazel.de> wrote: > On 2015-12-15 08:13:06 -0500, Robert Haas wrote: >> On Tue, Dec 15, 2015 at 7:59 AM, Andres Freund <andres@anarazel.de> 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
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
Andres Freund <andres@anarazel.de> 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
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.
On Tue, Dec 15, 2015 at 9:17 AM, Andres Freund <andres@anarazel.de> 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
On Tue, Dec 15, 2015 at 11:04:07AM -0500, Robert Haas wrote: > On Tue, Dec 15, 2015 at 9:17 AM, Andres Freund <andres@anarazel.de> 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