clean up docs for v12

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 publication must 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&amp;diff=33390&amp;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