clean up docs for v12

classic Classic list List threaded Threaded
39 messages Options
12
Reply | Threaded
Open this post in threaded view
|

clean up docs for v12

Justin Pryzby
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

v1-0001-Clean-up-docs-for-log_statement_sample_rate.patch (4K) Download Attachment
v1-0002-review-docs-for-pg12dev.patch (28K) Download Attachment
v1-0003-JIT-typos.patch (4K) Download Attachment
v1-0004-Add-comma-for-readability.patch (8K) Download Attachment
v1-0005-Consistent-language-must-be-superuser.patch (1K) Download Attachment
v1-0006-Consistent-spelling-timestamp.patch (21K) Download Attachment
v1-0007-overridden-vs-overwritten.patch (3K) Download Attachment
v1-0008-Clean-up-language-in-cf984672.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

Justin Pryzby
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

v2-0001-Clean-up-docs-for-log_statement_sample_rate.patch (1K) Download Attachment
v2-0002-review-docs-for-pg12dev.patch (32K) Download Attachment
v2-0003-JIT-typos.patch (4K) Download Attachment
v2-0004-Add-comma-for-readability.patch (12K) Download Attachment
v2-0005-Consistent-language-must-be-superuser.patch (1K) Download Attachment
v2-0006-Consistent-spelling-timestamp.patch (21K) Download Attachment
v2-0007-overridden-vs-overwritten.patch (3K) Download Attachment
v2-0008-Clean-up-language-in-cf984672-Improve-behavior-of.patch (2K) Download Attachment
v2-0009-its-own-is-possessive.patch (1K) Download Attachment
v2-0010-Duplicate-word-in-comment.patch (868 bytes) Download Attachment
v2-0011-is-vs-are-plural.patch (2K) Download Attachment
v2-0012-docs-create-or-reindex-progress-of-ptn-tables.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

Justin Pryzby
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 publication must be a superuser.")));
src/backend/commands/subscriptioncmds.c:                                 errhint("The owner of a subscription must be a superuser.")));

Justin


Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

Michael Paquier-2
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

jit_comments.patch (1K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

Andres Freund
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


Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

Alvaro Herrera-9
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


Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

Tom Lane-2
Alvaro Herrera <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

Andres Freund
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


Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

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


Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

Tom Lane-2
In reply to this post by Andres Freund
Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

Tom Lane-2
In reply to this post by Andres Freund
Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

Andres Freund
Hi,

On 2019-04-22 13:27:17 -0400, Tom Lane wrote:

> Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

Tom Lane-2
Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

Andres Freund
Hi,

On 2019-04-22 14:17:48 -0400, Tom Lane wrote:

> Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

Andres Freund
In reply to this post by Tom Lane-2
Hi,

On 2019-04-22 13:18:18 -0400, Tom Lane wrote:

> Andres Freund <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

Michael Paquier-2
In reply to this post by Alvaro Herrera-9
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

Justin Pryzby
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


Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: clean up docs for v12

Justin Pryzby
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


12