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
On Fri, Mar 1, 2024 at 11:28 AM Michael Paquier wrote:
> 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
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
On Mon, Feb 19, 2024 at 4:22 AM Michael Paquier wrote:
> 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
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
On Wed, Jan 24, 2024 at 10:53 PM Robert Haas wrote:
> 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(
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
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_verifybackup before system identifier validation.
Regards,
Amul
From 7ff9e
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
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
> > "System-Identifier" 64-bit integer whose value is derived from
> pg_control while
> > generating it, and the manif
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
35 matches
Mail list logo
