Обсуждение: clean up docs for v12

I reviewed docs like this:
git log -p remotes/origin/REL_11_STABLE..HEAD -- doc

And split some into separate patches, which may be useful at least for
reviewing.

I'm mailing now rather than after feature freeze to avoid duplicative work and
see if there's any issue.

Note, I also/already mailed this one separately:
|Clean up docs for log_statement_sample_rate..
https://www.postgresql.org/message-id/flat/20190328135918.GA27808%40telsasoft.com

Justin

Find attached updated patches for v12 docs.

Note that Alvaro applied an early patch for log_statement_sample_rate, but
unfortunately I hadn't sent a v2 patch with additional change from myon, so
there's one remaining hunk included here.

If needed I can split up differently for review, or resend a couple on separate
threads, or resend inline.

Patches are currently optimized for review, but maybe should be squished into
one and/or reindented before merging.

Justin

On Mon, Apr 08, 2019 at 09:18:28AM -0500, Justin Pryzby wrote:
> Find attached updated patches for v12 docs.

Thanks for taking the time to dig into such things.

> Note that Alvaro applied an early patch for log_statement_sample_rate, but
> unfortunately I hadn't sent a v2 patch with additional change from myon, so
> there's one remaining hunk included here.

This was in 0001.  Committed separately from the rest as the author
and discussion are different.

> If needed I can split up differently for review, or resend a couple on separate
> threads, or resend inline.
>
> Patches are currently optimized for review, but maybe should be squished into
> one and/or reindented before merging.

That's helpful.  However most of what you are proposing does not seem
necessary, and the current phrasing looks correct English to me, but I
am not a native speaker.  I am particularly referring to patches 0005
(publications use "a superuser" in error messages as well which could
be fixed as well?), 0006, 0007, 0008, 0011 and 0012.  I have committed
the most obvious mistakes extracted your patch set though.

Here are some comments about portions which need more work based on
what I looked at.

-        * Check if's guaranteed the all the desired attributes are available in
+        * Check if it's guaranteed that all the desired attributes are available in
         * tuple. If so, we can start deforming. If not, need to make sure to
     * fetch the missing columns.
Here I think that we should have "Check if all the desired attributes
are available in the tuple." for the first sentence.

     * If this is the first attribute, slot->tts_nvalid was 0. Therefore
-    * reset offset to 0 to, it be from a previous execution.
+    * also reset offset to 0, it may be from a previous execution.
The last part should be "as it may be from a previous execution"?

Andres, perhaps you have comments on these?
--
Michael

Thanks for committing those portions.

On Fri, Apr 19, 2019 at 05:00:26PM +0900, Michael Paquier wrote:
> I am particularly referring to patches 0005
> (publications use "a superuser" in error messages as well which could
> be fixed as well?),

I deliberately avoided changing thesee "errhint" messages, since the style
guide indicates that errhint should be a complete sentence.

