On Thu, Oct 5, 2023 at 1:48 AM Amit Kapila wrote:
>
> Then, we should also try to create slots before invoking pg_resetwal.
> The idea is that we can write a new binary mode function that will do
> exactly what pg_resetwal does to compute the next segment and use that
> location as a new location
On Thu, Oct 5, 2023 at 4:24 PM Amit Kapila wrote:
>
> > > Today, I discussed this problem with Andres at PGConf NYC and he
> > > suggested as following. To verify, if there is any pending unexpected
> > > WAL after shutdown, we can have an API like
> > > pg_logical_replication_slot_advance() which
On Thu, Oct 5, 2023 at 2:29 PM Dilip Kumar wrote:
>
> On Thu, Oct 5, 2023 at 1:48 AM Amit Kapila
wrote:
> >
> > On Tue, Oct 3, 2023 at 9:58 AM Bharath Rupireddy
> > wrote:
> > >
> > > On Fri, Sep 29, 2023 at 5:27 PM Hayato Kuroda (Fujitsu)
> > > wrote:
> > > >
> > > > Yeah, the approach enforce
Dear Amit, Andres,
Thank you for giving the decision! Basically I will follow your idea and make
a patch accordingly.
> Today, I discussed this problem with Andres at PGConf NYC and he
> suggested as following. To verify, if there is any pending unexpected
> WAL after shutdown, we can have an API
On Thu, Oct 5, 2023 at 1:48 AM Amit Kapila wrote:
>
> On Tue, Oct 3, 2023 at 9:58 AM Bharath Rupireddy
> wrote:
> >
> > On Fri, Sep 29, 2023 at 5:27 PM Hayato Kuroda (Fujitsu)
> > wrote:
> > >
> > > Yeah, the approach enforces developers to check the decodability.
> > > But the benefit seems sma
On Tue, Oct 3, 2023 at 9:58 AM Bharath Rupireddy
wrote:
>
> On Fri, Sep 29, 2023 at 5:27 PM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > Yeah, the approach enforces developers to check the decodability.
> > But the benefit seems smaller than required efforts for it because the
> > function
> > would
Dear Bharath,
While checking more, I found some problems your PoC.
1. rm_is_record_decodable() returns true when WAL records are decodable.
Based on that, should is_valid be false when the function is true?
E.g., XLOG_HEAP_INSERT is accepted in the PoC.
2. XLOG_CHECKPOINT_SHUTDOWN and XLOG_
On Tue, Oct 3, 2023 at 9:58 AM Bharath Rupireddy
wrote:
>
> On Fri, Sep 29, 2023 at 5:27 PM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > Yeah, the approach enforces developers to check the decodability.
> > But the benefit seems smaller than required efforts for it because the
> > function
> > would
Dear Bharath,
> I'm attaching 0002 patch (on top of v45) which implements the new
> decodable callback approach that I have in mind. IMO, this new
> approach is extensible, better than the current approach (hard-coding
> of certain WAL records that may be generated during pg_upgrade) taken
> by th
On Tue, Oct 3, 2023 at 9:58 AM Bharath Rupireddy
wrote:
>
> On Fri, Sep 29, 2023 at 5:27 PM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > Yeah, the approach enforces developers to check the decodability.
> > But the benefit seems smaller than required efforts for it because the
> > function
> > would
On Fri, Sep 29, 2023 at 5:27 PM Hayato Kuroda (Fujitsu)
wrote:
>
> Yeah, the approach enforces developers to check the decodability.
> But the benefit seems smaller than required efforts for it because the
> function
> would be used only by pg_upgrade. Could you tell me if you have another use
>
Dear Bharath,
Thanks for giving your idea!
> > I think this approach can work, but I am not sure if it's better than other
> > approaches. Mainly because it has almost the same maintaince burden as the
> > current approach, i.e. we need to verify and update the check function each
> > time we add
On Fri, Sep 29, 2023 at 1:00 PM Bharath Rupireddy
wrote:
>
> On Thu, Sep 28, 2023 at 6:08 PM Zhijie Hou (Fujitsu)
> wrote:
>
> IMO, WAL scanning approach looks better. However, if were to optimize
> it by not scanning WAL records for every replication slot
> confirmed_flush_lsn (CFL), start with
On Thu, Sep 28, 2023 at 6:08 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Thursday, September 28, 2023 5:32 PM Bharath Rupireddy
> wrote:
>
> > Perhaps, a function in logical/decode.c returning the WAL record as valid
> > if the
> > record type is any of the above. A note in replication/decode.h and/o
On Thursday, September 28, 2023 5:32 PM Bharath Rupireddy
wrote:
Hi,
>
> On Mon, Sep 25, 2023 at 4:31 PM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > > 4.
> > > +/*
> > > + * There is a possibility that following records may be generated
> > > + * during the upgrade.
> > >
On Mon, Sep 25, 2023 at 4:31 PM Hayato Kuroda (Fujitsu)
wrote:
>
> > 4.
> > +/*
> > + * There is a possibility that following records may be generated
> > + * during the upgrade.
> > + */
> > +is_valid = is_xlog_record_type(rmid, info, RM_XLOG_ID,
> > XLOG_C
On Thu, Sep 28, 2023 at 2:22 PM Bharath Rupireddy
wrote:
>
> On Fri, Sep 22, 2023 at 9:40 AM Michael Paquier wrote:
> >
> > On Thu, Sep 21, 2023 at 01:50:28PM +0530, Amit Kapila wrote:
> > > We have discussed this point. Normally, we don't have such options in
> > > upgrade, so we were hesitent t
On Thu, Sep 28, 2023 at 1:24 PM Bharath Rupireddy
wrote:
>
> On Thu, Sep 28, 2023 at 1:06 PM Amit Kapila wrote:
> >
> > On Thu, Sep 28, 2023 at 10:44 AM Bharath Rupireddy
> > wrote:
> > >
> > > > No, without that commit, there is a very high possibility that even if
> > > > we have sent the WAL
On Fri, Sep 22, 2023 at 9:40 AM Michael Paquier wrote:
>
> On Thu, Sep 21, 2023 at 01:50:28PM +0530, Amit Kapila wrote:
> > We have discussed this point. Normally, we don't have such options in
> > upgrade, so we were hesitent to add a new one for this but there is a
> > discussion to add an --exc
On Thu, Sep 28, 2023 at 1:06 PM Amit Kapila wrote:
>
> On Thu, Sep 28, 2023 at 10:44 AM Bharath Rupireddy
> wrote:
> >
> > > No, without that commit, there is a very high possibility that even if
> > > we have sent the WAL to the subscriber and got the acknowledgment of
> > > the same, we would m
On Thu, Sep 28, 2023 at 10:44 AM Bharath Rupireddy
wrote:
>
> On Mon, Sep 25, 2023 at 2:06 PM Amit Kapila wrote:
> >
> > > > [1]
> > > > https://www.postgresql.org/message-id/CAA4eK1%2BLtWDKXvxS7gnJ562VX%2Bs3C6%2B0uQWamqu%3DUuD8hMfORg%40mail.gmail.com
> > >
> > > I see. IIUC, without that commit
On Mon, Sep 25, 2023 at 2:06 PM Amit Kapila wrote:
>
> > > [1]
> > > https://www.postgresql.org/message-id/CAA4eK1%2BLtWDKXvxS7gnJ562VX%2Bs3C6%2B0uQWamqu%3DUuD8hMfORg%40mail.gmail.com
> >
> > I see. IIUC, without that commit e0b2eed [1], it may happen that the
> > slot's on-disk confirmed_flush L
Dear Bharath,
Thank you for reviewing!
> Thanks for the new patch. Here's a comment on v46:
>
> 1.
> +Datum
> +binary_upgrade_validate_wal_logical_end(PG_FUNCTION_ARGS
> +{ oid => '8046', descr => 'for use by pg_upgrade',
> + proname => 'binary_upgrade_validate_wal_logical_end', proisstrict =>
On Tue, Sep 26, 2023 at 10:51 AM Hayato Kuroda (Fujitsu)
wrote:
>
> Again, thank you for reviewing! PSA a new version.
Thanks for the new patch. Here's a comment on v46:
1.
+Datum
+binary_upgrade_validate_wal_logical_end(PG_FUNCTION_ARGS
+{ oid => '8046', descr => 'for use by pg_upgrade',
+ pro
Dear Bharath,
Again, thank you for reviewing! PSA a new version.
>
> Here are some more comments/thoughts on the v44 patch:
>
> 1.
> +# pg_upgrade will fail because the slot still has unconsumed WAL records
> +command_fails(
> +[
>
> Add a test case to hit fprintf(script, "The slot \"%s\"
On Monday, September 25, 2023 7:01 PM Kuroda, Hayato/黒田 隼人
wrote:
> To: 'Bharath Rupireddy'
> Cc: Amit Kapila ; Dilip Kumar
> >
> > 5. In continuation to the above comment:
> >
> > Why can't this logic be something like - if there's any WAL record
> > seen after a slot's confirmed flush LSN is o
On Mon, Sep 25, 2023 at 1:23 PM Bharath Rupireddy
wrote:
>
> On Mon, Sep 25, 2023 at 12:32 PM Dilip Kumar wrote:
> >
> > > > Is there anything else that stops this patch from supporting migration
> > > > of logical replication slots from PG versions < 17?
> > >
> > > IMHO one of the main change w
Dear Bharath,
Thank you for giving comments! Before addressing your comments,
I wanted to reply some of them.
> 4.
> +/*
> + * There is a possibility that following records may be generated
> + * during the upgrade.
> + */
> +is_valid = is_xlog_record_type(
On Mon, Sep 25, 2023 at 1:23 PM Bharath Rupireddy
wrote:
>
> On Mon, Sep 25, 2023 at 12:32 PM Dilip Kumar wrote:
> >
> > > > Is there anything else that stops this patch from supporting migration
> > > > of logical replication slots from PG versions < 17?
> > >
> > > IMHO one of the main change w
On Mon, Sep 25, 2023 at 12:32 PM Dilip Kumar wrote:
>
> > > Is there anything else that stops this patch from supporting migration
> > > of logical replication slots from PG versions < 17?
> >
> > IMHO one of the main change we are doing in PG 17 is that on shutdown
> > checkpoint we are ensuring
On Sat, Sep 23, 2023 at 10:18 AM Hayato Kuroda (Fujitsu)
wrote:
>
> Again, thank you for reviewing! Here is a new version patch.
Here are some more comments/thoughts on the v44 patch:
1.
+# pg_upgrade will fail because the slot still has unconsumed WAL records
+command_fails(
+[
Add a test
On Mon, Sep 25, 2023 at 12:30 PM Dilip Kumar wrote:
>
> On Mon, Sep 25, 2023 at 11:15 AM Bharath Rupireddy
> wrote:
> >
> > On Fri, Sep 22, 2023 at 12:11 PM Amit Kapila
> > wrote:
> > >
> > > Yeah, both by tests and manually verifying the WAL records. Basically,
> > > we need to care about reco
On Mon, Sep 25, 2023 at 11:15 AM Bharath Rupireddy
wrote:
>
> On Fri, Sep 22, 2023 at 12:11 PM Amit Kapila wrote:
> >
> > Yeah, both by tests and manually verifying the WAL records. Basically,
> > we need to care about records that could be generated by background
> > processes like checkpointer/
On Fri, Sep 22, 2023 at 12:11 PM Amit Kapila wrote:
>
> Yeah, both by tests and manually verifying the WAL records. Basically,
> we need to care about records that could be generated by background
> processes like checkpointer/bgwriter or can be generated during system
> table scans. You may want
Dear Bharath,
> > You mentioned at line 118, but at that time logical replication system is
> > not
> created.
> > The subscriber is created at line 163.
> > Therefore WALs would not be consumed automatically.
>
> So, not calling pg_logical_slot_get_changes() on test_slot1 won't
> consume the WA
Dear Bharath,
Again, thank you for reviewing! Here is a new version patch.
> 1.
> +{ oid => '8046', descr => 'for use by pg_upgrade',
> + proname => 'binary_upgrade_validate_wal_records', proisstrict => 'f',
> + provolatile => 'v', proparallel => 'u', prorettype => 'bool',
>
> +if (PG_ARGI
On Fri, Sep 22, 2023 at 10:57 AM Bharath Rupireddy
wrote:
>
> On Thu, Sep 21, 2023 at 5:45 PM Amit Kapila wrote:
> >
> > > Thanks for the patch. I have some comments on v42:
> >
> > Probably trying to keep it similar with
> > binary_upgrade_create_empty_extension(). I think it depends what
> > be
On Fri, Sep 22, 2023 at 11:59 AM Bharath Rupireddy
wrote:
>
> On Thu, Sep 21, 2023 at 6:54 PM Hayato Kuroda (Fujitsu)
> wrote:
> >
>
> 1.
> +/*
> + * Use max_slot_wal_keep_size as -1 to prevent the WAL removal by the
> + * checkpointer process. If WALs required by logical replication
On Fri, Sep 22, 2023 at 10:57 AM Bharath Rupireddy
wrote:
>
> On Thu, Sep 21, 2023 at 5:45 PM Amit Kapila wrote:
> > > 3.
> > > +/* Quick exit if the given lsn is larger than current one */
> > > +if (start_lsn >= GetFlushRecPtr(NULL))
> > > +PG_RETURN_BOOL(false);
> > > +
> > >
>
On Thu, Sep 21, 2023 at 6:54 PM Hayato Kuroda (Fujitsu)
wrote:
>
> > 6.
> > +if (PQntuples(res) != 1)
> > +pg_fatal("could not count the number of logical replication
> > slots");
> > +
> >
> > Not existing a single logical replication slot an error? I think it
> > must be if (PQntupl
On Thu, Sep 21, 2023 at 5:45 PM Amit Kapila wrote:
>
> > Thanks for the patch. I have some comments on v42:
>
> Probably trying to keep it similar with
> binary_upgrade_create_empty_extension(). I think it depends what
> behaviour we expect for NULL input.
confirmed_flush_lsn for a logical slot c
On Thu, Sep 21, 2023 at 01:50:28PM +0530, Amit Kapila wrote:
> We have discussed this point. Normally, we don't have such options in
> upgrade, so we were hesitent to add a new one for this but there is a
> discussion to add an --exclude-logical-slots option. We are planning
> to add that as a sepa
Dear Bharath,
Thank you for reviewing! Before addressing them, I would like to reply some
comments.
> 6.
> +if (PQntuples(res) != 1)
> +pg_fatal("could not count the number of logical replication slots");
> +
>
> Not existing a single logical replication slot an error? I think it
>
On Thu, Sep 21, 2023 at 4:57 PM Bharath Rupireddy
wrote:
>
> On Wed, Sep 20, 2023 at 7:20 PM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > Good catch, I could not notice because it worked well in my RHEL. Here is
> > the
> > updated version.
>
> Thanks for the patch. I have some comments on v42:
>
>
On Wed, Sep 20, 2023 at 7:20 PM Hayato Kuroda (Fujitsu)
wrote:
>
> Good catch, I could not notice because it worked well in my RHEL. Here is the
> updated version.
Thanks for the patch. I have some comments on v42:
1.
+{ oid => '8046', descr => 'for use by pg_upgrade',
+ proname => 'binary_upgr
Dear Hackers,
> Good catch, I could not notice because it worked well in my RHEL. Here is the
> updated version.
I did some cosmetic changes for the patch, the functionality was not changed.
E.g., a macro function was replaced to an inline.
Note that cfbot got angry to old patch, but it seemed t
On Thu, Sep 21, 2023 at 1:10 PM Michael Paquier wrote:
>
> On Wed, Sep 20, 2023 at 11:28:33AM +, Hayato Kuroda (Fujitsu) wrote:
> > Good catch, I could not notice because it worked well in my RHEL. Here is
> > the
> > updated version.
>
> I am getting slowly up to date with this patch.. But
On Wed, Sep 20, 2023 at 11:28:33AM +, Hayato Kuroda (Fujitsu) wrote:
> Good catch, I could not notice because it worked well in my RHEL. Here is the
> updated version.
I am getting slowly up to date with this patch.. But before going in
depth with more review, there is something that I got to
Dear Amit,
Thank you for reviewing! New version can be available in [1].
>
> +{ oid => '8046', descr => 'for use by pg_upgrade',
> + proname => 'binary_upgrade_validate_wal_records',
> + prorows => '10', proretset => 't', provolatile => 's', prorettype =>
> 'bool',
> + proargtypes => 'pg_lsn
Dear Amit,
> +int
> +count_old_cluster_logical_slots(void)
> +{
> + int dbnum;
> + int slot_count = 0;
> +
> + for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
> + slot_count += old_cluster.dbarr.dbs[dbnum].slot_arr.nslots;
> +
> + return slot_count;
> +}
>
> In this code, aren't we assum
On Wed, Sep 20, 2023 at 12:16 PM Amit Kapila wrote:
>
> On Wed, Sep 20, 2023 at 11:00 AM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > Dear Amit,
>
> +int
> +count_old_cluster_logical_slots(void)
> +{
> + int dbnum;
> + int slot_count = 0;
> +
> + for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum+
On Wed, Sep 20, 2023 at 12:12 PM Amit Kapila wrote:
>
> On Wed, Sep 20, 2023 at 11:51 AM Dilip Kumar wrote:
> >
> > On Wed, Sep 20, 2023 at 11:00 AM Hayato Kuroda (Fujitsu)
> > wrote:
> > >
> > > Dear Amit,
> > >
> > > Thank you for reviewing! PSA new version. In this version I ran pgindent
> >
On Wed, Sep 20, 2023 at 11:00 AM Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Amit,
+int
+count_old_cluster_logical_slots(void)
+{
+ int dbnum;
+ int slot_count = 0;
+
+ for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
+ slot_count += old_cluster.dbarr.dbs[dbnum].slot_arr.nslots;
+
+ return sl
On Wed, Sep 20, 2023 at 11:51 AM Dilip Kumar wrote:
>
> On Wed, Sep 20, 2023 at 11:00 AM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > Dear Amit,
> >
> > Thank you for reviewing! PSA new version. In this version I ran pgindent
> > again.
> >
>
> + /*
> + * There is a possibility that following record
On Wed, Sep 20, 2023 at 11:00 AM Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Amit,
>
> Thank you for reviewing! PSA new version. In this version I ran pgindent
> again.
>
+ /*
+ * There is a possibility that following records may be generated
+ * during the upgrade.
+ */
+ if (!CHECK_WAL_RECORD(rmid
Dear Amit,
Thank you for reviewing! PSA new version. In this version I ran pgindent again.
> +#include "access/xlogdefs.h"
> #include "common/relpath.h"
> #include "libpq-fe.h"
>
> The above include is not required. I have removed that and made a few
> cosmetic changes in the attached.
Yes, i
On Tue, Sep 19, 2023 at 11:47 AM Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Amit,
>
> Thank you for reviewing! PSA new version!
>
*
+#include "access/xlogdefs.h"
#include "common/relpath.h"
#include "libpq-fe.h"
The above include is not required. I have removed that and made a few
cosmetic change
Dear Amit,
Thank you for reviewing! PSA new version!
> > Sorry, wrong patch attached. PSA the correct ones.
> > There is a possibility that XLOG_PARAMETER_CHANGE may be generated,
> when GUC
> > parameters are changed just before doing the upgrade. Added to list.
> >
>
> You forgot to update 000
On Fri, Sep 15, 2023 at 6:32 PM Hayato Kuroda (Fujitsu)
wrote:
>
> > Thank you for reviewing! PSA new version patch set.
>
> Sorry, wrong patch attached. PSA the correct ones.
> There is a possibility that XLOG_PARAMETER_CHANGE may be generated, when GUC
> parameters are changed just before doing
On Friday, September 15, 2023 9:02 PM Kuroda, Hayato/黒田 隼人
wrote:
>
> Sorry, wrong patch attached. PSA the correct ones.
> There is a possibility that XLOG_PARAMETER_CHANGE may be generated,
> when GUC parameters are changed just before doing the upgrade. Added to
> list.
I did some simple perf
On Friday, September 15, 2023 8:33 PM Kuroda, Hayato/黒田 隼人
wrote:
>
>
> Also, I did a self-reviewing again and reworded comments.
>
> BTW, the 0002 ports some functions from pg_walinspect, it may be not
> elegant.
> Coupling degree between core/extensions should be also lower. So I made
> anot
> Thank you for reviewing! PSA new version patch set.
Sorry, wrong patch attached. PSA the correct ones.
There is a possibility that XLOG_PARAMETER_CHANGE may be generated, when GUC
parameters are changed just before doing the upgrade. Added to list.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
Dear Amit,
Thank you for reviewing! PSA new version patch set.
> Few comments:
> 1. Why is the FPI record (XLOG_FPI_FOR_HINT) not considered a record
> to be ignored? This can be generated during reading system tables.
Oh, I just missed. Written in comments atop the function, but not added here.
On Fri, Sep 15, 2023 at 8:43 AM Hayato Kuroda (Fujitsu)
wrote:
>
Few comments:
1. Why is the FPI record (XLOG_FPI_FOR_HINT) not considered a record
to be ignored? This can be generated during reading system tables.
2.
+binary_upgrade_validate_wal_record_types_after_lsn(PG_FUNCTION_ARGS)
{
...
+
Dear Peter,
Thank you for reviewing! New patch is available in [1].
> 1.
> Configure the servers for log shipping. (You do not need to run
> pg_backup_start() and
> pg_backup_stop()
> or take a file system backup as the standbys are still synchronized
> - with the p
Dear Amit,
Again, thank you for reviewing! New patch is available in [1].
> 2.
> + /*
> + * Store the names of output plugins as well. There is a possibility
> + * that duplicated plugins are set, but the consumer function
> + * check_loadable_libraries() will avoid checking the same library, so
Dear hackers,
> > So basically, while scanning from confirmed_flush we must ensure that
> > we find a first record as SHUTDOWN CHECKPOINT record at the same LSN,
> > and after that, we should not get any other WAL other than like you
> > said shutdown checkpoint, running_xacts. That way we will e
On Thu, Sep 14, 2023 at 10:37 AM Dilip Kumar wrote:
>
> On Thu, Sep 14, 2023 at 10:00 AM Amit Kapila wrote:
> >
> > On Thu, Sep 14, 2023 at 9:21 AM Dilip Kumar wrote:
>
> > > > ---
> > > >
> > > > 3) Introduce a new pg_upgrade option(e.g. skip_slot_check), and suggest
> > > > if user
>
On Thu, Sep 14, 2023 at 10:00 AM Amit Kapila wrote:
>
> On Thu, Sep 14, 2023 at 9:21 AM Dilip Kumar wrote:
> > > Cons: Although we have some examples for using functions
> > > (binary_upgrade_set_next_pg_enum_oid ...) to set some variables during
> > > upgrade
> > > , but not sure if it's a sta
On Thu, Sep 14, 2023 at 9:21 AM Dilip Kumar wrote:
>
> On Thu, Sep 14, 2023 at 8:40 AM Zhijie Hou (Fujitsu)
> wrote:
>
> >
> > Here are some more ideas about the issue for reference.
> >
> > 1) Extending the controlfile.
> >
> > We can dd a new field (e.g. non_upgrade_checkPoint) to record the la
On Thu, Sep 14, 2023 at 8:40 AM Zhijie Hou (Fujitsu)
wrote:
>
> Here are some more ideas about the issue for reference.
>
> 1) Extending the controlfile.
>
> We can dd a new field (e.g. non_upgrade_checkPoint) to record the last check
> point
> ptr happened in non-upgrade mode. The new field won
On Wed, Sep 13, 2023 at 7:22 PM Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Amit,
>
> Thank you for reviewing! Before making a patch I can reply the important
> point.
>
> > 1. One thing to note is that if user checks whether the old cluster is
> > upgradable with --check option and then try to upgra
On Wednesday, September 13, 2023 9:52 PM Kuroda, Hayato/黒田 隼人
wrote:
>
> Dear Amit,
>
> Thank you for reviewing! Before making a patch I can reply the important
> point.
>
> > 1. One thing to note is that if user checks whether the old cluster is
> > upgradable with --check option and then tr
Dear Amit,
Thank you for reviewing! Before making a patch I can reply the important point.
> 1. One thing to note is that if user checks whether the old cluster is
> upgradable with --check option and then try to upgrade, that will also
> fail. Because during the --check run there would at least
On Tue, Sep 12, 2023 at 5:20 PM Hayato Kuroda (Fujitsu)
wrote:
>
Few comments:
=
1. One thing to note is that if user checks whether the old cluster is
upgradable with --check option and then try to upgrade, that will also
fail. Because during the --check run there would at least one
Hi Kuroda-san. Here are my review comments for patch v36-0002.
==
doc/src/sgml/ref/pgupgrade.sgml
1.
Configure the servers for log shipping. (You do not need to run
pg_backup_start() and
pg_backup_stop()
or take a file system backup as the standbys are still synchroni
Dear Hou,
Thank you for reviewing!
> 1.
>
> #include "access/transam.h"
> #include "catalog/pg_language_d.h"
> +#include "fe_utils/string_utils.h"
> #include "pg_upgrade.h"
>
> It seems we don't need this head file anymore.
Removed.
> 2.
> + if (*invalidated && SlotIsLogical(s)
Dear Peter,
Thank you for reviewing! PSA new version.
> src/backend/replication/slot.c
>
> 3. InvalidatePossiblyObsoleteSlot
>
> + /*
> + * Raise an ERROR if the logical replication slot is invalidating. It
> + * would not happen because max_slot_wal_keep_size is set to -1 during
> + * the upgr
Dear Peter,
Thank you for reviewing!
=
> Commit message
>
> 1.
> Note that the pg_resetwal command would remove WAL files, which are required
> as
> restart_lsn. If WALs required by logical replication slots are removed, they
> are
> unusable. Therefore, during the upgrade, slot restoration
Dear Michael,
> On Tue, Sep 12, 2023 at 02:33:25AM +, Zhijie Hou (Fujitsu) wrote:
> > 2.
> > + if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade)
> > + elog(ERROR, "Replication slots must not be invalidated
> during the upgrade.");
> >
> > I think normally the
Dear Peter,
Thank you for reviewing! Before posting new patch set, I want to respond some
comments.
>
> ==
> 1. GENERAL -- Cluster Terminology
>
> This is not really a problem of your patch, but during message review,
> I noticed the terms old/new cluster VERSUS source/target cluster and
>
On Tue, Sep 12, 2023 at 02:33:25AM +, Zhijie Hou (Fujitsu) wrote:
> 2.
> + if (*invalidated && SlotIsLogical(s) && IsBinaryUpgrade)
> + elog(ERROR, "Replication slots must not be invalidated
> during the upgrade.");
>
> I think normally the first letter is lowe
On Monday, September 11, 2023 9:22 PM Kuroda, Hayato/黒田 隼人
wrote:
>
> Thank you for reviewing! PSA new version.
Thanks for updating the patch, few cosmetic comments:
1.
#include "access/transam.h"
#include "catalog/pg_language_d.h"
+#include "fe_utils/string_utils.h"
#include "pg_upgrade.h"
Hi Kuroda-san.
Here are some additional review comments for v35-0002 (and because we
overlapped, my v34-0002 review comments have not been addressed yet)
==
Commit message
1.
Note that the pg_resetwal command would remove WAL files, which are required as
restart_lsn. If WALs required by log
Hi Kuroda-san, here are my review comments for v34-0002
There is likely to be some overlap because others have modified and/or
commented on some of the same points as me, and v35 was already posted
before this review. I'll leave it to you to sort out any clashes and
ignore them where appropriate.
Dear Amit,
Thank you for reviewing!
> Few comments:
> ==
> 1.
> +linkend="view-pg-replication-slots">pg_replication_slots.c
> onfirmed_flush_lsn
> + of all slots on the old cluster must be the same as the latest
> + checkpoint location.
>
> We can add something li
Dear Amit,
Thank you for giving a suggestion!
> >
> > > 2. Why get_old_cluster_logical_slot_infos() need to use
> > > pg_malloc_array whereas for similar stuff get_rel_infos() use
> > > pg_malloc()?
> >
> > They did a same thing. I used pg_malloc_array() macro to keep the code
> > within 80 colu
Dear Dilip,
Thank you for reviewing! PSA new version.
>
> 1.
> Note that slot restoration must be done after the final pg_resetwal command
> during the upgrade because pg_resetwal will remove WALs that are required by
> the slots. Due to this restriction, the timing of restoring replication slot
On Fri, Sep 8, 2023 at 6:31 PM Hayato Kuroda (Fujitsu)
wrote:
>
> Thank you for reviewing! PSA new version! PSA new version.
>
Few comments:
==
1.
+ pg_replication_slots.confirmed_flush_lsn
+ of all slots on the old cluster must be the same as the latest
+ checkpoint
On Mon, Sep 11, 2023 at 11:16 AM Amit Kapila wrote:
>
> On Mon, Sep 11, 2023 at 10:39 AM Dilip Kumar wrote:
> >
> > 3.
> > - with the primary.) Replication slots are not copied and must
> > - be recreated.
> > + with the primary.) Replication slots on the old standby are not
On Mon, Sep 11, 2023 at 10:39 AM Dilip Kumar wrote:
>
> 3.
> - with the primary.) Replication slots are not copied and must
> - be recreated.
> + with the primary.) Replication slots on the old standby are not
> copied.
> + Only logical slots on the primary are migrated
On Fri, Sep 8, 2023 at 6:31 PM Hayato Kuroda (Fujitsu)
wrote:
Comments on the latest patch.
1.
Note that slot restoration must be done after the final pg_resetwal command
during the upgrade because pg_resetwal will remove WALs that are required by
the slots. Due to this restriction, the timing o
On Fri, Sep 8, 2023 at 6:36 PM Hayato Kuroda (Fujitsu)
wrote:
>
> > 2. Why get_old_cluster_logical_slot_infos() need to use
> > pg_malloc_array whereas for similar stuff get_rel_infos() use
> > pg_malloc()?
>
> They did a same thing. I used pg_malloc_array() macro to keep the code
> within 80 col
Dear Amit,
> On Fri, Sep 8, 2023 at 2:12 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> > 2.
> >
> > + if (nslots_on_new)
> > + {
> > + if (nslots_on_new == 1)
> > + pg_fatal("New cluster must not have logical
> replication slots but found a slot.");
> > +
Dear Amit,
Thank you for reviewing!
> Few comments:
> =
> 1.
>
> + All slots on the old cluster must be usable, i.e., there are no slots
> + whose
> +linkend="view-pg-replication-slots">pg_replication_slots.
> wal_status
> + is lost.
> +
>
> Shall we
Dear Hou,
Thank you for reviewing! PSA new version! PSA new version.
> Here are some comments:
>
> 1.
>
> bool reap_child(bool wait_for_child);
> +
> +XLogRecPtr strtoLSN(const char *str, bool *have_error);
>
> This function has be removed.
Removed.
> 2.
>
> + if (nslots_on_n
On Fri, Sep 8, 2023 at 2:12 PM Zhijie Hou (Fujitsu)
wrote:
>
> 2.
>
> + if (nslots_on_new)
> + {
> + if (nslots_on_new == 1)
> + pg_fatal("New cluster must not have logical
> replication slots but found a slot.");
> + else
> +
On Thu, Sep 7, 2023 at 5:54 PM Hayato Kuroda (Fujitsu)
wrote:
>
> Thank you for reviewing! PSA new version.
>
Few comments:
=
1.
+ All slots on the old cluster must be usable, i.e., there are no slots
+ whose
+ pg_replication_slots.wal_status
+ is lost.
+
On Thursday, September 7, 2023 8:24 PM Kuroda, Hayato/黒田 隼人
wrote:
>
> Dear Peter,
>
> Thank you for reviewing! PSA new version.
Thanks for updating the patches !
Here are some comments:
1.
bool reap_child(bool wait_for_child);
+
+XLogRecPtr strtoLSN(const char *str, bool *ha
Dear Peter,
Thank you for reviewing! PSA new version.
> ==
> src/bin/pg_upgrade/check.c
>
> 1. check_new_cluster_logical_replication_slots
>
> + res = executeQueryOrDie(conn, "SHOW max_replication_slots;");
> + max_replication_slots = atoi(PQgetvalue(res, 0, 0));
> +
> + if (PQntuples(res)
101 - 200 of 402 matches
Mail list logo
