Thread: Add version macro to libpq-fe.h

I am making use of the new pipeline mode added to libpq in
PostgreSQL 14. At the same time I would still like to support
older libpq versions by not providing the extended functionality
that depends on this mode.

The natural way to achieve this in C/C++ is to conditionally
enable code that depends on the additional APIs based on the
preprocessor macro. And I could easily do this if libpq-fe.h
provided a macro containing its version.

Now, such a macro (PG_VERSION_NUM) is provided by pg_config.h
that normally accompanies libpq-fe.h. However, I don't believe
the presence of this file is guaranteed. All the documentation
says[1] about headers is this:

"Client programs that use libpq must include the header file 
libpq-fe.h and must link with the libpq library."

And there are good reasons why packagers of libpq may decide to
omit this header (in a nutshell, it embeds target architecture-
specific information, see this discussion for background[2]). And
I may not want to include it in my code (it defines a lot of free-
named macros that may clash with my names).

So I am wondering if it would make sense to provide a better way
to obtain the libpq version as a macro?

To me, as a user, the simplest way would be to have such a macro
defined by libpq-fe.h. This would also provide a reasonable
fallback for previous versions: if this macro is not defined, I
know I am dealing with version prior to 14 and if I need to know
which exactly I can try to include pg_config.h (perhaps with the
help of __has_include if I am using C++).

If simply moving this macro to libpq-fe.h is not desirable (for
example, because it is auto-generated), then perhaps we could
move this (and a few other version-related macros[3]) to a
separate header (for example, libpq-version.h) and either include
it from libpq-fe.h or define a macro in libpq-fe.h that signals
its presence (e.g., PG_HAS_VERSION or some such).

What do you think?


[1] https://www.postgresql.org/docs/9.3/libpq.html

[2] https://bugzilla.redhat.com/show_bug.cgi?id=828467

[3] PG_MAJORVERSION
    PG_MAJORVERSION_NUM
    PG_MINORVERSION_NUM
    PG_VERSION
    PG_VERSION_NUM
    PG_VERSION_STR (this one includes target so maybe leave it in pg_config.h)

Boris Kolpackov <boris@codesynthesis.com> writes:
> I am making use of the new pipeline mode added to libpq in
> PostgreSQL 14. At the same time I would still like to support
> older libpq versions by not providing the extended functionality
> that depends on this mode.

Good point.

> The natural way to achieve this in C/C++ is to conditionally
> enable code that depends on the additional APIs based on the
> preprocessor macro. And I could easily do this if libpq-fe.h
> provided a macro containing its version.

I think putting a version number as such in there is a truly
horrid idea.  However, I could get behind adding a boolean flag
that says specifically whether the pipeline feature exists.
Then you'd do something like

#ifdef LIBPQ_HAS_PIPELINING

rather than embedding knowledge of exactly which release
added that.

            regards, tom lane

On Thu, Jun 17, 2021 at 9:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I think putting a version number as such in there is a truly
> horrid idea.  However, I could get behind adding a boolean flag
> that says specifically whether the pipeline feature exists.
> Then you'd do something like
>
> #ifdef LIBPQ_HAS_PIPELINING
>
> rather than embedding knowledge of exactly which release
> added that.

I realize that this kind of feature-based testing is generally
considered a best practice, but the problem is we're unlikely to do it
consistently. If we put a version number in there, people will be able
to test for whatever they want.

Then again, why would pg_config.h be absent?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Jun 17, 2021 at 9:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think putting a version number as such in there is a truly
>> horrid idea.  However, I could get behind adding a boolean flag
>> that says specifically whether the pipeline feature exists.

> I realize that this kind of feature-based testing is generally
> considered a best practice, but the problem is we're unlikely to do it
> consistently. If we put a version number in there, people will be able
> to test for whatever they want.

We don't really add major new APIs to libpq very often, so I don't
find that too compelling.  I do find it compelling that user code
shouldn't embed knowledge about "feature X arrived in version Y".

> Then again, why would pg_config.h be absent?

