Re: Naming of network_ops vs. inet_ops for SP-GIST

[Michael Paquier]

On Tue, Jan 24, 2023 at 03:22:44PM -0500, Tom Lane wrote:
> I wonder whether we shouldn't just revert this table to
> showing opclass names, and avert our eyes from the theoretical
> inconsistency.  Michael, looks like it was your 7a1cd5260
> that changed it; what do you think?

Yes, the docs should be fixed here.  The intention is not to show the
operator families but the names of the opclasses.  I can only spot one
difference in SpGiST for network_ops -> inet_ops as of the report.
BRIN, GIN and GiST look to be clean after a second lookup.

I don't have a strong opinion about the naming inconsistency between
the opclass name and the opfamily name in this case, though, couldn't
it create more problems than actually fix something?

Anyway, attached is a patch for the docs.  Thoughts?
--
Michael
diff --git a/doc/src/sgml/spgist.sgml b/doc/src/sgml/spgist.sgml
index 00432512de..102f8627bd 100644
--- a/doc/src/sgml/spgist.sgml
+++ b/doc/src/sgml/spgist.sgml
@@ -91,18 +91,7 @@
  |>> (box,box)
 
  
-  kd_point_ops
-  |>> (point,point)
-  <-> (point,point)
- 
- << (point,point)
- >> (point,point)
- <<| (point,point)
- ~= (point,point)
- <@ (point,box)
-
- 
-  network_ops
+  inet_ops
   << (inet,inet)
   
  
@@ -117,6 +106,17 @@
  >= (inet,inet)
  && (inet,inet)
 
+ 
+  kd_point_ops
+  |>> (point,point)
+  <-> (point,point)
+ 
+ << (point,point)
+ >> (point,point)
+ <<| (point,point)
+ ~= (point,point)
+ <@ (point,box)
+
  
   poly_ops
   << (polygon,polygon)


signature.asc
Description: PGP signature

Re: Naming of network_ops vs. inet_ops for SP-GIST

[Michael Paquier]

On Tue, Jan 24, 2023 at 10:38:28PM -0500, Tom Lane wrote:
> Works for me.

Thanks for checking, applied that.
--
Michael


signature.asc
Description: PGP signature

Re: meson doc: Is "-Dtap-tests" a typo?

[Michael Paquier]

On Tue, Feb 14, 2023 at 09:06:13AM +, Katsuragi Yuta wrote:
> The parameter name "-Dtap-tests" in the meson's doc seems to be a typo.
> I think "-Dtap_tests" is the correct name.  Attached patch fixes that.
> 
> I got an unknown options error by using "-Dtap-tests=enabled". However,
> meson_options.txt told me the correct name was "-Dtap_tests".

  
-  -Dtap-tests={ auto | enabled | disabled }
+  -Dtap_tests={ auto | enabled | disabled }

And meson.build refers to the same term, as of "tap_tests":
# Check whether tap tests are enabled or not
tap_tests_enabled = false
tapopt = get_option('tap_tests')

So you are indeed right.  Nice catch.  Will fix.
--
Michael


signature.asc
Description: PGP signature

Re: Add missing meson arguments in docs

[Michael Paquier]

On Tue, Feb 21, 2023 at 10:40:12AM +0100, Jelte Fennema wrote:
> The -Dcassert and -Db_coverage of the meson build didn't show in the
> docs that they needed to be passed true or false. All other options
> specified the arguments they expected. This patch fixes that.

Indeed, values have to be specified after the equal sign of each
option, so applied what you are suggesting here.

-Db_coverage is part of the core options of meson because it is not
listed in meson_options.txt, no?  Documenting the switch is fine, but
it seems like we lack contents here.  The paragraph describing
-Db_coverage links to regress-coverage, still this only has
instructions for ./configure.  Shouldn't we have some docs to show one
how to achieve the same with meson?  I'd guess that a separate section
for meson would make the most sense, for as long as we support both
methods in parallel.

https://wiki.postgresql.org/wiki/Meson tells nothing about that, for
one.
--
Michael


signature.asc
Description: PGP signature

Re: Add missing meson arguments in docs

[Michael Paquier]

On Wed, Feb 22, 2023 at 09:24:37AM +0100, Peter Eisentraut wrote:
> Yeah, this is just in the long tail of things to work through.

I am going through that, and found out what can be done to do coverage
reports.  I will post a patch separately on -hackers.
--
Michael


signature.asc
Description: PGP signature

Re: incorrect info in dblink examples

[Michael Paquier]

On Mon, Feb 27, 2023 at 07:24:37PM +, PG Doc comments form wrote:
> In your examples,   AS t1(proname name, prosrc text)
> should actually be   AS t1(proname text, prosrc text)
> 
> This occurs frequently in the documentation at the following link: 
> https://www.postgresql.org/docs/current/contrib-dblink-function.html

Why?  pg_proc.proname uses "name" as data type, so this is not wrong.
--
Michael


signature.asc
Description: PGP signature

Re: Certificate authentication docs in multiple places

[Michael Paquier]

On Tue, Mar 28, 2023 at 04:28:24PM +0200, Peter Eisentraut wrote:
> Sure, some cross-linking between those two sections seems sensible.

Steve, would you like to propose a patch?
--
Michael


signature.asc
Description: PGP signature

Re: Certificate authentication docs in multiple places

[Michael Paquier]

On Mon, Apr 10, 2023 at 09:47:08AM +0100, Steve Atkins wrote:
> Yes, I’ll do that once I get some time to get the doc toolchain working.

Well, if you don't want to spend time setting up a toolchain, feel
free to send a patch just hacking the SGML files.  I am OK to take it
from there, as long as the idea behind the patch is clear.
--
Michael


signature.asc
Description: PGP signature

Re: Typo

[Michael Paquier]

On Tue, May 23, 2023 at 08:52:25PM +, PG Doc comments form wrote:
> There appears to be a typo, here:
> https://www.postgresql.org/docs/current/history.html#:~:text=Postgres95%20code%20was%20completely%20ANSI%20C.
> A word or two should be added between 'completely' and 'ANSI C', such as
> 're-written in', or 're-coded using', or some such.

This is the current sentence, and it sounds kind of OK to me, FWIW:
"Postgres95 code was completely ANSI C and trimmed in size by 25%.
--
Michael


signature.asc
Description: PGP signature

Re: Change "two" to "three" for decades of development in history

[Michael Paquier]

On Fri, Jun 23, 2023 at 03:17:26AM +0200, Erik Wienhold wrote:
> An SGML entity [0] or an xsltproc stringparam [1] looks viable.  Question is
> how to calculate the number of decades in the Makefile.  It's trivial in SQL 
> :)

-  University of California at Berkeley.  With over two decades of
+  University of California at Berkeley.  With over three decades of
   development behind it, PostgreSQL is now
   the most advanced open-source database available anywhere.

Seeing what has been committed, I don't think that such complications
are necessary if this sentence is reworded without numbers.  For
instance:
"With multiple decades of development behind it, PostgreSQL.."

My 2c.
--
Michael


signature.asc
Description: PGP signature

Re: Change "two" to "three" for decades of development in history

[Michael Paquier]

On Fri, Jun 23, 2023 at 10:50:20PM -0400, Bruce Momjian wrote:
> Applied patch is "With decades of development".

Thanks.
--
Michael


signature.asc
Description: PGP signature

Re: Improvement of clarity in pg_ctl command docummentation

[Michael Paquier]

On Sat, Jul 15, 2023 at 05:57:40AM +, PG Doc comments form wrote:
> I was reading the documentation about pg_ctl and there everything was well
> written about the usage and option that can be used with the pg_ctl command
> but there is not mentioned that we can not run pg_ctl command as root and
> why we cant run it as root.

Yes, I guess that you are right in the fact that we don't document
pg_ctl cannot be run as root.  The short explanation behind this
restriction is that the PostgreSQL backend is considered as having the
same rights as the OS user running it, and we don't want to encourage
unsecure behaviors where systems could be easily compromised.
--
Michael


signature.asc
Description: PGP signature

Re: Documentation does not mention that basebackup could not be used on newer major version

[Michael Paquier]

On Mon, Sep 18, 2023 at 10:08:30PM +0200, Laurenz Albe wrote:
> On Mon, 2023-09-18 at 15:29 +, PG Doc comments form wrote:
>> My main issue is that `pg_basebackup` page does not mention that this backup
>> is compatible only with current version of database. I can not do basebackup
>> on v11 and restore that on v15, for example.
> 
> I think that that is amply documented by the fact that pg_basebackup is
> *not* mentioned in https://www.postgresql.org/docs/current/upgrading.html .

Yeah.  The issue with this one is that it does a direct physical copy
of the files.  You cannot expect pg_basebackup to be able to do all
the work that pg_upgrade would do underground, like binary upgrades,
and more.

> You can't expect us to enumerate every tool that is not suitable for
> upgrading.

Just noting in passing.  There is a lot of maintenance effort for
downward compatibility (tools like pg_dump and pg_basebackup at
version N are able to work with a backend version older, say at N-1).
Upward compabitility may work in some cases, even for dumps, still
these would likely require extra manipulation to be able to load to
a version of the backend older than the version of pg_dump used.
Being able to use pg_basebackup to work with older backend versions is
a really important property we try to keep available.
--
Michael


signature.asc
Description: PGP signature

Re: Minipatch concerning tags

[Michael Paquier]

On Fri, Oct 20, 2023 at 11:22:55AM +0300, Maxim Yablokov wrote:
> PostgresPro documentation team (Ekaterina Kiryanova, Elena Indrupskaya, Oleg
> Sibiryakov and me) have prepared a patch with some improvements concerning
> missed/inapt tags. Please have a look.

Most of what you are proposing makes sense.

> - It means that, for walsenders which are lagging (if any), some WAL 
> records up
> + It means that, for walsenders that are lagging (if 
> any), 

But not this one.  This could just be "WAL senders", but we also use
the term "walsender" in physical and logical replication areas, so a
plural looks OK for me here, leaving it as it is now.
--
Michael


signature.asc
Description: PGP signature

Re: Minipatch concerning tags

[Michael Paquier]

On Sat, Oct 21, 2023 at 04:11:50PM +0900, Michael Paquier wrote:
> But not this one.  This could just be "WAL senders", but we also use
> the term "walsender" in physical and logical replication areas, so a
> plural looks OK for me here, leaving it as it is now.

I have reverted this change, added one markup for a catalog_xmin that
was missed, fixed a few whitespaces, and applied it on HEAD. 

Thanks!
--
Michael


signature.asc
Description: PGP signature

Re: Minipatch concerning tags + new spelling/punctuation patch

[Michael Paquier]

On Mon, Oct 23, 2023 at 12:33:03PM +0300, Maxim Yablokov wrote:
> We have also prepared a small patch concerning spelling, punctuation, etc.
> We've decided to send it separately, so it'll be easier to review. Please
> have a look.

Yep, these are improvements.  So applied and backpatched (all these
applied down to 16, except for one down to 11).

> (just in case: e.kiryan...@postgrespro.ru, e.indrupsk...@postgrespro.ru,
> o.sibirya...@postgrespro.ru, m.yablo...@postgrespro.ru)

Thanks for the list.  I've used that in the commit message.
--
Michael


signature.asc
Description: PGP signature

Re: Typo in "43.9.1. Reporting Errors and Messages"?

[Michael Paquier]

