pgsql: Prefetch data referenced by the WAL, take II.

Hi,

On 2022-04-07 07:44:20 +0000, Thomas Munro wrote:

Prefetch data referenced by the WAL, take II.

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2022-04-07%2008%3A17%3A27
thinks that xlogpretcher.h doesn't include enough...

I had added those checks to CI, but apparently somehow screwed that up.

Or maybe it's the scripts that are screwed up? Because I don't see any error
checking in headerscheck/cpluspluscheck. And indeed, locally they show the
errors, but exit with 0.

Greetings,

Andres Freund

On 4/7/22 04:36, Andres Freund wrote:

Hi,

On 2022-04-07 07:44:20 +0000, Thomas Munro wrote:

Prefetch data referenced by the WAL, take II.

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2022-04-07%2008%3A17%3A27
thinks that xlogpretcher.h doesn't include enough...

I had added those checks to CI, but apparently somehow screwed that up.

Or maybe it's the scripts that are screwed up? Because I don't see any error
checking in headerscheck/cpluspluscheck. And indeed, locally they show the
errors, but exit with 0.

Yeah, you can't rely on the exit status, if they produce output that
should be regarded as a failure (that's what crake does).

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:

On 4/7/22 04:36, Andres Freund wrote:

Or maybe it's the scripts that are screwed up? Because I don't see any error
checking in headerscheck/cpluspluscheck. And indeed, locally they show the
errors, but exit with 0.

Yeah, you can't rely on the exit status, if they produce output that
should be regarded as a failure (that's what crake does).

Yeah, those scripts were only intended to be run by hand. If someone
feels like upgrading them to also produce a useful exit status,
I'm for it.

regards, tom lane

On 2022-Apr-07, Thomas Munro wrote:

Prefetch data referenced by the WAL, take II.

I propose a small wording change in guc.c,

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 9fbbfb1be5..9803741708 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2840,7 +2840,7 @@ static struct config_int ConfigureNamesInt[] =
 	{
 		{"wal_decode_buffer_size", PGC_POSTMASTER, WAL_RECOVERY,
 			gettext_noop("Maximum buffer size for reading ahead in the WAL during recovery."),
-			gettext_noop("This controls the maximum distance we can read ahead in the WAL to prefetch referenced blocks."),
+			gettext_noop("This controls the maximum distance we can read ahead in the WAL to prefetch data blocks referenced therein."),
 			GUC_UNIT_BYTE
 		},
 		&wal_decode_buffer_size,

"referenced blocks" seems otherwise a bit unclear to me. Other wording
suggestions welcome. I first thought of "...to prefetch referenced data
blocks", which is probably OK too.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Siempre hay que alimentar a los dioses, aunque la tierra esté seca" (Orual)

On Sun, Sep 4, 2022 at 7:54 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2022-Apr-07, Thomas Munro wrote:
I propose a small wording change in guc.c,

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 9fbbfb1be5..9803741708 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2840,7 +2840,7 @@ static struct config_int ConfigureNamesInt[] =
{
{"wal_decode_buffer_size", PGC_POSTMASTER, WAL_RECOVERY,
gettext_noop("Maximum buffer size for reading ahead in the WAL during recovery."),
-                       gettext_noop("This controls the maximum distance we can read ahead in the WAL to prefetch referenced blocks."),
+                       gettext_noop("This controls the maximum distance we can read ahead in the WAL to prefetch data blocks referenced therein."),
GUC_UNIT_BYTE
},
&wal_decode_buffer_size,

"referenced blocks" seems otherwise a bit unclear to me. Other wording
suggestions welcome. I first thought of "...to prefetch referenced data
blocks", which is probably OK too.

I'd go for your second suggestion. 'therein' doesn't convey much (we
already said 'in the WAL', and therein is just a slightly formal and
somehow more Germanic way of saying 'in it' which is kinda
duplicative; maybe 'by it' is what we want but that's still kinda
implied already), whereas adding 'data' makes it slightly clearer that
it's blocks from relations and not, say, the WAL itself.

Also, hmm, it's not really the 'Maximum buffer size', it's the 'Buffer size'.

On 2022-Sep-13, Thomas Munro wrote:

I'd go for your second suggestion. 'therein' doesn't convey much (we
already said 'in the WAL', and therein is just a slightly formal and
somehow more Germanic way of saying 'in it' which is kinda
duplicative; maybe 'by it' is what we want but that's still kinda
implied already), whereas adding 'data' makes it slightly clearer that
it's blocks from relations and not, say, the WAL itself.

Also, hmm, it's not really the 'Maximum buffer size', it's the 'Buffer size'.

Sounds reasonable. Pushed these changes. Thank you!

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"La libertad es como el dinero; el que no la sabe emplear la pierde" (Alvarez)