Обсуждение: AdvanceXLInsertBuffer vs. WAL segment compressibility

Teaser: change made in 9.4 to simplify WAL segment compression
made it easier to compress a low-activity-period WAL segment
from 16 MB to about 27 kB ... but much harder to do better than
that, as I was previously doing (about two orders of magnitude
better).

At $work, we have a usually-low-activity PG database, so that almost
always the used fraction of each 16 MB WAL segment is far smaller
than 16 MB, and so it's a big win for archived-WAL storage space
if an archive-command can be written that compresses those files
effectively.

Our database was also running on a pre-9.4 version, and I'm
currently migrating to 9.5.3.  As I understand it, 9.4 was where
commit 9a20a9b landed, which changed what happens in the unwritten
'tail' of log segments.

In my understanding, before 9.4, the 'tail' of any log segment
on disk just wasn't written, and so (as segment recycling simply
involves renaming a file that held some earlier segment), the
remaining content was simply whatever had been there before
recycling. That was never a problem for recovery (which could
tell when it reached the end of real data), but was not well
compressible with a generic tool like gzip. Specialized tools
like pg_clearxlogtail existed, but had to know too much about
the internal format, and ended up unmaintained and therefore
difficult to trust.

The change in 9.4 included this, from the git comment:
 This has one user-visible change: switching to a new WAL segment with pg_switch_xlog() now fills the remaining unused
portionof the segment with zeros.
 

... thus making the segments easily compressible with bog standard
tools. So I can just point gzip at one of our WAL segments from a
light-activity period and it goes from 16 MB down to about 27 kB.
Nice, right?

But why does it break my earlier approach, which was doing about
two orders of magnitude better, getting low-activity WAL segments
down to 200 to 300 *bytes*? (Seriously: my last solid year of
archived WAL is contained in a 613 MB zip file.)

That approach was based on using rsync (also bog standard) to
tease apart the changed and unchanged bits of the newly-archived
segment and the last-seen content of the file with the same
i-number. You would expect that to work just as well when the
tail is always zeros as it was working before, right?

And what's breaking it now is the tiny bit of fine
print that's in the code comment for AdvanceXLInsertBuffer but
not in the git comment above:
 * ... Any new pages are initialized to zeros, with pages headers * initialized properly.

That innocuous "headers initialized" means that the tail of the
file is *almost* all zeros, but every 8 kB there is a tiny header,
and in each tiny header, there is *one byte* that differs from
its value in the pre-recycle content at the same i-node, because
that one byte in each header reflects the WAL segment number.

Before the 9.4 change, I see there were still headers there,
and they did contain a byte matching the segment number, but in
the unwritten portion of course it matched the pre-recycle
segment number, and rsync easily detected the whole unchanged
tail of the file. Now there is one changed byte every 8 kB,
and the rsync output, instead of being 100x better than vanilla
gzip, is about 3x worse.

Taking a step back, isn't overwriting the whole unused tail of
each 16 MB segment really just an I/O intensive way of communicating
to the archive-command where the valid data ends?  Could that not
be done more efficiently by adding another code, say %e, in
archive-command, that would be substituted by the offset of the
end of the XLOG_SWITCH record? That way, however archive-command
is implemented, it could simply know how much of the file to
copy.

Would it then be possible to go back to the old behavior (or make
it selectable) of not overwriting the full 16 MB every time?
Or did the 9.4 changes also change enough other logic that stuff
would now break if that isn't done?

-Chap

On Sat, Jul 23, 2016 at 3:32 AM, Chapman Flack <chap@anastigmatix.net> wrote:
>
> Would it then be possible to go back to the old behavior (or make
> it selectable) of not overwriting the full 16 MB every time?
>

I don't see going back to old behaviour is an improvement, because as
as you pointed out above that it helps to improve the compression
ratio of WAL files for tools like gzip and it doesn't seem advisable
to loose that capability.  I think providing an option to select that
behaviour could be one choice, but use case seems narrow to me
considering there are tools (pglesslog) to clear the tail.  Do you
find any problems with that tool which makes you think that it is not
reliable?

> Or did the 9.4 changes also change enough other logic that stuff
> would now break if that isn't done?
>

The changes related to the same seems to be isolated (mainly in
CopyXLogRecordToWAL()) and doesn't look to impact other parts of
system, although some more analysis is needed to confirm the same, but
I think the point to make it optional doesn't seem convincing to me.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 07/23/2016 08:25 AM, Amit Kapila wrote:
> On Sat, Jul 23, 2016 at 3:32 AM, Chapman Flack <chap@anastigmatix.net> wrote:
>>
>> Would it then be possible to go back to the old behavior (or make
>> it selectable) of not overwriting the full 16 MB every time?
>>
> 
> I don't see going back to old behaviour is an improvement, because as
> as you pointed out above that it helps to improve the compression
> ratio of WAL files for tools like gzip and it doesn't seem advisable
> to loose that capability.  I think providing an option to select that
> behaviour could be one choice, but use case seems narrow to me
> considering there are tools (pglesslog) to clear the tail.  Do you
> find any problems with that tool which makes you think that it is not
> reliable?

It was a year or so ago when I was surveying tools that attempted
to do that. I had found pg_clearxlogtail, and I'm sure I also found
pglesslog / pg_compresslog ... my notes from then simply refer to
"contrib efforts like pg_clearxlogtail" and observed either a dearth
of recent search results for them, or a predominance of results
of the form "how do I get this to compile for PG x.x?"

pg_compresslog is mentioned in a section, Compressed Archive Logs,
of the PG 9.1 manual:
https://www.postgresql.org/docs/9.1/static/continuous-archiving.html#COMPRESSED-ARCHIVE-LOGS

That section is absent in the docs any version > 9.1.

The impression that leaves is of tools that relied too heavily
on internal format knowledge to be viable outside of core, which
have had at least periods of incompatibility with newer PG versions,
and whose current status, if indeed any are current, isn't easy
to find out.

It seems a bit risky (to me, anyway) to base a backup strategy
on having a tool in the pipeline that depends so heavily on
internal format knowledge, can become uncompilable between PG
releases, and isn't part of core and officially supported.

And that, I assume, was also the motivation to put the zeroing
in AdvanceXLInsertBuffer, eliminating the need for one narrow,
specialized tool like pg{_clear,_compress,less}log{,tail}, so
the job can be done with ubiquitous, bog standard (and therefore
*very* exhaustively tested) tools like gzip.

So it's just kind of unfortunate that there used to be a *further*
factor of 100 (nothing to sneeze at) possible using rsync
(another non-PG-specific, ubiquitous, exhaustively tested tool)
but a trivial feature of the new behavior has broken that.

Factors of 100 are enough to change the sorts of things you think
about, like possibly retaining years-long unbroken histories of
transactions in WAL.

What would happen if the overwriting of the log tail were really
done with just zeros, as the git comment implied, rather than zeros
with initialized headers? Could the log-reading code handle that
gracefully? That would support all forms of non-PG-specific,
ubiquitous tools used for compression; it would not break the rsync
approach.

