Thread: backup manifests

In the lengthy thread on block-level incremental backup,[1] both
Vignesh C[2] and Stephen Frost[3] have suggested storing a manifest as
part of each backup, somethig that could be useful not only for
incremental backups but also for full backups. I initially didn't
think this was necessary,[4] but some of my colleagues figured out
that my design was broken, because my proposal was to detect new
blocks just using LSN, and that ignores the fact that CREATE DATABASE
and ALTER TABLE .. SET TABLESPACE do physical copies without bumping
page LSNs, which I knew but somehow forgot about.  Fortunately, some
of my colleagues realized my mistake in testing.[5] Because of this
problem, for an LSN-based approach to work, we'll need to send not
only an LSN, but also a list of files (and file sizes) that exist in
the previous full backup; so, some kind of backup manifest now seems
like a good idea to me.[6] That whole approach might still be dead on
arrival if it's possible to add new blocks with old LSNs to existing
files,[7] but there seems to be room to hope that there are no such
cases.[8]

So, let's suppose we invent a backup manifest. What should it contain?
I imagine that it would consist of a list of files, and the lengths of
those files, and a checksum for each file. I think you should have a
choice of what kind of checksums to use, because algorithms that used
to seem like good choices (e.g. MD5) no longer do; this trend can
probably be expected to continue. Even if we initially support only
one kind of checksum -- presumably SHA-something since we have code
for that already for SCRAM -- I think that it would also be a good
idea to allow for future changes. And maybe it's best to just allow a
choice of SHA-224, SHA-256, SHA-384, and SHA-512 right out of the
gate, so that we can avoid bikeshedding over which one is secure
enough. I guess we'll still have to argue about the default. I also
think that it should be possible to build a manifest with no
checksums, so that one need not pay the overhead of computing
checksums if one does not wish. Of course, such a manifest is of much
less utility for checking backup integrity, but you can still check
that you've got the right files, which is noticeably better than
nothing.  The manifest should probably also contain a checksum of its
own contents so that the integrity of the manifest itself can be
verified. And maybe a few other bits of metadata, but I'm not sure
exactly what.  Ideas?

Once we invent the concept of a backup manifest, what do we need to do
with them? I think we'd want three things initially:

(1) When taking a backup, have the option (perhaps enabled by default)
to include a backup manifest.
(2) Given an existing backup that has not got a manifest, construct one.
(3) Cross-check a manifest against a backup and complain about extra
files, missing files, size differences, or checksum mismatches.

One thing I'm not quite sure about is where to store the backup
manifest. If you take a base backup in tar format, you get base.tar,
pg_wal.tar (unless -Xnone), and an additional tar file per tablespace.
Does the backup manifest go into base.tar? Get written into a separate
file outside of any tar archive? Something else? And what about a
plain-format backup? I suppose then we should just write the manifest
into the top level of the main data directory, but perhaps someone has
another idea.

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

[1]
https://www.postgresql.org/message-id/flat/CA%2BTgmoYxQLL%3DmVyN90HZgH0X_EUrw%2BaZ0xsXJk7XV3-3LygTvA%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CALDaNm310fUZ72nM2n%3DcD0eSHKRAoJPuCyvvR0dhTEZ9Oytyzg%40mail.gmail.com
[3] https://www.postgresql.org/message-id/20190916143817.GA6962%40tamriel.snowman.net
[4] https://www.postgresql.org/message-id/CA%2BTgmoaj-zw4Mou4YBcJSkHmQM%2BJA-dAVJnRP8zSASP1S4ZVgw%40mail.gmail.com
[5] https://www.postgresql.org/message-id/CAM2%2B6%3DXfJX%3DKXvpTgDvgd1rQjya_Am27j4UvJtL3nA%2BJMCTGVQ%40mail.gmail.com
[6] https://www.postgresql.org/message-id/CA%2BTgmoYg9i8TZhyjf8MqCyU8unUVuW%2B03FeBF1LGDu_-eOONag%40mail.gmail.com
[7] https://www.postgresql.org/message-id/CA%2BTgmoYT9xODgEB6y6j93hFHqobVcdiRCRCp0dHh%2BfFzZALn%3Dw%40mail.gmail.com
and nearby messages
[8] https://www.postgresql.org/message-id/20190916173933.GE6962%40tamriel.snowman.net

Hi Robert,

On 9/18/19 1:48 PM, Robert Haas wrote:
> That whole approach might still be dead on
> arrival if it's possible to add new blocks with old LSNs to existing
> files,[7] but there seems to be room to hope that there are no such
> cases.[8]

I sure hope there are no such cases, but we should be open to the idea
just in case.

> So, let's suppose we invent a backup manifest. What should it contain?
> I imagine that it would consist of a list of files, and the lengths of
> those files, and a checksum for each file. 

These are essential.

Also consider adding the timestamp.  You have justifiable concerns about
using timestamps for deltas and I get that.  However, there are a number
of methods that can be employed to make it *much* safer.  I won't go
into that here since it is an entire thread in itself.  Suffice to say
we can detect many anomalies in the timestamps and require a checksum
backup when we see them.  I'm really interested in scanning the WAL for
changed files but that method is very complex and getting it right might
be harder than ensuring FS checksums are reliable.  Still worth trying,
though, since the benefits are enormous.  We are planning to use
timestamp + size + wal data to do incrementals if we get there.

Consider adding a reference to each file that specifies where the file
can be found in if it is not in this backup.  As I understand the
pg_basebackup proposal, it would only be implementing differential
backups, i.e. an incremental that is *only* based on the last full
backup.  So, the reference can be inferred in this case.  However, if
the user selects the wrong full backup on restore, and we have labeled
each backup, then a differential restore with references against the
wrong full backup would result in a hard error rather than corruption.

> I think you should have a
> choice of what kind of checksums to use, because algorithms that used
> to seem like good choices (e.g. MD5) no longer do; this trend can
> probably be expected to continue. Even if we initially support only
> one kind of checksum -- presumably SHA-something since we have code
> for that already for SCRAM -- I think that it would also be a good
> idea to allow for future changes. And maybe it's best to just allow a
> choice of SHA-224, SHA-256, SHA-384, and SHA-512 right out of the
> gate, so that we can avoid bikeshedding over which one is secure
> enough. I guess we'll still have to argue about the default. 

