Thread: [HACKERS] datetime.h defines like PM conflict with external libraries

[HACKERS] datetime.h defines like PM conflict with external libraries

From
Andres Freund
Date:
Hi,

In my llvm jit work I'd to

#undef PM
/* include some llvm headers */
#define PM 1

because llvm has a number of functions which have an argument named PM.
Now that works, but it's fairly ugly. Perhaps it would be a good idea to
name these defines in a manner that's slightly less likely to conflict?

Alternatively we could use #pragma push_macro() around the includes, but
that'd be a new dependency.

Better ideas?

Greetings,

Andres Freund


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

Re: [HACKERS] datetime.h defines like PM conflict with externallibraries

From
Andrew Dunstan
Date:



On 10/03/2017 03:00 PM, Andres Freund wrote:
> Hi,
>
> In my llvm jit work I'd to
>
> #undef PM
> /* include some llvm headers */
> #define PM 1
>
> because llvm has a number of functions which have an argument named PM.
> Now that works, but it's fairly ugly. Perhaps it would be a good idea to
> name these defines in a manner that's slightly less likely to conflict?
>
> Alternatively we could use #pragma push_macro() around the includes, but
> that'd be a new dependency.
>
> Better ideas?
>


AFAICT at a quick glance these are only used in a couple of files. Maybe
the defs need to be floated off to a different header with more limited
inclusion?

cheers

andrew

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



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

Re: [HACKERS] datetime.h defines like PM conflict with externallibraries

From
Andres Freund
Date:
Hi,

On 2017-10-03 16:34:38 -0400, Andrew Dunstan wrote:
> On 10/03/2017 03:00 PM, Andres Freund wrote:
> > Hi,
> >
> > In my llvm jit work I'd to
> >
> > #undef PM
> > /* include some llvm headers */
> > #define PM 1
> >
> > because llvm has a number of functions which have an argument named PM.
> > Now that works, but it's fairly ugly. Perhaps it would be a good idea to
> > name these defines in a manner that's slightly less likely to conflict?
> >
> > Alternatively we could use #pragma push_macro() around the includes, but
> > that'd be a new dependency.
> >
> > Better ideas?

> AFAICT at a quick glance these are only used in a couple of files. Maybe
> the defs need to be floated off to a different header with more limited
> inclusion?

Why not just rename them to PG_PM etc? If we force potential external
users to do some changes, we can use more unique names just as well -
the effort to adapt won't be meaningfully higher... IMNSHO there's not
much excuse for defining macros like PM globally.

- Andres


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

Re: [HACKERS] datetime.h defines like PM conflict with external libraries

From
Tom Lane
Date:
Andres Freund <andres@anarazel.de> writes:
> On 2017-10-03 16:34:38 -0400, Andrew Dunstan wrote:
>> AFAICT at a quick glance these are only used in a couple of files. Maybe
>> the defs need to be floated off to a different header with more limited
>> inclusion?

> Why not just rename them to PG_PM etc? If we force potential external
> users to do some changes, we can use more unique names just as well -
> the effort to adapt won't be meaningfully higher... IMNSHO there's not
> much excuse for defining macros like PM globally.

I like the new-header-file idea because it will result in minimal code
churn and thus minimal back-patching hazards.

I do *not* like "PG_PM".  For our own purposes that adds no uniqueness
at all.  If we're to touch these symbols then I'd go for names like
"DATETIME_PM".  Or maybe "DT_PM" ... there's a little bit of precedent
for the DT_ prefix already.
        regards, tom lane


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

Re: [HACKERS] datetime.h defines like PM conflict with externallibraries

From
Andrew Dunstan
Date:

On 10/03/2017 04:43 PM, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
>> On 2017-10-03 16:34:38 -0400, Andrew Dunstan wrote:
>>> AFAICT at a quick glance these are only used in a couple of files. Maybe
>>> the defs need to be floated off to a different header with more limited
>>> inclusion?
>> Why not just rename them to PG_PM etc? If we force potential external
>> users to do some changes, we can use more unique names just as well -
>> the effort to adapt won't be meaningfully higher... IMNSHO there's not
>> much excuse for defining macros like PM globally.
> I like the new-header-file idea because it will result in minimal code
> churn and thus minimal back-patching hazards.
>
> I do *not* like "PG_PM".  For our own purposes that adds no uniqueness
> at all.  If we're to touch these symbols then I'd go for names like
> "DATETIME_PM".  Or maybe "DT_PM" ... there's a little bit of precedent
> for the DT_ prefix already.
>
>             


