Обсуждение: [HACKERS] pg_recvlogical.c doesn't build with --disable-integer-datetimes

Hi,

Since commit 7c030783a5bd07cadffc2a1018bc33119a4c7505 it seems that $SUBJECT.

pg_recvlogical.c:501:37: error: incompatible pointer types passing
'int64 *' (aka 'long *') to parameter of type 'TimestampTz *' (aka
'double *') [-Werror,-Wincompatible-pointer-types]
                                if (!flushAndSendFeedback(conn, &now))

pg_recvlogical.c:547:36: error: incompatible pointer types passing
'int64 *' (aka 'long *') to parameter of type 'TimestampTz *' (aka
'double *') [-Werror,-Wincompatible-pointer-types]
                        if (!flushAndSendFeedback(conn, &now))

pg_recvlogical.c:596:36: error: incompatible pointer types passing
'int64 *' (aka 'long *') to parameter of type 'TimestampTz *' (aka
'double *') [-Werror,-Wincompatible-pointer-types]
                        if (!flushAndSendFeedback(conn, &now))

Maybe the attached is the right fix for that?  (Sorry, I didn't test
because I haven't got around to learning how to drive this new stuff
yet.)

-- 
Thomas Munro
http://www.enterprisedb.com

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

Thomas Munro wrote:
> Hi,
> 
> Since commit 7c030783a5bd07cadffc2a1018bc33119a4c7505 it seems that $SUBJECT.

Added it to open items list.



-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Thomas Munro <thomas.munro@enterprisedb.com> writes:
> Since commit 7c030783a5bd07cadffc2a1018bc33119a4c7505 it seems that $SUBJECT.

Hmm ... I thought we had at least one buildfarm member still testing
--disable-integer-datetimes.  Evidently somebody needs to close that
gap.

> Maybe the attached is the right fix for that?

I'll look at this later today, if no one beats me to it.
        regards, tom lane

I wrote:
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
>> Since commit 7c030783a5bd07cadffc2a1018bc33119a4c7505 it seems that $SUBJECT.

> Hmm ... I thought we had at least one buildfarm member still testing
> --disable-integer-datetimes.  Evidently somebody needs to close that
> gap.

The plot thickens: we *do* have one such critter, mandrill, but for some
reason it's not generating any complaint about this.  Must be a very
lax compiler.

My own compilers don't generate errors either, only warnings.  Maybe
we could catch this sort of thing mechanically with a critter configured
with -Werror as well as --disable-integer-datetimes, but I'm a tad
hesitant to have a buildfarm machine configured with -Werror.  We'd
be raising the stakes quite a bit on keeping the code warning-free
for a specific compiler that might prove to be picky in silly ways.

But if it were a pretty mainstream compiler that hadn't shown much
evidence of wanting to throw useless warnings, maybe that's fine.

Thoughts?
        regards, tom lane

Tom Lane wrote:

> My own compilers don't generate errors either, only warnings.  Maybe
> we could catch this sort of thing mechanically with a critter configured
> with -Werror as well as --disable-integer-datetimes, but I'm a tad
> hesitant to have a buildfarm machine configured with -Werror.  We'd
> be raising the stakes quite a bit on keeping the code warning-free
> for a specific compiler that might prove to be picky in silly ways.
> 
> But if it were a pretty mainstream compiler that hadn't shown much
> evidence of wanting to throw useless warnings, maybe that's fine.
> 
> Thoughts?

Note that Thomas' compiler was using -Werror.

I think we're at a point where one gcc plus one clang animals set up
with -Werror would be a good thing to have, and keep green.  In
practice, this list fulfills the role of such a compiler, only with much
higher communication overhead.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>> My own compilers don't generate errors either, only warnings.  Maybe
>> we could catch this sort of thing mechanically with a critter configured
>> with -Werror as well as --disable-integer-datetimes, but I'm a tad
>> hesitant to have a buildfarm machine configured with -Werror.  We'd
>> be raising the stakes quite a bit on keeping the code warning-free
>> for a specific compiler that might prove to be picky in silly ways.
>> 
>> But if it were a pretty mainstream compiler that hadn't shown much
>> evidence of wanting to throw useless warnings, maybe that's fine.

