On Fri, Aug 11, 2023 at 10:46:31AM +0530, Amit Kapila wrote:
> On Thu, Aug 10, 2023 at 7:07 PM Masahiko Sawada wrote:
> > What I imagined is that we do this check before
> > check_and_dump_old_cluster() while the server is 'off'. Reading the
> > slot state file would be simple and I guess we would
On Fri, Aug 11, 2023 at 11:18:09AM +0530, Amit Kapila wrote:
> On Fri, Aug 11, 2023 at 10:43 AM Julien Rouhaud wrote:
> > I disagree. As I mentioned before any module registered in
> > shared_preload_libraries can spawn background workers which can perform any
> > activity. There were previous r
On Fri, Aug 11, 2023 at 10:43 AM Julien Rouhaud wrote:
>
> On Thu, Aug 10, 2023 at 04:30:40PM +0900, Masahiko Sawada wrote:
> > On Thu, Aug 10, 2023 at 2:27 PM Amit Kapila wrote:
> > >
> > > Sawada-San, Julien, and others, do you have any thoughts on the above
> > > point?
> >
> > IIUC during th
On Thu, Aug 10, 2023 at 7:07 PM Masahiko Sawada wrote:
>
> On Thu, Aug 10, 2023 at 12:52 PM Amit Kapila wrote:
> >
> >
> > Are you suggesting doing this before we start the old cluster or after
> > we stop the old cluster? I was thinking about the pros and cons of
> > doing this check when the se
Hi,
On Thu, Aug 10, 2023 at 04:30:40PM +0900, Masahiko Sawada wrote:
> On Thu, Aug 10, 2023 at 2:27 PM Amit Kapila wrote:
> >
> > Sawada-San, Julien, and others, do you have any thoughts on the above point?
>
> IIUC during the old cluster running in the middle of pg_upgrade it
> doesn't accept TC
On Thu, Aug 10, 2023 at 10:37:04PM +0900, Masahiko Sawada wrote:
> On Thu, Aug 10, 2023 at 12:52 PM Amit Kapila wrote:
> > Are you suggesting doing this before we start the old cluster or after
> > we stop the old cluster? I was thinking about the pros and cons of
> > doing this check when the ser
Dear hackers,
Based on recent discussions, I updated the patch set. I did not reply one by one
because there are many posts, but thank you for giving many suggestion!
Followings shows what I changed.
1.
This feature is now enabled by default. Instead
"--exclude-logical-replication-slots"
was ad
On Thu, Aug 10, 2023 at 12:52 PM Amit Kapila wrote:
>
> On Thu, Aug 10, 2023 at 6:46 AM Masahiko Sawada wrote:
> >
> > On Wed, Aug 9, 2023 at 1:15 PM Amit Kapila wrote:
> > >
> > > On Wed, Aug 9, 2023 at 8:01 AM Masahiko Sawada
> > > wrote:
> > >
> > > I feel it would be a good idea to provide
On Thu, Aug 10, 2023 at 2:27 PM Amit Kapila wrote:
>
> On Mon, Aug 7, 2023 at 3:46 PM Amit Kapila wrote:
> >
> > On Mon, Aug 7, 2023 at 1:06 PM Julien Rouhaud wrote:
> > >
> > > On Mon, Aug 07, 2023 at 12:42:33PM +0530, Amit Kapila wrote:
> > > > On Mon, Aug 7, 2023 at 11:29 AM Julien Rouhaud
On Mon, Aug 7, 2023 at 3:46 PM Amit Kapila wrote:
>
> On Mon, Aug 7, 2023 at 1:06 PM Julien Rouhaud wrote:
> >
> > On Mon, Aug 07, 2023 at 12:42:33PM +0530, Amit Kapila wrote:
> > > On Mon, Aug 7, 2023 at 11:29 AM Julien Rouhaud wrote:
> > > >
> > > > Unless I'm missing something I don't see wha
On Thu, Aug 10, 2023 at 6:46 AM Masahiko Sawada wrote:
>
> On Wed, Aug 9, 2023 at 1:15 PM Amit Kapila wrote:
> >
> > On Wed, Aug 9, 2023 at 8:01 AM Masahiko Sawada
> > wrote:
> >
> > I feel it would be a good idea to provide such a tool for users to
> > avoid getting errors during upgrade but I
On Wed, Aug 9, 2023 at 1:15 PM Amit Kapila wrote:
>
> On Wed, Aug 9, 2023 at 8:01 AM Masahiko Sawada wrote:
> >
> > On Mon, Aug 7, 2023 at 6:02 PM Amit Kapila wrote:
> > >
> > > On Mon, Aug 7, 2023 at 2:02 PM Masahiko Sawada
> > > wrote:
> > > >
> > > > WAL records for hint bit updates could b
On Wed, Aug 9, 2023 at 8:01 AM Masahiko Sawada wrote:
>
> On Mon, Aug 7, 2023 at 6:02 PM Amit Kapila wrote:
> >
> > On Mon, Aug 7, 2023 at 2:02 PM Masahiko Sawada
> > wrote:
> > >
> > > WAL records for hint bit updates could be generated even in upgrading
> > > mode?
> > >
> >
> > Do you mean
On Mon, Aug 7, 2023 at 6:02 PM Amit Kapila wrote:
>
> On Mon, Aug 7, 2023 at 2:02 PM Masahiko Sawada wrote:
> >
> > On Mon, Aug 7, 2023 at 12:54 PM Amit Kapila wrote:
> > >
> > > On Sun, Aug 6, 2023 at 6:02 PM Masahiko Sawada
> > > wrote:
> > > >
> > > > IIUC the above query checks if the WAL
Dear Amit, Julien,
> > > >
> > > > Unless I'm missing something I don't see what prevents something to
> connect
> > > > using the replication protocol and issue any query or even create new
> > > > replication slots?
> > > >
> > >
> > > I think the point is that if we have any slots where we have
On Mon, Aug 7, 2023 at 1:06 PM Julien Rouhaud wrote:
>
> On Mon, Aug 07, 2023 at 12:42:33PM +0530, Amit Kapila wrote:
> > On Mon, Aug 7, 2023 at 11:29 AM Julien Rouhaud wrote:
> > >
> > > Unless I'm missing something I don't see what prevents something to
> > > connect
> > > using the replicatio
On Mon, Aug 7, 2023 at 2:02 PM Masahiko Sawada wrote:
>
> On Mon, Aug 7, 2023 at 12:54 PM Amit Kapila wrote:
> >
> > On Sun, Aug 6, 2023 at 6:02 PM Masahiko Sawada
> > wrote:
> > >
> > > IIUC the above query checks if the WAL record written at the slot's
> > > confirmed_flush_lsn is a CHECKPOIN
On Mon, Aug 7, 2023 at 12:54 PM Amit Kapila wrote:
>
> On Sun, Aug 6, 2023 at 6:02 PM Masahiko Sawada wrote:
> >
> > On Wed, Aug 2, 2023 at 5:13 PM Hayato Kuroda (Fujitsu)
> > wrote:
> > >
> > > > 4.
> > > > + /*
> > > > + * Check that all logical replication slots have reached the current
> >
On Mon, Aug 07, 2023 at 12:42:33PM +0530, Amit Kapila wrote:
> On Mon, Aug 7, 2023 at 11:29 AM Julien Rouhaud wrote:
> >
> > Unless I'm missing something I don't see what prevents something to connect
> > using the replication protocol and issue any query or even create new
> > replication slots?
On Mon, Aug 7, 2023 at 11:29 AM Julien Rouhaud wrote:
>
> On Mon, Aug 07, 2023 at 09:24:02AM +0530, Amit Kapila wrote:
> >
> > I think autovacuum is not enabled during the upgrade. See comment "Use
> > -b to disable autovacuum." in start_postmaster(). However, I am not
> > sure if there can't be a
On Mon, Aug 07, 2023 at 09:24:02AM +0530, Amit Kapila wrote:
>
> I think autovacuum is not enabled during the upgrade. See comment "Use
> -b to disable autovacuum." in start_postmaster(). However, I am not
> sure if there can't be any additional WAL from checkpointer or
> bgwriter. Checkpointer has
On Wed, Aug 2, 2023 at 7:46 AM Jonathan S. Katz wrote:
>
> Can I take this a step further on the user interface and ask why the
> flag would be "--include-logical-replication-slots" vs. being enabled by
> default?
>
> Are there reasons why we wouldn't enable this feature by default on
> pg_upgrade
On Sun, Aug 6, 2023 at 6:02 PM Masahiko Sawada wrote:
>
> On Wed, Aug 2, 2023 at 5:13 PM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > > 4.
> > > + /*
> > > + * Check that all logical replication slots have reached the current WAL
> > > + * position.
> > > + */
> > > + res = executeQueryOrDie(conn,
>
On Wed, Aug 2, 2023 at 5:13 PM Hayato Kuroda (Fujitsu)
wrote:
>
> > 4.
> > + /*
> > + * Check that all logical replication slots have reached the current WAL
> > + * position.
> > + */
> > + res = executeQueryOrDie(conn,
> > + "SELECT slot_name FROM pg_catalog.pg_replication_slots "
> > + "WHERE (
Dear Amit,
> So, we have three options here (a) As you have done in the patch,
> document this limitation and request user to perform some manual steps
> to drop the subscription; (b) don't allow upgrade to proceed if there
> are invalid slots in the old cluster; (c) provide a new function like
>
On Wed, Aug 2, 2023 at 1:43 PM Hayato Kuroda (Fujitsu)
wrote:
>
> > 3.
> > + /*
> > + * Get replication slots.
> > + *
> > + * XXX: Which information must be extracted from old node? Currently three
> > + * attributes are extracted because they are used by
> > + * pg_create_logical_replication_slo
On Wed, Aug 2, 2023 at 1:43 PM Hayato Kuroda (Fujitsu)
wrote:
>
> Thank you for giving comments! PSA new version patchset.
>
> > 1. Do we really need 0001 patch after the latest change proposed by
> > Vignesh in the 0004 patch?
>
> I removed 0001 patch and revived old patch which serializes slots
Dear Amit,
> I see your point related to WALAVAIL_REMOVED status of the slot but
> did you test the scenario I have explained in my comment? Basically, I
> want to know whether it can impact the user in some way. So, please
> check whether the corresponding subscriptions will be allowed to drop.
>
On Wed, Aug 2, 2023 at 1:43 PM Hayato Kuroda (Fujitsu)
wrote:
>
> Thank you for giving comments! PSA new version patchset.
>
> > 3.
> > + /*
> > + * Get replication slots.
> > + *
> > + * XXX: Which information must be extracted from old node? Currently three
> > + * attributes are extracted becau
Dear Amit,
Thank you for giving comments! PSA new version patchset.
> 1. Do we really need 0001 patch after the latest change proposed by
> Vignesh in the 0004 patch?
I removed 0001 patch and revived old patch which serializes slots at shutdown.
This is because the problem which slots are not se
Dear Vignesh,
Thank you for making the PoC!
> Here is a patch which checks that there are no WAL records other than
> CHECKPOINT_SHUTDOWN WAL record to be consumed based on the discussion
> from [1].
Basically I agreed your approach. Thanks!
> Patch 0001 and 0002 is same as the patch posted by
Dear Jonathan,
Thank you for reading the thread!
> Can I take this a step further on the user interface and ask why the
> flag would be "--include-logical-replication-slots" vs. being enabled by
> default?
>
> Are there reasons why we wouldn't enable this feature by default on
> pg_upgrade, and
On 8/1/23 5:39 AM, Amit Kapila wrote:
On Fri, Jul 28, 2023 at 5:48 PM vignesh C wrote:
Here is a patch which checks that there are no WAL records other than
CHECKPOINT_SHUTDOWN WAL record to be consumed based on the discussion
from [1].
Few comments:
=
2.
+ if (dopt.logical_s
On Fri, Jul 28, 2023 at 5:48 PM vignesh C wrote:
>
> Here is a patch which checks that there are no WAL records other than
> CHECKPOINT_SHUTDOWN WAL record to be consumed based on the discussion
> from [1].
>
Few comments:
=
1. Do we really need 0001 patch after the latest change prop
On Fri, 21 Jul 2023 at 13:00, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear hackers,
>
> > Based on the above, we are considering that we delay the timing of shutdown
> > for
> > logical walsenders. The preliminary workflow is:
> >
> > 1. When logical walsenders receives siginal from checkpointer, it c
Dear hackers,
> Based on the above, we are considering that we delay the timing of shutdown
> for
> logical walsenders. The preliminary workflow is:
>
> 1. When logical walsenders receives siginal from checkpointer, it consumes all
>of WAL records, change its state into WALSNDSTATE_STOPPING,
On Wed, Jul 19, 2023 at 7:33 PM Hayato Kuroda (Fujitsu)
wrote:
>
> > 2.
> > + /*
> > + * Dump logical replication slots if needed.
> > + *
> > + * XXX We cannot dump replication slots at the same time as the schema
> > + * dump because we need to separate the timing of restoring
> > + * replicatio
Dear Amit,
Thanks for reviewing! The patch could be available at [1].
> Few comments/questions
>
> 1.
> +check_for_parameter_settings(ClusterInfo *new_cluster)
> {
> ...
> +
> + res = executeQueryOrDie(conn, "SHOW max_replication_slots;");
> + max_replication_slots = atoi(PQg
Dear Peter,
Thank you for reviewing! PSA new version patchset.
> ==
> Commit message
>
> 1.
> For pg_dump this commit includes a new option called
> "--logical-replication-slots-only".
> This option can be used to dump logical replication slots. When this option is
> specified, the slot_name
Dear Amit,
> I have studied this a bit more and it seems that is true for physical
> walsenders where we set the state of walsender as WALSNDSTATE_STOPPING
> in XLogSendPhysical, then the checkpointer finishes writing checkpoint
> record and then postmaster sends SIGUSR2 for walsender to exit. IIU
On Mon, Jul 17, 2023 at 6:19 PM Amit Kapila wrote:
>
> On Fri, Jun 30, 2023 at 7:29 PM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > I have analyzed more, and concluded that there are no difference between
> > manual
> > and shutdown checkpoint.
> >
> > The difference was whether the CHECKPOINT recor
On Fri, Jun 30, 2023 at 7:29 PM Hayato Kuroda (Fujitsu)
wrote:
>
> I have analyzed more, and concluded that there are no difference between
> manual
> and shutdown checkpoint.
>
> The difference was whether the CHECKPOINT record has been decoded or not.
> The overall workflow of this test was:
>
Hi Kuroda-san, I haven't looked at this thread for a very long time so
to re-familiarize myself with it I read all the latest v16-0001 patch.
Here are a number of minor review comments I noted in passing:
==
Commit message
1.
For pg_dump this commit includes a new option called
"--logical-re
On Thu, Jun 8, 2023 at 9:24 AM Hayato Kuroda (Fujitsu)
wrote:
>
Few comments/questions
1.
+check_for_parameter_settings(ClusterInfo *new_cluster)
{
...
+
+ res = executeQueryOrDie(conn, "SHOW max_replication_slots;");
+ max_replication_slots = atoi(PQgetvalue(res, 0, 0));
+
+
Dear Amit,
Thank you for giving comments!
> > > Sorry for the delay, I didn't had time to come back to it until this
> > > afternoon.
> >
> > No issues, everyone is busy:-).
> >
> > > I don't think that your analysis is correct. Slots are guaranteed to be
> > > stopped after all the normal back
On Fri, Apr 14, 2023 at 4:00 PM Hayato Kuroda (Fujitsu)
wrote:
>
> > Sorry for the delay, I didn't had time to come back to it until this
> > afternoon.
>
> No issues, everyone is busy:-).
>
> > I don't think that your analysis is correct. Slots are guaranteed to be
> > stopped after all the nor
Dear Vignesh,
Thank you for reviewing! PSA new version patch set.
> Few minor comments:
> 1) we could remove the variable slotname from the below code by using
> PQgetvalue directly in pg_log:
> + for (i = 0; i < ntups; i++)
> + {
> + char *slotname;
> +
> +
Dear Vignesh,
Thank you for reviewing! New version will be attached the next post.
> Few comments
> 1) check_for_parameter_settings, check_for_confirmed_flush_lsn and
> check_are_logical_slots_active functions all have the same messages,
> we can keep it unique so that it is easy for user to inte
On Mon, 22 May 2023 at 15:50, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Wang,
>
> Thank you for reviewing! PSA new version.
>
> > For patches 0001
> >
> > 1. The latest patch set fails to apply because the new commit (0245f8d) in
> > HEAD.
>
> I didn't notice that. Thanks, fixed.
>
> > 2. In file p
On Mon, 22 May 2023 at 15:50, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Wang,
>
> Thank you for reviewing! PSA new version.
Thanks for the updated patch, Few comments:
Few comments
1) check_for_parameter_settings, check_for_confirmed_flush_lsn and
check_are_logical_slots_active functions all have t
Dear Wang,
Thank you for reviewing! PSA new version.
> For patches 0001
>
> 1. The latest patch set fails to apply because the new commit (0245f8d) in
> HEAD.
I didn't notice that. Thanks, fixed.
> 2. In file pg_dump.h.
> ```
> +/*
> + * The LogicalReplicationSlotInfo struct is used to repres
On Tues, May 16, 2023 at 14:15 PM Kuroda, Hayato/黒田 隼人
wrote:
> Dear Peter,
>
> Thanks for reviewing! PSA new version patchset.
Thanks for updating the patch set.
Here are some comments:
===
For patches 0001
1. The latest patch set fails to apply because the new commit (0245f8d) in HEAD.
~~~
Hi Kuroda-san,
I confirmed the patch changes from v13-0001 to v14-0001 have addressed
the comments from my previous post, and the cfbot is passing OK, so I
don't have any more review comments at this time.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Dear Peter,
Thanks for reviewing! PSA new version patchset.
> 1. get_logical_slot_infos_per_db
>
> I noticed that the way this is coded, 'ntups' and 'num_slots' seems to
> have exactly the same meaning. IMO you can simplify this by removing
> 'ntups'.
>
> BEFORE
> + int ntups;
> + int num_slots
Hi Kuroda-san.
I looked at the latest patch v13-0001. Here are some minor comments.
==
src/bin/pg_upgrade/info.c
1. get_logical_slot_infos_per_db
I noticed that the way this is coded, 'ntups' and 'num_slots' seems to
have exactly the same meaning. IMO you can simplify this by removing
'ntup
Dear Peter,
Thank you for reviewing! PSA new version.
> 1. check_new_cluster
>
> + if (user_opts.include_logical_slots)
> + {
> + get_logical_slot_infos(&new_cluster);
> + check_for_parameter_settings(&new_cluster);
> + }
> +
> check_new_cluster_is_empty();
> ~
>
> The code is OK, but maybe y
Hi Kuroda-san. Here are some comments for patch v12-0001.
==
src/bin/pg_upgrade/check.c
1. check_new_cluster
+ if (user_opts.include_logical_slots)
+ {
+ get_logical_slot_infos(&new_cluster);
+ check_for_parameter_settings(&new_cluster);
+ }
+
check_new_cluster_is_empty();
~
The code is O
Dear Peter,
Thanks for reviewing! New patch can be available at [1].
> 1. help
>
> printf(_(" --insertsdump data as INSERT
> commands, rather than COPY\n"));
> printf(_(" --load-via-partition-rootload partitions via the
> root table\n"));
> + printf(_(" --logical-r
Dear Wang,
Thank you for reviewing! PSA new version.
> 1. In the function getLogicalReplicationSlots
> ```
> + /*
> + * Note: Currently we do not have any options to include/exclude
> slots
> + * in dumping, so all the slots must be selected.
> +
Dear Wang,
> I'm not sure if there is any reason to not expose this new option? Do we have
> concerns that users who use this new option by mistake may cause data
> inconsistencies?
>
> BTW, I think that all options of pg_dump (please see the array of long_options
> in the main function of the pg_
On Tue, May 9, 2023 at 17:44 PM Hayato Kuroda (Fujitsu)
wrote:
> Thank you for reviewing! PSA new version.
Thanks for your patches.
Here are some comments for 0001 patch:
1. In the function getLogicalReplicationSlots
```
+ /*
+* Note: Currently we do not have any o
On Thu, May 11, 2023 at 10:12 AM Peter Smith wrote:
> Hi Kuroda-san. I checked again the v11-0001.
>
> Here are a few more review comments.
>
> ==
> src/bin/pg_dump/pg_dump.c
>
> 1. help
>
> printf(_(" --insertsdump data as INSERT
> commands, rather than COPY\n"));
>
Hi Kuroda-san. I checked again the v11-0001.
Here are a few more review comments.
==
src/bin/pg_dump/pg_dump.c
1. help
printf(_(" --insertsdump data as INSERT
commands, rather than COPY\n"));
printf(_(" --load-via-partition-rootload partitions via the
root tabl
Dear Peter,
Thank you for reviewing! PSA new version.
>
> General.
>
> 1. pg_dump option is documented to the user.
>
> I'm not sure about exposing the new pg_dump
> --logical-replication-slots-only option to the user.
>
> I thought this pg_dump option was intended only to be called
> *intern
Hi Kuroda-san. Here are some review comments for the v10-0001 patch.
==
General.
1. pg_dump option is documented to the user.
I'm not sure about exposing the new pg_dump
--logical-replication-slots-only option to the user.
I thought this pg_dump option was intended only to be called
*inter
Dear Alvaro,
Thanks for giving suggestion!
> A point on preserving physical replication slots: because we change WAL
> format from one major version to the next (adding new messages or
> changing format for other messages), we can't currently rely on physical
> slots working across different majo
On 2023-May-02, Julien Rouhaud wrote:
> On Tue, May 02, 2023 at 12:55:18PM +0200, Alvaro Herrera wrote:
> > A point on preserving physical replication slots: because we change WAL
> > format from one major version to the next (adding new messages or
> > changing format for other messages), we can
On Tue, 2 May 2023, 19:43 Julien Rouhaud, wrote:
> Hi,
>
> On Tue, May 02, 2023 at 12:55:18PM +0200, Alvaro Herrera wrote:
> > On 2023-Apr-07, Julien Rouhaud wrote:
> >
> > > That being said, I have a hard time believing that we could actually
> preserve
> > > physical replication slots. I don't
Hi,
On Tue, May 02, 2023 at 12:55:18PM +0200, Alvaro Herrera wrote:
> On 2023-Apr-07, Julien Rouhaud wrote:
>
> > That being said, I have a hard time believing that we could actually
> > preserve
> > physical replication slots. I don't think that pg_upgrade final state is
> > fully
> > reproduc
On 2023-Apr-07, Julien Rouhaud wrote:
> That being said, I have a hard time believing that we could actually preserve
> physical replication slots. I don't think that pg_upgrade final state is
> fully
> reproducible: not all object oids are preserved, and the various pg_restore
> are run in par
Dear Peter,
> A suggestion: You could write some/most tests against test_decoding
> rather than the publication/subscription system. That way, you can
> avoid many timing issues in the tests and you can check more exactly
> that the slots produce the output you want. This would also help ensure
On 24.04.23 14:03, Hayato Kuroda (Fujitsu) wrote:
so at least there's a good chance that they will still be at
shutdown, and will therefore send all the data to the subscribers? Having a
regression tests for that scenario would also be a good idea. Having an
uncommitted write transaction should
Dear Hackers,
> Thank you for giving comments! PSA new version.
Note that due to the current version could not work well on FreeBSD, maybe
because of the timing issue[1]. I'm now analyzing the reason and will post
the fixed version.
[1]: https://cirrus-ci.com/build/4676441267240960
Best Regards
Hi,
On Mon, Apr 24, 2023 at 12:03:05PM +, Hayato Kuroda (Fujitsu) wrote:
>
> > I think that this test should be different when just checking for the
> > prerequirements (live_check / --check) compared to actually doing the
> > upgrade,
> > as it's almost guaranteed that the slots won't have s
Dear Vignesh,
Thank you for giving comments. New patchset can be available in [1].
> Thanks for the updated patch.
> A Few comments:
> 1) if the verbose option is enabled, we should print the new slot
> information, we could add a function print_slot_infos similar to
> print_rel_infos which could
Dear Julien,
Thank you for giving comments! PSA new version.
> I think that this test should be different when just checking for the
> prerequirements (live_check / --check) compared to actually doing the upgrade,
> as it's almost guaranteed that the slots won't have sent everything when the
> so
On Thu, 20 Apr 2023 at 11:01, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Vignesh,
>
> Thank you for reviewing! PSA new patchset.
>
> > > Additionally, I added a checking functions in 0003
> > > According to pg_resetwal and other functions, the length of
> > CHECKPOINT_SHUTDOWN
> > > record seems (Siz
Hi,
On Thu, Apr 20, 2023 at 05:31:16AM +, Hayato Kuroda (Fujitsu) wrote:
> Dear Vignesh,
>
> Thank you for reviewing! PSA new patchset.
>
> > > Additionally, I added a checking functions in 0003
> > > According to pg_resetwal and other functions, the length of
> > CHECKPOINT_SHUTDOWN
> > > rec
Dear Vignesh,
Thank you for reviewing! PSA new patchset.
> > Additionally, I added a checking functions in 0003
> > According to pg_resetwal and other functions, the length of
> CHECKPOINT_SHUTDOWN
> > record seems (SizeOfXLogRecord + SizeOfXLogRecordDataHeaderShort +
> sizeof(CheckPoint)).
> > T
Dear Vignesh,
Thanks for giving a comment! New patch will be available soon.
> Thanks for the patches.
> Currently the two_phase enabled slots are not getting restored from
> the dumped contents, this is because we are passing the twophase value
> as the second parameter which indicates if it is
On Fri, 14 Apr 2023 at 16:00, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Julien,
>
> > Sorry for the delay, I didn't had time to come back to it until this
> > afternoon.
>
> No issues, everyone is busy:-).
>
> > I don't think that your analysis is correct. Slots are guaranteed to be
> > stopped af
On Fri, 14 Apr 2023 at 16:00, Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Julien,
>
> > Sorry for the delay, I didn't had time to come back to it until this
> > afternoon.
>
> No issues, everyone is busy:-).
>
> > I don't think that your analysis is correct. Slots are guaranteed to be
> > stopped af
Dear Julien,
> Sorry for the delay, I didn't had time to come back to it until this
> afternoon.
No issues, everyone is busy:-).
> I don't think that your analysis is correct. Slots are guaranteed to be
> stopped after all the normal backends have been stopped, exactly to avoid such
> extraneo
Hi,
Sorry for the delay, I didn't had time to come back to it until this afternoon.
On Mon, Apr 10, 2023 at 09:18:46AM +, Hayato Kuroda (Fujitsu) wrote:
>
> I have analyzed about the point but it seemed to be difficult. This is because
> some additional records like followings may be inserted
Dear Peter,
Thank you for checking. Then we can wait comments from others.
PSA modified version.
> 1.
> There were a couple of comments that I thought would appear less
> squished (aka more readable) if there was a blank line preceding the
> XXX.
>
> 1a. This one is in getLogicalReplicationSlots
Hi Kuroda-san.
I do not have any more review comments for the v5 patch, but here are
a few remaining nitpick items.
==
General
1.
There were a couple of comments that I thought would appear less
squished (aka more readable) if there was a blank line preceding the
XXX.
1a. This one is in get
Dear Peter,
Thank you for giving comments. PSA new version.
> src/bin/pg_dump/pg_backup.h
>
> 1.
> + int logical_slot_only;
>
> The field should be plural - "logical_slots_only"
Fixed.
> src/bin/pg_dump/pg_dump.c
>
> 2.
> + appendPQExpBufferStr(query,
> + "SELECT r.slot_name, r.plugin, r.two
FYI, here are some minor review comments for v4-0001
==
src/bin/pg_dump/pg_backup.h
1.
+ int logical_slot_only;
The field should be plural - "logical_slots_only"
==
src/bin/pg_dump/pg_dump.c
2.
+ appendPQExpBufferStr(query,
+ "SELECT r.slot_name, r.plugin, r.two_phase "
+ "FROM pg_repl
Dear hackers,
My PoC does not read and copy logical mappings files to new node, but I
did not analyzed in detail whether it is correct. Now I have done this and
considered that they do not have to be copied because transactions which
executed
at the same time as rewriting are no longer decoded. H
Dear Peter,
Thank you for giving explanation.
>
> Hopefully, someone will correct me if this explanation is wrong, but
> my understanding of the different prefixes is like this --
>
> "XXX" is used as a marker for future developers to consider maybe
> revisiting/improving something that the com
Dear Peter,
Thank you for giving comments! PSA new version.
> ==
> doc/src/sgml/ref/pgupgrade.sgml
>
> 1.
> +
> + --include-logical-replication-slots
> +
> +
> +Upgrade logical replication slots. Only permanent replication slots
> +included. Note that p
On Sat, Apr 8, 2023 at 12:00 AM Hayato Kuroda (Fujitsu)
wrote:
>
...
> > 17. main
> >
> > + /*
> > + * Create replication slots if requested.
> > + *
> > + * XXX This must be done after doing pg_resetwal command because the
> > + * command will remove required WALs.
> > + */
> > + if (user_opts.in
Here are a few more review comments for patch v3-0001.
==
doc/src/sgml/ref/pgupgrade.sgml
1.
+
+ --include-logical-replication-slots
+
+
+Upgrade logical replication slots. Only permanent replication slots
+included. Note that pg_upgrade does not check t
Dear Julien,
Thank you for giving idea! I have analyzed about it.
> > > If
> > > yes, how does this work if some subscriber node isn't connected when the
> > > publisher node is stopped? I guess you could add a check in pg_upgrade to
> make
> > > sure that all logical slot are indeed caught up a
Dear Julien,
> Well, even if physical replication slots were eventually preserved during
> pg_upgrade, maybe users would like to only keep one kind of the others so
> having both options could make sense.
You meant to say that we can rename options like "logical-*" and later add a new
option for
On Fri, Apr 07, 2023 at 12:51:51PM +, Hayato Kuroda (Fujitsu) wrote:
> Dear Julien,
>
> > > Agreed, but then shouldn't the option be named "--logical-slots-only" or
> > > something like that, same for all internal function names?
> >
> > Seems right. Will be fixed in next version. Maybe
> > "
On Fri, Apr 07, 2023 at 09:40:14AM +, Hayato Kuroda (Fujitsu) wrote:
>
> > As I mentioned in my original thread, I'm not very familiar with that code,
> > but
> > I'm a bit worried about "all the changes generated on publisher must be send
> > and applied". Is that a hard requirement for the
Dear Peter,
Thank you for reviewing briefly. PSA new version.
If you can I want to ask the opinion about the checking by pg_upgrade [1].
> ==
> General
>
> 1.
> Since these two new options are made to work together, I think the
> names should be more similar. e.g.
>
> pg_dump: "--slot_only"
Dear Julien,
> > Agreed, but then shouldn't the option be named "--logical-slots-only" or
> > something like that, same for all internal function names?
>
> Seems right. Will be fixed in next version. Maybe
> "--logical-replication-slots-only"
> will be used, per Peter's suggestion [1].
After co
Dear Julien,
Thank you for giving comments!
> As I mentioned in my original thread, I'm not very familiar with that code,
> but
> I'm a bit worried about "all the changes generated on publisher must be send
> and applied". Is that a hard requirement for the feature to work reliably?
I think th
301 - 400 of 403 matches
Mail list logo