Yeah. If we use a prefix +1 for DT_. If we do that then I think they
should *all* be prefixed, not just the ones we know of conflicts for.

cheers

andrew

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



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

Re: [HACKERS] datetime.h defines like PM conflict with externallibraries

From
Alvaro Herrera
Date:
Andrew Dunstan wrote:

> On 10/03/2017 04:43 PM, Tom Lane wrote:

> > I like the new-header-file idea because it will result in minimal code
> > churn and thus minimal back-patching hazards.
> >
> > I do *not* like "PG_PM".  For our own purposes that adds no uniqueness
> > at all.  If we're to touch these symbols then I'd go for names like
> > "DATETIME_PM".  Or maybe "DT_PM" ... there's a little bit of precedent
> > for the DT_ prefix already.
> 
> Yeah. If we use a prefix +1 for DT_. If we do that then I think they
> should *all* be prefixed, not just the ones we know of conflicts for.

Maybe it'd be good idea to unify some of that stuff so that ecpg can use
it, too?  Having a second copy of the same stuff in
src/interfaces/ecpg/pgtypeslib/dt.h is pretty terrible.  Even if not,
let's make sure they don't diverge.

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


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

Re: [HACKERS] datetime.h defines like PM conflict with externallibraries

From
Michael Meskes
Date:
> Maybe it'd be good idea to unify some of that stuff so that ecpg can
> use
> it, too?  Having a second copy of the same stuff in
> src/interfaces/ecpg/pgtypeslib/dt.h is pretty terrible.  Even if not,
> let's make sure they don't diverge.

Please let's unify whatever we can. The fewer manual sync we need, the
better.

Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL


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

Re: [HACKERS] datetime.h defines like PM conflict with external libraries

From
Tom Lane
Date:
Michael Meskes <meskes@postgresql.org> writes:
>> Maybe it'd be good idea to unify some of that stuff so that ecpg can
>> use it, too?  Having a second copy of the same stuff in
>> src/interfaces/ecpg/pgtypeslib/dt.h is pretty terrible.  Even if not,
>> let's make sure they don't diverge.

> Please let's unify whatever we can. The fewer manual sync we need, the
> better.

Isn't pgtypeslib/*.h exposed to ecpg-using applications?
        regards, tom lane


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

Re: [HACKERS] datetime.h defines like PM conflict with externallibraries

From
Michael Meskes
Date:
> Isn't pgtypeslib/*.h exposed to ecpg-using applications?