On Tue, Oct 31, 2023 at 09:00:00PM +0300, Alexander Lakhin wrote:
> I don't remember details, but I think the primary reason for the change
> was that "RAISE_EXCEPTION" occurred in the whole tree only once (before
> 66bde49d96). Now I see, that I had chosen the wrong replacement — I agree
> with Euler, change to "raise_exception" would be more appropriate.

Indeed, it looks like the origin of the confusion is the casing here,
so changing to "raise_exception" like in the appendix sounds good to
me:
https://www.postgresql.org/docs/devel/errcodes-appendix.html

So you mean something like the attached then?

> (I've found a similar mention of ERRCODE_xxx in btree.sgml:
>   Before doing so, the function should check the sign
>   of offset: if it is less than zero, raise
>   error ERRCODE_INVALID_PRECEDING_OR_FOLLOWING_SIZE (22013)
>   with error text like invalid preceding or following size in window
>   function.
> but I think that's okay here, because that identifier supposed to be used
> as-is in ereport/elog.)

Yep, still this one is not that old (0a459cec96d3).
--
Michael
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 83f867f91d..5977534a62 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -3942,7 +3942,7 @@ RAISE unique_violation USING MESSAGE = 'Duplicate user ID: ' || user_id;

 If no condition name nor SQLSTATE is specified in a
 RAISE EXCEPTION command, the default is to use
-ERRCODE_RAISE_EXCEPTION (P0001).
+raise_exception (P0001).
 If no message text is specified, the default is to use the condition
 name or SQLSTATE as message text.



signature.asc
Description: PGP signature

Re: Typo in "43.9.1. Reporting Errors and Messages"?

[Michael Paquier]

On Wed, Nov 01, 2023 at 09:18:47AM +0900, Michael Paquier wrote:
> So you mean something like the attached then?

Fixed that with f8b96c211da0 down to 11, in time for next week's
release set.
--
Michael


signature.asc
Description: PGP signature

Re: Delete description of trigger file

[Michael Paquier]

On Fri, Dec 01, 2023 at 11:54:14AM +0900, Shinya Kato wrote:
>  later disconnected, the standby goes back to step 1 and tries to
>  restore the file from the archive again. This loop of retries from the
>  archive, pg_wal, and via streaming replication goes 
> on until the server
> -is stopped or failover is triggered by a trigger file.
> +is stopped.

Just removing this information looks incorrect to me, because a
promotion would cause the retries to stop the WAL lookups.  Shouldn't
the last part of this sentence be reworded as of a "or is promoted"?
--
Michael


signature.asc
Description: PGP signature

Re: Delete description of trigger file

[Michael Paquier]

On Sat, Dec 02, 2023 at 05:52:50PM +0900, Shinya Kato wrote:
> Thanks for the review.
> Yes, that's true. A new patch is attached.

Thanks, applied.
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] Fix link to pg_ident_file_mappings

[Michael Paquier]

On Wed, Feb 21, 2024 at 08:50:59AM +0100, Daniel Gustafsson wrote:
> On 21 Feb 2024, at 03:24, Erik Wienhold  wrote:
>> 
>> The docs on pg_reload_conf() in v15, v16, and devel have an incorrect
>> link to pg_ident_file_mappings.  The attached patch fixes that.
> 
> Nice catch, will fix.

Thanks you both.  This looks like a copy-pasto from a2c84990bea7.\
--
Michael


signature.asc
Description: PGP signature

Re: column_name of ALTER MATERIALIZED VIEW should only refer to an existing column

[Michael Paquier]

On Wed, May 22, 2024 at 02:59:37AM +0200, Erik Wienhold wrote:
> Here's a patch for $SUBJECT.  Looks like the current wording was copied
> from ALTER TABLE.  In ALTER VIEW we correctly state that column_name
> must be an existing column.

Fun.  You are right, none of the patterns supported by this query are
able to add an attribute, so this new formulation makes sense.

Will fix once we are out of release freeze time on HEAD.  Thanks!
--
Michael


signature.asc
Description: PGP signature

Re: column_name of ALTER MATERIALIZED VIEW should only refer to an existing column

[Michael Paquier]

On Wed, May 22, 2024 at 02:12:36PM +0900, Michael Paquier wrote:
> Will fix once we are out of release freeze time on HEAD.  Thanks!

And done as of dd087e1c13bf.
--
Michael


signature.asc
Description: PGP signature

Re: A minor bug in the doc of "SQL Functions Returning Sets" in xfunc.sgml.

[Michael Paquier]

On Fri, Jul 19, 2024 at 10:46:04AM +0900, 日向充 wrote:
> I have found executable examples that do not work correctly
> in the doc of "SQL Functions Returning Sets" in xfunc.sgml.
> So I fixed the examples as follows.
> - Changed CREATE TABLE tab
>   '(y int, z int)' to '(x int, y int, z int)'
> - Changed INSERT INTO tab
>   '(1, 2), (3, 4), (5, 6), (7, 8)'
>   to '(1, 2, 3), (4, 5, 6), (7, 8, 9), (10, 11, 12)'
> - Changed CREATE FUNCTION sum_n_product_with_tab
>   '(x int, OUT sum int, OUT product int)'
>   to '(int, OUT sum int, OUT product int)'
> - Changed CREATE FUNCTION sum_n_product_with_tab
>   'SELECT $1 + tab.y, $1 * tab.y FROM tab;'
>   to 'SELECT $1 + tab.x, $1 * tab.x FROM tab;'
> - Changed result of "SELECT * FROM sum_n_product_with_tab(10);"
> 
> The above will improve the results of examples as follows in this chapter.
> 
> Do you think?

Not sure that this is worth changing.  The examples work OK when taken
in isolation or are able to demonstrate the point they want to show.
In short, not all these queries are here to be compatible with the
contents in the same area.  See for example the case of the "nodes"
table on the same page, created nowhere.  "tab" is just a more generic
table name that's more spread.
--
Michael


signature.asc
Description: PGP signature

Re: Undocumented optionality of handler_statements

[Michael Paquier]

On Mon, Jul 22, 2024 at 01:55:52PM +, PG Doc comments form wrote:
> In
> https://www.postgresql.org/docs/16/plpgsql-control-structures.html#PLPGSQL-ERROR-TRAPPING
> handler_statements are documented as optional.
> 
> However, the following example shows that handler_statements can be omitted.

You have a good point.  This could be clarified better in the
documentation by making handler_statements conditional with square
brackets around it.  I'd rather add an extra sentence to tell that not
specifying handler_statements is equivalent to taking no action.

Perhaps you would like to write a patch?
--
Michael


signature.asc
Description: PGP signature

Re: Table rewrite supporting functions for event triggers

[Michael Paquier]

On Tue, Sep 03, 2024 at 09:34:02PM +0200, Laurenz Albe wrote:
> On Tue, 2024-09-03 at 11:54 -0400, Greg Sabino Mullane wrote:
>> How about something like this?
> 
> This patch looks good to me.

-Returns a code explaining the reason(s) for rewriting.  The exact
-meaning of the codes is release dependent.
+Returns a code explaining the reason(s) for rewriting. The value is
+a bitmap built from the following values: 1 (the table has changed
+persistence), 2 (a column has changed a default value), 4 (a column
+has a new data type), and 8 (the table access method has changed).

Agreed that the user experience with this function is poor and that
the documentation should be improved.  Still, I am not sure that this
is optimal.  On top of the values, how about adding the variable names
and also mention that these are defined in event_trigger.h?

Putting the documentation change aside for a bit, could it be better
to redesign this function and return a text value rather than an
integer?  We could directly return the names, minus "AT_REWRITE_", for
instance.
--
Michael


signature.asc
Description: PGP signature

Re: Undocumented optionality of handler_statements

[Michael Paquier]

On Tue, Jul 23, 2024 at 01:25:39PM +0200, Philipp Salvisberg wrote:
> read "optional" as "mandatory".

They're optional, like in empty being optional.  If not specified, the
block goes to its END.

> Therefore, I suggest to change this example by adding a NULL
> statement as in other examples. This change would make the
> documentation consistent and handle the optionality of
> handler_statements as an implementation detail. I created a patch
> for plpgsql.sgml based on the master branch, adding a NULL statement
> in empty exception handlers (see attached file
> doc_patch_using_null_stmt_instead_of_empty_exception_handler_v1.diff).

These examples have been around for 20 years with, and I think that it
is helpful to show this pattern as well.  So if I were to do something
about that, I would suggest the attached.
--
Michael
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 78e4983139..3a5e7bc296 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -2804,9 +2804,9 @@ BEGIN
 statements
 EXCEPTION
 WHEN condition  OR condition ...  THEN
-handler_statements
+ handler_statements 
  WHEN condition  OR condition ...  THEN
-  handler_statements
+   handler_statements 
   ... 
 END;
 
@@ -2821,8 +2821,8 @@ END;
  abandoned, and control passes to the EXCEPTION list.
  The list is searched for the first condition
  matching the error that occurred.  If a match is found, the
- corresponding handler_statements are
- executed, and then control passes to the next statement after
+ corresponding handler_statements, if
+ specified are executed, and then control passes to the next statement after
  END.  If no match is found, the error propagates out
  as though the EXCEPTION clause were not there at all:
  the error can be caught by an enclosing block with


signature.asc
Description: PGP signature

Re: Table rewrite supporting functions for event triggers

[Michael Paquier]

On Wed, Sep 11, 2024 at 10:14:27AM -0400, Greg Sabino Mullane wrote:
>  I dunno - so would we smush them together and return something like:
> 
> "ALTER_PERSISTENCE and COLUMN_REWRITE"

If multiple are set, let's just make it text[], then.

> That would be a step backwards for anyone possibly using that integer
> programatically to (for example) give a pretty user-facing message about
> why the event was triggered.

I don't know either how much people are relying on these numbers in
applications.  If this is like what we do in the regression tests and
print it in notice messages within a PL/pgSQL function, that's not
going to matter.

Or just have a separate function..

Do you have a comment about mentioning the variables or the header in
the docs for the stable branches?  I'm aware that this is a rare
practice, but so is this function's design.  My argument is
greppability between the code and the docs, mainly, to not miss an
update of the docs if more reasons are added.  That would be unlikely,
but a backpatch of a reason is not impossible ABI-wise.
--
Michael


signature.asc
Description: PGP signature

Re: Table rewrite supporting functions for event triggers

[Michael Paquier]

On Thu, Sep 12, 2024 at 08:52:00AM -0400, Greg Sabino Mullane wrote:
> I do like the simplicity of the bitmap:
> 
> if (reason & 1)
>   print "Table has changed from logged to unlogged"
> if (reason & 2)
>   print "Default has been changed"
> 
> versus with text[]:
> 
> foreach reason in tablereason[]
>   if reason.match_exact("ALTER_PERSISTENCE")
> print "Table has changed from logged to unlogged"
>   if reason.match_regex("DEFAULT")
> print "Default has been changed"
> ...

Okay.  I am not going to be annoying with compatibility, then :D

> My initial reaction was that this is indeed a rare case, and to avoid
> putting that level of code detail in the docs. Your argument is a good one,
> but it still feels wrong to put that there. Yes, this puts a little more
> onus on future developers, but updating the docs is already a core
> requirement for patches.
> 
> (On reflection, maybe reverse it - put a code comment in event_trigger.h
> reminding people to also update the docs? But again, that's seems like
> something obvious anyway for someone making that change.)

I am not so sure, TBH.  One example: I know these values in
tablecmds.c for some time because that's an area I tend to focus on
for bug fixes, but forgot entirely about the SQL function in event
triggers that feed on it until I found your post.  A comment in
event_trigger.h to mention the doc update would work for me.  That
would be impossible to miss.
--
Michael


