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

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

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

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

Andres Freund <andres(at)anarazel(dot)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

On 10/03/2017 04:43 PM, Tom Lane wrote:
> Andres Freund <andres(at)anarazel(dot)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

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

> 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

Michael Meskes <meskes(at)postgresql(dot)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

> 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

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(at)sss(dot)pgh(dot)pa(dot)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, bool EuroDates);
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

> 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

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