Likely because somebody decided it was a server-side include rather
than an application-side include.

A more critical point is that if pg_config is present, it'll likely
contain the server version, which might not have a lot to do with the
libpq version.  Debian's already shipping things in a way that decouples
those, and I gather Red Hat is moving in that direction too.

I think what people really want to know is "if I try to call
PQenterPipelineMode, will that compile?".  Comparing v13 and v14
libpq-fe.h, I see that there is a solution available now:
"#ifdef PQ_QUERY_PARAM_MAX_LIMIT".  But depending on that seems
like a bit of a hack, because I'm not sure that it's directly tied
to the pipelining feature.

            regards, tom lane

> On 17 Jun 2021, at 19:16, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> A more critical point is that if pg_config is present, it'll likely
> contain the server version, which might not have a lot to do with the
> libpq version.  Debian's already shipping things in a way that decouples
> those, and I gather Red Hat is moving in that direction too.
>
> I think what people really want to know is "if I try to call
> PQenterPipelineMode, will that compile?".

I think this is the most compelling argument for feature-based gating rather
than promote version based.  +1 on doing "#ifdef LIBPQ_HAS_PIPELINING" or along
those lines and try to be consistent going forward.  If we've truly failed to
do so in X releases time, then we can revisit this.

--
Daniel Gustafsson        https://vmware.com/

On Thu, Jun 17, 2021 at 1:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> We don't really add major new APIs to libpq very often, so I don't
> find that too compelling.  I do find it compelling that user code
> shouldn't embed knowledge about "feature X arrived in version Y".

I just went and looked at how exports.txt has evolved over the years.
Since PostgreSQL 8.1, every release except for 9.4 and 11 added at
least one new function to libpq. That means in 14 releases we've done
something that might break someone's compile 12 times. Now maybe you
want to try to argue that few of those changes are "major," but I
don't know how that could be a principled argument. Every new function
is something someone may want to use, and thus a potential compile
break.

Some of those releases also changed behavior. For example, version 10
allowed multi-host connection strings and URLs. People might want to
know about that sort of thing, too.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Hi,

On 2021-06-17 13:16:17 -0400, Tom Lane wrote:
> > Then again, why would pg_config.h be absent?
> 
> Likely because somebody decided it was a server-side include rather
> than an application-side include.

Which is the right call - pg_config.h can't easily be included in
applications that themselves use autoconf. Most problematically it
defines all the standard autotools PACKAGE_* macros that are guaranteed
to conflict in any autotools using project. There's obviously also a lot
of other defines in there that quite possibly could conflict.

We probably split pg_config.h at some point. Even for extensions it can
be annoying because pg_config.h is always included in server code, which
means that the extension can't easily include an autoheader style header
itself.


I'm not sure I understand why you think that exposing the version number
for libpq is such a bad idea?


I think it'd be reasonable to add a few more carefully chosen macros to
pg_config_ext.h.

Greetings,

Andres Freund

Robert Haas <robertmhaas@gmail.com> writes:
> I just went and looked at how exports.txt has evolved over the years.
> Since PostgreSQL 8.1, every release except for 9.4 and 11 added at
> least one new function to libpq. That means in 14 releases we've done
> something that might break someone's compile 12 times. Now maybe you
> want to try to argue that few of those changes are "major," but I
> don't know how that could be a principled argument. Every new function
> is something someone may want to use, and thus a potential compile
> break.

Interesting, but then you have to explain why this is the first time
that somebody has asked for a version number in libpq-fe.h.  Maybe
all those previous additions were indeed minor enough that the
problem didn't come up.  (Another likely possibility, perhaps, is
that people have been misusing the server version for this purpose,
and have been lucky enough to not have that approach fail for them.)

Anyway, I do not see why we can't establish a principle going forward
that new additions to libpq's API should involve at least one macro,
so that they can be checked for with #ifdefs.  Just because the
version-number approach offloads work from us doesn't make it a good
idea, because the work doesn't vanish; it will be dumped in the laps
of packagers and end users.

