Thread: Re: UUNET socket-file-location patch
I have to agree with Peter E. on this patch: it's poorly thought out.
I don't mind the idea of being able to relocate the socket file,
but the client-side interface they've chosen is silly. Having to
add another switch to every client app is not reasonable --- it's
bad enough that you had to hack every one of the clients we supply,
but what of client apps that just use libpq or one of the other
interface libraries? They'll be unable to talk to such a postmaster
without further work.
We should revert all the client-side changes from this patch, and
instead teach libpq and the other interfaces to treat a host name
that starts with a slash as being a path to a socket file (replacing
the default assumption of "/tmp"). Much cleaner, especially for
existing client apps.
regards, tom lane
> I have to agree with Peter E. on this patch: it's poorly thought out. Now you tell me. :-) > I don't mind the idea of being able to relocate the socket file, > but the client-side interface they've chosen is silly. Having to > add another switch to every client app is not reasonable --- it's > bad enough that you had to hack every one of the clients we supply, > but what of client apps that just use libpq or one of the other > interface libraries? They'll be unable to talk to such a postmaster > without further work. The only solution for other apps is to use the environment variable PGUNXSOCKET or use PQconnectdb with unixsocket="lkjasdf". Overloading the hostname with a leading slash is another nice option. If I do that in libpq, then all the apps can benefit, right? > We should revert all the client-side changes from this patch, and > instead teach libpq and the other interfaces to treat a host name > that starts with a slash as being a path to a socket file (replacing > the default assumption of "/tmp"). Much cleaner, especially for > existing client apps. I can easily back out whatever you want. Let me back out the client changes, and hack libpq to handle the leading slash. Sounds good. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Actually, the thing doesn't even compile:
pqcomm.c: In function `StreamServerPort':
pqcomm.c:259: warning: implicit declaration of function `hstrerror'
pqcomm.c:259: `h_errno' undeclared (first use in this function)
pqcomm.c:259: (Each undeclared identifier is reported only once
pqcomm.c:259: for each function it appears in.)
make[3]: *** [pqcomm.o] Error 1
postmaster.c: In function `get_host_port':
postmaster.c:1338: warning: implicit declaration of function `hstrerror'
postmaster.c:1338: `h_errno' undeclared (first use in this function)
postmaster.c:1338: (Each undeclared identifier is reported only once
postmaster.c:1338: for each function it appears in.)
make[3]: *** [postmaster.o] Error 1
Please revert this patch, so I can get some work done?
regards, tom lane
Looks like it needs a resolver include.... LER * Tom Lane <tgl@sss.pgh.pa.us> [001113 13:11]: > Actually, the thing doesn't even compile: > > pqcomm.c: In function `StreamServerPort': > pqcomm.c:259: warning: implicit declaration of function `hstrerror' > pqcomm.c:259: `h_errno' undeclared (first use in this function) > pqcomm.c:259: (Each undeclared identifier is reported only once > pqcomm.c:259: for each function it appears in.) > make[3]: *** [pqcomm.o] Error 1 > > postmaster.c: In function `get_host_port': > postmaster.c:1338: warning: implicit declaration of function `hstrerror' > postmaster.c:1338: `h_errno' undeclared (first use in this function) > postmaster.c:1338: (Each undeclared identifier is reported only once > postmaster.c:1338: for each function it appears in.) > make[3]: *** [postmaster.o] Error 1 > > Please revert this patch, so I can get some work done? > > regards, tom lane -- Larry Rosenman http://www.lerctr.org/~ler Phone: +1 972-414-9812 (voice) Internet: ler@lerctr.org US Mail: 1905 Steamboat Springs Drive, Garland, TX 75044-6749
On BSDI, hstrerror is defined in netdb.h. Do you have it anywhere? Is that a proper function call? > Actually, the thing doesn't even compile: > > pqcomm.c: In function `StreamServerPort': > pqcomm.c:259: warning: implicit declaration of function `hstrerror' > pqcomm.c:259: `h_errno' undeclared (first use in this function) > pqcomm.c:259: (Each undeclared identifier is reported only once > pqcomm.c:259: for each function it appears in.) > make[3]: *** [pqcomm.o] Error 1 > > postmaster.c: In function `get_host_port': > postmaster.c:1338: warning: implicit declaration of function `hstrerror' > postmaster.c:1338: `h_errno' undeclared (first use in this function) > postmaster.c:1338: (Each undeclared identifier is reported only once > postmaster.c:1338: for each function it appears in.) > make[3]: *** [postmaster.o] Error 1 > > Please revert this patch, so I can get some work done? > > regards, tom lane > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> On BSDI, hstrerror is defined in netdb.h. Do you have it anywhere? Is
> that a proper function call?
There is no hstrerror anywhere on HPUX. h_errno is defined in
<netdb.h>, but only with nonstandard compilation options:
#ifdef _XOPEN_SOURCE_EXTENDED
extern int h_errno;
#endif
The man page for gethostbyname() points out that h_errno will be garbage
if the hostname was not resolved via a nameserver, anyway.
On the whole, this code looks much less than portable to me.
regards, tom lane
OK, hterror removed. > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > On BSDI, hstrerror is defined in netdb.h. Do you have it anywhere? Is > > that a proper function call? > > There is no hstrerror anywhere on HPUX. h_errno is defined in > <netdb.h>, but only with nonstandard compilation options: > > #ifdef _XOPEN_SOURCE_EXTENDED > extern int h_errno; > #endif > > The man page for gethostbyname() points out that h_errno will be garbage > if the hostname was not resolved via a nameserver, anyway. > > On the whole, this code looks much less than portable to me. > > regards, tom lane > -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Tom Lane writes: > We should revert all the client-side changes from this patch, and > instead teach libpq and the other interfaces to treat a host name > that starts with a slash as being a path to a socket file (replacing > the default assumption of "/tmp"). Much cleaner, especially for > existing client apps. Should the parameter determine the directory or the full file name? I'd go for the former, but it's not a strong case. -- Peter Eisentraut peter_e@gmx.net http://yi.org/peter-e/
Peter Eisentraut <peter_e@gmx.net> writes:
> Should the parameter determine the directory or the full file name? I'd
> go for the former, but it's not a strong case.
Directory was what I had in mind too, but I'm not sure what Bruce
actually did ...
regards, tom lane
> Peter Eisentraut <peter_e@gmx.net> writes: > > Should the parameter determine the directory or the full file name? I'd > > go for the former, but it's not a strong case. > > Directory was what I had in mind too, but I'm not sure what Bruce > actually did ... I did whatever the patch did. I believe it is the full path. I believe it is used here: #define UNIXSOCK_PATH(sun,port,defpath) \ ((defpath && defpath[0] != '\0') ? (strncpy((sun).sun_path, defpath, sizeof((sun).sun_path)), (sun).sun_path[sizeof((sun).sun_path)-1] = '\0') : sprintf((sun).sun_path, "/tmp/.s.PGSQL.%d", (port))) -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> Peter Eisentraut <peter_e@gmx.net> writes:
>>>> Should the parameter determine the directory or the full file name? I'd
>>>> go for the former, but it's not a strong case.
>>
>> Directory was what I had in mind too, but I'm not sure what Bruce
>> actually did ...
> I did whatever the patch did. I believe it is the full path. I believe
> it is used here:
> #define UNIXSOCK_PATH(sun,port,defpath) \
> ((defpath && defpath[0] != '\0') ? (strncpy((sun).sun_path,
> defpath, sizeof((sun).sun_path)),
> (sun).sun_path[sizeof((sun).sun_path)-1] = '\0') :
> sprintf((sun).sun_path, "/tmp/.s.PGSQL.%d", (port)))
Hmm. I think it would make more sense to make the parameter be just
the directory, not the full path including filename --- for one thing,
doing it like that renders the port-number parameter useless. Why not
#define UNIXSOCK_PATH(sun,port,defpath) \ snprintf((sun).sun_path, sizeof((sun).sun_path), "%s/.s.PGSQL.%d", \
(((defpath) && *(defpath) != '\0') ? (defpath) : "/tmp"), \ (port))
regards, tom lane
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> Peter Eisentraut <peter_e@gmx.net> writes: > >>>> Should the parameter determine the directory or the full file name? I'd > >>>> go for the former, but it's not a strong case. > >> > >> Directory was what I had in mind too, but I'm not sure what Bruce > >> actually did ... > > > I did whatever the patch did. I believe it is the full path. I believe > > it is used here: > > > #define UNIXSOCK_PATH(sun,port,defpath) \ > > ((defpath && defpath[0] != '\0') ? (strncpy((sun).sun_path, > > defpath, sizeof((sun).sun_path)), > > (sun).sun_path[sizeof((sun).sun_path)-1] = '\0') : > > sprintf((sun).sun_path, "/tmp/.s.PGSQL.%d", (port))) > > Hmm. I think it would make more sense to make the parameter be just > the directory, not the full path including filename --- for one thing, > doing it like that renders the port-number parameter useless. Why not > > #define UNIXSOCK_PATH(sun,port,defpath) \ > snprintf((sun).sun_path, sizeof((sun).sun_path), "%s/.s.PGSQL.%d", \ > (((defpath) && *(defpath) != '\0') ? (defpath) : "/tmp"), \ > (port)) I can do that. Of course, I have to now change all the documentation to match it. :-) -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
Bruce Momjian writes: > > Hmm. I think it would make more sense to make the parameter be just > > the directory, not the full path including filename --- for one thing, > > doing it like that renders the port-number parameter useless. Why not > > > > #define UNIXSOCK_PATH(sun,port,defpath) \ > > snprintf((sun).sun_path, sizeof((sun).sun_path), "%s/.s.PGSQL.%d", \ > > (((defpath) && *(defpath) != '\0') ? (defpath) : "/tmp"), \ > > (port)) > > I can do that. Of course, I have to now change all the documentation to > match it. :-) Rather: #define UNIXSOCK_PATH(sun,port,defpath) \ snprintf((sun).sun_path, sizeof((sun).sun_path), "%s/.s.PGSQL.%d", \ (defpath), (port)) and make "/tmp" the default in guc.c. -- Peter Eisentraut peter_e@gmx.net http://yi.org/peter-e/
Peter Eisentraut <peter_e@gmx.net> writes:
> #define UNIXSOCK_PATH(sun,port,defpath) \
> snprintf((sun).sun_path, sizeof((sun).sun_path), "%s/.s.PGSQL.%d", \
> (defpath), (port))
> and make "/tmp" the default in guc.c.
No, because this has to work on the client side too. The default path
*must* be determined at compile time, or clients and servers will be
unable to find each other.
I wouldn't object to having "/tmp" be given as a macro PG_STD_SOCKET_DIR
in config.h, making it potentially configurable on a site-wide basis,
but that's as far as I think we can go.
regards, tom lane
Tom Lane writes: > > #define UNIXSOCK_PATH(sun,port,defpath) \ > > snprintf((sun).sun_path, sizeof((sun).sun_path), "%s/.s.PGSQL.%d", \ > > (defpath), (port)) > > > and make "/tmp" the default in guc.c. > > No, because this has to work on the client side too. The default path > *must* be determined at compile time, or clients and servers will be > unable to find each other. The only difference between your snippet and mine is that yours sets the default to "" and interprets it as "/tmp" when it is used, whereas mine sets the default to "/tmp" to begin with. Clients don't see the difference. -- Peter Eisentraut peter_e@gmx.net http://yi.org/peter-e/
Peter Eisentraut <peter_e@gmx.net> writes:
> Tom Lane writes:
>>>> and make "/tmp" the default in guc.c.
>>
>> No, because this has to work on the client side too. The default path
>> *must* be determined at compile time, or clients and servers will be
>> unable to find each other.
> The only difference between your snippet and mine is that yours sets the
> default to "" and interprets it as "/tmp" when it is used, whereas mine
> sets the default to "/tmp" to begin with. Clients don't see the
> difference.
Clients that don't contain guc.c are going to see a difference, no?
Where are they getting the default from?
regards, tom lane
> Peter Eisentraut <peter_e@gmx.net> writes: > > Tom Lane writes: > >>>> and make "/tmp" the default in guc.c. > >> > >> No, because this has to work on the client side too. The default path > >> *must* be determined at compile time, or clients and servers will be > >> unable to find each other. > > > The only difference between your snippet and mine is that yours sets the > > default to "" and interprets it as "/tmp" when it is used, whereas mine > > sets the default to "/tmp" to begin with. Clients don't see the > > difference. > > Clients that don't contain guc.c are going to see a difference, no? > Where are they getting the default from? Good point. This macro is called by the backend, and the libpq frontend code. We would have to push the /tmp default into libpq code too, which seems messier. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000+ If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania19026
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> Peter Eisentraut <peter_e@gmx.net> writes: > >>>> Should the parameter determine the directory or the full file name? I'd > >>>> go for the former, but it's not a strong case. > >> > >> Directory was what I had in mind too, but I'm not sure what Bruce > >> actually did ... > > > I did whatever the patch did. I believe it is the full path. I believe > > it is used here: > > > #define UNIXSOCK_PATH(sun,port,defpath) \ > > ((defpath && defpath[0] != '\0') ? (strncpy((sun).sun_path, > > defpath, sizeof((sun).sun_path)), > > (sun).sun_path[sizeof((sun).sun_path)-1] = '\0') : > > sprintf((sun).sun_path, "/tmp/.s.PGSQL.%d", (port))) > > Hmm. I think it would make more sense to make the parameter be just > the directory, not the full path including filename --- for one thing, > doing it like that renders the port-number parameter useless. Why not > > #define UNIXSOCK_PATH(sun,port,defpath) \ > snprintf((sun).sun_path, sizeof((sun).sun_path), "%s/.s.PGSQL.%d", \ > (((defpath) && *(defpath) != '\0') ? (defpath) : "/tmp"), \ > (port)) > > regards, tom lane > OK, here is the diff to make the socket file option specify just a directory, not a full path. Documentation changes were also made. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 ? Makefile.custom ? GNUmakefile ? Makefile.global ? log ? crtags ? backend/postgres ? backend/catalog/global.description ? backend/catalog/global.bki ? backend/catalog/template1.bki ? backend/catalog/template1.description ? backend/port/Makefile ? bin/initdb/initdb ? bin/initlocation/initlocation ? bin/ipcclean/ipcclean ? bin/pg_config/pg_config ? bin/pg_ctl/pg_ctl ? bin/pg_dump/pg_dump ? bin/pg_dump/pg_restore ? bin/pg_dump/pg_dumpall ? bin/pg_id/pg_id ? bin/pg_passwd/pg_passwd ? bin/pgaccess/pgaccess ? bin/pgtclsh/Makefile.tkdefs ? bin/pgtclsh/Makefile.tcldefs ? bin/pgtclsh/pgtclsh ? bin/pgtclsh/pgtksh ? bin/psql/psql ? bin/scripts/createlang ? include/config.h ? include/stamp-h ? interfaces/ecpg/lib/libecpg.so.3.2.0 ? interfaces/ecpg/preproc/ecpg ? interfaces/libpgeasy/libpgeasy.so.2.1 ? interfaces/libpgtcl/libpgtcl.so.2.1 ? interfaces/libpq/libpq.so.2.1 ? interfaces/perl5/blib ? interfaces/perl5/Makefile ? interfaces/perl5/pm_to_blib ? interfaces/perl5/Pg.c ? interfaces/perl5/Pg.bs ? pl/plperl/blib ? pl/plperl/Makefile ? pl/plperl/pm_to_blib ? pl/plperl/SPI.c ? pl/plperl/plperl.bs ? pl/plpgsql/src/libplpgsql.so.1.0 ? pl/tcl/Makefile.tcldefs Index: include/libpq/pqcomm.h =================================================================== RCS file: /home/projects/pgsql/cvsroot/pgsql/src/include/libpq/pqcomm.h,v retrieving revision 1.45 diff -c -r1.45 pqcomm.h *** include/libpq/pqcomm.h 2000/11/15 18:36:06 1.45 --- include/libpq/pqcomm.h 2000/11/22 01:33:54 *************** *** 51,66 **** /* Configure the UNIX socket address for the well known port. */ #if defined(SUN_LEN) - #define UNIXSOCK_PATH(sun,port,defpath) \ - ((defpath && defpath[0] != '\0') ? (strncpy((sun).sun_path, defpath, sizeof((sun).sun_path)), (sun).sun_path[sizeof((sun).sun_path)-1]= '\0') : sprintf((sun).sun_path, "/tmp/.s.PGSQL.%d", (port))) #define UNIXSOCK_LEN(sun) \ (SUN_LEN(&(sun))) #else - #define UNIXSOCK_PATH(sun,port,defpath) \ - ((defpath && defpath[0] != '\0') ? (strncpy((sun).sun_path, defpath, sizeof((sun).sun_path)), (sun).sun_path[sizeof((sun).sun_path)-1]= '\0') : sprintf((sun).sun_path, "/tmp/.s.PGSQL.%d", (port))) #define UNIXSOCK_LEN(sun) \ (strlen((sun).sun_path)+ offsetof(struct sockaddr_un, sun_path)) #endif /* * We do this because sun_len is in BSD's struct, while others don't. --- 51,65 ---- /* Configure the UNIX socket address for the well known port. */ #if defined(SUN_LEN) #define UNIXSOCK_LEN(sun) \ (SUN_LEN(&(sun))) #else #define UNIXSOCK_LEN(sun) \ (strlen((sun).sun_path)+ offsetof(struct sockaddr_un, sun_path)) #endif + + #define UNIXSOCK_PATH(sun,port,defpath) \ + (snprintf((sun).sun_path, UNIXSOCK_LEN(sun), "%s/.s.PGSQL.%d", (defpath && *(defpath) != '\0') ? (defpath) : "/tmp",(port))) /* * We do this because sun_len is in BSD's struct, while others don't.