pg_type_d.h location incorrect

Hi

Attached a patch to fix the incorrect location description of pg_type_d.h posted by myself at [1]/messages/by-id/162149020918.26174.7150424047314144297@wrigleys.postgresql.org.

[1]: /messages/by-id/162149020918.26174.7150424047314144297@wrigleys.postgresql.org

Regards,
Tang

"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

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

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]/messages/by-id/YK4A4g07AvGa9FVF@paquier.xyz.

[1]: /messages/by-id/YK4A4g07AvGa9FVF@paquier.xyz

Regards,
Tang

"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