Thread: Cygwin PostgreSQL ESQL Patch

Cygwin PostgreSQL ESQL Patch

From
Jason Tishler
Date:
The first part of the attached patch solves the Cygwin PostgreSQL ESQL
problem reported by Michael Lemke.  See the Cygwin mailing list thread
that begins with the following:

    http://sources.redhat.com/ml/cygwin/2001-04/msg00827.html

The second part of the patch tweaks

    src/interfaces/ecpg/test/Makefile

so the test ESQL programs build under Cygwin too.  The most notable
changes cause the libraries to be listed after the sources file on
the cc/gcc command lines.

Note that I tried the patch under Red Hat 6.2 Linux without any ill
effects.

Thanks,
Jason

--
Jason Tishler
Director, Software Engineering       Phone: +1 (732) 264-8770 x235
Dot Hill Systems Corp.               Fax:   +1 (732) 264-8798
82 Bethany Road, Suite 7             Email: Jason.Tishler@dothill.com
Hazlet, NJ 07730 USA                 WWW:   http://www.dothill.com

Attachment

Re: [PATCHES] Cygwin PostgreSQL ESQL Patch

From
Tom Lane
Date:
Jason Tishler <Jason.Tishler@dothill.com> writes:
> --- include/sqlca.h    2001/03/22 06:16:20    1.16
> +++ include/sqlca.h    2001/04/20 14:15:01
> @@ -1,6 +1,12 @@
>  #ifndef POSTGRES_SQLCA_H
>  #define POSTGRES_SQLCA_H

> +#ifdef __CYGWIN__
> +#define DLLIMPORT __declspec (dllimport)
> +#else
> +#define DLLIMPORT
> +#endif /* __CYGWIN__ */
> +
>  #define SQLERRMC_LEN    70

This seems rather bad --- why should sqlca.h define this symbol?  What
happens if sqlca.h is included in a file that also includes other
Postgres includes, which will probably pull in c.h where the real
definition is?  I'd suggest adding

    #include "postgres_ext.h"
    #include "c.h"

instead, which may be more namespace pollution than we'd like, but
at least it does not create a potential definition conflict.

            regards, tom lane

Re: [PATCHES] Cygwin PostgreSQL ESQL Patch

From
Jason Tishler
Date:
Tom,

On Fri, Apr 20, 2001 at 12:43:30PM -0400, Tom Lane wrote:
> Jason Tishler <Jason.Tishler@dothill.com> writes:
> > --- include/sqlca.h    2001/03/22 06:16:20    1.16
> > +++ include/sqlca.h    2001/04/20 14:15:01
> > @@ -1,6 +1,12 @@
> >  #ifndef POSTGRES_SQLCA_H
> >  #define POSTGRES_SQLCA_H
>
> > +#ifdef __CYGWIN__
> > +#define DLLIMPORT __declspec (dllimport)
> > +#else
> > +#define DLLIMPORT
> > +#endif /* __CYGWIN__ */
> > +
> >  #define SQLERRMC_LEN    70
>
> This seems rather bad

Sorry, but I *did* try to start a discussion to determine the best way
to resolve this issue.  See the following for details:

    http://postgresql.readysetnet.com/mhonarc/pgsql-cygwin/2001-04/msg00004.html

Since no one responded, I took my best WAG.

> --- why should sqlca.h define this symbol?  What
> happens if sqlca.h is included in a file that also includes other
> Postgres includes, which will probably pull in c.h where the real
> definition is?

I never thought about this possibility -- I erroneously thought that
sqlca.h was used by ESQL only.

> I'd suggest adding
>
>     #include "postgres_ext.h"
>     #include "c.h"
>
> instead, which may be more namespace pollution than we'd like, but
> at least it does not create a potential definition conflict.

In the above mentioned URL, I suggested including c.h as another
possibly better solution.  However, I saw the following in ecpglib.h:

    /*
     * this is a small part of c.h since we don't want to leak all postgres
     * definitions into ecpg programs
     */

so I decided against including c.h in the first version of my patch.

Nevertheless, I appreciate your guidance and I will redo my patch as
you suggested.

Thanks,
Jason

--
Jason Tishler
Director, Software Engineering       Phone: +1 (732) 264-8770 x235
Dot Hill Systems Corp.               Fax:   +1 (732) 264-8798
82 Bethany Road, Suite 7             Email: Jason.Tishler@dothill.com
Hazlet, NJ 07730 USA                 WWW:   http://www.dothill.com

Re: [PATCHES] Cygwin PostgreSQL ESQL Patch

From
Tom Lane
Date:
Jason Tishler <Jason.Tishler@dothill.com> writes:
>> --- why should sqlca.h define this symbol?  What
>> happens if sqlca.h is included in a file that also includes other
>> Postgres includes, which will probably pull in c.h where the real
>> definition is?

> I never thought about this possibility -- I erroneously thought that
> sqlca.h was used by ESQL only.