> I think we're at a point where one gcc plus one clang animals set up
> with -Werror would be a good thing to have, and keep green.  In
> practice, this list fulfills the role of such a compiler, only with much
> higher communication overhead.

Yeah.  I have longfin which is running Apple's clang, and is on a machine
that doesn't have much to do otherwise.  I propose to turn on -Werror in
its configuration, and to configure a second critter on the same hardware
that runs with -Werror as well as --disable-integer-datetimes.  Somebody
else should do similarly with a reasonably modern/stable gcc release.

In the longer term, we might want to encourage more buildfarm owners to
use -Werror, but only with mainstream compilers that are generally pretty
sane about warnings.  Some of the older critters generate
hundreds/thousands of useless warnings; I'm not interested in trying to
suppress all of those.
        regards, tom lane

I wrote:
> Yeah.  I have longfin which is running Apple's clang, and is on a machine
> that doesn't have much to do otherwise.  I propose to turn on -Werror in
> its configuration, and to configure a second critter on the same hardware
> that runs with -Werror as well as --disable-integer-datetimes.  Somebody
> else should do similarly with a reasonably modern/stable gcc release.

So I tried doing that by adding -Werror to longfin's CFLAGS environment,
and it crashed and burned in configure.  The most obvious problem is
that we didn't bother to supply a prototype for does_int64_work() in
the probes for 64-bit ints.  However, even after fixing that, configure
produces totally bollixed output because -Werror breaks many of its
built-in tests.  We could fix the int64 tests but we have no good way of
fixing the built-in tests.

AFAICS, if you want to build with -Werror, you have to configure without
that and then inject it afterwards.  I wonder how people who use -Werror
are handling that.  Is it possible to do at all in a buildfarm critter?
        regards, tom lane

On February 17, 2017 11:40:57 AM PST, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>I wrote:
>> Yeah.  I have longfin which is running Apple's clang, and is on a
>machine
>> that doesn't have much to do otherwise.  I propose to turn on -Werror
>in
>> its configuration, and to configure a second critter on the same
>hardware
>> that runs with -Werror as well as --disable-integer-datetimes.
>Somebody
>> else should do similarly with a reasonably modern/stable gcc release.
>
>So I tried doing that by adding -Werror to longfin's CFLAGS
>environment,
>and it crashed and burned in configure.  The most obvious problem is
>that we didn't bother to supply a prototype for does_int64_work() in
>the probes for 64-bit ints.  However, even after fixing that, configure
>produces totally bollixed output because -Werror breaks many of its
>built-in tests.  We could fix the int64 tests but we have no good way
>of
>fixing the built-in tests.
>
>AFAICS, if you want to build with -Werror, you have to configure
>without
>that and then inject it afterwards.  I wonder how people who use
>-Werror
>are handling that.  Is it possible to do at all in a buildfarm critter?

Yea, I complained about that before. I think we'd have to add an extra configure option for it, because we rely in
settingit only for certain tests.   Personally I use CEXTRA (IIRC) to achieve that result after configure ran. 

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

> On Feb 17, 2017, at 11:40 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> I wrote:
>> Yeah.  I have longfin which is running Apple's clang, and is on a machine
>> that doesn't have much to do otherwise.  I propose to turn on -Werror in
>> its configuration, and to configure a second critter on the same hardware
>> that runs with -Werror as well as --disable-integer-datetimes.  Somebody
>> else should do similarly with a reasonably modern/stable gcc release.
> 
> So I tried doing that by adding -Werror to longfin's CFLAGS environment,
> and it crashed and burned in configure.  The most obvious problem is
> that we didn't bother to supply a prototype for does_int64_work() in
> the probes for 64-bit ints.  However, even after fixing that, configure
> produces totally bollixed output because -Werror breaks many of its
> built-in tests.  We could fix the int64 tests but we have no good way of
> fixing the built-in tests.
> 
> AFAICS, if you want to build with -Werror, you have to configure without
> that and then inject it afterwards.  I wonder how people who use -Werror
> are handling that.  Is it possible to do at all in a buildfarm critter?