BTW, by that principle, we should likely be adding a symbol
associated with the new tracing features, as well as one for
pipelining.  Or is it good enough to tell people they can
check "#ifdef PQTRACE_SUPPRESS_TIMESTAMPS" ?

            regards, tom lane

Andres Freund <andres@anarazel.de> writes:
> I'm not sure I understand why you think that exposing the version number
> for libpq is such a bad idea?
> I think it'd be reasonable to add a few more carefully chosen macros to
> pg_config_ext.h.

The primary problem I've got with that is the risk of confusion
between server and libpq version numbers.  In particular, if we do
it like that then we've just totally screwed the Debian packagers.
They will have to choose whether to install pg_config_ext.h from
their server build or their libpq build.  Both choices are wrong,
depending on what applications want to know.

Now we could alternatively invent a libpq_version.h and hope that
packagers remember to install the right version of that.  But I
think it's a better user experience all around to do it the other
way.

            regards, tom lane

Hi,

On 2021-06-17 14:41:40 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I'm not sure I understand why you think that exposing the version number
> > for libpq is such a bad idea?
> > I think it'd be reasonable to add a few more carefully chosen macros to
> > pg_config_ext.h.
> 
> The primary problem I've got with that is the risk of confusion
> between server and libpq version numbers.  In particular, if we do
> it like that then we've just totally screwed the Debian packagers.
> They will have to choose whether to install pg_config_ext.h from
> their server build or their libpq build.  Both choices are wrong,
> depending on what applications want to know.

That's a fair point.

However, we kind of already force them to do so - libpq already depends
on pg_config_ext.h, so they need to deal with the issue in some
form. It's not particularly likely to lead to a problem to have a
mismatching pg_config_ext.h, though, so maybe that's not too bad.

Our make install actually forsees the issue to some degree, and installs
pg_config_ext.h in two places, which then debian builds on:

$ apt-file search pg_config_ext.h
libpq-dev: /usr/include/postgresql/pg_config_ext.h
postgresql-server-dev-13: /usr/include/postgresql/13/server/pg_config_ext.h
postgresql-server-dev-14: /usr/include/postgresql/14/server/pg_config_ext.h

Greetings,

Andres Freund

Tom Lane <tgl@sss.pgh.pa.us> writes:

> Robert Haas <robertmhaas@gmail.com> writes:
>> I just went and looked at how exports.txt has evolved over the years.
>> Since PostgreSQL 8.1, every release except for 9.4 and 11 added at
>> least one new function to libpq. That means in 14 releases we've done
>> something that might break someone's compile 12 times. Now maybe you
>> want to try to argue that few of those changes are "major," but I
>> don't know how that could be a principled argument. Every new function
>> is something someone may want to use, and thus a potential compile
>> break.
>
> Interesting, but then you have to explain why this is the first time
> that somebody has asked for a version number in libpq-fe.h.  Maybe
> all those previous additions were indeed minor enough that the
> problem didn't come up.  (Another likely possibility, perhaps, is
> that people have been misusing the server version for this purpose,
> and have been lucky enough to not have that approach fail for them.)

FWIW, the perl DBD::Pg module extracts the version number from
`pg_config --version` at build time, and uses that to define a
PGLIBVERSION which is used to define fatal fallbacks for a few
functions:

https://metacpan.org/release/TURNSTEP/DBD-Pg-3.15.0/source/dbdimp.c#L26-55

I have an unfinished branch which does similar for PQsetSingleRowMode,
(added in 9.2).

- ilmari

On Thu, Jun 17, 2021 at 2:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Interesting, but then you have to explain why this is the first time
> that somebody has asked for a version number in libpq-fe.h.  Maybe
> all those previous additions were indeed minor enough that the
> problem didn't come up.  (Another likely possibility, perhaps, is
> that people have been misusing the server version for this purpose,
> and have been lucky enough to not have that approach fail for them.)