AFAICT sqlca.h will be included by some (if not all) ecpg client
programs.  I'm not sure whether we should expect that such clients
might also include other Postgres headers.  However, sqlca.h is also
included by several modules of ecpg itself, which certainly do include
a ton of Postgres headers.  So there is a distinct risk that this will
fail when using a compiler that complains about redefinition of macros.

> In the above mentioned URL, I suggested including c.h as another
> possibly better solution.  However, I saw the following in ecpglib.h:
>     /*
>      * this is a small part of c.h since we don't want to leak all postgres
>      * definitions into ecpg programs
>      */
> so I decided against including c.h in the first version of my patch.

That's a fair point.  Maybe it should be

+#ifndef DLLIMPORT
+#ifdef __CYGWIN__
+#define DLLIMPORT __declspec (dllimport)
+#else
+#define DLLIMPORT
+#endif /* __CYGWIN__ */
+#endif /* DLLIMPORT */

            regards, tom lane

Re: [PATCHES] Cygwin PostgreSQL ESQL Patch

From
Jason Tishler
Date:
On Fri, Apr 20, 2001 at 02:38:53PM -0400, Tom Lane wrote:
> Jason Tishler <Jason.Tishler@dothill.com> writes:
> > In the above mentioned URL, I suggested including c.h as another
> > possibly better solution.  However, I saw the following in ecpglib.h:
> >     /*
> >      * this is a small part of c.h since we don't want to leak all postgres
> >      * definitions into ecpg programs
> >      */
> > so I decided against including c.h in the first version of my patch.
>
> That's a fair point.  Maybe it should be
>
> +#ifndef DLLIMPORT
> +#ifdef __CYGWIN__
> +#define DLLIMPORT __declspec (dllimport)
> +#else
> +#define DLLIMPORT
> +#endif /* __CYGWIN__ */
> +#endif /* DLLIMPORT */

I will redo my patch with the modification suggested above.

Thanks,
Jason

--
Jason Tishler
Director, Software Engineering       Phone: +1 (732) 264-8770 x235
Dot Hill Systems Corp.               Fax:   +1 (732) 264-8798
82 Bethany Road, Suite 7             Email: Jason.Tishler@dothill.com
Hazlet, NJ 07730 USA                 WWW:   http://www.dothill.com

Re: [PATCHES] Cygwin PostgreSQL ESQL Patch

From
Jason Tishler
Date:
Tom,

On Fri, Apr 20, 2001 at 03:38:56PM -0400, Jason Tishler wrote:
> On Fri, Apr 20, 2001 at 02:38:53PM -0400, Tom Lane wrote:
> > That's a fair point.  Maybe it should be
> >
> > +#ifndef DLLIMPORT
> > +#ifdef __CYGWIN__
> > +#define DLLIMPORT __declspec (dllimport)
> > +#else
> > +#define DLLIMPORT
> > +#endif /* __CYGWIN__ */
> > +#endif /* DLLIMPORT */
>
> I will redo my patch with the modification suggested above.

I have redone my patch incorporating the above plus I fixed a glaring
hole now I know that sqlca.h is included by more then ESQL clients.
Without the src/makefiles/Makefile.win part, ecpg.dll no longer built
under Cygwin.

This time I have rebuilt everything from scratch on both Linux and
Cygwin without any errors.  I have also rerun the regression tests and
all passed.

I apologize for the sloppiness of my previous patch -- that will not
happen again.

Thanks,
Jason

--
Jason Tishler
Director, Software Engineering       Phone: +1 (732) 264-8770 x235
Dot Hill Systems Corp.               Fax:   +1 (732) 264-8798
82 Bethany Road, Suite 7             Email: Jason.Tishler@dothill.com
Hazlet, NJ 07730 USA                 WWW:   http://www.dothill.com

Attachment

Re: [PATCHES] Cygwin PostgreSQL ESQL Patch

From
Jason Tishler
Date:
Tom,

What is the status of the following patch:

    http://postgresql.readysetnet.com/mhonarc/pgsql-cygwin/2001-04/msg00031.html

Is it possible to get it into 7.1.1?

Thanks,
Jason

--
Jason Tishler
Director, Software Engineering       Phone: +1 (732) 264-8770 x235
Dot Hill Systems Corp.               Fax:   +1 (732) 264-8798
82 Bethany Road, Suite 7             Email: Jason.Tishler@dothill.com
Hazlet, NJ 07730 USA                 WWW:   http://www.dothill.com

Re: [PATCHES] Cygwin PostgreSQL ESQL Patch

From
Tom Lane
Date:
Jason Tishler <Jason.Tishler@dothill.com> writes:
> What is the status of the following patch:
>     http://postgresql.readysetnet.com/mhonarc/pgsql-cygwin/2001-04/msg00031.html