signature.asc
Description: PGP signature

Re: PostgreSQLドキュメントバグ報告_9.3. 算術関数と演算子

[Michael Paquier]

葛西さま

2017-11-22 17:46 GMT+09:00 "葛西 裕昭(システム開発部-開発9チーム CLINKS)" :
> 突然失礼いたします。
> 最新の PostgreSQL 9.6.5文書 について、
> ドキュメントのバグと思う箇所がありましたので、報告いたします。
>
> 9.3. 算術関数と演算子
> 表9.5 算術関数
> 3行目　ceil(dp or numeric)
>
> 上記行の「戻り値型」が抜けており、「説明」以右がずれております。
>
> 対象のURLは下記の通りです。
> https://www.postgresql.jp/document/9.6/html/functions-math.html#functions-math-func-table

本メーリングリストは英語のリストなので、日本語の場合はhttps://www.postgresql.jp/npo/mailinglistからご連絡ください。
よろしくお願い致します。

Or in English: I am redirecting Kasai-san to the Japanese mailing lists.
-- 
Michael

Re: libpq options

[Michael Paquier]

On Wed, Nov 22, 2017 at 10:27 PM,   wrote:
> The following documentation comment has been logged on the website:
>
> Page: https://www.postgresql.org/docs/9.6/static/libpq-connect.html
> Description:
>
> The documentation refers the libpq connection option "replication" 
> here in
> "Streaming Replication Protocol" page;
>
> https://www.postgresql.org/docs/current/static/protocol-replication.html
>
> However the option "replication" is not clarified (or even listed) 
> in libpq
> options page.
>
> https://www.postgresql.org/docs/current/static/libpq-connect.html#LIBPQ-PARAMKEYWORDS
>
> I searched the source and find that pg_conn struct has this
>
> struct pg_conn
> {
> ...
>   char *replication; /* connect as the replication standby? */
> ...
> }
>
> Adding something simple like above and referring back to one of replication
> pages would be good I think.

Yeah, it is mainly a developer option which is why I guess it is not
documented. Like you, I think it should be added as part of the
connection parameter, and mentioned it a couple of days back:
https://www.postgresql.org/message-id/CAB7nPqQAtKfG3H%2BuK11JNivtJtZYE9yVCrPuejRMjp8tUDe0nQ%40mail.gmail.com
-- 
Michael

Re: libpq options

[Michael Paquier]

On Wed, Nov 22, 2017 at 11:36 PM, Michael Paquier
 wrote:
> Yeah, it is mainly a developer option which is why I guess it is not
> documented. Like you, I think it should be added as part of the
> connection parameter, and mentioned it a couple of days back:
> https://www.postgresql.org/message-id/CAB7nPqQAtKfG3H%2BuK11JNivtJtZYE9yVCrPuejRMjp8tUDe0nQ%40mail.gmail.com

Attached is a patch as an attempt to bring together the best of both
worlds. The idea is to move the description of how the connection
parameter replication works from the replication protocol page into
the section of libpq dedicated to connection parameters, and add links
between both sections.

Thoughts?
-- 
Michael
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 4703309254..09bfcbbec3 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1222,6 +1222,24 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
  
 
+ 
+  replication
+  
+  
+   This option determines if a backend should use the replication
+   protocol. A Boolean value of true tells the backend
+   to go into walsender mode, wherein a small set of replication commands
+   can be issued instead of SQL statements. Only the simple query protocol
+   can be used in walsender mode. Passing database
+   as the value instructs walsender to connect to the database specified
+   in the dbname parameter, which will allow the
+   connection to be used for logical replication from that database.
+   For a detailed description about the replication protocol, consult
+   .
+  
+  
+ 
+
  
   sslmode
   
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 8174e3defa..b85a3edda9 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -1630,15 +1630,9 @@ supported at the moment is tls-unique, defined in RFC 5929.
 
 
 To initiate streaming replication, the frontend sends the
-replication parameter in the startup message. A Boolean value
-of true tells the backend to go into walsender mode, wherein a
-small set of replication commands can be issued instead of SQL statements. Only
-the simple query protocol can be used in walsender mode.
-Replication commands are logged in the server log when
+ connection parameter in the
+startup message. Replication commands are logged in the server log when
  is enabled.
-Passing database as the value instructs walsender to connect to
-the database specified in the dbname parameter, which will allow
-the connection to be used for logical replication from that database.
 
 
  For the purpose of testing replication commands, you can make a replication

Re: libpq options

[Michael Paquier]

On Tue, Nov 28, 2017 at 5:00 PM, Şahap Aşçı  wrote:
> I think this solves the consistency issue that i am talking about. Well, i am 
> just looking from documentation user point of view.

Thanks. Input is always welcome. I have added an entry in the commit
fest app so as this is not lost:
https://commitfest.postgresql.org/16/1387/
-- 
Michael

Re: followed by example doesn't work

[Michael Paquier]

On Mon, Dec 4, 2017 at 11:48 PM, Tom Lane  wrote:
> Rudy Barbieri  writes:
>> But it could be better improve the documentation highlighting that in
>> previous versions there was a bug.
>
> I see no bug here ... failing to recognize an operator that was added in
> later versions can hardly be classed as a bug.

If you wish to have a given operator supported in a past version of
Postgres, you could have it done in the shape of an extension as well.
There are many people and companies able to do so if you cannot.
-- 
Michael

Re: Replication parameters in recovery.conf

[Michael Paquier]

On Wed, Dec 6, 2017 at 11:19 AM, Peter Eisentraut
 wrote:
> The reasons for this are basically all historical and we are trying to
> get rid of it (by moving these settings to postgresql.conf, mostly).  So
> I don't think we need to spend a lot of time rationalizing this at this
> point.

Yeah, there are as well parameters that could get removed on the way,
like hot_standby for example. Most deployments don't use it to off
these days, and in Postgres 10 this moves makes even more sense as
wal_level = replica maps to both "archive" and "hot_standby", but
means the latter.
-- 
Michael

Re: Missing column_constraint explanation

[Michael Paquier]

On Wed, Dec 20, 2017 at 6:08 PM, PG Doc comments form
 wrote:
> The following documentation comment has been logged on the website:
>
> Page: https://www.postgresql.org/docs/9.6/static/sql-altertable.html
> Description:
>
> Missing column_constraint explanation in parameters section

Those docs say already that ADD COLUMN follows the same grammar as
CREATE TABLE, which basically means that there is no need to duplicate
the same definition in two places. Note that the same thing applies to
table_constraint.
-- 
Michael

Re: Missing column_constraint explanation

[Michael Paquier]

On Thu, Dec 21, 2017 at 12:15 PM, Stephen Frost  wrote:
> Now, if we could do that in such a way that we avoid having to actually
> duplicate the 'source' for these productions into different places in
> the documentation, that would be fantastic because it certainly isn't
> fun having to find all the places that need to be updated, but I'm not
> sure how easy that would be to do (and to make work with how psql's help
> is generated...).

You are looking for something like how feature-supported.sgml is
handled after its automatic generation, except that in this case you
just create a new sgml file which has the definition data you want to
load, define it with , and then load
it using something like an entity &blah; in the CREATE or ALTER TABLE
docs. That's a bit of refactoring though, but you could shape it by
putting all those lower-level definitions in a subdirectory like
sgml/defs or such, avoiding any duplication in those definitions.
-- 
Michael

Re: Missing column_constraint explanation

[Michael Paquier]

On Sat, Jan 13, 2018 at 09:06:22PM -0500, Stephen Frost wrote:
> I'm not really sure that we want to go there for this case though.
> Perhaps others disagree, but that seems like a lot to avoid this
> particular duplication, which really isn't all that bad.
> 
> This patch also seems to have gotten lost in the shuffle of things, but
> it still applies cleanly and I took another look at it today and it
> looks good to me, so I'm going to stick it in the CF and mark it as
> Needs Review for now.  Perhaps someone else can give it another
> once-over to make sure everything looks good and, if so, mark it as
> Ready for Committer and then I'll take care of it.

I may be missing something, but no patch is attached to this
thread. Honestly I think that duplicating this code should be avoided,
and the patch to produce is not that complicated technically. So are you
planning to just duplicate hte definitions in CREATE TABLE to ALTER
TABLE? This is a recipy for forgetting updates in the future on one
page or the other...
--
Michael


signature.asc
Description: PGP signature

Re: Missing column_constraint explanation

[Michael Paquier]

On Sun, Jan 14, 2018 at 09:33:13AM -0500, Stephen Frost wrote:
> I'm not really sure why the thread keeps getting broken, but the
> original was here:
> 
> https://www.postgresql.org/message-id/flat/CAB_COdgOoA=G18RhWPoW8zZ+xOxTns7xD7psHA=ct+xccok...@mail.gmail.com#CAB_COdgOoA=G18RhWPoW8zZ+xOxTns7xD7psHA=ct+xccok...@mail.gmail.com
> 
> where the latest patch is from Amit.

Indeed I missed this one, thanks. This one is a new thread, where
somebody has commented directly on the docs of the website.

> Further, as mentioned upthread in
> https://www.postgresql.org/message-id/20171221031511.GX4628%40tamriel.snowman.net
> I'm concerned that this wouldn't be very easy to make work with the way
> psql's documentation is generated.  Did you take a look at how that
> works with create_help.pl?  I don't see create_help.pl as supporting
> 'entity'.  We could perhaps fix that, but I'm not really sure it's worth
> it and it's certainly quite a bit to add on to this particular patch.

Good point here.
--
Michael


signature.asc
Description: PGP signature

Re: Correction of intermediate certificate handling

[Michael Paquier]

On Mon, Jan 15, 2018 at 07:22:38PM -0500, Bruce Momjian wrote:
> I asked Stephen Frost and David Steele for details on the arcane art of
> SSL certificate creation.  They showed me scripts they use and explained
> that they properly pass intermediate certificates to clients.  The trick
> was to use the v3_ca extension when creating root and intermediate
> certificates.
> 
> My talk documents this behavior.  In this talk:
> 
>   https://momjian.us/main/writings/pgsql/tls.pdf
> 
> slide 47 and 49 use -extensions v3_ca.  Slides 73 and 74 show that the
> intermediate is not needed on the client if it is created with v3_ca and
> exist on the server.  Slide 75 shows that the server certificate must be
> first in server.crt.
> 
> I have created the attached doc patch to add this information to our
> docs.  I would like to backpatch this since what we have now, while it
> works, is inaccurate.

I have spent some time looking at your patch, this gets a +1 from here.

This bit is important. I am happy that your patch mentions that
intermediate certificates avoid the need to store root ones on the
client. Should the docs mention terms like "chain of trust"?

Perhaps the docs could also include an example of command to create a
root and an intermediate certificate in runtime.sgml or such?

On top of that, src/test/ssl does not provide any kind of coverage for
that. It would be an area of improvement for those tests.
--
Michael


signature.asc
Description: PGP signature

Re: Correction of intermediate certificate handling

[Michael Paquier]

On Tue, Jan 16, 2018 at 11:21:22AM -0500, Bruce Momjian wrote:
> On Tue, Jan 16, 2018 at 02:33:05PM +0900, Michael Paquier wrote:
> > This bit is important. I am happy that your patch mentions that
> > intermediate certificates avoid the need to store root ones on the
> > client. Should the docs mention terms like "chain of trust"?
> 
> I think the question is how much do we want to "teach" people in our
> docs.  We do oddly but wisely link from our docs to HP OpenVMS docs
> about how the chain of trust works:
> 
>   http://h41379.www4.hpe.com/doc/83final/ba554_90007/ch04s02.html
> 
> I will write up a paragraph about the concepts for our docs for the
> group's review.