Even so, it still seems to me that a cheaper solution is a %e
substitution in archive_command: just *tell* the command where
the valid bytes end. Accomplishes the same thing as ~ 16 MB
of otherwise-unnecessary I/O at the time of archiving each
lightly-used segment.

Then the actual zeroing could be suppressed to save I/O, maybe
with a GUC variable, or maybe just when archive_command is seen
to contain a %e. Commands that don't have a %e continue to work
and compress effectively because of the zeroing.

-Chap

On Mon, Jul 25, 2016 at 11:21 PM, Chapman Flack <chap@anastigmatix.net> wrote:
> The impression that leaves is of tools that relied too heavily
> on internal format knowledge to be viable outside of core, which
> have had at least periods of incompatibility with newer PG versions,
> and whose current status, if indeed any are current, isn't easy
> to find out.

WAL format has gone through a lot of changes in 9.4 as well. 9.3 has
as well introduced xlogreader.c which is what *any* client trying to
read WAL into an understandable format should use.

> And that, I assume, was also the motivation to put the zeroing
> in AdvanceXLInsertBuffer, eliminating the need for one narrow,
> specialized tool like pg{_clear,_compress,less}log{,tail}, so
> the job can be done with ubiquitous, bog standard (and therefore
> *very* exhaustively tested) tools like gzip.

Exactly, and honestly this has been a huge win to make such segments
more compressible.

> Even so, it still seems to me that a cheaper solution is a %e
> substitution in archive_command: just *tell* the command where
> the valid bytes end. Accomplishes the same thing as ~ 16 MB
> of otherwise-unnecessary I/O at the time of archiving each
> lightly-used segment.
>
> Then the actual zeroing could be suppressed to save I/O, maybe
> with a GUC variable, or maybe just when archive_command is seen
> to contain a %e. Commands that don't have a %e continue to work
> and compress effectively because of the zeroing.

This is over-complicating things for little gain. The new behavior of
filling in with zeros the tail of a segment makes things far better
when using gzip in archive_command.
-- 
Michael

On 07/25/16 22:09, Michael Paquier wrote:

> This is over-complicating things for little gain. The new behavior of
> filling in with zeros the tail of a segment makes things far better
> when using gzip in archive_command.

Then how about filling with actual zeros, instead of with mostly-zeros
as is currently done?  That would work just as well for gzip, and would
not sacrifice the ability to do 100x better than gzip.

-Chap

On Tue, Jul 26, 2016 at 9:02 AM, Chapman Flack <chap@anastigmatix.net> wrote:
> On 07/25/16 22:09, Michael Paquier wrote:
>
>> This is over-complicating things for little gain. The new behavior of
>> filling in with zeros the tail of a segment makes things far better
>> when using gzip in archive_command.
>
> Then how about filling with actual zeros, instead of with mostly-zeros
> as is currently done?  That would work just as well for gzip, and would
> not sacrifice the ability to do 100x better than gzip.
>

There is a flag XLP_BKP_REMOVABLE for the purpose of ignoring empty
blocks, any external tool/'s relying on it can break, if make
everything zero. Now, it might be possible to selectively initialize
the fields that doesn't harm the methodology for archive you are using
considering there is no other impact of same in code. However, it
doesn't look to be a neat way to implement the requirement.  In
general, if you have a very low WAL activity, then the final size of
compressed WAL shouldn't be much even if you use gzip.  It seems your
main concern is that the size of WAL even though not high, but it is
more than what you were earlier getting for your archive data.  I
think that is a legitimate concern, but I don't see much options apart
for providing some selective way to not initialize everything in WAL
page headers or have some tool like pg_lesslog that can be shipped as
part of contrib module.  I am not sure whether your use case is
important enough to proceed with one of those options or may be
consider some another approach.  Does any body else see the use case
reported by Chapman important enough that we try to have some solution
for it in-core?


-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 07/26/2016 08:48 AM, Amit Kapila wrote:

> general, if you have a very low WAL activity, then the final size of
> compressed WAL shouldn't be much even if you use gzip.  It seems your

9.5 pg_xlog, low activity test cluster (segment switches forced
only by checkpoint timeouts), compressed with gzip -9:

$ for i in 0*; do echo -n "$i  " && gzip -9 <$i | wc -c; done
000000010000000100000042  27072
000000010000000100000043  27075
000000010000000100000044  27077
000000010000000100000045  27073
000000010000000100000046  27075

Log from live pre-9.4 cluster, low-activity time of day, delta
compression using rsync:

2016-07-26 03:54:02 EDT (walship) INFO: using 2.39s user, 0.4s system,
9.11s on
wall:
231 byte 000000010000004600000029_000000010000004600000021_fwd
...
2016-07-26 04:54:01 EDT (walship) INFO: using 2.47s user, 0.4s system,
8.43s on
wall:
232 byte 00000001000000460000002A_000000010000004600000022_fwd
...
2016-07-26 05:54:02 EDT (walship) INFO: using 2.56s user, 0.29s system,
9.44s onwall:
230 byte 00000001000000460000002B_000000010000004600000023_fwd

So when I say "factor of 100", I'm understating slightly. (Those
timings, for the curious, include sending a copy offsite via ssh.)

> everything zero. Now, it might be possible to selectively initialize
> the fields that doesn't harm the methodology for archive you are using
> considering there is no other impact of same in code. However, it

Indeed, it is only the one header field that duplicates the low-
order part of the (hex) file name that breaks delta compression,
because it has always been incremented even when nothing else is
different, and it's scattered 2000 times through the file.
Would it break anything for *that* to be zero in dummy blocks?

-Chap

On Fri, Jul 22, 2016 at 6:02 PM, Chapman Flack <chap@anastigmatix.net> wrote:
> At $work, we have a usually-low-activity PG database, so that almost
> always the used fraction of each 16 MB WAL segment is far smaller
> than 16 MB, and so it's a big win for archived-WAL storage space
> if an archive-command can be written that compresses those files
> effectively.

I'm kind of curious WHY you are using archiving and forcing regular
segment switches rather than just using streaming replication.
Pre-9.0, use of archive_timeout was routine, since there was no other
way to ensure that the data ended up someplace other than your primary
with reasonable regularity.  But, AFAIK, streaming replication
essentially obsoleted that use case.  You can just dribble the
individual bytes over the wire a few at a time to the standby or, with
pg_receivexlog, to an archive location.  If it takes 6 months to fill
up a WAL segment, you don't care: you'll always have all the bytes
that were generated more than a fraction of a second before the master
melted into a heap of slag.

I'm not saying you don't have a good reason for doing what you are
doing, just that I cannot think of one.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 07/26/2016 04:21 PM, Robert Haas wrote:

> I'm kind of curious WHY you are using archiving and forcing regular
> segment switches rather than just using streaming replication.
> ... AFAIK, streaming replication
> essentially obsoleted that use case.  You can just dribble the
> individual bytes over the wire a few at a time to the standby or, with
> pg_receivexlog, to an archive location.  If it takes 6 months to fill
> up a WAL segment, you don't care: you'll always have all the bytes