pryzbyj@pryzbyj:~/src/postgres$ git grep 'must be a superuser' |grep errhint
src/backend/commands/event_trigger.c:                            errhint("The owner of an event trigger must be a
superuser.")));
src/backend/commands/foreigncmds.c:                              errhint("The owner of a foreign-data wrapper must be a
superuser.")));
src/backend/commands/publicationcmds.c:                                  errhint("The owner of a FOR ALL TABLES
publicationmust be a superuser.")));
 
src/backend/commands/subscriptioncmds.c:                                 errhint("The owner of a subscription must be a
superuser.")));

Justin

On Fri, Apr 19, 2019 at 09:43:01AM -0500, Justin Pryzby wrote:
> Thanks for committing those portions.

I have done an extra pass on your patch set to make sure that I am
missing nothing, and the last two remaining places which need some
tweaks are the comments from the JIT code you pointed out.  Attached
is a patch with these adjustments.
--
Michael

Hi,

On 2019-04-22 14:48:26 +0900, Michael Paquier wrote:
> On Fri, Apr 19, 2019 at 09:43:01AM -0500, Justin Pryzby wrote:
> > Thanks for committing those portions.
> 
> I have done an extra pass on your patch set to make sure that I am
> missing nothing, and the last two remaining places which need some
> tweaks are the comments from the JIT code you pointed out.  Attached
> is a patch with these adjustments.
> --
> Michael

> diff --git a/src/backend/jit/llvm/llvmjit_deform.c b/src/backend/jit/llvm/llvmjit_deform.c
> index 94b4635218..e7aa92e274 100644
> --- a/src/backend/jit/llvm/llvmjit_deform.c
> +++ b/src/backend/jit/llvm/llvmjit_deform.c
> @@ -298,9 +298,9 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc desc,
>      }
>  
>      /*
> -     * Check if's guaranteed the all the desired attributes are available in
> -     * tuple. If so, we can start deforming. If not, need to make sure to
> -     * fetch the missing columns.
> +     * Check if all the desired attributes are available in the tuple.  If so,
> +     * we can start deforming.  If not, we need to make sure to fetch the
> +     * missing columns.
>       */

That's imo not an improvement. The guaranteed bit is actually
relevant. What this block is doing is eliding the check against the
tuple header for the number of attributes, if NOT NULL attributes for
later columns guarantee that the desired columns are present in the NULL
bitmap. But the rephrasing makes it sound like we're actually checking
against the tuple.

I think it'd be better just to fix s/the all/that all/.


>      if ((natts - 1) <= guaranteed_column_number)
>      {
> @@ -383,7 +383,7 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc desc,
>  
>          /*
>           * If this is the first attribute, slot->tts_nvalid was 0. Therefore
> -         * reset offset to 0 to, it be from a previous execution.
> +         * reset offset to 0 too, as it may be from a previous execution.
>           */
>          if (attnum == 0)
>          {

That obviously makes sense.


Greetings,

Andres Freund

On 2019-Apr-22, Andres Freund wrote:

> On 2019-04-22 14:48:26 +0900, Michael Paquier wrote:

> >      /*
> > -     * Check if's guaranteed the all the desired attributes are available in
> > -     * tuple. If so, we can start deforming. If not, need to make sure to
> > -     * fetch the missing columns.
> > +     * Check if all the desired attributes are available in the tuple.  If so,
> > +     * we can start deforming.  If not, we need to make sure to fetch the
> > +     * missing columns.
> >       */
> 
> That's imo not an improvement. The guaranteed bit is actually
> relevant. What this block is doing is eliding the check against the
> tuple header for the number of attributes, if NOT NULL attributes for
> later columns guarantee that the desired columns are present in the NULL
> bitmap. But the rephrasing makes it sound like we're actually checking
> against the tuple.
> 
> I think it'd be better just to fix s/the all/that all/.

(and s/if's/if it's/)

> 
> >      if ((natts - 1) <= guaranteed_column_number)
> >      {
> > @@ -383,7 +383,7 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc desc,
> >  
> >          /*
> >           * If this is the first attribute, slot->tts_nvalid was 0. Therefore
> > -         * reset offset to 0 to, it be from a previous execution.
> > +         * reset offset to 0 too, as it may be from a previous execution.
> >           */
> >          if (attnum == 0)
> >          {
> 
> That obviously makes sense.

Hmm, I think "as it *is*", not "as it *may be*", right?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Apr-22, Andres Freund wrote:
>> On 2019-04-22 14:48:26 +0900, Michael Paquier wrote:
>>> /*
>>> -     * Check if's guaranteed the all the desired attributes are available in
>>> -     * tuple. If so, we can start deforming. If not, need to make sure to
>>> -     * fetch the missing columns.
>>> +     * Check if all the desired attributes are available in the tuple.  If so,
>>> +     * we can start deforming.  If not, we need to make sure to fetch the
>>> +     * missing columns.
>>> */

>> That's imo not an improvement. The guaranteed bit is actually
>> relevant. What this block is doing is eliding the check against the
>> tuple header for the number of attributes, if NOT NULL attributes for
>> later columns guarantee that the desired columns are present in the NULL
>> bitmap. But the rephrasing makes it sound like we're actually checking
>> against the tuple.
>>
>> I think it'd be better just to fix s/the all/that all/.

> (and s/if's/if it's/)

ISTM that Michael's proposed wording change shows that the existing
comment is easily misinterpreted.  I don't think these minor grammatical
fixes will avoid the misinterpretation problem, and so some more-extensive
rewording is called for.

But TBH, now that I look at the code, I think the entire optimization
is a bad idea and should be removed.  Am I right in thinking that the
presence of a wrong attnotnull marker could cause the generated code to
actually crash, thanks to not checking the tuple's natts field?  I don't
have enough faith in our enforcement of those constraints to want to see
JIT taking that risk to save a nanosecond or two.

(Possibly I'd not think this if I weren't fresh off a couple of days
with my nose in the ALTER TABLE SET NOT NULL code.  But right now,
I think that believing that that code does not and never will have
any bugs is just damfool.)

            regards, tom lane

Hi,

On 2019-04-22 12:33:24 -0400, Tom Lane wrote:
> ISTM that Michael's proposed wording change shows that the existing
> comment is easily misinterpreted.  I don't think these minor grammatical
> fixes will avoid the misinterpretation problem, and so some more-extensive
> rewording is called for.

Fair enough.


> But TBH, now that I look at the code, I think the entire optimization
> is a bad idea and should be removed.  Am I right in thinking that the
> presence of a wrong attnotnull marker could cause the generated code to
> actually crash, thanks to not checking the tuple's natts field?  I don't
> have enough faith in our enforcement of those constraints to want to see
> JIT taking that risk to save a nanosecond or two.

It's not a minor optimization, it's very measurable. Without the check
there's no pipeline stall when the memory for the tuple header is not in
the CPU cache (very common, especially for seqscans and such, due to the
"backward" memory location ordering of tuples in seqscans, which CPUs
don't predict). Server grade CPUs of the last ~5 years just march on and
start the work to fetch the first attributes (especially if they're NOT
NULL) - but can't do that if natts has to be checked. And starting to
check the NULL bitmap for NOT NULL attributes, would make that even
worse - and would required if we don't trust attnotnull.


> (Possibly I'd not think this if I weren't fresh off a couple of days
> with my nose in the ALTER TABLE SET NOT NULL code.  But right now,
> I think that believing that that code does not and never will have
> any bugs is just damfool.)

But there's plenty places where we rely on NOT NULL actually working?
We'll return wrong query results, and even crash in non-JIT places
because we thought there was guaranteed to be datum?

Greetings,

Andres Freund

Hi,

On 2019-04-22 09:43:56 -0700, Andres Freund wrote:
> On 2019-04-22 12:33:24 -0400, Tom Lane wrote:
> > ISTM that Michael's proposed wording change shows that the existing
> > comment is easily misinterpreted.  I don't think these minor grammatical
> > fixes will avoid the misinterpretation problem, and so some more-extensive
> > rewording is called for.
> 
> Fair enough.

The computation of that variable above has:

         * If the column is possibly missing, we can't rely on its (or
         * subsequent) NOT NULL constraints to indicate minimum attributes in
         * the tuple, so stop here.
         */
        if (att->atthasmissing)
            break;

        /*
         * Column is NOT NULL and there've been no preceding missing columns,
         * it's guaranteed that all columns up to here exist at least in the
         * NULL bitmap.
         */
        if (att->attnotnull)
            guaranteed_column_number = attnum;

and only then the comment referenced in the discussion here follows:
    /*
     * Check if's guaranteed the all the desired attributes are available in
     * tuple. If so, we can start deforming. If not, need to make sure to
     * fetch the missing columns.
     */


I think just reformulating that to something like

    /*
     * Check if it's guaranteed that all the desired attributes are available
     * in the tuple (but still possibly NULL), by dint of either the last
     * to-be-deformed column being NOT NULL, or subsequent ones not accessed
     * here being NOT NULL.  If that's not guaranteed the tuple headers natt's
     * has to be checked, and missing attributes potentially have to be
     * fetched (using slot_getmissingattrs().
    */

should make that clearer?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2019-04-22 12:33:24 -0400, Tom Lane wrote:
>> But TBH, now that I look at the code, I think the entire optimization
>> is a bad idea and should be removed.  Am I right in thinking that the
>> presence of a wrong attnotnull marker could cause the generated code to
>> actually crash, thanks to not checking the tuple's natts field?  I don't
>> have enough faith in our enforcement of those constraints to want to see
>> JIT taking that risk to save a nanosecond or two.

> It's not a minor optimization, it's very measurable.

Doesn't matter, if it's unsafe.

>> (Possibly I'd not think this if I weren't fresh off a couple of days
>> with my nose in the ALTER TABLE SET NOT NULL code.  But right now,
>> I think that believing that that code does not and never will have
>> any bugs is just damfool.)

> But there's plenty places where we rely on NOT NULL actually working?

I do not think there are any other places where we make this particular
assumption.  Given the number of ways in which we rely on there being
natts checks to avoid rewriting tables, I'm very afraid of the idea
that JIT is making more assumptions than the mainline code does.

In hopes of putting some fear into you too, I exhibit the following
behavior, which is not a bug according to our current definitions:

regression=# create table pp(f1 int);
CREATE TABLE
regression=# create table cc() inherits (pp);
CREATE TABLE
regression=# insert into cc values(1);
INSERT 0 1
regression=# insert into cc values(2);
INSERT 0 1
regression=# insert into cc values(null);
INSERT 0 1
regression=# alter table pp add column f2 text;
ALTER TABLE
regression=# alter table pp add column f3 text;
ALTER TABLE
regression=# alter table only pp alter f3 set not null;
ALTER TABLE
regression=# select * from pp;
 f1 | f2 | f3 
----+----+----
  1 |    | 
  2 |    | 
    |    | 
(3 rows)

The tuples coming out of cc will still have natts = 1, I believe.
If they were deformed according to pp's tupdesc, there'd be a
problem.  Now, we shouldn't do that, because this is not the only
possible discrepancy between parent and child tupdescs --- but
I think this example shows that attnotnull is a lot spongier
than you are assuming, even without considering the possibility
of outright bugs.

            regards, tom lane

Andres Freund <andres@anarazel.de> writes:
> The computation of that variable above has:

>          * If the column is possibly missing, we can't rely on its (or
>          * subsequent) NOT NULL constraints to indicate minimum attributes in
>          * the tuple, so stop here.
>          */
>         if (att->atthasmissing)
>             break;

BTW, why do we have to stop?  ISTM that a not-null column without
atthasmissing is enough to prove this, regardless of the state of prior
columns.  (This is assuming that you trust attnotnull for this, which
as I said I don't, but that's not relevant to this question.)  I wonder
also if it wouldn't be smart to explicitly check that the "guaranteeing"
column is not attisdropped.

> I think just reformulating that to something like

>     /*
>      * Check if it's guaranteed that all the desired attributes are available
>      * in the tuple (but still possibly NULL), by dint of either the last
>      * to-be-deformed column being NOT NULL, or subsequent ones not accessed
>      * here being NOT NULL.  If that's not guaranteed the tuple headers natt's
>      * has to be checked, and missing attributes potentially have to be
>      * fetched (using slot_getmissingattrs().
>     */

> should make that clearer?

OK by me.

            regards, tom lane

Hi,

On 2019-04-22 13:27:17 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > The computation of that variable above has:
> 
> >          * If the column is possibly missing, we can't rely on its (or
> >          * subsequent) NOT NULL constraints to indicate minimum attributes in
> >          * the tuple, so stop here.
> >          */
> >         if (att->atthasmissing)
> >             break;
> 
> BTW, why do we have to stop?  ISTM that a not-null column without
> atthasmissing is enough to prove this, regardless of the state of prior
> columns.  (This is assuming that you trust attnotnull for this, which
> as I said I don't, but that's not relevant to this question.)

Are you wondering if we could also use this kind of logic to infer the
length of the null bitmap if there's preceding columns with
atthasmissing true as long as there's a later !hasmissing column that's
NOT NULL?  Right. The logic could be made more powerful - I implemented
the above after Andrew's commit of fast-not-null broke JIT (not because
of that logic, but because it simply didn't look up the missing
columns).  I assume it doesn't terribly matter to be fast once
attributes after a previously missing one are accessed - it's likely not
going to be the hotly accessed data?


> I wonder
> also if it wouldn't be smart to explicitly check that the "guaranteeing"
> column is not attisdropped.

Yea, that probably would be smart.  I don't think there's an active
problem, because we remove NOT NULL when deleting an attribute, but it
seems good to be doubly sure / explain why that's safe:

        /* Remove any NOT NULL constraint the column may have */
        attStruct->attnotnull = false;

I'm a bit unsure whether to make it an assert, elog(ERROR) or just not
assume column presence?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2019-04-22 13:27:17 -0400, Tom Lane wrote:
>> I wonder
>> also if it wouldn't be smart to explicitly check that the "guaranteeing"
>> column is not attisdropped.

> Yea, that probably would be smart.  I don't think there's an active
> problem, because we remove NOT NULL when deleting an attribute, but it
> seems good to be doubly sure / explain why that's safe:
>         /* Remove any NOT NULL constraint the column may have */
>         attStruct->attnotnull = false;
> I'm a bit unsure whether to make it an assert, elog(ERROR) or just not
> assume column presence?

I'd just make the code look like

        /*
         * If it's NOT NULL then it must be present in every tuple,
         * unless there's a "missing" entry that could provide a non-null
         * value for it.  Out of paranoia, also check !attisdropped.
         */
        if (att->attnotnull &&
            !att->atthasmissing &&
            !att->attisdropped)
            guaranteed_column_number = attnum;

I don't think the extra check is so expensive as to be worth obsessing
over.

            regards, tom lane

Hi,

On 2019-04-22 14:17:48 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2019-04-22 13:27:17 -0400, Tom Lane wrote:
> >> I wonder
> >> also if it wouldn't be smart to explicitly check that the "guaranteeing"
> >> column is not attisdropped.
> 
> > Yea, that probably would be smart.  I don't think there's an active
> > problem, because we remove NOT NULL when deleting an attribute, but it
> > seems good to be doubly sure / explain why that's safe:
> >         /* Remove any NOT NULL constraint the column may have */
> >         attStruct->attnotnull = false;
> > I'm a bit unsure whether to make it an assert, elog(ERROR) or just not
> > assume column presence?
> 
> I'd just make the code look like
> 
>         /*
>          * If it's NOT NULL then it must be present in every tuple,
>          * unless there's a "missing" entry that could provide a non-null
>          * value for it.  Out of paranoia, also check !attisdropped.
>          */
>         if (att->attnotnull &&
>             !att->atthasmissing &&
>             !att->attisdropped)
>             guaranteed_column_number = attnum;
> 
> I don't think the extra check is so expensive as to be worth obsessing
> over.

Oh, yea, the cost is irrelevant here - it's one-off work basically, and
pales in comparison to the cost of JITing. I was more thinking about
whether it's worth "escalating" the violation of assumptions.

Greetings,

Andres Freund

Hi,

On 2019-04-22 13:18:18 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> >> (Possibly I'd not think this if I weren't fresh off a couple of days
> >> with my nose in the ALTER TABLE SET NOT NULL code.  But right now,
> >> I think that believing that that code does not and never will have
> >> any bugs is just damfool.)
> 
> > But there's plenty places where we rely on NOT NULL actually working?
> 
> I do not think there are any other places where we make this particular
> assumption.

Sure, not exactly the assumtion that JITed deforming benefits from, but
as far as I can tell, plenty things would be broken just as well if we
allowed NOT NULL columns to not be present (whether "physically" present
or present via atthasmissing) for tuples in a table. Fast defaults
wouldn't work, Assert(!isnull) checks would fire, primary keys would be
broken etc.



> In hopes of putting some fear into you too, I exhibit the following
> behavior, which is not a bug according to our current definitions:
> 
> regression=# create table pp(f1 int);
> CREATE TABLE
> regression=# create table cc() inherits (pp);
> CREATE TABLE
> regression=# insert into cc values(1);
> INSERT 0 1
> regression=# insert into cc values(2);
> INSERT 0 1
> regression=# insert into cc values(null);
> INSERT 0 1
> regression=# alter table pp add column f2 text;
> ALTER TABLE
> regression=# alter table pp add column f3 text;
> ALTER TABLE
> regression=# alter table only pp alter f3 set not null;
> ALTER TABLE
> regression=# select * from pp;
>  f1 | f2 | f3 
> ----+----+----
>   1 |    | 
>   2 |    | 
>     |    | 
> (3 rows)
> 
> The tuples coming out of cc will still have natts = 1, I believe.
> If they were deformed according to pp's tupdesc, there'd be a
> problem.  Now, we shouldn't do that, because this is not the only
> possible discrepancy between parent and child tupdescs --- but
> I think this example shows that attnotnull is a lot spongier
> than you are assuming, even without considering the possibility
> of outright bugs.

Unortunately it doesn't really put the fear into me - given that
attribute numbers don't even have to match between inheritance children,
making inferrences about the length of the NULL bitmap seems peanuts
compared to the breakage of using the wrong tupdesc to deform.

Greetings,

Andres Freund

On Mon, Apr 22, 2019 at 12:19:55PM -0400, Alvaro Herrera wrote:
> On 2019-Apr-22, Andres Freund wrote:
>> I think it'd be better just to fix s/the all/that all/.
>
> (and s/if's/if it's/)

FWIW, I have noticed that part when gathering all the pieces for what
became 148266f, still the full paragraph was sort of confusing, so I
have just fixed the most obvious issues reported first.
--
Michael

Hi,

On Tue, Apr 23, 2019 at 11:50:42AM +0900, Michael Paquier wrote:
> On Mon, Apr 22, 2019 at 12:19:55PM -0400, Alvaro Herrera wrote:
> > On 2019-Apr-22, Andres Freund wrote:
> >> I think it'd be better just to fix s/the all/that all/.
> > 
> > (and s/if's/if it's/)
> 
> FWIW, I have noticed that part when gathering all the pieces for what
> became 148266f, still the full paragraph was sort of confusing, so I
> have just fixed the most obvious issues reported first.

I saw you closed the item here:
https://wiki.postgresql.org/index.php?title=PostgreSQL_12_Open_Items&diff=33390&oldid=33389

But I think the biggest part of the patch is still not even reviewed ?
I'm referring to ./*review-docs-for-pg12dev.patch 

I haven't updated the JIT changes since there's larger discussion regarding the
code.

Justin

On Fri, Apr 26, 2019 at 12:17:22PM -0500, Justin Pryzby wrote:
> But I think the biggest part of the patch is still not even reviewed ?
> I'm referring to ./*review-docs-for-pg12dev.patch

Nope.  I looked at the patch, and as mentioned upthread the suggested
changes did not seem like improvements as the existing sentences make
sense, at least to me.  Do you have any particular part of your patch
where you think your wording is an improvement?  Why do you think so?
--
Michael

On Sat, Apr 27, 2019 at 09:44:20AM +0900, Michael Paquier wrote:
> On Fri, Apr 26, 2019 at 12:17:22PM -0500, Justin Pryzby wrote:
> > But I think the biggest part of the patch is still not even reviewed ?
> > I'm referring to ./*review-docs-for-pg12dev.patch
> 
> Nope.  I looked at the patch, and as mentioned upthread the suggested
> changes did not seem like improvements as the existing sentences make
> sense, at least to me.  Do you have any particular part of your patch
> where you think your wording is an improvement?  Why do you think so?

That's mostly new language from v12 commits which I specifically reviewed and
worth cleaning up before release.

If nobody else is interested then I'll forget about it, but they're *all*
(minor) improvements IMO.  

I don't think it's be useful to enumerate justifications for each hunk; if one
of them isn't agreed to be an improvement, I'd just remove it.

But here's some one-liner excerpts.

-      is <literal>2</literal> bits and maximum is <literal>4095</literal>.  Parameters for
+      is <literal>2</literal> bits and the maximum is <literal>4095</literal>.  Parameters for

Adding "the" makes it a complete sentence and not a fragment.

-        all autovacuum actions. Minus-one (the default) disables logging
+        all autovacuum actions. <literal>-1</literal> (the default) disables logging

There's nothing else that says "minus-one" anywhere else on that page.  I just
found one in auto-explain.sgml, which I changed.

-    than 16KB; <function>gss_wrap_size_limit()</function> should be used by the
+    than 16kB; <function>gss_wrap_size_limit()</function> should be used by the

Every other use in documentation has a lowercase "kay", and PG itself doesn't
accept "KB" unit suffix.

-     A few features included in the C99 standard are, at this time, not be
+     A few features included in the C99 standard are, at this time, not
      permitted to be used in core <productname>PostgreSQL</productname>

Indisputably wrong ?

Justin

On Fri, Apr 26, 2019 at 09:56:47PM -0500, Justin Pryzby wrote:
> But here's some one-liner excerpts.
>
> -      is <literal>2</literal> bits and maximum is <literal>4095</literal>.  Parameters for
> +      is <literal>2</literal> bits and the maximum is <literal>4095</literal>.  Parameters for
>
> Adding "the" makes it a complete sentence and not a fragment.

Not sure here either that it matters.

> -        all autovacuum actions. Minus-one (the default) disables logging
> +        all autovacuum actions. <literal>-1</literal> (the default) disables logging
>
> There's nothing else that says "minus-one" anywhere else on that page.  I just
> found one in auto-explain.sgml, which I changed.

That's one of these I am not sure about.

> -    than 16KB; <function>gss_wrap_size_limit()</function> should be used by the
> +    than 16kB; <function>gss_wrap_size_limit()</function> should be used by the
>
> Every other use in documentation has a lowercase "kay", and PG itself doesn't
> accept "KB" unit suffix.

Right.  There are more places like that, particularly in the comments
of the code.

> -     A few features included in the C99 standard are, at this time, not be
> +     A few features included in the C99 standard are, at this time, not
>       permitted to be used in core <productname>PostgreSQL</productname>
>
> Indisputably wrong ?

Yep, this one is wrong as-is.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Fri, Apr 26, 2019 at 09:56:47PM -0500, Justin Pryzby wrote:
>> -        all autovacuum actions. Minus-one (the default) disables logging
>> +        all autovacuum actions. <literal>-1</literal> (the default) disables logging
>>
>> There's nothing else that says "minus-one" anywhere else on that page.  I just
>> found one in auto-explain.sgml, which I changed.

> That's one of these I am not sure about.

FWIW, I think we generally write this the way Justin suggests.  It's
more precise, at least if you're reading it in a way that makes
<literal> text distinguishable from plain text: what to put into
the config file is exactly "-1", and not for instance "minus-one".

            regards, tom lane

On Sat, Apr 27, 2019 at 11:10:46AM -0400, Tom Lane wrote:
> FWIW, I think we generally write this the way Justin suggests.  It's
> more precise, at least if you're reading it in a way that makes
> <literal> text distinguishable from plain text: what to put into
> the config file is exactly "-1", and not for instance "minus-one".

Okay, sold.  I have done and extra pass on v2-0002 and included again
some obvious mistakes.  I have noticed a couple of things on the way.
--
Michael

Hi,

On 2019-04-22 14:17:48 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2019-04-22 13:27:17 -0400, Tom Lane wrote:
> >> I wonder
> >> also if it wouldn't be smart to explicitly check that the "guaranteeing"
> >> column is not attisdropped.
> 
> > Yea, that probably would be smart.  I don't think there's an active
> > problem, because we remove NOT NULL when deleting an attribute, but it
> > seems good to be doubly sure / explain why that's safe:
> >         /* Remove any NOT NULL constraint the column may have */
> >         attStruct->attnotnull = false;
> > I'm a bit unsure whether to make it an assert, elog(ERROR) or just not
> > assume column presence?
> 
> I'd just make the code look like
> 
>         /*
>          * If it's NOT NULL then it must be present in every tuple,
>          * unless there's a "missing" entry that could provide a non-null
>          * value for it.  Out of paranoia, also check !attisdropped.
>          */
>         if (att->attnotnull &&
>             !att->atthasmissing &&
>             !att->attisdropped)
>             guaranteed_column_number = attnum;
> 
> I don't think the extra check is so expensive as to be worth obsessing
> over.

Pushed. Did so separately from Justin's changes, since this is a small
functional change.

Greetings,

Andres Freund

Hi,

On 2019-04-08 09:18:28 -0500, Justin Pryzby wrote:
> From aae1a84b74436951222dba42b21de284ed8b1ac9 Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <pryzbyj@telsasoft.com>
> Date: Sat, 30 Mar 2019 17:24:35 -0500
> Subject: [PATCH v2 03/12] JIT typos..
> 
> ..which I sent to Andres some time ago and which I noticed were never applied
> (nor rejected).
> 
> https://www.postgresql.org/message-id/20181127184133.GM10913%40telsasoft.com
> ---
>  src/backend/jit/llvm/llvmjit_deform.c   | 22 +++++++++++-----------
>  src/backend/jit/llvm/llvmjit_inline.cpp |  2 +-
>  2 files changed, 12 insertions(+), 12 deletions(-)

I pushed these, minus the ones that were obsoleted by the slightly
larger changes resulting from the discussion of this patch.

Thanks for the patch!

Greetings,

Andres Freund

On Sat, Mar 30, 2019 at 05:43:33PM -0500, Justin Pryzby wrote:
> I reviewed docs like this:
> git log -p remotes/origin/REL_11_STABLE..HEAD -- doc

On Fri, Apr 19, 2019 at 05:00:26PM +0900, Michael Paquier wrote:
> However most of what you are proposing does not seem necessary, and the
> current phrasing looks correct English to me, but I am not a native speaker.

Michael reviewed and committed several portions of previous version of this
patch, but I think each of the remaining changes are individually each minor
improvements and combine are a significant improvement, so I'm requesting
additional review.  I think it'd be much better than submission and review of
dozens of separate mails while individually rediscovering the same things..

Thanks in advance for any review.

Justin

Hi,

On 2019-05-20 13:20:01 -0500, Justin Pryzby wrote:
> On Sat, Mar 30, 2019 at 05:43:33PM -0500, Justin Pryzby wrote:
> Thanks in advance for any review.

I find these pretty tedious to work with. I'm somewhat dyslexic, not a
native speaker. So it requires a lot of concentration to go through
them...


> @@ -3052,7 +3052,7 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
>         simplifies <command>ATTACH/DETACH PARTITION</command> operations:
>         the partition dependencies need only be added or removed.
>         Example: a child partitioned index is made partition-dependent
> -       on both the partition table it is on and the parent partitioned
> +       on both the table partition and the parent partitioned
>         index, so that it goes away if either of those is dropped, but
>         not otherwise.  The dependency on the parent index is primary,
>         so that if the user tries to drop the child partitioned index,

> @@ -3115,7 +3115,7 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
>     Note that it's quite possible for two objects to be linked by more than
>     one <structname>pg_depend</structname> entry.  For example, a child
>     partitioned index would have both a partition-type dependency on its
> -   associated partition table, and an auto dependency on each column of
> +   associated table partition, and an auto dependency on each column of
>     that table that it indexes.  This sort of situation expresses the union
>     of multiple dependency semantics.  A dependent object can be dropped
>     without <literal>CASCADE</literal> if any of its dependencies satisfies

Hm, that's not an improvement from my POV? The version before isn't great either,
but it seems to improve this'd require a somewhat bigger hammer.



> @@ -6947,8 +6948,8 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
>         <para>
>          Causes each action executed by autovacuum to be logged if it ran for at
>          least the specified number of milliseconds.  Setting this to zero logs
> -        all autovacuum actions. <literal>-1</literal> (the default) disables
> -        logging autovacuum actions.  For example, if you set this to
> +        all autovacuum actions. <literal>-1</literal> (the default) disables logging
> +        autovacuum actions.  For example, if you set this to
>          <literal>250ms</literal> then all automatic vacuums and analyzes that run
>          250ms or longer will be logged.  In addition, when this parameter is
>          set to any value other than <literal>-1</literal>, a message will be

Hm?


> diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
> index a0a7435..cfe83a8 100644
> --- a/doc/src/sgml/ddl.sgml
> +++ b/doc/src/sgml/ddl.sgml
> @@ -3867,12 +3867,12 @@ CREATE INDEX ON measurement (logdate);
>  
>      <para>
>        Normally the set of partitions established when initially defining the
> -      table are not intended to remain static.  It is common to want to
> -      remove old partitions of data and periodically add new partitions for
> +      table are not intended to remain static.  It's common to
> +      remove partitions of old data and add partitions for
>        new data. One of the most important advantages of partitioning is
> -      precisely that it allows this otherwise painful task to be executed
> +      allowing this otherwise painful task to be executed
>        nearly instantaneously by manipulating the partition structure, rather
> -      than physically moving large amounts of data around.
> +      than physically moving around large amounts of data.
>      </para>

I don't understand what the point of changing things like "It is" to
"It's". There's more uses of the former in the docs.

I'm also not sure that I like the removal of "to want" etc,
because just because it's a common desire, it's not automatically common
practice. And I think the 'periodically' is actually a reasonable hint
that partition creation doesn't happen automatically or in the
foreground.


>       <row>
>        <entry><structfield>partitions_done</structfield></entry>
>        <entry><type>bigint</type></entry>
>        <entry>
> -       When creating an index on a partitioned table, this column is set to
> -       the number of partitions on which the index has been completed.
> +       When creating an index on a partitioned table, this is
> +       the number of partitions for which the process is complete.
>        </entry>
>       </row>
>      </tbody>

> @@ -3643,9 +3643,9 @@ SELECT pg_stat_get_backend_pid(s.backendid) AS pid,
>        <entry>
>         The index is being built by the access method-specific code.  In this phase,
>         access methods that support progress reporting fill in their own progress data,
> -       and the subphase is indicated in this column.  Typically,
> +       and the subphase is indicated in this column.
>         <structname>blocks_total</structname> and <structname>blocks_done</structname>
> -       will contain progress data, as well as potentially
> +       will contain progress data, as may
>         <structname>tuples_total</structname> and <structname>tuples_done</structname>.
>        </entry>

Hm, if you're intent on removing "this column", why not here?

Is the removal of "typically" and "potentially" actually correct here?
Somebody clearly wanted to indicate it's not guaranteed?


> @@ -3922,9 +3922,9 @@ SELECT pg_stat_get_backend_pid(s.backendid) AS pid,
>    <title>CLUSTER Progress Reporting</title>
>  
>    <para>
> -   Whenever <command>CLUSTER</command> or <command>VACUUM FULL</command> is
> -   running, the <structname>pg_stat_progress_cluster</structname> view will
> -   contain a row for each backend that is currently running either command.
> +   The <structname>pg_stat_progress_cluster</structname> view will contain a
> +   row for each backend that is running either
> +   <command>CLUSTER</command> or <command>VACUUM FULL</command>.
>     The tables below describe the information that will be reported and
>     provide information about how to interpret it.
>    </para>

Unrelated to your change, but I noticed it in this hunk. Isn't it weird
to say "will contain" rather than just "contains" for this type of
reference documentation?


> diff --git a/doc/src/sgml/perform.sgml b/doc/src/sgml/perform.sgml
> index a84be85..65c161b 100644
> --- a/doc/src/sgml/perform.sgml
> +++ b/doc/src/sgml/perform.sgml
> @@ -899,10 +899,10 @@ EXPLAIN ANALYZE SELECT * FROM tenk1 WHERE unique1 < 100 AND unique2 > 9000
>      Generally, the <command>EXPLAIN</command> output will display details for
>      every plan node which was generated by the query planner.  However, there
>      are cases where the executor is able to determine that certain nodes are
> -    not required; currently, the only node types to support this are the
> -    <literal>Append</literal> and <literal>MergeAppend</literal> nodes.  These
> -    node types have the ability to discard subnodes which they are able to
> -    determine won't contain any records required by the query.  It is possible
> +    not required; currently, the only node types to support this are
> +    <literal>Append</literal> and <literal>MergeAppend</literal>, which
> +    are able to discard subnodes when it's deduced that
> +    they will not contain any records required by the query.  It is possible
>      to determine that nodes have been removed in this way by the presence of a
>      "Subplans Removed" property in the <command>EXPLAIN</command> output.
>     </para>

Shrug.


> diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml
> index 49b081a..dd80bd0 100644
> --- a/doc/src/sgml/ref/alter_table.sgml
> +++ b/doc/src/sgml/ref/alter_table.sgml
> @@ -219,7 +219,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
>  
>       <para>
>        <literal>SET NOT NULL</literal> may only be applied to a column
> -      providing none of the records in the table contain a
> +      provided none of the records in the table contain a
>        <literal>NULL</literal> value for the column.  Ordinarily this is
>        checked during the <literal>ALTER TABLE</literal> by scanning the
>        entire table; however, if a valid <literal>CHECK</literal> constraint is

It'd be easier to review / apply this kind of thing if stylistic
choices, especially borderline ones, were separated from clear typos
(like this one).


> diff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
> index 30bb38b..bf4f550 100644
> --- a/doc/src/sgml/ref/create_index.sgml
> +++ b/doc/src/sgml/ref/create_index.sgml
> @@ -181,8 +181,8 @@ CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] <replaceable class=
>         </para>
>  
>         <para>
> -        Currently, the B-tree and the GiST index access methods support this
> -        feature.  In B-tree and the GiST indexes, the values of columns listed
> +        Currently, only the B-tree and GiST index access methods support this
> +        feature.  In B-tree and GiST indexes, the values of columns listed
>          in the <literal>INCLUDE</literal> clause are included in leaf tuples
>          which correspond to heap tuples, but are not included in upper-level
>          index entries used for tree navigation.

Hm. External index AMs also can also support uniqueness. So perhaps not
adding only is actually better? I realize that other places in the same
file already say "only".


> diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
> index 4d91eeb..6f6d220 100644
> --- a/doc/src/sgml/ref/pg_rewind.sgml
> +++ b/doc/src/sgml/ref/pg_rewind.sgml
> @@ -106,15 +106,14 @@ PostgreSQL documentation
>     </para>
>  
>     <para>
> -    <application>pg_rewind</application> will fail immediately if it finds
> -    files it cannot write directly to.  This can happen for example when
> -    the source and the target server use the same file mapping for read-only
> -    SSL keys and certificates.  If such files are present on the target server
> +    <application>pg_rewind</application> will fail immediately if it experiences
> +    a write error.  This can happen for example when
> +    the source and target server use the same file mapping for read-only
> +    SSL keys and certificates.  If such files are present on the target server,
>      it is recommended to remove them before running

That's not really the same. The failure will often be *before* a write
error. E.g.
    mode = O_WRONLY | O_CREAT | PG_BINARY;
    if (trunc)
        mode |= O_TRUNC;
    dstfd = open(dstpath, mode, pg_file_create_mode);
    if (dstfd < 0)
        pg_fatal("could not open target file \"%s\": %m",
                 dstpath);
will fail, rather than a write().




> @@ -474,10 +474,8 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
>             </listitem>
>            </itemizedlist>
>  
> -        Because in "prepared" mode <application>pgbench</application> reuses
> -        the parse analysis result for the second and subsequent query
> -        iteration, <application>pgbench</application> runs faster in the
> -        prepared mode than in other modes.
> +        <application>pgbench</application> runs faster in prepared mode because the
> +        parse analysis happens only during the first query.
>         </para>

That text seems wrong before and after. It's not just parse analysis,
but also planning?


> --- a/doc/src/sgml/runtime.sgml
> +++ b/doc/src/sgml/runtime.sgml
> @@ -2634,8 +2634,9 @@ openssl x509 -req -in server.csr -text -days 365 \
>     using <acronym>GSSAPI</acronym> to encrypt client/server communications for
>     increased security.  Support requires that a <acronym>GSSAPI</acronym>
>     implementation (such as MIT krb5) is installed on both client and server
> -   systems, and that support in <productname>PostgreSQL</productname> is
> -   enabled at build time (see <xref linkend="installation"/>).
> +   systems, and must be enabled at the time
> +   <productname>PostgreSQL</productname> is built (see <xref
> +   linkend="installation"/>).
>    </para>

This is weird before and after. I'd just say "that PostgreSQL has been
built with GSSAPI support".


>      <para>
> -     For example <literal>_StaticAssert()</literal> and
> +     For example, <literal>_StaticAssert()</literal> and
>       <literal>__builtin_constant_p</literal> are currently used, even though
> -     they are from newer revisions of the C standard and a
> -     <productname>GCC</productname> extension respectively. If not available
> -     we respectively fall back to using a C99 compatible replacement that
> -     performs the same checks, but emits rather cryptic messages and do not
> +     they are from a newer revision of the C standard and a
> +     <productname>GCC</productname> extension, respectively. If not available, in the first case, 
> +     we fall back to using a C99 compatible replacement that
> +     performs the same checks, but emits rather cryptic messages; in the second case, we do not
>       use <literal>__builtin_constant_p</literal>.
>      </para>

To me the point of changing just about equivalent formulations into
another isn't clear.


> @@ -3381,7 +3381,7 @@ if (!ptr)
>      The parallel safety property (<literal>PARALLEL
>      UNSAFE</literal>, <literal>PARALLEL RESTRICTED</literal>, or
>      <literal>PARALLEL SAFE</literal>) must also be specified if you hope
> -    to use the function in parallelized queries.
> +    queries calling the function to use parallel query.
>      It can also be useful to specify the function's estimated execution
>      cost, and/or the number of rows a set-returning function is estimated
>      to return.  However, the declarative way of specifying those two

Seems like a larger rewrite is needed


> @@ -3393,7 +3393,7 @@ if (!ptr)
>      It is also possible to attach a <firstterm>planner support
>      function</firstterm> to a SQL-callable function (called
>      its <firstterm>target function</firstterm>), and thereby provide
> -    knowledge about the target function that is too complex to be
> +    information about the target function that is too complex to be
>      represented declaratively.  Planner support functions have to be
>      written in C (although their target functions might not be), so this is
>      an advanced feature that relatively few people will use.

Why s/knowledge/information/?


> From d8c8b5416726909203db53cfa73ea2c7cc0fe9a0 Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <pryzbyj@telsasoft.com>
> Date: Fri, 29 Mar 2019 19:37:35 -0500
> Subject: [PATCH v3 02/12] Add comma for readability

You did plenty of that in the previous set of changes...

> ---
>  doc/src/sgml/backup.sgml              |  2 +-
>  doc/src/sgml/bki.sgml                 |  2 +-
>  doc/src/sgml/client-auth.sgml         |  4 ++--
>  doc/src/sgml/config.sgml              |  4 ++--
>  doc/src/sgml/ddl.sgml                 |  2 +-
>  doc/src/sgml/indices.sgml             |  2 +-
>  doc/src/sgml/installation.sgml        |  2 +-
>  doc/src/sgml/logical-replication.sgml |  2 +-
>  doc/src/sgml/protocol.sgml            |  6 +++---
>  doc/src/sgml/ref/create_table.sgml    |  2 +-
>  doc/src/sgml/ref/create_table_as.sgml |  2 +-
>  doc/src/sgml/ref/pgupgrade.sgml       |  2 +-
>  doc/src/sgml/ref/psql-ref.sgml        |  2 +-
>  doc/src/sgml/sources.sgml             | 14 +++++++-------
>  doc/src/sgml/wal.sgml                 |  2 +-
>  doc/src/sgml/xoper.sgml               |  2 +-
>  16 files changed, 26 insertions(+), 26 deletions(-)
> 
> diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
> index b67da89..ae41bc7 100644
> --- a/doc/src/sgml/backup.sgml
> +++ b/doc/src/sgml/backup.sgml
> @@ -1024,7 +1024,7 @@ SELECT pg_start_backup('label', true);
>       consider during this backup.
>      </para>
>      <para>
> -     As noted above, if the server crashes during the backup it may not be
> +     As noted above, if the server crashes during the backup, it may not be
>       possible to restart until the <literal>backup_label</literal> file has
>       been manually deleted from the <envar>PGDATA</envar> directory.  Note
>       that it is very important to never remove the
> diff --git a/doc/src/sgml/bki.sgml b/doc/src/sgml/bki.sgml
> index aa3d6f8..e27fa76 100644
> --- a/doc/src/sgml/bki.sgml
> +++ b/doc/src/sgml/bki.sgml
> @@ -403,7 +403,7 @@
>      8000—9999.  This minimizes the risk of OID collisions with other
>      patches being developed concurrently.  To keep the 8000—9999
>      range free for development purposes, after a patch has been committed
> -    to the master git repository its OIDs should be renumbered into
> +    to the master git repository, its OIDs should be renumbered into
>      available space below that range.  Typically, this will be done
>      near the end of each development cycle, moving all OIDs consumed by
>      patches committed in that cycle at the same time.  The script
> diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
> index ffed887..098900e 100644
> --- a/doc/src/sgml/client-auth.sgml
> +++ b/doc/src/sgml/client-auth.sgml
> @@ -157,7 +157,7 @@ hostnogssenc <replaceable>database</replaceable>  <replaceable>user</replaceable
>        </para>
>  
>        <para>
> -       To make use of this option the server must be built with
> +       To make use of this option, the server must be built with
>         <acronym>SSL</acronym> support. Furthermore,
>         <acronym>SSL</acronym> must be enabled
>         by setting the <xref linkend="guc-ssl"/> configuration parameter (see
> @@ -189,7 +189,7 @@ hostnogssenc <replaceable>database</replaceable>  <replaceable>user</replaceable
>        </para>
>  
>        <para>
> -       To make use of this option the server must be built with
> +       To make use of this option, the server must be built with
>         <acronym>GSSAPI</acronym> support.  Otherwise,
>         the <literal>hostgssenc</literal> record is ignored except for logging
>         a warning that it cannot match any connections.
> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
> index 73eb768..54b91d3 100644
> --- a/doc/src/sgml/config.sgml
> +++ b/doc/src/sgml/config.sgml
> @@ -3458,7 +3458,7 @@ restore_command = 'copy "C:\\server\\archivedir\\%f" "%p"'  # Windows
>          reached. The default is <literal>pause</literal>, which means recovery will
>          be paused. <literal>promote</literal> means the recovery process will finish
>          and the server will start to accept connections.
> -        Finally <literal>shutdown</literal> will stop the server after reaching the
> +        Finally, <literal>shutdown</literal> will stop the server after reaching the
>          recovery target.
>         </para>
>         <para>
> @@ -4188,7 +4188,7 @@ ANY <replaceable class="parameter">num_sync</replaceable> ( <replaceable class="
>         </para>
>         <para>
>          The delay occurs once the database in recovery has reached a consistent
> -        state, until the standby is promoted or triggered. After that the standby
> +        state, until the standby is promoted or triggered. After that, the standby
>          will end recovery without further waiting.
>         </para>
>         <para>
> diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
> index cfe83a8..8135507 100644
> --- a/doc/src/sgml/ddl.sgml
> +++ b/doc/src/sgml/ddl.sgml
> @@ -3866,7 +3866,7 @@ CREATE INDEX ON measurement (logdate);
>      <title>Partition Maintenance</title>
>  
>      <para>
> -      Normally the set of partitions established when initially defining the
> +      Normally, the set of partitions established when initially defining the
>        table are not intended to remain static.  It's common to
>        remove partitions of old data and add partitions for
>        new data. One of the most important advantages of partitioning is
> diff --git a/doc/src/sgml/indices.sgml b/doc/src/sgml/indices.sgml
> index 95c0a19..e940ddb 100644
> --- a/doc/src/sgml/indices.sgml
> +++ b/doc/src/sgml/indices.sgml
> @@ -1081,7 +1081,7 @@ SELECT x FROM tab WHERE x = 'key' AND z < 42;
>     scan.  Even in the successful case, this approach trades visibility map
>     accesses for heap accesses; but since the visibility map is four orders
>     of magnitude smaller than the heap it describes, far less physical I/O is
> -   needed to access it.  In most situations the visibility map remains
> +   needed to access it.  In most situations, the visibility map remains
>     cached in memory all the time.
>    </para>
>  
> diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
> index 4493862..847e028 100644
> --- a/doc/src/sgml/installation.sgml
> +++ b/doc/src/sgml/installation.sgml
> @@ -2527,7 +2527,7 @@ xcodebuild -version -sdk macosx Path
>  </programlisting>
>      Note that building an extension using a different sysroot version than
>      was used to build the core server is not really recommended; in the
> -    worst case it could result in hard-to-debug ABI inconsistencies.
> +    worst case, it could result in hard-to-debug ABI inconsistencies.
>     </para>
>  
>     <para>
> diff --git a/doc/src/sgml/logical-replication.sgml b/doc/src/sgml/logical-replication.sgml
> index 3f2f674..31c814c 100644
> --- a/doc/src/sgml/logical-replication.sgml
> +++ b/doc/src/sgml/logical-replication.sgml
> @@ -201,7 +201,7 @@
>  
>    <para>
>     Subscriptions are dumped by <command>pg_dump</command> if the current user
> -   is a superuser.  Otherwise a warning is written and subscriptions are
> +   is a superuser.  Otherwise, a warning is written and subscriptions are
>     skipped, because non-superusers cannot read all subscription information
>     from the <structname>pg_subscription</structname> catalog.
>    </para>
> diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
> index 70f7286..3f907f6 100644
> --- a/doc/src/sgml/protocol.sgml
> +++ b/doc/src/sgml/protocol.sgml
> @@ -1449,7 +1449,7 @@ SELECT 1/0;
>      <literal>S</literal>, perform an <acronym>SSL</acronym> startup handshake
>      (not described here, part of the <acronym>SSL</acronym>
>      specification) with the server.  If this is successful, continue
> -    with sending the usual StartupMessage.  In this case the
> +    with sending the usual StartupMessage.  In this case, the
>      StartupMessage and all subsequent data will be
>      <acronym>SSL</acronym>-encrypted.  To continue after
>      <literal>N</literal>, send the usual StartupMessage and proceed without
> @@ -1462,7 +1462,7 @@ SELECT 1/0;
>      the server predates the addition of <acronym>SSL</acronym> support
>      to <productname>PostgreSQL</productname>.  (Such servers are now very ancient,
>      and likely do not exist in the wild anymore.)
> -    In this case the connection must
> +    In this case, the connection must
>      be closed, but the frontend might choose to open a fresh connection
>      and proceed without requesting <acronym>SSL</acronym>.
>     </para>
> @@ -1528,7 +1528,7 @@ SELECT 1/0;
>      The frontend should also be prepared to handle an ErrorMessage
>      response to GSSENCRequest from the server.  This would only occur if
>      the server predates the addition of <acronym>GSSAPI</acronym> encryption
> -    support to <productname>PostgreSQL</productname>.  In this case the
> +    support to <productname>PostgreSQL</productname>.  In this case, the
>      connection must be closed, but the frontend might choose to open a fresh
>      connection and proceed without requesting <acronym>GSSAPI</acronym>
>      encryption.  Given the length limits specified above, the ErrorMessage
> diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
> index 44a61ef..dbb1468 100644
> --- a/doc/src/sgml/ref/create_table.sgml
> +++ b/doc/src/sgml/ref/create_table.sgml
> @@ -1189,7 +1189,7 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM
>        This clause specifies optional storage parameters for a table or index;
>        see <xref linkend="sql-createtable-storage-parameters"
>        endterm="sql-createtable-storage-parameters-title"/> for more
> -      information.  For backward-compatibility the <literal>WITH</literal>
> +      information.  For backward-compatibility, the <literal>WITH</literal>
>        clause for a table can also include <literal>OIDS=FALSE</literal> to
>        specify that rows of the new table should not contain OIDs (object
>        identifiers), <literal>OIDS=TRUE</literal> is not supported anymore.
> diff --git a/doc/src/sgml/ref/create_table_as.sgml b/doc/src/sgml/ref/create_table_as.sgml
> index b5c4ce6..0880459 100644
> --- a/doc/src/sgml/ref/create_table_as.sgml
> +++ b/doc/src/sgml/ref/create_table_as.sgml
> @@ -142,7 +142,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
>        This clause specifies optional storage parameters for the new table;
>        see <xref linkend="sql-createtable-storage-parameters"
>        endterm="sql-createtable-storage-parameters-title"/> for more
> -      information.   For backward-compatibility the <literal>WITH</literal>
> +      information.   For backward-compatibility, the <literal>WITH</literal>
>        clause for a table can also include <literal>OIDS=FALSE</literal> to
>        specify that rows of the new table should contain no OIDs (object
>        identifiers), <literal>OIDS=TRUE</literal> is not supported anymore.
> diff --git a/doc/src/sgml/ref/pgupgrade.sgml b/doc/src/sgml/ref/pgupgrade.sgml
> index 8288676..6a898d9 100644
> --- a/doc/src/sgml/ref/pgupgrade.sgml
> +++ b/doc/src/sgml/ref/pgupgrade.sgml
> @@ -746,7 +746,7 @@ psql --username=postgres --file=script.sql postgres
>     <application>pg_upgrade</application> launches short-lived postmasters in
>     the old and new data directories.  Temporary Unix socket files for
>     communication with these postmasters are, by default, made in the current
> -   working directory.  In some situations the path name for the current
> +   working directory.  In some situations, the path name for the current
>     directory might be too long to be a valid socket name.  In that case you
>     can use the <option>-s</option> option to put the socket files in some
>     directory with a shorter path name.  For security, be sure that that
> diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
> index b867640..55cc78c 100644
> --- a/doc/src/sgml/ref/psql-ref.sgml
> +++ b/doc/src/sgml/ref/psql-ref.sgml
> @@ -1053,7 +1053,7 @@ testdb=>
>          These operations are not as efficient as the <acronym>SQL</acronym>
>          <command>COPY</command> command with a file or program data source or
>          destination, because all data must pass through the client/server
> -        connection.  For large amounts of data the <acronym>SQL</acronym>
> +        connection.  For large amounts of data, the <acronym>SQL</acronym>
>          command might be preferable.
>          </para>
>          </tip>
> diff --git a/doc/src/sgml/sources.sgml b/doc/src/sgml/sources.sgml
> index 2b9f59d..520bbae 100644
> --- a/doc/src/sgml/sources.sgml
> +++ b/doc/src/sgml/sources.sgml
> @@ -100,7 +100,7 @@ less -x4
>     <para>
>      There are two required elements for every message: a severity level
>      (ranging from <literal>DEBUG</literal> to <literal>PANIC</literal>) and a primary
> -    message text.  In addition there are optional elements, the most
> +    message text.  In addition, there are optional elements, the most
>      common of which is an error identifier code that follows the SQL spec's
>      SQLSTATE conventions.
>      <function>ereport</function> itself is just a shell function that exists
> @@ -473,7 +473,7 @@ Hint:       the addendum
>  
>     <para>
>      Rationale: Messages are not necessarily displayed on terminal-type
> -    displays.  In GUI displays or browsers these formatting instructions are
> +    displays.  In GUI displays or browsers, these formatting instructions are
>      at best ignored.
>     </para>
>  
> @@ -897,14 +897,14 @@ BETTER: unrecognized node type: 42
>     <simplesect>
>      <title>Function-Like Macros and Inline Functions</title>
>      <para>
> -     Both, macros with arguments and <literal>static inline</literal>
> -     functions, may be used. The latter are preferable if there are
> +     Both macros with arguments and <literal>static inline</literal>
> +     functions may be used. The latter are preferable if there are
>       multiple-evaluation hazards when written as a macro, as e.g. the
>       case with
>  <programlisting>
>  #define Max(x, y)       ((x) > (y) ? (x) : (y))
>  </programlisting>
> -     or when the macro would be very long. In other cases it's only
> +     or when the macro would be very long. In other cases, it's only
>       possible to use macros, or at least easier.  For example because
>       expressions of various types need to be passed to the macro.
>      </para>
> @@ -936,7 +936,7 @@ MemoryContextSwitchTo(MemoryContext context)
>     <simplesect>
>      <title>Writing Signal Handlers</title>
>      <para>
> -     To be suitable to run inside a signal handler code has to be
> +     To be suitable to run inside a signal handler, code has to be
>       written very carefully. The fundamental problem is that, unless
>       blocked, a signal handler can interrupt code at any time. If code
>       inside the signal handler uses the same state as code outside,
> @@ -945,7 +945,7 @@ MemoryContextSwitchTo(MemoryContext context)
>       interrupted code.
>      </para>
>      <para>
> -     Barring special arrangements code in signal handlers may only
> +     Barring special arrangements, code in signal handlers may only
>       call async-signal safe functions (as defined in POSIX) and access
>       variables of type <literal>volatile sig_atomic_t</literal>. A few
>       functions in <command>postgres</command> are also deemed signal safe; specifically,
> diff --git a/doc/src/sgml/wal.sgml b/doc/src/sgml/wal.sgml
> index 4eb8feb..30bde24 100644
> --- a/doc/src/sgml/wal.sgml
> +++ b/doc/src/sgml/wal.sgml
> @@ -326,7 +326,7 @@
>     before returning a success indication to the client.  The client is
>     therefore guaranteed that a transaction reported to be committed will
>     be preserved, even in the event of a server crash immediately after.
> -   However, for short transactions this delay is a major component of the
> +   However, for short transactions, this delay is a major component of the
>     total transaction time.  Selecting asynchronous commit mode means that
>     the server returns success as soon as the transaction is logically
>     completed, before the <acronym>WAL</acronym> records it generated have
> diff --git a/doc/src/sgml/xoper.sgml b/doc/src/sgml/xoper.sgml
> index 260e43c..55cd3b1 100644
> --- a/doc/src/sgml/xoper.sgml
> +++ b/doc/src/sgml/xoper.sgml
> @@ -375,7 +375,7 @@ table1.column1 OP table2.column2
>       Another example is that on machines that meet the <acronym>IEEE</acronym>
>       floating-point standard, negative zero and positive zero are different
>       values (different bit patterns) but they are defined to compare equal.
> -     If a float value might contain negative zero then extra steps are needed
> +     If a float value might contain negative zero, then extra steps are needed
>       to ensure it generates the same hash value as positive zero.
>      </para>
>  
> -- 
> 2.7.4
> 

> From 4d7e3b99d6e9e203e539cc8658554294121732b1 Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <pryzbyj@telsasoft.com>
> Date: Fri, 29 Mar 2019 19:40:49 -0500
> Subject: [PATCH v3 03/12] Consistent language: "must be superuser"
> 
> ---
>  src/backend/storage/ipc/signalfuncs.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/backend/storage/ipc/signalfuncs.c b/src/backend/storage/ipc/signalfuncs.c
> index 4bfbd57..1df5861 100644
> --- a/src/backend/storage/ipc/signalfuncs.c
> +++ b/src/backend/storage/ipc/signalfuncs.c
> @@ -115,7 +115,7 @@ pg_cancel_backend(PG_FUNCTION_ARGS)
>      if (r == SIGNAL_BACKEND_NOSUPERUSER)
>          ereport(ERROR,
>                  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> -                 (errmsg("must be a superuser to cancel superuser query"))));
> +                 (errmsg("must be superuser to cancel superuser query"))));
>  
>      if (r == SIGNAL_BACKEND_NOPERMISSION)
>          ereport(ERROR,
> @@ -139,12 +139,12 @@ pg_terminate_backend(PG_FUNCTION_ARGS)
>      if (r == SIGNAL_BACKEND_NOSUPERUSER)
>          ereport(ERROR,
>                  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> -                 (errmsg("must be a superuser to terminate superuser process"))));
> +                 (errmsg("must be superuser to terminate superuser process"))));
>
There's a number of
errhint("The owner of a subscription must be a superuser.")));
style messages, if you're trying for further consistency here...


