On Thu, Dec 14, 2023 at 4:40 PM Amit Kapila wrote:
>
> On Thu, Dec 14, 2023 at 7:00 AM Peter Smith wrote:
> >
> > Hi, here are a few more review comments for the patch v47-0002
> >
> > (plus my review comments of v45-0002 [1] are yet to be addressed)
> >
> > ==
> > 1. General
> >
> > For
On Thu, Dec 14, 2023 at 7:00 AM Peter Smith wrote:
>
> Hi, here are a few more review comments for the patch v47-0002
>
> (plus my review comments of v45-0002 [1] are yet to be addressed)
>
> ==
> 1. General
>
> For consistency and readability, try to use variables of the same
> names
On Thursday, December 14, 2023 12:45 PM Peter Smith
wrote:
> A review comment for v47-0001
Thanks for the comment.
>
> ==
> src/backend/replication/slot.c
>
> 1. GetStandbySlotList
>
> +static void
> +WalSndRereadConfigAndReInitSlotList(List **standby_slots) {
> + char
On Wed, Dec 13, 2023 at 3:53 PM Peter Smith wrote:
>
> 12.
> +static void
> +synchronize_one_slot(WalReceiverConn *wrconn, RemoteSlot *remote_slot,
> + bool *slot_updated)
> +{
> + ReplicationSlot *s;
> + ReplicationSlot *slot;
> + char sync_state = '\0';
>
> In my previous review [1]#33a I
A review comment for v47-0001
==
src/backend/replication/slot.c
1. GetStandbySlotList
+static void
+WalSndRereadConfigAndReInitSlotList(List **standby_slots)
+{
+ char*pre_standby_slot_names;
+
+ ProcessConfigFile(PGC_SIGHUP);
+
+ /*
+ * If we are running on a standby, there is no need
Hi, here are a few more review comments for the patch v47-0002
(plus my review comments of v45-0002 [1] are yet to be addressed)
==
1. General
For consistency and readability, try to use variables of the same
names whenever they have the same purpose, even when they declared are
in
Hi Shveta, here are some review comments for v45-0002.
==
doc/src/sgml/bgworker.sgml
1.
+
+
+ BgWorkerStart_PostmasterStart
+
+
+ BgWorkerStart_PostmasterStart
+ Start as soon as postgres itself has finished its own initialization;
+ processes
On Wed, Dec 13, 2023 at 10:40 AM Amit Kapila wrote:
>
> On Mon, Dec 11, 2023 at 5:13 PM shveta malik wrote:
> >
> > On Mon, Dec 11, 2023 at 1:22 PM Drouvot, Bertrand
> > wrote:
> > >
> > > > If we agree
> > > > on that then it would be good to prohibit setting this GUC on standby
> > > > or at
On Tue, Dec 12, 2023 at 5:56 PM Nisha Moond wrote:
>
> A review on v45 patch:
>
> If one creates a logical slot with failover=true as -
> select pg_create_logical_replication_slot('logical_slot','pgoutput',
> false, true, true);
>
> Then, uses the existing logical slot while creating a
On Mon, Dec 11, 2023 at 5:13 PM shveta malik wrote:
>
> On Mon, Dec 11, 2023 at 1:22 PM Drouvot, Bertrand
> wrote:
> >
> > > If we agree
> > > on that then it would be good to prohibit setting this GUC on standby
> > > or at least it should be a no-op even if this GUC should be set on
> > >
On Tue, Dec 12, 2023 at 2:44 PM shveta malik wrote:
>
> On Mon, Dec 11, 2023 at 7:12 PM Amit Kapila wrote:
> >
>
> > I am
> > asking this because the context used is TopMemoryContext which should
> > be used only if we need something specific to be retained at the
> > process level which doesn't
A review on v45 patch:
If one creates a logical slot with failover=true as -
select pg_create_logical_replication_slot('logical_slot','pgoutput',
false, true, true);
Then, uses the existing logical slot while creating a subscription -
postgres=# create subscription sub4 connection
On Monday, December 11, 2023 3:32 PM Peter Smith
>
> Here are some review comments for v44-0001
>
> ==
> src/backend/replication/slot.c
>
>
> 1. ReplicationSlotCreate
>
> * during getting changes, if the two_phase option is enabled it can skip
> * prepare because by that time
On Mon, Dec 11, 2023 at 7:12 PM Amit Kapila wrote:
>
> On Mon, Dec 11, 2023 at 2:41 PM shveta malik wrote:
> >
> > >
> > > 5.
> > > +synchronize_slots(WalReceiverConn *wrconn)
> > > {
> > > ...
> > > ...
> > > + /* The syscache access needs a transaction env. */
> > > +
On Mon, Dec 11, 2023 at 2:41 PM shveta malik wrote:
>
> >
> > 5.
> > +synchronize_slots(WalReceiverConn *wrconn)
> > {
> > ...
> > ...
> > + /* The syscache access needs a transaction env. */
> > + StartTransactionCommand();
> > +
> > + /*
> > + * Make result tuples live outside
On Mon, Dec 11, 2023 at 1:22 PM Drouvot, Bertrand
wrote:
>
> Hi,
>
> On 12/8/23 10:06 AM, Amit Kapila wrote:
> > On Wed, Dec 6, 2023 at 4:53 PM shveta malik wrote:
> >>
> >> PFA v43, changes are:
> >>
> >
> > I wanted to discuss 0003 patch about cascading standby's. It is not
> > clear to me
On Mon, Dec 11, 2023 at 2:21 PM shveta malik wrote:
>
> On Mon, Dec 11, 2023 at 1:47 PM Dilip Kumar wrote:
> >
> > On Fri, Dec 8, 2023 at 2:36 PM Amit Kapila wrote:
> > >
> > > On Wed, Dec 6, 2023 at 4:53 PM shveta malik
> > > wrote:
> > > >
> > > > PFA v43, changes are:
> > > >
> > >
> > > I
On Thu, Dec 7, 2023 at 1:33 PM Peter Smith wrote:
>
> Hi.
>
> Here are my review comments for patch v43-0002.
>
Thanks for the feedback. I have addressed most of these in v45. Please
find my response on a few which are pending or are not needed.
> ==
> Commit message
>
> 1.
> The nap time
On Sun, Dec 10, 2023 at 4:33 PM Amit Kapila wrote:
>
> On Wed, Dec 6, 2023 at 4:53 PM shveta malik wrote:
> >
> > v43-002:
> >
>
> Review comments on v43-0002:
> =
Thanks for the feedback Amit. Addressed these in v45. Please find my
response on a few of these.
> 1.
>
On Mon, Dec 11, 2023 at 1:47 PM Dilip Kumar wrote:
>
> On Fri, Dec 8, 2023 at 2:36 PM Amit Kapila wrote:
> >
> > On Wed, Dec 6, 2023 at 4:53 PM shveta malik wrote:
> > >
> > > PFA v43, changes are:
> > >
> >
> > I wanted to discuss 0003 patch about cascading standby's. It is not
> > clear to me
On Mon, Dec 11, 2023 at 1:02 PM Peter Smith wrote:
>
> Here are some review comments for v44-0001
>
> ~~~
>
> 3. assign_standby_slot_names
>
> + if (!SplitIdentifierString(standby_slot_names_cpy, ',', _slots))
> + {
> + /* This should not happen if GUC checked check_standby_slot_names. */
> +
On Fri, Dec 8, 2023 at 2:36 PM Amit Kapila wrote:
>
> On Wed, Dec 6, 2023 at 4:53 PM shveta malik wrote:
> >
> > PFA v43, changes are:
> >
>
> I wanted to discuss 0003 patch about cascading standby's. It is not
> clear to me whether we want to allow physical standbys to further wait
> for
Hi,
On 12/8/23 10:06 AM, Amit Kapila wrote:
On Wed, Dec 6, 2023 at 4:53 PM shveta malik wrote:
PFA v43, changes are:
I wanted to discuss 0003 patch about cascading standby's. It is not
clear to me whether we want to allow physical standbys to further wait
for cascading standby to sync
Here are some review comments for v44-0001
==
src/backend/replication/slot.c
1. ReplicationSlotCreate
* during getting changes, if the two_phase option is enabled it can skip
* prepare because by that time start decoding point has been moved. So the
* user will only get
FYI -- the patch 0002 did not apply cleanly for me on top of the 050
test file created by patch 0001.
[postgres@CentOS7-x64 oss_postgres_misc]$ git apply
../patches_misc/v44-0001-Allow-logical-walsenders-to-wait-for-the-physica.patch
[postgres@CentOS7-x64 oss_postgres_misc]$ git apply
On Wed, Dec 6, 2023 at 4:53 PM shveta malik wrote:
>
> v43-002:
>
Review comments on v43-0002:
=
1.
synchronize_one_slot()
{
...
+ /*
+ * With hot_standby_feedback enabled and invalidations handled
+ * apropriately as above, this should never happen.
+ */
+ if
On Wed, Dec 6, 2023 at 4:53 PM shveta malik wrote:
>
> PFA v43, changes are:
>
I wanted to discuss 0003 patch about cascading standby's. It is not
clear to me whether we want to allow physical standbys to further wait
for cascading standby to sync their slots. If we allow such a feature
one may
On Thu, Dec 7, 2023 at 2:57 PM Drouvot, Bertrand
wrote:
>
> Hi,
>
> On 12/7/23 10:07 AM, shveta malik wrote:
> > On Thu, Dec 7, 2023 at 1:19 PM Drouvot, Bertrand
> > wrote:
> >> Might be worth to add comments in the code (around the WalRcv->latestWalEnd
> >> checks) that no "lagging" sync are
Hi.
Here is another review comment for the patch v43-0001.
==
src/bin/pg_dump/pg_dump.c
1. getSubscriptions
+ if (fout->remoteVersion >= 17)
+ appendPQExpBufferStr(query,
+ " subfailoverstate\n");
+ else
+ appendPQExpBuffer(query,
+ " '%c' AS subfailoverstate\n",
+
On Wed, Dec 6, 2023 at 4:53 PM shveta malik wrote:
>
> v43-001:
> 1) Support of 'failover' dump in pg_dump. It was missing earlier.
>
Review v43-0001
1.
+ * However, we do not enable failover for slots created by the table sync
+ * worker. This is because the table sync slot
Hi,
On 12/7/23 10:07 AM, shveta malik wrote:
On Thu, Dec 7, 2023 at 1:19 PM Drouvot, Bertrand
wrote:
Might be worth to add comments in the code (around the WalRcv->latestWalEnd
checks) that no "lagging" sync are possible if the walreceiver is not started
though?
I am a bit confused. Do you
On Thu, Dec 7, 2023 at 1:19 PM Drouvot, Bertrand
wrote:
>
> Hi,
>
> On 12/6/23 11:58 AM, shveta malik wrote:
> > On Wed, Dec 6, 2023 at 3:00 PM Drouvot, Bertrand
> > wrote:
> >>
> >> Hi,
> >>
> >> On 12/6/23 7:18 AM, shveta malik wrote:
> >>> On Wed, Dec 6, 2023 at 10:56 AM Amit Kapila
> >>>
Hi.
Here are my review comments for patch v43-0002.
==
Commit message
1.
The nap time of worker is tuned according to the activity on the primary.
The worker starts with nap time of 10ms and if no activity is observed on
the primary for some time, then nap time is increased to 10sec. And if
Hi,
On 12/6/23 12:23 PM, shveta malik wrote:
On Wed, Dec 6, 2023 at 4:28 PM shveta malik wrote:
PFA v43, changes are:
Thanks!
v43-001:
1) Support of 'failover' dump in pg_dump. It was missing earlier.
v43-002:
1) Slot-sync worker now checks validity of primary_slot_name by
connecting
Hi,
On 12/6/23 11:58 AM, shveta malik wrote:
On Wed, Dec 6, 2023 at 3:00 PM Drouvot, Bertrand
wrote:
Hi,
On 12/6/23 7:18 AM, shveta malik wrote:
On Wed, Dec 6, 2023 at 10:56 AM Amit Kapila wrote:
I feel that is indirectly relying on the fact that the primary won't
advance logical slots
On Wed, Dec 6, 2023 at 3:00 PM Drouvot, Bertrand
wrote:
>
> Hi,
>
> On 12/6/23 7:18 AM, shveta malik wrote:
> > On Wed, Dec 6, 2023 at 10:56 AM Amit Kapila wrote:
> >>
> >> I feel that is indirectly relying on the fact that the primary won't
> >> advance logical slots unless physical standby has
Hi,
On 12/6/23 7:18 AM, shveta malik wrote:
On Wed, Dec 6, 2023 at 10:56 AM Amit Kapila wrote:
I feel that is indirectly relying on the fact that the primary won't
advance logical slots unless physical standby has consumed data.
Yes, that is the basis of this discussion.
Yes.
But now
On Wed, Dec 6, 2023 at 10:56 AM Amit Kapila wrote:
>
> On Tue, Dec 5, 2023 at 7:38 PM Drouvot, Bertrand
> wrote:
> >
> > On 12/5/23 12:32 PM, Amit Kapila wrote:
> > > On Tue, Dec 5, 2023 at 10:38 AM shveta malik
> > > wrote:
> > >>
> > >> On Mon, Dec 4, 2023 at 10:07 PM Drouvot, Bertrand
> >
On Tue, Dec 5, 2023 at 7:38 PM Drouvot, Bertrand
wrote:
>
> On 12/5/23 12:32 PM, Amit Kapila wrote:
> > On Tue, Dec 5, 2023 at 10:38 AM shveta malik wrote:
> >>
> >> On Mon, Dec 4, 2023 at 10:07 PM Drouvot, Bertrand
> >> wrote:
>
> >>>
> >>> Maybe another option could be to have the
Hi,
On 12/5/23 12:32 PM, Amit Kapila wrote:
On Tue, Dec 5, 2023 at 10:38 AM shveta malik wrote:
On Mon, Dec 4, 2023 at 10:07 PM Drouvot, Bertrand
wrote:
Maybe another option could be to have the walreceiver a way to let the slot sync
worker knows that it (the walreceiver) was not able
Hi,
On 12/5/23 11:29 AM, shveta malik wrote:
On Tue, Dec 5, 2023 at 2:18 PM Drouvot, Bertrand
wrote:
Wouldn't that make sense to move it once we are sure that
walrcv_startstreaming() returns true and first_stream is true, here?
"
if (first_stream)
+
On Tue, Dec 5, 2023 at 10:38 AM shveta malik wrote:
>
> On Mon, Dec 4, 2023 at 10:07 PM Drouvot, Bertrand
> wrote:
> >
> > >
> > >> ~~~
> > >> 4. primary_slot_name GUC value test:
> > >>
> > >> When standby is started with a non-existing primary_slot_name, the
> > >> wal-receiver gives an error
On Tue, Dec 5, 2023 at 2:18 PM Drouvot, Bertrand
wrote:
>
> Hi,
>
> On 12/5/23 6:08 AM, shveta malik wrote:
> > On Mon, Dec 4, 2023 at 10:07 PM Drouvot, Bertrand
> > wrote:
> >> Maybe another option could be to have the walreceiver a way to let the
> >> slot sync
> >> worker knows that it (the
Hi,
On 12/5/23 6:08 AM, shveta malik wrote:
On Mon, Dec 4, 2023 at 10:07 PM Drouvot, Bertrand
wrote:
Maybe another option could be to have the walreceiver a way to let the slot sync
worker knows that it (the walreceiver) was not able to start due to non existing
replication slot on the
On Mon, Dec 4, 2023 at 10:07 PM Drouvot, Bertrand
wrote:
>
> Hi,
>
> On 12/4/23 6:10 AM, shveta malik wrote:
> > On Fri, Dec 1, 2023 at 5:40 PM Nisha Moond wrote:
> >>
> >> Review for v41 patch.
> >
> > Thanks for the feedback.
> >
> >> ~~~
> >> 2.
> >> IIUC, the slotsyncworker's connection to
Hi,
On 12/4/23 6:10 AM, shveta malik wrote:
On Fri, Dec 1, 2023 at 5:40 PM Nisha Moond wrote:
Review for v41 patch.
Thanks for the feedback.
~~~
2.
IIUC, the slotsyncworker's connection to the primary is to execute a
query. Its aim is not walsender type connection, but at primary when
Hi,
On 12/4/23 4:33 AM, Zhijie Hou (Fujitsu) wrote:
On Wednesday, November 29, 2023 5:55 PM Drouvot, Bertrand
wrote:
I think I'm fine with documenting the fact that the user should not change the
failover value. But if he does change it (because at the end nothing prevents it
to do so) then
On Mon, Dec 4, 2023 at 10:40 AM shveta malik wrote:
>
> On Fri, Dec 1, 2023 at 5:40 PM Nisha Moond wrote:
> >
> > Review for v41 patch.
>
> Thanks for the feedback.
>
> >
> > 1.
> > ==
> > src/backend/utils/misc/postgresql.conf.sample
> >
> > +#enable_syncslot = on # enables slot
On Fri, Dec 1, 2023 at 5:40 PM Nisha Moond wrote:
>
> Review for v41 patch.
Thanks for the feedback.
>
> 1.
> ==
> src/backend/utils/misc/postgresql.conf.sample
>
> +#enable_syncslot = on # enables slot synchronization on the physical
> standby from the primary
>
> enable_syncslot is
On Wednesday, November 29, 2023 5:55 PM Drouvot, Bertrand
wrote:
Hi,
> On 11/29/23 6:58 AM, Zhijie Hou (Fujitsu) wrote:
> > On Tuesday, November 28, 2023 8:07 PM Drouvot, Bertrand
> wrote:
> >
> > Hi,
> >
> >> On 11/27/23 9:57 AM, Zhijie Hou (Fujitsu) wrote:
> >>> On Monday, November 27, 2023
On Fri, Dec 1, 2023 at 9:31 PM Nisha Moond wrote:
>
> On Fri, Dec 1, 2023 at 5:40 PM Nisha Moond wrote:
> >
> > Review for v41 patch.
> >
> > 1.
> > ==
> > src/backend/utils/misc/postgresql.conf.sample
> >
> > +#enable_syncslot = on # enables slot synchronization on the physical
> > standby
On Fri, Dec 1, 2023 at 5:40 PM Nisha Moond wrote:
>
> Review for v41 patch.
>
> 1.
> ==
> src/backend/utils/misc/postgresql.conf.sample
>
> +#enable_syncslot = on # enables slot synchronization on the physical
> standby from the primary
>
> enable_syncslot is disabled by default, so, it
Review for v41 patch.
1.
==
src/backend/utils/misc/postgresql.conf.sample
+#enable_syncslot = on # enables slot synchronization on the physical
standby from the primary
enable_syncslot is disabled by default, so, it should be 'off' here.
~~~
2.
IIUC, the slotsyncworker's connection to the
Hi,
On 12/1/23 12:06 PM, Amit Kapila wrote:
On Wed, Nov 29, 2023 at 3:24 PM Drouvot, Bertrand
wrote:
I think I'm fine with documenting the fact that the user should not change the
failover
value. But if he does change it (because at the end nothing prevents it to do
so) then
I think the
On Wed, Nov 29, 2023 at 3:24 PM Drouvot, Bertrand
wrote:
>
> On 11/29/23 6:58 AM, Zhijie Hou (Fujitsu) wrote:
> > On Tuesday, November 28, 2023 8:07 PM Drouvot, Bertrand
> > wrote:
> >
> > Hi,
> >
> >> On 11/27/23 9:57 AM, Zhijie Hou (Fujitsu) wrote:
> >>> On Monday, November 27, 2023 4:51 PM
Hi,
On 12/1/23 4:19 AM, shveta malik wrote:
On Thu, Nov 30, 2023 at 5:37 PM Ajin Cherian wrote:
1. In my opinion, the second message "aborting the wait...moving to
the next slot" does not hold much value. There might not even be a
"next slot", there might be just one slot. I think the first
On Fri, Dec 1, 2023 at 3:43 PM Drouvot, Bertrand
wrote:
>
> Hi,
>
> On 11/30/23 1:06 PM, Ajin Cherian wrote:
> > On Wed, Nov 29, 2023 at 8:17 PM Zhijie Hou (Fujitsu)
> >
> > 3. If creation of a slot on the standby fails for one slot because a
> > slot of the same name exists, then thereafter no
Hi,
On 11/30/23 1:06 PM, Ajin Cherian wrote:
On Wed, Nov 29, 2023 at 8:17 PM Zhijie Hou (Fujitsu)
3. If creation of a slot on the standby fails for one slot because a
slot of the same name exists, then thereafter no new sync slots are
created on standby. Is this expected? I do see that
On Fri, Dec 1, 2023 at 11:17 AM Zhijie Hou (Fujitsu)
wrote:
>
> On Friday, December 1, 2023 12:51 PM shveta malik
> wrote:
>
> Hi,
>
> >
> > On Fri, Dec 1, 2023 at 9:40 AM Zhijie Hou (Fujitsu)
> > wrote:
> > >
> > > On Wednesday, November 29, 2023 5:12 PM Zhijie Hou (Fujitsu)
> > wrote:
> > >
On Friday, December 1, 2023 12:51 PM shveta malik
wrote:
Hi,
>
> On Fri, Dec 1, 2023 at 9:40 AM Zhijie Hou (Fujitsu)
> wrote:
> >
> > On Wednesday, November 29, 2023 5:12 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> > I was reviewing slotsync worker design and here
> > are few comments on 0002
On Fri, Dec 1, 2023 at 9:40 AM Zhijie Hou (Fujitsu)
wrote:
>
> On Wednesday, November 29, 2023 5:12 PM Zhijie Hou (Fujitsu)
> wrote:
>
> I was reviewing slotsync worker design and here
> are few comments on 0002 patch:
Thanks for reviewing the patch.
>
> 1.
>
> + if (!WalRcv ||
> +
On Wednesday, November 29, 2023 5:12 PM Zhijie Hou (Fujitsu)
wrote:
I was reviewing slotsync worker design and here
are few comments on 0002 patch:
1.
+ if (!WalRcv ||
+ (WalRcv->slotname[0] == '\0') ||
+ XLogRecPtrIsInvalid(WalRcv->latestWalEnd))
I think
On Thu, Nov 30, 2023 at 5:37 PM Ajin Cherian wrote:
>
> On Wed, Nov 29, 2023 at 8:17 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> > This has been fixed.
> >
> > Best Regards,
> > Hou zj
>
> Thanks for addressing my comments. Some comments from my testing of patch v41
>
> 1. In my opinion, the second
On Wed, Nov 29, 2023 at 8:17 PM Zhijie Hou (Fujitsu)
wrote:
>
> This has been fixed.
>
> Best Regards,
> Hou zj
Thanks for addressing my comments. Some comments from my testing of patch v41
1. In my opinion, the second message "aborting the wait...moving to
the next slot" does not hold much
Here are some review comments for v41-0001.
==
doc/src/sgml/ref/alter_subscription.sgml
1.
+
+ When altering the
+ slot_name,
+ the failover property of the new slot may
differ from the
+ failover
+ parameter specified in the subscription, you need to adjust the
Hi,
On 11/29/23 3:58 AM, Amit Kapila wrote:
On Tue, Nov 28, 2023 at 2:17 PM Drouvot, Bertrand
wrote:
What do you think about also adding a pg_alter_logical_replication_slot() or
such
function?
That would allow users to alter manually created logical replication slots
without
the need to
Hi,
On 11/29/23 6:58 AM, Zhijie Hou (Fujitsu) wrote:
On Tuesday, November 28, 2023 8:07 PM Drouvot, Bertrand
wrote:
Hi,
On 11/27/23 9:57 AM, Zhijie Hou (Fujitsu) wrote:
On Monday, November 27, 2023 4:51 PM shveta malik
wrote:
Here is the updated version(v39_2) which include all the
On Thursday, November 23, 2023 6:06 PM Ajin Cherian wrote:
>
> On Tue, Nov 21, 2023 at 8:32 PM shveta malik
> wrote:
> >
> > v37 fails to apply to HEAD due to a recent commit e83aa9f92fdd,
> > rebased the patches. PFA v37_2 patches.
>
> Thanks for the patch. Some comments:
Thanks for the
On Wednesday, November 29, 2023 11:04 AM Peter Smith
wrote:
Thanks for the comments.
> ==
> 1. General.
>
> Previously (see [1] #0) I asked a question about if there is some
> documentation
> missing. Seems not yet answered.
The document was add in V39-0002 in logicaldecoding.sgml
On Tuesday, November 28, 2023 8:07 PM Drouvot, Bertrand
wrote:
Hi,
> On 11/27/23 9:57 AM, Zhijie Hou (Fujitsu) wrote:
> > On Monday, November 27, 2023 4:51 PM shveta malik
> wrote:
> >
> > Here is the updated version(v39_2) which include all the changes made in
> 0002.
> > Please use for
On Tue, Nov 28, 2023 at 9:28 PM Drouvot, Bertrand
wrote:
>
> Hi,
>
> On 11/28/23 10:40 AM, shveta malik wrote:
> > On Tue, Nov 28, 2023 at 12:19 PM Drouvot, Bertrand
> > wrote:
> >>
> >> Hi,
> >>
> >> On 11/28/23 4:13 AM, shveta malik wrote:
> >>> On Mon, Nov 27, 2023 at 4:08 PM Amit Kapila
>
Hi. Here are some review comments for the patch v39_2-0001.
Multiple items from my previous review [1] seemed unanswered, so it
wasn't clear if they were discarded because they were wrong or maybe
accidently missed. I've repeated all those again here, as well as some
new comments.
==
1.
On Tue, Nov 28, 2023 at 2:17 PM Drouvot, Bertrand
wrote:
>
> On 11/2/23 1:27 AM, Zhijie Hou (Fujitsu) wrote:
> > On Tuesday, October 31, 2023 6:45 PM Amit Kapila
> > wrote:
> >> We have create_replication_slot and drop_replication_slot in repl_gram.y.
> >> How
> >> about if introduce
Hi,
On 11/28/23 10:40 AM, shveta malik wrote:
On Tue, Nov 28, 2023 at 12:19 PM Drouvot, Bertrand
wrote:
Hi,
On 11/28/23 4:13 AM, shveta malik wrote:
On Mon, Nov 27, 2023 at 4:08 PM Amit Kapila wrote:
On Mon, Nov 27, 2023 at 2:27 PM Zhijie Hou (Fujitsu)
wrote:
Here is the updated
Hi,
On 11/27/23 9:57 AM, Zhijie Hou (Fujitsu) wrote:
On Monday, November 27, 2023 4:51 PM shveta malik
wrote:
Here is the updated version(v39_2) which include all the changes made in 0002.
Please use for review, and sorry for the confusion.
Thanks!
As far v39_2-0001:
"
Altering the
On Fri, Nov 17, 2023 at 5:08 PM Amit Kapila wrote:
>
> On Thu, Nov 16, 2023 at 5:34 PM shveta malik wrote:
> >
> > PFA v35.
> >
>
> Review v35-0002*
> ==
> 1.
> As quoted in the commit message,
> >
> If a logical slot is invalidated on the primary, slot on the standby is also
>
On Tue, Nov 28, 2023 at 3:10 PM shveta malik wrote:
>
> On Tue, Nov 28, 2023 at 12:19 PM Drouvot, Bertrand
> wrote:
> >
> > Hi,
> >
> > On 11/28/23 4:13 AM, shveta malik wrote:
> > > On Mon, Nov 27, 2023 at 4:08 PM Amit Kapila
> > > wrote:
> > >>
> > >> On Mon, Nov 27, 2023 at 2:27 PM Zhijie
Hi,
On 11/2/23 1:27 AM, Zhijie Hou (Fujitsu) wrote:
On Tuesday, October 31, 2023 6:45 PM Amit Kapila
wrote:
We have create_replication_slot and drop_replication_slot in repl_gram.y. How
about if introduce alter_replication_slot and handle the 'failover' flag with
that?
The idea is we will
Hi,
On 11/28/23 4:13 AM, shveta malik wrote:
On Mon, Nov 27, 2023 at 4:08 PM Amit Kapila wrote:
On Mon, Nov 27, 2023 at 2:27 PM Zhijie Hou (Fujitsu)
wrote:
Here is the updated version(v39_2) which include all the changes made in 0002.
Please use for review, and sorry for the confusion.
On Mon, Nov 27, 2023 at 4:08 PM Amit Kapila wrote:
>
> On Mon, Nov 27, 2023 at 2:27 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> > Here is the updated version(v39_2) which include all the changes made in
> > 0002.
> > Please use for review, and sorry for the confusion.
> >
>
> ---
Hi,
On 11/27/23 1:23 PM, Zhijie Hou (Fujitsu) wrote:
On Monday, November 27, 2023 8:05 PM Drouvot, Bertrand
wrote:
Hi,
On 11/6/23 2:30 AM, Zhijie Hou (Fujitsu) wrote:
On Friday, November 3, 2023 7:32 PM Amit Kapila
I don't see a corresponding change in repl_gram.y. I think the
Hi,
On 11/27/23 1:23 PM, Zhijie Hou (Fujitsu) wrote:
On Monday, November 27, 2023 8:05 PM Drouvot, Bertrand
wrote:
Did not look in details but it looks like there is more to do here as
this is failing (with v39_2):
"
postgres@primary: psql replication=database
psql (17devel)
Type "help" for
On Monday, November 27, 2023 8:05 PM Drouvot, Bertrand
wrote:
Hi,
> On 11/6/23 2:30 AM, Zhijie Hou (Fujitsu) wrote:
> > On Friday, November 3, 2023 7:32 PM Amit Kapila
>
> >>
> >> I don't see a corresponding change in repl_gram.y. I think the following
> >> part
> of
> >> the code needs to
Hi,
On 11/6/23 2:30 AM, Zhijie Hou (Fujitsu) wrote:
On Friday, November 3, 2023 7:32 PM Amit Kapila
I don't see a corresponding change in repl_gram.y. I think the following part of
the code needs to be changed:
/* CREATE_REPLICATION_SLOT slot [TEMPORARY] LOGICAL plugin [options] */
|
On Mon, Nov 27, 2023 at 2:27 PM Zhijie Hou (Fujitsu)
wrote:
>
> Here is the updated version(v39_2) which include all the changes made in 0002.
> Please use for review, and sorry for the confusion.
>
--- a/src/backend/replication/logical/launcher.c
+++ b/src/backend/replication/logical/launcher.c
On Mon, Nov 27, 2023 at 2:15 PM Drouvot, Bertrand
wrote:
>
> Hi,
>
> On 11/27/23 7:02 AM, Zhijie Hou (Fujitsu) wrote:
> > On Monday, November 27, 2023 12:03 PM Zhijie Hou (Fujitsu)
> > wrote:
> >>
> >> Attach the V38 patch set which addressed all comments in [1][2] except for
> >> the
> >>
Hi,
On 11/27/23 7:02 AM, Zhijie Hou (Fujitsu) wrote:
On Monday, November 27, 2023 12:03 PM Zhijie Hou (Fujitsu)
wrote:
Attach the V38 patch set which addressed all comments in [1][2] except for the
ones that mentioned above.
[1]
Hi,
On 11/24/23 10:45 AM, Amit Kapila wrote:
On Fri, Nov 24, 2023 at 1:53 PM Drouvot, Bertrand
wrote:
On 11/24/23 4:35 AM, Zhijie Hou (Fujitsu) wrote:
On Thursday, November 23, 2023 11:45 PM Drouvot, Bertrand
wrote:
IOW, the logical slot's LSN can still be advanced after the
walreceiver
On Fri, Nov 24, 2023 at 1:53 PM Drouvot, Bertrand
wrote:
>
> On 11/24/23 4:35 AM, Zhijie Hou (Fujitsu) wrote:
> > On Thursday, November 23, 2023 11:45 PM Drouvot, Bertrand
> > wrote:
> >
> > IOW, the logical slot's LSN can still be advanced after the
> > walreceiver shutdown if it was far
Hi,
On 11/23/23 11:45 AM, Amit Kapila wrote:
On Wed, Nov 22, 2023 at 10:02 AM Zhijie Hou (Fujitsu)
wrote:
Or we could just document that it is user's responsibility to match the failover
property in case it changes the slot_name.
Personally, I think we should document this behavior
Hi,
On 11/24/23 4:35 AM, Zhijie Hou (Fujitsu) wrote:
On Thursday, November 23, 2023 11:45 PM Drouvot, Bertrand
wrote:
IOW, the logical slot's LSN can still be advanced after the
walreceiver shutdown if it was far bebind the physical slot's LSN.
oh yeah right, it would need much more
On Thursday, November 23, 2023 11:45 PM Drouvot, Bertrand
wrote:
Hi,
>
> On 11/23/23 6:13 AM, Amit Kapila wrote:
> > On Tue, Nov 21, 2023 at 4:35 PM Drouvot, Bertrand
> > wrote:
> >>
> >> On 11/21/23 10:32 AM, shveta malik wrote:
> >>> On Tue, Nov 21, 2023 at 2:02 PM shveta malik
> wrote:
>
Hi,
On 11/23/23 6:13 AM, Amit Kapila wrote:
On Tue, Nov 21, 2023 at 4:35 PM Drouvot, Bertrand
wrote:
On 11/21/23 10:32 AM, shveta malik wrote:
On Tue, Nov 21, 2023 at 2:02 PM shveta malik wrote:
v37 fails to apply to HEAD due to a recent commit e83aa9f92fdd,
rebased the patches. PFA
On Wed, Nov 22, 2023 at 10:02 AM Zhijie Hou (Fujitsu)
wrote:
>
> On Tuesday, November 21, 2023 5:33 PM shveta malik
> wrote:
> >
> >
> > v37 fails to apply to HEAD due to a recent commit e83aa9f92fdd, rebased the
> > patches. PFA v37_2 patches.
>
> Thanks for updating the patches.
>
> I'd like
On Tue, Nov 21, 2023 at 8:32 PM shveta malik wrote:
>
> v37 fails to apply to HEAD due to a recent commit e83aa9f92fdd,
> rebased the patches. PFA v37_2 patches.
Thanks for the patch. Some comments:
subscriptioncmds.c:
CreateSubscription()
and tablesync.c:
process_syncing_tables_for_apply()
On Tue, Nov 21, 2023 at 4:35 PM Drouvot, Bertrand
wrote:
>
> On 11/21/23 10:32 AM, shveta malik wrote:
> > On Tue, Nov 21, 2023 at 2:02 PM shveta malik wrote:
> >>
>
> > v37 fails to apply to HEAD due to a recent commit e83aa9f92fdd,
> > rebased the patches. PFA v37_2 patches.
>
> Thanks!
>
>
In addition to my recent v35-0001 comment not yet addressed [1], here
are some review comments for patch v37-0001.
==
src/backend/replication/walsender.c
1. PhysicalWakeupLogicalWalSnd
+/*
+ * Wake up logical walsenders with failover-enabled slots if the physical slot
+ * of the current
On Tuesday, November 21, 2023 5:33 PM shveta malik
wrote:
>
>
> v37 fails to apply to HEAD due to a recent commit e83aa9f92fdd, rebased the
> patches. PFA v37_2 patches.
Thanks for updating the patches.
I'd like to discuss one issue related to the correct handling of failover flag
when
Hi,
On 11/21/23 10:32 AM, shveta malik wrote:
On Tue, Nov 21, 2023 at 2:02 PM shveta malik wrote:
v37 fails to apply to HEAD due to a recent commit e83aa9f92fdd,
rebased the patches. PFA v37_2 patches.
Thanks!
Regarding the promotion flow: If the primary is available and reachable I
On Tue, Nov 21, 2023 at 1:13 PM Drouvot, Bertrand
wrote:
>
> Hi,
>
> On 11/21/23 6:16 AM, Amit Kapila wrote:
> > On Mon, Nov 20, 2023 at 6:51 PM Drouvot, Bertrand
> > wrote:
> >> As far the 'i' state here, from what I see, it is currently useful for:
> >>
> >> 1. Cascading standby to not sync
501 - 600 of 839 matches
Mail list logo