Well, I don't know for sure. I sometimes find it difficult to account
for the behavior even of the small number of people I know fairly
well, let alone the rather large number of people I've never even met.
But if I had to speculate ... I think one contributing factor is that
the number of people who write applications that use a C-language
connector to the database isn't terribly large, because most
application developers are going to use a higher-level language like
Java or Python or something. And of those that do, I would guess most
of them aren't trying to write applications that work across versions,
and so the problem doesn't arise. Now I know that personally, I have
tried to do that on a number of occasions, and I've accidentally used
functions that only existed in newer versions on, err, most of those
occasions. I chose to handle that problem by either (a) rewriting the
code to use only functions that appeared in all relevant versions of
libpq or (b) upgrading all the versions of libpq in my environment to
something new enough that it would work. If I'd run into a problem
that couldn't be handled in either of those ways, I likely would have
handled it by (c) depending on some symbol that actually indicates the
server version number, and demanding that anybody compiling my code
use a packaging system where those versions were the same. But none of
those workarounds seem like a real argument against having a version
indicator for libpq proper.

> Anyway, I do not see why we can't establish a principle going forward
> that new additions to libpq's API should involve at least one macro,
> so that they can be checked for with #ifdefs.  Just because the
> version-number approach offloads work from us doesn't make it a good
> idea, because the work doesn't vanish; it will be dumped in the laps
> of packagers and end users.

What work? Including an additional #define in a header file doesn't
create any work for packagers or end-users that I can see. If
anything, it seems easier for end-users. If you want a function that
first appears in v16, just test whether the version number is >= 16.
On the other hand if we promise to add at least one #define to that
file for each new release, then somebody's got to be like, oh, let's
see, this function was added in v16, now which #define got added in
that release ... hmm, let me go diff the branches in git ... how is
that any better? Especially because it seems really likely that we
will fail to actually follow this principle consistently, in which
case they may find that #define that they need doesn't even exist.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Jun 17, 2021 at 2:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> ...  Just because the
>> version-number approach offloads work from us doesn't make it a good
>> idea, because the work doesn't vanish; it will be dumped in the laps
>> of packagers and end users.

> What work? Including an additional #define in a header file doesn't
> create any work for packagers or end-users that I can see. If
> anything, it seems easier for end-users. If you want a function that
> first appears in v16, just test whether the version number is >= 16.

You're omitting the step of "figure out which version the feature you
want to use appeared in".  A few years down the road, that'll get
harder than it might seem to be for a shiny new feature.

As for the packagers, this creates a requirement to include the right
version of the right file in the right sub-package.  Admittedly, if
we hack things so that the #define appears directly in libpq-fe.h through
some configure magic, then there's nothing extra for packagers to get
right; but if we put it anywhere else, we're adding ways for them to
get it wrong.

> On the other hand if we promise to add at least one #define to that
> file for each new release,

New libpq API feature, not every new release.  I don't really see
that that's much harder than, say, bumping catversion.

> ... then somebody's got to be like, oh, let's
> see, this function was added in v16, now which #define got added in
> that release ... hmm, let me go diff the branches in git ... how is
> that any better?

I repeat that you are evaluating this through the lens of how much
work it is for us as opposed to other people, and I fundamentally
disagree with that being the primary metric.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

> I think putting a version number as such in there is a truly
> horrid idea.  However, I could get behind adding a boolean flag
> that says specifically whether the pipeline feature exists.
> Then you'd do something like
> 
> #ifdef LIBPQ_HAS_PIPELINING
> 
> rather than embedding knowledge of exactly which release
> added that.

That would be even better, but I agree with what others have
said: we would have to keep adding such feature test macros
going forward.

I think ideally you would want to have both since the version
macro could still be helpful in dealing with "features" that you
did not plan to add (aka bugs).


> Comparing v13 and v14 libpq-fe.h, I see that there is a solution
> available now: "#ifdef PQ_QUERY_PARAM_MAX_LIMIT".

Hm, it must have been added recently since I don't see it in 14beta1.
But thanks for the pointer, if nothing better comes up this will
have to do.