I ran into the same problem when adding -Werror, and fixed it as follows:

diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index d39d6ca..96fbc60 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -240,7 +240,7 @@ endif # not PGXSCC = @CC@GCC = @GCC@SUN_STUDIO_CC = @SUN_STUDIO_CC@
-CFLAGS = @CFLAGS@
+CFLAGS = @CFLAGS@ -WerrorCFLAGS_VECTOR = @CFLAGS_VECTOR@CFLAGS_SSE42 = @CFLAGS_SSE42@
That adds -Werror to the compilation of sources but not to the compilation
of configure test programs.  I'd be happy to hear if anybody has a cleaner
way of doing this.

Mark Dilger

Andres Freund <andres@anarazel.de> writes:
> On February 17, 2017 11:40:57 AM PST, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> AFAICS, if you want to build with -Werror, you have to configure
>> without that and then inject it afterwards.  I wonder how people who
>> use -Werror are handling that.  Is it possible to do at all in a
>> buildfarm critter?

> Yea, I complained about that before. I think we'd have to add an extra configure option for it, because we rely in
settingit only for certain tests.   Personally I use CEXTRA (IIRC) to achieve that result after configure ran. 

[ pokes around... ]  Ah, that's called COPT, and it's entirely
undocumented :-(.  Probably ought to fix that.
        regards, tom lane

On Sat, Feb 18, 2017 at 9:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On February 17, 2017 11:40:57 AM PST, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> AFAICS, if you want to build with -Werror, you have to configure
>>> without that and then inject it afterwards.  I wonder how people who
>>> use -Werror are handling that.  Is it possible to do at all in a
>>> buildfarm critter?
>
>> Yea, I complained about that before. I think we'd have to add an extra configure option for it, because we rely in
settingit only for certain tests.   Personally I use CEXTRA (IIRC) to achieve that result after configure ran.
 
>
> [ pokes around... ]  Ah, that's called COPT, and it's entirely
> undocumented :-(.  Probably ought to fix that.

One way to set that up is like this:

$ cat src/Makefile.custom
COPT=-Wall -Werror $(CC_OPT)

-- 
Thomas Munro
http://www.enterprisedb.com

Thomas Munro <thomas.munro@enterprisedb.com> writes:
> On Sat, Feb 18, 2017 at 9:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> [ pokes around... ]  Ah, that's called COPT, and it's entirely
>> undocumented :-(.  Probably ought to fix that.

> One way to set that up is like this:

> $ cat src/Makefile.custom
> COPT=-Wall -Werror $(CC_OPT)

Well, we don't document Makefile.custom either, and probably now is
not the time to start.  I'm inclined to just explain that you can
set COPT in the environment of the "make" step to add flags that
you couldn't or didn't tell configure about.
        regards, tom lane

> On Feb 17, 2017, at 12:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 
> Thomas Munro <thomas.munro@enterprisedb.com> writes:
>> On Sat, Feb 18, 2017 at 9:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> [ pokes around... ]  Ah, that's called COPT, and it's entirely
>>> undocumented :-(.  Probably ought to fix that.
> 
>> One way to set that up is like this:
> 
>> $ cat src/Makefile.custom
>> COPT=-Wall -Werror $(CC_OPT)

Makefile.custom is deprecated per comment in src/Makefile.global, so
you'd at least have to remove or edit that comment.  It was enough to
scare me off using Makefile.custom as a solution for this when I was
working on getting -Werror into all my builds.

> Well, we don't document Makefile.custom either, and probably now is
> not the time to start.  I'm inclined to just explain that you can
> set COPT in the environment of the "make" step to add flags that
> you couldn't or didn't tell configure about.

There is a test in configure.in for whether the compiler is GCC, followed
by additional flags if so.  On my platform (Mac laptop) I was able to add
additional flags against HEAD without any of them actually triggering a
warning (though my last merge from HEAD was a while ago):

diff --git a/configure.in b/configure.in
index b9831bc..d6e7e24 100644
--- a/configure.in
+++ b/configure.in
@@ -446,6 +446,24 @@ fiif test "$GCC" = yes -a "$ICC" = no; then  CFLAGS="-Wall -Wmissing-prototypes -Wpointer-arith"
+  PGAC_PROG_CC_CFLAGS_OPT([-Wempty-body])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wignored-qualifiers])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wimplicit-fallthrough])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wtype-limits])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wuninitialized])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wshift-negative-value])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wmissing-include-dirs])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wshift-overflow])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wswitch-default])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wdangling-else])
+  PGAC_PROG_CC_CFLAGS_OPT([-Waggregate-return])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wstrict-prototypes])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wmissing-declarations])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wredundant-decls])
+  PGAC_PROG_CC_CFLAGS_OPT([-Winline])
+  PGAC_PROG_CC_CFLAGS_OPT([-Woverlength-strings])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wc++-compat])
+  PGAC_PROG_CC_CFLAGS_OPT([-Wold-style-definition])  # These work in some but not all gcc versions
PGAC_PROG_CC_CFLAGS_OPT([-Wdeclaration-after-statement]) PGAC_PROG_CC_CFLAGS_OPT([-Wendif-labels])
 
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index d39d6ca..96fbc60 100644
--- a/src/Makefile.global.in
+++ b/src/Makefile.global.in
@@ -240,7 +240,7 @@ endif # not PGXSCC = @CC@GCC = @GCC@SUN_STUDIO_CC = @SUN_STUDIO_CC@
-CFLAGS = @CFLAGS@
+CFLAGS = @CFLAGS@ -WerrorCFLAGS_VECTOR = @CFLAGS_VECTOR@CFLAGS_SSE42 = @CFLAGS_SSE42@