... Out of steam. And, as it turns out, battery power.

Greetings,

Andres Freund

Hi,

On 2019/05/21 7:59, Andres Freund wrote:
> On 2019-05-20 13:20:01 -0500, Justin Pryzby wrote:
>> @@ -3052,7 +3052,7 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
>>         simplifies <command>ATTACH/DETACH PARTITION</command> operations:
>>         the partition dependencies need only be added or removed.
>>         Example: a child partitioned index is made partition-dependent
>> -       on both the partition table it is on and the parent partitioned
>> +       on both the table partition and the parent partitioned
>>         index, so that it goes away if either of those is dropped, but
>>         not otherwise.  The dependency on the parent index is primary,
>>         so that if the user tries to drop the child partitioned index,
> 
>> @@ -3115,7 +3115,7 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
>>     Note that it's quite possible for two objects to be linked by more than
>>     one <structname>pg_depend</structname> entry.  For example, a child
>>     partitioned index would have both a partition-type dependency on its
>> -   associated partition table, and an auto dependency on each column of
>> +   associated table partition, and an auto dependency on each column of
>>     that table that it indexes.  This sort of situation expresses the union
>>     of multiple dependency semantics.  A dependent object can be dropped
>>     without <literal>CASCADE</literal> if any of its dependencies satisfies
> 
> Hm, that's not an improvement from my POV? The version before isn't great either,
> but it seems to improve this'd require a somewhat bigger hammer.