Part of it is just the legacy situation: at the moment, the offsite
host is of a different architecture and hasn't got PostgreSQL
installed (but it's easily ssh'd to for delivering compressed WAL
segments).  We could change that down the road, and pg_receivexlog
would work for getting the bytes over there.

My focus for the moment was just on migrating a cluster to 9.5
without changing the surrounding arrangements all at once.
Seeing how much worse our compression ratio will be, though,
maybe I need to revisit that plan.

Even so, I'd be curious whether it would break anything to have
xlp_pageaddr simply set to InvalidXLogRecPtr in the dummy zero
pages written to fill out a segment. At least until it's felt
that archive_timeout has been so decidedly obsoleted by streaming
replication that it is removed, and the log-tail zeroing code
with it.

That at least would eliminate the risk of anyone else repeating
my astonishment. :)  I had read that 9.4 added built-in log-zeroing
code, and my first reaction was "cool! that may make the compression
technique we're using unnecessary, but certainly can't make it worse"
only to discover that it did, by ~ 300x, becoming now 3x *worse* than
plain gzip, which itself is ~ 100x worse than what we had.

-Chap

On Tue, Jul 26, 2016 at 9:48 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> Does any body else see the use case
> reported by Chapman important enough that we try to have some solution
> for it in-core?

The lack of updates in the pg_lesslog project is a sign that it is not
that much used. I does not seem a good idea to bring in-core a tool
not used that much by users.
-- 
Michael

On 07/26/16 20:01, Michael Paquier wrote:
> On Tue, Jul 26, 2016 at 9:48 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> Does any body else see the use case
>> reported by Chapman important enough that we try to have some solution
>> for it in-core?
> 
> The lack of updates in the pg_lesslog project is a sign that it is not
> that much used. I does not seem a good idea to bring in-core a tool
> not used that much by users.

Effectively, it already was brought in-core in commit 9a20a9b.
Only, that change had an unintended consequence that *limits*
compressibility - and it would not have that consequence, if
it were changed to simply set xlp_pageaddr to InvalidXLogRecPtr
in the dummy zero pages written to fill out a segment.

-Chap

On Tue, Jul 26, 2016 at 05:42:43PM -0400, Chapman Flack wrote:
> Even so, I'd be curious whether it would break anything to have
> xlp_pageaddr simply set to InvalidXLogRecPtr in the dummy zero
> pages written to fill out a segment. At least until it's felt
> that archive_timeout has been so decidedly obsoleted by streaming
> replication that it is removed, and the log-tail zeroing code
> with it.
> 
> That at least would eliminate the risk of anyone else repeating
> my astonishment. :)  I had read that 9.4 added built-in log-zeroing
> code, and my first reaction was "cool! that may make the compression
> technique we're using unnecessary, but certainly can't make it worse"
> only to discover that it did, by ~ 300x, becoming now 3x *worse* than
> plain gzip, which itself is ~ 100x worse than what we had.

My guess is that the bytes are there to detect problems where a 512-byte
disk sector is zeroed by a disk failure.  I don't see use changing that
for the use-case you have described.

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+                     Ancient Roman grave inscription +

On 08/02/2016 02:33 PM, Bruce Momjian wrote:

> My guess is that the bytes are there to detect problems where
> a 512-byte disk sector is zeroed by a disk failure.

Does that seem plausible? (a) there is only one such header for
every 16 512-byte disk sectors, so it only affords a 6% chance of
detecting a zeroed sector, and (b) the header contains other
non-zero values in fields other than xlp_pageaddr, so the use
of a fixed value for _that field_ in zeroed tail blocks would
not prevent (or even reduce the 6% probability of) detecting
a sector zeroed by a defect.

-Chap

On Tue, Aug 2, 2016 at 2:33 PM, Bruce Momjian <bruce@momjian.us> wrote:
> On Tue, Jul 26, 2016 at 05:42:43PM -0400, Chapman Flack wrote:
>> Even so, I'd be curious whether it would break anything to have
>> xlp_pageaddr simply set to InvalidXLogRecPtr in the dummy zero
>> pages written to fill out a segment. At least until it's felt
>> that archive_timeout has been so decidedly obsoleted by streaming
>> replication that it is removed, and the log-tail zeroing code
>> with it.
>>
>> That at least would eliminate the risk of anyone else repeating
>> my astonishment. :)  I had read that 9.4 added built-in log-zeroing
>> code, and my first reaction was "cool! that may make the compression
>> technique we're using unnecessary, but certainly can't make it worse"
>> only to discover that it did, by ~ 300x, becoming now 3x *worse* than
>> plain gzip, which itself is ~ 100x worse than what we had.
>
> My guess is that the bytes are there to detect problems where a 512-byte
> disk sector is zeroed by a disk failure.  I don't see use changing that
> for the use-case you have described.

Is there actually any code that makes such a check?

I'm inclined to doubt that was the motivation, though admittedly we're
both speculating about the contents of Heikki's brain, a tricky
proposition on a good day.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

(I'm cleaning up my inbox, hence the delayed reply)

On 08/02/2016 10:51 PM, Robert Haas wrote:
> On Tue, Aug 2, 2016 at 2:33 PM, Bruce Momjian <bruce@momjian.us> wrote:
>> On Tue, Jul 26, 2016 at 05:42:43PM -0400, Chapman Flack wrote:
>>> Even so, I'd be curious whether it would break anything to have
>>> xlp_pageaddr simply set to InvalidXLogRecPtr in the dummy zero
>>> pages written to fill out a segment. At least until it's felt
>>> that archive_timeout has been so decidedly obsoleted by streaming
>>> replication that it is removed, and the log-tail zeroing code
>>> with it.
>>>
>>> That at least would eliminate the risk of anyone else repeating
>>> my astonishment. :)  I had read that 9.4 added built-in log-zeroing
>>> code, and my first reaction was "cool! that may make the compression
>>> technique we're using unnecessary, but certainly can't make it worse"
>>> only to discover that it did, by ~ 300x, becoming now 3x *worse* than
>>> plain gzip, which itself is ~ 100x worse than what we had.
>>
>> My guess is that the bytes are there to detect problems where a 512-byte
>> disk sector is zeroed by a disk failure.  I don't see use changing that
>> for the use-case you have described.
>
> Is there actually any code that makes such a check?
>
> I'm inclined to doubt that was the motivation, though admittedly we're
> both speculating about the contents of Heikki's brain, a tricky
> proposition on a good day.

Given that we used to just leave them as garbage, it seems pretty safe 
to zero them out now.

It's kind of nice that all the XLOG pages have valid page headers. One 
way to think of the WAL switch record is that it's a very large WAL 
record that just happens to consume the rest of the WAL segment. Except 
that it's not actually represented like that; the xl_tot_len field of an 
XLOG switch record does not include the zeroed out portion. Instead, 
there's special handling in the reader code, that skips to the end of 
the segment when it sees a switch record. So that point is moot.