How about we add (some of) these extra warnings, plus -Werror,
in a section that is only active for platforms/compilers where we
know there aren't spurious warnings?  That would make detecting
unintentionally introduced warnings simpler, without the use of
COPT.  Perhaps where the compiler is GCC or CLANG, and the
platform is x86_64 redhat, something like that?

Mark Dilger

On February 17, 2017 1:13:10 PM PST, Mark Dilger <hornschnorter@gmail.com> wrote:
>How about we add (some of) these extra warnings, plus -Werror,
>in a section that is only active for platforms/compilers where we
>know there aren't spurious warnings?  That would make detecting
>unintentionally introduced warnings simpler, without the use of
>COPT.  Perhaps where the compiler is GCC or CLANG, and the
>platform is x86_64 redhat, something like that?

Strongly against that. I do *not* want Werrror enabled during development. I have a lot of warnings enabled, but while
hackingI often have some of them triggering (e.g. an unused variable or static function).  Preventing me from compiling
inthose scenarios has no benefits.   Werror it's useful in automated scenarios (e.g. my pre push script compiles with
it),a lot less in interactive scenarios where you can just use make -s. 

Andres

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

Mark Dilger <hornschnorter@gmail.com> writes:
>  if test "$GCC" = yes -a "$ICC" = no; then
>    CFLAGS="-Wall -Wmissing-prototypes -Wpointer-arith"
> +  PGAC_PROG_CC_CFLAGS_OPT([-Wempty-body])
> +  PGAC_PROG_CC_CFLAGS_OPT([-Wignored-qualifiers])
> +  PGAC_PROG_CC_CFLAGS_OPT([-Wimplicit-fallthrough])
> +  PGAC_PROG_CC_CFLAGS_OPT([-Wtype-limits])
> +  PGAC_PROG_CC_CFLAGS_OPT([-Wuninitialized])
> +  PGAC_PROG_CC_CFLAGS_OPT([-Wshift-negative-value])
> +  PGAC_PROG_CC_CFLAGS_OPT([-Wmissing-include-dirs])
> +  PGAC_PROG_CC_CFLAGS_OPT([-Wshift-overflow])
> +  PGAC_PROG_CC_CFLAGS_OPT([-Wswitch-default])
> +  PGAC_PROG_CC_CFLAGS_OPT([-Wdangling-else])
> +  PGAC_PROG_CC_CFLAGS_OPT([-Waggregate-return])
> +  PGAC_PROG_CC_CFLAGS_OPT([-Wstrict-prototypes])
> +  PGAC_PROG_CC_CFLAGS_OPT([-Wmissing-declarations])
> +  PGAC_PROG_CC_CFLAGS_OPT([-Wredundant-decls])
> +  PGAC_PROG_CC_CFLAGS_OPT([-Winline])
> +  PGAC_PROG_CC_CFLAGS_OPT([-Woverlength-strings])
> +  PGAC_PROG_CC_CFLAGS_OPT([-Wc++-compat])
> +  PGAC_PROG_CC_CFLAGS_OPT([-Wold-style-definition])
>    # These work in some but not all gcc versions
>    PGAC_PROG_CC_CFLAGS_OPT([-Wdeclaration-after-statement])
>    PGAC_PROG_CC_CFLAGS_OPT([-Wendif-labels])

