Re: backup manifests

Thank you for review comments.

On Fri, Dec 20, 2019 at 9:14 PM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Fri, Dec 20, 2019 at 8:24 AM Suraj Kharage
> <suraj(dot)kharage(at)enterprisedb(dot)com> wrote:
> > Thank you for review comments.
>
> Thanks for the new version.
>
> + <term><option>--verify-backup </option></term>
>
> Whitespace.
>
Corrected.

>
> +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.
>

Removed.

> + 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.
>

Yeah, removed this check

>
> 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.
>

Yes, created new function which will read the file and return the buffer.

>
> + 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().
>

Corrected this check. Below suggestion, allow us to put '\0' in between the
line.
since SHA256 is used to generate for backup manifest, so that we can feed
that
line early to the checksum machinery.

>
> + /*
> + * 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.
>
Made the change in backup manifest as well in backup validatort patch.
Thanks to Rushabh Lathia for the offline discussion and 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 manifest checksum.
so update_checksum() should be called after recognizing the type, i.e: if
it is a File type record. Do you see any issues with this?

+ filesize = atol(size);
>
> Using strtol() would allow for some error checking.
>
corrected.

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

>
> + 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;
>

Thanks for the suggestion. Corrected as per the above inputs.

> + 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.
>

Make sense. corrected.

>
> +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.
>
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?

Rajkaumar has changed the tap test case patch as per revised error
messages.
Please find attached patch stack incorporated the above comments.

--
--

Thanks & Regards,
Suraj kharage,
EnterpriseDB Corporation,
The Postgres Database Company.