Based on my original calculations (which sadly I don't have anymore),
the combination of SHA1, size, and file name is *extremely* unlikely to
generate a collision.  As in, unlikely to happen before the end of the
universe kind of unlikely.  Though, I guess it depends on your
expectations for the lifetime of the universe.

These checksums don't have to be cryptographically secure, in the sense
that you could infer the plaintext from the checksum.  They just need to
have a suitably low collision rate.  These days I would choose something
with more bits because the computation time is similar, though the
larger size requires more storage.

> I also
> think that it should be possible to build a manifest with no
> checksums, so that one need not pay the overhead of computing
> checksums if one does not wish. 

Our benchmarks have indicated that checksums only account for about 1%
of total cpu time when gzip -6 compression is used.  Without compression
the percentage may be higher of course, but in that case we find network
latency is the primary bottleneck.

For S3 backups we do a SHA1 hash for our manifest, a SHA256 hash for
authv4 and a good-old-fashioned MD5 checksum for each upload part.  This
is barely noticeable when compression is enabled.

> Of course, such a manifest is of much
> less utility for checking backup integrity, but you can still check
> that you've got the right files, which is noticeably better than
> nothing.  

Absolutely -- and yet.  There was a time when we made checksums optional
but eventually gave up on that once we profiled and realized how low the
cost was vs. the benefit.

> The manifest should probably also contain a checksum of its
> own contents so that the integrity of the manifest itself can be
> verified. 

This is a good idea.  Amazingly we've never seen a manifest checksum
error in the field but it's only a matter of time.

And maybe a few other bits of metadata, but I'm not sure
> exactly what.  Ideas?

A backup label for sure.  You can also use this as the directory/tar
name to save the user coming up with one.  We use YYYYMMDDHH24MMSSF for
full backups and YYYYMMDDHH24MMSSF_YYYYMMDDHH24MMSS(D|I) for
incrementals and have logic to prevent two backups from having the same
label.  This is unlikely outside of testing but still a good idea.

Knowing the start/stop time of the backup is useful in all kinds of
ways, especially monitoring and time-targeted PITR.  Start/stop LSN is
also good.  I know this is also in backup_label but having it all in one
place is nice.

We include the version/sysid of the cluster to avoid mixups.  It's a
great extra check on top of references to be sure everything is kosher.

A manifest version is good in case we change the format later.  I'd
recommend JSON for the format since it is so ubiquitous and easily
handles escaping which can be gotchas in a home-grown format.  We
currently have a format that is a combination of Windows INI and JSON
(for human-readability in theory) and we have become painfully aware of
escaping issues.  Really, why would you drop files with '=' in their
name in PGDATA?  And yet it happens.

> Once we invent the concept of a backup manifest, what do we need to do
> with them? I think we'd want three things initially:
> 
> (1) When taking a backup, have the option (perhaps enabled by default)
> to include a backup manifest.

Manifests are cheap to builds so I wouldn't make it an option.

> (2) Given an existing backup that has not got a manifest, construct one.

Might be too late to be trusted and we'd have to write extra code for
it.  I'd leave this for a project down the road, if at all.

> (3) Cross-check a manifest against a backup and complain about extra
> files, missing files, size differences, or checksum mismatches.

Verification is the best part of the manifest.  Plus, you can do
verification pretty cheaply on restore.  We also restore pg_control last
so clusters that have a restore error won't start.

> One thing I'm not quite sure about is where to store the backup
> manifest. If you take a base backup in tar format, you get base.tar,
> pg_wal.tar (unless -Xnone), and an additional tar file per tablespace.
> Does the backup manifest go into base.tar? Get written into a separate
> file outside of any tar archive? Something else? And what about a
> plain-format backup? I suppose then we should just write the manifest
> into the top level of the main data directory, but perhaps someone has
> another idea.

We do:

[backup_label]/
    backup.manifest
    pg_data/
    pg_tblspc/

In general, having the manifest easily accessible is ideal.

Regards,
-- 
-David
david@pgmasters.net

On Wed, Sep 18, 2019 at 9:11 PM David Steele <david@pgmasters.net> wrote:
> Also consider adding the timestamp.

Sounds reasonable, even if only for the benefit of humans who might
look at the file.  We can decide later whether to use it for anything
else (and third-party tools could make different decisions from core).
I assume we're talking about file mtime here, not file ctime or file
atime or the time the manifest was generated, but let me know if I'm
wrong.

> Consider adding a reference to each file that specifies where the file
> can be found in if it is not in this backup.  As I understand the
> pg_basebackup proposal, it would only be implementing differential
> backups, i.e. an incremental that is *only* based on the last full
> backup.  So, the reference can be inferred in this case.  However, if
> the user selects the wrong full backup on restore, and we have labeled
> each backup, then a differential restore with references against the
> wrong full backup would result in a hard error rather than corruption.

I intend that we should be able to support incremental backups based
either on a previous full backup or based on a previous incremental
backup. I am not aware of a technical reason why we need to identify
the specific backup that must be used. If incremental backup B is
taken based on a pre-existing backup A, then I think that B can be
restored using either A or *any other backup taken after A and before
B*. In the normal case, there probably wouldn't be any such backup,
but AFAICS the start-LSNs are a sufficient cross-check that the chosen
base backup is legal.

> Based on my original calculations (which sadly I don't have anymore),
> the combination of SHA1, size, and file name is *extremely* unlikely to
> generate a collision.  As in, unlikely to happen before the end of the
> universe kind of unlikely.  Though, I guess it depends on your
> expectations for the lifetime of the universe.

Somebody once said that we should be prepared for it to end at an any
time, or not, and that the time at which it actually was due to end
would not be disclosed in advance. This is probably good life advice
which I ought to take more frequently than I do, but I think we can
finesse the issue for purposes of this discussion. What I'd say is: if
the probability of getting a collision is demonstrably many orders of
magnitude less than the probability of the disk writing the block
incorrectly, then I think we're probably reasonably OK. Somebody might
differ, which is perhaps a mild point in favor of LSN-based
approaches, but as a practical matter, if a bad block is a billion
times more likely to be the result of a disk error than a checksum
mismatch, then it's a negligible risk.

> And maybe a few other bits of metadata, but I'm not sure
> > exactly what.  Ideas?
>
> A backup label for sure.  You can also use this as the directory/tar
> name to save the user coming up with one.  We use YYYYMMDDHH24MMSSF for
> full backups and YYYYMMDDHH24MMSSF_YYYYMMDDHH24MMSS(D|I) for
> incrementals and have logic to prevent two backups from having the same
> label.  This is unlikely outside of testing but still a good idea.
>
> Knowing the start/stop time of the backup is useful in all kinds of
> ways, especially monitoring and time-targeted PITR.  Start/stop LSN is
> also good.  I know this is also in backup_label but having it all in one
> place is nice.
>
> We include the version/sysid of the cluster to avoid mixups.  It's a
> great extra check on top of references to be sure everything is kosher.

I don't think it's a good idea to duplicate the information that's
already in the backup_label. Storing two copies of the same
information is just an invitation to having to worry about what
happens if they don't agree.

> A manifest version is good in case we change the format later.

Yeah.

> I'd
> recommend JSON for the format since it is so ubiquitous and easily
> handles escaping which can be gotchas in a home-grown format.  We
> currently have a format that is a combination of Windows INI and JSON
> (for human-readability in theory) and we have become painfully aware of
> escaping issues.  Really, why would you drop files with '=' in their
> name in PGDATA?  And yet it happens.

I am not crazy about JSON because it requires that I get a json parser
into src/common, which I could do, but given the possibly-imminent end
of the universe, I'm not sure it's the greatest use of time. You're
right that if we pick an ad-hoc format, we've got to worry about
escaping, which isn't lovely.

> > (1) When taking a backup, have the option (perhaps enabled by default)
> > to include a backup manifest.
>
> Manifests are cheap to builds so I wouldn't make it an option.

Huh. That's an interesting idea. Thanks.

> > (3) Cross-check a manifest against a backup and complain about extra
> > files, missing files, size differences, or checksum mismatches.
>
> Verification is the best part of the manifest.  Plus, you can do
> verification pretty cheaply on restore.  We also restore pg_control last
> so clusters that have a restore error won't start.

There's no "restore" operation here, really. A backup taken by
pg_basebackup can be "restored" by copying the whole thing, but it can
also be used just where it is. If we were going to build something
into some in-core tool to copy backups around, this would be a smart
way to implement said tool, but I'm not planning on that myself.

> > One thing I'm not quite sure about is where to store the backup
> > manifest. If you take a base backup in tar format, you get base.tar,
> > pg_wal.tar (unless -Xnone), and an additional tar file per tablespace.
> > Does the backup manifest go into base.tar? Get written into a separate
> > file outside of any tar archive? Something else? And what about a
> > plain-format backup? I suppose then we should just write the manifest
> > into the top level of the main data directory, but perhaps someone has
> > another idea.
>
> We do:
>
> [backup_label]/
>     backup.manifest
>     pg_data/
>     pg_tblspc/
>
> In general, having the manifest easily accessible is ideal.

That's a fine choice for a tool, but a I'm talking about something
that is part of the actual backup format supported by PostgreSQL, not
what a tool might wrap around it. The choice is whether, for a
tar-format backup, the manifest goes inside a tar file or as a
separate file. To put that another way, a patch adding backup
manifests does not get to redesign where pg_basebackup puts anything
else; it only gets to decide where to put the manifest.

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

On Thu, Sep 19, 2019 at 9:51 AM Robert Haas <robertmhaas@gmail.com> wrote:
> I intend that we should be able to support incremental backups based
> either on a previous full backup or based on a previous incremental
> backup. I am not aware of a technical reason why we need to identify
> the specific backup that must be used. If incremental backup B is
> taken based on a pre-existing backup A, then I think that B can be
> restored using either A or *any other backup taken after A and before
> B*. In the normal case, there probably wouldn't be any such backup,
> but AFAICS the start-LSNs are a sufficient cross-check that the chosen
> base backup is legal.

Scratch that: there can be overlapping backups, so you have to
cross-check both start and stop LSNs.

> > > (3) Cross-check a manifest against a backup and complain about extra
> > > files, missing files, size differences, or checksum mismatches.
> >
> > Verification is the best part of the manifest.  Plus, you can do
> > verification pretty cheaply on restore.  We also restore pg_control last
> > so clusters that have a restore error won't start.
>
> There's no "restore" operation here, really. A backup taken by
> pg_basebackup can be "restored" by copying the whole thing, but it can
> also be used just where it is. If we were going to build something
> into some in-core tool to copy backups around, this would be a smart
> way to implement said tool, but I'm not planning on that myself.

Scratch that: incremental backups need a restore tool, so we can use
this technique there. And it can work for full backups too, because
why not?

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

Hi Robert,

On 9/19/19 9:51 AM, Robert Haas wrote:
> On Wed, Sep 18, 2019 at 9:11 PM David Steele <david@pgmasters.net> wrote:
>> Also consider adding the timestamp.
> 
> Sounds reasonable, even if only for the benefit of humans who might
> look at the file.  We can decide later whether to use it for anything
> else (and third-party tools could make different decisions from core).
> I assume we're talking about file mtime here, not file ctime or file
> atime or the time the manifest was generated, but let me know if I'm
> wrong.

In my experience only mtime is useful.

>> Based on my original calculations (which sadly I don't have anymore),
>> the combination of SHA1, size, and file name is *extremely* unlikely to
>> generate a collision.  As in, unlikely to happen before the end of the
>> universe kind of unlikely.  Though, I guess it depends on your
>> expectations for the lifetime of the universe.

> What I'd say is: if
> the probability of getting a collision is demonstrably many orders of
> magnitude less than the probability of the disk writing the block
> incorrectly, then I think we're probably reasonably OK. Somebody might
> differ, which is perhaps a mild point in favor of LSN-based
> approaches, but as a practical matter, if a bad block is a billion
> times more likely to be the result of a disk error than a checksum
> mismatch, then it's a negligible risk.

Agreed.

>> We include the version/sysid of the cluster to avoid mixups.  It's a
>> great extra check on top of references to be sure everything is kosher.
> 
> I don't think it's a good idea to duplicate the information that's
> already in the backup_label. Storing two copies of the same
> information is just an invitation to having to worry about what
> happens if they don't agree.

OK, but now we have backup_label, tablespace_map, 
XXXXXXXXXXXXXXXXXXXXXXXX.XXXXXXXX.backup (in the WAL) and now perhaps a 
backup.manifest file.  I feel like we may be drowning in backup info files.

>> I'd
>> recommend JSON for the format since it is so ubiquitous and easily
>> handles escaping which can be gotchas in a home-grown format.  We
>> currently have a format that is a combination of Windows INI and JSON
>> (for human-readability in theory) and we have become painfully aware of
>> escaping issues.  Really, why would you drop files with '=' in their
>> name in PGDATA?  And yet it happens.
> 
> I am not crazy about JSON because it requires that I get a json parser
> into src/common, which I could do, but given the possibly-imminent end
> of the universe, I'm not sure it's the greatest use of time. You're
> right that if we pick an ad-hoc format, we've got to worry about
> escaping, which isn't lovely.

My experience is that JSON is simple to implement and has already dealt 
with escaping and data structure considerations.  A home-grown solution 
will be at least as complex but have the disadvantage of being non-standard.

>>> One thing I'm not quite sure about is where to store the backup
>>> manifest. If you take a base backup in tar format, you get base.tar,
>>> pg_wal.tar (unless -Xnone), and an additional tar file per tablespace.
>>> Does the backup manifest go into base.tar? Get written into a separate
>>> file outside of any tar archive? Something else? And what about a
>>> plain-format backup? I suppose then we should just write the manifest
>>> into the top level of the main data directory, but perhaps someone has
>>> another idea.
>>
>> We do:
>>
>> [backup_label]/
>>      backup.manifest
>>      pg_data/
>>      pg_tblspc/
>>
>> In general, having the manifest easily accessible is ideal.
> 
> That's a fine choice for a tool, but a I'm talking about something
> that is part of the actual backup format supported by PostgreSQL, not
> what a tool might wrap around it. The choice is whether, for a
> tar-format backup, the manifest goes inside a tar file or as a
> separate file. To put that another way, a patch adding backup
> manifests does not get to redesign where pg_basebackup puts anything
> else; it only gets to decide where to put the manifest.

Fair enough.  The point is to make the manifest easily accessible.

I'd keep it in the data directory for file-based backups and as a 
separate file for tar-based backups.  The advantage here is that we can 
pick a file name that becomes reserved which a tool can't do.

Regards,
-- 
-David
david@pgmasters.net

On 9/19/19 11:00 AM, Robert Haas wrote:

> On Thu, Sep 19, 2019 at 9:51 AM Robert Haas <robertmhaas@gmail.com> wrote:
>> I intend that we should be able to support incremental backups based
>> either on a previous full backup or based on a previous incremental
>> backup. I am not aware of a technical reason why we need to identify
>> the specific backup that must be used. If incremental backup B is
>> taken based on a pre-existing backup A, then I think that B can be
>> restored using either A or *any other backup taken after A and before
>> B*. In the normal case, there probably wouldn't be any such backup,
>> but AFAICS the start-LSNs are a sufficient cross-check that the chosen
>> base backup is legal.
> 
> Scratch that: there can be overlapping backups, so you have to
> cross-check both start and stop LSNs.

Overall we have found it's much simpler to label each backup and 
cross-check that against the pg version and system id.  Start LSN is 
pretty unique, but backup labels work really well and are more widely 
understood.

>>>> (3) Cross-check a manifest against a backup and complain about extra
>>>> files, missing files, size differences, or checksum mismatches.
>>>
>>> Verification is the best part of the manifest.  Plus, you can do
>>> verification pretty cheaply on restore.  We also restore pg_control last
>>> so clusters that have a restore error won't start.
>>
>> There's no "restore" operation here, really. A backup taken by
>> pg_basebackup can be "restored" by copying the whole thing, but it can
>> also be used just where it is. If we were going to build something
>> into some in-core tool to copy backups around, this would be a smart
>> way to implement said tool, but I'm not planning on that myself.
> 
> Scratch that: incremental backups need a restore tool, so we can use
> this technique there. And it can work for full backups too, because
> why not?

Agreed, once we have a restore tool, use it for everything.

-- 
-David
david@pgmasters.net

On Thu, Sep 19, 2019 at 11:10:46PM -0400, David Steele wrote:
> On 9/19/19 11:00 AM, Robert Haas wrote:
>> On Thu, Sep 19, 2019 at 9:51 AM Robert Haas <robertmhaas@gmail.com> wrote:
>> > I intend that we should be able to support incremental backups based
>> > either on a previous full backup or based on a previous incremental
>> > backup. I am not aware of a technical reason why we need to identify
>> > the specific backup that must be used. If incremental backup B is
>> > taken based on a pre-existing backup A, then I think that B can be
>> > restored using either A or *any other backup taken after A and before
>> > B*. In the normal case, there probably wouldn't be any such backup,
>> > but AFAICS the start-LSNs are a sufficient cross-check that the chosen
>> > base backup is legal.
>>
>> Scratch that: there can be overlapping backups, so you have to
>> cross-check both start and stop LSNs.
>
> Overall we have found it's much simpler to label each backup and cross-check
> that against the pg version and system id.  Start LSN is pretty unique, but
> backup labels work really well and are more widely understood.

Warning.  The start LSN could be the same for multiple backups when
taken from a standby.
--
Michael

On Thu, Sep 19, 2019 at 11:10 PM David Steele <david@pgmasters.net> wrote:
> Overall we have found it's much simpler to label each backup and
> cross-check that against the pg version and system id.  Start LSN is
> pretty unique, but backup labels work really well and are more widely
> understood.

I see your point, but part of my point is that uniqueness is not a
technical requirement. However, it may be a requirement for user
comprehension.

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

On Thu, Sep 19, 2019 at 11:06 PM David Steele <david@pgmasters.net> wrote:
> > I am not crazy about JSON because it requires that I get a json parser
> > into src/common, which I could do, but given the possibly-imminent end
> > of the universe, I'm not sure it's the greatest use of time. You're
> > right that if we pick an ad-hoc format, we've got to worry about
> > escaping, which isn't lovely.
>
> My experience is that JSON is simple to implement and has already dealt
> with escaping and data structure considerations.  A home-grown solution
> will be at least as complex but have the disadvantage of being non-standard.

I think that's fair and just spent a little while investigating how
difficult it would be to disentangle the JSON parser from the backend.
It has dependencies on the following bits of backend-only
functionality:

- check_stack_depth(). No problem, I think.  Just skip it for frontend code.

- pg_mblen() / GetDatabaseEncoding(). Not sure what to do about this.
Some of our infrastructure for dealing with encoding is available in
the frontend and backend, but this part is backend-only.

- elog() / ereport(). Kind of a pain. We could just kill the program
if an error occurs, but that seems a bit ham-fisted. Refactoring the
code so that the error is returned rather than thrown might be the way
to go, but it's not simple, because you're not just passing a string.

                        ereport(ERROR,

(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
                                         errmsg("invalid input syntax
for type %s", "json"),
                                         errdetail("Character with
value 0x%02x must be escaped.",
                                                           (unsigned char) *s),
                                         report_json_context(lex)));

- appendStringInfo et. al. I don't think it would be that hard to move
this to src/common, but I'm also not sure it really solves the
problem, because StringInfo has a 1GB limit, and there's no rule at
all that a backup manifest has got to be less than 1GB.

https://www.pgcon.org/2013/schedule/events/595.en.html

This gets at another problem that I just started to think about. If
the file is just a series of lines, you can parse it one line and a
time and do something with that line, then move on. If it's a JSON
blob, you have to parse the whole file and get a potentially giant
data structure back, and then operate on that data structure. At
least, I think you do. There's probably some way to create a callback
structure that lets you presuppose that the toplevel data structure is
an array (or object) and get back each element of that array (or
key/value pair) as it's parsed, but that sounds pretty annoying to get
working. Or we could just decide that you have to have enough memory
to hold the parsed version of the entire manifest file in memory all
at once, and if you don't, maybe you should drop some tables or buy
more RAM. That still leaves you with bypassing the 1GB size limit on
StringInfo, maybe by having a "huge" option, or perhaps by
memory-mapping the file and then making the StringInfo point directly
into the mapped region. Perhaps I'm overthinking this and maybe you
have a simpler idea in mind about how it can be made to work, but I
find all this complexity pretty unappealing.

Here's a competing proposal: let's decide that lines consist of
tab-separated fields. If a field contains a \t, \r, or \n, put a " at
the beginning, a " at the end, and double any " that appears in the
middle. This is easy to generate and easy to parse. It lets us
completely ignore encoding considerations. Incremental parsing is
straightforward. Quoting will rarely be needed because there's very
little reason to create a file inside a PostgreSQL data directory that
contains a tab or a newline, but if you do it'll still work.  The lack
of quoting is nice for humans reading the manifest, and nice in terms
of keeping the manifest succinct; in contrast, note that using JSON
doubles every backslash.

I hear you saying that this is going to end up being just as complex
in the end, but I don't think I believe it.  It sounds to me like the
difference between spending a couple of hours figuring this out and
spending a couple of months trying to figure it out and maybe not
actually getting anywhere.

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

On Thu, Sep 19, 2019 at 11:06 PM David Steele <david@pgmasters.net> wrote:
> > I don't think it's a good idea to duplicate the information that's
> > already in the backup_label. Storing two copies of the same
> > information is just an invitation to having to worry about what
> > happens if they don't agree.
>
> OK, but now we have backup_label, tablespace_map,
> XXXXXXXXXXXXXXXXXXXXXXXX.XXXXXXXX.backup (in the WAL) and now perhaps a
> backup.manifest file.  I feel like we may be drowning in backup info files.

I agree!

I'm not sure what to do about it, though.  The information that is
present in the tablespace_map file could have been stored in the
backup_label file, I think, and that would have made sense, because
both files are serving a very similar purpose: they tell the server
that it needs to do some non-standard stuff when it starts up, and
they give it instructions for what those things are. And, as a
secondary purpose, humans or third-party tools can read them and use
that information for whatever purpose they wish.

The proposed backup_manifest file is a little different. I don't think
that anyone is proposing that the server should read that file: it is
there solely for the purpose of helping our own tools or third-party
tools or human beings who are, uh, acting like tools.[1] We're also
proposing to put it in a different place: the backup_label goes into
one of the tar files, but the backup_manifest would sit outside of any
tar file.

If we were designing this from scratch, maybe we'd roll all of this
into one file that serves as backup manifest, tablespace map, backup
label, and backup history file, but then again, maybe separating the
instructions-to-the-server part from the backup-integrity-checking
part makes sense.  At any rate, even if we knew for sure that's the
direction we wanted to go, getting there from here looks a bit rough.
If we just add a backup manifest, people who don't care can mostly
ignore it and then should be mostly fine. If we start trying to create
the one backup information system to rule them all, we're going to
break people's tools. Maybe that's worth doing someday, but the paint
isn't even dry on removing recovery.conf yet.

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

[1] There are a surprising number of installations where, in effect,
the DBA is the backup-and-restore tool, performing all the steps by
hand and hoping not to mess any of them up. The fact that nearly every
PostgreSQL company offers tools to make this easier does not seem to
have done a whole lot to diminish the number of people using ad-hoc
solutions.

On Fri, Sep 20, 2019 at 9:46 AM Robert Haas <robertmhaas@gmail.com> wrote:
> - appendStringInfo et. al. I don't think it would be that hard to move
> this to src/common, but I'm also not sure it really solves the
> problem, because StringInfo has a 1GB limit, and there's no rule at
> all that a backup manifest has got to be less than 1GB.

Hmm.  That's actually going to be a problem on the server side, no
matter what we do on the client side.  We have to send the manifest
after we send everything else, so that we know what we sent. But if we
sent a lot of files, the manifest might be really huge. I had been
thinking that we would generate the manifest on the server and send it
to the client after everything else, but maybe this is an argument for
generating the manifest on the client side and writing it
incrementally. That would require the client to peek at the contents
of every tar file it receives all the time, which it currently doesn't
need to do, but it does peek inside them a little bit, so maybe it's
OK.

Another alternative would be to have the server spill the manifest in
progress to a temp file and then stream it from there to the client.

Thoughts?

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

On 9/20/19 9:46 AM, Robert Haas wrote:
> On Thu, Sep 19, 2019 at 11:06 PM David Steele <david@pgmasters.net> wrote:
>
>> My experience is that JSON is simple to implement and has already dealt
>> with escaping and data structure considerations.  A home-grown solution
>> will be at least as complex but have the disadvantage of being non-standard.
>
> I think that's fair and just spent a little while investigating how
> difficult it would be to disentangle the JSON parser from the backend.
> It has dependencies on the following bits of backend-only
> functionality:

> - elog() / ereport(). Kind of a pain. We could just kill the program
> if an error occurs, but that seems a bit ham-fisted. Refactoring the
> code so that the error is returned rather than thrown might be the way
> to go, but it's not simple, because you're not just passing a string.

Seems to me we are overdue for elog()/ereport() compatible
error-handling in the front end.  Plus mem contexts.

It sucks to make that a prereq for this project but the longer we kick
that can down the road...

> https://www.pgcon.org/2013/schedule/events/595.en.html

This talk was good fun.  The largest number of tables we've seen is a
few hundred thousand, but that still adds up to more than a million
files to backup.

> This gets at another problem that I just started to think about. If
> the file is just a series of lines, you can parse it one line and a
> time and do something with that line, then move on. If it's a JSON
> blob, you have to parse the whole file and get a potentially giant
> data structure back, and then operate on that data structure. At
> least, I think you do. 

JSON can definitely be parsed incrementally, but for practical reasons
certain structures work better than others.

> There's probably some way to create a callback
> structure that lets you presuppose that the toplevel data structure is
> an array (or object) and get back each element of that array (or
> key/value pair) as it's parsed, but that sounds pretty annoying to get
> working. 

And that's how we do it.  It's annoying and yeah it's complicated but it
is very fast and memory-efficient.

> Or we could just decide that you have to have enough memory
> to hold the parsed version of the entire manifest file in memory all
> at once, and if you don't, maybe you should drop some tables or buy
> more RAM. 

I assume you meant "un-parsed" here?

> That still leaves you with bypassing the 1GB size limit on
> StringInfo, maybe by having a "huge" option, or perhaps by
> memory-mapping the file and then making the StringInfo point directly
> into the mapped region. Perhaps I'm overthinking this and maybe you
> have a simpler idea in mind about how it can be made to work, but I
> find all this complexity pretty unappealing.

Our String object has the same 1GB limit.  Partly because it works and
saves a bit of memory per object, but also because if we find ourselves
exceeding that limit we know we've probably made a design error.

Parsing in stream means that you only need to store the final in-memory
representation of the manifest which can be much more compact.  Yeah,
it's complicated, but the memory and time savings are worth it.

Note that our Perl implementation took the naive approach and has worked
pretty well for six years, but can choke on really large manifests with
out of memory errors.  Overall, I'd say getting the format right is more
important than having the perfect initial implementation.

> Here's a competing proposal: let's decide that lines consist of
> tab-separated fields. If a field contains a \t, \r, or \n, put a " at
> the beginning, a " at the end, and double any " that appears in the
> middle. This is easy to generate and easy to parse. It lets us
> completely ignore encoding considerations. Incremental parsing is
> straightforward. Quoting will rarely be needed because there's very
> little reason to create a file inside a PostgreSQL data directory that
> contains a tab or a newline, but if you do it'll still work.  The lack
> of quoting is nice for humans reading the manifest, and nice in terms
> of keeping the manifest succinct; in contrast, note that using JSON
> doubles every backslash.

There's other information you'll want to store that is not strictly file
info so you need a way to denote that.  It gets complicated quickly.

> I hear you saying that this is going to end up being just as complex
> in the end, but I don't think I believe it.  It sounds to me like the
> difference between spending a couple of hours figuring this out and
> spending a couple of months trying to figure it out and maybe not
> actually getting anywhere.

Maybe the initial implementation will be easier but I am confident we'll
pay for it down the road.  Also, don't we want users to be able to read
this file?  Do we really want them to need to cook up a custom parser in
Perl, Go, Python, etc.?

-- 
-David
david@pgmasters.net

On 9/20/19 10:59 AM, Robert Haas wrote:
> On Fri, Sep 20, 2019 at 9:46 AM Robert Haas <robertmhaas@gmail.com> wrote:
>> - appendStringInfo et. al. I don't think it would be that hard to move
>> this to src/common, but I'm also not sure it really solves the
>> problem, because StringInfo has a 1GB limit, and there's no rule at
>> all that a backup manifest has got to be less than 1GB.
> 
> Hmm.  That's actually going to be a problem on the server side, no
> matter what we do on the client side.  We have to send the manifest
> after we send everything else, so that we know what we sent. But if we
> sent a lot of files, the manifest might be really huge. I had been
> thinking that we would generate the manifest on the server and send it
> to the client after everything else, but maybe this is an argument for
> generating the manifest on the client side and writing it
> incrementally. That would require the client to peek at the contents
> of every tar file it receives all the time, which it currently doesn't
> need to do, but it does peek inside them a little bit, so maybe it's
> OK.
> 
> Another alternative would be to have the server spill the manifest in
> progress to a temp file and then stream it from there to the client.

This seems reasonable to me.

We keep an in-memory representation which is just an array of structs
and is fairly compact -- 1 million files uses ~150MB of memory.  We just
format and stream this to storage when saving.  Saving is easier than
loading, of course.

-- 
-David
david@pgmasters.net

On 9/20/19 9:46 AM, Robert Haas wrote:

> least, I think you do. There's probably some way to create a callback
> structure that lets you presuppose that the toplevel data structure is
> an array (or object) and get back each element of that array (or
> key/value pair) as it's parsed,

If a JSON parser does find its way into src/common, it probably wants
to have such an incremental mode available, similar to [2] offered
in the "Jackson" library for Java.

The Jackson developer has propounded a thesis[1] that such a parsing
library ought to offer "Three -- and Only Three" different styles of
API corresponding to three ways of organizing the code using the
library ([2], [3], [4], which also resemble the different APIs
supplied in Java for XML processing).

Regards,
-Chap


[1] http://www.cowtowncoder.com/blog/archives/2009/01/entry_132.html
[2] http://www.cowtowncoder.com/blog/archives/2009/01/entry_137.html
[3] http://www.cowtowncoder.com/blog/archives/2009/01/entry_153.html
[4] http://www.cowtowncoder.com/blog/archives/2009/01/entry_152.html

On Fri, Sep 20, 2019 at 11:09 AM David Steele <david@pgmasters.net> wrote:
> Seems to me we are overdue for elog()/ereport() compatible
> error-handling in the front end.  Plus mem contexts.
>
> It sucks to make that a prereq for this project but the longer we kick
> that can down the road...

There are no doubt many patches that would benefit from having more
backend infrastructure exposed in frontend contexts, and I think we're
slowly moving in that direction, but I generally do not believe in
burdening feature patches with major infrastructure improvements.
Sometimes it's necessary, as in the case of parallel query, which
required upgrading a whole lot of backend infrastructure in order to
have any chance of doing something useful. In most cases, however,
there's a way of getting the patch done that dodges the problem.

For example, I think there's a pretty good argument that Heikki's
design for relation forks was a bad one. It's proven to scale poorly
and create performance problems and extra complexity in quite a few
places. It would likely have been better, from a strictly theoretical
point of view, to insist on a design where the FSM and VM pages got
stored inside the relation itself, and the heap was responsible for
figuring out how various pages were being used. When BRIN came along,
we insisted on precisely that design, because it was clear that
further straining the relation fork system was not a good plan.
However, if we'd insisted on that when Heikki did the original work,
it might have delayed the arrival of the free space map for one or
more releases, and we got big benefits out of having that done sooner.
There's nothing stopping someone from writing a patch to get rid of
relation forks and allow a heap AM to have multiple relfilenodes (with
the extra ones used for the FSM and VM) or with multiplexing all the
data inside of a single file. Nobody has, though, because it's hard,
and the problems with the status quo are not so bad as to justify the
amount of development effort that would be required to fix it. At some
point, that problem is probably going to work its way to the top of
somebody's priority list, but it's already been about 10 years since
that all happened and everyone has so far dodged dealing with the
problem, which in turn has enabled them to work on other things that
are perhaps more important.

I think the same principle applies here. It's reasonable to ask the
author of a feature patch to fix issues that are closely related to
the feature in question, or even problems that are not new but would
be greatly exacerbated by the addition of the feature. It's not
reasonable to stack up a list of infrastructure upgrades that somebody
has to do as a condition of having a feature patch accepted that does
not necessarily require those upgrades. I am not convinced that JSON
is actually a better format for a backup manifest (more on that
below), but even if I were, I believe that getting a backup manifest
functionality into PostgreSQL 13, and perhaps incremental backup on
top of that, is valuable enough to justify making some compromises to
make that happen. And I don't mean "compromises" as in "let's commit
something that doesn't work very well;" rather, I mean making design
choices that are aimed at making the project something that is
feasible and can be completed in reasonable time, rather than not.

And saying, well, the backup manifest format *has* to be JSON because
everything else suxxor is not that. We don't have a single other
example of a file that we read and write in JSON format. Extension
control files use a custom format. Backup labels and backup history
files and timeline history files and tablespace map files use custom
formats. postgresql.conf, pg_hba.conf, and pg_ident.conf use custom
formats. postmaster.opts and postmaster.pid use custom formats. If
JSON is better and easier, at least one of the various people who
coded those things up would have chosen to use it, but none of them
did, and nobody's made a serious attempt to convert them to use it.
That might be because we lack the infrastructure for dealing with JSON
and building it is more work than anybody's willing to do, or it might
be because JSON is not actually better for these kinds of use cases,
but either way, it's hard to see why this particular patch should be
burdened with a requirement that none of the previous ones had to
satisfy.

Personally, I'd be intensely unhappy if a motion to convert
postgresql.conf or pg_hba.conf to JSON format gathered enough steam to
be adopted.  It would be darn useful, because you could specify
complex values for options instead of being limited to scalars, but it
would also make the configuration files a lot harder for human beings
to read and grep and the quality of error reporting would probably
decline significantly.  Also, appending a setting to the file,
something which is currently quite simple, would get a lot harder.
Ad-hoc file formats can be problematic, but they can also have real
advantages in terms of readability, brevity, and fitness for purpose.

> This talk was good fun.  The largest number of tables we've seen is a
> few hundred thousand, but that still adds up to more than a million
> files to backup.

A quick survey of some of my colleagues turned up a few examples of
people with 2-4 million files to backup, so similar kind of ballpark.
Probably not big enough for the manifest to hit the 1GB mark, but
getting close.

> > Or we could just decide that you have to have enough memory
> > to hold the parsed version of the entire manifest file in memory all
> > at once, and if you don't, maybe you should drop some tables or buy
> > more RAM.
>
> I assume you meant "un-parsed" here?

I don't think I meant that, although it seems like you might need to
store either all the parsed data or all the unparsed data or even
both, depending on exactly what you are trying to do.

> > I hear you saying that this is going to end up being just as complex
> > in the end, but I don't think I believe it.  It sounds to me like the
> > difference between spending a couple of hours figuring this out and
> > spending a couple of months trying to figure it out and maybe not
> > actually getting anywhere.
>
> Maybe the initial implementation will be easier but I am confident we'll
> pay for it down the road.  Also, don't we want users to be able to read
> this file?  Do we really want them to need to cook up a custom parser in
> Perl, Go, Python, etc.?

Well, I haven't heard anybody complain that they can't read a
backup_label file because it's too hard to cook up a parser.  And I
think the reason is pretty clear: such files are not hard to parse.
Similarly for a pg_hba.conf file.  This case is a little more
complicated than those, but AFAICS, not enormously so. Actually, it
seems like a combination of those two cases: it has some fixed
metadata fields that can be represented with one line per field, like
a backup_label, and then a bunch of entries for files that are
somewhat like entries in a pg_hba.conf file, in that they can be
represented by a line per record with a certain number of fields on
each line.

I attach here a couple of patches.  The first one does some
refactoring of relevant code in pg_basebackup, and the second one adds
checksum manifests using a format that I pulled out of my ear. It
probably needs some adjustment but I don't think it's crazy.  Each
file gets a line that looks like this:

File $FILENAME $FILESIZE $FILEMTIME $FILECHECKSUM

Right now, the file checksums are computed using SHA-256 but it could
be changed to anything else for which we've got code. On my system,
shasum -a256 $FILE produces the same answer that shows up here.  At
the bottom of the manifest there's a checksum of the manifest itself,
which looks like this:

Manifest-Checksum
385fe156a8c6306db40937d59f46027cc079350ecf5221027d71367675c5f781

That's a SHA-256 checksum of the file contents excluding the final
line. It can be verified by feeding all the file contents except the
last line to shasum -a256. I can't help but observe that if the file
were defined to be a JSONB blob, it's not very clear how you would
include a checksum of the blob contents in the blob itself, but with a
format based on a bunch of lines of data, it's super-easy to generate
and super-easy to write tools that verify it.

This is just a prototype so I haven't written a verification tool, and
there's a bunch of testing and documentation and so forth that would
need to be done aside from whatever we've got to hammer out in terms
of design issues and file formats.  But I think it's cool, and perhaps
some discussion of how it could be evolved will get us closer to a
resolution everybody can at least live with.

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

On 9/20/19 2:55 PM, Robert Haas wrote:
> On Fri, Sep 20, 2019 at 11:09 AM David Steele <david@pgmasters.net> wrote:
>>
>> It sucks to make that a prereq for this project but the longer we kick
>> that can down the road...
> 
> There are no doubt many patches that would benefit from having more
> backend infrastructure exposed in frontend contexts, and I think we're
> slowly moving in that direction, but I generally do not believe in
> burdening feature patches with major infrastructure improvements.

The hardest part about technical debt is knowing when to incur it.  It
is never a cut-and-dried choice.

>> This talk was good fun.  The largest number of tables we've seen is a
>> few hundred thousand, but that still adds up to more than a million
>> files to backup.
> 
> A quick survey of some of my colleagues turned up a few examples of
> people with 2-4 million files to backup, so similar kind of ballpark.
> Probably not big enough for the manifest to hit the 1GB mark, but
> getting close.

I have so many doubts about clusters with this many tables, but we do
support it, so...

>>> I hear you saying that this is going to end up being just as complex
>>> in the end, but I don't think I believe it.  It sounds to me like the
>>> difference between spending a couple of hours figuring this out and
>>> spending a couple of months trying to figure it out and maybe not
>>> actually getting anywhere.
>>
>> Maybe the initial implementation will be easier but I am confident we'll
>> pay for it down the road.  Also, don't we want users to be able to read
>> this file?  Do we really want them to need to cook up a custom parser in
>> Perl, Go, Python, etc.?
> 
> Well, I haven't heard anybody complain that they can't read a
> backup_label file because it's too hard to cook up a parser.  And I
> think the reason is pretty clear: such files are not hard to parse.
> Similarly for a pg_hba.conf file.  This case is a little more
> complicated than those, but AFAICS, not enormously so. Actually, it
> seems like a combination of those two cases: it has some fixed
> metadata fields that can be represented with one line per field, like
> a backup_label, and then a bunch of entries for files that are
> somewhat like entries in a pg_hba.conf file, in that they can be
> represented by a line per record with a certain number of fields on
> each line.

Yeah, they are not hard to parse, but *everyone* has to cook up code for
it.  A bit of a bummer, that.

> I attach here a couple of patches.  The first one does some
> refactoring of relevant code in pg_basebackup, and the second one adds
> checksum manifests using a format that I pulled out of my ear. It
> probably needs some adjustment but I don't think it's crazy.  Each
> file gets a line that looks like this:
> 
> File $FILENAME $FILESIZE $FILEMTIME $FILECHECKSUM

We also include page checksum validation failures in the file record.
Not critical for the first pass, perhaps, but something to keep in mind.

> Right now, the file checksums are computed using SHA-256 but it could
> be changed to anything else for which we've got code. On my system,
> shasum -a256 $FILE produces the same answer that shows up here.  At
> the bottom of the manifest there's a checksum of the manifest itself,
> which looks like this:
> 
> Manifest-Checksum
> 385fe156a8c6306db40937d59f46027cc079350ecf5221027d71367675c5f781
> 
> That's a SHA-256 checksum of the file contents excluding the final
> line. It can be verified by feeding all the file contents except the
> last line to shasum -a256. I can't help but observe that if the file
> were defined to be a JSONB blob, it's not very clear how you would
> include a checksum of the blob contents in the blob itself, but with a
> format based on a bunch of lines of data, it's super-easy to generate
> and super-easy to write tools that verify it.

You can do this in JSON pretty easily by handling the terminating
brace/bracket:

{
<some json contents>*,
"checksum":<sha256>
}

But of course a linefeed-delimited file is even easier.

> This is just a prototype so I haven't written a verification tool, and
> there's a bunch of testing and documentation and so forth that would
> need to be done aside from whatever we've got to hammer out in terms
> of design issues and file formats.  But I think it's cool, and perhaps
> some discussion of how it could be evolved will get us closer to a
> resolution everybody can at least live with.

I had a quick look and it seems pretty reasonable.  I'll need to
generate a manifest to see if I can spot any obvious gotchas.

-- 
-David
david@pgmasters.net

On Sat, Sep 21, 2019 at 12:25 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
Some comments:

Manifest file will be in plain text format even if compression is
specified, should we compress it?
May be this is intended, just raised the point to make sure that it is intended.
+static void
+ReceiveBackupManifestChunk(size_t r, char *copybuf, void *callback_data)
+{
+ WriteManifestState *state = callback_data;
+
+ if (fwrite(copybuf, r, 1, state->file) != 1)
+ {
+ pg_log_error("could not write to file \"%s\": %m", state->filename);
+ exit(1);
+ }
+}

WALfile.done file gets added but wal file information is not included
in the manifest file, should we include WAL file also?
@@ -599,16 +618,20 @@ perform_base_backup(basebackup_options *opt)
  (errcode_for_file_access(),
  errmsg("could not stat file \"%s\": %m", pathbuf)));

- sendFile(pathbuf, pathbuf, &statbuf, false, InvalidOid);
+ sendFile(pathbuf, pathbuf, &statbuf, false, InvalidOid, manifest,
+ NULL);

  /* unconditionally mark file as archived */
  StatusFilePath(pathbuf, fname, ".done");
- sendFileWithContent(pathbuf, "");
+ sendFileWithContent(pathbuf, "", manifest);

Should we add an option to make manifest generation configurable to
reduce overhead during backup?

Manifest file does not include directory information, should we include it?

There is one warning:
In file included from ../../../src/include/fe_utils/string_utils.h:20:0,
                 from pg_basebackup.c:34:
pg_basebackup.c: In function ‘ReceiveTarFile’:
../../../src/interfaces/libpq/pqexpbuffer.h:60:9: warning: the
comparison will always evaluate as ‘false’ for the address of ‘buf’
will never be NULL [-Waddress]
  ((str) == NULL || (str)->maxlen == 0)
         ^
pg_basebackup.c:1203:7: note: in expansion of macro ‘PQExpBufferBroken’
   if (PQExpBufferBroken(&buf))

pg_gmtime can fail in case of malloc failure:
+ /*
+ * Convert time to a string. Since it's not clear what time zone to use
+ * and since time zone definitions can change, possibly causing confusion,
+ * use GMT always.
+ */
+ pg_strftime(timebuf, sizeof(timebuf), "%Y-%m-%d %H:%M:%S %Z",
+ pg_gmtime(&mtime));

Regards,
Vignesh
EnterpriseDB: http://www.enterprisedb.com

On Mon, Sep 30, 2019 at 5:31 AM Jeevan Chalke
<jeevan.chalke@enterprisedb.com> wrote:
> Entry for directory is not added in manifest. So it might be difficult
> at client to get to know about the directories. Will it be good to add
> an entry for each directory too? May be like:
> Dir    <dirname> <mtime>

Well, what kind of corruption would this allow us to detect that we
can't detect as things stand? I think the only case is an empty
directory. If it's not empty, we'd have some entries for the files in
that directory, and those files won't be able to exist unless the
directory does. But, how would we end up backing up an empty
directory, anyway?

I don't really *mind* adding directories into the manifest, but I'm
not sure how much it helps.

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

On 11/19/19 5:00 AM, Rushabh Lathia wrote:
>
>
> My colleague Suraj did testing and noticed the performance impact
> with the checksums.   On further testing, he found that specifically with
> sha its more of performance impact.  
>
>

I admit I haven't been following along closely, but why do we need a
cryptographic checksum here instead of, say, a CRC? Do we think that
somehow the checksum might be forged? Use of cryptographic hashes as
general purpose checksums has become far too common IMNSHO.


cheers


andrew


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

On 11/19/19 5:00 AM, Rushabh Lathia wrote:
> 
> My colleague Suraj did testing and noticed the performance impact
> with the checksums.   On further testing, he found that specifically with
> sha its more of performance impact.  

We have found that SHA1 adds about 3% overhead when the backup is also
compressed (gzip -6), which is what most people want to do.  This
percentage goes down even more if the backup is being transferred over a
network or to an object store such as S3.

We judged that the lower collision rate of SHA1 justified the additional
expense.

That said, making SHA256 optional seems reasonable.  We decided not to
make our SHA1 checksums optional to reduce the test matrix and because
parallelism largely addressed performance concerns.

Regards,
-- 
-David
david@pgmasters.net

On Tue, Nov 19, 2019 at 8:49 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:
> I admit I haven't been following along closely, but why do we need a
> cryptographic checksum here instead of, say, a CRC? Do we think that
> somehow the checksum might be forged? Use of cryptographic hashes as
> general purpose checksums has become far too common IMNSHO.

I tend to agree with you. I suspect if we just use CRC, some people
are going to complain that they want something "stronger" because that
will make them feel better about error detection rates or obscure
threat models or whatever other things a SHA-based approach might be
able to catch that CRC would not catch. However, I suspect that for
normal use cases, CRC would be totally adequate, and the fact that the
performance overhead is almost none vs. a whole lot - at least in this
test setup, other results might vary depending on what you test -
makes it look pretty appealing.

My gut reaction is to make CRC the default, but have an option that
you can use to either turn it off entirely (if even 1-2% is too much
for you) or opt in to SHA-something if you want it. I don't think we
should offer an option for MD5, because MD5 is a dirty word these days
and will cause problems for users who have to worry about FIPS 140-2
compliance. Phrased more positively, if you want a cryptographic hash
at all, you should probably use one that isn't widely viewed as too
weak.

Thoughts?

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

On 11/22/19 10:58 AM, Robert Haas wrote:
> On Tue, Nov 19, 2019 at 8:49 AM Andrew Dunstan
> <andrew.dunstan@2ndquadrant.com> wrote:
>> I admit I haven't been following along closely, but why do we need a
>> cryptographic checksum here instead of, say, a CRC? Do we think that
>> somehow the checksum might be forged? Use of cryptographic hashes as
>> general purpose checksums has become far too common IMNSHO.
> 
> I tend to agree with you. I suspect if we just use CRC, some people
> are going to complain that they want something "stronger" because that
> will make them feel better about error detection rates or obscure
> threat models or whatever other things a SHA-based approach might be
> able to catch that CRC would not catch. 

Well, the maximum amount of data that can be protected with a 32-bit CRC
is 512MB according to all the sources I found (NIST, Wikipedia, etc).  I
presume that's what we are talking about since I can't find any 64-bit
CRC code in core or this patch.

So, that's half of what we need with the default relation segment size
(I've seen larger in the field).

> I don't think we
> should offer an option for MD5, because MD5 is a dirty word these days
> and will cause problems for users who have to worry about FIPS 140-2
> compliance. 

+1.

> Phrased more positively, if you want a cryptographic hash
> at all, you should probably use one that isn't widely viewed as too
> weak.

Sure.  There's another advantage to picking an algorithm with lower
collision rates, though.

CRCs are fine for catching transmission errors (as caveated above) but
not as great for comparing two files for equality.  With strong hashes
you can confidently compare local files against the path, size, and hash
stored in the manifest and save yourself a round-trip to the remote
storage to grab the file if it has not changed locally.

This is the basic premise of what we call delta restore which can speed
up restores by orders of magnitude.

Delta restore is the main advantage that made us decide to require SHA1
checksums.  In most cases, restore speed is more important than backup
speed.

Regards,
-- 
-David
david@pgmasters.net

On Tue, Nov 19, 2019 at 4:34 PM David Steele <david@pgmasters.net> wrote:
> On 11/19/19 5:00 AM, Rushabh Lathia wrote:
> > My colleague Suraj did testing and noticed the performance impact
> > with the checksums.   On further testing, he found that specifically with
> > sha its more of performance impact.
>
> We have found that SHA1 adds about 3% overhead when the backup is also
> compressed (gzip -6), which is what most people want to do.  This
> percentage goes down even more if the backup is being transferred over a
> network or to an object store such as S3.

I don't really understand why your tests and Suraj's tests are showing
such different results, or how compression plays into it. I tried
running shasum -a$N lineitem-big.csv on my laptop, where that file
contains ~70MB of random-looking data whose source I no longer
remember. Here are the results by algorithm: SHA1, ~25 seconds; SHA224
or SHA256, ~52 seconds; SHA384 and SHA512, ~39 seconds. Aside from the
interesting discovery that the algorithms with more bits actually run
faster on this machine, this seems to show that there's only about a
~2x difference between the SHA1 that you used and that I (pretty much
arbitrarily) used. But Rushabh and Suraj are reporting 43-54%
overhead, and even if you divide that by two it's a lot more than 3%.

One possible explanation is that the compression is really slow, and
so it makes the checksum overhead a smaller percentage of the total.
Like, if you've already slowed down the backup by 8x, then 24%
overhead turns into 3% overhead! But I assume that's not the real
explanation here. Another explanation is that your tests were
I/O-bound rather than CPU-bound, maybe because you tested with a much
larger database or a much smaller amount of I/O bandwidth. If you had
CPU cycles to burn, then neither compression nor checksums will cost
much in terms of overall runtime. But that's a little hard to swallow,
too, because I don't think the testing mentioned above was done using
any sort of exotic test configuration, so why would yours be so
different? Another possibility is that Suraj and Rushabh messed up the
tests, or alternatively that you did. Or, it could be that your
checksum implementation is way faster than the one PG uses, and so the
impact was much less. I don't know, but I'm having a hard time
understanding the divergent results. Any ideas?

> We judged that the lower collision rate of SHA1 justified the additional
> expense.
>
> That said, making SHA256 optional seems reasonable.  We decided not to
> make our SHA1 checksums optional to reduce the test matrix and because
> parallelism largely addressed performance concerns.

Just to be clear, I really don't have any objection to using SHA1
instead of SHA256, or anything else for that matter. I picked the one
to use out of a hat for the purpose of having a POC quickly; I didn't
have any intention to insist on that as the final selection. It seems
likely that anything we pick here will eventually be considered
obsolete, so I think we need to allow for configurability, but I don't
have a horse in the game as far as an initial selection goes.

Except - and this gets back to the previous point - I don't want to
slow down backups by 40% by default. I wouldn't mind slowing them down
3% by default, but 40% is too much overhead. I think we've gotta
either the overhead of using SHA way down or not use SHA by default.

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

On Fri, Nov 22, 2019 at 1:10 PM David Steele <david@pgmasters.net> wrote:
> Well, the maximum amount of data that can be protected with a 32-bit CRC
> is 512MB according to all the sources I found (NIST, Wikipedia, etc).  I
> presume that's what we are talking about since I can't find any 64-bit
> CRC code in core or this patch.

Could you give a more precise citation for this? I can't find a
reference to that in the Wikipedia article off-hand and I don't know
where to look in NIST. I apologize if I'm being dense here, but I
don't see why there should be any limit on the amount of data that can
be protected. The important thing is that if the original file F is
altered to F', we hope that CHECKSUM(F) != CHECKSUM(F'). The
probability of that, assuming that the alteration is random rather
than malicious and that the checksum function is equally likely to
produce every possible output, is just 1-2^-${CHECKSUM_BITS},
regardless of the length of the message (except that there might be
some special cases for very short messages, which don't matter here).

This analysis by me seems to match
https://en.wikipedia.org/wiki/Cyclic_redundancy_check, which says:

"Typically an n-bit CRC applied to a data block of arbitrary length
will detect any single error burst not longer than n bits, and the
fraction of all longer error bursts that it will detect is (1 −
2^−n)."

Notice the phrase "a data block of arbitrary length" and the formula "1 - 2^-n".

> > Phrased more positively, if you want a cryptographic hash
> > at all, you should probably use one that isn't widely viewed as too
> > weak.
>
> Sure.  There's another advantage to picking an algorithm with lower
> collision rates, though.
>
> CRCs are fine for catching transmission errors (as caveated above) but
> not as great for comparing two files for equality.  With strong hashes
> you can confidently compare local files against the path, size, and hash
> stored in the manifest and save yourself a round-trip to the remote
> storage to grab the file if it has not changed locally.

I agree in part. I think there are two reasons why a cryptographically
strong hash is desirable for delta restore. First, since the checksums
are longer, the probability of a false match happening randomly is
lower, which is important. Even if the above analysis is correct and
the chance of a false match is just 2^-32 with a 32-bit CRC, if you
back up ten million files every day, you'll likely get a false match
within a few years or less, and once is too often. Second, unlike what
I supposed above, the contents of a PostgreSQL data file are not
chosen at random, unlike transmission errors, which probably are more
or less random. It seems somewhat possible that there is an adversary
who is trying to choose the data that gets stored in some particular
record so as to create a false checksum match. A CRC is a lot easier
to fool than a crytographic hash, so I think that using a CRC of *any*
length for this kind of use case would be extremely dangerous no
matter the probability of an accidental match.

> This is the basic premise of what we call delta restore which can speed
> up restores by orders of magnitude.
>
> Delta restore is the main advantage that made us decide to require SHA1
> checksums.  In most cases, restore speed is more important than backup
> speed.

I see your point, but it's not the whole story. We've encountered a
bunch of cases where the time it took to complete a backup exceeded
the user's desired backup interval, which is obviously very bad, or
even more commonly where it exceeded the length of the user's
"low-usage" period when they could tolerate the extra overhead imposed
by the backup. A few percentage points is probably not a big deal, but
a user who has an 8-hour window to get the backup done overnight will
not be happy if it's taking 6 hours now and we tack 40%-50% on to
that. So I think that we either have to disable backup checksums by
default, or figure out a way to get the overhead down to something a
lot smaller than what current tests are showing -- which we could
possibly do without changing the algorithm if we can somehow make it a
lot cheaper, but otherwise I think the choice is between disabling the
functionality altogether by default and adopting a less-expensive
algorithm. Maybe someday when delta restore is in core and widely used
and CPUs are faster, it'll make sense to revise the default, and
that's cool, but I can't see imposing a big overhead by default to
enable a feature core doesn't have yet...

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

On 11/22/19 1:24 PM, Robert Haas wrote:
> On Tue, Nov 19, 2019 at 4:34 PM David Steele <david@pgmasters.net> wrote:
>> On 11/19/19 5:00 AM, Rushabh Lathia wrote:
>>> My colleague Suraj did testing and noticed the performance impact
>>> with the checksums.   On further testing, he found that specifically with
>>> sha its more of performance impact.
>>
>> We have found that SHA1 adds about 3% overhead when the backup is also
>> compressed (gzip -6), which is what most people want to do.  This
>> percentage goes down even more if the backup is being transferred over a
>> network or to an object store such as S3.
> 
> I don't really understand why your tests and Suraj's tests are showing
> such different results, or how compression plays into it. I tried
> running shasum -a$N lineitem-big.csv on my laptop, where that file
> contains ~70MB of random-looking data whose source I no longer
> remember. Here are the results by algorithm: SHA1, ~25 seconds; SHA224
> or SHA256, ~52 seconds; SHA384 and SHA512, ~39 seconds. Aside from the
> interesting discovery that the algorithms with more bits actually run
> faster on this machine, this seems to show that there's only about a
> ~2x difference between the SHA1 that you used and that I (pretty much
> arbitrarily) used. But Rushabh and Suraj are reporting 43-54%
> overhead, and even if you divide that by two it's a lot more than 3%.
> 
> One possible explanation is that the compression is really slow, and
> so it makes the checksum overhead a smaller percentage of the total.
> Like, if you've already slowed down the backup by 8x, then 24%
> overhead turns into 3% overhead! But I assume that's not the real
> explanation here. 

That's the real explanation here.  Hash calculations run at the same
speed, they just become a smaller portion of the *total* time once
compression (gzip -6) is added.  With something like lz4 hashing will
obviously be a big percentage of the total.

Also consider how much extra latency you get from copying over a
network.  My 3% did not include that but realistically most backups are
running over a network (hopefully).

>> That said, making SHA256 optional seems reasonable.  We decided not to
>> make our SHA1 checksums optional to reduce the test matrix and because
>> parallelism largely addressed performance concerns.
> 
> Just to be clear, I really don't have any objection to using SHA1
> instead of SHA256, or anything else for that matter. I picked the one
> to use out of a hat for the purpose of having a POC quickly; I didn't
> have any intention to insist on that as the final selection. It seems
> likely that anything we pick here will eventually be considered
> obsolete, so I think we need to allow for configurability, but I don't
> have a horse in the game as far as an initial selection goes.

We decided that SHA1 was good enough and there was no need to go up to
SHA256.  What we were interested in was collision rates and what the
chance of getting a false positive were based on the combination of
path, size, and hash.  With SHA1 the chance of a collision was literally
astronomically low (as in the universe would probably end before it
happened, depending on whether you are an expand forever or contract
proponent).

> Except - and this gets back to the previous point - I don't want to
> slow down backups by 40% by default. I wouldn't mind slowing them down
> 3% by default, but 40% is too much overhead. I think we've gotta
> either the overhead of using SHA way down or not use SHA by default.

Maybe -- my take is that the measurements, an uncompressed backup to the
local filesystem, are not a very realistic use case.

However, I'm still fine with leaving the user the option of checksums or
no.  I just wanted to point out that CRCs have their limits so maybe
that's not a great option unless it is properly caveated and perhaps not
the default.

Regards,
-- 
-David
david@pgmasters.net

On 11/22/19 2:01 PM, Robert Haas wrote:
> On Fri, Nov 22, 2019 at 1:10 PM David Steele <david@pgmasters.net> wrote:
>> Well, the maximum amount of data that can be protected with a 32-bit CRC
>> is 512MB according to all the sources I found (NIST, Wikipedia, etc).  I
>> presume that's what we are talking about since I can't find any 64-bit
>> CRC code in core or this patch.
> 
> Could you give a more precise citation for this? 

See:
https://www.nist.gov/system/files/documents/2017/04/26/lrdc_systems_part2_032713.pdf
Search for "The maximum block size"

https://en.wikipedia.org/wiki/Cyclic_redundancy_check
"The design of the CRC polynomial depends on the maximum total length of
the block to be protected (data + CRC bits)", which I took to mean there
are limits.

Here another interesting bit from:
https://en.wikipedia.org/wiki/Mathematics_of_cyclic_redundancy_checks
"Because a CRC is based on division, no polynomial can detect errors
consisting of a string of zeroes prepended to the data, or of missing
leading zeroes" -- but it appears to matter what CRC you are using.
There's a variation that works in this case and hopefully we are using
that one.

This paper talks about appropriate block lengths vs crc length:
http://users.ece.cmu.edu/~koopman/roses/dsn04/koopman04_crc_poly_embedded.pdf
but it is concerned with network transmission and small block lengths.

> "Typically an n-bit CRC applied to a data block of arbitrary length
> will detect any single error burst not longer than n bits, and the
> fraction of all longer error bursts that it will detect is (1 −
> 2^−n)."

I'm not sure how encouraging I find this -- a four-byte error not a lot
and 2^32 is only 4 billion.  We have individual users who have backed up
more than 4 billion files over the last few years.

>> This is the basic premise of what we call delta restore which can speed
>> up restores by orders of magnitude.
>>
>> Delta restore is the main advantage that made us decide to require SHA1
>> checksums.  In most cases, restore speed is more important than backup
>> speed.
> 
> I see your point, but it's not the whole story. We've encountered a
> bunch of cases where the time it took to complete a backup exceeded
> the user's desired backup interval, which is obviously very bad, or
> even more commonly where it exceeded the length of the user's
> "low-usage" period when they could tolerate the extra overhead imposed
> by the backup. A few percentage points is probably not a big deal, but
> a user who has an 8-hour window to get the backup done overnight will
> not be happy if it's taking 6 hours now and we tack 40%-50% on to
> that. So I think that we either have to disable backup checksums by
> default, or figure out a way to get the overhead down to something a
> lot smaller than what current tests are showing -- which we could
> possibly do without changing the algorithm if we can somehow make it a
> lot cheaper, but otherwise I think the choice is between disabling the
> functionality altogether by default and adopting a less-expensive
> algorithm. Maybe someday when delta restore is in core and widely used
> and CPUs are faster, it'll make sense to revise the default, and
> that's cool, but I can't see imposing a big overhead by default to
> enable a feature core doesn't have yet...

OK, I'll buy that.  But I *don't* think CRCs should be allowed for
deltas (when we have them) and I *do* think we should caveat their
effectiveness (assuming we can agree on them).

In general the answer to faster backups should be more cores/faster
network/faster disk, not compromising backup integrity.  I understand
we'll need to wait until we have parallelism in pg_basebackup to justify
that answer.

Regards,
-- 
-David
david@pgmasters.net

Moin Robert,

On 2019-11-22 20:01, Robert Haas wrote:
> On Fri, Nov 22, 2019 at 1:10 PM David Steele <david@pgmasters.net> 
> wrote:
>> Well, the maximum amount of data that can be protected with a 32-bit 
>> CRC
>> is 512MB according to all the sources I found (NIST, Wikipedia, etc).  
>> I
>> presume that's what we are talking about since I can't find any 64-bit
>> CRC code in core or this patch.
> 
> Could you give a more precise citation for this? I can't find a
> reference to that in the Wikipedia article off-hand and I don't know
> where to look in NIST. I apologize if I'm being dense here, but I
> don't see why there should be any limit on the amount of data that can
> be protected. The important thing is that if the original file F is
> altered to F', we hope that CHECKSUM(F) != CHECKSUM(F'). The
> probability of that, assuming that the alteration is random rather
> than malicious and that the checksum function is equally likely to
> produce every possible output, is just 1-2^-${CHECKSUM_BITS},
> regardless of the length of the message (except that there might be
> some special cases for very short messages, which don't matter here).
> 
> This analysis by me seems to match
> https://en.wikipedia.org/wiki/Cyclic_redundancy_check, which says:
> 
> "Typically an n-bit CRC applied to a data block of arbitrary length
> will detect any single error burst not longer than n bits, and the
> fraction of all longer error bursts that it will detect is (1 −
> 2^−n)."
> 
> Notice the phrase "a data block of arbitrary length" and the formula "1 
> - 2^-n".

It is related to the number of states, and the birthday problem factors 
in it, too:

    https://en.wikipedia.org/wiki/Birthday_problem

If you have a 32 bit checksum or hash, it can represent only 2**32-1 
states at most (or less, if the
algorithmn isn't really good).

Each byte is 8 bit, so 2 ** 32 / 8 is 512 Mbyte. If you process your 
data bit by bit, each
new bit would add a new state (consider: missing bit == 0, added bit == 
1). If each new state
is repesented by a different checksum, all possible 2 ** 32 values are 
exhausted after
processing 512 Mbyte, after that you get one of the former states again 
- aka a collision.

There is no way around it with so little bits, no matter what algorithmn 
you choose.

>> > Phrased more positively, if you want a cryptographic hash
>> > at all, you should probably use one that isn't widely viewed as too
>> > weak.
>> 
>> Sure.  There's another advantage to picking an algorithm with lower
>> collision rates, though.
>> 
>> CRCs are fine for catching transmission errors (as caveated above) but
>> not as great for comparing two files for equality.  With strong hashes
>> you can confidently compare local files against the path, size, and 
>> hash
>> stored in the manifest and save yourself a round-trip to the remote
>> storage to grab the file if it has not changed locally.
> 
> I agree in part. I think there are two reasons why a cryptographically
> strong hash is desirable for delta restore. First, since the checksums
> are longer, the probability of a false match happening randomly is
> lower, which is important. Even if the above analysis is correct and
> the chance of a false match is just 2^-32 with a 32-bit CRC, if you
> back up ten million files every day, you'll likely get a false match
> within a few years or less, and once is too often. Second, unlike what
> I supposed above, the contents of a PostgreSQL data file are not
> chosen at random, unlike transmission errors, which probably are more
> or less random. It seems somewhat possible that there is an adversary
> who is trying to choose the data that gets stored in some particular
> record so as to create a false checksum match. A CRC is a lot easier
> to fool than a crytographic hash, so I think that using a CRC of *any*
> length for this kind of use case would be extremely dangerous no
> matter the probability of an accidental match.

Agreed. See above.

However, if you choose a hash, please do not go below SHA-256. Both MD5
and SHA-1 already had collision attacks, and these only got to be bound
to be worse.

   https://www.mscs.dal.ca/~selinger/md5collision/
   https://shattered.io/

It might even be a wise idea to encode the used Hash-Algorithm into the
manifest file, so it can be changed later. The hash length might be not
enough to decide which algorithm is the one used.

>> This is the basic premise of what we call delta restore which can 
>> speed
>> up restores by orders of magnitude.
>> 
>> Delta restore is the main advantage that made us decide to require 
>> SHA1
>> checksums.  In most cases, restore speed is more important than backup
>> speed.
> 
> I see your point, but it's not the whole story. We've encountered a
> bunch of cases where the time it took to complete a backup exceeded
> the user's desired backup interval, which is obviously very bad, or
> even more commonly where it exceeded the length of the user's
> "low-usage" period when they could tolerate the extra overhead imposed
> by the backup. A few percentage points is probably not a big deal, but
> a user who has an 8-hour window to get the backup done overnight will
> not be happy if it's taking 6 hours now and we tack 40%-50% on to
> that. So I think that we either have to disable backup checksums by
> default, or figure out a way to get the overhead down to something a
> lot smaller than what current tests are showing -- which we could
> possibly do without changing the algorithm if we can somehow make it a
> lot cheaper, but otherwise I think the choice is between disabling the
> functionality altogether by default and adopting a less-expensive
> algorithm. Maybe someday when delta restore is in core and widely used
> and CPUs are faster, it'll make sense to revise the default, and
> that's cool, but I can't see imposing a big overhead by default to
> enable a feature core doesn't have yet...

Modern algorithms are amazingly fast on modern hardware, some even
are implemented in hardware nowadays:

  https://software.intel.com/en-us/articles/intel-sha-extensions

Quote from:

  
https://neosmart.net/blog/2017/will-amds-ryzen-finally-bring-sha-extensions-to-intels-cpus/

  "Despite the extremely limited availability of SHA extension support
   in modern desktop and mobile processors, crypto libraries have already
   upstreamed support to great effect. Botan’s SHA extension patches show 
a
   significant 3x to 5x performance boost when taking advantage of the 
hardware
   extensions, and the Linux kernel itself shipped with hardware SHA 
support
   with version 4.4, bringing a very respectable 3.6x performance upgrade 
over
   the already hardware-assisted SSE3-enabled code."

If you need to load the data from disk and shove it over a network, the
hashing will certainly be very little overhead, it might even be 
completely
invisible, since it can run in paralell to all the other things. Sure, 
there
is the thing called zero-copy-networking, but if you have to compress 
the
data bevore sending it to the network, you have to put it through the 
CPU,
anyway. And if you have more than one core, the second one can to the
hashing it paralell to the first one doing the compression.

To get a feeling one can use:

    openssl speed md5 sha1 sha256 sha512

On my really-not-fast desktop CPU (i5-4690T CPU @ 2.50GHz) it says:

  The 'numbers' are in 1000s of bytes per second processed.
   type       16 bytes     64 bytes    256 bytes   1024 bytes   8192 
bytes  16384 bytes
   md5       122638.55k   277023.96k   487725.57k   630806.19k   
683892.74k   688553.98k
   sha1      127226.45k   313891.52k   632510.55k   865753.43k   
960995.33k   977215.19k
   sha256     77611.02k   173368.15k   325460.99k   412633.43k   
447022.92k   448020.48k
   sha512     51164.77k   205189.87k   361345.79k   543883.26k   
638372.52k   645933.74k

Or in other words, it can hash nearly 931 MByte /s with SHA-1 and about
427 MByte / s with SHA-256 (if I haven't miscalculated something). You'd
need a
pretty fast disk (aka M.2 SSD) and network (aka > 1 Gbit) to top these 
speeds
and then you'd use a real CPU for your server, not some poor Intel 
powersaving
surfing thingy-majingy :)

Best regards,

Tels

On 11/22/19 5:15 PM, Tels wrote:
> On 2019-11-22 20:01, Robert Haas wrote:
>> On Fri, Nov 22, 2019 at 1:10 PM David Steele <david@pgmasters.net> wrote:
> 
>>> > Phrased more positively, if you want a cryptographic hash
>>> > at all, you should probably use one that isn't widely viewed as too
>>> > weak.
>>>
>>> Sure.  There's another advantage to picking an algorithm with lower
>>> collision rates, though.
>>>
>>> CRCs are fine for catching transmission errors (as caveated above) but
>>> not as great for comparing two files for equality.  With strong hashes
>>> you can confidently compare local files against the path, size, and hash
>>> stored in the manifest and save yourself a round-trip to the remote
>>> storage to grab the file if it has not changed locally.
>>
>> I agree in part. I think there are two reasons why a cryptographically
>> strong hash is desirable for delta restore. First, since the checksums
>> are longer, the probability of a false match happening randomly is
>> lower, which is important. Even if the above analysis is correct and
>> the chance of a false match is just 2^-32 with a 32-bit CRC, if you
>> back up ten million files every day, you'll likely get a false match
>> within a few years or less, and once is too often. Second, unlike what
>> I supposed above, the contents of a PostgreSQL data file are not
>> chosen at random, unlike transmission errors, which probably are more
>> or less random. It seems somewhat possible that there is an adversary
>> who is trying to choose the data that gets stored in some particular
>> record so as to create a false checksum match. A CRC is a lot easier
>> to fool than a crytographic hash, so I think that using a CRC of *any*
>> length for this kind of use case would be extremely dangerous no
>> matter the probability of an accidental match.
> 
> Agreed. See above.
> 
> However, if you choose a hash, please do not go below SHA-256. Both MD5
> and SHA-1 already had collision attacks, and these only got to be bound
> to be worse.

I don't think collision attacks are a big consideration in the general
case.  The manifest is generally stored with the backup files so if a
file is modified it is then trivial to modify the manifest as well.

Of course, you could store the manifest separately or even just know the
hash of the manifest and store that separately.  In that case SHA-256
might be useful and it would be good to have the option, which I believe
is the plan.

I do wonder if you could construct a successful collision attack (even
in MD5) that would also result in a valid relation file.  Probably, at
least eventually.

Regards,
-- 
-David
david@pgmasters.net

Moin,

On 2019-11-22 23:30, David Steele wrote:
> On 11/22/19 5:15 PM, Tels wrote:
>> On 2019-11-22 20:01, Robert Haas wrote:
>>> On Fri, Nov 22, 2019 at 1:10 PM David Steele <david@pgmasters.net> 
>>> wrote:
>> 
>>>> > Phrased more positively, if you want a cryptographic hash
>>>> > at all, you should probably use one that isn't widely viewed as too
>>>> > weak.
>>>> 
>>>> Sure.  There's another advantage to picking an algorithm with lower
>>>> collision rates, though.
>>>> 
>>>> CRCs are fine for catching transmission errors (as caveated above) 
>>>> but
>>>> not as great for comparing two files for equality.  With strong 
>>>> hashes
>>>> you can confidently compare local files against the path, size, and 
>>>> hash
>>>> stored in the manifest and save yourself a round-trip to the remote
>>>> storage to grab the file if it has not changed locally.
>>> 
>>> I agree in part. I think there are two reasons why a 
>>> cryptographically
>>> strong hash is desirable for delta restore. First, since the 
>>> checksums
>>> are longer, the probability of a false match happening randomly is
>>> lower, which is important. Even if the above analysis is correct and
>>> the chance of a false match is just 2^-32 with a 32-bit CRC, if you
>>> back up ten million files every day, you'll likely get a false match
>>> within a few years or less, and once is too often. Second, unlike 
>>> what
>>> I supposed above, the contents of a PostgreSQL data file are not
>>> chosen at random, unlike transmission errors, which probably are more
>>> or less random. It seems somewhat possible that there is an adversary
>>> who is trying to choose the data that gets stored in some particular
>>> record so as to create a false checksum match. A CRC is a lot easier
>>> to fool than a crytographic hash, so I think that using a CRC of 
>>> *any*
>>> length for this kind of use case would be extremely dangerous no
>>> matter the probability of an accidental match.
>> 
>> Agreed. See above.
>> 
>> However, if you choose a hash, please do not go below SHA-256. Both 
>> MD5
>> and SHA-1 already had collision attacks, and these only got to be 
>> bound
>> to be worse.
> 
> I don't think collision attacks are a big consideration in the general
> case.  The manifest is generally stored with the backup files so if a
> file is modified it is then trivial to modify the manifest as well.

That is true. However, a simple way around this is to sign the manifest
with a public key l(GPG or similiar). And if the manifest contains
strong, hard-to-forge hashes, we got a mure more secure backup, where
(almost) nobody else can alter the manifest, nor can he mount easy
collision attacks against the single files.

Without the strong hashes it would be pointless to sign the manifest.

> Of course, you could store the manifest separately or even just know 
> the
> hash of the manifest and store that separately.  In that case SHA-256
> might be useful and it would be good to have the option, which I 
> believe
> is the plan.
> 
> I do wonder if you could construct a successful collision attack (even
> in MD5) that would also result in a valid relation file.  Probably, at
> least eventually.

With MD5, certainly. One way is to have two block of 512 bits that hash
to the different MD5s. It is trivial to re-use one already existing from
the known examples.

Here is one, where the researchers constructed 12 PDFs that all
have the same MD5 hash:

   https://www.win.tue.nl/hashclash/Nostradamus/

If you insert one of these blocks into a relation and dump it, you could
swap it (probably?) out on disk for the other block. I'm not sure this
is of practical usage as an attack, tho. It would, however, cast doubt
on the integrity of the backup and prove that MD5 is useless.

OTOH, finding a full collision with MD5 should also be in reach with
todays hardware. It is hard find exact numbers but this:

    https://www.win.tue.nl/hashclash/SingleBlock/

gives the following numbers for 2008/2009:

   "Finding the birthday bits took 47 hours (expected was 3 days) on the
   cluster of 215 Playstation 3 game consoles at LACAL, EPFL. This is
   roughly equivalent to 400,000 hours on a single PC core. The single
   near-collision block construction took 18 hours and 20 minutes on a
   single PC core."

Today one can probably compute it on a single GPU in mere hours. And you
can rent massive amounts of them in the cloud for real cheap.

Here are a few, now a bit dated, references:

    https://blog.codinghorror.com/speed-hashing/
    http://codahale.com/how-to-safely-store-a-password/

Best regards,

Tels

On 11/23/19 3:13 AM, Tels wrote:
>
> Without the strong hashes it would be pointless to sign the manifest.
>
>

I guess I must have missed where we are planning to add a cryptographic
signature.


cheers


andrew


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

On 11/23/19 4:34 PM, Andrew Dunstan wrote:
> 
> On 11/23/19 3:13 AM, Tels wrote:
>>
>> Without the strong hashes it would be pointless to sign the manifest.
>>
> 
> I guess I must have missed where we are planning to add a cryptographic
> signature.

I don't think we were planning to, but the user could do so if they wished.

-- 
-David
david@pgmasters.net

On 2019-11-24 15:38, David Steele wrote:
> On 11/23/19 4:34 PM, Andrew Dunstan wrote:
>> 
>> On 11/23/19 3:13 AM, Tels wrote:
>>> 
>>> Without the strong hashes it would be pointless to sign the manifest.
>>> 
>> 
>> I guess I must have missed where we are planning to add a 
>> cryptographic
>> signature.
> 
> I don't think we were planning to, but the user could do so if they 
> wished.

That was what I meant.

Best regards,

Tels

On Fri, Nov 22, 2019 at 2:29 PM David Steele <david@pgmasters.net> wrote:
> See:
> https://www.nist.gov/system/files/documents/2017/04/26/lrdc_systems_part2_032713.pdf
> Search for "The maximum block size"

Hmm, so it says: "The maximum block size that can be protected by a
32-bit CRC is 512MB." My problem is that (1) it doesn't back this up
with a citation or any kind of logical explanation and (2) it's not
very clear what "protected" means. Tels replies downthread to explain
that the internal state of the 32-bit CRC calculation is also limited
to 32 bits, and changes once per bit, so that after processing 512MB =
2^29 bytes = 2^32 bits of data, you're guaranteed to start repeating
internal states. Perhaps this is also what the NIST folks had in mind,
though it's hard to know.

This link provides some more details:

https://community.arm.com/developer/tools-software/tools/f/keil-forum/17467/crc-for-256-byte-data

Not everyone on the thread agrees with everybody else, but it seems
like there are size limits below which a CRC-n is guaranteed to detect
all 1-bit and 2-bit errors, and above which this is no longer
guaranteed. They put the limit *lower* than what NIST supposes, namely
2^(n-1)-1 bits, which would be 256MB, not 512MB, if I'm doing math
correctly. However, they also say that above that value, you are still
likely to detect most errors. Absent an intelligent adversary, the
chance of a random collision when corruption is present is still about
1 in 4 billion (2^-32).

To me, guaranteed detection of 1-bit and 2-bit errors (and the other
kinds of specific things CRC is designed to catch) doesn't seem like a
principle design consideration. It's nice if we can get it and I'm not
against it, but these are algorithms that are designed to be used when
data undergoes a digital-to-analog-to-digital conversion, where for
example it's possible that that the conversion back to digital loses
sync and reads 9 bits or 7 bits rather than 8 bits. And that's not
really what we're doing here: we all know that bits get flipped
sometimes, but nobody uses scp to copy a 1GB file and ends up with a
file that is 1GB +/- a few bits. Some lower-level part of the
communication stack is handling that part of the work; you're going to
get exactly 1GB. So it seems to me that here, as with XLOG, we're not
relying on the specific CRC properties that were intended to be used
to catch and in some cases repair bit flips caused by wrinkles in an
A-to-D conversion, but just on its general tendency to probably not
match if any bits got flipped. And those properties hold regardless of
input length.

That being said, having done some reading on this, I am a little
concerned that we're getting further and further from the design
center of the CRC algorithm. Like relation segment files, XLOG records
are not packets subject to bit insertions, but at least they're small,
and relation files are not. Using a 40-year-old algorithm that was
intended to be used for things like making sure the modem hadn't lost
framing in the last second to verify 1GB files feels, in some nebulous
way, like we might be stretching. That being said, I'm not sure what
we think the reasonable alternatives are. Users aren't going to be
better off if we say that, because CRC-32C might not do a great job
detecting errors, we're not going to check for errors at all. If we go
the other way and say we're going to use some variant of SHA, they
will be better off, but at the price of what looks like a
*significant* hit in terms of backup time.

> > "Typically an n-bit CRC applied to a data block of arbitrary length
> > will detect any single error burst not longer than n bits, and the
> > fraction of all longer error bursts that it will detect is (1 −
> > 2^−n)."
>
> I'm not sure how encouraging I find this -- a four-byte error not a lot
> and 2^32 is only 4 billion.  We have individual users who have backed up
> more than 4 billion files over the last few years.

I agree that people have a lot more than 4 billion files backed up,
but I'm not sure it matters very much given the use case I'm trying to
enable. There's a lot of difference between delta restore and backup
integrity checking. For backup integrity checking, my goal is that, on
those occasions when a file gets corrupted, the chances that we notice
that it has been corrupted. For that purpose, a 32-bit checksum is
probably sufficient. If a file gets corrupted, we have about a
1-in-4-billion chance of being unable to detect it. If 4 billion files
get corrupted, we'll miss, on average, one of those corruption events.
That's sad, but so is the fact that you had *4 billion corrupted
files*. This is not the total number of files backed up; this is the
number of those that got corrupted. I don't really know how common it
is to copy a file and end up with a corrupt copy, but if you say it's
one-in-a-million, which I suspect is far too high, then you'd have to
back up something like 4 quadrillion files before you missed a
corruption event, and that's a *very* big number.

Now delta restore is a whole different kettle of fish. The birthday
problem is huge here. If you've got a 32-bit checksum for file A, and
you go and look it up in a database of checksums, and that database
has even 1 billion things in it, you've got a pretty decent shot of
latching onto a file that is not actually the same as file A. The
problem goes away almost entirely if you only compare against previous
versions of that file from that database cluster. You've probably only
got tens or maybe at the very outside hundreds or thousands of backups
of that particular file, and a collision is unlikely even with only a
32-bit checksum -- though even there maybe you'd like to use something
larger just to be on the safe side. But if you're going to compare to
other files from the same cluster, or even worse any file from any
cluster, 32 bits is *woefully* inadequate. TBH even using SHA for such
use cases feels a little scary to me. It's probably good enough --
2^160 for SHA-1 is a *lot* bigger than 2^32, and 2^512 for SHA-512 is
enormous. But I'd want to spend time thinking very carefully about the
math before designing such a system.

> OK, I'll buy that.  But I *don't* think CRCs should be allowed for
> deltas (when we have them) and I *do* think we should caveat their
> effectiveness (assuming we can agree on them).

Sounds good.

> In general the answer to faster backups should be more cores/faster
> network/faster disk, not compromising backup integrity.  I understand
> we'll need to wait until we have parallelism in pg_basebackup to justify
> that answer.

I would like to dispute that characterization of what we're talking
about here. If we added a 1-bit checksum (parity bit) it would be
*strictly better* than what we're doing right now, which is nothing.
That's not a serious proposal because it's obvious we can do a lot
better for trivial additional cost, but deciding that we're going to
use a weaker kind of checksum to avoid adding too much overhead is not
wimping out, because it's still going to be strong enough to catch the
overwhelming majority of problems that go undetected today. Even an
*8-bit* checksum would give us a >99% chance of catching a corrupted
file, which would be noticeably better than the 0% chance we have
today. Even a manifest with no checksums at all that just checked the
presence and size of files would catch tons of operator error, e.g.

- wait, that database had tablespaces?
- were those logs in pg_clog anything important?
- oh, i wasn't supposed to start postgres on the copy of the database
stored in the backup directory?

So I don't think we're talking about whether to compromise backup
integrity. I think we're talking about - if we're going to make backup
integrity better than it is today, how much better should we try to
make it, and what are the trade-offs there? The straw man here is that
we could make the database infinitely secure if we put it in a
concrete bunker and sunk it to the bottom of the ocean, with the small
price that we'd no longer be able to access it either. Somewhere
between that extreme and the other extreme of setting the
authentication method to 0.0.0.0/0 trust there's a happy medium where
security is tolerably good but ease of access isn't crippled, and the
same thing applies here. We could (probably) be the first database on
the planet to store a 1024-bit encrypted checksum of every 8kB block,
but that seems like it's going too far in the "concrete bunker"
direction. IMHO, at least, we should be aiming for something that has
a high probability of catching real problems and a low probability of
being super-annoying.

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

On Fri, Nov 22, 2019 at 2:02 PM David Steele <david@pgmasters.net> wrote:
> > Except - and this gets back to the previous point - I don't want to
> > slow down backups by 40% by default. I wouldn't mind slowing them down
> > 3% by default, but 40% is too much overhead. I think we've gotta
> > either the overhead of using SHA way down or not use SHA by default.
>
> Maybe -- my take is that the measurements, an uncompressed backup to the
> local filesystem, are not a very realistic use case.

Well, compression is a feature we don't have yet, in core. So for
people who are only using core tools, an uncompressed backup is a very
realistic use case, because it's the only kind they can get. Granted
the situation is different if you are using pgbackrest.

I don't have enough experience to know how often people back up to
local filesystems vs. remote filesystems mounted locally vs. overtly
over-the-network. I sometimes get the impression that users choose
their backup tools and procedures with, as Tom would say, the aid of a
dart board, but that's probably the cynic in me talking. Or maybe a
reflection of the fact that I usually end up talking to the users for
whom things have gone really, really badly wrong, rather than the ones
for whom things went as planned.

> However, I'm still fine with leaving the user the option of checksums or
> no.  I just wanted to point out that CRCs have their limits so maybe
> that's not a great option unless it is properly caveated and perhaps not
> the default.

I think the default is the sticking point here. To me, it looks like
CRC is a better default than nothing at all because it should still
catch a high percentage of issues that would otherwise be missed, and
a better default than SHA because it's so cheap to compute. However,
I'm certainly willing to consider other theories.

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

On Fri, Nov 22, 2019 at 5:15 PM Tels <nospam-pg-abuse@bloodgate.com> wrote:
> It is related to the number of states...

Thanks for this explanation. See my reply to David where I also
discuss this point.

> However, if you choose a hash, please do not go below SHA-256. Both MD5
> and SHA-1 already had collision attacks, and these only got to be bound
> to be worse.
>
>    https://www.mscs.dal.ca/~selinger/md5collision/
>    https://shattered.io/

Yikes, that second link, about SHA-1, is depressing. Now, it's not
likely that an attacker has access to your backup repository and can
spend 6500 years of CPU time to engineer a Trojan file there (maybe
more, because the files are probably bigger than the PDFs they used in
that case) and then induce you to restore and rely upon that backup.
However, it's entirely likely that somebody is going to eventually ban
SHA-1 as the attacks get better, which is going to be a problem for us
whether the underlying exposures are problems or not.

> It might even be a wise idea to encode the used Hash-Algorithm into the
> manifest file, so it can be changed later. The hash length might be not
> enough to decide which algorithm is the one used.

I agree. Let's write
SHA256:bc1c3a57369acd0d2183a927fb2e07acbbb1c97f317bbc3b39d93ec65b754af5
or similar rather than just the hash. That way even if the entire SHA
family gets cracked, we can easily substitute in something else that
hasn't been cracked yet.

(It is unclear to me why anyone supposes that *any* popular hash
function won't eventually be cracked. For a K-bit hash function, there
are 2^K possible outputs, where K is probably in the hundreds. But
there are 2^{2^33} possible 1GB files. So for every possible output
value, there are 2^{2^33-K} inputs that produce that value, which is a
very very big number. The probability that any given input produces a
certain output is very low, but the number of possible inputs that
produce a given output is very high; so assuming that nobody's ever
going to figure out how to construct them seems optimistic.)

> To get a feeling one can use:
>
>     openssl speed md5 sha1 sha256 sha512
>
> On my really-not-fast desktop CPU (i5-4690T CPU @ 2.50GHz) it says:
>
>   The 'numbers' are in 1000s of bytes per second processed.
>    type       16 bytes     64 bytes    256 bytes   1024 bytes   8192
> bytes  16384 bytes
>    md5       122638.55k   277023.96k   487725.57k   630806.19k
> 683892.74k   688553.98k
>    sha1      127226.45k   313891.52k   632510.55k   865753.43k
> 960995.33k   977215.19k
>    sha256     77611.02k   173368.15k   325460.99k   412633.43k
> 447022.92k   448020.48k
>    sha512     51164.77k   205189.87k   361345.79k   543883.26k
> 638372.52k   645933.74k
>
> Or in other words, it can hash nearly 931 MByte /s with SHA-1 and about
> 427 MByte / s with SHA-256 (if I haven't miscalculated something). You'd
> need a
> pretty fast disk (aka M.2 SSD) and network (aka > 1 Gbit) to top these
> speeds
> and then you'd use a real CPU for your server, not some poor Intel
> powersaving
> surfing thingy-majingy :)

I mean, how fast is in theory doesn't matter nearly as much as what
happens when you benchmark the proposed implementation, and the
results we have so far don't support the theory that this is so cheap
as to be negligible.

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

On Wed, Dec 4, 2019 at 1:01 PM Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
> As per the  discussion on the thread, here is the patch which
>
> a) Make checksum for manifest file optional.
> b) Allow user to choose a particular algorithm.
>
> Currently with the WIP patch SHA256 and CRC checksum algorithm
> supported.  Patch also changed the manifest file format to append
> the used algorithm name before the checksum, this way it will be
> easy to validator to know which algorithm to used.
>
> Ex:
> ./db/bin/pg_basebackup -D bksha/ --manifest-with-checksums=SHA256
>
> $ cat bksha/backup_manifest  | more
> PostgreSQL-Backup-Manifest-Version 1
> File backup_label 226 2019-12-04 17:46:46 GMT
SHA256:7cf53d1b9facca908678ab70d93a9e7460cd35cedf7891de948dcf858f8a281a
> File pg_xact/0000 8192 2019-12-04 17:46:46 GMT
SHA256:8d2b6cb1dc1a6e8cee763b52d75e73571fddce06eb573861d44082c7d8c03c26
>
> ./db/bin/pg_basebackup -D bkcrc/ --manifest-with-checksums=CRC
> PostgreSQL-Backup-Manifest-Version 1
> File backup_label 226 2019-12-04 17:58:40 GMT CRC:343138313931333134
> File pg_xact/0000 8192 2019-12-04 17:46:46 GMT CRC:363538343433333133
>
> Pending TODOs:
> - Documentation update
> - Code cleanup
> - Testing.
>
> I will further continue to work on the patch and meanwhile feel free to provide
> thoughts/inputs.

+ initilize_manifest_checksum(&cCtx);

Spelling.

-

Spurious.

+ case MC_CRC:
+ INIT_CRC32C(cCtx->crc_ctx);

Suggest that we do CRC -> CRC32C throughout the patch. Someone might
conceivably want some other CRC variant, mostly likely 64-bit, in the
future.

+final_manifest_checksum(ChecksumCtx *cCtx, char *checksumbuf)

finalize

  printf(_("      --manifest-with-checksums\n"
- "                         do calculate checksums for manifest files\n"));
+ "                         calculate checksums for manifest files
using provided algorithm\n"));

Switch name is wrong. Suggest --manifest-checksums.
Help usually shows that an argument is expected, e.g.
--manifest-checksums=ALGORITHM or
--manifest-checksums=sha256|crc32c|none

This seems to apply over some earlier version of the patch.  A
consolidated patch, or the whole stack, would be better.

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

On Thu, Dec 5, 2019 at 11:22 AM Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
> Here is the whole stack of patches.

Please include proper attribution and, where somebody's written them,
commit messages in each patch in the stack. For example, I see that
your 0001 is mostly the same as my 0001 from upthread, but now it
says:

From a3e075d5edb5031ea358e049f8cb07031fc480a3 Mon Sep 17 00:00:00 2001
From: Rushabh Lathia <rushabh.lathia@enterprisedb.com>
Date: Wed, 13 Nov 2019 15:19:22 +0530
Subject: [PATCH 1/5] Reduce code duplication and eliminate weird macro tricks.

...with no indication of who the original author was.

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

On Thu, Dec 5, 2019 at 11:22 AM Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
> Here is the whole stack of patches.

I committed 0001, as that's just refactoring and I think (hope) it's
uncontroversial. I think 0002-0005 need to be squashed together
(crediting all authors properly and in the appropriate order) as it's
quite hard to understand right now, and that Suraj's patch to validate
the backup should be included in the patch stack. It needs
documentation. Also, we need, either in that patch or a separate, TAP
tests that exercise this feature. Things we should try to check:

- Plain format backups can be verified against the manifest.
- Tar format backups can be verified against the manifest after
untarring (this might be a problem; not sure there's any guarantee
that we have a working "tar" command available).
- Verification succeeds for all available checksums algorithms and
also for no checksum algorithm (should still check which files are
present, and sizes).
- If we tamper with a backup by removing a file, adding a file, or
changing the size of a file, the modification is detected even without
checksums.
- If we tamper with a backup by changing the contents of a file but
not the size, the modification is detected if checksums are used.
- Everything above still works if there is user-defined tablespace
that contains a table.

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

On Fri, Dec 6, 2019 at 1:35 AM Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
> There is one review comment from Jeevan Chalke, which still pending
> to address is:
>
>> 4.
>> Why we need a "File" at the start of each entry as we are adding files only?
>> I wonder if we also need to provide a tablespace name and directory marker so
>> that we have "Tablespace" and "Dir" at the start.
>
> Sorry, I am not quite sure about this, may be Robert is right person
> to answer this.

I did it that way for extensibility. Notice that the first and last
line of the manifest begin with other words, so someone parsing the
manifest can identify the line type by looking just at the first word.
Someone might in the future find some need to add other kinds of lines
that don't exist today.

"Tablespace" and "Dir" are, in fact, pretty good examples of things
that someone might want to add in the future. I don't really see a
clear need for either one today, although maybe somebody else will,
but I think we should leave ourselves room to add such things in the
future.

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

On Tue, Dec 10, 2019 at 6:40 AM Suraj Kharage
<suraj.kharage@enterprisedb.com> wrote:
> Please find attached patch for backup validator implementation (0004 patch). This patch is based
> on Rushabh's latest patch for backup manifest.
>
> There are some functions required at client side as well, so I have moved those functions
> and some data structure at common place so that they can be accessible for both. (0003 patch).
>
> My colleague Rajkumar Raghuwanshi has prepared the WIP patch (0005) for tap test cases which
> is also attached. As of now, test cases related to the tablespace and tar backup  format are missing,
> will continue work on same and submit the complete patch.
>
> With this mail, I have attached the complete patch stack for backup manifest and backup
> validate implementation.
>
> Please let me know your thoughts on the same.

Well, for the second time on this thread, please don't take a bunch of
somebody else's code and post it in a patch that doesn't attribute
that person as one of the authors. For the second time on this thread,
the person is me, but don't borrow *anyone's* code without proper
attribution. It's really important!

On a related note, it's a very good idea to use git format-patch and
git rebase -i to maintain patch stacks like this. Rushabh seems to
have done that, but the files you're posting look like raw 'git diff'
output. Notice that this gives him a way to include authorship
information and a tentative commit message in each patch, but you
don't have any of that.

Also on a related note, part of the process of adapting existing code
to a new purpose is adapting the comments. You haven't done that:

+ * Search a result-set hash table for a row matching a given filename.
...
+ * Insert a row into a result-set hash table, provided no such row is already
...
+ * Most of the values
+ * that we're hashing are short integers formatted as text, so there
+ * shouldn't be much room for pathological input.

I think that what we should actually do here is try to use simplehash.
Right now, it won't work for frontend code, but I posted some patches
to try to address that issue:

https://www.postgresql.org/message-id/CA%2BTgmob8oyh02NrZW%3DxCScB%2B5GyJ-jVowE3%2BTWTUmPF%3DFsGWTA%40mail.gmail.com

That would have a few advantages. One, we wouldn't need to know the
number of elements in advance, because simplehash can grow
dynamically. Two, we could use the iteration interfaces to walk the
hash table.  Your solution to that is pgrhash_seq_search, but that's
actually not well-designed, because it's not a generic iterator
function but something that knows specifically about the 'touch' flag.
I incidentally suggest renaming 'touch' to 'matched;' 'touch' is not
bad, but I think 'matched' will be a little more recognizable.

Please run pgindent. If needed, first add locally defined types to
typedefs.list, so that things indent properly.

It's not a crazy idea to try to share some data structures and code
between the frontend and the backend here, but I think
src/common/backup.c and src/include/common/backup.h is a far too
generic name given what the code is actually doing. It's mostly about
checksums, not backup, and I think it should be named accordingly. I
suggest removing "manifestinfo" and renaming the rest to just talk
about checksums rather than manifests. That would make it logical to
reuse this for any other future code that needs a configurable
checksum type. Also, how about adding a function like:

extern bool parse_checksum_algorithm(char *name, ChecksumAlgorithm *algo);

...which would return true and set *algo if name is recognized, and
return false otherwise. That code could be used on both the client and
server sides of this patch, and by any future patches that want to
return this scaffolding.

The file header for backup.h has the wrong filename (string.h). The
header format looks somewhat atypical compared to what we normally do,
too.

It's arguable, but I tend to think that it would be better to
hex-encode the CRC rather than printing it as an integer.  Maybe
hex_encode() is another thing that could be moved into the new
src/common file.

As I said before about Rushabh's patch set, it's very confusing that
we have so many patches here stacked up. Like, you have 0002 moving
stuff, and then 0003 moving it again. That's super-confusing. Please
try to structure the patch set so as to make it as easy to review as
possible.

Regarding the test case patch, error checks are important! Don't do
things like this:

+open my $modify_file_sha256, '>>', "$tempdir/backup_verify/postgresql.conf";
+print $modify_file_sha256 "port = 5555\n";
+close $modify_file_sha256;

If the open fails, then it and the print and the close are going to
silently do nothing. That's bad. I don't know exactly what the
customary error-checking is for things like this in TAP tests, but I
hope it's not like this, because this has a pretty fair chance of
looking like it's testing something that it isn't. Let's figure out
what the best practice in this area is and adhere to it.

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

On Tue, Dec 17, 2019 at 12:54 AM Suraj Kharage
<suraj.kharage@enterprisedb.com> wrote:
> I have implemented the simplehash in backup validator patch as Robert suggested. Please find attached 0002 patch for
thesame.
 
>
> kindly review and let me know your thoughts.

+#define CHECKSUM_LENGTH 256

This seems wrong. Not all checksums are the same length, and none of
the ones we're using are 256 bytes in length, and if we've got to have
a constant someplace for the maximum checksum length, it should
probably be in the new header file, not here. But I don't think we
should need this in the first place; see comments below about how to
revise the parsing of the manifest file.

+    char        filetype[10];

A mysterious 10-byte field with no comments explaining what it
means... and the same magic number 10 appears in at least one other
place in the patch.

+typedef struct manifesthash_hash *hashtab;

This declares a new *type* called hashtab, not a variable called
hashtab. The new type is not used anywhere, but later, you have
several variables of the same type that have this name. Just remove
this: it's wrong and unused.

+static enum ChecksumAlgorithm checksum_type = MC_NONE;

Remove "enum". Not needed, because you have a typedef for it in the
header, and not per style.

+static  manifesthash_hash *create_manifest_hash(char manifest_path[MAXPGPATH]);

Whitespace is wrong. The whole patch needs a visit from pgindent with
a properly-updated typedefs.list.

Also, you will struggle to find anywhere else in the code base where
pass a character array as a function argument. I don't know why this
isn't just char *.

+    if(verify_backup)

Whitespace wrong here, too.

+ * Read the backup_manifest file and generate the hash table, then scan data
+ * directroy and verify each file. Finally, iterate on hash table to find
+ * out missing files.

You've got a word spelled wrong here, but the bigger problem is that
this comment doesn't actually describe what this function is trying to
do. Instead, it describes how it does it. If it's necessary to explain
what steps the function takes in order to accomplish some goal, you
should comment individual bits of code in the function. The header
comment is a high-level overview, not a description of the algorithm.

It's also pretty unhelpful, here and elsewhere, to refer to "the hash
table" as if there were only one, and as if the reader were supposed
to know something about it when you haven't told them anything about
it.

+        if (!entry->matched)
+        {
+            pg_log_info("missing file: %s", entry->filename);
+        }
+

The braces here are not project style. We usually omit braces when
only a single line of code is present.

I think some work needs to be done to standardize and improve the
messages that get produced here.  You have:

1. missing file: %s
2. duplicate file present: %s
3. size changed for file: %s, original size: %d, current size: %zu
4. checksum difference for file: %s
5. extra file found: %s

I suggest:

1. file \"%s\" is present in manifest but missing from the backup
2. file \"%s\" has multiple manifest entries
(this one should probably be pg_log_error(), not pg_log_info(), as it
represents a corrupt-manifest problem)
3. file \"%s" has size %lu in manifest but size %lu in backup
4. file \"%s" has checksum %s in manifest but checksum %s in backup
5. file \"%s" is present in backup but not in manifest

Your patch actually doesn't compile on my system, because for the
third message above, it uses %zu to print the size. But %zu is for
size_t, not off_t. I went looking for other places in the code where
we print off_t; based on that, I think the right thing to do is to
print it using %lu and write (unsigned long) st.st_size.

+    char        file_checksum[256];
+    char        header[1024];

More arbitrary constants.

+    if (!file)
+    {
+        pg_log_error("could not open backup_manifest");

That's bad error reporting.  See e.g. readfile() in initdb.c.

+    if (fscanf(file, "%1023[^\n]\n", header) != 1)
+    {
+        pg_log_error("error while reading the header from backup_manifest");

That's also bad error reporting. It is only a slight step up from
"ERROR: error".

And we have another magic number (1023).

+    appendPQExpBufferStr(manifest, header);
+    appendPQExpBufferStr(manifest, "\n");
...
+        appendPQExpBuffer(manifest, "File\t%s\t%d\t%s\t%s\n", filename,
+                          filesize, mtime, checksum_with_type);

This whole thing seems completely crazy to me. Basically, you're
trying to use fscanf() to parse the file. But then, because fscanf()
doesn't give you the original bytes back, you're trying to reassemble
the data that you parsed to recover the original line, so that you can
stuff it in the buffer and eventually checksum it. However, that's
highly error-prone. You're basically duplicating server code, and thus
risking getting out of sync in the server code, to work around a
problem that is entirely self-inflicted, namely, deciding to use
fscanf().

What I would recommend is:

1. Use open(), read(), close() rather than the fopen() family of
functions. As we have discovered elsewhere, fread() doesn't promise to
set errno, so we can't necessarily get reliable error-reporting out of
it.

2. Before you start reading the file, create a buffer that's large
enough to hold the whole thing, by using fstat() to figure out how big
the file is. Read the whole file into that buffer.  If you're not able
to read the whole file -- i.e. open() or read() or close() fail --
then just error out and exit.

3. Now advance through the file line by line. Write a function that
knows how to search forward for the next \r or \n but with checks to
make sure it can't run off the end of the buffer, and use that to
locate the end of each line so that you can walk forward. As you walk
forward line by line, add the line you just processed to the checksum.
That way, you only need a single pass over the data. Also, you can
modify it in place.  More on that below.

4. As you examine each line, start by examining the first word. You'll
need a function that finds the first word by searching forward for a
tab character, but not beyond the end of the line. The first word of
the first line should be PostgreSQL-Backup-Manifest-Version and the
second word should be 1. Then on each subsequent line check whether
the first word is File or Manifest-Checksum or something else,
erroring out in the last case. If it's Manifest-Checksum, verify that
this is the last line of the file and that the checksum matches. If
it's File, break the line into fields so you can add it to the hash
table. You'll want a pointer to the filename and a pointer to the
checksum, and you'll want to parse the size as an integer. Instead of
allocating new memory for those fields, just overwrite the character
that follows the field with a \0. There must be one - either \t or \n
- so you shouldn't run off the end of the buffer.

If you do this, a bunch of the fixed-size buffers you have right now
go away. You don't need the variable filetype[10] any more, or
checksum_with_type[CHECKSUM_LENGTH], or checksum[CHECKSUM_LENGTH], or
the character arrays inside DataDirectoryFileInfo. Instead you can
just have pointers into the buffer that contains the file. And you
don't need this code to back up using fseek() and reread the lines,
either.

Also read this article:

https://stackoverflow.com/questions/2430303/disadvantages-of-scanf

Note that the very first point in the article talks about the problem
of overrunning the buffer, which you certainly have in the current
code right here:

+        if (fscanf(file, "%s\t%s\t%d\t%23[^\t] %s\n", filetype, filename,

filetype is declared as char[10], but %s could read arbitrarily much data.

+        filename = (char*) pg_malloc(MAXPGPATH);

pg_malloc returns void *, so no cast is required.

+        if (strcmp(checksum_with_type, "-") == 0)
+        {
+            checksum_type = MC_NONE;
+        }
+        else
+        {
+            if (strncmp(checksum_with_type, "SHA256", 6) == 0)

Use parse_checksum_algorithm. Right now you've invented a "common"
function with 1 caller, but I explicitly suggested previously that you
put it in common so that you could reuse it.

+        if (strcmp(de->d_name, ".") == 0 || strcmp(de->d_name, "..") == 0 ||
+            strcmp(de->d_name, "pg_wal") == 0)
+            continue;

Ignoring pg_wal at the top level might be OK, but this will ignore a
pg_wal entry anywhere in the directory tree.

+    /* Skip backup manifest file. */
+    if (strcmp(de->d_name, "backup_manifest") == 0)
+        return;

Same problem.

+    filename = createPQExpBuffer();
+    if (!filename)
+    {
+        pg_log_error("out of memory");
+        exit(1);
+    }
+
+    appendPQExpBuffer(filename, "%s%s", relative_path, de->d_name);

Just use char filename[MAXPGPATH] and snprintf here, as you do
elsewhere. It will be simpler and save memory.

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

On Fri, Dec 20, 2019 at 8:24 AM Suraj Kharage
<suraj.kharage@enterprisedb.com> wrote:
> Thank you for review comments.

Thanks for the new version.

+      <term><option>--verify-backup </option></term>

Whitespace.

+struct manifesthash_hash *hashtab;

Uh, I had it in mind that you would nuke this line completely, not
just remove "typedef" from it. You shouldn't need a global variable
here.

+ if (buf == NULL)

pg_malloc seems to have an internal check such that it never returns
NULL. I don't see anything like this test in other callers.

The order of operations in create_manifest_hash() seems unusual:

+ fd = open(manifest_path, O_RDONLY, 0);
+ if (fstat(fd, &stat))
+ buf = pg_malloc(stat.st_size);
+ hashtab = manifesthash_create(1024, NULL);
...
+ entry = manifesthash_insert(hashtab, filename, &found);
...
+ close(fd);

I would have expected open-fstat-read-close to be consecutive, and the
manifesthash stuff all done afterwards. In fact, it seems like reading
the file could be a separate function.

+ if (strncmp(checksum, "SHA256", 6) == 0)

This isn't really right; it would give a false match if we had a
checksum algorithm with a name like SHA2560 or SHA256C or
SHA256ExceptWayBetter. The right thing to do is find the colon first,
and then probably overwrite it with '\0' so that you have a string
that you can pass to parse_checksum_algorithm().

+ /*
+ * we don't have checksum type in the header, so need to
+ * read through the first file enttry to find the checksum
+ * type for the manifest file and initilize the checksum
+ * for the manifest file itself.
+ */

This seems to be proceeding on the assumption that the checksum type
for the manifest itself will always be the same as the checksum type
for the first file in the manifest. I don't think that's the right
approach. I think the manifest should always have a SHA256 checksum,
regardless of what type of checksum is used for the individual files
within the manifest. Since the volume of data in the manifest is
presumably very small compared to the size of the database cluster
itself, I don't think there should be any performance problem there.

+ filesize = atol(size);

Using strtol() would allow for some error checking.

+ * Increase the checksum by its lable length so that we can
+ checksum = checksum + checksum_lable_length;

Spelling.

+ pg_log_error("invalid record found in \"%s\"", manifest_path);

Error message needs work.

+VerifyBackup(void)
+create_manifest_hash(char *manifest_path)
+nextLine(char *buf)

Your function names should be consistent with the surrounding style,
and with each other, as far as possible. Three different conventions
within the same patch and source file seems over the top.

Also keep in mind that you're not writing code in a vacuum. There's a
whole file of code here, and around that, a whole project.
scan_data_directory() is a good example of a function whose name is
clearly too generic. It's not a general-purpose function for scanning
the data directory; it's specifically a support function for verifying
a backup. Yet, the name gives no hint of this.

+verify_file(struct dirent *de, char fn[MAXPGPATH], struct stat st,
+ char relative_path[MAXPGPATH], manifesthash_hash *hashtab)

I think I commented on the use of char[] parameters in my previous review.

+ /* Skip backup manifest file. */
+ if (strcmp(de->d_name, "backup_manifest") == 0)
+ return;

Still looks like this will be skipped at any level of the directory
hierarchy, not just the top. And why are we skipping backup_manifest
here bug pg_wal in scan_data_directory? That's a rhetorical question,
because I know the answer: verify_file() is only getting called for
files, so you can't use it to skip directories. But that's not a good
excuse for putting closely-related checks in different parts of the
code. It's just going to result in the checks being inconsistent and
each one having its own bugs that have to be fixed separately from the
other one, as here. Please try to reorganize this code so that it can
be done in a consistent way.

I think this is related to the way you're traversing the directory
tree, which somehow looks a bit awkward to me. At the top of
scan_data_directory(), you've got code that uses basedir and
subdirpath to construct path and relative_path. I was initially
surprised to see that this was the job of this function, rather than
the caller, but then I thought: well, as long as it makes life easy
for the caller, it's probably fine. However, I notice that the only
non-trivial caller is the scan_data_directory() itself, and it has to
go and construct newsubdirpath from subdirpath and the directory name.

It seems to me that this would get easier if you defined
scan_data_directory() -- or whatever we end up calling it -- to take
two pathname-related arguments:

- basepath, which would be $PGDATA and would never change as we
recurse down, so same as what you're now calling basedir
- pathsuffix, which would be an empty string at the top level and at
each recursive level we'd add a slash and then de->d_name.

So at the top of the function we wouldn't need an if statement,
because you could just do:

snprintf(path, MAXPGPATH, "%s%s", basedir, pathsuffix);

And when you recurse you wouldn't need an if statement either, because
you could just do:

snprintf(newpathsuffix, MAXPGPATH, "%s/%s", pathsuffix, de->d_name);

What I'd suggest is constructing newpathsuffix right after rejecting
"." and ".." entries, and then you can reject both pg_wal and
backup_manifest, at the top-level only, using symmetric and elegant
code:

if (strcmp(newpathsuffix, "/pg_wal") == 0 || strcmp(newpathsuffix,
"/backup_manifest") == 0)
    continue;

+ record = manifesthash_lookup(hashtab, filename);;
+ if (record)
+ {
...long block...
+ }
+ else
+ pg_log_info("file \"%s\" is present in backup but not in manifest",
+ filename);

Try to structure the code in such a way that you minimize unnecessary
indentation. For example, in this case, you could instead write:

if (record == NULL)
{
    pg_log_info(...)
    return;
}

and the result would be that everything inside that long if-block is
now at the top level of the function and indented one level less. And
I think if you look at this function you'll see a way that you can
save a *second* level of indentation for much of that code. Please
check the rest of the patch for similar cases, too.

+static char *
+nextLine(char *buf)
+{
+ while (*buf != '\0' && *buf != '\n')
+ buf = buf + 1;
+
+ return buf + 1;
+}

I'm pretty sure that my previous review mentioned the importance of
protecting against buffer overruns here.

+static char *
+nextWord(char *line)
+{
+ while (*line != '\0' && *line != '\t' && *line != '\n')
+ line = line + 1;
+
+ return line + 1;
+}

Same problem here.

In both cases, ++ is more idiomatic.

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

On Sun, Dec 22, 2019 at 8:32 PM Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
> Agree, that performance won't be a problem, but that will be bit confusing
> to the user.  As at the start user providing the manifest-checksum (assume
> that user-provided CRC32C) and at the end, user will find the SHA256
> checksum string in the backup_manifest file.

I don't think that's particularly confusing. The documentation should
say that this is the algorithm to be used for checksumming the files
which are backed up. The algorithm to be used for the manifest itself
is another matter. To me, it seems far MORE confusing if the algorithm
used for the manifest itself is magically inferred from the algorithm
used for one of the File lines therein.

> Does this also means that irrespective of whether user provided a checksum
> option or not,  we will be always generating the checksum for the backup_manifest file?

Yes, that is what I am proposing.

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

On Tue, Dec 24, 2019 at 5:42 AM Suraj Kharage
<suraj.kharage@enterprisedb.com> wrote:
> Made the change in backup manifest as well in backup validatort patch. Thanks to Rushabh Lathia for the offline
discussionand help.
 
>
> To examine the first word of each line, I am using below check:
> if (strncmp(line, "File", 4) == 0)
> {
> ..
> }
> else if (strncmp(line, "Manifest-Checksum", 17) == 0)
> {
> ..
> }
> else
>     error
>
> strncmp might be not right here, but we can not put '\0' in between the line (to find out first word)
> before we recognize the line type.
> All the lines expect line last one (where we have manifest checksum) are feed to the checksum machinary to calculate
manifestchecksum.
 
> so update_checksum() should be called after recognizing the type, i.e: if it is a File type record. Do you see any
issueswith this?
 

I see the problem, but I don't think your solution is right, because
the first test would pass if the line said FiletMignon rather than
just File, which we certainly don't want. You've got to write the test
so that you're checking against the whole first word, not just some
prefix of it. There are several possible ways to accomplish that, but
this isn't one of them.

>> + pg_log_error("invalid record found in \"%s\"", manifest_path);
>>
>> Error message needs work.

Looks better now, but you have a messages that say "invalid checksums
type \"%s\" found in \"%s\"". This is wrong because checksums would
need to be singular in this context (checksum). Also, I think it could
be better phrased as "manifest file \"%s\" specifies unknown checksum
algorithm \"%s\" at line %d".

>> Your function names should be consistent with the surrounding style,
>> and with each other, as far as possible. Three different conventions
>> within the same patch and source file seems over the top.

This appears to be fixed.

>> Also keep in mind that you're not writing code in a vacuum. There's a
>> whole file of code here, and around that, a whole project.
>> scan_data_directory() is a good example of a function whose name is
>> clearly too generic. It's not a general-purpose function for scanning
>> the data directory; it's specifically a support function for verifying
>> a backup. Yet, the name gives no hint of this.

But this appears not to be fixed.

>> if (strcmp(newpathsuffix, "/pg_wal") == 0 || strcmp(newpathsuffix,
>> "/backup_manifest") == 0)
>>     continue;
>
> Thanks for the suggestion. Corrected as per the above inputs.

You need a comment here, like "Ignore the possible presence of a
backup_manifest file and/or a pg_wal directory in the backup being
verified." and then maybe another sentence explaining why that's the
right thing to do.

+             * The forth parameter to VerifyFile() will pass the relative path
+             * of file to match exactly with the filename present in manifest.

I don't know what this comment is trying to tell me, which might be
something you want to try to fix. However, I'm pretty sure it's
supposed to say "fourth" not "forth".

>> and the result would be that everything inside that long if-block is
>> now at the top level of the function and indented one level less. And
>> I think if you look at this function you'll see a way that you can
>> save a *second* level of indentation for much of that code. Please
>> check the rest of the patch for similar cases, too.
>
> Make sense. corrected.

I don't agree. A large chunk of VerifyFile() is still subject to a
quite unnecessary level of indentation.

> I have added a check for EOF, but not sure whether that woule be right here.
> Do we need to check the length of buffer as well?

That's really, really not right. EOF is not a character that can
appear in the buffer. It's chosen on purpose to be a value that never
matches any actual character when both the character and the EOF value
are regarded as values of type 'int'. That guarantee doesn't apply
here though because you're dealing with values of type 'char'. So what
this code is doing is searching for an impossible value using
incorrect logic, which has very little to do with the actual need
here, which is to avoid running off the end of the buffer. To see what
the problem is, try creating a file with no terminating newline, like
this:

echo -n this file has no terminating newline >> some-file

I doubt it will be very hard to make this patch crash horribly. Even
if you can't, it seems pretty clear that the logic isn't right.

I don't really know what the \0 tests in NextLine() and NextWord()
think they're doing either. If there's a \0 in the buffer before you
add one, it was in the original input data, and pretending like that
marks a word or line boundary seems like a fairly arbitrary choice.

What I suggest is:

(1) Allocate one byte more than the file size for the buffer that's
going to hold the file, so that if you write a \0 just after the last
byte of the file, you don't overrun the allocated buffer.

(2) Compute char *endptr = buf + len.

(3) Pass endptr to NextLine and NextWord and write the loop condition
something like while (*buf != '\n' && buf < endptr).

Other notes:

- The error handling in ReadFileIntoBuffer() does not seem to consider
the case of a short read. If you look through the source tree, you can
find examples of how we normally handle that.

- Putting string_hash_sdbm() into encode.c seems like a surprising
choice. What does this have to do with encoding anything? And why is
it going into src/common at all if it's only intended for frontend
use?

- It seems like whether or not any problems were found while verifying
the manifest ought to affect the exit status of pg_basebackup. I'm not
exactly sure what exit codes ought to be used, but you could look for
similar precedents. Document this, too.

- As much as possible let's have errors in the manifest file report
the line number, and let's also try to make them more specific, e.g.
instead of "invalid manifest record found in \"%s\"", perhaps
"manifest file \"%s\" contains invalid keyword \"%s\" at line %d".

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

Moin,

sorry for the very late reply. There was a discussion about the specific 
format of the backup manifests, and maybe that was already discussed and 
I just overlooked it:

1) Why invent your own format, and not just use a machine-readable 
format that already exists? It doesn't have to be full blown XML, or 
even JSON, something simple as YAML would already be better. That way 
not everyone has to write their own parser. Or maybe it is already YAML 
and just the different keywords where under discussion?

2) It would be very wise to add a version number to the format. That 
will making an extension later much easier and avoids the "we need  to 
add X, but that breaks compatibility with all software out there" 
situations that often arise a few years down the line.

Best regards,

and a happy New Year 2020

Tels

On Tue, Dec 31, 2019 at 01:30:01PM +0100, Tels wrote:
> Moin,
> 
> sorry for the very late reply. There was a discussion about the specific
> format of the backup manifests, and maybe that was already discussed and I
> just overlooked it:
> 
> 1) Why invent your own format, and not just use a machine-readable format
> that already exists? It doesn't have to be full blown XML, or even JSON,
> something simple as YAML would already be better. That way not everyone has
> to write their own parser. Or maybe it is already YAML and just the
> different keywords where under discussion?

YAML is extremely fragile and error-prone. It's also a superset of
JSON, so I don't understand what you mean by "as simple as."

-1 from me on YAML

That said, I agree that there's no reason to come up with a bespoke
format and parser when JSON is already available in every PostgreSQL
installation.  Imposing a structure atop that includes a version
number, as you suggest, seems pretty straightforward, and should be
done.

Would it make sense to include some kind of capability description in
the format along with the version number?

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On 12/31/19 10:43 AM, David Fetter wrote:
> On Tue, Dec 31, 2019 at 01:30:01PM +0100, Tels wrote:
>> Moin,
>>
>> sorry for the very late reply. There was a discussion about the specific
>> format of the backup manifests, and maybe that was already discussed and I
>> just overlooked it:
>>
>> 1) Why invent your own format, and not just use a machine-readable format
>> that already exists? It doesn't have to be full blown XML, or even JSON,
>> something simple as YAML would already be better. That way not everyone has
>> to write their own parser. Or maybe it is already YAML and just the
>> different keywords where under discussion?
> 
> YAML is extremely fragile and error-prone. It's also a superset of
> JSON, so I don't understand what you mean by "as simple as."
> 
> -1 from me on YAML

-1 from me as well.  YAML is easy to write but definitely non-trivial to 
read.

> That said, I agree that there's no reason to come up with a bespoke
> format and parser when JSON is already available in every PostgreSQL
> installation.  Imposing a structure atop that includes a version
> number, as you suggest, seems pretty straightforward, and should be
> done.

+1.  I continue to support a format that would be easily readable 
without writing a lot of code.

-- 
-David
david@pgmasters.net

On Tue, Dec 31, 2019 at 9:16 PM David Steele <david@pgmasters.net> wrote:
> > That said, I agree that there's no reason to come up with a bespoke
> > format and parser when JSON is already available in every PostgreSQL
> > installation.  Imposing a structure atop that includes a version
> > number, as you suggest, seems pretty straightforward, and should be
> > done.
>
> +1.  I continue to support a format that would be easily readable
> without writing a lot of code.

So, if someone can suggest to me how I could read JSON from a tool in
src/bin without writing a lot of code, I'm all ears. So far that's
been asserted but not been demonstrated to be possible. Getting the
JSON parser that we have in the backend to work from frontend doesn't
look all that straightforward, for reasons that I talked about in
http://postgr.es/m/CA+TgmobZrNYR-ATtfZiZ_k-W7tSPgvmYZmyiqumQig4R4fkzHw@mail.gmail.com

As to the suggestion that a version number be included, that's been
there in every version of the patch I've posted.

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

On Wed, Jan 01, 2020 at 01:43:40PM -0500, Robert Haas wrote:
> On Tue, Dec 31, 2019 at 9:16 PM David Steele <david@pgmasters.net> wrote:
> > > That said, I agree that there's no reason to come up with a bespoke
> > > format and parser when JSON is already available in every PostgreSQL
> > > installation.  Imposing a structure atop that includes a version
> > > number, as you suggest, seems pretty straightforward, and should be
> > > done.
> >
> > +1.  I continue to support a format that would be easily readable
> > without writing a lot of code.
> 
> So, if someone can suggest to me how I could read JSON from a tool in
> src/bin without writing a lot of code, I'm all ears. So far that's
> been asserted but not been demonstrated to be possible. Getting the
> JSON parser that we have in the backend to work from frontend doesn't
> look all that straightforward, for reasons that I talked about in
> http://postgr.es/m/CA+TgmobZrNYR-ATtfZiZ_k-W7tSPgvmYZmyiqumQig4R4fkzHw@mail.gmail.com

Maybe I'm missing something obvious, but wouldn't combining
pg_read_file() with a cast to JSONB fix this, as below?

shackle@[local]:5413/postgres(13devel)(892328) # SELECT jsonb_pretty(j::jsonb) FROM
pg_read_file('/home/shackle/advanced_comparison.json')AS t(j);
 
            jsonb_pretty            
════════════════════════════════════
 [                                 ↵
     {                             ↵
         "message": "hello world!",↵
         "severity": "[DEBUG]"     ↵
     },                            ↵
     {                             ↵
         "message": "boz",         ↵
         "severity": "[INFO]"      ↵
     },                            ↵
     {                             ↵
         "message": "foo",         ↵
         "severity": "[DEBUG]"     ↵
     },                            ↵
     {                             ↵
         "message": "null",        ↵
         "severity": "null"        ↵
     }                             ↵
 ]
(1 row)

Time: 3.050 ms

> As to the suggestion that a version number be included, that's been
> there in every version of the patch I've posted.

and thanks for that!

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

David Fetter <david@fetter.org> writes:
> On Wed, Jan 01, 2020 at 01:43:40PM -0500, Robert Haas wrote:
>> So, if someone can suggest to me how I could read JSON from a tool in
>> src/bin without writing a lot of code, I'm all ears.

> Maybe I'm missing something obvious, but wouldn't combining
> pg_read_file() with a cast to JSONB fix this, as below?

Only if you're prepared to restrict the use of the tool to superusers
(or at least people with whatever privilege that function requires).

Admittedly, you can probably feed the data to the backend without
use of an intermediate file; but it still requires a working backend
connection, which might be a bit of a leap for backup-related tools.
I'm sure Robert was envisioning doing this processing inside the tool.

            regards, tom lane

On Wed, Jan 1, 2020 at 7:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> David Fetter <david@fetter.org> writes:
> > On Wed, Jan 01, 2020 at 01:43:40PM -0500, Robert Haas wrote:
> >> So, if someone can suggest to me how I could read JSON from a tool in
> >> src/bin without writing a lot of code, I'm all ears.
>
> > Maybe I'm missing something obvious, but wouldn't combining
> > pg_read_file() with a cast to JSONB fix this, as below?
>
> Only if you're prepared to restrict the use of the tool to superusers
> (or at least people with whatever privilege that function requires).
>
> Admittedly, you can probably feed the data to the backend without
> use of an intermediate file; but it still requires a working backend
> connection, which might be a bit of a leap for backup-related tools.
> I'm sure Robert was envisioning doing this processing inside the tool.

Yeah, exactly. I don't think verifying a backup should require a
running server, let alone a running server on the same machine where
the backup is stored and for which you have superuser privileges.
AFAICS, the only options to make that work with JSON are (1) introduce
a new hand-coded JSON parser designed for frontend operation, (2) add
a dependency on an external JSON parser that we can use from frontend
code, or (3) adapt the existing JSON parser used in the backend so
that it can also be used in the frontend.

I'd be willing to do (1) -- it wouldn't be the first time I've written
JSON parser for PostgreSQL -- but I think it will take an order of
magnitude more code than using a file with tab-separated columns as
I've proposed, and I assume that there will be complaints about having
two JSON parsers in core. I'd also be willing to do (2) if that's the
consensus, but I'd vote against such an approach if somebody else
proposed it because (a) I'm not aware of a widely-available library
upon which we could depend and (b) introducing such a dependency for a
minor feature like this seems fairly unpalatable to me, and it'd
probably still be more code than just using a tab-separated file.  I'd
be willing to do (3) if somebody could explain to me how to solve the
problems with porting that code to work on the frontend side, but the
only suggestion so far as to how to do that is to port memory
contexts, elog/report, and presumably encoding handling to work on the
frontend side. That seems to me to be an unreasonably large lift,
especially given that we have lots of other files that use ad-hoc
formats already, and if somebody ever gets around to converting all of
those to JSON, they can certainly convert this one at the same time.

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

Robert Haas <robertmhaas@gmail.com> writes:
> AFAICS, the only options to make that work with JSON are (1) introduce
> a new hand-coded JSON parser designed for frontend operation, (2) add
> a dependency on an external JSON parser that we can use from frontend
> code, or (3) adapt the existing JSON parser used in the backend so
> that it can also be used in the frontend.
> ...  I'd
> be willing to do (3) if somebody could explain to me how to solve the
> problems with porting that code to work on the frontend side, but the
> only suggestion so far as to how to do that is to port memory
> contexts, elog/report, and presumably encoding handling to work on the
> frontend side. That seems to me to be an unreasonably large lift,

Yeah, agreed.  The only consideration that'd make that a remotely
sane idea is that if somebody did the work, there would be other
uses for it.  (One that comes to mind immediately is cleaning up
ecpg's miserably-maintained fork of the backend datetime code.)

But there's no denying that it would be a large amount of work
(if it's even feasible), and nobody has stepped up to volunteer.
It's not reasonable to hold up this particular feature waiting
for that to happen.

If a tab-delimited file can handle this requirement, that seems
like a sane choice to me.

            regards, tom lane

On Wed, Jan 01, 2020 at 08:57:11PM -0500, Robert Haas wrote:
> On Wed, Jan 1, 2020 at 7:46 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > David Fetter <david@fetter.org> writes:
> > > On Wed, Jan 01, 2020 at 01:43:40PM -0500, Robert Haas wrote:
> > >> So, if someone can suggest to me how I could read JSON from a tool in
> > >> src/bin without writing a lot of code, I'm all ears.
> >
> > > Maybe I'm missing something obvious, but wouldn't combining
> > > pg_read_file() with a cast to JSONB fix this, as below?
> >
> > Only if you're prepared to restrict the use of the tool to superusers
> > (or at least people with whatever privilege that function requires).
> >
> > Admittedly, you can probably feed the data to the backend without
> > use of an intermediate file; but it still requires a working backend
> > connection, which might be a bit of a leap for backup-related tools.
> > I'm sure Robert was envisioning doing this processing inside the tool.
> 
> Yeah, exactly. I don't think verifying a backup should require a
> running server, let alone a running server on the same machine where
> the backup is stored and for which you have superuser privileges.

Thanks for clarifying the context.

> AFAICS, the only options to make that work with JSON are (1) introduce
> a new hand-coded JSON parser designed for frontend operation, (2) add
> a dependency on an external JSON parser that we can use from frontend
> code, or (3) adapt the existing JSON parser used in the backend so
> that it can also be used in the frontend.
> 
> I'd be willing to do (1) -- it wouldn't be the first time I've written
> JSON parser for PostgreSQL -- but I think it will take an order of
> magnitude more code than using a file with tab-separated columns as
> I've proposed, and I assume that there will be complaints about having
> two JSON parsers in core. I'd also be willing to do (2) if that's the
> consensus, but I'd vote against such an approach if somebody else
> proposed it because (a) I'm not aware of a widely-available library
> upon which we could depend and

I believe jq has an excellent one that's available under a suitable
license.

Making jq a dependency seems like a separate discussion, though. At
the moment, we don't use git tools like submodel/subtree, and deciding
which (or whether) seems like a gigantic discussion all on its own.

> (b) introducing such a dependency for a minor feature like this
> seems fairly unpalatable to me, and it'd probably still be more code
> than just using a tab-separated file.  I'd be willing to do (3) if
> somebody could explain to me how to solve the problems with porting
> that code to work on the frontend side, but the only suggestion so
> far as to how to do that is to port memory contexts, elog/report,
> and presumably encoding handling to work on the frontend side.

This port has come up several times recently in different contexts.
How big a chunk of work would it be?  Just so we're clear, I'm not
suggesting that this port should gate this feature.

> That seems to me to be an unreasonably large lift, especially given
> that we have lots of other files that use ad-hoc formats already,
> and if somebody ever gets around to converting all of those to JSON,
> they can certainly convert this one at the same time.

Would that require some kind of file converter program, or just a
really loud notice in the release notes?

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Thu, Jan 2, 2020 at 1:03 PM David Fetter <david@fetter.org> wrote:
> I believe jq has an excellent one that's available under a suitable
> license.
>
> Making jq a dependency seems like a separate discussion, though. At
> the moment, we don't use git tools like submodel/subtree, and deciding
> which (or whether) seems like a gigantic discussion all on its own.

Yep. And it doesn't seem worth it for a relatively small feature like
this. If we already had it, it might be worth using for a relatively
small feature like this, but that's a different issue.

> > (b) introducing such a dependency for a minor feature like this
> > seems fairly unpalatable to me, and it'd probably still be more code
> > than just using a tab-separated file.  I'd be willing to do (3) if
> > somebody could explain to me how to solve the problems with porting
> > that code to work on the frontend side, but the only suggestion so
> > far as to how to do that is to port memory contexts, elog/report,
> > and presumably encoding handling to work on the frontend side.
>
> This port has come up several times recently in different contexts.
> How big a chunk of work would it be?  Just so we're clear, I'm not
> suggesting that this port should gate this feature.

I don't really know. It's more of a research project than a coding
project, at least initially, I think. For instance, psql has its own
non-local-transfer-of-control mechanism using sigsetjmp(). If you
wanted to introduce elog/ereport on the frontend, would you make psql
use it? Or just let psql continue to do what it does now and introduce
the new mechanism as an option for code going forward? Or try to make
the two mechanisms work together somehow? Will you start using the
same error codes that we use in the backend on the frontend side, and
if so, what will they do, given that what the backend does is just
embed them in a protocol message that any particular client may or may
not display? Similarly, should frontend errors support reporting a
hint, detail, statement, or query? Will it be confusing if backend and
frontend errors are too similar? If you make memory contexts available
in the frontend, what if any code will you adapt to use them? There's
a lot of stuff in src/bin. If you want the encoding machinery on the
front end, what will you use in place of the backend's idea of the
"database encoding"? What will you do about dependencies on Datum in
frontend code? Somebody would need to study all this stuff, come up
with a tentative set of decisions, write patches, get it all working,
and then quite possibly have the choices they made get second-guessed
by other people who have different ideas. If you come up with a really
good, clean proposal that doesn't provoke any major disagreements, you
might be able to get this done in a couple of months. If you can't
come up with something people good, or if you're the only one who
thinks what you come up with is good, it might take years.

It seems to me that in a perfect world a lot of the code we have in
the backend that is usefully reusable in other contexts would be
structured so that it doesn't have random dependencies on backend-only
machinery like memory contexts and elog/ereport. For example, if you
write a function that returns an error message rather than throwing an
error, then you can arrange to call that from either frontend or
backend code and the caller can do whatever it wishes with that error
text. However, once you've written your code so that an error gets
thrown six layers down in the call stack, it's really hard to
rearrange that so that the error is returned, and if you are
populating not only the primary error message but error code, detail,
hint, etc. it's almost impractical to think that you can rearrange
things that way anyway. And generally you want to be populating those
things, as a best practice for backend code. So while in theory I kind
of like the idea of adapting the JSON parser we've already got to just
not depend so heavily on a backend environment, it's not really very
clear how to actually make that happen. At least not to me.

> > That seems to me to be an unreasonably large lift, especially given
> > that we have lots of other files that use ad-hoc formats already,
> > and if somebody ever gets around to converting all of those to JSON,
> > they can certainly convert this one at the same time.
>
> Would that require some kind of file converter program, or just a
> really loud notice in the release notes?

Maybe neither. I don't see why it wouldn't be possible to be
backward-compatible just by keeping the old code around and having it
parse as far as the version number. Then it could decide to continue
on with the old code or call the new code, depending.

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

Greetings,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > AFAICS, the only options to make that work with JSON are (1) introduce
> > a new hand-coded JSON parser designed for frontend operation, (2) add
> > a dependency on an external JSON parser that we can use from frontend
> > code, or (3) adapt the existing JSON parser used in the backend so
> > that it can also be used in the frontend.
> > ...  I'd
> > be willing to do (3) if somebody could explain to me how to solve the
> > problems with porting that code to work on the frontend side, but the
> > only suggestion so far as to how to do that is to port memory
> > contexts, elog/report, and presumably encoding handling to work on the
> > frontend side. That seems to me to be an unreasonably large lift,
>
> Yeah, agreed.  The only consideration that'd make that a remotely
> sane idea is that if somebody did the work, there would be other
> uses for it.  (One that comes to mind immediately is cleaning up
> ecpg's miserably-maintained fork of the backend datetime code.)
>
> But there's no denying that it would be a large amount of work
> (if it's even feasible), and nobody has stepped up to volunteer.
> It's not reasonable to hold up this particular feature waiting
> for that to happen.

Sure, it'd be work, and for "adding a simple backup manifest", maybe too
much to be worth considering ... but that's not what is going on here,
is it?  Are we really *just* going to add a backup manifest to
pg_basebackup and call it done?  That's not what I understood the goal
here to be but rather to start doing a lot of other things with
pg_basebackup beyond just having a manifest and if you think just a bit
farther down the path, I think you start to realize that you're going to
need this base set of capabilities to get to a point where pg_basebackup
(or whatever it ends up being called) is able to have the kind of
capabilities that exist in other PG backup software already.

I'm sure I don't need to say where to find it, but I can point you to a
pretty good example of a similar effort, and we didn't start with "build
a manifest into a custom format" as the first thing implemented, but
rather a great deal of work was first put into building out things like
logging, memory management/contexts, error handling/try-catch, having a
string type, a variant type, etc.

In some ways, it's kind of impressive what we've got in our front-ends
tools even though we don't have these things, really, and certainly not
all in one nice library that they all use...  but at the same time, I
think that lack has also held those tools back, pg_basebackup among
them.

Anyway, off my high horse, I'll just say I agree w/ David and David wrt
using JSON for this over hacking together yet another format.  We didn't
do that as thoroughly as we should have (we've got a JSON parser and all
that, and use JSON quite a bit, but the actual manifest format is a mix
of ini-style and JSON, because it's got more in it than just a list of
files, something that I suspect will also end up being true of this down
the road and for good reasons, and we started with the ini format and
discovered it sucked and then started embedding JSON in it...), and
we've come to realize that was a bad idea, and intend to fix it in our
next manifest major version bump.  Would be unfortunate to see PG making
that same mistake.

Thanks,

Stephen

On Fri, Jan 3, 2020 at 11:44 AM Stephen Frost <sfrost@snowman.net> wrote:
> Sure, it'd be work, and for "adding a simple backup manifest", maybe too
> much to be worth considering ... but that's not what is going on here,
> is it?  Are we really *just* going to add a backup manifest to
> pg_basebackup and call it done?  That's not what I understood the goal
> here to be but rather to start doing a lot of other things with
> pg_basebackup beyond just having a manifest and if you think just a bit
> farther down the path, I think you start to realize that you're going to
> need this base set of capabilities to get to a point where pg_basebackup
> (or whatever it ends up being called) is able to have the kind of
> capabilities that exist in other PG backup software already.

I have no development plans for pg_basebackup that require extending
the format of the manifest file in any significant way, and am not
aware that anyone else has such plans either. If you are aware of
something I'm not, or if anyone else is, it would be helpful to know
about it.

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

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Fri, Jan 3, 2020 at 11:44 AM Stephen Frost <sfrost@snowman.net> wrote:
> > Sure, it'd be work, and for "adding a simple backup manifest", maybe too
> > much to be worth considering ... but that's not what is going on here,
> > is it?  Are we really *just* going to add a backup manifest to
> > pg_basebackup and call it done?  That's not what I understood the goal
> > here to be but rather to start doing a lot of other things with
> > pg_basebackup beyond just having a manifest and if you think just a bit
> > farther down the path, I think you start to realize that you're going to
> > need this base set of capabilities to get to a point where pg_basebackup
> > (or whatever it ends up being called) is able to have the kind of
> > capabilities that exist in other PG backup software already.
>
> I have no development plans for pg_basebackup that require extending
> the format of the manifest file in any significant way, and am not
> aware that anyone else has such plans either. If you are aware of
> something I'm not, or if anyone else is, it would be helpful to know
> about it.

You're certainly intending to do *something* with the manifest, and
while I appreciate that you feel you've come up with a complete use-case
that this simple manifest will be sufficient for, I frankly doubt
that'll actually be the case.  Not long ago it wasn't completely clear
that a manifest at *all* was even going to be necessary for the specific
use-case you had in mind (I'll admit I wasn't 100% sure myself at the
time either), but now that we're down the road of having one, I can't
agree with the blanket assumption that we're never going to want to
extend it, or even that it won't be necessary to add to it before this
particular use-case is fully addressed.

And the same goes for the other things that were discussed up-thread
regarding memory context and error handling and such.

I'm happy to outline the other things that one *might* want to include
in a manifest, if that would be helpful, but I'll also say that I'm not
planning to hack on adding that to pg_basebackup in the next month or
two.  Once we've actually got a manifest, if it's in an extendable
format, I could certainly see people wanting to do more with it though.

Thanks,

Stephen

On Fri, Jan 3, 2020 at 12:01 PM Stephen Frost <sfrost@snowman.net> wrote:
> You're certainly intending to do *something* with the manifest, and
> while I appreciate that you feel you've come up with a complete use-case
> that this simple manifest will be sufficient for, I frankly doubt
> that'll actually be the case.  Not long ago it wasn't completely clear
> that a manifest at *all* was even going to be necessary for the specific
> use-case you had in mind (I'll admit I wasn't 100% sure myself at the
> time either), but now that we're down the road of having one, I can't
> agree with the blanket assumption that we're never going to want to
> extend it, or even that it won't be necessary to add to it before this
> particular use-case is fully addressed.
>
> And the same goes for the other things that were discussed up-thread
> regarding memory context and error handling and such.

Well, I don't know how to make you happy here. It looks to me like
insisting on a JSON-format manifest will likely mean that this doesn't
get into PG13 or PG14 or probably PG15, because a port of all that
machinery to work in frontend code will be neither simple nor quick.
If you want this to happen for this release, you've got to be willing
to settle for something that can be implemented in the time we have.

I'm not sure whether what you and David are arguing boils down to
thinking that I'm wrong when I say that doing that is hard, or whether
you know it's hard but you just don't care because you'd rather see
the feature go nowhere than use a format other than JSON. I don't see
much difference between the latter position and a desire to block the
feature permanently. And if it's the former then you have yet to make
any suggestions for how to get it done with reasonable effort.

> I'm happy to outline the other things that one *might* want to include
> in a manifest, if that would be helpful, but I'll also say that I'm not
> planning to hack on adding that to pg_basebackup in the next month or
> two.  Once we've actually got a manifest, if it's in an extendable
> format, I could certainly see people wanting to do more with it though.

Well, as I say, it's got a version number, so somebody can always come
along with something better. I really think this is a red herring,
though. If somebody wants to track additional data about a backup,
there's no rule that they have to include it in the backup manifest. A
backup management solution might want to track things like who
initiated the backup, or for what purpose it was taken, or the IP
address of the machine where it was taken, or the backup system's own
identifier, but any of that stuff could (and probably should) be
stored in a file managed by that tool rather than in the server's own
manifest.  As to the per-file information, I believe that David and I
discussed that and the list of fields that I had seemed relatively OK,
and I believe I added at least one (mtime) per his suggestion. Of
course, it's a tab-separated file; more fields could easily be added
at the end, separated by tabs. Or, you could modify the file so that
after each "File" line you had another line with supplementary
information about that file, beginning with some other word. Or, you
could convert the whole file to JSON for v2 of the manifest, if,
contrary to my belief, that's a fairly simple thing to do. There are
probably other approaches as well. This file format has already had
considerably more thought about forward-compatibility than
pg_hba.conf, which has been retrofitted multiple times without
breaking the world.

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

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Fri, Jan 3, 2020 at 12:01 PM Stephen Frost <sfrost@snowman.net> wrote:
> > You're certainly intending to do *something* with the manifest, and
> > while I appreciate that you feel you've come up with a complete use-case
> > that this simple manifest will be sufficient for, I frankly doubt
> > that'll actually be the case.  Not long ago it wasn't completely clear
> > that a manifest at *all* was even going to be necessary for the specific
> > use-case you had in mind (I'll admit I wasn't 100% sure myself at the
> > time either), but now that we're down the road of having one, I can't
> > agree with the blanket assumption that we're never going to want to
> > extend it, or even that it won't be necessary to add to it before this
> > particular use-case is fully addressed.
> >
> > And the same goes for the other things that were discussed up-thread
> > regarding memory context and error handling and such.
>
> Well, I don't know how to make you happy here.

I suppose I should admit that, first off, I don't feel you're required
to make me happy, and I don't think it's necessary to make me happy to
get this feature into PG.

Since you expressed that interest though, I'll go out on a limb and say
that what would make me *really* happy would be to think about where the
project should be taking pg_basebackup, what we should be working on
*today* to address the concerns we hear about from our users, and to
consider the best way to implement solutions to what they're actively
asking for a core backup solution to be providing.  I get that maybe
that isn't how the world works and that sometimes we have people who
write our paychecks wanting us to work on something else, and yes, I'm
sure there are some users who are asking for this specific thing but I
certainly don't think it's a common ask of pg_basebackup or what users
feel is missing from the backup options we offer in core; we had users
on this list specifically saying they *wouldn't* use this feature
(referring to the differential backup stuff, of course), in fact,
because of the things which are missing, which is pretty darn rare.

That's what would make *me* happy.  Even some comments about how to
*get* there while also working towards these features would be likely
to make me happy.  Instead, I feel like we're being told that we need
this feature badly in v13 and we're going to cut bait and do whatever
is necessary to get us there.

> It looks to me like
> insisting on a JSON-format manifest will likely mean that this doesn't
> get into PG13 or PG14 or probably PG15, because a port of all that
> machinery to work in frontend code will be neither simple nor quick.

I certainly understand that these things take time, sometimes quite a
bit of it as the past 2 years have shown in this other little side
project, and that was hacking without having to go through the much
larger effort involved in getting things into PG core.  That doesn't
mean that kind of effort isn't worthwhile or that, because something is
a bunch of work, we shouldn't spend the time on it.  I do feel what
you're after here is a multi-year project, and I've said before that I
don't agree that this is a feature (the differential backup with
pg_basebackup thing) that makes any sense going into PG at this time,
but I'm also not trying to block this feature, just to share the
experience that we've gotten from working in this area for quite a
while and hopefully help guide the effort in PG away from pitfalls and
in a good direction long-term.

> If you want this to happen for this release, you've got to be willing
> to settle for something that can be implemented in the time we have.

I'm not sure what you're expecting here, but for my part, at least, I'm
not going to be terribly upset if this feature doesn't make this release
because there's an agreement and understanding that the current
direction isn't a good long-term solution.  Nor am I going to be
terribly upset about the time that's been spent on this particular
approach given that there's been no shortage of people commenting that
they'd rather see an extensible format, like JSON, and has been for
quite some time.

All that said- one thing we've done is to consider that *we* are the
ones who are writing the JSON, while also being the ones to read it- we
don't need the parsing side to understand and deal with *any* JSON that
might exist out there, just whatever it is the server creates/created.
It may be possible to use that to simplify the parser, or perhaps at
least to accept that if it ends up being given something else that it
might not perform as well with it.  I'm not sure how helpful that will
be to you, but I recall David finding it a helpful thought.

> I'm not sure whether what you and David are arguing boils down to
> thinking that I'm wrong when I say that doing that is hard, or whether
> you know it's hard but you just don't care because you'd rather see
> the feature go nowhere than use a format other than JSON. I don't see
> much difference between the latter position and a desire to block the
> feature permanently. And if it's the former then you have yet to make
> any suggestions for how to get it done with reasonable effort.

There seems to be a great deal of daylight between the two positions
you're proposing I might have (as I don't speak for David..).

I *do* think there's a lot of work that would need to be done here to
make this a good solution.  I'm *not* completely against other formats
besides JSON.  Even more so though, I am *not* argueing that this
feature should go 'nowhere', whether it uses JSON or not.

What I don't care for is having a hand-hacked inflexible format that's
going to require everyone down the road to implement their own parser
for it and bespoke code for every version of the custom format that
there ends up being, *including* PG core, to be clear.  Whatever utility
is going to be utilizing this manifest, it's going to need to support
older versions, just like pg_dump deals with older versions of custom
format dumps (though we still get people complaining about not being
able to use older tools with newer dumps- it'd be awful nice if we
could use JSON, or something, and then just *add* things that wouldn't
break older tools, except for the rare case where we don't have a
choice..).  Not to mention the debugging grief and such, since we can't
just use a tool like jq to check out what's going on.

As to the reference to pg_hba.conf- I don't think the packagers would
necessairly agree that there's been little grief around that, but even
so, a given pg_hba.conf is only going to be used with a given major
version and, sure, it might have to be updated to that newer major
version's format if we change the format and someone copies the old
version to the new version, but that's during a major version upgrade of
the server, and at least newer tools don't have to deal with the older
pg_hba.conf version.

Also, pg_hba.conf doesn't seem like a terribly good example in any case-
the last time the actual structure of that file was changed in a
breaking way was in 2002 when the 'user' column was added, and the
example pg_hba.conf from that commit works just fine with PG12, it
seems, based on some quick tests.  There have been other
backwards-incompatible changes, of course, the last being 6 years ago, I
think, when 'krb5' was removed.  I suppose there is some chance that you
might have a PG12-configured pg_hba.conf and you try copying that back
to a PG11 or PG10 server and it doesn't work, but that strikes me as far
less of an issue than trying to read a PG12 backup with a PG11 tool,
which we know people do because they complain on the lists about it with
pg_dump/pg_restore.

Thanks,

Stephen

On Fri, Jan 3, 2020 at 2:35 PM Stephen Frost <sfrost@snowman.net> wrote:
> > Well, I don't know how to make you happy here.
>
> I suppose I should admit that, first off, I don't feel you're required
> to make me happy, and I don't think it's necessary to make me happy to
> get this feature into PG.

Fair enough. That is gracious of you, but I would like to try to make
you happy if it is possible to do so.

> Since you expressed that interest though, I'll go out on a limb and say
> that what would make me *really* happy would be to think about where the
> project should be taking pg_basebackup, what we should be working on
> *today* to address the concerns we hear about from our users, and to
> consider the best way to implement solutions to what they're actively
> asking for a core backup solution to be providing.  I get that maybe
> that isn't how the world works and that sometimes we have people who
> write our paychecks wanting us to work on something else, and yes, I'm
> sure there are some users who are asking for this specific thing but I
> certainly don't think it's a common ask of pg_basebackup or what users
> feel is missing from the backup options we offer in core; we had users
> on this list specifically saying they *wouldn't* use this feature
> (referring to the differential backup stuff, of course), in fact,
> because of the things which are missing, which is pretty darn rare.

Well, I mean, what you seem to be suggesting here is that somebody is
driving me with a stick to do something that I don't really like but
have to do because otherwise I won't be able to make rent, but that's
actually not the case. I genuinely believe that this is a good design,
and it's driven by me, not some shadowy conglomerate of EnterpriseDB
executives who are out to make PostgreSQL sucks. If I'm wrong and the
design sucks, that's again not the fault of shadowy EnterpriseDB
executives; it's my fault. Incidentally, my boss is not very shadowy
anyhow; he's a super-nice guy, and a major reason why I work here. :-)

I don't think the issue here is that I haven't thought about what
users want, but that not everybody wants the same thing, and it's
seems like the people with whom I interact want somewhat different
things than those with whom you interact. EnterpriseDB has an existing
tool that does parallel and block-level incremental backup, and I
started out with the goal of providing those same capabilities in
core. They are quite popular with EnterpriseDB customers, and I'd like
to make them more widely available and, as far as I can, improve on
them. From our previous discussion and from a (brief) look at
pgbackrest, I gather that the interests of your customers are somewhat
different. Apparently, block-level incremental backup isn't quite as
important to your customers, perhaps because you've already got
file-level incremental backup, but various other things like
encryption and backup verification are extremely important, and you've
got a set of ideas about what would be valuable in the future which
I'm sure is based on real input from your customers. I hope you pursue
those ideas, and I hope you do it in core rather than in a separate
piece of software, but that's up to you. Meanwhile, I think that if I
have somewhat different ideas about what I'd like to pursue, that
ought to be just fine. And I don't think it is unreasonable to hope
that you'll acknowledge my goals as legitimate even if you have
different ones.

I want to point out that my idea about how to do all of this has
shifted by a considerable amount based on the input that you and David
have provided. My original design didn't involve a backup manifest,
but now it does. That turned out to be necessary, but it was also
something you suggested, and something where I asked and took advice
on what ought to go into it. Likewise, you suggested that the process
of taking the backup should involve giving the client more control
rather than trying to do everything on the server side, and that is
now the design which I plan to pursue. You suggested that because it
would be more advantageous for out-of-core backup tools, such as
pgbackrest, and I acknowledge that as a benefit and I think we're
headed in that direction. I am not doing a single thing which, to my
knowledge, blocks anything that you might want to do with
pg_basebackup in the future. I have accepted as much of your input as
I believe that I can without killing the project off completely. To go
further, I'd have to either accept years of delay or abandon my
priorities entirely and pursue yours.

> That's what would make *me* happy.  Even some comments about how to
> *get* there while also working towards these features would be likely
> to make me happy.  Instead, I feel like we're being told that we need
> this feature badly in v13 and we're going to cut bait and do whatever
> is necessary to get us there.

This seems like a really unfair accusation given how much work I've
put into trying to satisfy you and David. If this patch, the parallel
full backup patch, and the incremental backup patch were all to get
committed to v13, an outcome which seems pretty unlikely to me at this
point, then you would have a very significant number of things that
you have requested in the course of the various discussions, and
AFAICS the only thing you'd have that you don't want is the need to
parse the manifest file use while (<>) { @a = split /\t/, $_ } rather
than $a = parse_json(join '', <>). You would, for example, have the
ability to request an individual file from the server rather than a
complete tarball. Maybe the command that requests a file would lack an
encryption option, something which IIUC you would like to have, but
that certainly does not leave you worse off. It is easier to add an
encryption option to a command which you already have than it is to
invent a whole new command -- or really several whole new commands,
since such a command is not really usable unless you also have
facilities to start and stop a backup through the replication
protocol.

All that being said, I continue to maintain that insisting on JSON is
not a reasonable request. It is not easy to parse JSON, or a subset of
JSON. The amount of code required to write even a stripped-down JSON
parser is far more than the amount required to split a file on tabs,
and the existing code we have for the backend cannot be easily (or
even with moderate effort) adapted to work in the frontend. On the
other hand, the code that pgbackrest would need to parse the manifest
file format I've proposed could have easily been written in less time
than you've spent arguing about it. Heck, if it helps, I'll offer
write that patch myself (I could be a pgbackrest contributor!). I
don't want this effort to suck because something gets rushed through
too quickly, but I also don't want it to get derailed because of what
I view as a relatively minor detail. It is not always right to take
the easier road, but it is also not always wrong. I have no illusions
that what is being proposed here is perfect, but lots of features
started out imperfect and get better over time -- RLS and parallel
query come to mind, among others -- and we often learn from the
experience of shipping something which parts of the feature are most
in need of improvement.

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

Greetings,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Fri, Jan 3, 2020 at 2:35 PM Stephen Frost <sfrost@snowman.net> wrote:
> > > Well, I don't know how to make you happy here.
> >
> > I suppose I should admit that, first off, I don't feel you're required
> > to make me happy, and I don't think it's necessary to make me happy to
> > get this feature into PG.
>
> Fair enough. That is gracious of you, but I would like to try to make
> you happy if it is possible to do so.

I certainly appreciate that, but I don't know that it is possible to do
so while approaching this in the order that you are, which I tried to
point out previously.

> > Since you expressed that interest though, I'll go out on a limb and say
> > that what would make me *really* happy would be to think about where the
> > project should be taking pg_basebackup, what we should be working on
> > *today* to address the concerns we hear about from our users, and to
> > consider the best way to implement solutions to what they're actively
> > asking for a core backup solution to be providing.  I get that maybe
> > that isn't how the world works and that sometimes we have people who
> > write our paychecks wanting us to work on something else, and yes, I'm
> > sure there are some users who are asking for this specific thing but I
> > certainly don't think it's a common ask of pg_basebackup or what users
> > feel is missing from the backup options we offer in core; we had users
> > on this list specifically saying they *wouldn't* use this feature
> > (referring to the differential backup stuff, of course), in fact,
> > because of the things which are missing, which is pretty darn rare.
>
> Well, I mean, what you seem to be suggesting here is that somebody is
> driving me with a stick to do something that I don't really like but
> have to do because otherwise I won't be able to make rent, but that's
> actually not the case. I genuinely believe that this is a good design,
> and it's driven by me, not some shadowy conglomerate of EnterpriseDB
> executives who are out to make PostgreSQL sucks. If I'm wrong and the
> design sucks, that's again not the fault of shadowy EnterpriseDB
> executives; it's my fault. Incidentally, my boss is not very shadowy
> anyhow; he's a super-nice guy, and a major reason why I work here. :-)

Then I just have to disagree, really vehemently, that having a
block-level incremental backup solution without solid dependency
handling between incremental and full backups, solid WAL management and
archiving, expiration handling for incremental/full backups and WAL, and
the manifest that that this thread has been about, is a good design.

Ultimately, what this calls for is some kind of 'repository' which
you've stressed you don't think is a good idea for pg_basebackup to ever
deal with and I just can't disagree more with that.  I could perhaps
agree that it isn't appropriate for the specific tool "pg_basebackup" to
work with a repo because of the goal of that particular tool, but in
that case, I don't think pg_basebackup should be the tool to provide a
block-level incremental backup solution, it should continue to be a tool
to provide a simple and easy way to take a one-time, complete, snapshot
of a running PG system over the replication protocol- and adding support
for parallel backups, or encrypted backups, or similar things would be
completely in-line and appropriate for such a tool, and I'm not against
those features being added to pg_basebackup even in advance of anything
like support for a repo or dependency handling.

> I don't think the issue here is that I haven't thought about what
> users want, but that not everybody wants the same thing, and it's
> seems like the people with whom I interact want somewhat different
> things than those with whom you interact. EnterpriseDB has an existing
> tool that does parallel and block-level incremental backup, and I
> started out with the goal of providing those same capabilities in
> core. They are quite popular with EnterpriseDB customers, and I'd like
> to make them more widely available and, as far as I can, improve on
> them. From our previous discussion and from a (brief) look at
> pgbackrest, I gather that the interests of your customers are somewhat
> different. Apparently, block-level incremental backup isn't quite as
> important to your customers, perhaps because you've already got
> file-level incremental backup, but various other things like
> encryption and backup verification are extremely important, and you've
> got a set of ideas about what would be valuable in the future which
> I'm sure is based on real input from your customers. I hope you pursue
> those ideas, and I hope you do it in core rather than in a separate
> piece of software, but that's up to you. Meanwhile, I think that if I
> have somewhat different ideas about what I'd like to pursue, that
> ought to be just fine. And I don't think it is unreasonable to hope
> that you'll acknowledge my goals as legitimate even if you have
> different ones.

I'm all for block-level incremental backup, in general (though I've got
concerns about it from a correctness standpoint..  I certainly think
it's going to be difficult to get right and probably finicky, but
hopefully your experience with BART has let you identify where the
dragons lie and it'll be interesting to see what that code looks like
and if the approach used can be leveraged in other tools), but I am
concerned about how we're getting there.

> I want to point out that my idea about how to do all of this has
> shifted by a considerable amount based on the input that you and David
> have provided. My original design didn't involve a backup manifest,
> but now it does. That turned out to be necessary, but it was also
> something you suggested, and something where I asked and took advice
> on what ought to go into it. Likewise, you suggested that the process
> of taking the backup should involve giving the client more control
> rather than trying to do everything on the server side, and that is
> now the design which I plan to pursue. You suggested that because it
> would be more advantageous for out-of-core backup tools, such as
> pgbackrest, and I acknowledge that as a benefit and I think we're
> headed in that direction. I am not doing a single thing which, to my
> knowledge, blocks anything that you might want to do with
> pg_basebackup in the future. I have accepted as much of your input as
> I believe that I can without killing the project off completely. To go
> further, I'd have to either accept years of delay or abandon my
> priorities entirely and pursue yours.

While I'm hopeful that the parallel backup pieces will be useful to
out-of-core backup tools, I've been increasingly less confident that
it'll end up being very useful to pgbackrest, as much as I would like it
to be.  Perhaps after it's in place we might be able to work on it to
make it useful, but we'd need to push all the features like encryption
and options for compression and such into the backend, in a way that
works for pgbackrest, to be able to leverage it, and I'm not sure that
would get much support or that it could be done in a way that doesn't
end up causing problems for pg_basebackup, which clearly wouldn't be
acceptable.  Further, if we can't leverage the PG backup protocol that
you're building here, it seems pretty darn unlikely we'd have much use
for the manifest that's built as part of that.

I'm probably going to lose what credibility I have in critizing what
you're doing with pg_basebackup here, but I started off saying you don't
have to make me happy and this is part of why- I really don't think
there's much that you're doing with pg_basebackup that is ultimately
going to impact what plans I have for the future, for pretty much
anything.  I haven't got any real specific plans around pg_basebackup,
though, point-in-fact, if you put in a bunch of code that shows how to
get PG and pg_basebackup to do block-level incremental backups in a safe
and trusted way, that would actually be *really* useful to the
pgbackrest project because we could then lift that logic out of
pg_basebackup and leverage it.  If I wanted to be entirely selfish, I'd
be pushing you to get block-level incremental backup into pg_basebackup
as quickly as possible so that we could have such an example of "how to
do it in a way that, if it breaks, the PG community will figure out what
went wrong and fix it".  If you look at other things we've done, such as
not backing up unlogged tables, that's exactly the approach we've used:
introduce the feature into pg_basebackup *first*, make sure the
community agrees that it's a valid approach and will deal with any
issues with it (and will take pains to avoid *breaking* it in future
versions..), and only *then* introduce it into pgbackrest by using the
same approach.  Those other features were well in-line with what makes
sense for pg_basebackup too though.

We haven't done that though, and I haven't been pushing in that
direction, not because I think it's a bad feature or that I want to
block something going into pg_basebackup or whatever, but because I
think it's actually going to cause more problems for users than it
solves because some users will want to use it (though not all, as we've
seen on this list, as there's at least some users out there who are as
scared of the idea of having *just* this in pg_basebackup without the
other things I talk about above as I am) and then they're going to try
and hack together all those other things they need around WAL management
and archiving and expiration and they're likely to get it wrong- perhaps
in obvious ways, perhaps in relatively subtle ways, but either way,
they'll end up with backups that aren't valid that they only discover
when they're in an emergency.  Again, perhaps selfish me would say "oh
good, then they'll call me and pay me lots to fix it for them", but it
certainly wouldn't look good for the community- even if all of the
documentation and everything we put out there says that they way they
were doing it had this subtle issue or whatever (considering our docs
still promote a really bad, imv anyway, archive command kinda makes this
likely, if you ask me anyway..), and it wouldn't be good for the user.

> > That's what would make *me* happy.  Even some comments about how to
> > *get* there while also working towards these features would be likely
> > to make me happy.  Instead, I feel like we're being told that we need
> > this feature badly in v13 and we're going to cut bait and do whatever
> > is necessary to get us there.
>
> This seems like a really unfair accusation given how much work I've
> put into trying to satisfy you and David. If this patch, the parallel
> full backup patch, and the incremental backup patch were all to get
> committed to v13, an outcome which seems pretty unlikely to me at this
> point, then you would have a very significant number of things that
> you have requested in the course of the various discussions, and
> AFAICS the only thing you'd have that you don't want is the need to
> parse the manifest file use while (<>) { @a = split /\t/, $_ } rather
> than $a = parse_json(join '', <>). You would, for example, have the
> ability to request an individual file from the server rather than a
> complete tarball. Maybe the command that requests a file would lack an
> encryption option, something which IIUC you would like to have, but
> that certainly does not leave you worse off. It is easier to add an
> encryption option to a command which you already have than it is to
> invent a whole new command -- or really several whole new commands,
> since such a command is not really usable unless you also have
> facilities to start and stop a backup through the replication
> protocol.

No, the manifest format is definitely not the only issue that I have
with this- but as it relates to the thread about building a manifest, my
complaint really is isolated to the format and just forward thinking
about how the format you're advocating for will mean custom code for who
knows how many different tools.  While I appreciate the offer to write
all the bespoke code for every version of the manifest for pgbackrest,
I'm really not thrilled about the idea of having to have that extra code
and having to then maintain it.  Yes, when you compare the single format
of the manifest and the code required for it against a JSON parser, if
we only ever have this one format then it'd win in terms of code, but I
don't believe it'll end up being one format, instead we're going to end
up with multiple formats, each of which will have some additional code
for dealing with parsing it, and that's going to add up.  That's also
going to, as I said before, make it almost certain that we can't use
older tools with newer backups.  These are issues that we've thought
about and worried about over the years of pgbackrest and with that
experience we've come down on the side that a JSON-based format would be
an altogether better design.  That's why we're advocating for it, not
because it requires more code or so that it delays the efforts here, but
because we've been there, we've used other formats, we've dealt with
user complaints when we do break things, this is all history for us
that's helped us learn- for PG, it looks like the future with a static
format, and I get that the future is hard to predict and pg_basebackup
isn't pgbackrest and yeah, I could be completely wrong because I don't
actually have a crystal ball, but this starting point sure looks really
familiar.

Thanks,

Stephen

Hi Robert,

On 1/7/20 6:33 PM, Stephen Frost wrote:

 > These are issues that we've thought
 > about and worried about over the years of pgbackrest and with that
 > experience we've come down on the side that a JSON-based format would be
 > an altogether better design.  That's why we're advocating for it, not
 > because it requires more code or so that it delays the efforts here, but
 > because we've been there, we've used other formats, we've dealt with
 > user complaints when we do break things, this is all history for us
 > that's helped us learn- for PG, it looks like the future with a static
 > format, and I get that the future is hard to predict and pg_basebackup
 > isn't pgbackrest and yeah, I could be completely wrong because I don't
 > actually have a crystal ball, but this starting point sure looks really
 > familiar.

For example, have you considered what will happen if you have a file in 
the cluster with a tab in the name?  This is perfectly valid in Posix 
filesystems, at least.  You may already be escaping tabs but the simple 
code snippet you provided earlier isn't going to work so well either 
way.  It gets complicated quickly.

I know users should not be creating weird files in PGDATA, but it's 
amazing how often this sort of thing pops up.  We currently have an open 
issue because = in file names breaks our file format.  Tab is surely 
less common but it's amazing what users will do.

Another fun one is 03849840 which fixes the handling of \ characters in 
the code which checksums the manifest.  The file is not fully JSON but 
the checksums are and that was initially missed in the C migration.  The 
bug never got released but it easily could have been.

In short, using a quick-and-dirty homegrown format seemed great at first 
but has caused many headaches.  Because we don't change the repo format 
across releases we are kind of stuck with past sins until we create a 
new repo format and write update/compatability code.  Users are 
understandably concerned if new versions of the software won't work with 
their repo, some of which contain years of backups (really).

This doesn't even get into the work everyone else will need to do to 
read a custom format.  I do appreciate your offer of contributing parser 
code to pgBackRest, but honestly I'd rather it were not necessary. 
Though of course I'd still love to see a contribution of some sort from you!

Hard experience tells me that using a standard format where all these 
issues have been worked out is the way to go.

There are a few MIT-licensed JSON projects that are implemented in a 
single file.  cJSON is very capable while JSMN is very minimal. Is is 
possible that one of those (or something like it) would be acceptable? 
It looks like the one requirement we have is that the JSON can be 
streamed rather than just building up one big blob?  Even with that 
requirement there are a few tricks that can be used.  JSON nests rather 
nicely after all so the individual file records can be transmitted 
independently of the overall file format.

Your first question may be why didn't pgBackRest use one of those 
parsers?  The answer is that JSON parsing/rendering is pretty trivial. 
Memory management and a (datum-like) type system are the hard parts and 
pgBackRest already had those.

Would it be acceptable to bring in JSON code with a compatible license 
to use in libcommon?  If so I'm willing to help adapt that code for use 
in Postgres.  It's possible that the pgBackRest code could be adapted 
similarly, but it might make more sense to start from one of these 
general purpose parsers.

Thoughts?

-- 
-David
david@pgmasters.net

On Thu, Jan 9, 2020 at 8:19 PM David Steele <david@pgmasters.net> wrote:
> For example, have you considered what will happen if you have a file in
> the cluster with a tab in the name?  This is perfectly valid in Posix
> filesystems, at least.

Yeah, there's code for that in the patch I posted. I don't think the
validator patch deals with it, but that's fixable.

> You may already be escaping tabs but the simple
> code snippet you provided earlier isn't going to work so well either
> way.  It gets complicated quickly.

Sure, but obviously neither of those code snippets were intended to be
used straight out of the box. Even after you parse the manifest as
JSON, you would still - if you really want to validate it - check that
you have the keys and values you expect, that the individual field
values are sensible, etc. I still stand by my earlier contention that,
as things stand today, you can parse an ad-hoc format in less code
than a JSON format. If we had a JSON parser available on the front
end, I think it'd be roughly comparable, but maybe the JSON format
would come out a bit ahead. Not sure.

> There are a few MIT-licensed JSON projects that are implemented in a
> single file.  cJSON is very capable while JSMN is very minimal. Is is
> possible that one of those (or something like it) would be acceptable?
> It looks like the one requirement we have is that the JSON can be
> streamed rather than just building up one big blob?  Even with that
> requirement there are a few tricks that can be used.  JSON nests rather
> nicely after all so the individual file records can be transmitted
> independently of the overall file format.

I haven't really looked at these. I would have expected that including
a second JSON parser in core would provoke significant opposition.
Generally, people dislike having more than one piece of code to do the
same thing. I would also expect that depending on an external package
would provoke significant opposition. If we suck the code into core,
then we have to keep it up to date with the upstream, which is a
significant maintenance burden - look at all the time Tom has spent on
snowball, regex, and time zone code over the years. If we don't suck
the code into core but depend on it, then every developer needs to
have that package installed on their operating system, and every
packager has to make sure that it is being built for their OS so that
PostgreSQL can depend on it. Perhaps JSON is so popular today that
imposing such a requirement would provoke only a groundswell of
support, but based on past precedent I would assume that if I
committed a patch of this sort the chances that I'd have to revert it
would be about 99.9%. Optional dependencies for optional features are
usually pretty well-tolerated when they're clearly necessary: e.g. you
can't really do JIT without depending on something like LLVM, but the
bar for a mandatory dependency has historically been quite high.

> Would it be acceptable to bring in JSON code with a compatible license
> to use in libcommon?  If so I'm willing to help adapt that code for use
> in Postgres.  It's possible that the pgBackRest code could be adapted
> similarly, but it might make more sense to start from one of these
> general purpose parsers.

For the reasons above, I expect this approach would be rejected, by
Tom and by others.

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

Robert Haas <robertmhaas@gmail.com> writes:
> ... I would also expect that depending on an external package
> would provoke significant opposition. If we suck the code into core,
> then we have to keep it up to date with the upstream, which is a
> significant maintenance burden - look at all the time Tom has spent on
> snowball, regex, and time zone code over the years.

Also worth noting is that we have a seriously bad track record about
choosing external packages to depend on.  The regex code has no upstream
maintainer anymore (well, the Tcl guys seem to think that *we* are
upstream for that now), and snowball is next door to moribund.
With C not being a particularly hip language to develop in anymore,
it wouldn't surprise me in the least for any C-code JSON parser
we might pick to go dead pretty soon.

Between that problem and the likelihood that we'd need to make
significant code changes anyway to meet our own coding style etc
expectations, I think really we'd have to assume that we're going
to fork and maintain our own copy of any code we pick.

Now, if it's a small enough chunk of code (and really, how complex
is JSON parsing anyway) maybe that doesn't matter.  But I tend to
agree with Robert's position that it's a big ask for this patch
to introduce a frontend JSON parser.

            regards, tom lane

On Tue, Jan 14, 2020 at 12:53:04PM -0500, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > ... I would also expect that depending on an external package
> > would provoke significant opposition. If we suck the code into core,
> > then we have to keep it up to date with the upstream, which is a
> > significant maintenance burden - look at all the time Tom has spent on
> > snowball, regex, and time zone code over the years.
> 
> Also worth noting is that we have a seriously bad track record about
> choosing external packages to depend on.  The regex code has no upstream
> maintainer anymore (well, the Tcl guys seem to think that *we* are
> upstream for that now), and snowball is next door to moribund.
> With C not being a particularly hip language to develop in anymore,
> it wouldn't surprise me in the least for any C-code JSON parser
> we might pick to go dead pretty soon.

Given jq's extreme popularity and compatible license, I'd nominate that.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Greetings,

* David Fetter (david@fetter.org) wrote:
> On Tue, Jan 14, 2020 at 12:53:04PM -0500, Tom Lane wrote:
> > Robert Haas <robertmhaas@gmail.com> writes:
> > > ... I would also expect that depending on an external package
> > > would provoke significant opposition. If we suck the code into core,
> > > then we have to keep it up to date with the upstream, which is a
> > > significant maintenance burden - look at all the time Tom has spent on
> > > snowball, regex, and time zone code over the years.
> >
> > Also worth noting is that we have a seriously bad track record about
> > choosing external packages to depend on.  The regex code has no upstream
> > maintainer anymore (well, the Tcl guys seem to think that *we* are
> > upstream for that now), and snowball is next door to moribund.
> > With C not being a particularly hip language to develop in anymore,
> > it wouldn't surprise me in the least for any C-code JSON parser
> > we might pick to go dead pretty soon.
>
> Given jq's extreme popularity and compatible license, I'd nominate that.

I don't think that really changes Tom's concerns here about having an
"upstream" for this.

For my part, I don't really agree with the whole "we don't want two
different JSON parsers" when we've got two of a bunch of stuff between
the frontend and the backend, particularly since I don't really think
it'll end up being *that* much code.

My thought, which I had expressed to David (though he obviously didn't
entirely agree with me since he suggested the other options), was to
adapt the pgBackRest JSON parser, which isn't really all that much code.

Frustratingly, that code has got some internal pgBackRest dependency on
things like the memory context system (which looks, unsurprisingly, an
awful lot like what is in PG backend), the error handling and logging
systems (which are different from PG because they're quite intentionally
segregated from each other- something PG would benefit from, imv..), and
Variadics (known in the PG backend as Datums, and quite similar to
them..).

Even so, David's offered to adjust the code to use the frontend's memory
management (*cough* malloc()..), and error handling/logging, and he had
some idea for Variadics (or maybe just pulling the backend's Datum
system in..?  He could answer better), and basically write a frontend
JSON parser for PG without too much code, no external dependencies, and
to make sure it answers this requirement, and I've agreed that he can
spend some time on that instead of pgBackRest to get us through this, if
everyone else is agreeable to the idea.  Obviously this isn't intended
to box anyone in- if there turns out even after the code's been written
to be some fatal issue with using it, so be it, but we're offering to
help.

Thanks,

Stephen

On Tue, Jan 14, 2020 at 03:35:40PM -0500, Stephen Frost wrote:
> Greetings,
> 
> * David Fetter (david@fetter.org) wrote:
> > On Tue, Jan 14, 2020 at 12:53:04PM -0500, Tom Lane wrote:
> > > Robert Haas <robertmhaas@gmail.com> writes:
> > > > ... I would also expect that depending on an external package
> > > > would provoke significant opposition. If we suck the code into core,
> > > > then we have to keep it up to date with the upstream, which is a
> > > > significant maintenance burden - look at all the time Tom has spent on
> > > > snowball, regex, and time zone code over the years.
> > > 
> > > Also worth noting is that we have a seriously bad track record about
> > > choosing external packages to depend on.  The regex code has no upstream
> > > maintainer anymore (well, the Tcl guys seem to think that *we* are
> > > upstream for that now), and snowball is next door to moribund.
> > > With C not being a particularly hip language to develop in anymore,
> > > it wouldn't surprise me in the least for any C-code JSON parser
> > > we might pick to go dead pretty soon.
> > 
> > Given jq's extreme popularity and compatible license, I'd nominate that.
> 
> I don't think that really changes Tom's concerns here about having an
> "upstream" for this.
> 
> For my part, I don't really agree with the whole "we don't want two
> different JSON parsers" when we've got two of a bunch of stuff between
> the frontend and the backend, particularly since I don't really think
> it'll end up being *that* much code.
> 
> My thought, which I had expressed to David (though he obviously didn't
> entirely agree with me since he suggested the other options), was to
> adapt the pgBackRest JSON parser, which isn't really all that much code.
> 
> Frustratingly, that code has got some internal pgBackRest dependency on
> things like the memory context system (which looks, unsurprisingly, an
> awful lot like what is in PG backend), the error handling and logging
> systems (which are different from PG because they're quite intentionally
> segregated from each other- something PG would benefit from, imv..), and
> Variadics (known in the PG backend as Datums, and quite similar to
> them..).

It might be more fun to put in that infrastructure and have it gate
the manifest feature than to have two vastly different parsers to
contend with. I get that putting off the backup manifests isn't an
awesome prospect, but neither is rushing them in and getting them
wrong in ways we'll still be regretting a decade hence.

Best,
David.
-- 
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Hi Stephen,

On 1/14/20 1:35 PM, Stephen Frost wrote:
> 
> My thought, which I had expressed to David (though he obviously didn't
> entirely agree with me since he suggested the other options), was to
> adapt the pgBackRest JSON parser, which isn't really all that much code.

It's not that I didn't agree, it's just that the pgBackRest code does 
use mem contexts, the type system, etc.  After looking at some other 
solutions with similar amounts of code I thought they might be more 
acceptable.  At least it seemed like a good idea to throw it out there.

> Even so, David's offered to adjust the code to use the frontend's memory
> management (*cough* malloc()..), and error handling/logging, and he had
> some idea for Variadics (or maybe just pulling the backend's Datum
> system in..?  He could answer better), and basically write a frontend
> JSON parser for PG without too much code, no external dependencies, and
> to make sure it answers this requirement, and I've agreed that he can
> spend some time on that instead of pgBackRest to get us through this, if
> everyone else is agreeable to the idea.  

To keep it simple I think we are left with callbacks or a somewhat 
static "what's the next datum" kind of approach.  I think the latter 
could get us through a release or two while we make improvements.

> Obviously this isn't intended
> to box anyone in- if there turns out even after the code's been written
> to be some fatal issue with using it, so be it, but we're offering to
> help.

I'm happy to work up a prototype unless the consensus is that we 
absolutely don't want a second JSON parser in core.

Regards,
-- 
-David
david@pgmasters.net

David Steele <david@pgmasters.net> writes:
> I'm happy to work up a prototype unless the consensus is that we 
> absolutely don't want a second JSON parser in core.

How much code are we talking about?  If the answer is "a few hundred
lines", it's a lot easier to swallow than if it's "a few thousand".

            regards, tom lane

Hi Tom,

On 1/14/20 9:47 PM, Tom Lane wrote:
> David Steele <david@pgmasters.net> writes:
>> I'm happy to work up a prototype unless the consensus is that we
>> absolutely don't want a second JSON parser in core.
> 
> How much code are we talking about?  If the answer is "a few hundred
> lines", it's a lot easier to swallow than if it's "a few thousand".

It's currently about a thousand lines but we have a lot of functions to 
convert to/from specific types.  I imagine the line count would be 
similar using one of the approaches I discussed above.

Current source attached for reference.

Regards,
-- 
-David
david@pgmasters.net

On Tue, Jan 14, 2020 at 12:53:04PM -0500, Tom Lane wrote:
> Also worth noting is that we have a seriously bad track record about
> choosing external packages to depend on.  The regex code has no upstream
> maintainer anymore (well, the Tcl guys seem to think that *we* are
> upstream for that now), and snowball is next door to moribund.
> With C not being a particularly hip language to develop in anymore,
> it wouldn't surprise me in the least for any C-code JSON parser
> we might pick to go dead pretty soon.
> 
> Between that problem and the likelihood that we'd need to make
> significant code changes anyway to meet our own coding style etc
> expectations, I think really we'd have to assume that we're going
> to fork and maintain our own copy of any code we pick.
> 
> Now, if it's a small enough chunk of code (and really, how complex
> is JSON parsing anyway) maybe that doesn't matter.  But I tend to
> agree with Robert's position that it's a big ask for this patch
> to introduce a frontend JSON parser.

I know we have talked about our experience in maintaining external code:

*  TCL regex
*  Snowball
*  Timezone handling

However, the regex code is complex, and the Snowball and timezone code
is improved as they add new languages and time zones.  I don't see JSON
parsing as complex or likely to change much, so it might be acceptable
to include it in our frontend code.

As far as using tab-delimited data, I know this usage was compared to
postgresql.conf and pg_hba.conf, which don't change much.  However,
those files are not usually written, and do not contain user data, while
the backup file might contain user-specified paths if they are not just
relative to the PGDATA directory, and that would make escaping a
requirement.

-- 
  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 Fri, Jan 3, 2020 at 6:11 PM Suraj Kharage
<suraj.kharage@enterprisedb.com> wrote:
> Thank you for review comments.

Here's a new patch set for this feature.

0001 adds checksum helper functions, similar to what Suraj had
incorporated into my original patch but separated out into a separate
patch and with some different aesthetic decisions. I also decided to
support all of the SHA variants that PG knows about as options and
added a function to parse a checksum algorithm name, along the lines I
suggested previously.

0002 teaches the server to generate a backup manifest using the format
I originally proposed. This is similar to the patch I posted
previously, but it spools the manifest to disk as it's being
generated, so that we don't run the server out of memory or fail when
hitting the 1GB allocation limit.

0003 adds a new utility, pg_validatebackup, to validate a backup
against a manifest. Suraj tried to incorporate this into
pg_basebackup, which I initially thought might be OK but eventually
decided wasn't good, partly because this really wants to take some
command-line options entirely unrelated to the options accepted by
pg_basebackup. I tried to improve the error checking and the order in
which various things are done, too. This is a basically a complete
rewrite as compared with Suraj's version.

0004 modifies the server to generate a backup manifest in JSON format
rather than my originally proposed format. This allows for some
comparison of the code doing it one way vs. the other. Assuming we
stick with JSON, I will squash this with 0002 at some point.

0005 is a very much work-in-progress and proof-of-concept to modify
the backup validator to understand the JSON format. It doesn't
validate the manifest checksum at this point; it just prints it out.
The error handling needs work. It has other problems, and bugs.
Although I'm still not very happy about the idea of using JSON here,
I'm pretty happy with the basic approach this patch takes. It
demonstrates that the JSON parser can be used for non-trivial things
in frontend code, and I'd say the code even looks reasonably clean -
with the exception of small details like being buggy and
under-commented.

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

On 2/27/20 9:22 PM, Robert Haas wrote:
> Here's a new patch set for this feature.

Thanks Robert.  After applying all the 5 patches (v8-00*) against PG v13 
(commit id -afb5465e0cfce7637066eaaaeecab30b0f23fbe3) ,

There are few issues/observations

1)Getting segmentation fault error if  we try pg_validatebackup against  
a valid backup_manifest file but data directory path is WRONG

[centos@tushar-ldap-docker bin]$ ./pg_basebackup -D bk 
--manifest-checksums=sha224

[centos@tushar-ldap-docker bin]$ cp bk/backup_manifest /tmp/.

[centos@tushar-ldap-docker bin]$ ./pg_validatebackup -m 
/tmp/backup_manifest    random_directory/
pg_validatebackup: * manifest_checksum = 
f0460cd6aa13cf0c5e35426a41af940a9231e6425cd65115a19778b7abfdaef9
pg_validatebackup: error: could not open directory "random_directory": 
No such file or directory
Segmentation fault

2) when used '-R' option at the time of create base backup

[centos@tushar-ldap-docker bin]$ ./pg_basebackup -D bar -R
[centos@tushar-ldap-docker bin]$ ./pg_validatebackup  bar
pg_validatebackup: * manifest_checksum = 
a195d3a3a82a41200c9ac92c12d764d23c810e7e91b31c44a7d04f67ce012edc
pg_validatebackup: error: "standby.signal" is present on disk but not in 
the manifest
pg_validatebackup: error: "postgresql.auto.conf" has size 286 on disk 
but size 88 in the manifest
[centos@tushar-ldap-docker bin]$

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On 3/3/20 4:04 PM, tushar wrote:
> Thanks Robert.  After applying all the 5 patches (v8-00*) against PG 
> v13 (commit id -afb5465e0cfce7637066eaaaeecab30b0f23fbe3) , 

There is a scenario where pg_validatebackup is not throwing an error if 
some file deleted from pg_wal/ folder and  but later at the time of 
restoring - we are getting an error

[centos@tushar-ldap-docker bin]$ ./pg_basebackup  -D test1

[centos@tushar-ldap-docker bin]$ ls test1/pg_wal/
000000010000000000000010  archive_status

[centos@tushar-ldap-docker bin]$ rm -rf test1/pg_wal/*

[centos@tushar-ldap-docker bin]$ ./pg_validatebackup test1
pg_validatebackup: * manifest_checksum = 
88f1ed995c83e86252466a2c88b3e660a69cfc76c169991134b101c4f16c9df7
pg_validatebackup: backup successfully verified

[centos@tushar-ldap-docker bin]$ ./pg_ctl -D test1 start -o '-p 3333'
waiting for server to start....2020-03-02 20:05:22.732 IST [21441] LOG:  
starting PostgreSQL 13devel on x86_64-pc-linux-gnu, compiled by gcc 
(GCC) 4.8.5 20150623 (Red Hat 4.8.5-39), 64-bit
2020-03-02 20:05:22.733 IST [21441] LOG:  listening on IPv6 address 
"::1", port 3333
2020-03-02 20:05:22.733 IST [21441] LOG:  listening on IPv4 address 
"127.0.0.1", port 3333
2020-03-02 20:05:22.736 IST [21441] LOG:  listening on Unix socket 
"/tmp/.s.PGSQL.3333"
2020-03-02 20:05:22.739 IST [21442] LOG:  database system was 
interrupted; last known up at 2020-03-02 20:04:35 IST
2020-03-02 20:05:22.739 IST [21442] LOG:  creating missing WAL directory 
"pg_wal/archive_status"
2020-03-02 20:05:22.886 IST [21442] LOG:  invalid checkpoint record
2020-03-02 20:05:22.886 IST [21442] FATAL:  could not locate required 
checkpoint record
2020-03-02 20:05:22.886 IST [21442] HINT:  If you are restoring from a 
backup, touch 
"/home/centos/pg13_bk_mani/edb/edbpsql/bin/test1/recovery.signal" and 
add required recovery options.
     If you are not restoring from a backup, try removing the file 
"/home/centos/pg13_bk_mani/edb/edbpsql/bin/test1/backup_label".
     Be careful: removing 
"/home/centos/pg13_bk_mani/edb/edbpsql/bin/test1/backup_label" will 
result in a corrupt cluster if restoring from a backup.
2020-03-02 20:05:22.886 IST [21441] LOG:  startup process (PID 21442) 
exited with exit code 1
2020-03-02 20:05:22.886 IST [21441] LOG:  aborting startup due to 
startup process failure
2020-03-02 20:05:22.889 IST [21441] LOG:  database system is shut down
  stopped waiting
pg_ctl: could not start server
Examine the log output.
[centos@tushar-ldap-docker bin]$

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

Hi,
Another observation , if i change the ownership of a file which is under 
global/ directory
i.e

[root@tushar-ldap-docker global]# chown enterprisedb 2396

and run the pg_validatebackup command, i am getting this message -

[centos@tushar-ldap-docker bin]$ ./pg_validatebackup gggg
pg_validatebackup: * manifest_checksum = 
e8cb007bcc9c0deab6eff51cd8d9d9af6af35b86e02f3055e60e70e56737e877
pg_validatebackup: error: could not open file "global/2396": Permission 
denied
*** Error in `./pg_validatebackup': double free or corruption (!prev): 
0x0000000001850ba0 ***
======= Backtrace: =========
/lib64/libc.so.6(+0x81679)[0x7fa2248e3679]
./pg_validatebackup[0x401f4c]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7fa224884505]
./pg_validatebackup[0x402049]
======= Memory map: ========
00400000-00415000 r-xp 00000000 fd:03 4044545 
/home/centos/pg13_bk_mani/edb/edbpsql/bin/pg_validatebackup
00614000-00615000 r--p 00014000 fd:03 4044545 
/home/centos/pg13_bk_mani/edb/edbpsql/bin/pg_validatebackup
00615000-00616000 rw-p 00015000 fd:03 4044545 
/home/centos/pg13_bk_mani/edb/edbpsql/bin/pg_validatebackup
017f3000-01878000 rw-p 00000000 00:00 0                                  
[heap]
7fa218000000-7fa218021000 rw-p 00000000 00:00 0
7fa218021000-7fa21c000000 ---p 00000000 00:00 0
7fa21e122000-7fa21e137000 r-xp 00000000 fd:03 141697                     
/usr/lib64/libgcc_s-4.8.5-20150702.so.1
7fa21e137000-7fa21e336000 ---p 00015000 fd:03 141697                     
/usr/lib64/libgcc_s-4.8.5-20150702.so.1
7fa21e336000-7fa21e337000 r--p 00014000 fd:03 141697                     
/usr/lib64/libgcc_s-4.8.5-20150702.so.1
7fa21e337000-7fa21e338000 rw-p 00015000 fd:03 141697                     
/usr/lib64/libgcc_s-4.8.5-20150702.so.1
7fa21e338000-7fa224862000 r--p 00000000 fd:03 266442                     
/usr/lib/locale/locale-archive
7fa224862000-7fa224a25000 r-xp 00000000 fd:03 134456                     
/usr/lib64/libc-2.17.so
7fa224a25000-7fa224c25000 ---p 001c3000 fd:03 134456                     
/usr/lib64/libc-2.17.so
7fa224c25000-7fa224c29000 r--p 001c3000 fd:03 134456                     
/usr/lib64/libc-2.17.so
7fa224c29000-7fa224c2b000 rw-p 001c7000 fd:03 134456                     
/usr/lib64/libc-2.17.so
7fa224c2b000-7fa224c30000 rw-p 00000000 00:00 0
7fa224c30000-7fa224c47000 r-xp 00000000 fd:03 134485                     
/usr/lib64/libpthread-2.17.so
7fa224c47000-7fa224e46000 ---p 00017000 fd:03 134485                     
/usr/lib64/libpthread-2.17.so
7fa224e46000-7fa224e47000 r--p 00016000 fd:03 134485                     
/usr/lib64/libpthread-2.17.so
7fa224e47000-7fa224e48000 rw-p 00017000 fd:03 134485                     
/usr/lib64/libpthread-2.17.so
7fa224e48000-7fa224e4c000 rw-p 00000000 00:00 0
7fa224e4c000-7fa224e90000 r-xp 00000000 fd:03 4044478 
/home/centos/pg13_bk_mani/edb/edbpsql/lib/libpq.so.5.13
7fa224e90000-7fa225090000 ---p 00044000 fd:03 4044478 
/home/centos/pg13_bk_mani/edb/edbpsql/lib/libpq.so.5.13
7fa225090000-7fa225093000 r--p 00044000 fd:03 4044478 
/home/centos/pg13_bk_mani/edb/edbpsql/lib/libpq.so.5.13
7fa225093000-7fa225094000 rw-p 00047000 fd:03 4044478 
/home/centos/pg13_bk_mani/edb/edbpsql/lib/libpq.so.5.13
7fa225094000-7fa2250b6000 r-xp 00000000 fd:03 130333                     
/usr/lib64/ld-2.17.so
7fa22527d000-7fa2252a2000 rw-p 00000000 00:00 0
7fa2252b3000-7fa2252b5000 rw-p 00000000 00:00 0
7fa2252b5000-7fa2252b6000 r--p 00021000 fd:03 130333                     
/usr/lib64/ld-2.17.so
7fa2252b6000-7fa2252b7000 rw-p 00022000 fd:03 130333                     
/usr/lib64/ld-2.17.so
7fa2252b7000-7fa2252b8000 rw-p 00000000 00:00 0
7ffdf354f000-7ffdf3570000 rw-p 00000000 00:00 0                          
[stack]
7ffdf3572000-7ffdf3574000 r-xp 00000000 00:00 0                          
[vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  
[vsyscall]
Aborted
[centos@tushar-ldap-docker bin]$


I am getting the error message but along with "*** Error in 
`./pg_validatebackup': double free or corruption (!prev): 
0x0000000001850ba0 ***"  messages

Is this expected ?

regards,

On 3/3/20 8:19 PM, tushar wrote:
> On 3/3/20 4:04 PM, tushar wrote:
>> Thanks Robert.  After applying all the 5 patches (v8-00*) against PG 
>> v13 (commit id -afb5465e0cfce7637066eaaaeecab30b0f23fbe3) , 
>
> There is a scenario where pg_validatebackup is not throwing an error 
> if some file deleted from pg_wal/ folder and  but later at the time of 
> restoring - we are getting an error
>
> [centos@tushar-ldap-docker bin]$ ./pg_basebackup  -D test1
>
> [centos@tushar-ldap-docker bin]$ ls test1/pg_wal/
> 000000010000000000000010  archive_status
>
> [centos@tushar-ldap-docker bin]$ rm -rf test1/pg_wal/*
>
> [centos@tushar-ldap-docker bin]$ ./pg_validatebackup test1
> pg_validatebackup: * manifest_checksum = 
> 88f1ed995c83e86252466a2c88b3e660a69cfc76c169991134b101c4f16c9df7
> pg_validatebackup: backup successfully verified
>
> [centos@tushar-ldap-docker bin]$ ./pg_ctl -D test1 start -o '-p 3333'
> waiting for server to start....2020-03-02 20:05:22.732 IST [21441] 
> LOG:  starting PostgreSQL 13devel on x86_64-pc-linux-gnu, compiled by 
> gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-39), 64-bit
> 2020-03-02 20:05:22.733 IST [21441] LOG:  listening on IPv6 address 
> "::1", port 3333
> 2020-03-02 20:05:22.733 IST [21441] LOG:  listening on IPv4 address 
> "127.0.0.1", port 3333
> 2020-03-02 20:05:22.736 IST [21441] LOG:  listening on Unix socket 
> "/tmp/.s.PGSQL.3333"
> 2020-03-02 20:05:22.739 IST [21442] LOG:  database system was 
> interrupted; last known up at 2020-03-02 20:04:35 IST
> 2020-03-02 20:05:22.739 IST [21442] LOG:  creating missing WAL 
> directory "pg_wal/archive_status"
> 2020-03-02 20:05:22.886 IST [21442] LOG:  invalid checkpoint record
> 2020-03-02 20:05:22.886 IST [21442] FATAL:  could not locate required 
> checkpoint record
> 2020-03-02 20:05:22.886 IST [21442] HINT:  If you are restoring from a 
> backup, touch 
> "/home/centos/pg13_bk_mani/edb/edbpsql/bin/test1/recovery.signal" and 
> add required recovery options.
>     If you are not restoring from a backup, try removing the file 
> "/home/centos/pg13_bk_mani/edb/edbpsql/bin/test1/backup_label".
>     Be careful: removing 
> "/home/centos/pg13_bk_mani/edb/edbpsql/bin/test1/backup_label" will 
> result in a corrupt cluster if restoring from a backup.
> 2020-03-02 20:05:22.886 IST [21441] LOG:  startup process (PID 21442) 
> exited with exit code 1
> 2020-03-02 20:05:22.886 IST [21441] LOG:  aborting startup due to 
> startup process failure
> 2020-03-02 20:05:22.889 IST [21441] LOG:  database system is shut down
>  stopped waiting
> pg_ctl: could not start server
> Examine the log output.
> [centos@tushar-ldap-docker bin]$
>

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

Another scenario, in which if we modify Manifest-Checksum" value from 
backup_manifest file , we are not getting an error

[centos@tushar-ldap-docker bin]$ ./pg_validatebackup data/
pg_validatebackup: * manifest_checksum = 
28d082921650d0ae881de8ceb122c8d2af5f449f51ecfb446827f7f49f91f65d
pg_validatebackup: backup successfully verified

open backup_manifest file and replace

"Manifest-Checksum": 
"8d082921650d0ae881de8ceb122c8d2af5f449f51ecfb446827f7f49f91f65d"}
with
"Manifest-Checksum": "Hello World"}

rerun the pg_validatebackup

[centos@tushar-ldap-docker bin]$ ./pg_validatebackup data/
pg_validatebackup: * manifest_checksum = Hello World
pg_validatebackup: backup successfully verified

regards,

On 3/4/20 3:26 PM, tushar wrote:
> Hi,
> Another observation , if i change the ownership of a file which is 
> under global/ directory
> i.e
>
> [root@tushar-ldap-docker global]# chown enterprisedb 2396
>
> and run the pg_validatebackup command, i am getting this message -
>
> [centos@tushar-ldap-docker bin]$ ./pg_validatebackup gggg
> pg_validatebackup: * manifest_checksum = 
> e8cb007bcc9c0deab6eff51cd8d9d9af6af35b86e02f3055e60e70e56737e877
> pg_validatebackup: error: could not open file "global/2396": 
> Permission denied
> *** Error in `./pg_validatebackup': double free or corruption (!prev): 
> 0x0000000001850ba0 ***
> ======= Backtrace: =========
> /lib64/libc.so.6(+0x81679)[0x7fa2248e3679]
> ./pg_validatebackup[0x401f4c]
> /lib64/libc.so.6(__libc_start_main+0xf5)[0x7fa224884505]
> ./pg_validatebackup[0x402049]
> ======= Memory map: ========
> 00400000-00415000 r-xp 00000000 fd:03 4044545 
> /home/centos/pg13_bk_mani/edb/edbpsql/bin/pg_validatebackup
> 00614000-00615000 r--p 00014000 fd:03 4044545 
> /home/centos/pg13_bk_mani/edb/edbpsql/bin/pg_validatebackup
> 00615000-00616000 rw-p 00015000 fd:03 4044545 
> /home/centos/pg13_bk_mani/edb/edbpsql/bin/pg_validatebackup
> 017f3000-01878000 rw-p 00000000 00:00 
> 0                                  [heap]
> 7fa218000000-7fa218021000 rw-p 00000000 00:00 0
> 7fa218021000-7fa21c000000 ---p 00000000 00:00 0
> 7fa21e122000-7fa21e137000 r-xp 00000000 fd:03 
> 141697                     /usr/lib64/libgcc_s-4.8.5-20150702.so.1
> 7fa21e137000-7fa21e336000 ---p 00015000 fd:03 
> 141697                     /usr/lib64/libgcc_s-4.8.5-20150702.so.1
> 7fa21e336000-7fa21e337000 r--p 00014000 fd:03 
> 141697                     /usr/lib64/libgcc_s-4.8.5-20150702.so.1
> 7fa21e337000-7fa21e338000 rw-p 00015000 fd:03 
> 141697                     /usr/lib64/libgcc_s-4.8.5-20150702.so.1
> 7fa21e338000-7fa224862000 r--p 00000000 fd:03 
> 266442                     /usr/lib/locale/locale-archive
> 7fa224862000-7fa224a25000 r-xp 00000000 fd:03 
> 134456                     /usr/lib64/libc-2.17.so
> 7fa224a25000-7fa224c25000 ---p 001c3000 fd:03 
> 134456                     /usr/lib64/libc-2.17.so
> 7fa224c25000-7fa224c29000 r--p 001c3000 fd:03 
> 134456                     /usr/lib64/libc-2.17.so
> 7fa224c29000-7fa224c2b000 rw-p 001c7000 fd:03 
> 134456                     /usr/lib64/libc-2.17.so
> 7fa224c2b000-7fa224c30000 rw-p 00000000 00:00 0
> 7fa224c30000-7fa224c47000 r-xp 00000000 fd:03 
> 134485                     /usr/lib64/libpthread-2.17.so
> 7fa224c47000-7fa224e46000 ---p 00017000 fd:03 
> 134485                     /usr/lib64/libpthread-2.17.so
> 7fa224e46000-7fa224e47000 r--p 00016000 fd:03 
> 134485                     /usr/lib64/libpthread-2.17.so
> 7fa224e47000-7fa224e48000 rw-p 00017000 fd:03 
> 134485                     /usr/lib64/libpthread-2.17.so
> 7fa224e48000-7fa224e4c000 rw-p 00000000 00:00 0
> 7fa224e4c000-7fa224e90000 r-xp 00000000 fd:03 4044478 
> /home/centos/pg13_bk_mani/edb/edbpsql/lib/libpq.so.5.13
> 7fa224e90000-7fa225090000 ---p 00044000 fd:03 4044478 
> /home/centos/pg13_bk_mani/edb/edbpsql/lib/libpq.so.5.13
> 7fa225090000-7fa225093000 r--p 00044000 fd:03 4044478 
> /home/centos/pg13_bk_mani/edb/edbpsql/lib/libpq.so.5.13
> 7fa225093000-7fa225094000 rw-p 00047000 fd:03 4044478 
> /home/centos/pg13_bk_mani/edb/edbpsql/lib/libpq.so.5.13
> 7fa225094000-7fa2250b6000 r-xp 00000000 fd:03 
> 130333                     /usr/lib64/ld-2.17.so
> 7fa22527d000-7fa2252a2000 rw-p 00000000 00:00 0
> 7fa2252b3000-7fa2252b5000 rw-p 00000000 00:00 0
> 7fa2252b5000-7fa2252b6000 r--p 00021000 fd:03 
> 130333                     /usr/lib64/ld-2.17.so
> 7fa2252b6000-7fa2252b7000 rw-p 00022000 fd:03 
> 130333                     /usr/lib64/ld-2.17.so
> 7fa2252b7000-7fa2252b8000 rw-p 00000000 00:00 0
> 7ffdf354f000-7ffdf3570000 rw-p 00000000 00:00 
> 0                          [stack]
> 7ffdf3572000-7ffdf3574000 r-xp 00000000 00:00 
> 0                          [vdso]
> ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 
> 0                  [vsyscall]
> Aborted
> [centos@tushar-ldap-docker bin]$
>
>
> I am getting the error message but along with "*** Error in 
> `./pg_validatebackup': double free or corruption (!prev): 
> 0x0000000001850ba0 ***"  messages
>
> Is this expected ?
>
> regards,
>
> On 3/3/20 8:19 PM, tushar wrote:
>> On 3/3/20 4:04 PM, tushar wrote:
>>> Thanks Robert.  After applying all the 5 patches (v8-00*) against PG 
>>> v13 (commit id -afb5465e0cfce7637066eaaaeecab30b0f23fbe3) , 
>>
>> There is a scenario where pg_validatebackup is not throwing an error 
>> if some file deleted from pg_wal/ folder and  but later at the time 
>> of restoring - we are getting an error
>>
>> [centos@tushar-ldap-docker bin]$ ./pg_basebackup  -D test1
>>
>> [centos@tushar-ldap-docker bin]$ ls test1/pg_wal/
>> 000000010000000000000010  archive_status
>>
>> [centos@tushar-ldap-docker bin]$ rm -rf test1/pg_wal/*
>>
>> [centos@tushar-ldap-docker bin]$ ./pg_validatebackup test1
>> pg_validatebackup: * manifest_checksum = 
>> 88f1ed995c83e86252466a2c88b3e660a69cfc76c169991134b101c4f16c9df7
>> pg_validatebackup: backup successfully verified
>>
>> [centos@tushar-ldap-docker bin]$ ./pg_ctl -D test1 start -o '-p 3333'
>> waiting for server to start....2020-03-02 20:05:22.732 IST [21441] 
>> LOG:  starting PostgreSQL 13devel on x86_64-pc-linux-gnu, compiled by 
>> gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-39), 64-bit
>> 2020-03-02 20:05:22.733 IST [21441] LOG:  listening on IPv6 address 
>> "::1", port 3333
>> 2020-03-02 20:05:22.733 IST [21441] LOG:  listening on IPv4 address 
>> "127.0.0.1", port 3333
>> 2020-03-02 20:05:22.736 IST [21441] LOG:  listening on Unix socket 
>> "/tmp/.s.PGSQL.3333"
>> 2020-03-02 20:05:22.739 IST [21442] LOG:  database system was 
>> interrupted; last known up at 2020-03-02 20:04:35 IST
>> 2020-03-02 20:05:22.739 IST [21442] LOG:  creating missing WAL 
>> directory "pg_wal/archive_status"
>> 2020-03-02 20:05:22.886 IST [21442] LOG:  invalid checkpoint record
>> 2020-03-02 20:05:22.886 IST [21442] FATAL:  could not locate required 
>> checkpoint record
>> 2020-03-02 20:05:22.886 IST [21442] HINT:  If you are restoring from 
>> a backup, touch 
>> "/home/centos/pg13_bk_mani/edb/edbpsql/bin/test1/recovery.signal" and 
>> add required recovery options.
>>     If you are not restoring from a backup, try removing the file 
>> "/home/centos/pg13_bk_mani/edb/edbpsql/bin/test1/backup_label".
>>     Be careful: removing 
>> "/home/centos/pg13_bk_mani/edb/edbpsql/bin/test1/backup_label" will 
>> result in a corrupt cluster if restoring from a backup.
>> 2020-03-02 20:05:22.886 IST [21441] LOG:  startup process (PID 21442) 
>> exited with exit code 1
>> 2020-03-02 20:05:22.886 IST [21441] LOG:  aborting startup due to 
>> startup process failure
>> 2020-03-02 20:05:22.889 IST [21441] LOG:  database system is shut down
>>  stopped waiting
>> pg_ctl: could not start server
>> Examine the log output.
>> [centos@tushar-ldap-docker bin]$
>>
>

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

Another scenario, in which if we modify Manifest-Checksum" value from backup_manifest file , we are not getting an error

[centos@tushar-ldap-docker bin]$ ./pg_validatebackup data/
pg_validatebackup: * manifest_checksum = 28d082921650d0ae881de8ceb122c8d2af5f449f51ecfb446827f7f49f91f65d
pg_validatebackup: backup successfully verified

open backup_manifest file and replace

"Manifest-Checksum": "8d082921650d0ae881de8ceb122c8d2af5f449f51ecfb446827f7f49f91f65d"}
with
"Manifest-Checksum": "Hello World"}

rerun the pg_validatebackup

[centos@tushar-ldap-docker bin]$ ./pg_validatebackup data/
pg_validatebackup: * manifest_checksum = Hello World
pg_validatebackup: backup successfully verified

regards,

On 3/4/20 3:26 PM, tushar wrote:

Hi,
Another observation , if i change the ownership of a file which is under global/ directory
i.e

[root@tushar-ldap-docker global]# chown enterprisedb 2396

and run the pg_validatebackup command, i am getting this message -

[centos@tushar-ldap-docker bin]$ ./pg_validatebackup gggg
pg_validatebackup: * manifest_checksum = e8cb007bcc9c0deab6eff51cd8d9d9af6af35b86e02f3055e60e70e56737e877
pg_validatebackup: error: could not open file "global/2396": Permission denied
*** Error in `./pg_validatebackup': double free or corruption (!prev): 0x0000000001850ba0 ***
======= Backtrace: =========
/lib64/libc.so.6(+0x81679)[0x7fa2248e3679]
./pg_validatebackup[0x401f4c]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7fa224884505]
./pg_validatebackup[0x402049]
======= Memory map: ========
00400000-00415000 r-xp 00000000 fd:03 4044545 /home/centos/pg13_bk_mani/edb/edbpsql/bin/pg_validatebackup
00614000-00615000 r--p 00014000 fd:03 4044545 /home/centos/pg13_bk_mani/edb/edbpsql/bin/pg_validatebackup
00615000-00616000 rw-p 00015000 fd:03 4044545 /home/centos/pg13_bk_mani/edb/edbpsql/bin/pg_validatebackup
017f3000-01878000 rw-p 00000000 00:00 0                                  [heap]
7fa218000000-7fa218021000 rw-p 00000000 00:00 0
7fa218021000-7fa21c000000 ---p 00000000 00:00 0
7fa21e122000-7fa21e137000 r-xp 00000000 fd:03 141697                     /usr/lib64/libgcc_s-4.8.5-20150702.so.1
7fa21e137000-7fa21e336000 ---p 00015000 fd:03 141697                     /usr/lib64/libgcc_s-4.8.5-20150702.so.1
7fa21e336000-7fa21e337000 r--p 00014000 fd:03 141697                     /usr/lib64/libgcc_s-4.8.5-20150702.so.1
7fa21e337000-7fa21e338000 rw-p 00015000 fd:03 141697                     /usr/lib64/libgcc_s-4.8.5-20150702.so.1
7fa21e338000-7fa224862000 r--p 00000000 fd:03 266442                     /usr/lib/locale/locale-archive
7fa224862000-7fa224a25000 r-xp 00000000 fd:03 134456                     /usr/lib64/libc-2.17.so
7fa224a25000-7fa224c25000 ---p 001c3000 fd:03 134456                     /usr/lib64/libc-2.17.so
7fa224c25000-7fa224c29000 r--p 001c3000 fd:03 134456                     /usr/lib64/libc-2.17.so
7fa224c29000-7fa224c2b000 rw-p 001c7000 fd:03 134456                     /usr/lib64/libc-2.17.so
7fa224c2b000-7fa224c30000 rw-p 00000000 00:00 0
7fa224c30000-7fa224c47000 r-xp 00000000 fd:03 134485                     /usr/lib64/libpthread-2.17.so
7fa224c47000-7fa224e46000 ---p 00017000 fd:03 134485                     /usr/lib64/libpthread-2.17.so
7fa224e46000-7fa224e47000 r--p 00016000 fd:03 134485                     /usr/lib64/libpthread-2.17.so
7fa224e47000-7fa224e48000 rw-p 00017000 fd:03 134485                     /usr/lib64/libpthread-2.17.so
7fa224e48000-7fa224e4c000 rw-p 00000000 00:00 0
7fa224e4c000-7fa224e90000 r-xp 00000000 fd:03 4044478 /home/centos/pg13_bk_mani/edb/edbpsql/lib/libpq.so.5.13
7fa224e90000-7fa225090000 ---p 00044000 fd:03 4044478 /home/centos/pg13_bk_mani/edb/edbpsql/lib/libpq.so.5.13
7fa225090000-7fa225093000 r--p 00044000 fd:03 4044478 /home/centos/pg13_bk_mani/edb/edbpsql/lib/libpq.so.5.13
7fa225093000-7fa225094000 rw-p 00047000 fd:03 4044478 /home/centos/pg13_bk_mani/edb/edbpsql/lib/libpq.so.5.13
7fa225094000-7fa2250b6000 r-xp 00000000 fd:03 130333                     /usr/lib64/ld-2.17.so
7fa22527d000-7fa2252a2000 rw-p 00000000 00:00 0
7fa2252b3000-7fa2252b5000 rw-p 00000000 00:00 0
7fa2252b5000-7fa2252b6000 r--p 00021000 fd:03 130333                     /usr/lib64/ld-2.17.so
7fa2252b6000-7fa2252b7000 rw-p 00022000 fd:03 130333                     /usr/lib64/ld-2.17.so
7fa2252b7000-7fa2252b8000 rw-p 00000000 00:00 0
7ffdf354f000-7ffdf3570000 rw-p 00000000 00:00 0                          [stack]
7ffdf3572000-7ffdf3574000 r-xp 00000000 00:00 0                          [vdso]
ffffffffff600000-ffffffffff601000 r-xp 00000000 00:00 0                  [vsyscall]
Aborted
[centos@tushar-ldap-docker bin]$


I am getting the error message but along with "*** Error in `./pg_validatebackup': double free or corruption (!prev): 0x0000000001850ba0 ***"  messages

Is this expected ?

regards,

On 3/3/20 8:19 PM, tushar wrote:

On 3/3/20 4:04 PM, tushar wrote:

Thanks Robert.  After applying all the 5 patches (v8-00*) against PG v13 (commit id -afb5465e0cfce7637066eaaaeecab30b0f23fbe3) ,

There is a scenario where pg_validatebackup is not throwing an error if some file deleted from pg_wal/ folder and  but later at the time of restoring - we are getting an error

[centos@tushar-ldap-docker bin]$ ./pg_basebackup  -D test1

[centos@tushar-ldap-docker bin]$ ls test1/pg_wal/
000000010000000000000010  archive_status

[centos@tushar-ldap-docker bin]$ rm -rf test1/pg_wal/*

[centos@tushar-ldap-docker bin]$ ./pg_validatebackup test1
pg_validatebackup: * manifest_checksum = 88f1ed995c83e86252466a2c88b3e660a69cfc76c169991134b101c4f16c9df7
pg_validatebackup: backup successfully verified

[centos@tushar-ldap-docker bin]$ ./pg_ctl -D test1 start -o '-p 3333'
waiting for server to start....2020-03-02 20:05:22.732 IST [21441] LOG:  starting PostgreSQL 13devel on x86_64-pc-linux-gnu, compiled by gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-39), 64-bit
2020-03-02 20:05:22.733 IST [21441] LOG:  listening on IPv6 address "::1", port 3333
2020-03-02 20:05:22.733 IST [21441] LOG:  listening on IPv4 address "127.0.0.1", port 3333
2020-03-02 20:05:22.736 IST [21441] LOG:  listening on Unix socket "/tmp/.s.PGSQL.3333"
2020-03-02 20:05:22.739 IST [21442] LOG:  database system was interrupted; last known up at 2020-03-02 20:04:35 IST
2020-03-02 20:05:22.739 IST [21442] LOG:  creating missing WAL directory "pg_wal/archive_status"
2020-03-02 20:05:22.886 IST [21442] LOG:  invalid checkpoint record
2020-03-02 20:05:22.886 IST [21442] FATAL:  could not locate required checkpoint record
2020-03-02 20:05:22.886 IST [21442] HINT:  If you are restoring from a backup, touch "/home/centos/pg13_bk_mani/edb/edbpsql/bin/test1/recovery.signal" and add required recovery options.
    If you are not restoring from a backup, try removing the file "/home/centos/pg13_bk_mani/edb/edbpsql/bin/test1/backup_label".
    Be careful: removing "/home/centos/pg13_bk_mani/edb/edbpsql/bin/test1/backup_label" will result in a corrupt cluster if restoring from a backup.
2020-03-02 20:05:22.886 IST [21441] LOG:  startup process (PID 21442) exited with exit code 1
2020-03-02 20:05:22.886 IST [21441] LOG:  aborting startup due to startup process failure
2020-03-02 20:05:22.889 IST [21441] LOG:  database system is shut down
 stopped waiting
pg_ctl: could not start server
Examine the log output.
[centos@tushar-ldap-docker bin]$

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

Hi,

In a negative test scenario, if I changed size to -1 in backup_manifest, pg_validatebackup giving

error with a random size number.

[edb@localhost bin]$ ./pg_basebackup -p 5551 -D /tmp/bold --manifest-checksum 'SHA256'

[edb@localhost bin]$ ./pg_validatebackup /tmp/bold
pg_validatebackup: backup successfully verified

--change a file size to -1 and generate new checksum.

[edb@localhost bin]$ vi /tmp/bold/backup_manifest
[edb@localhost bin]$ shasum -a256 /tmp/bold/backup_manifest
c3d7838cbbf991c6108f9c1ab78f673c20d8073114500f14da6ed07ede2dc44a  /tmp/bold/backup_manifest
[edb@localhost bin]$ vi /tmp/bold/backup_manifest

[edb@localhost bin]$ ./pg_validatebackup /tmp/bold
pg_validatebackup: error: "global/4183" has size 0 on disk but size 18446744073709551615 in the manifest

Thanks & Regards,

Rajkumar Raghuwanshi

On Thu, Mar 5, 2020 at 9:37 AM Suraj Kharage <suraj.kharage@enterprisedb.com> wrote:

On Wed, Mar 4, 2020 at 7:21 PM tushar <tushar.ahuja@enterprisedb.com> wrote:

Hi,

There is a scenario in which i add something inside the pg_tablespace directory , i am getting an error like-

pg_validatebackup: * manifest_checksum = 77ddacb4e7e02e2b880792a19a3adf09266dd88553dd15cfd0c22caee7d9cc04
pg_validatebackup: error: "pg_tblspc/16385/PG_13_202002271/test" is present on disk but not in the manifest

but if i remove 'PG_13_202002271 ' directory then there is no error

[centos@tushar-ldap-docker bin]$ ./pg_validatebackup data
pg_validatebackup: * manifest_checksum = 77ddacb4e7e02e2b880792a19a3adf09266dd88553dd15cfd0c22caee7d9cc04
pg_validatebackup: backup successfully verified

This seems expected considering current design as we don't log the directory entries in backup_manifest. In your case, you have tablespace with no objects (empty tablespace) then backup_manifest does not have any entry for this hence when you remove this tablespace directory, validator could not detect it.

We can either document it or add the entry for directories in the manifest. Robert may have a better idea on this.

--

--

Thanks & Regards, 

Suraj kharage, 

EnterpriseDB Corporation, 

The Postgres Database Company.

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

Hi,

There is one scenario  where  i somehow able to run pg_validatebackup successfully but when i tried to start the server , it is failing

Steps to reproduce -

--create 2 base backup directory

[centos@tushar-ldap-docker bin]$ ./pg_basebackup -D db1
[centos@tushar-ldap-docker bin]$ ./pg_basebackup -D db2

--run pg_validatebackup , use backup_manifest of db1 directory against  db2/  . Will get an error

[centos@tushar-ldap-docker bin]$ ./pg_validatebackup -m db1/backup_manifest db2/
pg_validatebackup: * manifest_checksum = 5b131aff4a4f86e2a53efd84b003a67b9f615decb0039f19033eefa6f43c1ede
pg_validatebackup: error: checksum mismatch for file "backup_label"
 

--copy the backup_level of db1 to db2 folder

[centos@tushar-ldap-docker bin]$ cp db1/backup_label db2/.

--run pg_validatebackup .. working fine

[centos@tushar-ldap-docker bin]$ ./pg_validatebackup -m db1/backup_manifest db2/
pg_validatebackup: * manifest_checksum = 5b131aff4a4f86e2a53efd84b003a67b9f615decb0039f19033eefa6f43c1ede
pg_validatebackup: backup successfully verified
[centos@tushar-ldap-docker bin]$

--try to start the server

[centos@tushar-ldap-docker bin]$ ./pg_ctl -D db2 start -o '-p 7777'
waiting for server to start....2020-03-05 15:33:53.471 IST [24049] LOG:  starting PostgreSQL 13devel on x86_64-pc-linux-gnu, compiled by gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-39), 64-bit
2020-03-05 15:33:53.471 IST [24049] LOG:  listening on IPv6 address "::1", port 7777
2020-03-05 15:33:53.471 IST [24049] LOG:  listening on IPv4 address "127.0.0.1", port 7777
2020-03-05 15:33:53.473 IST [24049] LOG:  listening on Unix socket "/tmp/.s.PGSQL.7777"
2020-03-05 15:33:53.476 IST [24050] LOG:  database system was interrupted; last known up at 2020-03-05 15:32:51 IST
2020-03-05 15:33:53.573 IST [24050] LOG:  invalid checkpoint record
2020-03-05 15:33:53.573 IST [24050] FATAL:  could not locate required checkpoint record
2020-03-05 15:33:53.573 IST [24050] HINT:  If you are restoring from a backup, touch "/home/centos/pg13_bk_mani/edb/edbpsql/bin/db2/recovery.signal" and add required recovery options.
    If you are not restoring from a backup, try removing the file "/home/centos/pg13_bk_mani/edb/edbpsql/bin/db2/backup_label".
    Be careful: removing "/home/centos/pg13_bk_mani/edb/edbpsql/bin/db2/backup_label" will result in a corrupt cluster if restoring from a backup.
2020-03-05 15:33:53.574 IST [24049] LOG:  startup process (PID 24050) exited with exit code 1
2020-03-05 15:33:53.574 IST [24049] LOG:  aborting startup due to startup process failure
2020-03-05 15:33:53.575 IST [24049] LOG:  database system is shut down
 stopped waiting
pg_ctl: could not start server
Examine the log output.
[centos@tushar-ldap-docker bin]$

regards,

On 3/5/20 1:09 PM, Rajkumar Raghuwanshi wrote:

Hi,

In a negative test scenario, if I changed size to -1 in backup_manifest, pg_validatebackup giving

error with a random size number.

[edb@localhost bin]$ ./pg_basebackup -p 5551 -D /tmp/bold --manifest-checksum 'SHA256'

[edb@localhost bin]$ ./pg_validatebackup /tmp/bold
pg_validatebackup: backup successfully verified

--change a file size to -1 and generate new checksum.

[edb@localhost bin]$ vi /tmp/bold/backup_manifest
[edb@localhost bin]$ shasum -a256 /tmp/bold/backup_manifest
c3d7838cbbf991c6108f9c1ab78f673c20d8073114500f14da6ed07ede2dc44a  /tmp/bold/backup_manifest
[edb@localhost bin]$ vi /tmp/bold/backup_manifest

[edb@localhost bin]$ ./pg_validatebackup /tmp/bold
pg_validatebackup: error: "global/4183" has size 0 on disk but size 18446744073709551615 in the manifest

Thanks & Regards,

Rajkumar Raghuwanshi

On Thu, Mar 5, 2020 at 9:37 AM Suraj Kharage <suraj.kharage@enterprisedb.com> wrote:

On Wed, Mar 4, 2020 at 7:21 PM tushar <tushar.ahuja@enterprisedb.com> wrote:

Hi,

There is a scenario in which i add something inside the pg_tablespace directory , i am getting an error like-

pg_validatebackup: * manifest_checksum = 77ddacb4e7e02e2b880792a19a3adf09266dd88553dd15cfd0c22caee7d9cc04
pg_validatebackup: error: "pg_tblspc/16385/PG_13_202002271/test" is present on disk but not in the manifest

but if i remove 'PG_13_202002271 ' directory then there is no error

[centos@tushar-ldap-docker bin]$ ./pg_validatebackup data
pg_validatebackup: * manifest_checksum = 77ddacb4e7e02e2b880792a19a3adf09266dd88553dd15cfd0c22caee7d9cc04
pg_validatebackup: backup successfully verified

This seems expected considering current design as we don't log the directory entries in backup_manifest. In your case, you have tablespace with no objects (empty tablespace) then backup_manifest does not have any entry for this hence when you remove this tablespace directory, validator could not detect it.

We can either document it or add the entry for directories in the manifest. Robert may have a better idea on this.

--

--

Thanks & Regards, 

Suraj kharage, 

EnterpriseDB Corporation, 

The Postgres Database Company.

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On Thu, Mar 5, 2020 at 7:05 AM tushar <tushar.ahuja@enterprisedb.com> wrote:
> There is one small observation if we use slash (/) with option -i then not getting the desired result

Here's an updated patch set responding to many of the comments
received thus far. Since there are quite a few emails, let me
consolidate my comments and responses here.

Report: Segmentation fault if -m is used to point to a valid manifest,
but actual backup directory is nonexistent.
Response: Fixed; thanks for the report.

Report: pg_validatebackup doesn't complain about problems within the
pg_wal directory.
Response: That's out of scope. The WAL files are fetched separately
and are therefore not part of the manifest.

Report: Inaccessible file in data directory being validated leads to a
double free.
Response: Fixed; thanks for the report.

Report: Patch 0005 doesn't validate the manifest checksum.
Response: I know. I mentioned that when posting the previous patch
set. Fixed in this version, though.

Report: Removing an empty directory doesn't make backup validation
fail, even though it might cause problems for the server.
Response: That's a little unfortunate, but I'm not sure it's really
worth complicating the patch to deal with it. It's something of a
corner case.

Report: Negative file sizes in the backup manifest are interpreted as
large integers.
Response: That's also a little unfortunate, but I doubt it's worth
adding code to catch it, since any such manifest is corrupt. Also,
it's not like we're ignoring it; the error just isn't ideal.

Report: If I take the backup label from backup #1 and stick it into
otherwise-identical backup #2, validation succeeds but the server
won't start.
Response: That's because we can't validate the pg_wal directory. As
noted above, that's out of scope.

Report: Using --ignore with a slash-terminated pathname doesn't work
as expected.
Response: Fixed, thanks for the report.

Off-List Report: You forgot a PG_BINARY flag.
Response: Fixed. I thought I'd done this before but there were two
places and I'd only fixed one of them.

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

On 3/5/20 10:25 PM, Robert Haas wrote:
> Here's an updated patch set responding to many of the comments
> received thus far.
Thanks Robert. There is a scenario - if user provide port of v11 server 
at the time of  creating 'base backup'  against pg_basebackup(v13+ your 
patch applied)
with option --manifest-checksums,will lead to  this  below error

[centos@tushar-ldap-docker bin]$ ./pg_basebackup -R -p 9045 
--manifest-checksums=SHA224 -D dc1
pg_basebackup: error: could not initiate base backup: ERROR: syntax error
pg_basebackup: removing data directory "dc1"
[centos@tushar-ldap-docker bin]$

Steps to reproduce -
PG v11 is running
run pg_basebackup against that with option --manifest-checksums

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On Mon, Mar 9, 2020 at 12:22 PM tushar <tushar.ahuja@enterprisedb.com> wrote:
> On 3/5/20 10:25 PM, Robert Haas wrote:
> > Here's an updated patch set responding to many of the comments
> > received thus far.
> Thanks Robert. There is a scenario - if user provide port of v11 server
> at the time of  creating 'base backup'  against pg_basebackup(v13+ your
> patch applied)
> with option --manifest-checksums,will lead to  this  below error
>
> [centos@tushar-ldap-docker bin]$ ./pg_basebackup -R -p 9045
> --manifest-checksums=SHA224 -D dc1
> pg_basebackup: error: could not initiate base backup: ERROR: syntax error
> pg_basebackup: removing data directory "dc1"
> [centos@tushar-ldap-docker bin]$
>
> Steps to reproduce -
> PG v11 is running
> run pg_basebackup against that with option --manifest-checksums

Seems like expected behavior to me. We could consider providing a more
descriptive error message, but there's now way for it to work.

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

On Fri, Mar 6, 2020 at 3:58 AM Suraj Kharage
<suraj.kharage@enterprisedb.com> wrote:
> 1: Getting below error while compiling 0002 patch.
> 2:
>
> Few macros defined in 0003 patch not used anywhere in 0005 patch. Either we can replace these with hard-coded values
orremove them.
 

Thanks. I hope that I have straightened those things out in the new
version which is attached. This version also includes some other
changes. The non-JSON code is now completely gone. Also, I've
refactored the code that does parses the JSON manifest to make it
cleaner, and I've moved it out into a separate file. This might be
useful if anyone ends up wanting to reuse that code for some other
purpose, and I think it makes it easier to understand, too, since the
manifest parsing is now much better separated from the task of
actually validating the given directory against the manifest. I've
also added some tests, which are based in part on testing ideas from
Rajkumar Raghuwanshi and Mark Dilger, but this test code was written
by me. So now it's like this:

0001 - checksum helper functions. same as before.
0002 - patch the server to generate and send a manifest, and
pg_basebackup to receive it
0003 - add pg_validatebackup
0004 - TAP tests

Comments?

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

On 3/9/20 10:46 PM, Robert Haas wrote:
> Seems like expected behavior to me. We could consider providing a more
> descriptive error message, but there's now way for it to work.

Right , Error message need to be more user friendly .

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

Seems like expected behavior to me. We could consider providing a more
descriptive error message, but there's now way for it to work.

Right , Error message need to be more user friendly .

One scenario which i feel - should error out  even if  -s option is specified.
create  base  backup directory ( ./pg_basebackup data1)
Connect to root user and take out  the permission from pg_hba.conf file ( chmod 004 pg_hba.conf)

run pg_validatebackup - [centos@tushar-ldap-docker bin]$ ./pg_validatebackup data1
pg_validatebackup: error: could not open file "pg_hba.conf": Permission denied

run pg_validatebackup  with switch -s [centos@tushar-ldap-docker bin]$ ./pg_validatebackup data1 -s
pg_validatebackup: backup successfully verified

here file is not accessible so i think - it should throw you an error ( the same above one) instead of   blindly skipping it. 

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On Fri, Mar 13, 2020 at 9:53 AM tushar <tushar.ahuja@enterprisedb.com> wrote:
> run pg_validatebackup -
>
> [centos@tushar-ldap-docker bin]$ ./pg_validatebackup data1
> pg_validatebackup: error: could not open file "pg_hba.conf": Permission denied
>
> run pg_validatebackup  with switch -s
>
> [centos@tushar-ldap-docker bin]$ ./pg_validatebackup data1 -s
> pg_validatebackup: backup successfully verified
>
> here file is not accessible so i think - it should throw you an error ( the same above one) instead of   blindly
skippingit.
 

I don't really want to do that. That would require it to open every
file even if it doesn't need to read the data in the files. I think in
most cases that would just slow it down for no real benefit. If you've
specified -s, you have to be OK with getting a less complete check for
problems.

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

On Thu, Mar 12, 2020 at 10:47 AM tushar <tushar.ahuja@enterprisedb.com> wrote:
> On 3/9/20 10:46 PM, Robert Haas wrote:
> > Seems like expected behavior to me. We could consider providing a more
> > descriptive error message, but there's now way for it to work.
>
> Right , Error message need to be more user friendly .

OK. Done in the attached version, which also includes a few other changes:

- I expanded the regression tests. They now cover every line of code
in parse_manifest.c except for a few that I believe to be unreachable
(though I might be mistaken). Coverage for pg_validatebackup.c is also
improved, but it's not 100%; there are some cases that I don't know
how to hit outside of a kernel malfunction, and others that I only
know how to hit on non-Windows systems. For instance, it's easy to use
perl to make a file inaccessible on Linux with chmod(0, $filename),
but I gather that doesn't work on Windows. I'm going to spend a bit
more time looking at this, but I think it's already reasonably good.

- I fixed a couple of very minor bugs which I discovered by writing those tests.

- I added documentation, in part based on a draft Mark Dilger shared
with me off-list.

I don't think this is committable just yet, but I think it's getting
fairly close, so if anyone has major objections please speak up soon.

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