Re: 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(at)dunslane(dot)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(at)hagander(dot)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(at)dunslane(dot)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(at)hagander(dot)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(at)dunslane(dot)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(at)dunslane(dot)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(at)hagander(dot)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(at)hagander(dot)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(at)hagander(dot)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(at)hagander(dot)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

Andrew Dunstan wrote:
>
>
> Magnus Hagander wrote:
> > Tom Lane wrote:
> >
> >> Magnus Hagander <magnus(at)hagander(dot)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(at)hagander(dot)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