On Thu, Mar 14, 2024 at 11:05 AM Amul Sul wrote:
> Thank you, Robert.
Thanks for the patch!
--
Robert Haas
EDB: http://www.enterprisedb.com
On Thu, Mar 14, 2024 at 12:48 AM Robert Haas wrote:
> On Fri, Mar 8, 2024 at 12:14 AM Amul Sul wrote:
> > Thank you for the improvement.
> >
> > The caller of verify_control_file() has the full path of the control
> file that
> > can pass it and avoid recomputing. With this change, it doesn't re
On Fri, Mar 8, 2024 at 12:14 AM Amul Sul wrote:
> Thank you for the improvement.
>
> The caller of verify_control_file() has the full path of the control file that
> can pass it and avoid recomputing. With this change, it doesn't really need
> verifier_context argument -- only the manifest's syste
On Fri, Mar 8, 2024 at 1:22 AM Robert Haas wrote:
> On Thu, Mar 7, 2024 at 9:16 AM Robert Haas wrote:
> > It could. I just thought this was clearer. I agree that it's a bit
> > long, but I don't think this is worth bikeshedding very much. If at a
> > later time somebody feels strongly that it ne
On Thu, Mar 7, 2024 at 9:16 AM Robert Haas wrote:
> It could. I just thought this was clearer. I agree that it's a bit
> long, but I don't think this is worth bikeshedding very much. If at a
> later time somebody feels strongly that it needs to be changed, so be
> it. Right now, getting on with th
On Wed, Mar 6, 2024 at 11:22 PM Amul Sul wrote:
>> You are not changing silently the internals of get_controlfile(), so
>> no objections here. The name of the new routine could be shorter, but
>> being short of ideas what you are proposing looks fine by me.
>
> Could be get_controlfile_by_path()
On Thu, Mar 7, 2024 at 9:37 AM Michael Paquier wrote:
> On Wed, Mar 06, 2024 at 11:05:36AM -0500, Robert Haas wrote:
> > So with that in mind, here's my proposal. This is an adjustment of
> > Amit's previous refactoring patch. He renamed the existing
> > get_controlfile() to get_dir_controlfile()
On Wed, Mar 06, 2024 at 11:05:36AM -0500, Robert Haas wrote:
> So with that in mind, here's my proposal. This is an adjustment of
> Amit's previous refactoring patch. He renamed the existing
> get_controlfile() to get_dir_controlfile() and made a new
> get_controlfile() that accepted the path to th
On Mon, Mar 4, 2024 at 2:47 PM Robert Haas wrote:
> I don't have an enormously strong opinion on what the right thing to
> do is here either, but I am not convinced that the change proposed by
> Michael is an improvement. After all, that leaves us with the
> situation where we know the path to the
On Mon, Mar 4, 2024 at 12:35 AM Amul Sul wrote:
> Yes, you are correct. Both the current caller of get_controlfile() has
> access to the root directory.
>
> I have dropped the 0002 patch -- I don't have a very strong opinion to
> refactor
> get_controlfile() apart from saying that it might be goo
*pg_waldump_path,
- char *wal_directory, manifest_wal_range *first_wal_range)
+ char *wal_directory)
{
- manifest_wal_range *this_wal_range = first_wal_range;
+ manifest_data *manifest = context->manifest;
+ manifest_wal_range *this_wal_range = manifest->first_wal_range;
wh
On Wed, Feb 21, 2024 at 10:01 AM Michael Paquier
wrote:
> On Mon, Feb 19, 2024 at 12:06:19PM +0530, Amul Sul wrote:
> > On Mon, Feb 19, 2024 at 4:22 AM Michael Paquier
> wrote:
> >> And the new option should be documented at the top of the init()
> >> routine for perldoc.
> >
> > Added in the at
On Mon, Feb 19, 2024 at 12:06:19PM +0530, Amul Sul wrote:
> Agreed, now they will have an error as _could not read file "": Is
> a directory_. But, IIUC, that what usually happens with the dev version, and
> the extension needs to be updated for compatibility with the newer version for
> the same r
On Mon, Feb 19, 2024 at 12:06:19PM +0530, Amul Sul wrote:
> On Mon, Feb 19, 2024 at 4:22 AM Michael Paquier wrote:
>> And the new option should be documented at the top of the init()
>> routine for perldoc.
>
> Added in the attached version.
I've done some wordsmithing on 0001 and it is OK, so I
;, DataDir);
+
+ return get_controlfile(ControlFilePath, crc_ok_p);
+}
+
/*
* update_controlfile()
*
diff --git a/src/include/common/controldata_utils.h b/src/include/common/controldata_utils.h
index 04da70e87b2..56528360663 100644
--- a/src/include/common/controldata_utils.h
+++ b/src/inclu
On Thu, Feb 15, 2024 at 05:41:46PM +0530, Robert Haas wrote:
> On Thu, Feb 15, 2024 at 3:05 PM Amul Sul wrote:
> > Kindly have a look at the attached version.
>
> IMHO, 0001 looks fine, except probably the comment could be phrased a
> bit more nicely.
And the new option should be documented at t
On Thu, Feb 15, 2024 at 3:05 PM Amul Sul wrote:
> Kindly have a look at the attached version.
IMHO, 0001 looks fine, except probably the comment could be phrased a
bit more nicely. That can be left for whoever commits this to
wordsmith. Michael, what are your plans?
0002 seems like a reasonable
On Thu, Feb 15, 2024 at 7:18 AM Michael Paquier wrote:
> On Wed, Feb 14, 2024 at 12:29:07PM +0530, Amul Sul wrote:
> > Ok, I did that way in the attached version, I have passed the control
> file's
> > full path as a second argument to verify_system_identifier() what we
> gets in
> > verify_backu
On Wed, Feb 14, 2024 at 12:29:07PM +0530, Amul Sul wrote:
> Ok, I did that way in the attached version, I have passed the control file's
> full path as a second argument to verify_system_identifier() what we gets in
> verify_backup_file(), but that is not doing any useful things with it,
> since we
On Fri, Feb 2, 2024 at 12:03 AM Robert Haas wrote:
> On Thu, Feb 1, 2024 at 2:18 AM Amul Sul wrote:
> > I intended to minimize the out param of parse_manifest_file(), which
> currently
> > returns manifest_files_hash and manifest_wal_range, and I need two more
> --
> > manifest versions and the
On Thu, Feb 1, 2024 at 2:18 AM Amul Sul wrote:
> I intended to minimize the out param of parse_manifest_file(), which currently
> returns manifest_files_hash and manifest_wal_range, and I need two more --
> manifest versions and the system identifier.
Sure, but you could do context.ht = manifest_
On Thu, Feb 1, 2024 at 3:06 AM Robert Haas wrote:
> On Thu, Jan 25, 2024 at 2:52 AM Amul Sul wrote:
> > Thank you for the review-comments, updated version attached.
>
> I generally agree with 0001. I spent a long time thinking about your
> decision to make verifier_context contain a pointer to m
On Thu, Jan 25, 2024 at 2:52 AM Amul Sul wrote:
> Thank you for the review-comments, updated version attached.
I generally agree with 0001. I spent a long time thinking about your
decision to make verifier_context contain a pointer to manifest_data
instead of, as it does currently, a pointer to m
e *m;
progress_report(false);
- manifest_files_start_iterate(context->ht, &it);
- while ((m = manifest_files_iterate(context->ht, &it)) != NULL)
+ manifest_files_start_iterate(manifest->files, &it);
+ while ((m = manifest_files_iterate(manifest->files, &it)) != NULL)
{
On Mon, Jan 22, 2024 at 2:22 AM Amul Sul wrote:
> Thinking a bit more on this, I realized parse_manifest_file() has many out
> parameters. Instead parse_manifest_file() should simply return manifest data
> like load_backup_manifest(). Attached 0001 patch doing the same, and removed
> parser_conte
On Mon, Jan 22, 2024 at 10:08 AM Amul Sul wrote:
>
>
> On Fri, Jan 19, 2024 at 10:36 PM Amul Sul wrote:
>
>> On Wed, Jan 17, 2024 at 8:40 PM Robert Haas
>> wrote:
>>
>>>
>>>
>> Updated version is attached.
>>
>
> Another updated version attached -- fix missing manifest version check in
> pg_ver
ion.
Regards,
Amul
From 7ff9e85acbf0789d16d29dc316ce2bd9382ac879 Mon Sep 17 00:00:00 2001
From: Amul Sul
Date: Mon, 22 Jan 2024 10:05:20 +0530
Subject: [PATCH v3] Add system identifier to backup manifest
The backup manifest will be having a new item as "System-Iden
On Thu, Jan 18, 2024 at 6:39 AM Sravan Kumar
wrote:
> I have also done a review of the patch and some testing. The patch looks
> good, and I agree with Robert's comments.
>
Thank you for your review, testing and the offline discussion.
Regards,
Amul
-integer conversion and just
> strcmp() twice or (b) use strtol so that you can check that it
> succeeded. I don't want to accept manifest version 1a as 1.
>
Understood, corrected in the attached version.
> +/*
> + * Validate manifest system identifier against the database
On Wed, Jan 17, 2024 at 08:46:09AM -0500, Robert Haas wrote:
> On Wed, Jan 17, 2024 at 6:45 AM Alvaro Herrera
> wrote:
>> Hmm, okay, but what if I take a full backup from a primary server and
>> later I want an incremental from a standby, or the other way around?
>> Will this prevent me from usin
I have also done a review of the patch and some testing. The patch looks
good, and I agree with Robert's comments.
On Wed, Jan 17, 2024 at 8:40 PM Robert Haas wrote:
>
> On Wed, Jan 17, 2024 at 6:31 AM Amul Sul wrote:
> > With the attached patch, the backup manifest will have a new key item as
>
On Wed, Jan 17, 2024 at 6:31 AM Amul Sul wrote:
> With the attached patch, the backup manifest will have a new key item as
> "System-Identifier" 64-bit integer whose value is derived from pg_control
> while
> generating it, and the manifest version bumps to 2.
>
> This helps to identify the corre
On Wed, Jan 17, 2024 at 6:45 AM Alvaro Herrera wrote:
> Hmm, okay, but what if I take a full backup from a primary server and
> later I want an incremental from a standby, or the other way around?
> Will this prevent me from using such a combination?
The system identifier had BETTER match in such
On Wed, Jan 17, 2024 at 5:15 PM Alvaro Herrera
wrote:
> On 2024-Jan-17, Amul Sul wrote:
>
> > This helps to identify the correct database server and/or backup for the
> > subsequent backup operations. pg_verifybackup validates the manifest
> system
> > identifier against the backup control file
On 2024-Jan-17, Amul Sul wrote:
> This helps to identify the correct database server and/or backup for the
> subsequent backup operations. pg_verifybackup validates the manifest system
> identifier against the backup control file and fails if they don’t match.
> Similarly, pg_basebackup increment
EDB: http://www.enterprisedb.com
From 03d21efd7b03d17a55fc1dd1159e0838777f548a Mon Sep 17 00:00:00 2001
From: Amul Sul
Date: Wed, 17 Jan 2024 16:12:19 +0530
Subject: [PATCH v1] Add system identifier to backup manifest
The backup manifest will be having a new item as "System-Identifier"
a
36 matches
Mail list logo