When I wrote that code, I don't remember if I realized that we're 
initializing the page headers, or if I thought that it's good enough 
even if we do. I guess I didn't realize it, because a comment would've 
been in order if it was intentional.

So +1 on fixing that, a patch would be welcome. In the meanwhile, have 
you tried using a different compression program? Something else than 
gzip might do a better job at the almost zero pages.

- Heikki

On 06/21/17 04:51, Heikki Linnakangas wrote:
> (I'm cleaning up my inbox, hence the delayed reply)

I had almost completely forgotten ever bringing it up. :)

> When I wrote that code, I don't remember if I realized that we're
> initializing the page headers, or if I thought that it's good enough even if
> we do. I guess I didn't realize it, because a comment would've been in order
> if it was intentional.
> 
> So +1 on fixing that, a patch would be welcome.

Ok, that sounds like something I could take a whack at. Overall, xlog.c
is a bit daunting, but this particular detail seems fairly approachable.

> In the meanwhile, have you
> tried using a different compression program? Something else than gzip might
> do a better job at the almost zero pages.

Well, gzip was doing pretty well; it could get a 16 MB segment file down
to under 27 kB, or less than 14 bytes for each of 2000 pages, when a page
header is what, 20 bytes, it looks like? I'm not sure how much better
I'd expect a (non-custom) compression scheme to do. The real difference
comes between compressing (even well) a large unchanged area, versus being
able to recognize (again with a non-custom tool) that the whole area is
unchanged.

-Chap

I notice CopyXLogRecordToWAL contains this loop (in the case where
the record being copied is a switch):

while (CurrPos < EndPos)
{   /* initialize the next page (if not initialized already) */   WALInsertLockUpdateInsertingAt(CurrPos);
AdvanceXLInsertBuffer(CurrPos,false);   CurrPos += XLOG_BLCKSZ;
 
}

in which it calls, one page at a time, AdvanceXLInsertBuffer, which contains
its own loop able to do a sequence of pages. A comment explains why:

/** We do this one page at a time, to make sure we don't deadlock* against ourselves if wal_buffers < XLOG_SEG_SIZE.*/

I want to make sure I understand what the deadlock potential is
in this case. AdvanceXLInsertBuffer will call WaitXLogInsertionsToFinish
before writing any dirty buffer, and we do hold insertion slot locks
(all of 'em, in the case of a log switch, because that makes
XlogInsertRecord call WALInsertLockAcquireExclusive instead of just
WALInsertLockAcquire for other record types).

