mary server.
+
Let's word this more like the same sentence top of the page. See
review comment #3c
SUGGESTION
If the result (failover_ready) of both steps above
is true, then existing subscriptions can continue subscribing to
publications now on the new primary server without any loss of data.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ON test?
GENERAL - Missing ALTER SUBSCRIPTION test?
How come this patch adds a new CREATE SUBSCRIPTION option but does not
seem to include any test case for that option in either the CREATE
SUBSCRIPTION or ALTER SUBSCRIPTION regression tests?
==
[1] My v1 review -
https://www.postgresql.org/message-id/CAHut+PsuJfcaeg6zst=6pe5uyjv_uxvrhu3ck7w2ahb1uqy...@mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
rds.
>
Hi,
I think it might be better to keep all the discussions about GUC
quoting and related topics like this confined to the main thread here
[1]. Otherwise, we might end up with a bunch of competing patches.
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPv-kSN8SkxSdoHano_wPubqcg5789ejhCDZAcLFceBR-w%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, May 17, 2024 at 9:57 PM Peter Eisentraut wrote:
>
> On 17.05.24 05:31, Peter Smith wrote:
> >> I think we should accept your two patches
> >>
> >> v6-0001-GUC-names-docs.patch
> >> v6-0002-GUC-names-add-quotes.patch
> >>
> >&g
Hi Kuroda-san,
I did not apply these v12* patches, but I have diff'ed all of them
with the previous v11* patches and confirmed that all of my previous
review comments now seem to be addressed.
I don't have any more comments to make at this time. Thanks!
==
Kind Regards,
Peter Smith.
Fujitsu
On Thu, May 16, 2024 at 9:35 PM Peter Eisentraut wrote:
>
> On 04.01.24 07:53, Peter Smith wrote:
> >> Now that I read this again, I think this is wrong.
> >>
> >> We should decide the quoting for a category, not the actual content.
> >> Like, q
bout when this might
be useful.
==
[1]
https://github.com/postgres/postgres/commit/fa65a022db26bc446fb67ce1d7ac543fa4bb72e4
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, May 16, 2024 at 10:42 PM David Rowley wrote:
>
> On Thu, 16 May 2024 at 19:05, Peter Smith wrote:
> >
> > On Thu, May 16, 2024 at 3:11 PM David Rowley wrote:
> > > If you want to do this, what's the reason to limit it to just this one
> > > page of th
On Thu, May 16, 2024 at 3:11 PM David Rowley wrote:
>
> On Thu, 16 May 2024 at 12:29, Peter Smith wrote:
> > There are lots of subscription options listed on the CREATE
> > SUBSCRIPTION page [1].
> >
> > Although these boolean options are capable of accepting
to.h
7.
(Same as a previous review comment)
For all the API changes the new parameter name should be plural.
/publish_generated_column/publish_generated_columns/
==
src/include/replication/pgoutput.h
8.
bool publish_no_origin;
+ bool publish_generated_column;
} PGOutputData;
/publish_generated_column/publish_generated_columns/
==
Kind Regards,
Peter Smith.
Fujitsu Australia
bscription.html
[3]
https://www.postgresql.org/message-id/flat/8fab8-65d74c80-1-2f28e880%4039088166
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Always-use-true-false-instead-of-on-off-for-boole.patch
Description: Binary data
r aborting the prepared transaction.
>
> Fixed.
>
You wrote "Fixed" for that patch v9-0004 suggestion but I don't think
anything was changed at all. Accidentally missed?
==
Kind Regards,
Peter Smith.
Futjisu Australia
e aborted and the alter will proceed.
+ The default is false.
+
+
+
IMO this will be better broken into multiple paragraphs.
1. Specifies...
2. There is...
3. The default is...
==
src/test/subscription/t/099_twophase_added.pl
(Let's change all the on|off to true|false like you already did in patch 0002.
4.3.
+# Try altering the two_phase option to "off." The command will fail since there
+# is a prepared transaction and the 'force_alter' option is not specified as
+# true.
+my $stdout;
+my $stderr;
/off./false/
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Force alter", as already mentioned by an
earlier review comment (#4.7)
==
src/test/subscription/t/099_twophase_added.pl
4.11.
+# Alter the two_phase with the force_alter option. Apart from the the last
+# ALTER SUBSCRIPTION command, the command will abort the prepared transaction
+# and succeed.
There is typo "the the" and the wording is a bit strange. Why not just say:
SUGGESTION
Alter the two_phase true to false with the force_alter option enabled.
This command will succeed after aborting the prepared transaction.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
n parameters, instead of sometimes using
different values like on|off.
What do you think?
==
[1] https://www.postgresql.org/docs/devel/sql-createsubscription.html
Kind Regards,
Peter Smith.
Fujitsu Australia
"off", then normally, this PREPARE will do nothing
+# until the COMMIT PREPARED, but in this test, we toggle the two_phase to "on"
+# again before the COMMIT PREPARED happens.
This is a major test part so IMO this comment should have
# like it had before, to distinguish it from all
the sub-step comments.
==
My v7-0002 review -
https://www.postgresql.org/message-id/CAHut%2BPtu_w_UCGR-5DbenA%2By7wRiA8QPi_ZP%3DCCJ3SGdTn-%3D%3Dg%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia.
the prepared transaction are aborted
$result = $node_subscriber->safe_psql('postgres',
"SELECT count(*) FROM pg_prepared_xacts;");
is($result, q(0), "prepared transaction done by worker is aborted");
/transaction are aborted/transaction was aborted/
==
Response to my v7-0004 review --
https://www.postgresql.org/message-id/OSBPR01MB2552F738ACF1DA6838025C4FF5E62%40OSBPR01MB2552.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
ker is aborted");
+
/the prepared transaction are aborted/any prepared transactions are aborted/
==
Kind Regards,
Peter Smith
Fujitsu Australia
de_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION sub ENABLE;");
+
I felt the ENABLE statement should be above the SELECT statement so
that the code is more like it was before applying the patch.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
It's better to rename the SUBSCRIPTION in this TAP test so you can
avoid getting log warnings like:
psql::4: WARNING: subscriptions created by regression test
cases should have names starting with "regress_"
psql::4: NOTICE: created replication slot "sub" on publisher
==
Kind Regards,
Peter Smith.
Fujitsu Australia
quot; again
# before the COMMIT PREPARED happens.
~~~
7.
Maybe this test case needs a few more one-line comments for each of
the sub-steps. e.g.:
# prepare a transaction to insert some rows to the table
# verify the prepared tx is not yet replicated to the subscriber
(because 'two_phase = off')
# toggle the two_phase to 'on' *before* the COMMIT PREPARED
# verify the inserted rows got replicated ok
~~~
8.
IIUC this test will behave the same even if you DON'T do the toggle
'two_phase = on'. So I wonder is there something more you can do to
test this scenario more convincingly?
==
Kind Regards,
Peter Smith
Fujitsu Australia
cannot disable two_phase when uncommitted prepared
transactions present"),
+ errhint("Resolve these transactions and try again")));
The comment "/* Add error message */" seems unnecessary.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
criber->safe_psql('postgres',
+ "SELECT subtwophasestate FROM pg_subscription WHERE subname = 'tap_sub_copy';"
+);
+is($result, qq(e), 'two-phase is disabled');
The 'two-phase is disabled' is the identical message used in the
opposite case earlier, so something is amiss. Maybe this one should
say 'two-phase should be enabled' and the earlier counterpart should
say 'two-phase should be disabled'.
==
Kind Regards,
Peter Smith
Fujitsu Australia
Hi, just by visual inspection of the v3/v4 patch diffs, the latest v4 LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Sun, Mar 31, 2024 at 8:12 PM Andrey M. Borodin wrote:
>
>
>
> > On 15 Aug 2023, at 07:38, Peter Smith wrote:
> >
> > A rebase was needed due to a recent push [1].
> >
> > PSA v3.
>
>
> > On 14 Jan 2024, at 10:43, vignesh C wrote:
> &g
/0b84f5c419a300dc1b1a70cf63b9907208e52643/src/backend/replication/slotfuncs.c#L989
[2]
https://github.com/postgres/postgres/blob/0b84f5c419a300dc1b1a70cf63b9907208e52643/src/backend/replication/logical/slotsync.c#L1258
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Mar 13, 2024 at 12:48 PM Masahiko Sawada wrote:
>
> On Wed, Mar 13, 2024 at 10:15 AM Peter Smith wrote:
> >
> > On Tue, Mar 12, 2024 at 4:23 PM Masahiko Sawada
> > wrote:
> > >
> > > On Fri, Mar 8, 2024 at 12:58 PM Peter S
On Tue, Mar 12, 2024 at 4:23 PM Masahiko Sawada wrote:
>
> On Fri, Mar 8, 2024 at 12:58 PM Peter Smith wrote:
> >
...
> > > > 5.
> > > > + *
> > > > + * If 'indexed' is true, we create a hash table to track of each node's
> > > >
12b.
This comment should also say that the heap is ordered by tx size --
(e.g. the comparator is ReorderBufferTXNSizeCompare)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Mar 7, 2024 at 2:16 PM Masahiko Sawada wrote:
>
> On Tue, Mar 5, 2024 at 3:28 PM Peter Smith wrote:
> >
> > 4a.
> > The comment in simplehash.h says
> > * The following parameters are only relevant when SH_DEFINE is defined:
> > * - SH_KEY -
dby_slot_config->slot_num is
0. So shouldn't that be checked too?
Perhaps it is convenient to encapsulate this check using some macro:
#define StandbySlotNamesHasNoValue() (standby_slot_config = NULL ||
standby_slot_config->slot_num == 0)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ssary at all. Can't
you just use the bh_nodeidx value? E.g. If bh_nodeidx == NULL then it
means there is no index tracking, otherwise there is.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
't really "Make sure" of
anything because the caller does the check whether we need more space.
All that happens here is allocating more space. Maybe this function
comment should say something like "Double the space allocated for
nodes."
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ly flushed
+ * position only if we are not waiting for standbys to catch up.
+ */
Regarding that 1st sentence: maybe this logic used to be done
explicitly "within the loop" but IIUC this logic is now hidden inside
NeedToWaitForWal() so the comment should mention that.
--
Kind Regards,
On Mon, Mar 4, 2024 at 2:38 PM Amit Kapila wrote:
>
> On Mon, Mar 4, 2024 at 6:57 AM Peter Smith wrote:
> >
> > On Sun, Mar 3, 2024 at 2:56 PM Amit Kapila wrote:
> > >
> > > On Sun, Mar 3, 2024 at 5:17 AM Peter Smith wrote:
> &g
On Mon, Mar 4, 2024 at 2:49 PM Amit Kapila wrote:
>
> On Mon, Mar 4, 2024 at 7:25 AM Peter Smith wrote:
> >
> >
> > ==
> > doc/src/sgml/config.sgml
> >
> > 2.
> > +pg_logical_slot_peek_changes,
> > +when used wit
On Sun, Mar 3, 2024 at 6:51 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Sunday, March 3, 2024 7:47 AM Peter Smith wrote:
> >
> > 3.
> > +
> > +
> > + Value * is not accepted as it is inappropriate
> > to
> > + block lo
On Sun, Mar 3, 2024 at 2:56 PM Amit Kapila wrote:
>
> On Sun, Mar 3, 2024 at 5:17 AM Peter Smith wrote:
> >
...
> > 9. NeedToWaitForWal
> >
> > + /*
> > + * Check if the standby slots have caught up to the flushed position. It
> > + * is good to wait u
On Fri, Jan 12, 2024 at 4:07 PM Peter Smith wrote:
>
> On Mon, Jan 30, 2023 at 8:36 AM Tom Lane wrote:
> >
> > Zheng Li writes:
> > > The behavior is due to the following code
> > > https://github.com/postgres/postgres/blob/master/src/backend/commands/define.
On Sat, Mar 2, 2024 at 2:51 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Friday, March 1, 2024 12:23 PM Peter Smith wrote:
> >
...
> > ==
> > src/backend/replication/slot.c
> >
> > 2. validate_standby_slots
> >
> > + else if (!ReplicationSlot
might be no problem.
>
Hi, a while ago I asked this same question. See [1 #28] for the response..
--
[1]
https://www.postgresql.org/message-id/OS0PR01MB571646B8186F6A06404BD50194BDA%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
; flushed_lsn && !(prioritize_stop && got_STOPPING))
{
*wait_event = WAIT_EVENT_WAL_SENDER_WAIT_FOR_WAL;
return true;
}
if (failover_slot && !StandbySlotsHaveCaughtup(flushed_lsn, elevel))
{
*wait_event = WAIT_EVENT_WAIT_FOR_STANDBY_CONFIRMATION;
return true;
}
return false;
--
Kind Regards,
Peter Smith.
Fujitsu Australia
CALDECODING-REPLICATION-SLOTS-SYNCHRONIZATION
[3] [1]
https://www.postgresql.org/message-id/OS0PR01MB5716E581B4227DDEB4DE6C30944F2%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Replace-aka-in-docs.patch
Description: Binary data
On Tue, Feb 27, 2024 at 11:35 PM Amit Kapila wrote:
>
> On Tue, Feb 27, 2024 at 12:48 PM Peter Smith wrote:
> >
> > Here are some review comments for v99-0001
> >
> > ==
> > 0. GENERAL.
> >
> > +#standby_slot_names = '' #
On Wed, Feb 28, 2024 at 1:23 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Tuesday, February 27, 2024 3:18 PM Peter Smith
> wrote:
...
> > 20.
> > +#
> > +# | > standby1 (primary_slot_name = sb1_slot) # | > standby2
> > +(primary_slot_name = sb2_slot)
e a subcategory (for when
'replication' is true). Therefore, won't the modified check be better
to be written the other way around? This will also be consistent with
the way the Assert was written.
SUGGESTION
if (!replication || logical)
{
...
==
Kind Regards,
Peter Smith.
Fujitsu Australia
standby, but it doesn't seem to check
that it *does* return when the stopped standby comes alive again.
~~~
31.
+$result =
+ $subscriber1->safe_psql('postgres', "SELECT count(*) = 0 FROM tab_int;");
+is($result, 't',
+ "subscriber1 doesn't get data as the sb1_slot doesn't catch up");
Do you think this fragment should have a comment?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi, the patch v4 LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ctly to the term "Instance".
~~
Apart from those links, it looks good to me. Let's see what others think.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
o "on", but I find it rather
> non-intuitive.
>
Also, I don't understand how the word "draught" (aka "draft") makes
sense here -- I assume the intended word was "drought" (???).
==
Kind Regards,
Peter Smith.
Fujitsu Australia.
o, instead of ad-hoc hunting for and fixing
cases one at a time.
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPsf3NewXbsFKY88Qn1ON1_dMD6343MuWdMiiM2Ds9a_wA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
ribes to. For more information, see Section 30.2
Subscription node
A node where a subscription is defined for logical replication.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi Vignesh, I have no further comments. Patch v9 LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Feb 22, 2024 at 5:56 PM Michael Paquier wrote:
>
> On Thu, Feb 22, 2024 at 05:30:08PM +1100, Peter Smith wrote:
> > Oops. Perhaps I meant more like below -- in any case, the point was
> > the same -- to ensure RS_INVAL_NONE is what returns if something
> > une
On Thu, Feb 22, 2024 at 5:19 PM Peter Smith wrote:
>
> Hi, Sorry for the late comment but isn't the pushed logic now
> different to what it was there before?
>
> IIUC previously (in a non-debug build) if the specified
> conflict_reason was not found, it returned RS_INVAL_NONE
ationCauses[cause], conflict_reason) == 0;
Assert(found);
return found ? cause : RS_INVAL_NONE;
}
--
Kind Regards,
Peter Smith.
Fujitsu Australia
pg_replication_slots WHERE slot_type
= 'logical' AND conflict_reason IS NOT NULL;
slot_name
---
(0 rows)
versus
SELECT count(*) FROM pg_replication_slots WHERE slot_type = 'logical'
AND temporary IS false;
==
Kind Regards,
Peter Smith.
Fujitsu Australia
stat.h"
+#include "replication/slotsync.h"
#include "replication/slot.h"
+#include "replication/walsender.h"
#include "storage/fd.h"
The #include "replication/walsender.h" seems to be unnecessary.
==
src/backend/replication/walsender.c
3.
#include "replication/logical.h"
+#include "replication/slotsync.h"
#include "replication/slot.h"
The #include "replication/slotsync.h" is needed, but only for Assert code:
Assert(am_cascading_walsender || IsSyncingReplicationSlots());
So you could #ifdef around that #include if you wish to.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
e glossary term "logical
replication clusters".
==
Kind Regards,
Peter Smith.
Fujitsu Australia
(remote_slot, remote_dbid);
+
+ UnlockSharedObject(DatabaseRelationId, remote_dbid, 0, AccessShareLock);
IMO remove the blank lines (e.g., you don't use this kind of
formatting for spin locks)
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Feb 8, 2024 at 9:47 PM Peter Eisentraut wrote:
>
> On 23.01.24 02:06, Peter Smith wrote:
> > This has been working forever, but seems to have broken due to commit
> > [1] having an undeclared variable.
>
> > runtime error: file stylesheet-html-common.xsl line
et
+ * appropriately, otherwise raise ERROR.
+ */
/Validates if all/Check all/
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Feb 8, 2024 at 11:12 AM James Coleman wrote:
>
> On Wed, Feb 7, 2024 at 6:04 PM Peter Smith wrote:
> >
> > On Thu, Feb 8, 2024 at 9:04 AM James Coleman wrote:
> > >
> > > On Wed, Feb 7, 2024 at 3:22 PM Laurenz Albe
> > > wrote:
> > &
a replica identity is added to a publication that
replicates UPDATE or DELETE operations then subsequent UPDATE or
DELETE operations will cause an error on the publisher.
SUGGESTION
If a table without a replica identity (or with replica identity
behavior equivalent to NOTHING) is added to a publication that
replicates UPDATE or DELETE operations then subsequent UPDATE or
DELETE operations will cause an error on the publisher.
==
[1]
https://www.postgresql.org/docs/current/sql-altertable.html#SQL-ALTERTABLE-REPLICA-IDENTITY
[2] https://www.postgresql.org/docs/current/logical-replication-publication.html
Kind Regards,
Peter Smith.
Fujitsu Australia
but it is a safe-guard for cleaning up if some unexpected
ERROR happens. Maybe there should be a comment to say that.
==
src/test/recovery/t/040_standby_failover_slots_sync.pl
10.
+# Confirm that the logical failover slot is created on the standby and is
+# flagged as 'synced'
+is($standby1->safe_psql('postgres',
+ q{SELECT count(*) = 2 FROM pg_replication_slots WHERE slot_name IN
('lsub1_slot', 'lsub2_slot') AND synced;}),
+ "t",
+ 'logical slots have synced as true on standby');
/slot is created/slots are created/
/and is flagged/and are flagged/
==
Kind Regards,
Peter Smith.
Fujitsu Australia
check_remote_synced_slot_exists
validate_slotsync_params check_local_config
IsSyncingReplicationSlots IsSyncingReplicationSlots
pg_sync_replication_slots pg_sync_replication_slots
==
Kind Regards,
Peter Smith.
Fujitsu Australia
~
Thoughts?
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPtJAAPghc4GPt0k%3DjeMz1qu4H7mnaDifOHsVsMqi-qOLA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
ues as the
remote slot.
==
.../t/040_standby_failover_slots_sync.pl
25.
+
+$standby1->safe_psql('postgres', "SELECT pg_sync_replication_slots();");
Since this is where we use the function added by this patch, it
deserves to have a comment.
SUGGESTION
# Synchronize the primary server slots to the standby.
==
src/tools/pgindent/typedefs.list
26.
It looks like 'RemoteSlot' should be included in the typedefs.list
file. Probably this is the explanation for the space problems I
reported earlier in this post.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Feb 2, 2024 at 11:18 PM shveta malik wrote:
>
> On Fri, Feb 2, 2024 at 12:25 PM Peter Smith wrote:
> >
> > Here are some review comments for v750002.
>
> Thanks for the feedback Peter. Addressed all in v76 except one.
>
> > (this is a WIP but this i
On Sat, Feb 3, 2024 at 5:28 PM Amit Kapila wrote:
>
> On Thu, Feb 1, 2024 at 5:58 AM Peter Smith wrote:
> >
> > OK. Now using your suggested 2nd sentence:
> >
> > +# The subscription's running status and failover option should be preserved
> > +# in the upgra
ame = walrcv_get_dbname_from_conninfo(PrimaryConnInfo);
+ if (*dbname == NULL)
+ {
+ ereport(LOG,
+
+ /*
+ * translator: 'dbname' is a specific option; %s is a GUC variable
+ * name
+ */
+ errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("bad configuration for slot synchronization"),
+ errhint("'dbname' must be specified in \"%s\".", "primary_conninfo"));
+ return false;
+ }
+
+ return true;
+}
I wonder if it is better to log all the problems in one go instead of
making users stumble onto them one at a time after fixing one and then
hitting the next problem. e.g. just set some variable "all_ok =
false;" each time instead of all the "return false;"
Then at the end of the function just "return all_ok;"
==
Kind Regards,
Peter Smith.
Fujitsu Australia
sing any description of the new parameter 'replication'.
~~~
8.
+/*
+ * walrcv_get_dbname_from_conninfo_fn
+ *
+ * Returns the dbid from the primary_conninfo
+ */
+typedef char *(*walrcv_get_dbname_from_conninfo_fn) (const char *conninfo);
+
/dbid/database name/
==
Kind Regards,
Peter Smith.
Fujitsu Australia
|| strcmp(PrimaryConnInfo, "") == 0)
SUGGESTION
if (PrimaryConnInfo == NULL || *PrimaryConnInfo == '\0')
==
Kind Regards,
Peter Smith.
Fujitsu Australia
to false.
~
I also tweaked some other nearby comments/messages which did not
mention the 'failover' preservation.
PSA v2.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v2-0001-Fix-pg_upgrade-test-comment.patch
Description: Binary data
ment. Let's see what others think.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ust going by visual inspection of the v2/v3 patch diffs, the
latest v3 LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Jan 31, 2024 at 4:51 PM vignesh C wrote:
>
> On Wed, 31 Jan 2024 at 10:27, Peter Smith wrote:
> >
> > Hi,
> >
> > PSA a small fix for a misleading comment found in the pg_upgrade test code.
>
> Is the double # intentional here, as I did not see this
gres/postgres/commit/776621a5e4796fa214b6b29a7ca134f6c138572a
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Jan 31, 2024 at 2:18 PM Amit Kapila wrote:
>
> On Wed, Jan 31, 2024 at 7:27 AM Peter Smith wrote:
> >
> > I saw that v73-0001 was pushed, but it included some last-minute
> > changes that I did not get a chance to check yesterday.
> >
> > Here are so
Hi,
PSA a small fix for a misleading comment found in the pg_upgrade test code.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Fix-test-comment.patch
Description: Binary data
subscription's failover and two_phase options say: for example, the
slot on the publisher could ...
SUGGESTION:
Otherwise, the slot on the publisher may behave differently from what
these subscription options say: for example, the slot on the publisher
could ...
==
Kind Regards,
Peter Smith
ading. Aren't these the
new/upgraded subscriptions being checked here?
Should the comment be more like:
# The subscription's running status and failover option should be preserved.
# Upgraded regress_sub1 should still have enabled and failover as true.
# Upgraded regress_sub2 should still have enabled and failover as false.
==
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Fri, Jan 19, 2024 at 7:21 PM Masahiko Sawada wrote:
>
> On Thu, Jan 18, 2024 at 6:54 PM Amit Kapila wrote:
> >
> > On Thu, Jan 18, 2024 at 11:15 AM Peter Smith wrote:
> > >
> > > On Thu, Jan 18, 2024 at 12:55 PM Masahiko Sawada
> > > wrote:
>
mment is not quite relevant to these tests. So, add another
one just these:
SUGGESTION:
##
# Test that cannot modify the failover option for enabled subscriptions.
######
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Jan 25, 2024 at 9:12 AM Peter Smith wrote:
>
> On Tue, Jan 23, 2024 at 12:32 PM Peter Smith wrote:
> >
> > On Tue, Jan 23, 2024 at 12:13 PM Tom Lane wrote:
> > >
> > > Peter Smith writes:
> > > > I usually the HTML documentation locall
*) cmd;
+ }
+ ;
+
IMO write that comment with parentheses, so it matches the code.
SUGGESTION
ALTER_REPLICATION_SLOT slot ( options )
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPtDWSmW8uiRJF1LfGQJikmo7V2jdysLuRmtsanNZc7fNw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
Migration of subscription dependencies is only
+supported when the old cluster is version 17.0 or later. Subscription
+dependencies on clusters before version 17.0 will silently be ignored.
+
Perhaps the part from "pg_upgrade attempts..." should be in a new paragraph.
==
Kind
are configuration issues, so probably all these ereports could
also set errcode(ERRCODE_INVALID_PARAMETER_VALUE).
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ditions should be written the other way around (failover &&
RecoveryInProgress()) to avoid the unnecessary function calls when
'failover' flag is probably mostly default false anyway.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
rade...
==
[1]
https://english.stackexchange.com/questions/192187/upgradation-not-universally-accepted#:~:text=Not%20all%20dictionaries%20(or%20native,by%20most%20non%2DIE%20speakers.
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Jan 23, 2024 at 12:32 PM Peter Smith wrote:
>
> On Tue, Jan 23, 2024 at 12:13 PM Tom Lane wrote:
> >
> > Peter Smith writes:
> > > I usually the HTML documentation locally using command:
> > > make STYLE=website html
> > > This has been
physical standbys/physical standby/
==
src/include/replication/slot.h
19.
+
+ /*
+ * Is this a failover slot (sync candidate for physical standbys)? Only
+ * relevant for logical slots on the primary server.
+ */
+ bool failover;
(same as earlier)
/physical standbys/physical standby/
==
[1] Nisha errmsg -
https://www.postgresql.org/message-id/CABdArM5-VR4Akt_AHap_0Ofne0cTcsdnN6FcNe%2BMU8eXsa_ERQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
src/include/replication/walreceiver.h
5.
typedef char *(*walrcv_identify_system_fn) (WalReceiverConn *conn,
TimeLineID *primary_tli);
+/*
+ * walrcv_get_dbname_from_conninfo_fn
+ *
+ * Returns the dbid from the primary_conninfo
+ */
+typedef char *(*walrcv_get_dbname_from_conninfo_fn) (const char *conninfo);
It looks like a blank line that previously existed has been lost.
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPsf3NewXbsFKY88Qn1ON1_dMD6343MuWdMiiM2Ds9a_wA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Jan 23, 2024 at 12:13 PM Tom Lane wrote:
>
> Peter Smith writes:
> > I usually the HTML documentation locally using command:
> > make STYLE=website html
> > This has been working forever, but seems to have broken due to commit
> > [1] having an undeclar
ror 10
==
[1]
https://github.com/postgres/postgres/commit/b0f0a9432d0b6f53634a96715f2666f6d4ea25a1
Kind Regards,
Peter Smith.
Fujitsu Australia
i.com/task/5581154296791040
Kind Regards,
Peter Smith.
com/github/postgresql-cfbot/postgresql/commitfest/46/4759
Kind Regards,
Peter Smith.
com/github/postgresql-cfbot/postgresql/commitfest/46/4738
Kind Regards,
Peter Smith.
i.com/task/6688578378399744
Kind Regards,
Peter Smith.
com/github/postgresql-cfbot/postgresql/commitfest/46/4725
Kind Regards,
Peter Smith.
1 - 100 of 1450 matches
Mail list logo