I'm not terribly for that.  For the most part, if a gcc warning isn't
included by '-Wall', there's a reason for it.  We could talk about
whether individual extra warnings are useful for the Postgres project,
but I don't want to suddenly move the goalposts to where we are
enforcing a bunch of pedantic warnings that different compiler versions
might not even interpret the same.  I'm particularly not for turning
them up in the way you suggest here, where violations would result in
buildfarm failures on machines running with -Werror.

Right now I'd rather focus on getting to where we can have -Werror on in
the buildfarm at all.  longfin says we've got work to do on that, at least
in the back branches.  It may be that we can't expect near-EOL branches to
always compile perfectly cleanly on newer compilers.
        regards, tom lane

I wrote:
> Right now I'd rather focus on getting to where we can have -Werror on in
> the buildfarm at all.  longfin says we've got work to do on that, at least
> in the back branches.  It may be that we can't expect near-EOL branches to
> always compile perfectly cleanly on newer compilers.

I found that longfin's compiler is happy with 9.4 and up as-is, and it
required only a few very small and safe back-patches to make 9.3 work
too.  However, it's looking like -Werror with 9.2 is a bridge too far.
The thing that seems untenable is that to make 9.2 compile clean, we would
have to back-patch commit 71450d7fd, along with a fair amount of follow-on
work, so that the compiler would understand that ereport(ERROR) doesn't
return.  I don't think we want to go that far --- and maybe introduce
portability issues for someone's ancient compiler --- for a branch that's
near EOL anyway.

I'll adjust longfin's configuration so that it only applies -Werror in
9.3 and up.
        regards, tom lane

On 2/17/17 16:13, Mark Dilger wrote:
> +  PGAC_PROG_CC_CFLAGS_OPT([-Wc++-compat])

If your compiler isn't warning about anything with that, then there is
something wrong with it.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> On Feb 17, 2017, at 3:37 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>
> On 2/17/17 16:13, Mark Dilger wrote:
>> +  PGAC_PROG_CC_CFLAGS_OPT([-Wc++-compat])
>
> If your compiler isn't warning about anything with that, then there is
> something wrong with it.


$ gcc --version
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.3.0 (clang-703.0.31)
Target: x86_64-apple-darwin15.6.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

Mark Dilger <hornschnorter@gmail.com> writes:
>> On Feb 17, 2017, at 3:37 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>> If your compiler isn't warning about anything with that, then there is
>> something wrong with it.

> $ gcc --version
> Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr
--with-gxx-include-dir=/usr/include/c++/4.2.1
> Apple LLVM version 7.3.0 (clang-703.0.31)
> Target: x86_64-apple-darwin15.6.0
> Thread model: posix
> InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

Interesting.  That option doesn't seem to do anything on my Mac's compiler
either (Apple LLVM version 8.0.0 (clang-800.0.42.1)).  However, it surely
spews a metric buttload of warnings with RHEL6's gcc.  Points up my
concern upthread about different peoples' compilers interpreting these
less-common switches differently.
        regards, tom lane