Does not the fact we hold all the insertion slots exclude the possibility
that any dirty buffer (preceding the one we're touching) needs to be checked
for in-flight insertions?

I've been thinking along the lines of another parameter to
AdvanceXLInsertBuffer to indicate when the caller is exactly this loop
filling out the tail after a log switch (originally, to avoid filling
in page headers). It now seems to me that, if AdvanceXLInsertBuffer
has that information, it could also be safe for it to skip the
WaitXLogInsertionsToFinish in that case. Would that eliminate the
deadlock potential, and allow the loop in CopyXLogRecordToWAL to be
replaced with a single call to AdvanceXLInsertBuffer and a single
WALInsertLockUpdateInsertingAt ?

Or have I overlooked some other subtlety?

-Chap

On 06/25/17 21:20, Chapman Flack wrote:
> I want to make sure I understand what the deadlock potential is
> in this case. AdvanceXLInsertBuffer will call WaitXLogInsertionsToFinish
> ...
> Does not the fact we hold all the insertion slots exclude the possibility
> that any dirty buffer (preceding the one we're touching) needs to be checked
> for in-flight insertions?     [in the filling-out-the-log-tail case only]

Anyone?

Or have I not even achieved 'wrong' yet?

-Chap

On 06/26/2017 04:20 AM, Chapman Flack wrote:
> I notice CopyXLogRecordToWAL contains this loop (in the case where
> the record being copied is a switch):
>
> while (CurrPos < EndPos)
> {
>     /* initialize the next page (if not initialized already) */
>     WALInsertLockUpdateInsertingAt(CurrPos);
>     AdvanceXLInsertBuffer(CurrPos, false);
>     CurrPos += XLOG_BLCKSZ;
> }
>
> in which it calls, one page at a time, AdvanceXLInsertBuffer, which contains
> its own loop able to do a sequence of pages. A comment explains why:
>
> /*
>  * We do this one page at a time, to make sure we don't deadlock
>  * against ourselves if wal_buffers < XLOG_SEG_SIZE.
>  */
>
> I want to make sure I understand what the deadlock potential is
> in this case. AdvanceXLInsertBuffer will call WaitXLogInsertionsToFinish
> before writing any dirty buffer, and we do hold insertion slot locks
> (all of 'em, in the case of a log switch, because that makes
> XlogInsertRecord call WALInsertLockAcquireExclusive instead of just
> WALInsertLockAcquire for other record types).
>
> Does not the fact we hold all the insertion slots exclude the possibility
> that any dirty buffer (preceding the one we're touching) needs to be checked
> for in-flight insertions?

Hmm. That's not the problem, though. Imagine that instead of the loop 
above, you do just:

WALInsertLockUpdateInsertingAt(CurrPos);
AdvanceXLInsertBuffer(EndPos, false);

AdvanceXLInsertBuffer() will call XLogWrite(), to flush out any pages 
before EndPos, to make room in the wal_buffers for the new pages. Before 
doing that, it will call WaitXLogInsertionsToFinish() to wait for any 
insertions to those pages to be completed. But the backend itself is 
advertising the insertion position CurrPos, and it will therefore wait 
for itself, forever.

> I've been thinking along the lines of another parameter to
> AdvanceXLInsertBuffer to indicate when the caller is exactly this loop
> filling out the tail after a log switch (originally, to avoid filling
> in page headers). It now seems to me that, if AdvanceXLInsertBuffer
> has that information, it could also be safe for it to skip the
> WaitXLogInsertionsToFinish in that case. Would that eliminate the
> deadlock potential, and allow the loop in CopyXLogRecordToWAL to be
> replaced with a single call to AdvanceXLInsertBuffer and a single
> WALInsertLockUpdateInsertingAt ?
>
> Or have I overlooked some other subtlety?

The most straightforward solution would be to just clear each page with 
memset() in the loop. It's a bit wasteful to clear the page again, just 
after AdvanceXLInsertBuffer() has initialized it, but this isn't 
performance-critical.

- Heikki

On 07/03/2017 09:39 AM, Heikki Linnakangas wrote:

> The most straightforward solution would be to just clear each page with
> memset() in the loop. It's a bit wasteful to clear the page again, just
> after AdvanceXLInsertBuffer() has initialized it, but this isn't
> performance-critical.

An in that straightforward approach, I imagine it would suffice to
memset just the length of a (short) page header; the page content
is already zeroed, and there isn't going to be a switch at the very
start of a segment, so a long header won't be encountered ... will it?

-Chap

On 07/03/2017 09:39 AM, Heikki Linnakangas wrote:

> Hmm. That's not the problem, though. Imagine that instead of the loop
> above, you do just:
> 
> WALInsertLockUpdateInsertingAt(CurrPos);
> AdvanceXLInsertBuffer(EndPos, false);
> 
> AdvanceXLInsertBuffer() will call XLogWrite(), to flush out any pages
> before EndPos, to make room in the wal_buffers for the new pages. Before
> doing that, it will call WaitXLogInsertionsToFinish()

Although it's moot in the straightforward approach of re-zeroing in
the loop, it would still help my understanding of the system to know
if there is some subtlety that would have broken what I proposed
earlier, which was an extra flag to AdvanceXLInsertBuffer() that
would tell it not only to skip initializing headers, but also to
skip the WaitXLogInsertionsToFinish() check ... because I have
the entire region reserved and I hold all the writer slots
at that moment, it seems safe to assure AdvanceXLInsertBuffer()
that there are no outstanding writes to wait for.

I suppose it's true there's not much performance to gain; it would
save a few pairs of lock operations per empty page to be written,
but then, the more empty pages there are at the time of a log switch,
the less busy the system is, so the less it matters.

-Chap

On 07/03/2017 06:30 PM, Chapman Flack wrote:
> On 07/03/2017 09:39 AM, Heikki Linnakangas wrote:
>
>> Hmm. That's not the problem, though. Imagine that instead of the loop
>> above, you do just:
>>
>> WALInsertLockUpdateInsertingAt(CurrPos);
>> AdvanceXLInsertBuffer(EndPos, false);
>>
>> AdvanceXLInsertBuffer() will call XLogWrite(), to flush out any pages
>> before EndPos, to make room in the wal_buffers for the new pages. Before
>> doing that, it will call WaitXLogInsertionsToFinish()
>
> Although it's moot in the straightforward approach of re-zeroing in
> the loop, it would still help my understanding of the system to know
> if there is some subtlety that would have broken what I proposed
> earlier, which was an extra flag to AdvanceXLInsertBuffer() that
> would tell it not only to skip initializing headers, but also to
> skip the WaitXLogInsertionsToFinish() check ... because I have
> the entire region reserved and I hold all the writer slots
> at that moment, it seems safe to assure AdvanceXLInsertBuffer()
> that there are no outstanding writes to wait for.

Yeah, I suppose that would work, too.

- Heikki

On Fri, Jun 23, 2017 at 6:08 AM, Chapman Flack <chap@anastigmatix.net> wrote:
> Well, gzip was doing pretty well; it could get a 16 MB segment file down
> to under 27 kB, or less than 14 bytes for each of 2000 pages, when a page
> header is what, 20 bytes, it looks like? I'm not sure how much better
> I'd expect a (non-custom) compression scheme to do. The real difference
> comes between compressing (even well) a large unchanged area, versus being
> able to recognize (again with a non-custom tool) that the whole area is
> unchanged.

Have you tried as well lz4 for your cases? It performs faster than
gzip at minimum compression and compresses less, but I am really
wondering if for almost zero pages it performs actually better.
-- 
Michael

On Thu, Jul 6, 2017 at 3:48 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
> On 07/03/2017 06:30 PM, Chapman Flack wrote:
>> Although it's moot in the straightforward approach of re-zeroing in
>> the loop, it would still help my understanding of the system to know
>> if there is some subtlety that would have broken what I proposed
>> earlier, which was an extra flag to AdvanceXLInsertBuffer() that
>> would tell it not only to skip initializing headers, but also to
>> skip the WaitXLogInsertionsToFinish() check ... because I have
>> the entire region reserved and I hold all the writer slots
>> at that moment, it seems safe to assure AdvanceXLInsertBuffer()
>> that there are no outstanding writes to wait for.
>
> Yeah, I suppose that would work, too.

FWIW, I would rather see any optimization done in
AdvanceXLInsertBuffer() instead of seeing a second memset re-zeroing
the WAL page header after its data has been initialized by
AdvanceXLInsertBuffer() once. That's too late for 10, but you still
have time for a patch to be integrated in 11.
-- 
Michael

On 07/17/17 11:29, Michael Paquier wrote:

> FWIW, I would rather see any optimization done in
> AdvanceXLInsertBuffer() instead of seeing a second memset re-zeroing
> the WAL page header after its data has been initialized by
> AdvanceXLInsertBuffer() once.

Is that an aesthetic 'rather', or is there a technical advantage you
have in mind?

I also began by looking at how to stop AdvanceXLInsertBuffer()
initializing headers and taking locks when neither is needed.
But Heikki's just-rezero-them suggestion has a definite simplicity
advantage. It can be implemented entirely with a tight group of
lines added to CopyXLogRecordToWAL, as opposed to modifying
AdvanceXLInsertBuffer in a few distinct places, adding a parameter,
and changing its call sites.

There's a technical appeal to making the changes in AdvanceXLInsertBuffer
(who wants to do unnecessary initialization and locking?), but the amount
of unnecessary work that can be avoided is proportional to the number of
unused pages at switch time, meaning it is largest when the system
is least busy, and may be of little practical concern.

Moreover, optimizing AdvanceXLInsertBuffer would reveal one more
complication: some of the empty pages about to be written out may
have been initialized opportunistically in earlier calls to
AdvanceXLInsertBuffer, so those already have populated headers, and
would need rezeroing anyway. And not necessarily just an insignificant
few of them: if XLOGChooseNumBuffers chose the maximum, it could even
be all of them.

That might also be handled by yet another conditional within
AdvanceXLInsertBuffer. But with all of that in view, maybe it is
just simpler to have one loop in CopyXLogRecordToWAL simply zero them all,
and leave AdvanceXLInsertBuffer alone, so no complexity is added when it
is called from other sites that are arguably hotter.

Zeroing SizeOfXLogShortPHD bytes doesn't cost a whole lot.

-Chap

On 07/17/17 11:29, Michael Paquier wrote:
> On Thu, Jul 6, 2017 at 3:48 PM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> On 07/03/2017 06:30 PM, Chapman Flack wrote:
>>> Although it's moot in the straightforward approach of re-zeroing in
>>> the loop, it would still help my understanding of the system to know
>>> if there is some subtlety that would have broken what I proposed
>>> earlier, which was an extra flag to AdvanceXLInsertBuffer() that
>>> ...
>>
>> Yeah, I suppose that would work, too.
> 
> FWIW, I would rather see any optimization done in
> AdvanceXLInsertBuffer() instead of seeing a second memset re-zeroing
> the WAL page header after its data has been initialized by
> AdvanceXLInsertBuffer() once.

Here is a patch implementing the simpler approach Heikki suggested
(though my original leaning had been to wrench on AdvanceXLInsertBuffer
as Michael suggests). The sheer simplicity of the smaller change
eventually won me over, unless there's a strong objection.

As noted before, the cost of the extra small MemSet is proportional
to the number of unused pages in the segment, and that is an indication
of how busy the system *isn't*. I don't have a time benchmark of the
patch's impact; if I should, what would be a good methodology?

Before the change, what some common compression tools can achieve on
a mostly empty (select pg_switch_wal()) segment on my hardware are:

gzip  27052 in 0.145s
xz     5852 in 0.678s
lzip   5747 in 1.254s
bzip2  1445 in 0.261s

bzip2 is already the clear leader (I don't have lz4 handy to include in
the comparison) at around 1/20th size gzip can achieve, and that's before
this patch. After:

gzip 16393 in 0.143s
xz    2640 in 0.520s
lzip  2535 in 1.198s
bzip2  147 in 0.238s

The patch gives gzip almost an extra factor of two, and about the same
for xz and lzip, and bzip2 gains nearly another order of magnitude.

I considered adding a regression test for unfilled-segment compressibility,
but wasn't sure where it would most appropriately go. I assume a TAP test
would be the way to do it.

-Chap

> On 25 Feb 2018, at 18:22, Chapman Flack <chap@anastigmatix.net> wrote:

> Here is a patch implementing the simpler approach Heikki suggested
> (though my original leaning had been to wrench on AdvanceXLInsertBuffer
> as Michael suggests). The sheer simplicity of the smaller change
> eventually won me over, unless there's a strong objection.

I had a look at this patch today.  The patch applies on current HEAD, and has
ample documentation in the included comment.  All existing tests pass, but
there are no new tests added (more on that below).  While somewhat outside my
wheelhouse, the implementation is the simple solution with no apparent traps
that I can think of.

Regarding the problem statement of the patch.  The headers on the zeroed
segments are likely an un-intended side effect, and serve no real purpose.
While they aren’t a problem to postgres, they do reduce the compressability of
the resulting files as evidenced by the patch author.

> As noted before, the cost of the extra small MemSet is proportional
> to the number of unused pages in the segment, and that is an indication
> of how busy the system *isn't*. I don't have a time benchmark of the
> patch's impact; if I should, what would be a good methodology?

This codepath doesn’t seem performance critical enough to warrant holding off
the patch awaiting a benchmark IMO.

> I considered adding a regression test for unfilled-segment compressibility,
> but wasn't sure where it would most appropriately go. I assume a TAP test
> would be the way to do it.

Adding a test that actually compress files seems hard to make stable, and adds
a dependency on external tools which is best to avoid when possible.  I took a
stab at this and added a test that ensures the last segment in the switched-
away file is completely zeroed out, which in a sense tests compressability.

The attached patch adds the test, and a neccessary extension to check_pg_config
to allow for extracting values from pg_config.h as opposed to just returning
the number of regex matches. (needed for XLOG_BLCKSZ.)

That being said, I am not entirely convinced that the test is adding much
value.  It’s however easier to reason about a written patch than an idea so I
figured I’d add it here either way.

Setting this to Ready for Committer and offering my +1 on getting this in.

cheers ./daniel

On 03/16/18 17:14, Daniel Gustafsson wrote:
> The attached patch adds the test, and a neccessary extension to check_pg_config
> to allow for extracting values from pg_config.h as opposed to just returning
> the number of regex matches. (needed for XLOG_BLCKSZ.)

Thanks for the review. I notice that cfbot has now flagged the patch as
failing, and when I look into it, it appears that cfbot is building with
your test patch, and without the xlog.c patch, and so the test naturally
fails. Does the cfbot require both patches to be attached to the same
email, in order to include them both? I'll try attaching both to this one,
and see what it does.

This is good confirmation that the test is effective. :)

-Chap

Chapman Flack <chap@anastigmatix.net> writes:
> Thanks for the review. I notice that cfbot has now flagged the patch as
> failing, and when I look into it, it appears that cfbot is building with
> your test patch, and without the xlog.c patch, and so the test naturally
> fails. Does the cfbot require both patches to be attached to the same
> email, in order to include them both?

I believe so --- AFAIK it does not know anything about dependencies
between different patches, and will just try to build whatever patch(es)
appear in the latest email on a given thread.  Munro might be able to
provide more detail.

            regards, tom lane

> On 18 Mar 2018, at 03:09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Chapman Flack <chap@anastigmatix.net> writes:
>> Thanks for the review. I notice that cfbot has now flagged the patch as
>> failing, and when I look into it, it appears that cfbot is building with
>> your test patch, and without the xlog.c patch, and so the test naturally
>> fails. Does the cfbot require both patches to be attached to the same
>> email, in order to include them both?
>
> I believe so --- AFAIK it does not know anything about dependencies
> between different patches, and will just try to build whatever patch(es)
> appear in the latest email on a given thread.  Munro might be able to
> provide more detail.

Right, I should’ve realized when I didn’t include your original patch as well,
sorry about that.  Now we know that the proposed test fails without the patch
applied and clears with it, that was at least an interesting side effect =)

cheers ./daniel

On 03/18/18 16:56, Daniel Gustafsson wrote:
> sorry about that.  Now we know that the proposed test fails without the patch
> applied and clears with it, that was at least an interesting side effect =)