As a separate patch, I think that it would be fine as well.

> > Perhaps the docs could also include an example of command to create a
> > root and an intermediate certificate in runtime.sgml or such?
> 
> Yes, I have thought about that.  My presentation has clear examples that
> we can use, again based on Stephen and David's scripts using v3_ca.  I
> will work up a possible patch for that too.

That too.

> > On top of that, src/test/ssl does not provide any kind of coverage for
> > that. It would be an area of improvement for those tests.
> 
> Wow, I have no idea how to do that.  Let me look.  Seems I have more
> work to do.

You would need to update src/test/ssl/Makefile to generate those
intermediate CAs, and then make ServerSetup::switch_server_cert smarter
in the way the series of certificates are handled. A suggestion I have
would be to create each certificate file separately and change the
routine so as it uses an array in input, the order of the items defining
what's the order the the data. For the client there is sslrootcert, so I
guess that a small routine able to take a set of certs and append them
to a single file would make it as well (switch_server_cert should use
it).

> Instead of appending to this doc patch, I will work on a second one for
> review.

I see nothing pressing here. If you are not familiar with the TAP test
facility, this could give you a good introduction to it.
--
Michael


signature.asc
Description: PGP signature

Re: Correction of intermediate certificate handling

[Michael Paquier]

On Tue, Jan 16, 2018 at 10:23:44PM -0500, Bruce Momjian wrote:
> On Wed, Jan 17, 2018 at 09:09:50AM +0900, Michael Paquier wrote:
> > On Tue, Jan 16, 2018 at 11:21:22AM -0500, Bruce Momjian wrote:
> > > On Tue, Jan 16, 2018 at 02:33:05PM +0900, Michael Paquier wrote:
> 
> I ended up merging the "chain of trust" changes into the "intermediate"
> patch since they affect adjacent sections of the docs.  You can see this
> as the first attached patch.

Thanks. I looked at crt.diff and the surroundings in the docs. This one
looks consistent to me.

> > > > Perhaps the docs could also include an example of command to create a
> > > > root and an intermediate certificate in runtime.sgml or such?
> > > 
> > > Yes, I have thought about that.  My presentation has clear examples that
> > > we can use, again based on Stephen and David's scripts using v3_ca.  I
> > > will work up a possible patch for that too.
> > 
> > That too.
> 
> I did that as a separate patch, which is the second attachment.

This is openssl.diff.

+Then, sign the request with the the private key to create a root
+certificate authority:
s/the the/the/

+
+openssl req -new -nodes -text -out root.csr \
+  -keyout root.key -subj "/CN=root.yourdomain.com"
+chmod og-rwx root.key
+openssl x509 -req -in root.csr -text -days 365 \
+  -extfile /etc/ssl/openssl.cnf -extensions v3_ca \
+  -signkey root.key -out root.crt
The succession of commands of commands for the intermediate certificates
is wild. Could it be possible to explain what each command means? Users
would not get lost this way.

> I don't think I will work on the testing changes.

Fine for me. This could do for a fine TODO item. Not one of those hard,
complicated and basically impossible things on the TODO list.
--
Michael


signature.asc
Description: PGP signature

Re: Correction of intermediate certificate handling

[Michael Paquier]

On Wed, Jan 17, 2018 at 08:39:55AM -0500, Bruce Momjian wrote:
> On Wed, Jan 17, 2018 at 07:34:42AM -0500, Bruce Momjian wrote:
> > > The succession of commands of commands for the intermediate certificates
> > > is wild. Could it be possible to explain what each command means? Users
> > > would not get lost this way.
> > 
> > Yes, I was not happy about that either.  I was afraid that pound-sign
> > comments would look like root prompts but I just added them and they
> > look fine.  Updated patch attached, with some expiration and wording
> > adjustments.  There is also a new paragraph at the end explaining where
> > to place the files.
> 
> Oh, and how far back should these be backpatched?  10?  9.6?  9.3? I am
> thinking it should be done as far back as possible as long as it is
> simple.

No objections from here to back-patch depending on the difficulty. I
think that a0572203 has created some conflicts in this area for
REL_10_STABLE, nothing huge though.
--
Michael


signature.asc
Description: PGP signature

Re: Correction of intermediate certificate handling

[Michael Paquier]

On Wed, Jan 17, 2018 at 07:34:42AM -0500, Bruce Momjian wrote:
> On Wed, Jan 17, 2018 at 05:20:00PM +0900, Michael Paquier wrote:
> > The succession of commands of commands for the intermediate certificates
> > is wild. Could it be possible to explain what each command means? Users
> > would not get lost this way.
> 
> Yes, I was not happy about that either.  I was afraid that pound-sign
> comments would look like root prompts but I just added them and they
> look fine.  Updated patch attached, with some expiration and wording
> adjustments.  There is also a new paragraph at the end explaining where
> to place the files.

Thanks, that's a net improvement. So +1 for this version.

+enterprise-wide root CAs) should be used in production.
Nit here. CA should not be plural.

+
+Then, sign the request with the the key to create a root certificate
+authority:
You still have a "the the" here.

/etc/ssl/openssl.cnf is not available on macos or Windows, which can
lead to a bit of confusion as I would imagine that people would
copy/paste such commands when testing things. Perhaps it would be worth
mentioning that this path is proper to usual Linux distributions (I can
see it at least on ArchLinux and Debian), with a reference to this
OpenSSL link: 
https://www.openssl.org/docs/manmaster/man5/config.html

There is as well a set of tiny configuration files in src/test/ssl.
--
Michael


signature.asc
Description: PGP signature

Re: Correction of intermediate certificate handling

[Michael Paquier]

On Wed, Jan 17, 2018 at 09:00:17PM -0500, Bruce Momjian wrote:
> On Thu, Jan 18, 2018 at 10:25:03AM +0900, Michael Paquier wrote:
> > /etc/ssl/openssl.cnf is not available on macos or Windows, which can
> > lead to a bit of confusion as I would imagine that people would
> > copy/paste such commands when testing things. Perhaps it would be worth
> > mentioning that this path is proper to usual Linux distributions (I can
> > see it at least on ArchLinux and Debian), with a reference to this
> > OpenSSL link: 
> > https://www.openssl.org/docs/manmaster/man5/config.html
> 
> One odd thing about the configuration file is that you don't need to
> modify it, but you do need to specify it for that command.
> 
> Fixed patch attached.

OK, what you are proposing here looks fine to me. That's an
improvement.
--
Michael


signature.asc
Description: PGP signature

Re: What does "Table rewrite" mean?

[Michael Paquier]

On Fri, Jan 19, 2018 at 11:33:43AM -0500, Tom Lane wrote:
> It means reading the whole table and writing it out in some modified
> form (for instance, with some column transformed into a new datatype).
> It's not "dangerous" in any way ... but if you've got many GB of data in
> the table and you can't afford to have the table locked for a long time,
> then it's something to avoid.

Yeah that can be costly. Note that WAL corresponding to this data needs
to be generated as well.
--
Michael


signature.asc
Description: PGP signature

Re: Correction of intermediate certificate handling

[Michael Paquier]

On Fri, Jan 26, 2018 at 08:09:30AM -0500, Bruce Momjian wrote:
> On Thu, Jan 25, 2018 at 10:59:23PM -0500, Peter Eisentraut wrote:
> > If you change the Makefile rule for generating the client CA to omit the
> > -extensions v3_ca option, then the first test will fail.
> 
> Oh, very good!

Good point, I didn't notice that. Thanks Peter.
--
Michael


signature.asc
Description: PGP signature

Re: i think there's a typo

[Michael Paquier]

On Thu, Feb 15, 2018 at 11:06:27PM +, PG Doc comments form wrote:
> SELECT usename FROM pg_user;
> 
> should be
> SELECT pg_user.usename FROM pg_user;

This is perfectly valid SQL.  The column selected is assumed to be from
pg_user.
--
Michael


signature.asc
Description: PGP signature

Re: docu bug?

[Michael Paquier]

On Mon, Feb 19, 2018 at 01:54:24PM +, PG Doc comments form wrote:
> The sample works with a NON-persistent connection (psql -c):
> 
> 
> 25.3.6.1. Standalone Hot Backups
> ...  touch /var/lib/pgsql/backup_in_progress
> psql -c "select pg_start_backup('hot_backup');"
> tar -cf /var/lib/pgsql/backup.tar /var/lib/pgsql/data/
> psql -c "select pg_stop_backup();"
> rm /var/lib/pgsql/backup_in_progress
> tar -rf /var/lib/pgsql/backup.tar /var/lib/pgsql/archive/
> ...
> 

This uses the set of exclusive backup APIs, so with non-persistent
connections the backup can be correctly completed.  Non-exclusive
backups can be defined using 'false' as third argument of 
pg_start_backup() and first argument of pg_stop_backup().  Note that for
pg_start_backup you would also need to define the second boolean
argument to decide if a checkpoint should be immediately taken or not.
So you would have basically the following flow on a persistem
connection:
=# SELECT pg_start_backup('my_backup', true_or_false, false);
-- keep connection alive and do stuff.
=# SELECT pg_stop_backup(false);

Do you think that the section "Tips and Examples" should mention
that an exclusive backup method is used for this example?  This may
reduce the confusion.
--
Michael


signature.asc
Description: PGP signature

Re: docu bug?

[Michael Paquier]

On Tue, Feb 20, 2018 at 08:00:35AM +, Zwettler Markus (OIZ) wrote:
> Yes. I think the exclusive backup method should be mentioned
> especially as it is the older one. 

Attached is a suggestion of patch.  What do you think?
--
Michael
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 9d8e69056f..54ead81e51 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -1466,7 +1466,7 @@ archive_command = 'test ! -f /var/lib/pgsql/backup_in_progress || (test ! -f /va
 
  
   With this preparation, a backup can be taken using a script like the
-  following:
+  following, which uses the exclusive backup method:
 
 touch /var/lib/pgsql/backup_in_progress
 psql -c "select pg_start_backup('hot_backup');"


signature.asc
Description: PGP signature

Re: pg_hba_file_rules permission issue

[Michael Paquier]

On Fri, Feb 23, 2018 at 09:41:27AM +, PG Doc comments form wrote:
> According to the documentation, I assume normal users will be able to view
> pg_hba_file_rules once they are granted select privileges. But for the
> privileged user it's giving following error while trying to view records:
> 
> ERROR:  permission denied for function pg_hba_file_rules
> SQL state: 42501

I think that you need as well execution rights on pg_hba_file_rules() to
give access to it.
--
Michael


signature.asc
Description: PGP signature

Re: libpq options

[Michael Paquier]

On Thu, Mar 01, 2018 at 01:35:54AM -0800, Andres Freund wrote:
> On 2017-11-25 19:05:54 +0900, Michael Paquier wrote:
>> A Boolean value of true tells the backend
>> +   to go into walsender mode, wherein a small set of replication 
>> commands
>> +   can be issued instead of SQL statements.
> 
> This actually is wrong now I think. Petr?

On more or less HEAD:
$ psql -X -d "replication=1 dbname=postgres"
postgres=# create table aa (a int);
ERROR:  cannot execute SQL commands in WAL sender for physical replication
$ psql -X -d "replication=database dbname=postgres"
postgres=# create table aa (a int);
CREATE TABLE

So one needs to use replication=database in order to be able to issue
normal SQL statements, while replication=true enforces physical
replication where this cannot happen (no connection to a specified
database).
--
Michael


