Thread: pg_type_d.h location incorrect
The following documentation comment has been logged on the website: Page: https://www.postgresql.org/docs/12/libpq-exec.html Description: >The OIDs of the built-in data types are defined in the file include/server/catalog/pg_type_d.h in the install directory. The location of "pg_type_d.h" is not correct if a user installs the PG from src and the install directory path didn't contain "postgres"/"pgsql" according to Makefile as below. >pkgincludedir = $(includedir) >ifeq "$(findstring pgsql, $(pkgincludedir))" "" >ifeq "$(findstring postgres, $(pkgincludedir))" "" >override pkgincludedir := $(pkgincludedir)/postgresql My test results are as below, please take it as your reference. [install Dir contains postgres/pgsql]/include/server/catalog/pg_type_d.h [install Dir doesn't contain postgres/pgsql]/include/postgresql/server/catalog/pg_type_d.h
Hi Attached a patch to fix the incorrect location description of pg_type_d.h posted by myself at [1]. [1] https://www.postgresql.org/message-id/162149020918.26174.7150424047314144297@wrigleys.postgresql.org Regards, Tang
Attachment
"tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com> writes:
> Attached a patch to fix the incorrect location description of pg_type_d.h posted by myself at [1].
I don't particularly find that to be an improvement. In the first
place, it's somewhere between unhelpful and flat out wrong for
people on Windows, where the backtick notation doesn't work (AFAIK).
In the second, it will distract almost every user, who will need
to stop for a second or two to think about what context they would
use backticks in, and about what pg_config is, and whether the right
pg_config is even in their $PATH, and about why --pkgincludedir is
the right switch. If they don't already have a bunch of those facts
swapped in, it will take a lot longer than a second or two to figure
out what is meant here. That seems completely out of proportion to
the value of having this passing mention be pedantically correct.
Maybe it'd be better to just refer to "catalog/pg_type_d.h", which
is the approved way to spell it in #include directives, and leave
it to users who actually care to figure out where that is in their
filesystems. I see from the git history that we've tried a few
different variations on trying to be more precise than that, and
they've apparently all been failures.
regards, tom lane
On Wednesday, May 26, 2021 12:20 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote >In the first place, it's somewhere between unhelpful and flat out wrong for >people on Windows, where the backtick notation doesn't work (AFAIK). >In the second, it will distract almost every user, who will need >to stop for a second or two to think about what context they would >use backticks in, and about what pg_config is, and whether the right >pg_config is even in their $PATH, and about why --pkgincludedir is >the right switch. If they don't already have a bunch of those facts >swapped in, it will take a lot longer than a second or two to figure >out what is meant here. That seems completely out of proportion to >the value of having this passing mention be pedantically correct. Thanks for your comments. You're right, backtick notation doesn't work at my windows machine. But I made the fix in accordance with the pg-doc as below. So maybe we need a fix there, too? https://www.postgresql.org/docs/current/libpq-pgservice.html a system-wide file at `pg_config --sysconfdir`/pg_service.conf Regards, Tang
On Wed, May 26, 2021 at 05:40:45AM +0000, tanghy.fnst@fujitsu.com wrote: > You're right, backtick notation doesn't work at my windows machine. > But I made the fix in accordance with the pg-doc as below. So maybe > we need a fix there, too? > > https://www.postgresql.org/docs/current/libpq-pgservice.html > a system-wide file at `pg_config --sysconfdir`/pg_service.conf Using catalog/pg_type_d.h sounds enough to me as well. While on it, we could adjust include/common/relpath.h in pgbuffercache.sgml? We tend to include the full path for things not generated. -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes: > On Wed, May 26, 2021 at 05:40:45AM +0000, tanghy.fnst@fujitsu.com wrote: >> You're right, backtick notation doesn't work at my windows machine. >> But I made the fix in accordance with the pg-doc as below. So maybe >> we need a fix there, too? >> >> https://www.postgresql.org/docs/current/libpq-pgservice.html >> a system-wide file at `pg_config --sysconfdir`/pg_service.conf > Using catalog/pg_type_d.h sounds enough to me as well. While on it, > we could adjust include/common/relpath.h in pgbuffercache.sgml? We > tend to include the full path for things not generated. I doubt we can drop the reference to pg_config in libpq-pgservice.html, because otherwise we'd have to just say "etc/" which is not very definite. However, we should avoid the use of backticks since that's a platform-specific thing. I set out to rewrite that, but the more I looked at that section the more help it seemed to need. I thought the para was quite confusing about which environment variables affect what; and I also noticed that nowhere do we explain how service-file settings interact with other settings. (If that had been better specified to begin with, maybe I'd not have made the mistake fixed in ea8013854.) So I ended up with the attached --- what do you think? Also, looking at this, I note that "~/.pg_service.conf" is still a platform-ism. We could make that slightly better by writing "$HOME/.pg_service.conf", but is that good enough? regards, tom lane diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 875950b83c..ca231f43c4 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -8091,26 +8091,30 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough) <para> The connection service file allows libpq connection parameters to be associated with a single service name. That service name can then be - specified by a libpq connection, and the associated settings will be + specified in a libpq connection string, and the associated settings will be used. This allows connection parameters to be modified without requiring - a recompile of the libpq application. The service name can also be + a recompile of the libpq-using application. The service name can also be specified using the <envar>PGSERVICE</envar> environment variable. </para> <para> - The connection service file can be a per-user service file - at <filename>~/.pg_service.conf</filename> or the location - specified by the environment variable <envar>PGSERVICEFILE</envar>, - or it can be a system-wide file - at <filename>`pg_config --sysconfdir`/pg_service.conf</filename> or in the directory - specified by the environment variable - <envar>PGSYSCONFDIR</envar>. If service definitions with the same - name exist in the user and the system file, the user file takes - precedence. + Service names can be defined in either a per-user service file or a + system-wide file. If the same service name exists in both the user + and the system file, the user file takes precedence. + By default, the per-user service file is located + at <filename>~/.pg_service.conf</filename>; this can be overridden by + setting the environment variable <envar>PGSERVICEFILE</envar>. + The system-wide file is named <filename>pg_service.conf</filename>. + By default it is sought in the <filename>etc</filename> directory + of the <productname>PostgreSQL</productname> installation + (use <literal>pg_config --sysconfdir</literal> to identify this + directory precisely). Another directory, but not a different file + name, can be specified by setting the environment variable + <envar>PGSYSCONFDIR</envar>. </para> <para> - The file uses an <quote>INI file</quote> format where the section + Either service file uses an <quote>INI file</quote> format where the section name is the service name and the parameters are connection parameters; see <xref linkend="libpq-paramkeywords"/> for a list. For example: @@ -8121,9 +8125,22 @@ host=somehost port=5433 user=admin </programlisting> - An example file is provided at + An example file is provided in + the <productname>PostgreSQL</productname> installation at <filename>share/pg_service.conf.sample</filename>. </para> + + <para> + Connection parameters obtained from a service file are combined with + parameters obtained from other sources. A service file setting + overrides the corresponding environment variable, and in turn can be + overridden by a value given directly in the connection string. + For example, using the above service file, a connection string + <literal>service=mydb port=5434</literal> will use + host <literal>somehost</literal>, port <literal>5434</literal>, + user <literal>admin</literal>, and other parameters as set by + environment variables or built-in defaults. + </para> </sect1>
On Thursday, May 27, 2021 10:58 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote >So I ended up with the attached --- what do you think? The doc-fix LGTM. But I couldn't apply the patch at HEAD(2941138e60). Maybe you did the fix at a branch other than HEAD? >Also, looking at this, I note that "~/.pg_service.conf" is still a >platform-ism. We could make that slightly better by writing >"$HOME/.pg_service.conf", but is that good enough? Agreed. Besides, A lot of places in current Doc have been useing ~/.pg_*** already. IMHO, it's good to keep the consistency. Regards, Tang
"tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com> writes:
> On Thursday, May 27, 2021 10:58 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote
>> So I ended up with the attached --- what do you think?
> The doc-fix LGTM.
> But I couldn't apply the patch at HEAD(2941138e60). Maybe you did the fix at a branch other than HEAD?
No ... "patch -p1 <~/improve-pgservice-docs.patch" works fine for me.
>> Also, looking at this, I note that "~/.pg_service.conf" is still a
>> platform-ism. We could make that slightly better by writing
>> "$HOME/.pg_service.conf", but is that good enough?
> Agreed. Besides, A lot of places in current Doc have been useing ~/.pg_*** already.
> IMHO, it's good to keep the consistency.
Oh, good point ... grepping finds lots of occurrences of '~/' and
only one of '$HOME/'. Seems like we ought to change that one:
$ grep -r 'HOME/' .
./config.sgml: This writes out files to <filename>$HOME/.debug/jit/</filename>; the
regards, tom lane
On Thursday, May 27, 2021 11:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote >No ... "patch -p1 <~/improve-pgservice-docs.patch" works fine for me. Oh, never tried "patch -p1" command before but it worked. Thanks!! BTW, here is my previous operation to apply your patch(failed, which is strange IMO) $ git apply --3way improve-pgservice-docs.patch error: patch failed: doc/src/sgml/libpq.sgml:8091 Falling back to three-way merge... error: patch failed: doc/src/sgml/libpq.sgml:8091 error: doc/src/sgml/libpq.sgml: patch does not apply >grepping finds lots of occurrences of '~/' and >only one of '$HOME/'. Seems like we ought to change that one: > >$ grep -r 'HOME/' . >./config.sgml: This writes out files to <filename>$HOME/.debug/jit/</filename>; the Agreed, thanks for the comment. Attached a patch to combine the above fix in your patch. Also fixed a typo in install-windows.sgml. Also fixed pgbuffercache.sgml as Michael commented at [1]. [1] https://www.postgresql.org/message-id/YK4A4g07AvGa9FVF%40paquier.xyz Regards, Tang
Attachment
"tanghy.fnst@fujitsu.com" <tanghy.fnst@fujitsu.com> writes:
> On Thursday, May 27, 2021 11:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote
>> No ... "patch -p1 <~/improve-pgservice-docs.patch" works fine for me.
> Oh, never tried "patch -p1" command before but it worked. Thanks!!
> BTW, here is my previous operation to apply your patch(failed, which is strange IMO)
> $ git apply --3way improve-pgservice-docs.patch
Yeah, "git apply" is extremely picky. I'm not sure we've ever figured out
all the reasons why it sometimes doesn't work. Good old "patch" pretty
much always works though.
> Attached a patch to combine the above fix in your patch.
> Also fixed a typo in install-windows.sgml.
> Also fixed pgbuffercache.sgml as Michael commented at [1].
LGTM, pushed with one minor additional tweak.
regards, tom lane