It was, and it got me looking at the test, and even though it does detect
the difference between patch-applied and patch-not-applied, I sort of wonder
if it does what it claims to. It seems to me that unpack('N8192', ...)
would want to return 8192 32-bit ints (in array context), but really only
be able to return 2048 of them (because it's only got 8192 bytes to unpack),
and then being used in scalar context, it only returns the first one anyway,
so the test only hinges on whether the first four bytes of the block are
zero or not. Which turns out to be enough to catch a non-zeroed header. :)

What would you think about replacing the last two lines with just

  ok($bytes =~ /\A\x00*+\z/, 'make sure wal segment is zeroed');

-Chap

> On 18 Mar 2018, at 22:54, Chapman Flack <chap@anastigmatix.net> wrote:
>
> On 03/18/18 16:56, Daniel Gustafsson wrote:
>> sorry about that.  Now we know that the proposed test fails without the patch
>> applied and clears with it, that was at least an interesting side effect =)
>
> It was, and it got me looking at the test, and even though it does detect
> the difference between patch-applied and patch-not-applied, I sort of wonder
> if it does what it claims to. It seems to me that unpack('N8192', ...)
> would want to return 8192 32-bit ints (in array context), but really only
> be able to return 2048 of them (because it's only got 8192 bytes to unpack),
> and then being used in scalar context, it only returns the first one anyway,
> so the test only hinges on whether the first four bytes of the block are
> zero or not. Which turns out to be enough to catch a non-zeroed header. :)

Good point, thats what I get for hacking without enough coffee.

