On Wed, Oct 2, 2024 at 8:30 PM Tom Lane wrote:
> Nope, it was my testing error: I supposed that this patch only
> affected pg_combinebackup and pg_verifybackup, so I only
> recompiled those modules not the whole tree. But there's one
> more place with a json_manifest_per_file_callback callback :-
I wrote:
> Sadly, it seems adder[1] is even pickier than mamba:
Nope, it was my testing error: I supposed that this patch only
affected pg_combinebackup and pg_verifybackup, so I only
recompiled those modules not the whole tree. But there's one
more place with a json_manifest_per_file_callback ca
Robert Haas writes:
> On Tue, Oct 1, 2024 at 1:32 PM Tom Lane wrote:
>> Yes, mamba thinks this is OK.
> Committed.
Sadly, it seems adder[1] is even pickier than mamba:
../pgsql/src/backend/backup/basebackup_incremental.c: In function
\342\200\230CreateIncrementalBackupInfo\342\200\231:
../pgs
On Tue, Oct 1, 2024 at 1:32 PM Tom Lane wrote:
> Yes, mamba thinks this is OK.
Committed.
--
Robert Haas
EDB: http://www.enterprisedb.com
I wrote:
> Robert Haas writes:
>> Here is an attempt at cleaning this up. I'm far from convinced that
>> it's fully correct; my local compiler (clang version 15.0.7) doesn't
>> seem fussed about conflating size_t with uint64, not even with -Wall
>> -Werror. I don't suppose you have a fussier compi
On Mon, Sep 30, 2024 at 6:05 PM Tom Lane wrote:
> Robert Haas writes:
> > On Mon, Sep 30, 2024 at 11:31 AM Tom Lane wrote:
> >> WFM, modulo the suggestion about changing data types.
>
> > I would prefer not to make the data type change here because it has
> > quite a few tentacles.
>
> I see you
On Tue, Oct 1, 2024 at 10:48 AM Tom Lane wrote:
> Sure, I can try it on mamba's host. It's slow though ...
OK, thanks.
> I apologize for rubbing you the wrong way on this. It was not my
> intent. (But, in fact, I had not realized you copied that code
> from somewhere else.)
That's good to he
Robert Haas writes:
> Here is an attempt at cleaning this up. I'm far from convinced that
> it's fully correct; my local compiler (clang version 15.0.7) doesn't
> seem fussed about conflating size_t with uint64, not even with -Wall
> -Werror. I don't suppose you have a fussier compiler locally tha
On Mon, Sep 30, 2024 at 6:01 PM Tom Lane wrote:
> > But then how do you think we should print
> > that? Cast to unsigned long long and use %llu?
>
> Our two standard solutions are to do that or to use UINT64_FORMAT.
> But UINT64_FORMAT is problematic in translatable strings because
> then the .po
Robert Haas writes:
> On Mon, Sep 30, 2024 at 11:31 AM Tom Lane wrote:
>> WFM, modulo the suggestion about changing data types.
> I would prefer not to make the data type change here because it has
> quite a few tentacles.
I see your point for the function's "len" argument, but perhaps it's
wor
Robert Haas writes:
> On Mon, Sep 30, 2024 at 11:24 AM Tom Lane wrote:
>> Um, wait ... we do have strtou64(), so you should use that.
> The thing we should be worried about is not how large a JSON blob
> might be, but rather how large any file that appears in the data
> directory might be. So ui
On Mon, Sep 30, 2024 at 11:31 AM Tom Lane wrote:
> WFM, modulo the suggestion about changing data types.
I would prefer not to make the data type change here because it has
quite a few tentacles. If I change member_copy_control_data() then I
have to change astreamer_verify_content() which means c
On Mon, Sep 30, 2024 at 11:24 AM Tom Lane wrote:
> > This is just a string in a
> > JSON file that represents an integer which will hopefully turn out to
> > be the size of the file on disk. I guess I don't know what type I
> > should be using here. Most things in PostgreSQL use a type like uint32
Robert Haas writes:
> On Sun, Sep 29, 2024 at 1:03 PM Tom Lane wrote:
>>> CID 1620458: Resource leaks (RESOURCE_LEAK)
>>> Variable "buffer" going out of scope leaks the storage it points to.
> This looks like a real leak. It can only happen once per tarfile when
> verifying a tar backup so it
Robert Haas writes:
> On Sat, Sep 28, 2024 at 6:59 PM Tom Lane wrote:
>> Now, manifest_file.size is in fact a size_t, so %zu is the correct
>> format spec for it. But astreamer_verify.checksum_bytes is declared
>> uint64. This leads me to question whether size_t was the correct
>> choice for ma
On Sun, Sep 29, 2024 at 1:03 PM Tom Lane wrote:
> *** CID 1620458: Resource leaks (RESOURCE_LEAK)
> /srv/coverity/git/pgsql-git/postgresql/src/bin/pg_verifybackup/pg_verifybackup.c:
> 1025 in verify_tar_file()
> 1019relpath);
> 1020
>
On Sat, Sep 28, 2024 at 6:59 PM Tom Lane wrote:
> Now, manifest_file.size is in fact a size_t, so %zu is the correct
> format spec for it. But astreamer_verify.checksum_bytes is declared
> uint64. This leads me to question whether size_t was the correct
> choice for manifest_file.size. If it is
Piling on a bit ... Coverity reported the following issues in
this new code. I have not analyzed them to see if they're
real problems.
*** CID 1620458: Resource leaks (RESOURCE_LEAK)
/srv/co
The 32-bit buildfarm animals don't like this too much [1]:
astreamer_verify.c: In function 'member_verify_checksum':
astreamer_verify.c:297:68: error: format '%zu' expects argument of type
'size_t', but argument 6 has type 'uint64' {aka 'long long unsigned int'}
[-Werror=format=]
297 |
On Fri, Sep 27, 2024 at 2:07 AM Amul Sul wrote:
> Thank you, Robert. The code changes look much better now.
Cool.
> A few minor comments:
>
> +each tablespace, named after the tablespace's OID. If the backup
> +is compressed, the relevant compression extension is added to
On Thu, Sep 26, 2024 at 12:18 AM Robert Haas wrote:
>
> On Thu, Sep 12, 2024 at 7:05 AM Amul Sul wrote:
> > The updated version attached. Thank you for the review !
>
> I have spent a bunch of time on this and have made numerous revisions.
> I hope to commit the result, aftering seeing what you a
On Thu, Sep 12, 2024 at 7:05 AM Amul Sul wrote:
> The updated version attached. Thank you for the review !
I have spent a bunch of time on this and have made numerous revisions.
I hope to commit the result, aftering seeing what you and the
buildfarm think (and anyone else who wishes to offer an o
On Tue, Sep 10, 2024 at 1:31 AM Robert Haas wrote:
>
> I would rather that you didn't add simple_ptr_list_destroy_deep()
> given that you don't need it for this patch series.
>
Done.
> +
> "\"%s\" unexpected file in the tar format backup",
>
> This doesn't seem grammatical to me. Perhaps change t
+ pg_log_error("pg_waldump cannot read from a tar");
"tar" isn't normally used as a noun as you do here, so I think this
should say "pg_waldump cannot read tar files".
Technically, the position of this check could lead to an unnecessary
failure, if -n wasn't specified but pg
I would rather that you didn't add simple_ptr_list_destroy_deep()
given that you don't need it for this patch series.
+
"\"%s\" unexpected file in the tar format backup",
This doesn't seem grammatical to me. Perhaps change this to: file
\"%s\" is not expected in a tar format backup
+ /* We
On Sat, Aug 24, 2024 at 2:02 AM Robert Haas wrote:
>
> On Wed, Aug 21, 2024 at 7:08 AM Amul Sul wrote:
> []
> Then the result verifies. But I feel like we should have some test
> cases that do this kind of stuff so that there is automated
> verification. In fact, the current patch seems to ha
On Wed, Aug 21, 2024 at 7:08 AM Amul Sul wrote:
> I have reworked a few comments, revised error messages, and made some
> minor tweaks in the attached version.
Thanks.
> Additionally, I would like to discuss a couple of concerns regarding
> error placement and function names to gather your sugge
On Tue, Aug 20, 2024 at 3:56 PM Amul Sul wrote:
>
> On Sat, Aug 17, 2024 at 1:34 AM Robert Haas wrote:
> >
> > On Fri, Aug 16, 2024 at 3:53 PM Robert Haas wrote:
[...]
> > There's probably more to look at here but I'm running out of energy for
> > today.
> >
>
> Thank you for the review and com
On Sat, Aug 17, 2024 at 1:34 AM Robert Haas wrote:
>
> On Fri, Aug 16, 2024 at 3:53 PM Robert Haas wrote:
> > +int64 num = strtoi64(relpath, &suffix, 10);
>
> Hit send too early. Here, seems like this should be strtoul(), not strtoi64().
>
Fixed in the attached version including others
On Fri, Aug 16, 2024 at 3:53 PM Robert Haas wrote:
> +int64 num = strtoi64(relpath, &suffix, 10);
Hit send too early. Here, seems like this should be strtoul(), not strtoi64().
The documentation of --format seems to be cut-and-pasted from
pg_basebackup and the language isn't really appro
On Wed, Aug 14, 2024 at 12:44 PM Robert Haas wrote:
> On Wed, Aug 14, 2024 at 9:20 AM Amul Sul wrote:
> > I agree with keeping verify_backup_file() separate, but I'm hesitant
> > about doing the same for verify_backup_directory().
>
> I don't have time today to go through your whole email or re-r
On Wed, Aug 14, 2024 at 9:20 AM Amul Sul wrote:
> I agree with keeping verify_backup_file() separate, but I'm hesitant
> about doing the same for verify_backup_directory().
I don't have time today to go through your whole email or re-review
the code, but I plan to circle back to that at a later t
On Tue, Aug 13, 2024 at 10:49 PM Robert Haas wrote:
>
> On Mon, Aug 12, 2024 at 5:13 AM Amul Sul wrote:
> > I tried this in the attached version and made a few additional changes
> > based on Sravan's off-list comments regarding function names and
> > descriptions.
> >
> > Now, verification happe
On Mon, Aug 12, 2024 at 5:13 AM Amul Sul wrote:
> I tried this in the attached version and made a few additional changes
> based on Sravan's off-list comments regarding function names and
> descriptions.
>
> Now, verification happens in two passes. The first pass simply
> verifies the file names,
On Wed, Aug 7, 2024 at 11:28 PM Robert Haas wrote:
>
> On Wed, Aug 7, 2024 at 1:05 PM Amul Sul wrote:
> > The main issue I have is computing the total_size of valid files that
> > will be checksummed and that exist in both the manifests and the
> > backup, in the case of a tar backup. This cannot
On Wed, Aug 7, 2024 at 1:05 PM Amul Sul wrote:
> The main issue I have is computing the total_size of valid files that
> will be checksummed and that exist in both the manifests and the
> backup, in the case of a tar backup. This cannot be done in the same
> way as with a plain backup.
I think yo
On Wed, Aug 7, 2024 at 9:12 PM Robert Haas wrote:
>
> [ I committed 0001, then noticed I had a type in the subject line of
> the commit message. Argh. ]
>
> On Wed, Aug 7, 2024 at 9:41 AM Amul Sul wrote:
> > With the patch, I am concerned that we won't be able to give an
> > accurate progress rep
[ I committed 0001, then noticed I had a type in the subject line of
the commit message. Argh. ]
On Wed, Aug 7, 2024 at 9:41 AM Amul Sul wrote:
> With the patch, I am concerned that we won't be able to give an
> accurate progress report as before. We add all the file sizes in the
> backup manifes
On Tue, Aug 6, 2024 at 10:39 PM Robert Haas wrote:
>
> On Thu, Aug 1, 2024 at 9:19 AM Amul Sul wrote:
> > > I think I would have made this pass context->show_progress to
> > > progress_report() instead of the whole verifier_context, but that's an
> > > arguable stylistic choice, so I'll defer to
On Thu, Aug 1, 2024 at 9:19 AM Amul Sul wrote:
> > I think I would have made this pass context->show_progress to
> > progress_report() instead of the whole verifier_context, but that's an
> > arguable stylistic choice, so I'll defer to you if you prefer it the
> > way you have it. Other than that,
On Mon, Aug 5, 2024 at 10:29 PM Robert Haas wrote:
>
> On Fri, Aug 2, 2024 at 7:43 AM Amul Sul wrote:
> > Please consider the attached version for the review.
>
> Thanks. I committed 0001-0003. The only thing that I changed was that
> in 0001, you forgot to pgindent, which actually mattered quite
On Fri, Aug 2, 2024 at 7:43 AM Amul Sul wrote:
> Please consider the attached version for the review.
Thanks. I committed 0001-0003. The only thing that I changed was that
in 0001, you forgot to pgindent, which actually mattered quite a bit,
because astreamer is one character shorter than bbstrea
Hi,
On 2024-07-31 16:07:03 -0400, Robert Haas wrote:
> On Wed, Jul 31, 2024 at 9:28 AM Amul Sul wrote:
> > Fixed -- I did that because it was part of a separate group in
> > pg_basebackup.
>
> Well, that's because pg_basebackup builds multiple executables, and
> these files needed to be linked
On Wed, Jul 31, 2024 at 9:28 AM Amul Sul wrote:
> Fixed -- I did that because it was part of a separate group in pg_basebackup.
Well, that's because pg_basebackup builds multiple executables, and
these files needed to be linked with some but not others. It looks
like when Andres added meson suppo
On Mon, Jul 22, 2024 at 7:53 AM Amul Sul wrote:
> Fix in the attached version.
First of all, in the interest of full disclosure, I suggested this
project to Amul, so I'm +1 on the concept. I think making more of our
backup-related tools work with tar and compressed tar formats -- and
perhaps even
Hi Amul,
thanks for working on this.
+ file_name_len = strlen(relpath);
> + if (file_name_len < file_extn_len ||
> + strcmp(relpath + file_name_len - file_extn_len, file_extn) != 0)
> + {
> + if (compress_algorithm == PG_COMPRESSION_NONE)
> + report_backup_error(context,
> + "\"%s\" is not a
46 matches
Mail list logo