signature.asc
Description: PGP signature

Re: MacPorts xsltproc is very slow?

[Michael Paquier]

On Sat, Mar 03, 2018 at 09:34:58AM +1300, Thomas Munro wrote:
> Recently I've been unable to build the documentation intermittently,
> and I think it's because sourceforge.net has become flakey.  Let me
> try right now...
> 
> $ make docs XSLTPROC=/usr/bin/xsltproc
> ...blah blah blah...
> warning: failed to load external entity
> "http://docbook.sourceforge.net/release/xsl/current/xhtml/chunk.xsl";
> ...blah blah blah...

I am seeing that as well lately on my Linux box as well with Debian.
And the build failure consists in a mountain of warnings and errors
where you need to dig up to the top to see the real problem.  That's
annoying :(
--
Michael


signature.asc
Description: PGP signature

Re: libpq options

[Michael Paquier]

On Fri, Mar 02, 2018 at 12:58:50PM +0100, Magnus Hagander wrote:
> To nitpick:
> 
> +   protocol. A Boolean value of true tells the
> backend
> 
> We don't really have boolean values here, do we? It's just the string true
> that's treated as a boolean by the backend. It just sounds really weird to
> me when written that way. Particularly since the next sentence talks about
> passing "database" as the same thing.
> 
> I know that's pretty much nitpicking, but I want to make sure I haven't
> actually missed/forgotten how some part of that one is handled..

Yes, that's indeed a bit inconsistent with the other parameters able to
use boolean-like parameters, like sslcompression or sslmode (deprecated
in favor requiressl), still those two ones are frontend-only
parameters, so they are just able to use "1" or "0".  Replication is
part of the startup packet which uses parse_bool() so valid values can
be true, false, yes, no, on, off, 1 or 0.  And it is even possible to
use upper-case characters.  So why not documenting all that precisely
then?

> It also talks separately about "going into walsender mode" (=physical
> replication) and "instructs the walsender to connect to the database". I
> think that's a bit confusing. I suggest just calling it "physical
> replication mode" and "logical replication mode", and not bother mentioning
> walsender since that's quite internal.

Indeed, this term is pretty spread across the documentation as well.

I have taken into account this feedback, and created the new version
attached, for a v2.

Thoughts?
--
Michael
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 2a8e1f2e07..8934559239 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1262,6 +1262,57 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
  
 
+ 
+  replication
+  
+  
+   This option determines if a backend should use the replication
+   protocol.  For a detailed description about the replication protocol,
+   consult . The following values,
+   which are case-insensitive, are supported:
+   
+
+ 
+  true, on,
+   yes or 1
+ 
+ 
+  
+   The backend goes into physical replication mode, wherein a small
+   set of replication commands can be issued instead of SQL statements.
+   Only the simple query protocol can be used in this case.
+  
+ 
+
+
+
+ database
+ 
+  
+   The backend goes into logical replication mode, as the value
+   instructs the backend to connect to the database specified in
+   the dbname parameter.  Only the simple query
+   protocol can be used in this case.
+  
+ 
+
+
+
+ 
+  false, off,
+   no or 0
+ 
+ 
+  
+   The backend is a regular one, which is the default behavior.
+  
+ 
+
+   
+  
+  
+ 
+
  
   sslmode
   
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 3cec9e0b0c..2e231efac0 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -1635,15 +1635,9 @@ that cannot support tls-unique for some reason.
 
 
 To initiate streaming replication, the frontend sends the
-replication parameter in the startup message. A Boolean value
-of true tells the backend to go into walsender mode, wherein a
-small set of replication commands can be issued instead of SQL statements. Only
-the simple query protocol can be used in walsender mode.
-Replication commands are logged in the server log when
+ connection parameter in the
+startup message. Replication commands are logged in the server log when
  is enabled.
-Passing database as the value instructs walsender to connect to
-the database specified in the dbname parameter, which will allow
-the connection to be used for logical replication from that database.
 
 
  For the purpose of testing replication commands, you can make a replication


signature.asc
Description: PGP signature

Re: libpq options

[Michael Paquier]

On Tue, Mar 06, 2018 at 09:05:38PM -0500, Peter Eisentraut wrote:
> I didn't like so much shrinking down the protocol section.  I have often
> wanted *more* detail there, not less.  So I combined your patch for the
> libpq chapter and added a bit more in the protocol chapter as well.
> 
> Committed.

Thanks for the commit, my idea was to limit duplication between both
sections.  If things get changed at some point, this avoids parts of the
documentation to rot.

 
-The commands accepted in walsender mode are:
+Replication commands are logged in the server log when
+ is enabled.
+
+
+
+The commands accepted in replication mode are:
Good addition here.
--
Michael


signature.asc
Description: PGP signature

Re: document json[b] limitation

[Michael Paquier]

On Wed, Apr 25, 2018 at 06:50:51PM +0300, Oleg Bartunov wrote:
> Oops, it should be 256 Mb :)

The numbers you are presenting are right, aka 1GB for json:
=# create table aa (a json);
CREATE TABLE
=# insert into aa select ('{"key":"' || repeat('a', 512 * 1024 * 1024) ||
 repeat('a', 500 * 1024 * 1024) || '"}')::json;
INSERT 0 1
=# insert into aa select ('{"key":"' || repeat('a', 512 * 1024 *
 1024) || repeat('a', 512 * 1024 * 1024) || '"}')::json;
ERROR:  XX000: invalid memory alloc request size 1073741836
LOCATION:  palloc, mcxt.c:934

And 256MB for jsonb:
=# create table ab (a jsonb);
CREATE TABLE
=# insert into aa select ('{"key":"' || repeat('a', 256 * 1024 *
1024) || '"}')::jsonb;
ERROR:  54000: string too long to represent as jsonb string
DETAIL:  Due to an implementation restriction, jsonb strings cannot
exceed 268435455 bytes.

Be sure to use an upper-case "B" to mean bytes and not bits in the
documentation.
--
Michael


signature.asc
Description: PGP signature

Re: authentication methods sections

[Michael Paquier]

On Sat, Apr 21, 2018 at 10:21:03AM -0400, Peter Eisentraut wrote:
> On 4/14/18 17:20, Magnus Hagander wrote:
> > On Thu, Apr 12, 2018 at 2:37 AM, Peter Eisentraut
> >  > > wrote:
> > 
> > I find that the section authentication methods
> >  > > is
> > becoming harder to read and navigate, because we keep adding
> > authentication methods and more information about them, and it's all on
> > one HTML page.  I propose to move those sections up one level in the
> > hierarchy so they end up on separate HTML pages.
> > 
> > 
> > +1, that definitely seems like a good idea. 
> 
> pushed

Sorry for coming late here, but the patch which has been committed,
56811e57, results in a section which is completely empty:
https://www.postgresql.org/docs/devel/static/auth-methods.html

And all the authentication methods get listed here:
https://www.postgresql.org/docs/devel/static/client-authentication.html

So I find the result even more confusing than the original.  Wouldn't it
be more simple to just remove "Authentication Methods" if the current
layer is kept?
--
Michael


signature.asc
Description: PGP signature

Re: Table 28.4. wait_event description typo

[Michael Paquier]

On Wed, May 30, 2018 at 10:12:29AM +0300, Pavlo Golub wrote:
> On monitoring statistic page [1] we have typo. Table 28.4. wait_event
> description contains:
> 
> ...
> ClientRead  Waiting to read data from the client.
> ClientWrite Waiting to write data from the client.
> ...
>   ^
> It should state TO CLIENT.

Good catch.  It seems that this is my fault as what has been done in
6f3bd98.  So let's update the docs as you suggest, the attached does so
FWIW.
--
Michael
diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index d272719ff4..8068e28184 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -1076,7 +1076,7 @@ deparseFromExpr(List *quals, deparse_expr_cxt *context)
 	/* Construct FROM clause */
 	appendStringInfoString(buf, " FROM ");
 	deparseFromExprForRel(buf, context->root, scanrel,
-		  (bms_num_members(scanrel->relids) > 1),
+		  (bms_membership(scanrel->relids) == BMS_MULTIPLE),
 		  (Index) 0, NULL, context->params_list);
 
 	/* Construct WHERE clause */
@@ -1262,7 +1262,7 @@ deparseLockingClause(deparse_expr_cxt *context)
 }
 
 /* Add the relation alias if we are here for a join relation */
-if (bms_num_members(rel->relids) > 1 &&
+if (bms_membership(rel->relids) == BMS_MULTIPLE &&
 	rc->strength != LCS_NONE)
 	appendStringInfo(buf, " OF %s%d", REL_ALIAS_PREFIX, relid);
 			}
@@ -2328,7 +2328,7 @@ deparseVar(Var *node, deparse_expr_cxt *context)
 	int			colno;
 
 	/* Qualify columns when multiple relations are involved. */
