Thread: pgwin32_safestat weirdness
I'm confused about the current state of the pgwin32_safestat stuff. Cygwin is now building happily, but MinGW is now broken on libpq. It looks like libpq now needs dirmod.o or maybe libpgport.a. What I really don't understand though is why MinGW is broken but MSVC isn't. cheers andrew
Andrew Dunstan wrote: > > I'm confused about the current state of the pgwin32_safestat stuff. > Cygwin is now building happily, but MinGW is now broken on libpq. It > looks like libpq now needs dirmod.o or maybe libpgport.a. What I > really don't understand though is why MinGW is broken but MSVC isn't. I don't know why MSVC survives that without digging deeper, but the original patch had the redefine only if !defined(FRONTEND), but that seems to have been lost in Toms fix and it's now always redefined. Tom - was there a reason it now runs in FRONTEND as well, or was that an oversight? //Magnus
Andrew Dunstan <andrew@dunslane.net> writes:
> I'm confused about the current state of the pgwin32_safestat stuff.
Me too. I tried to fix it a couple of days ago, but seem to have only
moved the problem around :-(
> Cygwin is now building happily, but MinGW is now broken on libpq. It
> looks like libpq now needs dirmod.o or maybe libpgport.a. What I really
> don't understand though is why MinGW is broken but MSVC isn't.
I don't think we should import dirmod.o into libpq; it's too big.
I suggest either
(1) Assume that we don't need "safe" stat for frontend code, and
compile the safestat stuff only when !defined(FRONTEND)
(2) Split safestat into its own file and include that in libpq.
I'm not touching it myself though, since I have no way to test it.
regards, tom lane
Magnus Hagander <magnus@hagander.net> writes:
> Tom - was there a reason it now runs in FRONTEND as well, or was that
> an oversight?
I did do that intentionally because I was worried about "frontend"
code maybe expecting stat to work fully. Like pg_standby for example.
I think the immediate problem is that libpq uses stat() as well,
and depending on your link rules that might mean that safestat
actually has to be bound into libpq.
I would not have a problem with assuming that libpq will never care
about st_size being right, but I'm a lot more nervous about making
that presumption for all FRONTEND code.
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > > Cygwin is now building happily, but MinGW is now broken on libpq. > > It looks like libpq now needs dirmod.o or maybe libpgport.a. What I > > really don't understand though is why MinGW is broken but MSVC > > isn't. > > I don't think we should import dirmod.o into libpq; it's too big. Is it really big enough to matter? Where would you in general "draw the line" for including? > I suggest either > > (1) Assume that we don't need "safe" stat for frontend code, and > compile the safestat stuff only when !defined(FRONTEND) > > (2) Split safestat into its own file and include that in libpq. Is there not a (3) which has it included in all frontend code *except* libpq? Do we have a define to do that off? Because I agree with your comments in the other mail that there may be other frontend stuff that might need it. In libpq, it's only used in one place to check if a file is present, and one then in the SSL code to determine permissions and such (which means it's being ignored on win32). //Magnus
Magnus Hagander <magnus@hagander.net> writes:
> Is there not a (3) which has it included in all frontend code *except*
> libpq? Do we have a define to do that off?
Offhand I can't think of one.
> In libpq, it's only used in one place to check if a file is present,
> and one then in the SSL code to determine permissions and such (which
> means it's being ignored on win32).
Maybe we could finess the problem by tweaking libpq to not use stat()
at all on Windows.
regards, tom lane
Tom Lane wrote: > >> In libpq, it's only used in one place to check if a file is present, >> and one then in the SSL code to determine permissions and such (which >> means it's being ignored on win32). >> > > Maybe we could finess the problem by tweaking libpq to not use stat() > at all on Windows. > > > I would be quite happy with that, but before we go down that path I'd like to know why the MSVC builds aren't failing now from this when the MinGW builds are. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> I would be quite happy with that, but before we go down that path I'd
> like to know why the MSVC builds aren't failing now from this when the
> MinGW builds are.
Maybe the MSVC linker is willing to bind libpq's call to a safestat copy
extracted from libpgport.a in the surrounding program --- IOW, it works
only for calling programs that include libpgport, but all ours do.
regards, tom lane
Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: > > I would be quite happy with that, but before we go down that path > > I'd like to know why the MSVC builds aren't failing now from this > > when the MinGW builds are. > > Maybe the MSVC linker is willing to bind libpq's call to a safestat > copy extracted from libpgport.a in the surrounding program --- IOW, > it works only for calling programs that include libpgport, but all > ours do. Actually, on msvc we link libpq against libpgport, and not the individual files. Since we have a defined export list, it should remove all the unused functions automatically. //Magnus
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: > > Is there not a (3) which has it included in all frontend code > > *except* libpq? Do we have a define to do that off? > > Offhand I can't think of one. > > > In libpq, it's only used in one place to check if a file is present, > > and one then in the SSL code to determine permissions and such > > (which means it's being ignored on win32). > > Maybe we could finess the problem by tweaking libpq to not use stat() > at all on Windows. Seems that for the use in fe-connect.c, we could just use open() instead of stat() if we care. In fe-secure.c we'd have to have a bit more code since we'd use open() or so on Win32 to test it, and stat() on unix (because on Unix we need the output from stat). Shouldn't be too hard to do, but I keep thinking it'd be cleaner to just not do the redefine when building libpq. It means we'd add a define like BUILDING_LIBPQ or something to the libpq Makefile, and exclude the redefine if set. I can go with either way, though ;-) //Magnus
Magnus Hagander <magnus@hagander.net> writes:
> Shouldn't be too hard to do, but I keep thinking it'd be cleaner to
> just not do the redefine when building libpq. It means we'd add a
> define like BUILDING_LIBPQ or something to the libpq Makefile, and
> exclude the redefine if set.
+1 for that general approach, but let's call the macro something
like UNSAFE_STAT_OKAY. If the day ever comes that we need safestat
inside libpq, or more likely that we want to exclude it from some
other piece of code, it'll be clearer what to do.
regards, tom lane
Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: > > Shouldn't be too hard to do, but I keep thinking it'd be cleaner to > > just not do the redefine when building libpq. It means we'd add a > > define like BUILDING_LIBPQ or something to the libpq Makefile, and > > exclude the redefine if set. > > +1 for that general approach, but let's call the macro something > like UNSAFE_STAT_OKAY. If the day ever comes that we need safestat > inside libpq, or more likely that we want to exclude it from some > other piece of code, it'll be clearer what to do. Hmm. I thought BUILDING_LIBPQ would be the more generic one, since we might want to control other stuff from it. I recall wanting that define at some point in the past, but I can't recall why... :-) But - I'll do it with UNSAFE_STAT_OK if that's what ppl want. And then a simple ifeq() section in libpq Makefile, right? Or we could have libpq define the BUILDING_LIBPQ, and have a header say #ifdef BUILDING_LIBPQ / #define UNSAFE_STAT_OK / #endif.... That would certainly be the most flexible, but maybe not the prettiest solution until such time as we actually need it. //Magnus
Magnus Hagander wrote: > Tom Lane wrote: > >> Magnus Hagander <magnus@hagander.net> writes: >> >>> Shouldn't be too hard to do, but I keep thinking it'd be cleaner to >>> just not do the redefine when building libpq. It means we'd add a >>> define like BUILDING_LIBPQ or something to the libpq Makefile, and >>> exclude the redefine if set. >>> >> +1 for that general approach, but let's call the macro something >> like UNSAFE_STAT_OKAY. If the day ever comes that we need safestat >> inside libpq, or more likely that we want to exclude it from some >> other piece of code, it'll be clearer what to do. >> > > Hmm. I thought BUILDING_LIBPQ would be the more generic one, since we > might want to control other stuff from it. I recall wanting that define > at some point in the past, but I can't recall why... :-) > > But - I'll do it with UNSAFE_STAT_OK if that's what ppl want. And then > a simple ifeq() section in libpq Makefile, right? > > Or we could have libpq define the BUILDING_LIBPQ, and have a header say > #ifdef BUILDING_LIBPQ / #define UNSAFE_STAT_OK / #endif.... That would > certainly be the most flexible, but maybe not the prettiest solution > until such time as we actually need it. > > > I think a simple approach is all we need for now - not even sure we need an ifeq() section in the makefile. Here's a patch, which I'll apply unless there's an objection. cheers andrew Index: src/include/port.h =================================================================== RCS file: /cvsroot/pgsql/src/include/port.h,v retrieving revision 1.120 diff -c -r1.120 port.h *** src/include/port.h 11 Apr 2008 23:53:00 -0000 1.120 --- src/include/port.h 16 Apr 2008 12:16:01 -0000 *************** *** 287,294 **** * * We must pull in sys/stat.h here so the system header definition * goes in first, and we redefine that, and not the other way around. */ ! #if defined(WIN32) && !defined(__CYGWIN__) #include <sys/stat.h> extern int pgwin32_safestat(const char *path, struct stat *buf); #define stat(a,b) pgwin32_safestat(a,b) --- 287,297 ---- * * We must pull in sys/stat.h here so the system header definition * goes in first, and we redefine that, and not the other way around. + * + * Some frontends don't need the size from stat, so if UNSAFE_STAT_OK + * is defined we don't bother with this. */ ! #if defined(WIN32) && !defined(__CYGWIN__) && !defined(UNSAFE_STAT_OK) #include <sys/stat.h> extern int pgwin32_safestat(const char *path, struct stat *buf); #define stat(a,b) pgwin32_safestat(a,b) Index: src/interfaces/libpq/Makefile =================================================================== RCS file: /cvsroot/pgsql/src/interfaces/libpq/Makefile,v retrieving revision 1.165 diff -c -r1.165 Makefile *** src/interfaces/libpq/Makefile 7 Apr 2008 14:15:58 -0000 1.165 --- src/interfaces/libpq/Makefile 16 Apr 2008 12:16:01 -0000 *************** *** 19,25 **** SO_MAJOR_VERSION= 5 SO_MINOR_VERSION= 2 ! override CPPFLAGS := -DFRONTEND -I$(srcdir) $(CPPFLAGS) -I$(top_builddir)/src/port ifneq ($(PORTNAME), win32) override CFLAGS += $(PTHREAD_CFLAGS) endif --- 19,25 ---- SO_MAJOR_VERSION= 5 SO_MINOR_VERSION= 2 ! override CPPFLAGS := -DFRONTEND -DUNSAFE_STAT_OK -I$(srcdir) $(CPPFLAGS) -I$(top_builddir)/src/port ifneq ($(PORTNAME), win32) override CFLAGS += $(PTHREAD_CFLAGS) endif
Andrew Dunstan wrote: > > > Magnus Hagander wrote: > > Tom Lane wrote: > > > >> Magnus Hagander <magnus@hagander.net> writes: > >> > >>> Shouldn't be too hard to do, but I keep thinking it'd be cleaner > >>> to just not do the redefine when building libpq. It means we'd > >>> add a define like BUILDING_LIBPQ or something to the libpq > >>> Makefile, and exclude the redefine if set. > >>> > >> +1 for that general approach, but let's call the macro something > >> like UNSAFE_STAT_OKAY. If the day ever comes that we need safestat > >> inside libpq, or more likely that we want to exclude it from some > >> other piece of code, it'll be clearer what to do. > >> > > > > Hmm. I thought BUILDING_LIBPQ would be the more generic one, since > > we might want to control other stuff from it. I recall wanting that > > define at some point in the past, but I can't recall why... :-) > > > > But - I'll do it with UNSAFE_STAT_OK if that's what ppl want. And > > then a simple ifeq() section in libpq Makefile, right? > > > > Or we could have libpq define the BUILDING_LIBPQ, and have a header > > say #ifdef BUILDING_LIBPQ / #define UNSAFE_STAT_OK / #endif.... > > That would certainly be the most flexible, but maybe not the > > prettiest solution until such time as we actually need it. > > > > > > > > I think a simple approach is all we need for now - not even sure we > need an ifeq() section in the makefile. > > Here's a patch, which I'll apply unless there's an objection. Seems a reasonable step for now, yeah - we can add BUILDING_LIBPQ sometime in the future if we need it. However, you patch needs to set the define in the MSVC build as well, to make sure that the produced libpq.dll is equivalent in functionality. /Magnus
Magnus Hagander wrote: >> >> Here's a patch, which I'll apply unless there's an objection. >> > > Seems a reasonable step for now, yeah - we can add BUILDING_LIBPQ > sometime in the future if we need it. > > However, you patch needs to set the define in the MSVC build as well, > to make sure that the produced libpq.dll is equivalent in functionality. > > > Applied with this addition. cheers andrew
Magnus Hagander <magnus@hagander.net> writes:
> Andrew Dunstan wrote:
>> I think a simple approach is all we need for now - not even sure we
>> need an ifeq() section in the makefile.
>>
>> Here's a patch, which I'll apply unless there's an objection.
> Seems a reasonable step for now, yeah - we can add BUILDING_LIBPQ
> sometime in the future if we need it.
+1 from here too, modulo this:
> However, you patch needs to set the define in the MSVC build as well,
> to make sure that the produced libpq.dll is equivalent in functionality.
regards, tom lane