Hi.
While reviewing another patch to the file info.c, I noticed there seem
to be some unnecessary calls to strlen(query) in get_rel_infos()
function.
i.e. The query is explicitly initialized to an empty string
immediately prior, so why the strlen?
PSA patch for this.
--
Kind Regards,
Peter
Hi --
While reviewing another patch for the file info.c, I noticed some
misplaced colon (':') in the verbose logs for print_rel_infos().
PSA patch to fix it.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-pg_upgrade-typo-in-verbose-log.patch
Description: Binary data
On Thu, May 11, 2023 at 6:30 PM Daniel Gustafsson wrote:
>
> > On 11 May 2023, at 07:41, Peter Smith wrote:
>
> > While reviewing another patch for the file info.c, I noticed some
> > misplaced colon (':') in the verbose logs for print_rel_infos().
&
7mNSLgA%40mail.gmail.com
[2] Kuroda-san reply to my v11 review -
https://www.postgresql.org/message-id/TYAPR01MB5866BD618DEE62AF1836E612F5749%40TYAPR01MB5866.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
adding similar new double
quotes to all other VERBOSE logging in your patch.
e.g. see get_logical_slot_infos:
pg_log(PG_VERBOSE, "Database: %s", pDbInfo->db_name);
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Mar 11, 2022 at 9:37 AM Euler Taveira wrote:
>
> On Fri, Mar 4, 2022, at 12:41 AM, Peter Smith wrote:
>
> PSA patch v3 to address all comments received so far.
>
> Peter,
>
> I started reviewing this documentation for row filter and I noticed that I was
> changi
On Wed, Mar 16, 2022 at 4:07 PM Amit Kapila wrote:
>
> On Mon, Aug 30, 2021 at 8:50 AM Peter Smith wrote:
> >
> > Patch v2 is the same; it only needed re-basing to the latest HEAD.
> >
>
> Why do you think it is correct to exit before trying to receive any
> me
f (HeadMatches("CREATE", "SUBSCRIPTION") && TailMatches("WITH", "("))
COMPLETE_WITH("binary", "connect", "copy_data", "create_slot",
- "enabled", "slot_name", "streaming",
+ "enabled", "slot_name", "streaming", "subscribe_local_only",
The patch accidentally added a space char before the "slot_name".
~~~
1.12 src/include/replication/walreceiver.h - "generated"
@@ -183,6 +183,7 @@ typedef struct
bool streaming; /* Streaming of large transactions */
bool twophase; /* Streaming of two-phase transactions at
* prepare time */
+ bool only_local; /* publish only locally generated data */
This is a similar review comment as #1.5 about saying the word "generated".
Maybe there is another way to word this?
~~~
1.13 src/test/regress/sql/subscription.sql - missing test case
Isn't there a missing test case for ensuring that the new option is boolean?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
blisher");
+
+$node_A->safe_psql('postgres', "
+CREATE SUBSCRIPTION tap_sub_A2
+CONNECTION '$node_B_connstr application_name=$appname_A'
+PUBLICATION tap_pub_B
+WITH (subscribe_local_only = on, copy_data = force)");
+
+$node_A->safe_psql('postgres', "
+DROP SUBSCRIPTION tap_sub_A2");
+
Maybe underneath it is the same, but from the outside, this looks like
a slightly different scenario from what is mentioned everywhere else
in the patch.
I think it would be better to create a new Node_C (aka Node3) so then
the TAP test can use the same example that you give in the commit
message and the PG docs notes.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Apr 5, 2022 at 12:36 PM Amit Kapila wrote:
>
> On Tue, Apr 5, 2022 at 6:21 AM Peter Smith wrote:
> >
> > Here are my comments for the latest patch v6-0001.
> >
> > (I will post my v6-0002 review comments separately)
> >
> > PATCH v6-0001 comme
On Sat, Apr 2, 2022 at 5:17 PM Amit Kapila wrote:
>
> On Fri, Apr 1, 2022 at 1:52 PM Peter Smith wrote:
> >
> > On Wed, Mar 16, 2022 at 4:07 PM Amit Kapila wrote:
> >
> > I think the STATE_CATCHUP guarantees the apply worker must have
> > received (or
(nodes N1, N2, N3, N4),
then 5-way sharing (nodes N1, N2, N3, N4, N5).
As an extra complication, for this test, all 5 nodes have different
initial table data, which gets replicated to the others whenever each
new node joins the existing share group.
PSA.
--
Kind Regards,
Peter Smith.
Fujitsu
se 2
subscriptions then the 2nd one would cause the PK violation that
probably you were anticipating would happen.
I have modified the 2-way example to use the same truncate pattern as others.
PSA the fixed test.sh script and accompanying files.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Cle
Hi Vignesh, FYI the patch is recently broken again and is failing on cfbot [1].
--
[1] http://cfbot.cputube.org/patch_38_3610.log
Kind Regards,
Peter Smith.
Fujitsu Australia
PSA patch v6 to address some of Amit's review comments [1].
--
[1]
https://www.postgresql.org/message-id/CAA4eK1JdwQQsxa%2BzpsBW5rCxEfXopYx381nwcCyeXk6mpF8ChQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
v6-0001-PG-DOCS-page-for-row-filters.patch
Description: Binary data
On Wed, Apr 6, 2022 at 8:58 PM Amit Kapila wrote:
>
> On Thu, Mar 24, 2022 at 11:48 AM Peter Smith wrote:
> >
>
> Review comments:
> ===
> 1.
> +The WHERE clause expression is evaluated with the same
> +role used for the replication connection.
/CAA4eK1JPyVoc1dUjeqbPd9D0_uYxWyyx-8fcsrgiZ5Tpr9OAuw%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/202204091045.v2ei4yupxqso%40alvherre.pgsql
Kind Regards,
Peter Smith.
Fujitsu Australia
v7-0001-PG-DOCS-page-for-row-filters.patch
Description: Binary data
On Fri, Apr 8, 2022 at 4:12 PM Peter Smith wrote:
>
> On Wed, Apr 6, 2022 at 8:58 PM Amit Kapila wrote:
> >
> > On Thu, Mar 24, 2022 at 11:48 AM Peter Smith wrote:
> > >
> >
> > Review comments:
> > ===
[snip]
>
> > 9. I suggest
On Mon, Apr 11, 2022 at 1:27 PM Amit Kapila wrote:
>
> On Fri, Apr 8, 2022 at 11:42 AM Peter Smith wrote:
> >
> > On Wed, Apr 6, 2022 at 8:58 PM Amit Kapila wrote:
> > >
> >
> > > 3.
> > > +
> > > +Whenever an UPDATE is proc
d Regards,
Peter Smith.
Fujitsu Australia
On Tue, Apr 12, 2022 at 11:31 AM houzj.f...@fujitsu.com
wrote:
>
> On Tuesday, April 12, 2022 8:40 AM Peter Smith wrote:
> >
> > FYI, I was playing with row filters and partitions recently, and while doing
> > something a bit unusual I received a cache leak warning.
>
On Thu, Apr 7, 2022 at 2:09 PM Peter Smith wrote:
>
> FYI, here is a test script that is using the current patch (v6) to
> demonstrate a way to share table data between different numbers of
> nodes (up to 5 of them here).
>
> The script starts off with just 2-way sharing (nod
PSA patch v9 which addresses most of Euler's review comments [1]
--
[1]
https://www.postgresql.org/message-id/1c78ebd4-b38d-4b5d-a6ea-d583efe87d97%40www.fastmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
v9-0001-Add-additional-documentation-for-row-filters.patch
Description: B
On Tue, Apr 12, 2022 at 3:33 AM Euler Taveira wrote:
>
> On Mon, Apr 11, 2022, at 7:40 AM, Amit Kapila wrote:
>
> On Mon, Apr 11, 2022 at 12:39 PM Peter Smith wrote:
> >
> > On Fri, Apr 8, 2022 at 4:12 PM Peter Smith wrote:
> >
> > OK. Added in v7 [1]
>
PSA patch v10 which addresses the remaining review comments from Euler [1]
--
[1]
https://www.postgresql.org/message-id/3cd8d622-6a26-4eaf-a5aa-ac78030e5f50%40www.fastmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
v10-0001-Add-additional-documentation-for-row-filters.patch
On Wed, Apr 13, 2022 at 12:08 AM Euler Taveira wrote:
>
> On Tue, Apr 12, 2022, at 5:30 AM, Peter Smith wrote:
>
> Not changed. Because in fact, I copied most of this sentence
> (including the uppercase, "operations", "and/or") from existing
> documentation
at mean error? Maybe there is a
dependency on it but OTOH it might be annoying - e.g. to disallow a
DROP TABLE when the only dependency was that the user wanted to SKIP
it...
--
Kind Regards,
Peter Smith.
Fujitsu Australia
I've changed the CF entry [1] status to "ready for committer".
--
[1] https://commitfest.postgresql.org/38/3605/
Kind Regards,
Peter Smith.
Fujitsu Australia
here as a reminder that the proto version number
needs to be bumped to 4 in PG16.
~~~
11. src/test/subscription/t/032_circular.pl
Perhaps there should be another test using a third "Node_C" which
publishes some data to Node_B. Then you can ensure that by using
local_only (when Node_A is
On Mon, Apr 18, 2022 at 3:53 PM Amit Kapila wrote:
>
> On Thu, Apr 14, 2022 at 8:55 AM Amit Kapila wrote:
> >
> > On Thu, Apr 14, 2022 at 1:29 AM Euler Taveira wrote:
> > >
> > > On Wed, Apr 13, 2022, at 12:24 AM, Peter Smith wrote:
> > >
> > &
nite recursion in
circular replication setup'
+);
+
+# check that the data published from node_C to node_B is not sent to node_A
+$result = $node_A->safe_psql('postgres', "SELECT * FROM tab_full;");
+is( $result, qq(11
+12),
+ 'Inserted successfully without leading to infinite recursion in
circular replication setup'
+);
+
The new test looked good, but the cut/paste text message ('Inserted
successfully without leading to infinite recursion in circular
replication setup') maybe needs changing because there is nothing
really "circular" about this test case.
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
SET (local_only = false);
DROP SUBSCRIPTION regress_testsub3;
DROP SUBSCRIPTION regress_testsub4;
+-- ok - valid coy_data options
Typo "coy_data". (it looks like this typo is not caused by this patch,
but I think this patch should fix it anyhow).
~~~
25. src/test/regress/sql/subscription.sql - test order
The new tests are OK but IMO they could be re-ordered so then they
will be more consistent for the positive and negative tests.
CURRENT
"true/force/on/1" and "off/false/0"
SUGGEST
"true/on/1/force" and "false/off/0"
--
Kind Regards,
Peter Smith.
Fujitsu Australia
bscriber_contents - misnamed?
This subroutine feels a bit misnamed. It seems to be doing lots of
things like detaching the Node_C and deleting all table data from all
nodes. That all seems quite different from just "clean subscriber
contents".
~~~
13. table initial data?
+clean_subscriber_contents($node_A, $node_B, $node_C);
+
+##
+# Add 3rd node when the new node has some data
+##
But does this test case *really* have some data? I am not so sure.
Doesn't the preceding "clean_subscriber_contents" call remove all the
data that might have been there? That is why I think all the tests out
to have SELECT (previous comment #8) so they can re-confirm what data
is really in those tables before doing each test part.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
79. src/include/replication/logicalproto.h - old extern
@@ -243,8 +243,10 @@ extern TransactionId
logicalrep_read_stream_start(StringInfo in,
extern void logicalrep_write_stream_stop(StringInfo out);
extern void logicalrep_write_stream_commit(StringInfo out,
ReorderBufferTXN *txn,
XLogRecPtr commit_lsn);
-extern TransactionId logicalrep_read_stream_commit(StringInfo out,
+extern TransactionId logicalrep_read_stream_commit_old(StringInfo out,
LogicalRepCommitData *commit_data);
Is anybody still using this "old" function? Maybe I missed it.
==
80. src/include/replication/logicalworker.h
@@ -13,6 +13,7 @@
#define LOGICALWORKER_H
extern void ApplyWorkerMain(Datum main_arg);
+extern void LogicalApplyBgwMain(Datum main_arg);
The new name seems inconsistent with the old one. What about calling
it ApplyBgworkerMain?
==
81. src/test/regress/expected/subscription.out
Isn't this missing some test cases for the new options added? E.g. I
never see streaming value is set to 's'.
==
82. src/test/subscription/t/029_on_error.pl
If options values were changed how I suggested (review comment #14)
then I think a change such as this would not be necessary because
everything would be backward compatible.
--
[1]
https://www.postgresql.org/message-id/CALDaNm2Fe%3Dg4Tx-DhzwD6NU0VRAfaPedXwWO01maNU7_OfS8fw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
hose called "mysecretXXX")
CREATE PUBLICATION foo FOR ALL TABLES (exclude_match = '(.*\.mysecret.*)');
~~
e.g.2. Only allow certain tables.
// ONLY publish my tables (those called "mytableXXX")
CREATE PUBLICATION foo FOR ALL TABLES (subset_match = '(.*\.mytable.*)');
// So following is equivalent to FOR ALL TABLES IN SCHEMA s1
CREATE PUBLICATION foo FOR ALL TABLES (subset_match = '(s1\..*)');
--
Kind Regards,
Peter Smith.
Fujitsu Australia
orce"));
> +
>
> The errmsg seems way too long for the source code. Can you use string
> concatenation or continuation chars to wrap the message over multiple
> lines?
Vignesh: I had seen that the long error message elsewhere also is in a
single line. I think we should keep it as it is to maintain the coding
standard. Thoughts?
OK, if you say it is already common practice then it's fine by me to
leave it as-is.
~~~
2.21 src/test/regress/expected/subscription.out - make check
make check fails.
1 of 214 tests failed.
2.21.a It looks like maybe you did not update the expected ordering of
some of the tests, after some minor adjustments in subscriprion.sql in
v10. So the expected output needs to be fixed in the patch.
2.21.b. Suggest adding this patch to CF so that the cfbot can pick up
such test problems earlier.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ion),
although we plan to implement it entirely using PostgreSQL’s PUB/SUB.
4.0 ACKNOWLEDGEMENTS
The following people have contributed to this proposal – Hayato
Kuroda, Vignesh C, Peter Smith, Amit Kapila.
5.0 REFERENCES
[1]
https://www.postgresql.org/message-id/flat/CALDaNm0gwjY_4HFxvvty01BOT01q_fJLKQ3pWP
, $node_C_connstr,
+ 'tap_pub_C', 'copy_data = force, local_only = on');
+create_subscription($node_B, $node_C, $subname_BC, $node_C_connstr,
+ 'tap_pub_C', 'copy_data = force, local_only = on');
+
Because the Node_C does not yet have any subscriptions aren't these
cases where you didn't really need to use "force"?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
"two_phase"
and "copy_data" but not for "password_required" (??)
==
Kind Regards,
Peter Smith
Fujitsu Australia
On Sat, Oct 14, 2023 at 5:57 AM Jeff Davis wrote:
>
> On Fri, 2023-10-13 at 11:18 +0200, Benoit Lobréau wrote:
> > I tried adding a section
Thanks for pushing the 0001 patch! I am unsure what the decision was
for the remaining patches, but anyway, here they are again (rebased).
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v6-0002-Change-ids-for-CREATE-SUBSCRIPTION-parameters.patch
Description: Binary data
v6-0001-Change
==
Kind Regards,
Peter Smith.
Fujitsu Australia
from the previous tests)/
~
8b.
Can't you combine all those SQL in the same $old_publisher->safe_psql.
~~~
9.
+
+# Actual run, successful upgrade is expected
+command_ok(
+ [
+ 'pg_upgrade', '--no-sync',
+ '-d', $old_publisher->data_dir,
+ '-D', $new_publisher->data_dir,
+ '-b', $bindir,
+ '-B', $bindir,
+ '-s', $new_publisher->host,
+ '-p', $old_publisher->port,
+ '-P', $new_publisher->port,
+ $mode,
+ ],
+ 'run of pg_upgrade of old cluster');
Now that the "Dry run" part is removed, it seems unnecessary to say
"Actual run" for this part.
SUGGESTION
# pg_upgrade should be successful.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
s been supported only since PG17.
if ($old_publisher->pg_version >= 17)
{
test_upgrade_from_PG17_and_later($old_publisher, $new_publisher, $mode);
}
else
{
test_upgrade_from_pre_PG17($old_publisher, $new_publisher, $mode);
}
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Test according to the major version of the old cluster.
+# Upgrading logical replication slots has been supported only since PG17.
+if ($old_publisher->pg_version->major >= 17)
This comment seems wrong IMO. I think we always running the latest
version of pg_upgrade so slot migration is always "supported" from now
on. IIUC you intended this comment to be saying something about the
old_publisher slots.
BEFORE
Upgrading logical replication slots has been supported only since PG17.
SUGGESTION
Upgrading logical replication slots from versions older than PG17 is
not supported.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi.
I happened upon a function comment referring to non-existent code
(that code was moved to another location many years ago).
Probably better to move that comment too. Thoughts?
PSA.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Relocate-old-comment.patch
Description: Binary
On Thu, Oct 19, 2023 at 2:55 PM Tom Lane wrote:
>
> Richard Guo writes:
> > On Thu, Oct 19, 2023 at 10:35 AM Peter Smith wrote:
> >> I happened upon a function comment referring to non-existent code
> >> (that code was moved to another location many years ago).
>
On Thu, Oct 19, 2023 at 3:26 PM Vik Fearing wrote:
>
> On 10/19/23 06:17, Peter Smith wrote:
> >> In short, maybe the whole comment should just be
> >>
> >> /*
> >> * boolin - input function for type boolean
> >> */
> >>
>
On Fri, Oct 20, 2023 at 2:31 AM Tom Lane wrote:
>
> Peter Smith writes:
> > PSA v2.
>
> Pushed.
>
Thanks for pushing.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
l_senders=5' and 'max_connections=10'
# PG_10: 'max_wal_senders=5' and 'max_connections=10'
# Everything else: 'max_wal_senders=10' and 'max_connections=10'
#
# The following code is needed to make adjustments for versions not
already being handled by Cluster.pm.
~
4c.
Alternatively, make necessary adjustments in the Cluster.pm to set
appropriate defaults for all older versions. Then probably you can
remove all this code entirely.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
and if so, skip
+ * this slot.
+ */
+ if (list_member_oid(database_oids_list, dboid))
+ continue;
Simplify the comment
SUGGESTION
Skip this slot if the database OID is already in the list.
======
src/backend/utils/activity/wait_event_names.txt
24.
+REPL_SLOTSYNC_MAIN "Waiting in main loop of slot-sync worker."
+REPL_SLOTSYNC_PRIMARY_CATCHUP "Waiting for primary to catch-up, in
slot-sync worker."
(this was already mentioned in the general review comment)
s/primary/the primary/
==
src/include/postmaster/bgworker_internals.h
25.
#define MAX_PARALLEL_WORKER_LIMIT 1024
+#define MAX_SLOTSYNC_WORKER_LIMIT 50
This constant seems to be not used anywhere except in guc_tables.c
where the GUC is defined. IMO you should make use of this in some
Assert or a message; Otherwise, might as well just remove it and
hardwire the 50 in the guc_tables.c directly.
==
src/include/replication/walreceiver.h
26. WalRcvFailoverSlotsData
+/*
+ * Failover logical slots dbids received from remote.
+ */
+typedef struct WalRcvFailoverSlotsData
+{
+ Oid dboid;
+} WalRcvFailoverSlotsData;
+
For now, the only data is `dbids` but maybe one day there will be more
stuff, so make the struct comment more generic.
SUGGESTION
Failover logical slots data received from remote.
==
src/include/replication/worker_internal.h
27. LogicalRepWorkerType
+
+typedef struct LogicalRepWorker
+{
+ LogicalWorkerHeader hdr;
+
+ /* What type of worker is this? */
+ LogicalRepWorkerType type;
+
Maybe add some struct-level comments for this.
==
[1]
https://www.postgresql.org/message-id/CAFPTHDaqn%2Bm47_vkAToQD6Pe8diut0F0g0bSr8PdcuW6cbSSkQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
alEnd)));
+ return;
+ }
Would elog be better here than using ereport(LOG, errmsg_internal...);
IIUC it does the same thing?
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPspseC03Fhsi%3DOqOtksagspE%2B0MVOhrhhUb64cc_4SE1w%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
2. If the node names will be shortened they should still retain *some*
meaning if possible:
old_publisher/subscriber/new_publisher --> node1/node2/node3 (means
nothing without studying the tests)
old_publisher/subscriber/new_publisher --> alpha/bravo/charlie (means
nothing without studying the tests)
How about:
old_publisher/subscriber/new_publisher --> node_p1/node_s/node_p2
or similar...
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ommand line.
+ /*
+ * The logical replication slots shouldn't be invalidated as
+ * max_slot_wal_keep_size GUC is set to -1 during the upgrade.
+ *
+ * The following is just a sanity check.
+ */
Given that, I felt a more relevant msg/hint might be like:
errmsg("\"max_slot_wal_keep_size\" must be set to -1 during the upgrade"),
errhint("Do not override \"max_slot_wal_keep_size\" using command line
options."));
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Thanks for pushing!
==
Kind Regards,
Peter Smith.
Fujitsu Australia
==
Kind Regards,
Peter Smith.
Fujitsu Australia
st be set to -1 during the upgrade"));
- }
+ Assert (!*invalidated || !SlotIsLogical(s) || !IsBinaryUpgrade);
IMO new Assert became trickier to understand than the original condition. YMMV.
SUGGESTION
Assert(!(*invalidated && SlotIsLogical(s) && IsBinaryUpgrade));
==
Kind Regards,
Peter Smith.
Fujitsu Australia
other error messages where they are sometimes quoted and
sometimes not quoted:
e.g. Not quoted -- errhint("You might need to run fewer transactions
at a time or increase max_connections.")));
e.g. Quoted -- errmsg("\"max_wal_size\" must be at least twice
\"wal_segment_size\"")));
Ideally, they should all look the same everywhere, shouldn't they?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Nov 1, 2023 at 8:02 PM Peter Smith wrote:
...
>
> I had intended to make a patch to address the inconsistency, but
> couldn't decide which of those styles was the preferred one.
>
> Then I worried this could be the tip of the iceberg -- GUC names occur
> in many oth
On Thu, Nov 2, 2023 at 1:25 AM Tom Lane wrote:
>
> Daniel Gustafsson writes:
> > On 1 Nov 2023, at 10:22, Peter Smith wrote:
> >> One idea to achieve consistency might be to always substitute GUC
> >> names using a macro.
> >>
> >> #define G
204291032.qfvyuqxkjnjw%40alvherre.pgsql
[3] Peter -
https://www.postgresql.org/message-id/CAHut%2BPu6Knwooc_NckMxszGrAJnytgpMadtoJ-OA-SFWT%2BGFYw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Austalia
On Thu, Nov 2, 2023 at 1:58 PM Kyotaro Horiguchi
wrote:
>
> Thanks you for the comments!
>
> At Wed, 1 Nov 2023 18:08:19 +1100, Peter Smith wrote
> in
> > Hi, here are some minor review comments for the v3 patch.
> >
> > ==
> > src/ba
On Thu, Nov 2, 2023 at 2:25 PM Peter Smith wrote:
>
> On Thu, Nov 2, 2023 at 1:58 PM Kyotaro Horiguchi
> wrote:
> >
> > Thanks you for the comments!
> >
> > At Wed, 1 Nov 2023 18:08:19 +1100, Peter Smith
> > wrote in
> > > Hi, here
o include a test case for getting an ERROR if the user tries
to override that GUC?
==
[1]
https://www.postgresql.org/message-id/20231027.115759.2206827438943188717.horikyota.ntt%40gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Nov 3, 2023 at 1:11 PM Amit Kapila wrote:
>
> On Thu, Nov 2, 2023 at 2:36 PM Amit Kapila wrote:
> >
> > On Thu, Nov 2, 2023 at 11:32 AM Michael Paquier wrote:
> > >
> > > On Thu, Nov 02, 2023 at 02:32:07PM +1100, Peter Smith wrote:
> > >
--
31a.
/relation/relations/
~
31b.
Do you think that comment is correct? All you are doing here is
allowing the old_sub to proceed because there is no longer any
conflict -- but isn't that just normal pub/sub behaviour that has
nothing to do with pg_upgrade?
~~~
32.
+# Stop the old subscriber, insert a row in each table while it's down and add
+# t2 to the publication
/in each table/in each publisher table/
Also, it is not each table -- it's only t1 and t2; not tab_primary_key.
~~~
33.
+ $new_sub->safe_psql('postgres', "SELECT count(*) FROM pg_subscription_rel");
+is($result, qq(2), "There should be 2 rows in pg_subscription_rel");
/2 rows in pg_subscription_rel/2 rows in pg_subscription_rel
(representing t1 and tab_primary_key)/
==
34. binary_upgrade_create_sub_rel_state
+{ oid => '8404', descr => 'for use by pg_upgrade (relation for
pg_subscription_rel)',
+ proname => 'binary_upgrade_create_sub_rel_state', proisstrict => 'f',
+ provolatile => 'v', proparallel => 'u', prorettype => 'void',
+ proargtypes => 'text oid char pg_lsn',
+ prosrc => 'binary_upgrade_create_sub_rel_state' },
As mentioned in a previous review comment #9, I felt this function
should have a different name: binary_upgrade_add_sub_rel_state.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
e or directory
Instead, it is found in the VPATH folder
[postgres@CentOS7-x64 pg_upgrade]$ pwd
/home/postgres/vpath_dir/src/bin/pg_upgrade
[postgres@CentOS7-x64 pg_upgrade]$ ls tmp_check/
delete_old_cluster.sh log results
==
Kind Regards,
Peter Smith.
Fujitsu Australia
FWIW, I am halfway through doing regex checking of the PG16 source for
all GUC names in messages to see what current styles are in use today.
Not sure if those numbers will influence the decision.
I hope I can post my findings today or tomorrow.
==
Kind Regards,
Peter Smith.
Fujitsu
stent state after "
"an operating system or hardware crash.")
~
I believe the word should have been "ensures"; not "insures".
In passing I found/fixed a bunch of similar misuses in comments.
==
Kind Regards,
Peter Smith.
Fujitsu Australia.
v1-0001-ensure-not-insure.patch
Description: Binary data
On Wed, Nov 8, 2023 at 7:40 AM Peter Smith wrote:
>
> FWIW, I am halfway through doing regex checking of the PG16 source for
> all GUC names in messages to see what current styles are in use today.
>
> Not sure if those numbers will influence the decision.
>
> I hope I can p
all traces of those GUCs had
been eliminated.
==
[1]
https://github.com/postgres/postgres/commit/15c121b3ed7eb2f290e19533e41ccca734d23574#diff-65c699b5d467081e780d255ea0ed7d720b5bca2427e300f9fd0776bffe51560a
Kind Regards,
Peter Smith.
Fujitsu Australia
quot;, "max_worker_processes")));
errhint("You may need to increase max_worker_processes.")));
errhint("You might need to increase %s.", "max_logical_replication_workers")));
~
The most common pattern there is "You might need to increase %s.".
Here is a patch to modify those other similar variations so they share
that common wording.
PSA.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Use-a-common-message-for-increasing-a-GUC.patch
Description: Binary data
,
state char [,sublsn pg_lsn])
~
Looks like v12 accidentally forgot to update this to the modified
function name 'binary_upgrade_add_sub_rel_state'
==
Kind Regards,
Peter Smith.
Fujitsu Australia
o reason
why they would want to) then at least the hook will give an error,
rather than us silently overwriting the user's value with -1.
So, patch v4 LGTM, except it is better to include a test case.
~
Meanwhile, if preventing the apply worker launch is considered better
to be implemented differently in ApplyLauncherRegister, then so be it.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
+ * sync worker spawned after the upgrade because the subscription ID
+ * tracked by the publisher does not match anymore.
missing whitespace
/SUBREL_STATE_DATASYNC:A relation/SUBREL_STATE_DATASYNC: A relation/
==
Kind Regards,
Peter Smith.
Fujitsu Australia
'is_logical' instead of
'is_physical', so the ereport substitution will match the other
ReplicationSlotAcquirecode suggested above (#2a).
~
SUGGESTION (For #3a and #3b)
if (am_walsender)
{
slotname = pstrdup(NameStr(slot->data.name));
is_logical = SlotIsLogical(slot);
}
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Nov 9, 2023 at 8:12 PM Daniel Gustafsson wrote:
>
> > On 9 Nov 2023, at 00:51, Peter Smith wrote:
> >
> > Hi,
> >
> > I happened to notice that some GUC names "max_fsm_pages" and
> > "max_fsm_relations" are still mentioned i
it better to be consistent with
the existing code?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Patch v15-0001 LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
d logical replication slot \"%s\""
: "acquired physical replication slot \"%s\"",
NameStr(s->data.name)));
~~~
2.
Ditto for the other ereport.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ad of "ready state"?
~~~
13. TEST: when the subscription's replication origin does not exist.
+$old_sub2->safe_psql('postgres',
+ "ALTER SUBSCRIPTION regress_sub2 disable");
/disable/DISABLE/
~~~
14.
+my $subid = $old_sub2->safe_psql('postgres',
+ "SELECT oid FROM pg_subscription WHERE subname = 'regress_sub2'");
+my $reporigin = 'pg_'.qq($subid);
+$old_sub2->safe_psql('postgres',
+ "SELECT pg_replication_origin_drop('$reporigin')"
+);
Maybe this part needs a comment to say the reason why the origin does
not exist -- it's because you found and explicitly dropped it.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
s better.
>
IIUC, that example is different because "SIGABRT" / "SIGKILL" are not
real words, so you don't want the translator to attempt to translate
them.You want them to appear in the message as-is.
OTOH in this patch "logical" and "physical" are just normal English
words that should be translated as part of the original message.
e.g. like in these similar messages:
- msgid "database \"%s\" is used by an active logical replication slot"
- msgstr "la base de données « %s » est utilisée par un slot de
réplication logique actif"
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Nov 16, 2023 at 12:18 PM Amit Kapila wrote:
>
> On Thu, Nov 16, 2023 at 3:48 AM Peter Smith wrote:
> >
> > ~
> >
> > SUGGESTION (#1a and #1b)
> >
> > ereport(log_replication_commands ? LOG : DEBUG1,
> > errmsg(SlotIsLogical(s)
&g
if there's a subscription with tables
+# in a state different than 'r' (ready), 'i' (init) and 's' (synchronized)
+# and/or b) if the subscription does not have a replication origin.
+# --
14a,
/does not have a/has no/
~
14b.
Maybe put a) and b) on newlines to be more readable.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
*/
{
...
}
~~~
PSA a small patch for making this change.
(I ran make check-world after this change and it was successful)
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Simplify-if-else-logic-of-walsender-CreateReplica.patch
Description: Binary data
LED 'e'
#define LOGICALREP_FAILOVER_STATE_DISABLED 'd'
#define LOGICALREP_FAILOVER_STATE_PENDING 'p'
#define LOGICALREP_FAILOVER_STATE_ENABLED 'e'
~
54a.
There should either be another comment (like the 'two_phase tri-state'
one) added for the FAILOVER states or that existing comment should be
expanded so that it also mentions the 'failover' tri-states.
~
54b.
Idea: If you are willing to change the constant names (not the values)
of the current tri-states then now both the 'two_phase' and 'failover'
could share them -- I also think this might give the ability to create
macros (if wanted) or to share more code instead of always handling
failover and two_phase separately.
SUGGESTION
#define LOGICALREP_TRISTATE_DISABLED 'd'
#define LOGICALREP_TRISTATE_PENDING 'p'
#define LOGICALREP_TRISTATE_ENABLED 'e'
~
54c.
The header comment at the top of worker.c should give more details
about the 'failover' tri-state. (also mentioned in another review
comment)
~~~
55. FormData_pg_subscription
+ char subfailoverstate; /* Enable Failover State */
+
/Enable Failover State/Failover state/
==
src/include/replication/slot.h
56.
+
+ /*
+ * Is this a failover slot (sync candidate for physical standbys)?
+ * Relevant for logical slots on the primary server.
+ */
+ bool failover;
} ReplicationSlotPersistentData;
~
/Relevant/Only relevant/
==
src/include/replication/walreceiver.h
57.
+#define walrcv_create_slot(conn, slotname, temporary, two_phase,
failover, snapshot_action, lsn) \
+ WalReceiverFunctions->walrcv_create_slot(conn, slotname, temporary,
two_phase, failover, snapshot_action, lsn)
double whitespace after the 'failover' parameter?
==
src/include/replication/walsender_private.h
58.
ConditionVariable wal_flush_cv;
ConditionVariable wal_replay_cv;
+ ConditionVariable wal_confirm_rcv_cv;
Should this new field have a comment? Or should it be grouped with the
2 preceding fields (if that same group comment is valid for all of
them)?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ng.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
' because the subscription is not yet refreshed.
Typo or missing word in comment?
"The upgraded" ??
==
Kind Regards,
Peter Smith.
Fujitsu Australia
x27;t', "subscriber1 doesn't get data from primary until
standby1 acknowledges changes");
Might it be better to write as "SELECT count(*) = $primary_row_count
FROM tab_int;" and expect it to return false?
==
src/test/regress/expected/subscription.out
7.
Everything
here was some problem and cause an ereport or
elog. In practice, maybe it makes no difference, but it did seem
slightly dubious to me to assign to a global before determining
everything is OK. Thoughts?
~~~
Anyway, PSA a patch for the 1-3 above.
==
[1] https://www.postgresql.org/message-id/flat/2694.1700471273%40antos
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-replorigin_session_setup-refactoring.patch
Description: Binary data
t;dbarr.dbs[dbnum];
+ PGconn*conn = connectToServer(cluster, active_db->db_name);
There seems no need for an extra variable ('cluster') here when you
can just reference 'old_cluster' directly in the code, the same as
other functions in this file do all the time.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Nov 9, 2023 at 10:04 PM Alvaro Herrera wrote:
>
> On 2023-Nov-09, Peter Smith wrote:
>
> > Notice that NOT QUOTED is the far more common pattern, so my vote
> > would be just to standardise on making everything this way. I know
> > there was some concern raised
(init) state and/or
+# b) if the subscription has no replication origin.
+# --
/if there's a subscription with tables in a state different than 'r'
(ready) or 'i' (init) state and/if there's a subscription with tables
On Fri, Nov 24, 2023 at 2:11 PM Michael Paquier wrote:
>
> On Thu, Nov 23, 2023 at 06:27:04PM +1100, Peter Smith wrote:
> > There may be some changes I've missed, but hopefully, this is a nudge
> > in the right direction.
>
> Thanks for spending some time on that
On Fri, Nov 24, 2023 at 8:53 PM Alvaro Herrera wrote:
>
> On 2023-Nov-24, Michael Paquier wrote:
>
> > On Thu, Nov 23, 2023 at 06:27:04PM +1100, Peter Smith wrote:
> > > There may be some changes I've missed, but hopefully, this is a nudge
> > > in the right
ndition mean some kind of impossible internal error
occurred -- i.e. should this be elog instead of ereport?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, Nov 27, 2023 at 12:44 PM Michael Paquier wrote:
>
> On Mon, Nov 27, 2023 at 10:04:35AM +1100, Peter Smith wrote:
> > On Fri, Nov 24, 2023 at 8:53 PM Alvaro Herrera
> > wrote:
> >> Yeah. Also, these could be changed to have the GUC name outside the
>
tor,
+ MAIN_FORKNUM));
How about using the macro: OidIsValid
~~~
11. ReorderBufferChangeSize
+ if (tup)
+ {
+ sz += sizeof(HeapTupleData);
+ len = tup->tuple.t_len;
+ sz += len;
+ }
Why is the 'sz' increment split into 2 parts?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ch version)
==
[1] https://www.postgresql.org/message-id/ZWQVxu8zWIx64V7l%40paquier.xyz
Kind Regards,
Peter Smith.
Fujitsu Australia
v3-0001-GUC-names-docs.patch
Description: Binary data
v3-0003-GUC-names-maybe-add-quotes.patch
Description: Binary data
v3-0002-GUC-names-fix-quotes.patch
D
amp;
loc <= RecentFlushPtr)
- return RecentFlushPtr;
+ {
+ WalSndFilterStandbySlots(RecentFlushPtr, &standby_slots);
It is better to use XLogRecPtrIsInvalid macro here. I know it was not
strictly added by your patch, but so much else changed nearby so I
thought this should be fixed at the same time.
==
src/bin/pg_upgrade/info.c
17. get_old_cluster_logical_slot_infos
+
slotinfos = (LogicalSlotInfo *) pg_malloc(sizeof(LogicalSlotInfo) *
num_slots);
Excessive whitespace.
==
[1] My previous review of v35-0001.
https://www.postgresql.org/message-id/CAHut%2BPv-yu71ogj_hRi6cCtmD55bsyw7XTxj1Nq8yVFKpY3NDQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
N regress_sub1 DISABLE");
+$old_sub->safe_psql('postgres',
+ "ALTER SUBSCRIPTION regress_sub1 SET (slot_name = none)");
+$old_sub->safe_psql('postgres', "DROP SUBSCRIPTION regress_sub1");
+
Probably it would be tidier to combine all of those.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
601 - 700 of 1980 matches
Mail list logo