The original "partition table" is meant as "table that is a partition", so
not wrong as such, though  I agree about the bigger hammer part,
especially seeing "a child partitioned index" in both the sentences that
Justin's patch touches, which should really be "an index partition".  So
the two sentences could be modified as follows, including Justin's change
for consistency of the use of "partition":

@@ -3051,13 +3051,12 @@ SCRAM-SHA-256$<replaceable><iteration
count></replaceable>:<replaceable>&l
        instead of, any dependencies the object would normally have.  This
        simplifies <command>ATTACH/DETACH PARTITION</command> operations:
        the partition dependencies need only be added or removed.
-       Example: a child partitioned index is made partition-dependent
-       on both the partition table it is on and the parent partitioned
-       index, so that it goes away if either of those is dropped, but
-       not otherwise.  The dependency on the parent index is primary,
-       so that if the user tries to drop the child partitioned index,
-       the error message will suggest dropping the parent index instead
-       (not the table).
+       Example: an index partition is made partition-dependent on both the
+       table partition it is on and the parent partitioned index, so that it
+       goes away if either of those is dropped, but not otherwise.
+       The dependency on the parent index is primary, so that if the user
+       tries to drop the index partition, the error will suggest dropping the
+       parent index instead (not the table).
       </para>
      </listitem>
     </varlistentry>