No, the public interface is is include/*.h, pgtypeslib/*.h is only
internal.

Michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL


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

Re: [HACKERS] datetime.h defines like PM conflict with externallibraries

From
Andres Freund
Date:
Hi,

On 2017-10-04 11:36:56 +0200, Alvaro Herrera wrote:
> Andrew Dunstan wrote:
> > On 10/03/2017 04:43 PM, Tom Lane wrote:
> > > I like the new-header-file idea because it will result in minimal code
> > > churn and thus minimal back-patching hazards.

I'm not sure it's that little code churn, and the insulation isn't
great. Based on my WIP patch adding a DT_ prefix it would affect at
least:

 contrib/adminpack/adminpack.c      |   8 +-
 src/backend/parser/gram.y          |  52 ++++-----
 src/backend/utils/adt/date.c       |  50 ++++----
 src/backend/utils/adt/datetime.c   | 614
+++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------------
 src/backend/utils/adt/formatting.c |  10 +-
 src/backend/utils/adt/json.c       |   8 +-
 src/backend/utils/adt/nabstime.c   |  32 +++---
 src/backend/utils/adt/timestamp.c  | 196 +++++++++++++++----------------
 src/backend/utils/adt/xml.c        |   6 +-
 src/backend/utils/misc/tzparser.c  |   4 +-
 src/bin/pg_waldump/compat.c        |   6 +-
 src/include/utils/datetime.h       | 216 +++++++++++++++++------------------

so I'm not quite convinced this that well isolated pieces of code.  I
wonder if just moving the defines around won't primarily increase pain.


I have however, for now, worked around the need to deal with this
problem (by moving more stuff .c files that are careful about their
includes). So this is more about historical raisins, I do not have an
urgent need to work on this.


> > > I do *not* like "PG_PM".  For our own purposes that adds no uniqueness
> > > at all.  If we're to touch these symbols then I'd go for names like
> > > "DATETIME_PM".  Or maybe "DT_PM" ... there's a little bit of precedent
> > > for the DT_ prefix already.
> > 
> > Yeah. If we use a prefix +1 for DT_. If we do that then I think they
> > should *all* be prefixed, not just the ones we know of conflicts for.

Attached is a WIP patch doing exactly this conversion for
datetime.h. Note that we'd want to do something about ecpg's dt.h if we
were to go for the approach.

While the changes are fairly verbose, they're also mechanical, so I
suspect the issues around backpatching - not that that code changes that
much - wouldn't be too hard to resolve.


> Maybe it'd be good idea to unify some of that stuff so that ecpg can use
> it, too?  Having a second copy of the same stuff in
> src/interfaces/ecpg/pgtypeslib/dt.h is pretty terrible.  Even if not,
> let's make sure they don't diverge.

I agree that that would be quite an advantage. It's more than just
datetime.h that'd need to be usable by ecpg. Luckily timestamp.h would
probably be easy,
commit a7801b62f21bd051444bd1119cd3745ecc8e14ec
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   2011-09-09 13:23:41 -0400

    Move Timestamp/Interval typedefs and basic macros into datatype/timestamp.h.
provides the basics.  I suspect we'd want to do something very similar
for datetime?


I however wonder if even that would be really going far enough - we'd
still end up with a lot of copied functions:

int            DecodeInterval(char **, int *, int, int *, struct tm *, fsec_t *);
int            DecodeTime(char *, int *, struct tm *, fsec_t *);
void        EncodeDateTime(struct tm *tm, fsec_t fsec, bool print_tz, int tz, const char *tzn, int style, char *str,
boolEuroDates);
 
void        EncodeInterval(struct tm *tm, fsec_t fsec, int style, char *str);
int            tm2timestamp(struct tm *, fsec_t, int *, timestamp *);
int            DecodeUnits(int field, char *lowtoken, int *val);
bool        CheckDateTokenTables(void);
void        EncodeDateOnly(struct tm *tm, int style, char *str, bool EuroDates);
int            GetEpochTime(struct tm *);
int            ParseDateTime(char *, char *, char **, int *, int *, char **);
int            DecodeDateTime(char **, int *, int, int *, struct tm *, fsec_t *, bool);
void        j2date(int, int *, int *, int *);
void        GetCurrentDateTime(struct tm *);
int            date2j(int, int, int);
void        TrimTrailingZeros(char *);
void        dt2time(double, int *, int *, int *, fsec_t *);

I suspect starting to implement infrastructure to deal with would be a
bit bigger a task than I can chew of right now though.  Medium term, it
seems to me, we should start actually move a lot of the adt code into a
library that can be included (or possibly just compiled?) both by
frontend and backend code.  Which kinda seems to imply we'd need
compatible elog support for frontend code, which I'd wished for many
times.

Michael, is there any problem including datatype/* headers in ecpg that
are frontend clean? I see no such usage so far, that's why I'm asking.


Greetings,

Andres Freund

Attachment

Re: [HACKERS] datetime.h defines like PM conflict with externallibraries

From
Michael Meskes
Date:
> Michael, is there any problem including datatype/* headers in ecpg
> that
> are frontend clean? I see no such usage so far, that's why I'm
> asking.

When the pgtypes library was created we tried to include only the bits
and pieces needed to not create unnecessary hassles, but if it compiles
cleanly I'm fine either way. I'm assuming you're talking about
including the files for compiling ecpg, not as externally visible
header files, right?

michael
--
Michael Meskes
Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org
Jabber: michael at xmpp dot meskes dot org
VfL Borussia! Força Barça! SF 49ers! Use Debian GNU/Linux, PostgreSQL


Re: [HACKERS] datetime.h defines like PM conflict with externallibraries

From
Alvaro Herrera
Date:
Andres Freund wrote:

> Medium term, it
> seems to me, we should start actually move a lot of the adt code into a
> library that can be included (or possibly just compiled?) both by
> frontend and backend code.  Which kinda seems to imply we'd need
> compatible elog support for frontend code, which I'd wished for many
> times.

I remember looking into moving this code into src/common/ a couple of
years ago, but the API was not identical in ecpg than backend mostly
because of the use of a few GUC vars, so I didn't finish it.  In many
cases it seemed possible to resolve easily (just change hardcoded use of
a GUC in the function with a parameter), but I'm not sure it was the
case everywhere.

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