Boris Kolpackov <boris@codesynthesis.com> writes:
> Tom Lane <tgl@sss.pgh.pa.us> writes:
>> I think putting a version number as such in there is a truly
>> horrid idea.  However, I could get behind adding a boolean flag
>> that says specifically whether the pipeline feature exists.

> That would be even better, but I agree with what others have
> said: we would have to keep adding such feature test macros
> going forward.

Yes, and I think that is a superior solution.  I think the
argument that it's too much effort is basically nonsense.

> I think ideally you would want to have both since the version
> macro could still be helpful in dealing with "features" that you
> did not plan to add (aka bugs).

I really doubt that a version number appearing in libpq-fe.h would
be helpful for deciding whether you need to work around a bug.
The problem again is version skew: how well does the libpq.so you
are running against today match up with the header you compiled
against (possibly months ago, possibly on a different machine)?
What you'd want for that sort of thing is a runtime test, i.e.
consult PQlibVersion().

That point, along with the previously-discussed point about confusion
between server and libpq versions, nicely illustrates another reason
why I'm resistant to just adding a version number to libpq-fe.h.
If we do that, application programmers will be presented with THREE
different Postgres version numbers, and it seems inevitable that
people will make mistakes and consult the wrong one for a particular
purpose.  I think we can at least reduce the confusion by handling
the question of which-features-are-visible-in-the-include-file in a
different style.

            regards, tom lane

On 2021-Jun-18, Boris Kolpackov wrote:

> Tom Lane <tgl@sss.pgh.pa.us> writes:
> 
> > I think putting a version number as such in there is a truly
> > horrid idea.  However, I could get behind adding a boolean flag
> > that says specifically whether the pipeline feature exists.
> > Then you'd do something like
> > 
> > #ifdef LIBPQ_HAS_PIPELINING
> > 
> > rather than embedding knowledge of exactly which release
> > added that.
> 
> That would be even better, but I agree with what others have
> said: we would have to keep adding such feature test macros
> going forward.

But we do not add that many significant features to libpq in the first
place, so I'm not sure it would be too bad.  As far as I am aware, this
is the first time someone has requested a mechanism to detect feature
presence specifically in libpq.

To put a number to it, I counted the number of commits to exports.txt
since Jan 2015 -- there are 17.  But many of them are just intra-release
fixups; the number of actual "features" is 11, an average of two per
year.  That seems small enough to me.

So I'm +1 on adding this "feature macro".

(The so-version major changed from 4 to 5 in commit 1e7bb2da573e, dated
April 2006.)

> I think ideally you would want to have both since the version
> macro could still be helpful in dealing with "features" that you
> did not plan to add (aka bugs).
> 
> 
> > Comparing v13 and v14 libpq-fe.h, I see that there is a solution
> > available now: "#ifdef PQ_QUERY_PARAM_MAX_LIMIT".
> 
> Hm, it must have been added recently since I don't see it in 14beta1.
> But thanks for the pointer, if nothing better comes up this will
> have to do.

Yeah, this one was added by commit cb92703384e2 on June 8th, three weeks
after beta1.

-- 
Álvaro Herrera       Valdivia, Chile
"Pido que me den el Nobel por razones humanitarias" (Nicanor Parra)

Alvaro Herrera <alvaro.herrera@2ndquadrant.com> writes:
> So I'm +1 on adding this "feature macro".

Concretely, how about the attached?  (I also got rid of a recently-added
extra comma.  While the compilers we use might not warn about that,
it seems unwise to assume that no user's compiler will.)

I guess one unresolved question is whether we want to mention these in
the SGML docs.  I vote "no", because it'll raise the maintenance cost
noticeably.  But I can see an argument on the other side.

            regards, tom lane

diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index ec378705ad..4677c51e1b 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -28,6 +28,13 @@ extern "C"
  */
 #include "postgres_ext.h"

+/*
+ * These symbols may be used in compile-time #ifdef tests for the availability
+ * of newer libpq features.
+ */
+#define LIBPQ_HAS_PIPELINING 1
+#define LIBPQ_HAS_TRACE_FLAGS 1
+
 /*
  * Option flags for PQcopyResult
  */
@@ -98,7 +105,7 @@ typedef enum
     PGRES_COPY_BOTH,            /* Copy In/Out data transfer in progress */
     PGRES_SINGLE_TUPLE,            /* single tuple from larger resultset */
     PGRES_PIPELINE_SYNC,        /* pipeline synchronization point */
-    PGRES_PIPELINE_ABORTED,        /* Command didn't run because of an abort
+    PGRES_PIPELINE_ABORTED        /* Command didn't run because of an abort
                                  * earlier in a pipeline */
 } ExecStatusType;