@@ -3113,10 +3112,10 @@ SCRAM-SHA-256$<replaceable><iteration
count></replaceable>:<replaceable>&l

   <para>
    Note that it's quite possible for two objects to be linked by more than
-   one <structname>pg_depend</structname> entry.  For example, a child
-   partitioned index would have both a partition-type dependency on its
-   associated partition table, and an auto dependency on each column of
-   that table that it indexes.  This sort of situation expresses the union
+   one <structname>pg_depend</structname> entry.  For example, an index
+   partition would have both a partition-type dependency on its assosiated
+   table partition, and an auto dependency on each column of that table that
+   it indexes.  This sort of situation expresses the union
    of multiple dependency semantics.  A dependent object can be dropped
    without <literal>CASCADE</literal> if any of its dependencies satisfies
    its condition for automatic dropping.  Conversely, all the

Thanks,
Amit

Hello,

I'm sorry if this is the wrong place for this or it's already been
covered (I did scan though this whole thread and a couple others), but
I noticed the docs at
https://www.postgresql.org/docs/devel/ddl-partitioning.html still say
you can't create a foreign key referencing a partitioned table, even
though the docs for
https://www.postgresql.org/docs/devel/sql-createtable.html have been
updated (compared to v11). My understanding is that foreign keys
*still* don't work as expected when pointing at traditional INHERITS
tables, but they *will* work with declaratively-partitioned tables. In
that case I suggest this change:

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index a0a7435a03..3b4f43bbad 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -3966,14 +3966,6 @@ ALTER TABLE measurement ATTACH PARTITION
measurement_y2008m02

