Обсуждение: Convert PGconn, PGresult to opaque types?
In last week's episode, I wrote:
> I have dealt with this by splitting libpq-fe.h into two files, leaving
> only exportable information in libpq-fe.h and moving the internals into
> a new file libpq-int.h. This should solve Ewan's problem and reduce
> application-code namespace pollution in general.
>
> (If I were really being fascist about this I would've moved the
> definitions of struct pg_conn and friends into libpq-int.h, leaving
> only an opaque "struct pg_conn" declaration to be seen by applications.
> But I'm afraid that that will break too many applications. Perhaps we
> can take that step in some future release.)
Today I am wondering whether it wouldn't be a good idea to just go ahead
and move those struct declarations into libpq-int.h. Then a pointer to
PGconn would be truly an opaque type for applications; all they'd see is
typedef struct pg_conn * PGconn;
Basically this would force applications to use the accessor functions
as recommended in the documentation, and not touch fields of a PGconn
object directly. (Ditto for PGresult.)
I think it is a real good idea to do this eventually, because it will
help ensure that applications don't assume things they shouldn't about
what is in a PGconn, and it will allow us to modify the PGconn structure
without breaking binary compatibility of applications across libpq
releases. This is a critical consideration now that libpq is released
as a shared library on Unix and will be available in DLL form for
Windows. People tend to think that they can update shared libraries
without recompiling the programs that use the libraries.
What occurs to me today is that if we intend to do it eventually, we
may as well do it *now*. Waiting will just give more programmers the
opportunity to write bad code, I think. And we are going to break
binary compatibility of libpq in 6.4 anyway --- the contents of PGconn
have changed since last time. So anyone who's not relying on the
accessor functions is already in trouble.
The downside is that we will probably break at least some applications
at source-code level. They weren't playing by the rules, but they'll
be annoyed anyway. (Anyone who's too lazy to fix their code can just
import libpq-int.h instead of libpq-fe.h, but one hopes not too many
people will take that way out.)
I know that both psql and libpgtcl are not playing entirely by the rules,
and will need to be fixed. I can handle that. I'm not volunteering
to fix any code outside the distribution, however.
Comments? Any strong reasons *not* to do this?
regards, tom lane
> In last week's episode, I wrote: > > I have dealt with this by splitting libpq-fe.h into two files, leaving > > only exportable information in libpq-fe.h and moving the internals into > > a new file libpq-int.h. This should solve Ewan's problem and reduce > > application-code namespace pollution in general. > > > > (If I were really being fascist about this I would've moved the > > definitions of struct pg_conn and friends into libpq-int.h, leaving > > only an opaque "struct pg_conn" declaration to be seen by applications. > > But I'm afraid that that will break too many applications. Perhaps we > > can take that step in some future release.) > > Today I am wondering whether it wouldn't be a good idea to just go ahead > and move those struct declarations into libpq-int.h. Then a pointer to > PGconn would be truly an opaque type for applications; all they'd see is > > typedef struct pg_conn * PGconn; > > Basically this would force applications to use the accessor functions > as recommended in the documentation, and not touch fields of a PGconn > object directly. (Ditto for PGresult.) I am scared about external stuff like php. If they use it, and we release something that doesn't work with their stuff, we are cooked until they upgrade. Not really sure what the advantage would be, aside from cleanliness. > > I think it is a real good idea to do this eventually, because it will > help ensure that applications don't assume things they shouldn't about > what is in a PGconn, and it will allow us to modify the PGconn structure > without breaking binary compatibility of applications across libpq > releases. This is a critical consideration now that libpq is released > as a shared library on Unix and will be available in DLL form for > Windows. People tend to think that they can update shared libraries > without recompiling the programs that use the libraries. > > What occurs to me today is that if we intend to do it eventually, we > may as well do it *now*. Waiting will just give more programmers the > opportunity to write bad code, I think. And we are going to break > binary compatibility of libpq in 6.4 anyway --- the contents of PGconn > have changed since last time. So anyone who's not relying on the > accessor functions is already in trouble. > > The downside is that we will probably break at least some applications > at source-code level. They weren't playing by the rules, but they'll > be annoyed anyway. (Anyone who's too lazy to fix their code can just > import libpq-int.h instead of libpq-fe.h, but one hopes not too many > people will take that way out.) > > I know that both psql and libpgtcl are not playing entirely by the rules, > and will need to be fixed. I can handle that. I'm not volunteering > to fix any code outside the distribution, however. > > Comments? Any strong reasons *not* to do this? > > regards, tom lane > > -- Bruce Momjian | 830 Blythe Avenue maillist@candle.pha.pa.us | Drexel Hill, Pennsylvania 19026 + If your life is a hard drive, | (610) 353-9879(w) + Christ can be your backup. | (610) 853-3000(h)
Bruce Momjian wrote:
> > Basically this would force applications to use the accessor functions
> > as recommended in the documentation, and not touch fields of a PGconn
> > object directly. (Ditto for PGresult.)
>
> I am scared about external stuff like php. If they use it, and we
> release something that doesn't work with their stuff, we are cooked
> until they upgrade.
>
> Not really sure what the advantage would be, aside from cleanliness.
The cleaness allows us to change the interals without breaking "external
stuff".
I think Tom is aiming for thread-safeness which can't be done as long as
external stuff insists on accessing global structs inside libpq.
Sometimes we have to break thing to make them work better.
regards,
--
---------------------------------------------
Göran Thyni, sysadm, JMS Bildbasen, Kiruna
Goran Thyni <goran@bildbasen.se> writes:
> Bruce Momjian wrote:
>>>> Basically this would force applications to use the accessor functions
>>>> as recommended in the documentation, and not touch fields of a PGconn
>>>> object directly. (Ditto for PGresult.)
>>
>> I am scared about external stuff like php. If they use it, and we
>> release something that doesn't work with their stuff, we are cooked
>> until they upgrade.
But if they are using any direct references to fields of the PGconn
struct, their stuff *already* won't work with 6.4. Admittedly it'd
most likely only take a recompile to fix, and not code changes
(however trivial). But if they'd been using only the documented API,
ie using the accessor functions and not directly touching the struct,
then a new shared library or DLL could be plopped right in without even
a recompile of calling applications.
Is the PHP source code available? It wouldn't take much work to check
whether it will compile without a definition for struct pg_conn.
> I think Tom is aiming for thread-safeness which can't be done as long as
> external stuff insists on accessing global structs inside libpq.
This is not a thread-safeness issue, it's an issue of being able to
promise binary compatibility across versions. Before the days of shared
libraries, source-code compatibility across versions was Good Enough,
because users had to rebuild their apps anyway to drop in a new version
of a library. Nowadays, people who don't even *have* the source of an
app still expect that they can shove in a new version of a shared library
that the app depends on. And that's a good thing, if it fixes some bugs
or adds new features; but it only works if the library's API is fully
binary compatible across releases. Hiding all but the simplest, most
stable structs is a necessary restriction if you hope to achieve that.
I made the wrong choice on this years ago with libjpeg (in self-defense,
that was before anyone had heard of shared libraries for Unix): I
exposed as part of the library's API a large parameter struct that I
knew would need to change with every new library version. At the time
it didn't seem like a problem, but I've learned to regret it. I see
people complaining all the time that their apps compiled against
libjpeg v6a stop working when they drop in v6b instead. Learn
from my bad example ;-)
Basically my feeling is that we will want to do this eventually, and
the pain level can only get worse the longer we put it off.
regards, tom lane
>>>>> "TL" == Tom Lane <tgl@sss.pgh.pa.us> writes: (sorry, attributions are lost here) >>>>> Basically this would force applications to use the accessor functions >>>>> as recommended in the documentation, and not touch fields of a PGconn >>>>> object directly. (Ditto for PGresult.) >>> >>> I am scared about external stuff like php. If they use it, and we >>> release something that doesn't work with their stuff, we are cooked >>> until they upgrade. TL> TL> But if they are using any direct references to fields of the PGconn TL> struct, their stuff *already* won't work with 6.4. Admittedly it'd TL> most likely only take a recompile to fix, and not code changes TL> (however trivial). But if they'd been using only the documented API, TL> ie using the accessor functions and not directly touching the struct, TL> then a new shared library or DLL could be plopped right in without even TL> a recompile of calling applications. TL> TL> Is the PHP source code available? It wouldn't take much work to check TL> whether it will compile without a definition for struct pg_conn. The PHP source is available from http://www.php.net/. From a quick look through it, it does access the PGconn structure directly. Stuff like (this is from the file php3.0.2a/functions/pgsql.c): lo_read((PGconn *)pgsql->conn, pgsql->lofd, buf, 8192)) However, the whole PostgreSQL-specific stuff is only 1400 lines worth, and the PHP guys are reputed to very active, so I don't think a change should pose too much of a problem if they are forewarned. [I have CCed the PHP/PostgreSQL module developers] -- Eric Marsden emarsden @ mail.dotcom.fr It's elephants all the way down
> Goran Thyni <goran@bildbasen.se> writes: > > Bruce Momjian wrote: > >>>> Basically this would force applications to use the accessor functions > >>>> as recommended in the documentation, and not touch fields of a PGconn > >>>> object directly. (Ditto for PGresult.) > >> > >> I am scared about external stuff like php. If they use it, and we > >> release something that doesn't work with their stuff, we are cooked > >> until they upgrade. > > But if they are using any direct references to fields of the PGconn > struct, their stuff *already* won't work with 6.4. Admittedly it'd > most likely only take a recompile to fix, and not code changes > (however trivial). But if they'd been using only the documented API, > ie using the accessor functions and not directly touching the struct, > then a new shared library or DLL could be plopped right in without even > a recompile of calling applications. > > Is the PHP source code available? It wouldn't take much work to check > whether it will compile without a definition for struct pg_conn. > > > I think Tom is aiming for thread-safeness which can't be done as long as > > external stuff insists on accessing global structs inside libpq. > > This is not a thread-safeness issue, it's an issue of being able to > promise binary compatibility across versions. Before the days of shared > libraries, source-code compatibility across versions was Good Enough, > because users had to rebuild their apps anyway to drop in a new version > of a library. Nowadays, people who don't even *have* the source of an > app still expect that they can shove in a new version of a shared library > that the app depends on. And that's a good thing, if it fixes some bugs > or adds new features; but it only works if the library's API is fully > binary compatible across releases. Hiding all but the simplest, most > stable structs is a necessary restriction if you hope to achieve that. > > I made the wrong choice on this years ago with libjpeg (in self-defense, > that was before anyone had heard of shared libraries for Unix): I > exposed as part of the library's API a large parameter struct that I > knew would need to change with every new library version. At the time > it didn't seem like a problem, but I've learned to regret it. I see > people complaining all the time that their apps compiled against > libjpeg v6a stop working when they drop in v6b instead. Learn > from my bad example ;-) > > Basically my feeling is that we will want to do this eventually, and > the pain level can only get worse the longer we put it off. > I am convinced. Hide the structure members, and lets go through beta like that. If we have problems, we can supply a patch to expose the structure members, with the knowledge there will be no workaround patch for 6.5. They have to fix it by then. -- Bruce Momjian | 830 Blythe Avenue maillist@candle.pha.pa.us | Drexel Hill, Pennsylvania 19026 + If your life is a hard drive, | (610) 353-9879(w) + Christ can be your backup. | (610) 853-3000(h)
On 24 Aug 1998, Eric Marsden wrote: > >>>>> "TL" == Tom Lane <tgl@sss.pgh.pa.us> writes: > > (sorry, attributions are lost here) > > >>>>> Basically this would force applications to use the accessor functions > >>>>> as recommended in the documentation, and not touch fields of a PGconn > >>>>> object directly. (Ditto for PGresult.) > >>> > >>> I am scared about external stuff like php. If they use it, and we > >>> release something that doesn't work with their stuff, we are cooked > >>> until they upgrade. > TL> > TL> But if they are using any direct references to fields of the PGconn > TL> struct, their stuff *already* won't work with 6.4. Admittedly it'd > TL> most likely only take a recompile to fix, and not code changes > TL> (however trivial). But if they'd been using only the documented API, > TL> ie using the accessor functions and not directly touching the struct, > TL> then a new shared library or DLL could be plopped right in without even > TL> a recompile of calling applications. > TL> > TL> Is the PHP source code available? It wouldn't take much work to check > TL> whether it will compile without a definition for struct pg_conn. > > The PHP source is available from http://www.php.net/. From a quick > look through it, it does access the PGconn structure directly. Stuff > like (this is from the file php3.0.2a/functions/pgsql.c): > > lo_read((PGconn *)pgsql->conn, pgsql->lofd, buf, 8192)) Shouldn't be a problem. In this case, pgsql is is pointer to a struct typedef struct pgLofp { PGconn *conn; int lofd; } pgLofp; so, pgsql->conn is a pointer to a PGconn struct to pass to lo_read, but the code doesn't touch or refer anywhere to the members of that struct. Anyway, I'll try to keep an eye on what's happening on pgsql-hackers list. > However, the whole PostgreSQL-specific stuff is only 1400 lines worth, > and the PHP guys are reputed to very active, so I don't think a change > should pose too much of a problem if they are forewarned. Jouni Ahto
Bruce Momjian <maillist@candle.pha.pa.us> writes:
> I am convinced. Hide the structure members, and lets go through beta
> like that. If we have problems, we can supply a patch to expose the
> structure members, with the knowledge there will be no workaround patch
> for 6.5. They have to fix it by then.
Actually, the "patch" will just be to #include libpq-int.h along with
lipq-fe.h. I arranged for libpq-int.h to be one of the installed header
files, with the idea that some people would rather do that than fix
their code properly. It's their choice: if they want to depend on libpq
internals and have a greater risk of their code breaking across
releases, they can. I just want to discourage that a little bit, and
make it real clear what's considered supported API and what's considered
internal detail.
regards, tom lane