> What would you think about replacing the last two lines with just
>
>  ok($bytes =~ /\A\x00*+\z/, 'make sure wal segment is zeroed’);

It seems expensive to regex over BLCKSZ, but it’s probably the safest option
and it’s not a performance critical codepath.  Feel free to whack the test
patch over the head with the above diff.

cheers ./daniel

On 03/18/18 19:28, Daniel Gustafsson wrote:
> It seems expensive to regex over BLCKSZ, but it’s probably the safest option
> and it’s not a performance critical codepath.  Feel free to whack the test
> patch over the head with the above diff.

Both patches in a single email for cfbot's enjoyment, coming right up.

-Chap

Greetings,

* Chapman Flack (chap@anastigmatix.net) wrote:
> On 03/18/18 19:28, Daniel Gustafsson wrote:
> > It seems expensive to regex over BLCKSZ, but it’s probably the safest option
> > and it’s not a performance critical codepath.  Feel free to whack the test
> > patch over the head with the above diff.
>
> Both patches in a single email for cfbot's enjoyment, coming right up.

Thanks for this.  I'm also of the opinion, having read through the
thread, that it was an unintentional side-effect to have the headers in
the otherwise-zero'd end of the WAL segment and that re-zero'ing the end
makes sense and while it's a bit of extra time, it's not on a
performance-critical path given this is only happening on a less-busy
system to begin with.

I'm mildly disappointed to see the gzip has only a 2x improvement with
the change, but that's certainly not this patch's fault.

Reviewing the patch itself..

>  .travis.yml                          | 47 ++++++++++++++++++++++++++++++++++++

... not something that I think we're going to add into the main tree.

> diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
> index 47a6c4d..a91ec7b 100644
> --- a/src/backend/access/transam/xlog.c
> +++ b/src/backend/access/transam/xlog.c
> @@ -1556,7 +1556,16 @@ CopyXLogRecordToWAL(int write_len, bool isLogSwitch, XLogRecData *rdata,
>          {
>              /* initialize the next page (if not initialized already) */
>              WALInsertLockUpdateInsertingAt(CurrPos);
> -            AdvanceXLInsertBuffer(CurrPos, false);
> +            /*
> +             * Fields in the page header preinitialized by AdvanceXLInsertBuffer
> +             * to nonconstant values reduce the compressibility of WAL segments,
> +             * and aren't needed in the freespace following a switch record.
> +             * Re-zero that header area. This is not performance-critical, as
> +             * the more empty pages there are for this loop to touch, the less
> +             * busy the system is.
> +             */
> +            currpos = GetXLogBuffer(CurrPos);
> +            MemSet(currpos, 0, SizeOfXLogShortPHD);
>              CurrPos += XLOG_BLCKSZ;
>          }
>      }

AdvanceXLInsertBuffer() does quite a bit, so I'm a bit surprised to see
this simply removing that call, you're confident there's nothing done
which still needs doing..?

Thanks!

Stephen

On Sun, Mar 25, 2018 at 11:27:31PM -0400, Stephen Frost wrote:
> AdvanceXLInsertBuffer() does quite a bit, so I'm a bit surprised to see
> this simply removing that call, you're confident there's nothing done
> which still needs doing..?

Have a look at BKP_REMOVABLE then.  This was moved to page headers in
2dd9322, still it seems to me that the small benefits outlined on this
thread don't justify breaking tools relying on this flag set, especially
if there is no replacement for it.

So I would vote to not commit this patch.
--
Michael

On 03/25/18 23:27, Stephen Frost wrote:
>>  .travis.yml                          | 47 ++++++++++++++++++++++++++++++++++++
> 
> ... not something that I think we're going to add into the main tree.

Looks like that got in by mistake, sorry.

> -            AdvanceXLInsertBuffer(CurrPos, false);
> ...
> +            currpos = GetXLogBuffer(CurrPos);
>
> AdvanceXLInsertBuffer() does quite a bit, so I'm a bit surprised to see
> this simply removing that call, you're confident there's nothing done
> which still needs doing..?

My belief from looking at the code was that AdvanceXLInsertBuffer() is among
the things GetXLogBuffer() does, so calling both would result in two calls
to the former (which I don't believe would hurt, it would only
do enough work the second time to determine it had already been done).

However, it is done *conditionally* within GetXLogBuffer(), so it doesn't
hurt to have extra eyes reviewing my belief that the condition will be true
in this case (looping through tail blocks that haven't been touched yet).

-Chap

Chapman Flack <chap@anastigmatix.net> writes:
> On 03/25/18 23:27, Stephen Frost wrote:
>> AdvanceXLInsertBuffer() does quite a bit, so I'm a bit surprised to see
>> this simply removing that call, you're confident there's nothing done
>> which still needs doing..?

> My belief from looking at the code was that AdvanceXLInsertBuffer() is among
> the things GetXLogBuffer() does, so calling both would result in two calls
> to the former (which I don't believe would hurt, it would only
> do enough work the second time to determine it had already been done).

If that's the argument, why is the WALInsertLockUpdateInsertingAt(CurrPos)
call still there?  GetXLogBuffer() would do that too.

In any case, the new comment seems very poorly written, as it fails to
explain what's going on, and the reference to a function that is not
actually called from the vicinity of the comment doesn't help.  I'd
suggest something like "GetXLogBuffer() will fetch and initialize the
next WAL page for us.  But it initializes the page header to nonzero,
which is undesirable because X, so we then reset the header to zero".
It might also be worth explaining how you know that the new page's
header is short not long.

            regards, tom lane

On 03/26/18 12:34, Tom Lane wrote:
> If that's the argument, why is the WALInsertLockUpdateInsertingAt(CurrPos)
> call still there?  GetXLogBuffer() would do that too.

"Because I hadn't noticed that," he said, sheepishly.

> In any case, the new comment ... fails to
> explain what's going on, and the reference to a function that is not
> actually called from the vicinity of the comment ...
> suggest something like "GetXLogBuffer() will fetch and initialize the
> next WAL page for us.  ... worth explaining how you know that the new
> page's header is short not long.

Here are patches responding to that (and also fixing the unintended
inclusion of .travis.yml).

What I have not done here is respond to Michael's objection, which
I haven't had time to think more about yet.

-Chap

On 03/26/18 01:43, Michael Paquier wrote:

> Have a look at BKP_REMOVABLE then.  This was moved to page headers in
> 2dd9322, still it seems to me that the small benefits outlined on this
> thread don't justify breaking tools relying on this flag set, especially
> if there is no replacement for it.

Is 2dd9322 a commit? I'm having difficulty finding it:

https://git.postgresql.org/gitweb/?p=postgresql.git&a=search&h=HEAD&st=commit&s=2dd9322

Am I searching wrong?

I probably won't have more time to look at this tonight, but could
I ask in advance for examples of tools that would need this bit when
encountering unwritten pages at the end of a segment?

I don't mean that as snark; it's just nonobvious to me and I might need
a little nudge where to look.

-Chap

On 03/28/2018 12:32 AM, Chapman Flack wrote:
> On 03/26/18 01:43, Michael Paquier wrote:
> 
>> Have a look at BKP_REMOVABLE then.  This was moved to page headers in
>> 2dd9322, still it seems to me that the small benefits outlined on this
>> thread don't justify breaking tools relying on this flag set, especially
>> if there is no replacement for it.
> 
> Is 2dd9322 a commit? I'm having difficulty finding it:
> 
> https://git.postgresql.org/gitweb/?p=postgresql.git&a=search&h=HEAD&st=commit&s=2dd9322
> 
> Am I searching wrong?
> 

Not sure what's up with gitweb, but git finds it without any issue:

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2dd9322ba6eea76800b38bfea0599fbc459458f2

> I probably won't have more time to look at this tonight, but could
> I ask in advance for examples of tools that would need this bit when
> encountering unwritten pages at the end of a segment?
> 
> I don't mean that as snark; it's just nonobvious to me and I might need
> a little nudge where to look.
> 
> -Chap
> 

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Mar 27, 2018 at 06:32:08PM -0400, Chapman Flack wrote:
> Is 2dd9322 a commit? I'm having difficulty finding it:
>
> https://git.postgresql.org/gitweb/?p=postgresql.git&a=search&h=HEAD&st=commit&s=2dd9322
>
> Am I searching wrong?
>
> I probably won't have more time to look at this tonight, but could
> I ask in advance for examples of tools that would need this bit when
> encountering unwritten pages at the end of a segment?

Here you go for one example:
https://sourceforge.net/projects/pglesslog/
At the end we may want to perhaps just drop the flag, but that's quite a
hard call as there could be people not around who use it..
--
Michael

On 03/27/18 20:09, Tomas Vondra wrote:
> Not sure what's up with gitweb, but git finds it without any issue:
> 
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=2dd9322ba6eea76800b38bfea0599fbc459458f2

Thanks, that worked.

On 03/27/18 22:10, Michael Paquier wrote:
> Here you go for one example:
> https://sourceforge.net/projects/pglesslog/

So far, I have been able to study the commit pertaining to
XLP_BKP_REMOVABLE. For again some odd reason, I am striking out on
finding pglesslog code to study. Using the clone URL offered by sourceforge:

$ git clone https://git.code.sf.net/p/pglesslog/code pglesslog-code
Cloning into 'pglesslog-code'...
warning: You appear to have cloned an empty repository.
Checking connectivity... done.

and there's a Files tab, but it tells me This project has no files.

I can find 1.4.2 beta on pgFoundry, but that predates the BKP_REMOVABLE
commit.


In any case, from my study of the commit, it is hard for me to see an issue.
The code comment says: "mark the header to indicate that WAL records
beginning in this page have removable backup blocks."

In the only case where this patch will zero a header--in the unused space
following the switch record in a segment--there are no "WAL records
beginning in this page". There will not be another WAL record of any kind
until the next valid page (with valid xlp_magic xlp_tli xlp_pageaddr),
which will be at the start of the next segment, and that page will have
XLP_BKP_REMOVABLE if it ought to, and that will tell the reader what it
needs to know.

Am I overlooking something?

-Chap

Chapman Flack <chap@anastigmatix.net> writes:
> On 03/27/18 22:10, Michael Paquier wrote:
>> Here you go for one example:
>> https://sourceforge.net/projects/pglesslog/

> In any case, from my study of the commit, it is hard for me to see an issue.
> The code comment says: "mark the header to indicate that WAL records
> beginning in this page have removable backup blocks."

Yeah, that commit just moved a flag from individual WAL records to page
headers, arguing that it was okay to assume that the same flag value
applies to all records on a page.  If there are no records in the page,
it doesn't matter what you think the flag value is.

A potentially stronger complaint is that WAL-reading tools might fail
outright on a page with an invalid header, but I'd say that's a robustness
issue that they'd need to address anyway.  There's never been any
guarantee that the trailing pages of a WAL segment are valid.

            regards, tom lane

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Chapman Flack <chap@anastigmatix.net> writes:
> > On 03/27/18 22:10, Michael Paquier wrote:
> >> Here you go for one example:
> >> https://sourceforge.net/projects/pglesslog/
>
> > In any case, from my study of the commit, it is hard for me to see an issue.
> > The code comment says: "mark the header to indicate that WAL records
> > beginning in this page have removable backup blocks."
>
> Yeah, that commit just moved a flag from individual WAL records to page
> headers, arguing that it was okay to assume that the same flag value
> applies to all records on a page.  If there are no records in the page,
> it doesn't matter what you think the flag value is.
>
> A potentially stronger complaint is that WAL-reading tools might fail
> outright on a page with an invalid header, but I'd say that's a robustness
> issue that they'd need to address anyway.  There's never been any
> guarantee that the trailing pages of a WAL segment are valid.

Agreed, I don't buy off that tools which fall apart when reading a page
with an invalid header should block this from moving forward- those
tools need to be fixed to not rely on trailing/unused WAL pages to be
valid.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> A potentially stronger complaint is that WAL-reading tools might fail
>> outright on a page with an invalid header, but I'd say that's a robustness
>> issue that they'd need to address anyway.  There's never been any
>> guarantee that the trailing pages of a WAL segment are valid.

> Agreed, I don't buy off that tools which fall apart when reading a page
> with an invalid header should block this from moving forward- those
> tools need to be fixed to not rely on trailing/unused WAL pages to be
> valid.

Yup.  Pushed with some rewriting of the comments.

I did not like the proposed test case too much, particularly not its
undocumented API change for check_pg_config, so I did not push that.
We already have test coverage for pg_switch_wal() so it doesn't seem
very critical to have more.

            regards, tom lane

On 03/30/18 16:21, Tom Lane wrote:

> Yup.  Pushed with some rewriting of the comments.

Thanks!

> I did not like the proposed test case too much, particularly not its
> undocumented API change for check_pg_config,

Other than that API change, was there something the test case could have
done differently to make you like it more?

-Chap

Chapman Flack <chap@anastigmatix.net> writes:
> On 03/30/18 16:21, Tom Lane wrote:
>> I did not like the proposed test case too much, particularly not its
>> undocumented API change for check_pg_config,

> Other than that API change, was there something the test case could have
> done differently to make you like it more?

Well, if that'd been properly documented I'd probably have pushed it
without complaint.  But I did wonder whether it could've been folded
into one of the existing tests of pg_switch_wal().  This doesn't seem
like a property worth spending a lot of cycles on testing.

            regards, tom lane

On Fri, Mar 30, 2018 at 07:11:02PM -0400, Tom Lane wrote:
> Chapman Flack <chap@anastigmatix.net> writes:
> > On 03/30/18 16:21, Tom Lane wrote:
> >> I did not like the proposed test case too much, particularly not its
> >> undocumented API change for check_pg_config,
>
> > Other than that API change, was there something the test case could have
> > done differently to make you like it more?
>
> Well, if that'd been properly documented I'd probably have pushed it
> without complaint.  But I did wonder whether it could've been folded
> into one of the existing tests of pg_switch_wal().  This doesn't seem
> like a property worth spending a lot of cycles on testing.

Sorry for coming in late.  I have been busy doing some net-archeology to
look for tools using XLP_BKP_REMOVABLE.  One is pglesslog that we
already know about.  However I have to be honest, I have not been able
to find its source code, nor have I seen another tool making use of
XLP_BKP_REMOVABLE.  Could we just remove the flag then?
--
Michael

On 03/30/18 19:57, Michael Paquier wrote:

> look for tools using XLP_BKP_REMOVABLE.  One is pglesslog that we
> already know about.  However I have to be honest, I have not been able
> to find its source code,

I'm glad it wasn't just me.

-Chap