I recall voicing unhappiness about putting a definition of DLLIMPORT
into sqlca.h, independent of the main one in c.h.  Not sure if there's
any good way around that, though.  Including c.h would be a cure worse
than the disease, probably (application namespace pollution and all that).

The Makefile changes look OK to me but Peter E. would be more qualified
to comment on them.

            regards, tom lane

Re: [PATCHES] Cygwin PostgreSQL ESQL Patch

From
Jason Tishler
Date:
Tom,

On Thu, May 03, 2001 at 09:15:52AM -0400, Tom Lane wrote:
> Jason Tishler <Jason.Tishler@dothill.com> writes:
> > What is the status of the following patch:
> >     http://postgresql.readysetnet.com/mhonarc/pgsql-cygwin/2001-04/msg00031.html
>
> I recall voicing unhappiness about putting a definition of DLLIMPORT
> into sqlca.h, independent of the main one in c.h.  Not sure if there's
> any good way around that, though.  Including c.h would be a cure worse
> than the disease, probably (application namespace pollution and all that).

See the following to refresh your memory:

On Fri, Apr 20, 2001 at 02:38:53PM -0400, Tom Lane wrote:
> Jason Tishler <Jason.Tishler@dothill.com> writes:
> > In the above mentioned URL, I suggested including c.h as another
> > possibly better solution.  However, I saw the following in ecpglib.h:
> >     /*
> >      * this is a small part of c.h since we don't want to leak all postgres
> >      * definitions into ecpg programs
> >      */
> > so I decided against including c.h in the first version of my patch.
>
> That's a fair point.  Maybe it should be
>
> +#ifndef DLLIMPORT
> +#ifdef __CYGWIN__
> +#define DLLIMPORT __declspec (dllimport)
> +#else
> +#define DLLIMPORT
> +#endif /* __CYGWIN__ */
> +#endif /* DLLIMPORT */

The current version of my patch is just doing what you suggested.  Are you
no longer recommending this approach as the best compromise?

> The Makefile changes look OK to me but Peter E. would be more qualified
> to comment on them.

Let's see what Peter has to say about the Makefile parts.

Thanks,
Jason

--
Jason Tishler
Director, Software Engineering       Phone: +1 (732) 264-8770 x235
Dot Hill Systems Corp.               Fax:   +1 (732) 264-8798
82 Bethany Road, Suite 7             Email: Jason.Tishler@dothill.com
Hazlet, NJ 07730 USA                 WWW:   http://www.dothill.com

Re: [PATCHES] Cygwin PostgreSQL ESQL Patch

From
Tom Lane
Date:
Jason Tishler <Jason.Tishler@dothill.com> writes:
> The current version of my patch is just doing what you suggested.  Are you
> no longer recommending this approach as the best compromise?

I don't have a better idea ... but I thought I'd mention the point
again, just in case someone else can think of one.

>> The Makefile changes look OK to me but Peter E. would be more qualified
>> to comment on them.

> Let's see what Peter has to say about the Makefile parts.

Peter, if those changes look OK would you commit the patch?  Or if
you're short of time today, let me know and I'll do it.  Need to get
7.1.1 wrapped up ...

            regards, tom lane

Re: [PATCHES] Cygwin PostgreSQL ESQL Patch

From
Peter Eisentraut
Date:
Tom Lane writes:

> Peter, if those changes look OK would you commit the patch?  Or if
> you're short of time today, let me know and I'll do it.  Need to get
> 7.1.1 wrapped up ...

Please commit and wrap ahead.  IMO, the #ifdef DLLIMPORT should stay in
sqlca.h, but the #ifdef __CYGWIN__ should probably outside of it (not
inside).

--
Peter Eisentraut   peter_e@gmx.net   http://funkturm.homeip.net/~peter



Re: [PATCHES] Cygwin PostgreSQL ESQL Patch

From
Tom Lane
Date:
Peter Eisentraut <peter_e@gmx.net> writes:
> Please commit and wrap ahead.  IMO, the #ifdef DLLIMPORT should stay in
> sqlca.h, but the #ifdef __CYGWIN__ should probably outside of it (not
> inside).

Huh?  The macro has to get defined as *something* --- for non-CYGWIN
it must become defined as empty.  Jason's code looks OK to me.

I applied the patch as it stood ... if you think you can improve the
style then go ahead ...

            regards, tom lane

Re: [PATCHES] Cygwin PostgreSQL ESQL Patch

From
Jason Tishler
Date:
Tom,

On Thu, May 03, 2001 at 12:15:46PM -0400, Tom Lane wrote:
> I applied the patch as it stood ...

Thanks!

Jason

--
Jason Tishler
Director, Software Engineering       Phone: +1 (732) 264-8770 x235
Dot Hill Systems Corp.               Fax:   +1 (732) 264-8798
82 Bethany Road, Suite 7             Email: Jason.Tishler@dothill.com
Hazlet, NJ 07730 USA                 WWW:   http://www.dothill.com