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

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