-      <listitem>
-       <para>
-       While primary keys are supported on partitioned tables, foreign
-       keys referencing partitioned tables are not supported.  (Foreign key
-       references from a partitioned table to some other table are supported.)
-      </para>
-     </listitem>
-
     <listitem>
      <para>
        <literal>BEFORE ROW</literal> triggers, if necessary, must be defined
        on individual partitions, not the partitioned table.
       </para>
@@ -4366,6 +4358,14 @@ ALTER TABLE measurement_y2008m02 INHERIT measurement;
        </para>
       </listitem>

+     <listitem>
+      <para>
+       While primary keys are supported on inheritance-partitioned
tables, foreign
+       keys referencing these tables are not supported.  (Foreign key
+       references from an inheritance-partitioned table to some other
table are supported.)
+      </para>
+     </listitem>
+
       <listitem>
        <para>
         If you are using manual <command>VACUUM</command> or

(I've also attached it as a patch file.) In other words, we should
move this caveat from the section on declaratively-partitioned tables
to the section on inheritance-partitioned tables.

Sorry again if this is the wrong conversation for this!

Yours,
Paul

Hi Paul,

On 2019/05/21 13:25, Paul A Jungwirth wrote:
> I'm sorry if this is the wrong place for this or it's already been
> covered (I did scan though this whole thread and a couple others), but
> I noticed the docs at
> https://www.postgresql.org/docs/devel/ddl-partitioning.html still say
> you can't create a foreign key referencing a partitioned table, even
> though the docs for
> https://www.postgresql.org/docs/devel/sql-createtable.html have been
> updated (compared to v11). My understanding is that foreign keys
> *still* don't work as expected when pointing at traditional INHERITS
> tables, but they *will* work with declaratively-partitioned tables.

You're right.  I think it's simply an oversight of f56f8f8da6, which
missed updating ddl.sgml

> (I've also attached it as a patch file.) In other words, we should
> move this caveat from the section on declaratively-partitioned tables
> to the section on inheritance-partitioned tables.
> 
> Sorry again if this is the wrong conversation for this!

Thanks for the patch.  To avoid it getting lost in the discussions of this
thread, it might be better to post the patch to a separate thread.

Thanks,
Amit

On Mon, May 20, 2019 at 9:36 PM Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> Thanks for the patch.  To avoid it getting lost in the discussions of this
> thread, it might be better to post the patch to a separate thread.

Okay, I'll make a new thread and a new CF entry. Thanks!

On 2019/05/21 13:39, Paul A Jungwirth wrote:
> On Mon, May 20, 2019 at 9:36 PM Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> Thanks for the patch.  To avoid it getting lost in the discussions of this
>> thread, it might be better to post the patch to a separate thread.
> 
> Okay, I'll make a new thread and a new CF entry. Thanks!

This sounds more like an open item to me [1], not something that have to
be postponed until the next CF.

Thanks,
Amit

[1] https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items

On Mon, May 20, 2019 at 9:44 PM Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> This sounds more like an open item to me [1], not something that have to
> be postponed until the next CF.
>
> [1] https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items

Oh sorry, I already created the CF entry. Should I withdraw it? I'll
ask on -infra about getting editor permission for the wiki and add a
note there instead.

Paul

On 2019/05/21 13:47, Paul A Jungwirth wrote:
> On Mon, May 20, 2019 at 9:44 PM Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> This sounds more like an open item to me [1], not something that have to
>> be postponed until the next CF.
>>
>> [1] https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items
> 
> Oh sorry, I already created the CF entry. Should I withdraw it? I'll
> ask on -infra about getting editor permission for the wiki and add a
> note there instead.

You could link the CF entry from the wiki (the open item), but then it
will have to be closed when the open entry will be closed, so double work
for whoever does the cleaning up duties.  Maybe, it's better to withdraw
it now.

Thanks,
Amit

On Tue, May 21, 2019 at 01:55:46PM +0900, Amit Langote wrote:
> You could link the CF entry from the wiki (the open item), but then it
> will have to be closed when the open entry will be closed, so double work
> for whoever does the cleaning up duties.  Maybe, it's better to withdraw
> it now.

If you could clean up the CF entry, and keep only the open item in the
list, that would be nice.  Thanks.
--
Michael

On Mon, May 20, 2019 at 10:22 PM Michael Paquier <michael@paquier.xyz> wrote:
> If you could clean up the CF entry, and keep only the open item in the
> list, that would be nice.  Thanks.

I withdrew the CF entry; hopefully that is all that needs to be done,
but if I should do anything else let me know.

Thanks,
Paul

On 2019-May-20, Paul A Jungwirth wrote:

> On Mon, May 20, 2019 at 9:44 PM Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> > This sounds more like an open item to me [1], not something that have to
> > be postponed until the next CF.
> >
> > [1] https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items
> 
> Oh sorry, I already created the CF entry. Should I withdraw it? I'll
> ask on -infra about getting editor permission for the wiki and add a
> note there instead.

You didn't actually ask, but I did it anyway.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

This patch was applied as f73293aba4d4.  Thanks, Paul and Michael.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Jun 20, 2019 at 07:34:10PM -0400, Alvaro Herrera wrote:
> This patch was applied as f73293aba4d4.  Thanks, Paul and Michael.

Thanks for the thread update, Alvaro.  I completely forgot to mention
the commit on this thread.
--
Michael

I made bunch of changes based on Andres' review and I split some more
indisputable 1 line changes from the large commit, hoping it will be easier to
review both.  Several bits and pieces of the patch have been applied piecemeal,
but I was hoping to avoid continuing to do that.

I think at least these are also necessary.
v5-0002-Say-it-more-naturally.patch
                                             
 
v5-0010-spelling-and-typos.patch
                                             
 

I suggest to anyone reading to look at the large patch last, since its changes
are longer and less easy to read.  Many of the changes are intended to improve
the text rather than to fix a definite error.

Justin