-	bool		qualify_col = (bms_num_members(relids) > 1);
+	bool		qualify_col = (bms_membership(relids) == BMS_MULTIPLE);
 
 	/*
 	 * If the Var belongs to the foreign relation that is deparsed as a
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index c278076e68..c2adb22dff 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1236,7 +1236,7 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
 
 
  ClientWrite
- Waiting to write data from the client.
+ Waiting to write data to the client.
 
 
  LibPQWalReceiverConnect


signature.asc
Description: PGP signature

Re: Table 28.4. wait_event description typo

[Michael Paquier]

On Wed, May 30, 2018 at 01:30:08PM -0400, Alvaro Herrera wrote:
> I do not think that patch does what you think it does.

Oops.  Sorry another patch got on the way.  Here you go.
--
Michael
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index c278076e68..c2adb22dff 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -1236,7 +1236,7 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
 
 
  ClientWrite
- Waiting to write data from the client.
+ Waiting to write data to the client.
 
 
  LibPQWalReceiverConnect


signature.asc
Description: PGP signature

Re: Table 28.4. wait_event description typo

[Michael Paquier]

On Wed, May 30, 2018 at 02:14:42PM -0400, Alvaro Herrera wrote:
> Okay, pushed.  Because of the sgml to xml conversion, it didn't apply to
> pg10 cleanly.

Thanks Álvaro.
--
Michael


signature.asc
Description: PGP signature

Re: Typo in sql-revoke.html

[Michael Paquier]

On Sun, Jun 10, 2018 at 01:31:34AM +0200, Erwin Brandstetter wrote:
> Replace
> "and user B has in turned granted it to user C"
> with
> "and user B has in turn granted it to user C"
> 
> Affects versions 7.4 - 11.

Thanks for the report, Erwin!  A fix has been pushed down to 9.3.
--
Michael


signature.asc
Description: PGP signature

Re: slight grammar error

[Michael Paquier]

On Fri, Jun 08, 2018 at 10:46:33PM +, PG Doc comments form wrote:
> The sentence:
> 
> "This setting will often help to reduce transaction latency, but it also can
> an adverse effect on performance"
> 
> should be:
> 
> "This setting will often help to reduce transaction latency, but it also can
> have an adverse effect on performance"

Indeed.  Thanks Christopher for the report!  Fixed and back-patched down
to 9.6 where checkpoint_flush_after has been introduced.
--
Michael


signature.asc
Description: PGP signature

Re: Changes in serial / sequence introduced in Postgresql 10

[Michael Paquier]

On Tue, Jun 19, 2018 at 02:49:08PM -0400, Bruce Momjian wrote:
> I don't think we realize there was a behavioral change here.  I think we
> were just trying to fix the case where the sequence maximum didn't match
> the serial maximum.  I am not sure if it is worth documenting it at this
> point though.

Yeah, I agree that it is not worth documenting it.  I don't recall
reviewing the full patch related to identity columns, but I surely
looked at patches which fixed post-commit bugs, and the new behavior is
as a whole more consistent as sequences created with serial map to the
real bound values associated with the underlying column type, and
bigserial does the same:
=# create table test (id bigserial primary key) ;
CREATE TABLE
=# select sequencename, max_value from pg_sequences;
 sequencename |  max_value
--+-
 test_id_seq  | 9223372036854775807
(1 row)
--
Michael


signature.asc
Description: PGP signature

Re: search.cpan.org is EOL, update links

[Michael Paquier]

On Thu, Jun 28, 2018 at 01:00:47PM +0200, Daniel Gustafsson wrote:
> search.cpan.org has now been EOL’d and turned off, with all URLs redirecting 
> to
> metacpan.org [1].  While the redirects are likely to the maintained for a long
> time, we might as well update to avoid the risk of stale redirects.  Attached
> patch changes the few links we have to instead use metacpan.

Okay, let's fix that.

While working on the links for CPAN, perhaps these should be switched to
https as well?
src/sgml/acronyms.sgml:
http://www.cpan.org/";>Comprehensive Perl Archive Network
contrib/fuzzystrmatch/dmetaphone.c:
version 0.05 available from http://www.cpan.org.
--
Michael


signature.asc
Description: PGP signature

Re: search.cpan.org is EOL, update links

[Michael Paquier]

On Fri, Jun 29, 2018 at 08:29:16AM +0200, Daniel Gustafsson wrote:
> Good point, we might as well do that too.  Fixed those two and an additional
> one in the attached updated version.

Did you notice the paragraph at the top of ppport.h?  This is a file
automatically generated so it does not sound like a good idea to apply
the change there.  Refreshing this file would make the most sense if
needed, but I have not looked at that in details.

Pushed the rest with a proper back-patch for the docs.
--
Michael


signature.asc
Description: PGP signature

Re: Dead link to hp docs

[Michael Paquier]

On Sun, Jul 01, 2018 at 11:29:47AM +, PG Doc comments form wrote:
> The following documentation comment has been logged on the website:
> 
> Page: https://www.postgresql.org/docs/9.6/static/ssl-tcp.html
> Description:
> 
> In the "18.9.1. Using Client Certificates" the link to ssl certs usage
> diagram seems to be dead
> http://h71000.www7.hp.com/doc/83final/ba554_90007/ch04s02.html
> i digged a bit at the wayback machine and have found where they moved it -
> http://h41379.www4.hpe.com/doc/83final/ba554_90007/ch04s02.html

Here are all the broken links in the source tree:

doc/src/sgml/libpq.sgml:
Wrong:   http://h71000.www7.hp.com/doc/83final/ba554_90007/ch04.html
Correct: http://h41379.www4.hpe.com/doc/83final/ba554_90007/ch04.html

doc/src/sgml/runtime.sgml:
Wrong: http://h71000.www7.hp.com/doc/83final/ba554_90007/ch04s02.html
Correct: http://h41379.www4.hpe.com/doc/83final/ba554_90007/ch04s02.html

src/include/port/atomics/generic-acc.h:
Wrong: 
http://h21007.www2.hp.com/portal/download/files/unprot/Itanium/inline_assem_ERS.pdf
Perhaps right:
http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.129.5445&rep=rep1&type=pdf

src/include/port/atomics/generic-acc.h:
src/include/storage/s_lock.h:
Wrong: 
http://h21007.www2.hp.com/portal/download/files/unprot/itanium/spinlocks.pdf
Perhaps right:
http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.129.5445&rep=rep1&type=pdf

It would be nice to fix the links to the docs for spinlocks and inline
assembler at the same time, but I am not sure where those are..
--
Michael


signature.asc
Description: PGP signature

Re: Build PostgreSQL manually with uuid

[Michael Paquier]

On Sun, Jul 01, 2018 at 10:04:01PM +, PG Doc comments form wrote:
> Following the documentation at
> https://www.postgresql.org/docs/10/static/install-procedure.html and
> configuring the option --with-uuid=ossp won't build the uuid-ossp extension
> at the following make / make install steps. For this, I need to change to
> ./contrib/uuid-ossp and execute an additional make / make install from
> there. This is missing in the documentation.

What do you have as value for with_uuid in src/Makefile.global.in?
uuid-ossp would be compiled from the root path if its value is something
else than "no" (see contrib/Makefile).  Based on what you do, it should
be set to "ossp", and this has not changed for ages.
--
Michael


signature.asc
Description: PGP signature

Re: Release note trimming: another modest proposal

[Michael Paquier]

On Mon, Aug 06, 2018 at 08:14:23AM +0100, Dean Rasheed wrote:
> On 5 August 2018 at 23:57, Tom Lane  wrote:
>> Anyway, I'd like to propose a compromise position that I don't think
>> has been discussed before: let's drop release notes for branches
>> that were already EOL when a given branch was released.
> 
> WFM. +1

+1.
--
Michael


signature.asc
Description: PGP signature

Re: password storage docs

[Michael Paquier]

On Mon, Aug 20, 2018 at 01:35:56PM +1200, Richard Hector wrote:
> I can't find information about the storage format for that at all -
> other than "... and supports storing passwords on the server in a
> cryptographically hashed form that is thought to be secure."
> 
> It would be nice to see more information on this.

The SCRAM verifiers stored conform to RFC 5803:
https://tools.ietf.org/html/rfc5803.
This is mentioned in the comments of auth-scram.c.  Do you think that
mentioning that in this paragraph of this doc would be useful?  We could
for example append "as defined in RFC 5803" in the last sentence.
--
Michael


signature.asc
Description: PGP signature

"System roles" mentioned in psql documentation

[Michael Paquier]

Hi all,

I have noticed that psql documentation mentions "system roles", however
in all other parts of the docs, we use the term "default roles".
Shouldn't we make this term more consistent and also add a link to the
table describing those roles?  Please see the attached.

Thanks,
--
Michael
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index eb9d93a168..df762b811c 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1521,7 +1521,8 @@ testdb=>
 unified into roles, this command is now equivalent to
 \du.)
 By default, only user-created roles are shown; supply the
-S modifier to include system roles.
+S modifier to include default roles described in
+.
 If pattern is specified,
 only those roles whose names match the pattern are listed.
 If the form \dg+ is used, additional information
@@ -1711,7 +1712,8 @@ testdb=>
 unified into roles, this command is now equivalent to
 \dg.)
 By default, only user-created roles are shown; supply the
-S modifier to include system roles.
+S modifier to include default roles described
+in .
 If pattern is specified,
 only those roles whose names match the pattern are listed.
 If the form \du+ is used, additional information


signature.asc
Description: PGP signature

Re: "System roles" mentioned in psql documentation

[Michael Paquier]

On Thu, Aug 23, 2018 at 10:53:35AM +0200, Jürgen Purtz wrote:
> This inconsistency is part of the more general problem that we miss a
> chapter, where our basic terms like 'database', 'cluster', 'segment',
> 'catalog', 'schema', ... are explicitly defined.

You may have a point here, not in the way of reworking entirely the
documentation, but in the fact that we may want to use "system objects"
instead of "system roles".  I am not personally sure that it is a better
improvement than using "default roles", but that's a point to raise.
--
Michael


signature.asc
Description: PGP signature

Re: you need this kind of explanation

[Michael Paquier]

On Mon, Sep 03, 2018 at 08:05:33AM +, PG Doc comments form wrote:
> The following documentation comment has been logged on the website:
> 
> Page: https://www.postgresql.org/docs/10/static/index.html
> Description:
> 
> right off the bat, you should have this kind of explanation up there in
> chapter 1:
> https://www.liquidweb.com/kb/what-is-the-default-password-for-postgresql/
> 
> this will orient any noobs immediately.
> 
> also, the word "role" and "user" should not be used interchangebly. they
> mean different things.

If you have any improvements that you think are adapted about the
documentation of Postgres, please feel free to provide any patch of base
patch that you could use for discussion.  I am not sure if any kind of
information provided in the blog post you are mentioning above makes
much sense though, as the way Postgres is packaged, installed, deployed
and configured depends heavily on the way a given distribution or OS
wants to set it up, which could have many different requirements.
--
Michael


signature.asc
Description: PGP signature

Re: Ambiguity in restore_command for recovery.conf

[Michael Paquier]

On Tue, Sep 18, 2018 at 04:10:17PM +, PG Doc comments form wrote:
> Documentation for restore_command in recovery.conf only says, "This
> parameter is required for archive recovery, but optional for streaming
> replication," but it doesn't say specifically when the restore_command
> string will be used and why. My guess is that as long as the WAL files
> are still available on the master (primary) host, the recovery will
> fetch them via the replication slot, but if a WAL is missing on the
> primary, only then will the restore_command be used. But this isn't
> explicitly stated. It could be that the restore_command is always
> used, even if the WAL file is still available on the primary.

The logic around the source used to fetch WAL segments is defined in
WaitForWALToBecomeAvailable/xlog.c.  A standby would give priority to
what is present in the local pg_wal or in the WAL archive before
switching to streaming.  If fetching a segment from the archive or the
local pg_wal fails, then a switch to streaming happens (of course if
primary_conninfo is defined).  There is nothing really linked to
replication slots in this case.
--
Michael


signature.asc
Description: PGP signature

Re: Mention FK creation take ShareRowExclusiveLock on referenced table

[Michael Paquier]

On Tue, Sep 18, 2018 at 12:32:54PM +0200, Adrien NAYRAT wrote:
> A few days ago I was surprised a CREATE TABLE containing FK constraint was
> stuck due to an automatic vacuum freeze (which took ShareUpdateExclusiveLock
> if I remember) on referenced table.

Right.  See the top of vacuum_rel() where lmode is set.

> After digging into the code I found theses lines in tablecmds.c :
> 
> /*
>  * Grab ShareRowExclusiveLock on the pk table, so that someone doesn't
>  * delete rows out from under us.
>  */
> 
> Maybe it should be documented in theses pages?
> 
> https://www.postgresql.org/docs/current/static/sql-createtable.html
> https://www.postgresql.org/docs/current/static/sql-altertable.html
> 
> If you agree I can send a patch.

That looks like a good idea.  Are you thinking about adding a comment
about that in "ADD table_constraint" for the ALTER TABLE page, and in
"FOREIGN KEY" for the CREATE TABLE page?
--
Michael


signature.asc
Description: PGP signature

Re: Mention FK creation take ShareRowExclusiveLock on referenced table

[Michael Paquier]

On Thu, Sep 20, 2018 at 08:23:45AM +0200, Adrien Nayrat wrote:
> Yes, here is the patch.

Thanks Adrien.  I have reworded a bit the thing, fixed a typo, and
pushed down to v11 where this applied without conflicts.
--
Michael


signature.asc
Description: PGP signature

Re: Mention FK creation take ShareRowExclusiveLock on referenced table

[Michael Paquier]

On Fri, Sep 21, 2018 at 09:09:36AM +0200, Adrien NAYRAT wrote:
> Thanks! As it could happen even on previous version, should we
> backpatch for the documentation?

I have patched HEAD, and then down until conflicts happened, which is
v10, thinking about it as a documentation improvement.  The behavior
exists for ages, so I have not bothered much...
--
Michael


signature.asc
Description: PGP signature

Re: pgrowlocks columns do not match docs: "modes" instead of "lock_type"

[Michael Paquier]

