Dear Thomas, Alexander,
> Thanks for reporting. Yes, it has been already reported by me [1], and the
> server
> log was provided by Andrew [2]. The issue was that a file creation was failed
> because the same one was unlink()'d just before but it was in
> STATUS_DELETE_PENDING
> status. Kindly
Dear Thomas, Alexander,
> 17.12.2023 07:02, Thomas Munro wrote:
> > FYI fairywren failed in this test:
> >
> >
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren=2023-1
> 2-16%2022%3A03%3A06
> >
> > ===8<===
> > Restoring database schemas in the new cluster
> > *failure*
> >
> >
17.12.2023 07:02, Thomas Munro wrote:
FYI fairywren failed in this test:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren=2023-12-16%2022%3A03%3A06
===8<===
Restoring database schemas in the new cluster
*failure*
Consult the last few lines of
FYI fairywren failed in this test:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fairywren=2023-12-16%2022%3A03%3A06
===8<===
Restoring database schemas in the new cluster
*failure*
Consult the last few lines of
On Wed, Dec 6, 2023 at 10:02 AM Amit Kapila wrote:
>
> On Wed, Dec 6, 2023 at 9:40 AM vignesh C wrote:
> >
> > On Tue, 5 Dec 2023 at 11:11, Hayato Kuroda (Fujitsu)
> > wrote:
> > >
> > > Dear Sawada-san, hackers,
> > >
> > > Based on comments I made a fix. PSA the patch.
> > >
> >
> > Thanks
On Wed, Dec 6, 2023 at 9:40 AM vignesh C wrote:
>
> On Tue, 5 Dec 2023 at 11:11, Hayato Kuroda (Fujitsu)
> wrote:
> >
> > Dear Sawada-san, hackers,
> >
> > Based on comments I made a fix. PSA the patch.
> >
>
> Thanks for the patch, the changes look good to me.
>
Thanks, I have added a comment
On Tue, 5 Dec 2023 at 11:11, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Sawada-san, hackers,
>
> Based on comments I made a fix. PSA the patch.
>
Thanks for the patch, the changes look good to me.
Regards,
Vignesh
Dear Sawada-san, hackers,
Based on comments I made a fix. PSA the patch.
>
> Is there any reason why this function can be executed only in binary
> upgrade mode? It seems to me that other functions in
> pg_upgrade_support.c must be called only in binary upgrade mode
> because it does some hacky
On Mon, Dec 4, 2023 at 11:59 AM Hayato Kuroda (Fujitsu)
wrote:
>
> Dear hackers,
>
> I found another failure related with the commit [1]. This is caused by missing
> wait on the test code. Amit helped me for this analysis and fix.
>
Pushed!
--
With Regards,
Amit Kapila.
Dear hackers,
I found another failure related with the commit [1]. This is caused by missing
wait on the test code. Amit helped me for this analysis and fix.
# Analysis of the failure
The failure is that restored slot is two_phase = false, whereas the slot is
created as two_phase = true. This
On Thu, Nov 30, 2023 at 6:49 PM Amit Kapila wrote:
>
> On Wed, Nov 29, 2023 at 7:33 AM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > > > Actually, we do not expect that it won't input NULL. IIUC all of slots
> > > > have
> > > > slot_name, and subquery uses its name. But will it be kept forever? I
On Wed, Nov 29, 2023 at 7:33 AM Hayato Kuroda (Fujitsu)
wrote:
>
> > > Actually, we do not expect that it won't input NULL. IIUC all of slots
> > > have
> > > slot_name, and subquery uses its name. But will it be kept forever? I
> > > think we
> > > can avoid any risk.
> > >
> > > > I've not
Dear Amit,
> Sorry, my analysis was not complete. On looking closely, I think the
> reason is that we are allowed to upgrade the slot iff there is no
> pending WAL to be processed.
Yes, the guard will strongly protect from data loss, but I do not take care in
the test.
> The test first
On Thu, Nov 30, 2023 at 8:40 AM Amit Kapila wrote:
>
> On Wed, Nov 29, 2023 at 2:56 PM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > > > >
> > > > > Pushed!
> > > >
> > > > Hi all, the CF entry for this is marked RfC, and CI is trying to apply
> > > > the last patch committed. Is there further work
On Wed, Nov 29, 2023 at 2:56 PM Hayato Kuroda (Fujitsu)
wrote:
>
> > > >
> > > > Pushed!
> > >
> > > Hi all, the CF entry for this is marked RfC, and CI is trying to apply
> > > the last patch committed. Is there further work that needs to be
> > > re-attached and/or rebased?
> > >
> >
> > No. I
Dear hackers,
> > >
> > > Pushed!
> >
> > Hi all, the CF entry for this is marked RfC, and CI is trying to apply
> > the last patch committed. Is there further work that needs to be
> > re-attached and/or rebased?
> >
>
> No. I have marked it as committed.
>
I found another failure related with
Dear Sawada-san,
> > Actually, we do not expect that it won't input NULL. IIUC all of slots have
> > slot_name, and subquery uses its name. But will it be kept forever? I think
> > we
> > can avoid any risk.
> >
> > > I've not tested it yet but even if it returns NULL, perhaps
> > >
On Tue, Nov 28, 2023 at 10:58 PM Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Sawada-san,
>
> > On Tue, Nov 28, 2023 at 7:04 PM Hayato Kuroda (Fujitsu)
> > wrote:
> > >
> > >
> > > Yeah, we followed binary_upgrade_create_empty_extension(). Also, we set as
> > > un-strict to keep a caller function
Dear Sawada-san,
> On Tue, Nov 28, 2023 at 7:04 PM Hayato Kuroda (Fujitsu)
> wrote:
> >
> >
> > Yeah, we followed binary_upgrade_create_empty_extension(). Also, we set as
> > un-strict to keep a caller function simpler.
> >
> > Currently get_old_cluster_logical_slot_infos() executes a query and
On Tue, Nov 28, 2023 at 6:50 PM Amit Kapila wrote:
>
> On Tue, Nov 28, 2023 at 1:32 PM Bharath Rupireddy
> wrote:
> >
> > On Tue, Nov 28, 2023 at 11:06 AM Masahiko Sawada
> > wrote:
> > >
> > > One month has already been passed since this main patch got committed
> > > but reading this change,
Dear Bharath, Sawada-san,
Welcome back!
> >
> > ---
> > { oid => '8046', descr => 'for use by pg_upgrade',
> > proname => 'binary_upgrade_logical_slot_has_caught_up', proisstrict =>
> > 'f',
> > provolatile => 'v', proparallel => 'u', prorettype => 'bool',
> > proargtypes => 'name',
> >
On Tue, Nov 28, 2023 at 1:32 PM Bharath Rupireddy
wrote:
>
> On Tue, Nov 28, 2023 at 11:06 AM Masahiko Sawada
> wrote:
> >
> > One month has already been passed since this main patch got committed
> > but reading this change, I have some questions on new
> >
On Tue, Nov 28, 2023 at 11:06 AM Masahiko Sawada wrote:
>
> One month has already been passed since this main patch got committed
> but reading this change, I have some questions on new
> binary_upgrade_logical_slot_has_caught_up() function:
>
> Is there any reason why this function can be
On Thu, Nov 9, 2023 at 7:07 PM Amit Kapila wrote:
>
> On Wed, Nov 8, 2023 at 11:05 PM vignesh C wrote:
> >
> > On Wed, 8 Nov 2023 at 08:43, vignesh C wrote:
> >
> > Here is a small improvisation where num_slots need not be initialized
> > as it will be used only after assigning the result now.
On Wed, Nov 22, 2023 at 1:30 PM John Naylor wrote:
>
> On Thu, Nov 9, 2023 at 5:07 PM Amit Kapila wrote:
> >
> > On Wed, Nov 8, 2023 at 11:05 PM vignesh C wrote:
> > >
> > > On Wed, 8 Nov 2023 at 08:43, vignesh C wrote:
> > >
> > > Here is a small improvisation where num_slots need not be
On Thu, Nov 9, 2023 at 5:07 PM Amit Kapila wrote:
>
> On Wed, Nov 8, 2023 at 11:05 PM vignesh C wrote:
> >
> > On Wed, 8 Nov 2023 at 08:43, vignesh C wrote:
> >
> > Here is a small improvisation where num_slots need not be initialized
> > as it will be used only after assigning the result now.
On Wed, Nov 8, 2023 at 11:05 PM vignesh C wrote:
>
> On Wed, 8 Nov 2023 at 08:43, vignesh C wrote:
>
> Here is a small improvisation where num_slots need not be initialized
> as it will be used only after assigning the result now. The attached
> patch has the changes for the same.
>
Pushed!
--
On Wed, 8 Nov 2023 at 08:43, vignesh C wrote:
>
> On Tue, 7 Nov 2023 at 13:25, Amit Kapila wrote:
> >
> > On Tue, Nov 7, 2023 at 10:01 AM Zhijie Hou (Fujitsu)
> > wrote:
> > >
> > > On Tuesday, November 7, 2023 12:14 PM Kuroda, Hayato/黒田 隼人
> > > wrote:
> > > >
> > > > Dear hackers,
> > > >
>
On Wed, Nov 8, 2023 at 8:44 AM vignesh C wrote:
>
> While verifying upgrade of subscriber patch, I found one issue with
> upgrade in verbose mode.
> I was able to reproduce this issue by performing a upgrade with a
> verbose option.
>
> The trace for the same is given below:
> Program received
On Wed, Nov 8, 2023 at 8:44 AM vignesh C wrote:
>
> While verifying upgrade of subscriber patch, I found one issue with
> upgrade in verbose mode.
> I was able to reproduce this issue by performing a upgrade with a
> verbose option.
>
> The trace for the same is given below:
> Program received
On Tue, 7 Nov 2023 at 13:25, Amit Kapila wrote:
>
> On Tue, Nov 7, 2023 at 10:01 AM Zhijie Hou (Fujitsu)
> wrote:
> >
> > On Tuesday, November 7, 2023 12:14 PM Kuroda, Hayato/黒田 隼人
> > wrote:
> > >
> > > Dear hackers,
> > >
> > > PSA the patch to solve the issue [1].
> > >
> > > Kindly Peter
On Tue, Nov 7, 2023 at 10:01 AM Zhijie Hou (Fujitsu)
wrote:
>
> On Tuesday, November 7, 2023 12:14 PM Kuroda, Hayato/黒田 隼人
> wrote:
> >
> > Dear hackers,
> >
> > PSA the patch to solve the issue [1].
> >
> > Kindly Peter E. and Andrew raised an issue that delete_old_cluster.sh is
> > generated
On Tuesday, November 7, 2023 12:14 PM Kuroda, Hayato/黒田 隼人
wrote:
>
> Dear hackers,
>
> PSA the patch to solve the issue [1].
>
> Kindly Peter E. and Andrew raised an issue that delete_old_cluster.sh is
> generated in the source directory, even when the VPATH/meson build.
> This can avoid by
On Tue, Nov 7, 2023 at 3:14 PM Hayato Kuroda (Fujitsu)
wrote:
>
> Dear hackers,
>
> PSA the patch to solve the issue [1].
>
> Kindly Peter E. and Andrew raised an issue that delete_old_cluster.sh is
> generated in the source directory, even when the VPATH/meson build.
> This can avoid by changing
Dear hackers,
PSA the patch to solve the issue [1].
Kindly Peter E. and Andrew raised an issue that delete_old_cluster.sh is
generated in the source directory, even when the VPATH/meson build.
This can avoid by changing the directory explicitly.
[1]:
Dear Amit,
I found several machines on BF got angry (e.g. [1]), because of missing update
meson.build. Sorry for that.
PSA the patch to fix it.
[1]:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rorqual=2023-10-27%2006%3A08%3A31
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
On Fri, Oct 27, 2023 at 11:24 AM Bharath Rupireddy
wrote:
>
> On Fri, Oct 27, 2023 at 10:10 AM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > Here is a patch for fixing to 003_logical_slots. Also, I got a comment off
> > list so that it was included.
> >
> > ```
> > -# Setup a pg_upgrade command.
On Fri, Oct 27, 2023 at 10:10 AM Hayato Kuroda (Fujitsu)
wrote:
>
> Here is a patch for fixing to 003_logical_slots. Also, I got a comment off
> list so that it was included.
>
> ```
> -# Setup a pg_upgrade command. This will be used anywhere.
> +# Setup a common pg_upgrade command to be used by
On Fri, Oct 27, 2023 at 11:09 AM Amit Kapila wrote:
>
> On Fri, Oct 27, 2023 at 10:43 AM Michael Paquier wrote:
> >
> > -"invalid_logical_replication_slots.txt");
> > +"invalid_logical_slots.txt");
> >
> > Or you could do something even shorter,
Dear Michael,
> Or you could do something even shorter, with "invalid_slots.txt".
I think current one seems better, because we only support logical replication
slots for now. We can extend as you said when we support physical slot as well.
Also, proposed length is sufficient for fairywren [1].
On Fri, Oct 27, 2023 at 10:43 AM Michael Paquier wrote:
>
> On Fri, Oct 27, 2023 at 04:40:43AM +, Hayato Kuroda (Fujitsu) wrote:
> > Yeah, Bharath has already reported, I agreed that the reason was [1].
> >
> > ```
> > In the Windows API (with some exceptions discussed in the following
> >
On Fri, Oct 27, 2023 at 04:40:43AM +, Hayato Kuroda (Fujitsu) wrote:
> Yeah, Bharath has already reported, I agreed that the reason was [1].
>
> ```
> In the Windows API (with some exceptions discussed in the following
> paragraphs),
> the maximum length for a path is MAX_PATH, which is
Dear Bharath, Amit, Peter,
Thank you for discussing! A patch can be available in [1].
> > > > +1 for
> s/003_upgrade_logical_replication_slots.pl/003_upgrade_logical_slots.pl
> > > > and s/invalid_logical_replication_slots.txt/invalid_logical_slots.txt.
> >
> > +1. The proposed file name sounds
Dear Hou,
> The BF animal fairywren[1] failed when testing
> 003_upgrade_logical_replication_slots.pl.
Good catch!
>
> The reason could be the length of this path(262) exceed the windows path
> limit(260 IIRC). If so, I recall we fixed similar things before (e213de8e7) by
> reducing the path
On Fri, Oct 27, 2023 at 8:06 AM Amit Kapila wrote:
>
> > > +1 for
> > > s/003_upgrade_logical_replication_slots.pl/003_upgrade_logical_slots.pl
> > > and s/invalid_logical_replication_slots.txt/invalid_logical_slots.txt.
>
> +1. The proposed file name sounds reasonable.
>
> Agreed. So, how about
On Fri, Oct 27, 2023 at 3:28 AM Peter Smith wrote:
>
> On Fri, Oct 27, 2023 at 2:26 AM Bharath Rupireddy
> wrote:
> >
> > On Thu, Oct 26, 2023 at 8:11 PM Zhijie Hou (Fujitsu)
> > wrote:
> > >
> > > The BF animal fairywren[1] failed when testing
> > > 003_upgrade_logical_replication_slots.pl.
>
On Fri, Oct 27, 2023 at 2:26 AM Bharath Rupireddy
wrote:
>
> On Thu, Oct 26, 2023 at 8:11 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> > The BF animal fairywren[1] failed when testing
> > 003_upgrade_logical_replication_slots.pl.
> >
> > From the log, I can see pg_upgrade failed to open the
> >
On Thu, Oct 26, 2023 at 8:11 PM Zhijie Hou (Fujitsu)
wrote:
>
> The BF animal fairywren[1] failed when testing
> 003_upgrade_logical_replication_slots.pl.
>
> From the log, I can see pg_upgrade failed to open the
> invalid_logical_replication_slots.txt:
>
> # Checking for valid logical
Hi,
The BF animal fairywren[1] failed when testing
003_upgrade_logical_replication_slots.pl.
From the log, I can see pg_upgrade failed to open the
invalid_logical_replication_slots.txt:
# Checking for valid logical replication slots
# could not open file
On Wed, Oct 25, 2023 at 1:50 PM Amit Kapila wrote:
>
> It would be better to gauge its value separately and add it once the
> main patch is committed.
> There should be a way to avoid this but we can decide it afterwards. I
> don't want to hold the main patch for this point. What do you think?
On Wed, Oct 25, 2023 at 1:39 PM Bharath Rupireddy
wrote:
>
> On Wed, Oct 25, 2023 at 11:39 AM Amit Kapila wrote:
> >
> > On Tue, Oct 24, 2023 at 1:20 PM Bharath Rupireddy
> > wrote:
> > >
> > >
> > > I spent some time on the v57 patch and it looks good to me - tests are
> > > passing, no
On Wed, Oct 25, 2023 at 11:39 AM Amit Kapila wrote:
>
> On Tue, Oct 24, 2023 at 1:20 PM Bharath Rupireddy
> wrote:
> >
> >
> > I spent some time on the v57 patch and it looks good to me - tests are
> > passing, no complaints from pgindent and pgperltidy. I turned the CF
> > entry
Dear Amit,
Based on your advice, I revised the patch again.
> >
> > I spent some time on the v57 patch and it looks good to me - tests are
> > passing, no complaints from pgindent and pgperltidy. I turned the CF
> > entry https://commitfest.postgresql.org/45/4273/ to RfC.
> >
>
> Thanks, the
On Tue, Oct 24, 2023 at 1:20 PM Bharath Rupireddy
wrote:
>
>
> I spent some time on the v57 patch and it looks good to me - tests are
> passing, no complaints from pgindent and pgperltidy. I turned the CF
> entry https://commitfest.postgresql.org/45/4273/ to RfC.
>
Thanks, the patch looks mostly
On Tue, Oct 24, 2023 at 11:32 AM Hayato Kuroda (Fujitsu)
wrote:
>
> > If we don't want to keep it generic then we should use something like
> > 'contains_decodable_change'. 'is_change_decodable' could have suited
> > here if we were checking a particular change.
>
> I kept the name for now. How
Dear Bharath, Amit,
Thanks for reviewing! PSA new version.
I addressed comments which have not been claimed.
> On Mon, Oct 23, 2023 at 2:00 PM Bharath Rupireddy
> wrote:
> >
> > On Mon, Oct 23, 2023 at 11:10 AM Hayato Kuroda (Fujitsu)
> > wrote:
> > >
> > > Thank you for reviewing! PSA new
On Sat, Oct 21, 2023 at 5:41 AM Bharath Rupireddy
wrote:
>
> On Fri, Oct 20, 2023 at 8:51 PM Zhijie Hou (Fujitsu)
> wrote:
>
> 9. IMO, binary_upgrade_logical_replication_slot_has_caught_up seems
> better, meaningful and consistent despite a bit long than just
>
On Mon, Oct 23, 2023 at 2:00 PM Bharath Rupireddy
wrote:
>
> On Mon, Oct 23, 2023 at 11:10 AM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > Thank you for reviewing! PSA new version.
>
> > > 6. A nit: how about is_decodable_txn or is_decodable_change or some
> > > other instead of just a plain name
On Mon, Oct 23, 2023 at 11:10 AM Hayato Kuroda (Fujitsu)
wrote:
>
> Thank you for reviewing! PSA new version.
> > 6. A nit: how about is_decodable_txn or is_decodable_change or some
> > other instead of just a plain name processing_required?
> > +/* Do we need to process any change in
Dear Bharath,
Thank you for reviewing! PSA new version.
> 1. A nit:
> +
> +/*
> + * We also skip decoding in 'fast_forward' mode. In passing set the
> + * 'processing_required' flag to indicate, were it not for this mode,
> + * processing *would* have been required.
> + */
>
On Fri, Oct 20, 2023 at 8:51 PM Zhijie Hou (Fujitsu)
wrote:
>
> Attach the new version patch.
Thanks. Here are some comments on v55 patch:
1. A nit:
+
+/*
+ * We also skip decoding in 'fast_forward' mode. In passing set the
+ * 'processing_required' flag to indicate, were it not for
On Friday, October 20, 2023 11:24 AM vignesh C wrote:
>
> On Thu, 19 Oct 2023 at 16:16, Hayato Kuroda (Fujitsu)
> wrote:
> >
> > Dear Vignesh,
> >
> > Thanks for revieing! New patch can be available in [1].
> >
> > > Few comments:
> > > 1) Even if we comment 3rd point "Emit a non-transactional
On Friday, October 20, 2023 9:50 AM Peter Smith wrote:
>
> Here are some review comments for v54-0001
Thanks for the review.
>
> ==
> src/backend/replication/slot.c
>
> 1.
> + if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade) {
> + ereport(ERROR,
On Thu, 19 Oct 2023 at 16:16, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Vignesh,
>
> Thanks for revieing! New patch can be available in [1].
>
> > Few comments:
> > 1) Even if we comment 3rd point "Emit a non-transactional message",
> > test_slot2 still appears in the
On Thu, 19 Oct 2023 at 16:14, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Vignesh,
>
> Thanks for reviewing! New patch can be available in [1].
>
> >
> > Few comments:
> > 1) We will be able to override the value of max_slot_wal_keep_size by
> > using --new-options like '--new-options "-c
> >
Here are some review comments for v54-0001
==
src/backend/replication/slot.c
1.
+ if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade)
+ {
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("replication slots must not be invalidated during the upgrade"),
+
Dear hackers,
> Thanks for reviewing! PSA new version.
Hmm. The cfbot got angry, whereas it can pass on my machine.
It seems that the ordering in invalid_logical_replication_slots.txt is not
fixed.
A change for checking the content was reverted. It could pass on my CI.
Best Regards,
Hayato
Dear Vignesh,
Thanks for revieing! New patch can be available in [1].
> Few comments:
> 1) Even if we comment 3rd point "Emit a non-transactional message",
> test_slot2 still appears in the invalid_logical_replication_slots.txt
> file. There is something wrong here.
> + # 2. Advance the
Dear Shlok,
>
> I have tested the above scenario. We are able to override the
> max_slot_wal_keep_size by using '--new-options "-c
> max_slot_wal_keep_size=val"'. And also with some insert statements
> during pg_upgrade, old WAL file were deleted and logical replication
> slots were
Dear Hou,
Thanks for reviewing! New patch can be available in [1].
> Thanks for updating the patch, here are few comments for the test.
>
> 1.
>
> >
> # The TAP Cluster.pm assigns default 'max_wal_senders' and 'max_connections'
> to
> # the same value (10) but PG12 and prior considered
Dear Vignesh,
Thanks for reviewing! New patch can be available in [1].
>
> Few comments:
> 1) We will be able to override the value of max_slot_wal_keep_size by
> using --new-options like '--new-options "-c
> max_slot_wal_keep_size=val"':
> + /*
> +* Use max_slot_wal_keep_size as
Dear Peter,
Thanks for reviewing! PSA new version.
> ==
> src/bin/pg_upgrade/t/003_upgrade_logical_replication_slots.pl
>
> 1.
> + # 2. max_replication_slots is set to smaller than the number of slots (2)
> + # present on the old cluster
>
> SUGGESTION
> 2. Set 'max_replication_slots' to
Dear Shlok,
Thanks for testing the feature!
>
> I tested a test scenario:
> I started a new publisher with 'max_replication_slots' parameter set
> to '1' and created a streaming replication with the new publisher as
> primary node.
Just to confirm what you did - you set up a physical
I tested a test scenario:
I started a new publisher with 'max_replication_slots' parameter set
to '1' and created a streaming replication with the new publisher as
primary node.
Then I did a pg_upgrade from old publisher to new publisher. The
upgrade failed with following error:
Restoring logical
On Wed, 18 Oct 2023 at 14:55, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Peter,
>
> Thank you for reviewing! PSA new version.
> Note that 0001 and 0002 are combined into one patch.
>
> > Here are some review comments for v51-0001
> >
> > ==
> > src/bin/pg_upgrade/check.c
> >
> > 0.
> >
> Few comments:
> 1) We will be able to override the value of max_slot_wal_keep_size by
> using --new-options like '--new-options "-c
> max_slot_wal_keep_size=val"':
> + /*
> +* Use max_slot_wal_keep_size as -1 to prevent the WAL removal by the
> +* checkpointer process. If
On Wednesday, October 18, 2023 5:26 PM Kuroda, Hayato/黒田 隼人
wrote:
>
> Thank you for reviewing! PSA new version.
> Note that 0001 and 0002 are combined into one patch.
Thanks for updating the patch, here are few comments for the test.
1.
>
# The TAP Cluster.pm assigns default
On Wed, 18 Oct 2023 at 14:55, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Peter,
>
> Thank you for reviewing! PSA new version.
> Note that 0001 and 0002 are combined into one patch.
>
> > Here are some review comments for v51-0001
> >
> > ==
> > src/bin/pg_upgrade/check.c
> >
> > 0.
> >
Here are some review comments for v52-0001
==
src/bin/pg_upgrade/t/003_upgrade_logical_replication_slots.pl
1.
+ # 2. max_replication_slots is set to smaller than the number of slots (2)
+ # present on the old cluster
SUGGESTION
2. Set 'max_replication_slots' to be less than the number of
Dear Peter,
Thank you for reviewing! New patch is available in [1].
> ==
> src/bin/pg_upgrade/t/003_upgrade_logical_replication_slots.pl
>
> 1.
> +# Set max_wal_senders to a lower value if the old cluster is prior to PG12.
> +# Such clusters regard max_wal_senders as part of
Dear Peter,
Thank you for reviewing! PSA new version.
Note that 0001 and 0002 are combined into one patch.
> Here are some review comments for v51-0001
>
> ==
> src/bin/pg_upgrade/check.c
>
> 0.
> +check_old_cluster_for_valid_slots(bool live_check)
> +{
> + char output_path[MAXPGPATH];
> +
Here are some comments for the patch v51-0002
==
src/bin/pg_upgrade/t/003_upgrade_logical_replication_slots.pl
1.
+# Set max_wal_senders to a lower value if the old cluster is prior to PG12.
+# Such clusters regard max_wal_senders as part of max_connections, but the
+# current TAP tester
On Wed, Oct 18, 2023 at 7:31 AM Peter Smith wrote:
>
> ==
> src/bin/pg_upgrade/check.c
>
> 0.
> +check_old_cluster_for_valid_slots(bool live_check)
> +{
> + char output_path[MAXPGPATH];
> + FILE*script = NULL;
> +
> + prep_status("Checking for valid logical replication slots");
> +
> +
Here are some review comments for v51-0001
==
src/bin/pg_upgrade/check.c
0.
+check_old_cluster_for_valid_slots(bool live_check)
+{
+ char output_path[MAXPGPATH];
+ FILE*script = NULL;
+
+ prep_status("Checking for valid logical replication slots");
+
+ snprintf(output_path,
Dear Vignesh,
Thank you for reviewing! New version can be available in [1].
> 1) Should this:
> +# Copyright (c) 2023, PostgreSQL Global Development Group
> +
> +# Tests for upgrading replication slots
> +
> be:
> "Tests for upgrading logical replication slots"
Fixed.
> 2) This statement is
Dear Amit,
Thanks for reviewing! PSA new version.
>
> Yeah, I think introducing additional complexity unless it is really
> required sounds a bit scary to me as well. BTW, please find attached
> some cosmetic changes.
Basically LGTM, but below part was conflicted with Bharath's comment [1].
On Mon, 16 Oct 2023 at 14:44, Amit Kapila wrote:
>
> On Sat, Oct 14, 2023 at 10:45 AM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > Here is a new patch.
> >
> > Previously I wrote:
> > > Based on above idea, I made new version patch which some functionalities
> > > were
> > > exported from
On Sat, Oct 14, 2023 at 10:45 AM Hayato Kuroda (Fujitsu)
wrote:
>
> Here is a new patch.
>
> Previously I wrote:
> > Based on above idea, I made new version patch which some functionalities
> > were
> > exported from pg_resetwal. In this approach, pg_upgrade itself removed WALs
> > and
> > then
Dear hackers,
Here is a new patch.
Previously I wrote:
> Based on above idea, I made new version patch which some functionalities were
> exported from pg_resetwal. In this approach, pg_upgrade itself removed WALs
> and
> then create logical slots, then pg_resetwal would be called with new
Dear Amit,
Thanks for reviewing! New patch is available at [1].
>
> Some more comments:
> 1. Let's restruture binary_upgrade_validate_wal_logical_end() a bit.
> First, let's change its name to binary_upgrade_slot_has_pending_wal()
> or something like that. Then move the context creation and
Dear Amit,
Thanks for your suggestion! PSA new version.
> The other problem is that pg_resetwal removes all pre-existing WAL
> files which in this case could lead to the removal of the WAL file
> corresponding to restart_lsn. This is because at least the shutdown
> checkpoint record will be
On Wed, Oct 11, 2023 at 4:27 PM Hayato Kuroda (Fujitsu)
wrote:
>
> Thank you for reviewing! PSA new version.
>
Some more comments:
1. Let's restruture binary_upgrade_validate_wal_logical_end() a bit.
First, let's change its name to binary_upgrade_slot_has_pending_wal()
or something like that.
Dear Amit,
Thank you for reviewing! PSA new version.
> > I think you need to set the new flag only when we are not skipping the
> > transaction or in other words when we decide to process the
> > transaction. Otherwise, how will you distinguish the case where the
> > xact is already decoded and
On Tue, Oct 10, 2023 at 6:17 PM Amit Kapila wrote:
>
> DecodeTXNNeedSkip(LogicalDecodingContext *ctx, XLogRecordBuffer *buf,
> Oid txn_dbid, RepOriginId origin_id)
> {
> - return (SnapBuildXactNeedsSkip(ctx->snapshot_builder, buf->origptr) ||
> - (txn_dbid != InvalidOid && txn_dbid !=
On Tue, Oct 10, 2023 at 4:51 PM Hayato Kuroda (Fujitsu)
wrote:
>
> >
> > Isn't it sufficient to add a test for silent mode in
> > begin/stream_start/begin_prepare kind of APIs and set
> > ctx->did_process? In all other APIs, we can assert that did_process
> > shouldn't be set and we never reach
Dear Bharath,
Thanks for giving comments and apologize for late reply.
New version is available in [1].
> +1 for this approach. It looks neat.
>
> I think we also need to add TAP tests to generate decodable WAL
> records (RUNNING_XACT, CHECKPOINT_ONLINE, XLOG_FPI_FOR_HINT,
> XLOG_SWITCH,
Dear Vignesh,
Thanks for reviewing! You can available new version in [1].
>
> Few comments:
> 1) Should we add binary upgrade check "CHECK_IS_BINARY_UPGRADE" for
> this funcion too:
> +binary_upgrade_create_logical_replication_slot(PG_FUNCTION_ARGS)
> +{
> + Namename =
Dear Amit,
Thank you for reviewing! PSA new version.
> > Internally, I added a new decoding mode - DECODING_MODE_SILENT - and
> used it.
> > If the decoding context is in the mode, the output plugin is not loaded, but
> > any WALs are decoded without skipping.
> >
>
> I think it may be okay not
On Sat, Oct 7, 2023 at 3:46 AM Amit Kapila wrote:
>
> On Fri, Oct 6, 2023 at 6:30 PM Hayato Kuroda (Fujitsu)
> >
> > Based on that, I added another binary function
> > binary_upgrade_create_logical_replication_slot().
> > This function is similar to pg_create_logical_replication_slot(), but the
On Thu, Oct 5, 2023 at 6:43 PM Bharath Rupireddy
wrote:
>
> On Thu, Oct 5, 2023 at 1:48 AM Amit Kapila wrote:
> >
>
> > The other potential problem Andres pointed out is that during shutdown
> > if due to some reason, the walreceiver goes down, we won't be able to
> > send the required WAL and
1 - 100 of 402 matches
Mail list logo