Alvaro Herrera <alvaro.herrera@2ndquadrant.com> writes:
> On 2021-Jun-18, Tom Lane wrote:
>> I guess one unresolved question is whether we want to mention these in
>> the SGML docs.  I vote "no", because it'll raise the maintenance cost
>> noticeably.  But I can see an argument on the other side.

> Well, if we do want docs for these macros, then IMO it'd be okay to have
> them in libpq-fe.h itself rather than SGML.  A one-line comment for each
> would suffice:

WFM.  I'd sort of supposed that the symbol names were self-documenting,
but you're right that a line or so of annotation improves things.

            regards, tom lane

I wrote:
> Alvaro Herrera <alvaro.herrera@2ndquadrant.com> writes:
>> Well, if we do want docs for these macros, then IMO it'd be okay to have
>> them in libpq-fe.h itself rather than SGML.  A one-line comment for each
>> would suffice:

> WFM.  I'd sort of supposed that the symbol names were self-documenting,
> but you're right that a line or so of annotation improves things.

Hearing no further comments, done that way.

            regards, tom lane

On Sat, Jun 19, 2021 at 11:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Hearing no further comments, done that way.

What will prevent us from forgetting to do something about this again,
a year from now?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> What will prevent us from forgetting to do something about this again,
> a year from now?

As long as we notice it before 15.0, we can fix it retroactively,
as we just did for 14.  For that matter, fixing before 15.1 or
so would likely be Good Enough.

But realistically, how is this any worse of a problem than a hundred
other easily-forgotten coding rules we have?  We manage to uphold
most of them most of the time.

            regards, tom lane

> On 21 Jun 2021, at 17:27, Robert Haas <robertmhaas@gmail.com> wrote:
> 
> On Sat, Jun 19, 2021 at 11:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Hearing no further comments, done that way.
> 
> What will prevent us from forgetting to do something about this again,
> a year from now?

An entry in a release checklist could perhaps be an idea?

--
Daniel Gustafsson        https://vmware.com/

Daniel Gustafsson <daniel@yesql.se> writes:
> On 21 Jun 2021, at 17:27, Robert Haas <robertmhaas@gmail.com> wrote:
>> What will prevent us from forgetting to do something about this again,
>> a year from now?

> An entry in a release checklist could perhaps be an idea?

Yeah, I was wondering if adding an entry to RELEASE_CHANGES would be
helpful.  Again, I'm not sure that this coding rule is much more
likely to be violated than any other.  On the other hand, the fact
that it's not critical until we approach release does suggest that
maybe it'd be useful to treat it as a checklist item.

            regards, tom lane

On 6/21/21 12:34 PM, Tom Lane wrote:
> Daniel Gustafsson <daniel@yesql.se> writes:
>> On 21 Jun 2021, at 17:27, Robert Haas <robertmhaas@gmail.com> wrote:
>>> What will prevent us from forgetting to do something about this again,
>>> a year from now?
>> An entry in a release checklist could perhaps be an idea?
> Yeah, I was wondering if adding an entry to RELEASE_CHANGES would be
> helpful.  Again, I'm not sure that this coding rule is much more
> likely to be violated than any other.  On the other hand, the fact
> that it's not critical until we approach release does suggest that
> maybe it'd be useful to treat it as a checklist item.
>
>         


Maybe for release note preparation, since that's focused on new
features, but this doesn't sound like a release prep function to me.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com