On Mon, Oct 01, 2018 at 09:42:08AM +, PG Doc comments form wrote:
> The description of the pgrowlocks extension says that the function returns a
> column called "lock_type". However this column is really called "modes", as
> shown in contrib/pgrowlocks/pgrowlocks--1.2.sql:
> 
> CREATE FUNCTION pgrowlocks(IN relname text,
> OUT locked_row TID, -- row TID
> OUT locker XID, -- locking XID
> OUT multi bool, -- multi XID?
> OUT xids xid[], -- multi XIDs
> OUT modes text[],   -- multi XID statuses
> OUT pids INTEGER[]) -- locker's process id
> RETURNS SETOF record
> AS 'MODULE_PATHNAME', 'pgrowlocks'
> LANGUAGE C STRICT PARALLEL SAFE;

0ac5ad5 has updated pgrowlocks from 1.0 to 1.1 and it forgot the
documentation.  The order of the columns is correct, but the example was
not.  So I updated the documentation with a fresh one, and fixed it all
the way down to 9.3.  Thanks for the report, Chris!
--
Michael


signature.asc
Description: PGP signature

Re: please inform data_directory

[Michael Paquier]

On Wed, Oct 10, 2018 at 03:59:59PM -0400, Bruce Momjian wrote:
> What do you mean by that?  I can call pg_read_file() before calling
> 'SHOW  data_directory'.  Are you saying that pg_read_file can only read
> in the data_directory?  It is designed that way.

Please note that v11 has added a new default role called
pg_read_server_files, which allows a role to read files out of the data
directory which is the default.
--
Michael


signature.asc
Description: PGP signature

Re: Using old master as new replica after clean switchover

[Michael Paquier]

On Thu, Oct 25, 2018 at 11:15:51AM +0200, Jehan-Guillaume de Rorthais wrote:
> On Thu, 25 Oct 2018 02:57:18 -0400
> Nikolay Samokhvalov  wrote:
>> My research shows that some people already rely on the following when
>> planned failover (aka switchover) procedure, doing it in production:
>> 
>>  1) shutdown the current master
>>  2) ensure that the "master candidate" replica has received all WAL data
>> including shutdown checkpoint from the old master
>>  3) promote the master candidate to make it new master
>>  4) configure recovery.conf on the old master node, while it's inactive
>>  5) start the old master node as a new replica following the new master.
> 
> Indeed.

The important point here is that the primary will wait for the shutdown
checkpoint record to be replayed on the standbys before finishing to
shut down.

> The only additional nice step would be to be able to run some more safety 
> tests
> AFTER the switchover process on te old master. The only way I can think of
> would be to run pg_rewind even if it doesn't do much.

Do you have something specific in mind here?  I am curious if you're
thinking about things like page-level checks for LSN matches under some
threshold or such, because you should not have pages on the previous
primary which have LSNs newer than the point up to which the standby has
replayed.

>> if so, let's add it to the documentation, making it official. The patch is
>> attached.
> 
> I suppose we should add the technical steps in a sample procedure?

If an addition to the docs is done, symbolizing the steps in a list
would be cleaner, with perhaps something in a dedicated section or a new
sub-section.  The failover flow you are mentioning is good practice
because that's safe, and there is always room for improvements in the
docs.
--
Michael


signature.asc
Description: PGP signature

Re: Change pg_attribute textual link to an actual link

[Michael Paquier]

On Mon, Oct 22, 2018 at 12:09:41PM +0200, Daniel Gustafsson wrote:
> I think your latter suggestion pretty much covers all we need, so updated the
> patch with that too.

Both the patch and the suggestions made look good to me, so committed.
--
Michael


signature.asc
Description: PGP signature

Re: A typo in release notes

[Michael Paquier]

On Mon, Nov 12, 2018 at 02:15:31PM +0300, Alexander Lakhin wrote:
> I believe, that the name "Masahiko Sawada" was misspelled in the
> latest release notes.
> See:
> https://www.postgresql.org/message-id/CAD21AoB5K1qJm%2Bwj%2BTTCP-sFPiSdnd0LGd_pNAfi3VXrGKfdjw%40mail.gmail.com
> The patch is attached.

Thanks Alexander for the report!  Committed and back-patched.
--
Michael


signature.asc
Description: PGP signature

Re: CREATE/ALTER ROLE with NULL password

[Michael Paquier]

On Wed, Nov 21, 2018 at 07:36:59PM +, PG Doc comments form wrote:
> The current synopsis for CREATE / ALTER ROLE give one of the allowed options
> as:
> [ ENCRYPTED ] PASSWORD 'password'
> and the current documentation for CREATE ROLE says:
> "The ENCRYPTED keyword has no effect, but is accepted for backwards
> compatibility."

The grammar is still supported, so keeping it documented has no actual
problems until it gets removed, if that happens.  Keeping it is not a
real maintenance burden either.

> I think it might be worth explicitly specifying the password-blanking form
> for both commands as a new option in their synopses, e.g.:
> 
> "
> CREATE ROLE name [ [ WITH ] option [ ... ] ]
> 
> where option can be:
> 
>   SUPERUSER | NOSUPERUSER
> | CREATEDB | NOCREATEDB
> ...
> | [ ENCRYPTED ] PASSWORD 'password' | PASSWORD NULL
> ...
> "

Yes, that the set of grammar combination supported, as ENCRYPTED
PASSWORD NULL is not possible.

> Also, there is inconsistency of quoting of 'password' in the synopsis for
> CREATE/ALTER ROLE (has quotes) vs. their respective parameters sections (no
> quotes).

Agreed, this should have quotes for consistency.  Any objections with
the attached set of fixes from anybody?
--
Michael
diff --git a/doc/src/sgml/ref/alter_role.sgml b/doc/src/sgml/ref/alter_role.sgml
index 573a3e80f7..dbf258ef50 100644
--- a/doc/src/sgml/ref/alter_role.sgml
+++ b/doc/src/sgml/ref/alter_role.sgml
@@ -33,7 +33,7 @@ ALTER ROLE role_specification [ WIT
 | REPLICATION | NOREPLICATION
 | BYPASSRLS | NOBYPASSRLS
 | CONNECTION LIMIT connlimit
-| [ ENCRYPTED ] PASSWORD 'password'
+| [ ENCRYPTED ] PASSWORD 'password' | PASSWORD NULL
 | VALID UNTIL 'timestamp'
 
 ALTER ROLE name RENAME TO new_name
@@ -168,7 +168,8 @@ ALTER ROLE { role_specification | A
   BYPASSRLS
   NOBYPASSRLS
   CONNECTION LIMIT connlimit
-  [ ENCRYPTED ] PASSWORD password
+  [ ENCRYPTED ] PASSWORD 'password'
+  PASSWORD NULL
   VALID UNTIL 'timestamp'
   

diff --git a/doc/src/sgml/ref/alter_user.sgml b/doc/src/sgml/ref/alter_user.sgml
index 8f50f43089..6769c8ecc4 100644
--- a/doc/src/sgml/ref/alter_user.sgml
+++ b/doc/src/sgml/ref/alter_user.sgml
@@ -33,7 +33,7 @@ ALTER USER role_specification [ WIT
 | REPLICATION | NOREPLICATION
 | BYPASSRLS | NOBYPASSRLS
 | CONNECTION LIMIT connlimit
-| [ ENCRYPTED ] PASSWORD 'password'
+| [ ENCRYPTED ] PASSWORD 'password' | PASSWORD NULL
 | VALID UNTIL 'timestamp'
 
 ALTER USER name RENAME TO new_name
diff --git a/doc/src/sgml/ref/create_role.sgml b/doc/src/sgml/ref/create_role.sgml
index 9c3b6978af..db842732a8 100644
--- a/doc/src/sgml/ref/create_role.sgml
+++ b/doc/src/sgml/ref/create_role.sgml
@@ -33,7 +33,7 @@ CREATE ROLE name [ [ WITH ] connlimit
-| [ ENCRYPTED ] PASSWORD 'password'
+| [ ENCRYPTED ] PASSWORD 'password' | PASSWORD NULL
 | VALID UNTIL 'timestamp'
 | IN ROLE role_name [, ...]
 | IN GROUP role_name [, ...]
@@ -210,7 +210,8 @@ CREATE ROLE name [ [ WITH ] 
 
  
-  [ ENCRYPTED ] PASSWORD password
+  [ ENCRYPTED ] PASSWORD 'password'
+  PASSWORD NULL
   

 Sets the role's password.  (A password is only of use for
diff --git a/doc/src/sgml/ref/create_user.sgml b/doc/src/sgml/ref/create_user.sgml
index a51dc50c97..198e06e723 100644
--- a/doc/src/sgml/ref/create_user.sgml
+++ b/doc/src/sgml/ref/create_user.sgml
@@ -33,7 +33,7 @@ CREATE USER name [ [ WITH ] connlimit
-| [ ENCRYPTED ] PASSWORD 'password'
+| [ ENCRYPTED ] PASSWORD 'password' | PASSWORD NULL
 | VALID UNTIL 'timestamp'
 | IN ROLE role_name [, ...]
 | IN GROUP role_name [, ...]


signature.asc
Description: PGP signature

Re: CREATE/ALTER ROLE with NULL password

[Michael Paquier]

On Wed, Nov 21, 2018 at 11:58:25PM -0700, David G. Johnston wrote:
> Should tweak the paragraph to point out this exception as well.
> 
>  The ENCRYPTED keyword has no effect, but is accepted for backwards
> compatibility[, except in the PASSWORD NULL form.]

The docs list the following with the patch as supported grammar:
[ ENCRYPTED ] PASSWORD 'password' | PASSWORD NULL
And it seems to me that '|' has priority over '[]', so ENCRYPTED does
not apply to PASSWORD NULL if phrased this way.
--
Michael


signature.asc
Description: PGP signature

Re: CREATE/ALTER ROLE with NULL password

[Michael Paquier]

On Thu, Nov 22, 2018 at 09:54:07AM -0700, David G. Johnston wrote:
> On Thursday, November 22, 2018, Michael Paquier  wrote:
>> The docs list the following with the patch as supported grammar:
>> [ ENCRYPTED ] PASSWORD 'password' | PASSWORD NULL
>> And it seems to me that '|' has priority over '[]', so ENCRYPTED does
>> not apply to PASSWORD NULL if phrased this way.
>
> Yes, the syntax block is perfectly clear but we still explain said grammer
> in words and should be precise there as well, IMO.  Not a big deal though.

Okay, thanks.  I have committed the simplest version.
--
Michael


signature.asc
Description: PGP signature

Re: typo

[Michael Paquier]

On Mon, Nov 26, 2018 at 12:47:51AM +, PG Doc comments form wrote:
> Correction: A table that has columns with potentially large entries will
> have an associated TOAST table, which is used for out-of-line storage of
> field values that are too large to keep in the table rows "PROPERLY".

Thanks, committed.  I have fixed the docs to reflect your suggestion.
--
Michael


signature.asc
Description: PGP signature

Re: typo

[Michael Paquier]

On Mon, Nov 26, 2018 at 08:17:06AM +0100, Vik Fearing wrote:
> On 26/11/2018 08:03, Magnus Hagander wrote:
>> Are you sure that's right? To me the original wording of that sentence
>> seems to convey the message properly, and the update done does not?
>
> Yeah, I just found this on the committers list and I disagree with the
> change as well.

[... checking around ...]
Hm.  I have read the sentence and the surroundings a couple of times
before doing anything, and using an adverb looked clearer than the
adjective.  Is an adjective more appropriate than an adverb here because
it insists more on the fact that each row is involved?  Just trying to
grab the difference.
--
Michael


signature.asc
Description: PGP signature

Re: typo

[Michael Paquier]

On Mon, Nov 26, 2018 at 09:14:18AM -0500, Tom Lane wrote:
> But I can see that a lot of people might not be familiar with that usage,
> so I've got no objections to rewriting it more clearly --- any
> suggestions?

It has been suggested upthread to use "in the table rows themselves",
which does not sound bad to me.  So that would give, quoting the whole
portion:
 A table that has columns with potentially large entries will have an
  associated TOAST table, which is used for
 out-of-line storage of
 -field values that are too large to keep in the table rows properly.
 +field values that are too large to keep in the table rows themselves.
  pg_class.reltoastrelid
 links from a table to
  its TOAST table, if any.

Now I cannot really stand as somebody able to decide the right thing on
this thread, proofs present on the table ;)
--
Michael


signature.asc
Description: PGP signature

Re: Return codes for archive and restore commands

[Michael Paquier]

On Wed, Nov 28, 2018 at 11:00:31AM +, PG Doc comments form wrote:
> For the archive command:
> <=128 There are not errors in the PostgreSQL log (messages with severity
> equal or higher than ERROR). Firstly 3 messages of type LOG about fault,
> then WARNING about this and pause for 1 minute, then repeated.
> >=129 FATAL error in the PostgeSQL log. The message about stoping an archive
> process, but not the database. Repeated after roughly 16 seconds.

This code is around for some time, and comes from this commit:
commit: 3ad0728c817bf8abd2c76bd11d856967509b307c
author: Tom Lane 
date: Tue, 21 Nov 2006 20:59:53 +
committer: Tom Lane 
date: Tue, 21 Nov 2006 20:59:53 +
On systems that have setsid(2) (which should be just about everything except
Windows), arrange for each postmaster child process to be its own process
group leader, and deliver signals SIGINT, SIGTERM, SIGQUIT to the whole
process group not only the direct child process.  This provides saner behavior
for archive and recovery scripts; in particular, it's possible to shut down a
warm-standby recovery server using "pg_ctl stop -m immediate", since delivery
of SIGQUIT to the startup subprocess will result in killing the waiting
recovery_command.  Also, this makes Query Cancel and statement_timeout apply
to scripts being run from backends via system().  (There is no support in the
core backend for that, but it's widely done using untrusted PLs.)  Per gripe
from Stephen Harris and subsequent discussion.

The relevant part if pgarch_archiveXlog() in pgarch.c, and this part
is most relevant:
* Per the Single Unix Spec, shells report exit status > 128 when a
* called command died on a signal.

> In this case PostgreSQL tries confirm rules for return codes of a unix
> shell. A unix shell return 126 in the case of "command not executable", 127
> in the case "command not found", 128+# of signal in the case if application
> interrupted by uncatched signal.

If you were to rewrite those paragraphs or make them more precise, how
would you actually shape your suggestions?  I personally quite like the
current formulations, but I am rather used to it to be honest.
--
Michael


signature.asc
Description: PGP signature

Re: Return codes for archive and restore commands

[Michael Paquier]

On Wed, Nov 28, 2018 at 09:39:58PM -0500, Stephen Frost wrote:
> Having discussed this quite a bit lately with David Steele and Magnus,
> it's pretty clear that we need to completely rip out how this works
> today and rewrite it based around an extension model where a background
> worker can start up and essentially take the place of the archiver
> process, with flexibility to jump forward through the WAL stream,
> communicate clearly with other processes, handle failure to do so
> gracefully based on the specific cases, etc.

Hm.  When an instance state is in PM_SHUTDOWN_2, the postmaster
explicitely waits for the WAL senders and the archiver to shut down.  So
I think that you would need more control regarding the timing a bgworker
should be shut down first to be completely correct.
--
Michael


signature.asc
Description: PGP signature

Re: Return codes for archive and restore commands

[Michael Paquier]

On Wed, Nov 28, 2018 at 10:27:31PM -0500, Stephen Frost wrote:
> Yes, it couldn't be exactly the same as a generic background worker,
> that's a good point.  We definitely need to make sure that the
> postmaster waits for the archiver to shut down, as it does for the WAL
> senders.

Just to be clear, please note I don't think that what removing the
archiver code from the core code is a bad idea, quite the contrary
actually.  But I doubt that it would be acceptable to rip off this code
without something which has the same properties and guarantees for any
users depending on it.  And archive_command is used a lot.
--
Michael


signature.asc
Description: PGP signature

Re: Typo in description of replay_lag attribute in pg_stat_replication view

[Michael Paquier]

On Mon, Dec 03, 2018 at 06:18:30PM +0300, Maksim Milyutin wrote:
> I have noticed that in description of *flush_lag* in pg_stat_replication
> view 
> (https://www.postgresql.org/docs/current/monitoring-stats.html#PG-STAT-REPLICATION-VIEW)
> there exists unknown value of synchronous_commit parameter - *remote_flush*.
> I think it was meant to use the value *on*.

Yes, you are right.  It should be "on" as "remote_flush" is not a valid
value.  remote_flush is listed in SyncCommitLevel though, so this makes
me wonder if we should also introduce a new value for this purpose
available for users.  The fix you propose looks good to me.  Any
opinions from others?
--
Michael


signature.asc
Description: PGP signature

Re: Typo in description of replay_lag attribute in pg_stat_replication view

[Michael Paquier]

On Tue, Dec 04, 2018 at 01:28:15PM +1300, Thomas Munro wrote:
> On Tue, Dec 4, 2018 at 1:18 PM Michael Paquier  wrote:
>> Yes, you are right.  It should be "on" as "remote_flush" is not a valid
>> value.  remote_flush is listed in SyncCommitLevel though, so this makes
>> me wonder if we should also introduce a new value for this purpose
>> available for users.  The fix you propose looks good to me.  Any
>> opinions from others?
> 
> +1 for the patch.

Thanks for confirming, Thomas.  I'll go apply hopefully tomorrow if
nobody has objections.

> As for introducing remote_flush as the true name of the level, this
> was discussed but somehow went off-course and never made it to the
> finish line:
> 
> https://www.postgresql.org/message-id/flat/CAEepm%3D3FFaanSS4sugG%2BApzq2tCVjEYCO2wOQBod2d7GWb%3DDvA%40mail.gmail.com

Oh, I forgot this one.  We may want to revive that...  remote_flush is
more meaningful than on, especially since there are more and more
possible values for synchronous_commit.
--
Michael


signature.asc
Description: PGP signature

Re: Typo in description of replay_lag attribute in pg_stat_replication view

[Michael Paquier]

On Wed, Dec 05, 2018 at 01:24:22AM +0300, Maksim Milyutin wrote:
> Yeah, I think the notion *remote_flush level* is more appropriate especially
> in the context of sync replication. Within this context maybe it makes sense
> to replace the word *level* to *value* in description of *flush_lag*?

I am not sure that this is an improvement.  Anyway, I have committed
your original patch as that's clearly a mistake and back-patched down to
v10.
--
Michael


signature.asc
Description: PGP signature

Re: pgBadger link needs to be updated

[Michael Paquier]

On Thu, Dec 13, 2018 at 10:46:22PM +, PG Doc comments form wrote:
> pgBadger link needs to be updated
> https://pgbadger.darold.net/

Thanks for the report.  I agree that it would be nice to refresh the
link to the project.  Is there any objections in doing so and backpatch?
--
Michael


signature.asc
Description: PGP signature

Re: pgBadger link needs to be updated

[Michael Paquier]

On Sun, Dec 16, 2018 at 03:39:21PM +0900, Michael Paquier wrote:
> Thanks for the report.  I agree that it would be nice to refresh the
> link to the project.  Is there any objections in doing so and backpatch?

Committed and back-patched, thanks Peter for the report!
--
Michael


signature.asc
Description: PGP signature

Re: Broken link in JSON Types documentation

[Michael Paquier]

On Wed, Jan 02, 2019 at 06:11:55PM -0500, Tom Lane wrote:
> In PG v10 and up this link goes to https://tools.ietf.org/html/rfc7159.
> Evidently whoever updated it didn't bother to back-patch into old
> branches.

I don't think that really cool to have user-facing documentation which
goes to the void on supported branches, especially if there is an
adequate replacement.  Tom, do you think that this is worth updating?
--
Michael


signature.asc
Description: PGP signature

Re: Broken link in JSON Types documentation

[Michael Paquier]

On Fri, Jan 04, 2019 at 09:50:34AM -0500, Tom Lane wrote:
> I didn't quite have the energy to do something about it yesterday,
> but if you do, feel free.
> 
> (I'd suggest looking up the commit that fixed it, to see if it fixed
> anything else.)

Sure, that was my plan.  The change is from d542859, which did not go
to 9.6 and older, and committed.  There was one conflict for libpq
which was simple enough to fix, and I have also double-checked the
rest of the docs for rogue links to RFCs.
--
Michael


signature.asc
Description: PGP signature

Re: .pgpass

[Michael Paquier]

On Thu, Jan 10, 2019 at 11:02:08AM +0100, Daniel Gustafsson wrote:
> On 10 Jan 2019, at 04:45, PG Doc comments form  wrote:
>> I kindly suggest to let me edit this part to create examples such as 
>> pg_restore -h HOST -p PORT -U SUPERUSERNAME -C -d postgres
>> DBtoRESTORE.BACKUPEXTENSION
>> 
>> this will be like generalized examples that users can replicate.
> 
> The documentation isn’t edited online, but is contained in the postgres source
> repository.  If you want to edit it you will have to clone the repository and
> propose a patch with your changes on the pgsql-docs mailinglist.  The “Working
> with Git” article on the postgres wiki can be a good place to start if you are
> new to working in the source repository.

Before shaping a patch it is always possible to discuss the changes
you would like to do with the paragraphs you would like to introduce
as there is always room for improvements.  No, it is hard to
understand how you would like to improve things, why it makes sense,
and why the current set of examples is not enough to explain what you
are looking for.  The documentation needs to remain concise and a
maximum representative as well, so as any human reading it does not
get lost in a flow of useless information.  Based on the information
given in your initial mail, it is not really possible to see how at
least one of those points can be actually improved, so you need first
to bring more precisions on the matter.
--
Michael


signature.asc
Description: PGP signature

Re: Some typos in documentation

[Michael Paquier]

On Sun, Jan 13, 2019 at 12:17:07AM +0300, Alexander Lakhin wrote:
> While cross-checking the unique words in doc/ I found a few typos.

Those are good finds, thanks for the report!

> LockFileCreateWrite -- Should be spelled as LockFileCreateWRITE (as in
> pgstat.c) or LockFileCreateWRITE in pg.stat.s should use CamelCase in
> whole. I don't find any dependencies so I would prefer the latter.

For consistency with the rest I think that we should use
LockFileCreateWrite in pgstat.c as well as you do, though this could
be qualified as a behavior change.  That's also the grammar which is
documented since the beginning.  Any objections from others?

> All the corresponding patches are attached.

Cool, I'll see to apply them where needed.  I'll just wait to see if
anybody has comments about the pgstat.c portion.
--
Michael


signature.asc
Description: PGP signature

Re: Some typos in documentation

[Michael Paquier]

On Sun, Jan 13, 2019 at 09:34:36AM +0900, Michael Paquier wrote:
> Cool, I'll see to apply them where needed.  I'll just wait to see if
> anybody has comments about the pgstat.c portion.

And done.
--
Michael


signature.asc
Description: PGP signature