On Thu, Nov 30, 2023 at 12:06 PM Peter Smith wrote:
>
> Here are some review comments for patch v20-0001
>
> 3.
> +# The subscription's running status should be preserved
> +my $result =
> + $new_sub->safe_psql('postgres',
> + "SELECT subenabled FR
On Wed, Nov 29, 2023 at 11:45 PM Tomas Vondra
wrote:
>
>
>
> On 11/27/23 23:06, Peter Smith wrote:
> > FWIW, here are some more minor review comments for v20231127-3-0001
> >
> > ==
> > .../replication/logical/reorderbuffer.c
> >
> >
alsenders?
e.g.
"Wake up logical walsenders..." --> "Wake up the logical walsender processes..."
======
[1] v35-0001 review.
https://www.postgresql.org/message-id/CAHut%2BPv-yu71ogj_hRi6cCtmD55bsyw7XTxj1Nq8yVFKpY3NDQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Nov 30, 2023 at 4:59 PM Michael Paquier wrote:
>
> On Tue, Nov 28, 2023 at 11:54:33AM +1100, Peter Smith wrote:
> > Here is patch set v3.
> >
> > Patches 0001 and 0002 are unchanged from v2.
>
> After some grepping, I've noticed that 0002 had a mistake
N regress_sub4 CONNECTION '$connstr' PUBLICATION
regress_pub3 WITH (enabled=false)"
+);
+
+my $subid = $old_sub->safe_psql('postgres',
+ "SELECT oid FROM pg_subscription WHERE subname = 'regress_sub4'");
+my $reporigin = 'pg_' . qq($subid);
+
+# Drop the subscription's replication origin
+$old_sub->safe_psql('postgres',
+ "SELECT pg_replication_origin_drop('$reporigin')");
+
+$old_sub->stop;
14a.
IMO better to have all this without blank lines, because it all
belongs to the first comment.
~
14b.
That 2nd comment "# Drop the..." is not required because the first
comment already says the same.
==
src/include/catalog/pg_subscription_rel.h
15.
extern void AddSubscriptionRelState(Oid subid, Oid relid, char state,
- XLogRecPtr sublsn);
+ XLogRecPtr sublsn, bool upgrade);
Shouldn't this 'upgrade' really be 'binary_upgrade' so it better
matches the comment you added in that function?
If you agree, then change it here and also in the function definition.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Sync initiated for the slot but
+ * not completed yet, waiting for
+ * the primary server to catch-up */
+#define SYNCSLOT_STATE_READY 'r' /* Initialization complete, ready
+ * to be synced further */
Already questioned the same elsewhere. IIUC the same tri-state values
of other attributes might be used here too without needing to
introduce 3 new values.
e.g.
#define SYNCSLOT_STATE_DISABLED 'd' /* No syncing for this slot */
#define SYNCSLOT_STATE_PENDING 'p' /* Sync is enabled but we must
wait for the primary server to catch up */
#define SYNCSLOT_STATE_ENABLED 'e' /* Sync is enabled and the slot is
ready to be synced */
~~~
79.
+ /*
+ * Is this a slot created by a sync-slot worker?
+ *
+ * Relevant for logical slots on the physical standby.
+ */
+ char sync_state;
+
I assumed that "Relevant for" means "Only relevant for". It should say that.
If correct, IMO a better field name might be 'standby_sync_state'
==
src/test/recovery/t/050_verify_slot_order.pl
80.
+$backup_name = 'backup2';
+$primary->backup($backup_name);
+
+# Create standby3
+my $standby3 = PostgreSQL::Test::Cluster->new('standby3');
+$standby3->init_from_backup(
+ $primary, $backup_name,
+ has_streaming => 1,
+ has_restoring => 1);
The mixture of 'backup2' for 'standby3' seems confusing. Is there a
reason to call it backup2?
~~~
81.
+# Verify slot properties on the standby
+is( $standby3->safe_psql('postgres',
+ q{SELECT failover, sync_state FROM pg_replication_slots WHERE
slot_name = 'lsub1_slot';}
+ ),
+ "t|r",
+ 'logical slot has sync_state as ready and failover as true on standby');
It might be better if the message has the same order as the SQL. Eg.
"failover as true and sync_state as ready".
~~~
82.
+# Verify slot properties on the primary
+is( $primary->safe_psql('postgres',
+q{SELECT failover, sync_state FROM pg_replication_slots WHERE
slot_name = 'lsub1_slot';}
+ ),
+ "t|n",
+ 'logical slot has sync_state as none and failover as true on primary');
+
It might be better if the message has the same order as the SQL. Eg.
"failover as true and sync_state as none".
~~~
83.
+# Test to confirm that restart_lsn of the logical slot on the primary
is synced to the standby
IMO the major test parts (like this one) may need more highlighting "#
-" so those comments don't get lost among all the
other comments.
~~~
84.
+# let the slots get synced on the standby
+sleep 2;
Won't this make the test prone to failure on slow machines? Is there
not a more deterministic way to wait for the sync?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
;%c' AS subfailoverstate\n",
+ LOGICALREP_FAILOVER_STATE_DISABLED);
+
That first appended string should include the table alias same as all
the nearby code does.
e.g. " subfailoverstate\n" should be " s.subfailoverstate\n"
==
Kind Regards,
Peter Smith.
Fujitsu Australia
github.com/postgres/postgres/commit/a243569bf65c5664436e8f63d870b7ee9c014dcb
[2] The case should match pg_settings -
https://www.postgresql.org/message-id/db3e4290ced77111c17e7a2adfb1d660734f5f78.camel%40cybertec.at
[3] Dislike of diff churn -
https://www.postgresql.org/message-id/ZWUd8dYYA9v83KvI%40paquier.xyz
Kind Regards,
Peter Smith.
Fujitsu Australia
v4-0002-GUC-names-use-mixed-case-for-intervalstyle-error-.patch
Description: Binary data
v4-0001-GUC-names-use-mixed-case-for-datestyle-in-message.patch
Description: Binary data
On Sat, Dec 9, 2023 at 1:48 AM Peter Eisentraut wrote:
>
> On 08.12.23 05:10, Peter Smith wrote:
> > Patch 0001 -- "datestyle" becomes DateStyle in messages
> > Rebased this again, which was part of an earlier patch set
> > - I think any GUC names documented
This v5* looks good to me, except it will need some further
modification if PeterE's suggestion [1] to keep quotes for the
MixedCase GUCs is adopted.
==
[1]
https://www.postgresql.org/message-id/9e7802b2-2cf2-4c2d-b680-b2ccb9db1d2f%40eisentraut.org
Kind Regards,
Peter Smith.
Fu
../patches_misc/v44-0002-Add-logical-slot-sync-capability-to-the-physical.patch
error: patch failed: src/test/recovery/t/050_standby_failover_slots_sync.pl:289
error: src/test/recovery/t/050_standby_failover_slots_sync.pl: patch
does not apply
==
Kind Regards,
Peter Smith.
Fujitsu Australia
0 FROM tab_int;");
+is($result, 't',
+ "subscriber1 gets data from primary after standby1 is removed from
the standby_slot_names list"
+);
/primary must/the primary must/
(continuing the suggestion from the previous review comment)
Now this SQL can use the variable too:
subscriber1->safe_psql('postgres', "SELECT count(*) =
$primary_row_count FROM tab_int;");
~~~
12.
+
+# Create another subscription enabling failover
+$subscriber1->safe_psql('postgres',
+ "CREATE SUBSCRIPTION regress_mysub3 CONNECTION '$publisher_connstr' "
+ . "PUBLICATION regress_mypub WITH (slot_name = lsub3_slot,
copy_data=false, failover = true, create_slot = false);"
+);
Maybe give some more information in that comment:
SUGGESTION
Create another subscription (using the same slot created above) that
enables failover.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
# a) the sync-ready('r') slot 'lsub3_slot' is retained on new primary
+# b) the initiated('i') slot 'logical_slot'is dropped on promotion
+# c) logical replication for mysub3 is resumed succesfully after failover
+##
/'logical_slot'is/'logical_slot' is/ (missing space)
/succesfully/successfully/
~~~
38.
+# Update subscription with new primary's connection info
+$subscriber3->safe_psql('postgres', "ALTER SUBSCRIPTION mysub3 DISABLE;");
+$subscriber3->safe_psql('postgres', "ALTER SUBSCRIPTION mysub3
CONNECTION '$standby3_conninfo';");
+$subscriber3->safe_psql('postgres', "ALTER SUBSCRIPTION mysub3 ENABLE;");
Consider combining all those DDLs.
~~~
39.
+
+# Insert data on the new primary
+$standby3->safe_psql('postgres',
+ "INSERT INTO tab_mypub3 SELECT generate_series(11, 20);");
+
+# Confirm that data in tab_mypub3 replicated on subscriber
+is( $subscriber3->safe_psql('postgres', q{SELECT count(*) FROM tab_mypub3;}),
+ "20",
+ 'data replicated from new primary');
Shouldn't there be some wait_for_subscription_sync logic (or similar)
here just to ensure the subscriber3 had time to receive that data
before you immediately check that it had arrived?
==
[1] My v43-0002 review.
https://www.postgresql.org/message-id/CAHut%2BPuuqEpDse5msENsVuK3rjTRN-QGS67rRCGVv%2BzcT-f0GA%40mail.gmail.com
[2] Replies to v43-0002 review.
https://www.postgresql.org/message-id/CAJpy0uDcOf5Hvk_CdCCAbfx9SY%2Bog%3D%3D%3DtgiuhWKzkYyqebui9g%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
he reason */
naptime = 6 * WORKER_INACTIVITY_NAPTIME_MS; /* 60 sec */
}
else
{
/* Normal standby */
...
}
==
[1] review of v45-0002.
https://www.postgresql.org/message-id/CAHut%2BPtOc7J_n24HJ6f_dFWTuD3X2ApOByQzZf6jZz%2B0wb-ebQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
to reload
+ * standby_slot_names since we do not support syncing slots to cascading
+ * standbys.
+ */
+ if (RecoveryInProgress())
+ return;
Should the RecoveryInProgress() check be first -- even before the
ProcessConfigFile call?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
t mode but slightly less robust
==
[1]
https://www.postgresql.org/docs/current/protocol-logicalrep-message-formats.html
Kind Regards,
Peter Smith.
Fujitsu Australia
ps we should fix this too.
>
OK, to deal with that can't you just include "origin" in the first
group which has no special protocol requirements?
SUGGESTION
-proto_version
-publication_names
-binary
-messages
-origin
Requires minimum protocol version 2:
-streaming (boolean)
Requires minimum protocol version 3:
-two_phase
Requires minimum protocol version 4:
-streaming (parallel)
==
[1] 55.4 https://www.postgresql.org/docs/devel/protocol-replication.html
Kind Regards,
Peter Smith.
Fujitsu Australia
I don't like that
> "streaming" appears twice, and I wouldn't know how to format this
> nicely.
>
I won't keep pushing to rearrange the docs. I think all the content is
OK anyway, so let's see if other people have any opinions on how the
new information is best presented.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, Dec 18, 2023 at 11:35 AM Peter Smith wrote:
>
> Hi, I had a look at the latest v02 patches.
>
> On Sat, Dec 16, 2023 at 12:36 AM Emre Hasegeli wrote:
> >
> > > OK, to deal with that can't you just include "origin" in the first
> > &g
ing back to my original thought
that since this state has no meaning for the primary server then
a) why do we even care what potential values it might have there, and
b) isn't it better to call this field 'standby_sync_state' to
emphasize it only has meaning for the standby?
e.g.
SUGGESTION
/*
* Synchronization state for a logical slot.
*
* The standby can have any value among the possible values of 'i','r' and
* 'n'. For the primary, this field value has no meaning.
*/
char standby_sync_state;
==
Kind Regards,
Peter Smith.
Fujitsu Australia
;
The comment is wrong -- it names a different table ('tab_mypub3' ?) to
what the SQL says.
==
[1] My v48-0002 review comments.
https://www.postgresql.org/message-id/CAHut%2BPsyZQZ1A4XcKw-D%3DvcTg16pN9Dw0PzE8W_X7Yz_bv00rQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
RROR: unrecognized pgoutput option: banana
CONTEXT: slot "test_slot_v1", output plugin "pgoutput", in the startup callback
--
But that doesn't seem right. AFAIK elog messages use errmsg_internal
so this message would not get translated.
PSA a patch to fix that.
==
On Tue, Dec 19, 2023 at 6:25 PM Amit Kapila wrote:
>
> On Tue, Dec 19, 2023 at 12:07 PM Peter Smith wrote:
> >
> > On Tue, Dec 19, 2023 at 1:35 AM Emre Hasegeli wrote:
> > >
> > > > Fair enough. I think we should push your first patch only in HEAD as
>
dropped/
~~~
4.
+
+($result, $stdout, $stderr) = $standby1->psql(
+'postgres',
+qq[ALTER_REPLICATION_SLOT lsub1_slot (failover);],
+replication => 'database');
+ok($stderr =~ /ERROR: cannot alter replication slot "lsub1_slot"/,
+ "synced slot on standby cannot be altered");
+
Add a comment for this test part
SUGGESTION
Attempting to alter a synced slot should result in an error
~~~
5.
IMO it would be better if the tests were done in the same order
mentioned in the comment. So either change the tests or change the
comment.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
s v14 onwards. Although the new
error message was only added for HEAD, isn't it still correct to say
"A valid version is required." for all the patches including v12 and
v13?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
(standby_slot_names);
ProcessConfigFile(PGC_SIGHUP);
if (!RecoveryInProgress())
{
if (strcmp(pre_standby_slot_names, standby_slot_names) != 0)
{
list_free(*standby_slots);
*standby_slots = GetStandbySlotList(true);
}
}
pfree(pre_standby_slot_names);
}
==
Kind Regards,
Peter Smith.
Fujitsu
uot;DateStyle")
where is the "you'd need to adjust the quotes" problem there?
==
[1] GUC quoting guidelines --
https://www.postgresql.org/docs/devel/error-style-guide.html
[2] Case in messages should be same as pg_settings --
https://www.postgresql.org/message-id/db3e4290ced77111c17e7a2adfb1d660734f5
UBSCRIPTION ... REFRESH PUBLICATION
+ for e.g.:
+
+node2=# ALTER SUBSCRIPTION sub1_node1_node2 REFRESH PUBLICATION;
+ALTER SUBSCRIPTION
+node2=# ALTER SUBSCRIPTION sub2_node1_node2 REFRESH PUBLICATION;
+ALTER SUBSCRIPTION
+
+
+
42a.
SUGGESTION
Refresh the node1 subscription's publications using...
~
42b.
The example looks wrong. IIUC these commands should be done on node1
but the example shows a node2 prompt.
==
[1]
https://www.postgresql.fastware.com/blog/bi-directional-replication-using-origin-filtering-in-postgresql
[2]
https://www.postgresql.org/message-id/CALDaNm3tv%2BnWMXO0q39EuwzbXEQyF5thT4Ha1PvfQ%2BfQgSdi_A%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
gt; Thought?
During my review, I also felt that step 10 is now so long that it is a
distraction from the other content on this page.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
src/test/regress/sql/subscription.sql
22.
+CREATE SUBSCRIPTION regress_testsub CONNECTION
'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect =
false, failover = true);
+
+\dRs+
This is currently only testing the explicit "failover=true".
Maybe you can also test the other kinds work as expected:
- explicit "SET (failover=false)"
- explicit "SET (failover)" with no value specified
==
[1]
https://github.com/postgres/postgres/commit/14dd0f27d7cd56ffae9ecdbe324965073d01a9ff
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Aug 29, 2023 at 10:35 PM Alvaro Herrera wrote:
>
> On 2023-Aug-29, Amit Kapila wrote:
>
> > On Mon, Aug 28, 2023 at 5:35 AM Peter Smith wrote:
> > >
> > > On Fri, Aug 25, 2023 at 8:15 PM Amit Kapila
> > > wrote:
> > >
> > >
tandbys. */
This comment can say "True if...", so it will be the same as the
earlier CATALOG comment for 'subfailover'.
==
Kind Regards,
Peter Smith.
Fujitsu Australia.
x27;t this typedef name be included in the typedef.list file?
--
[1] https://www.postgresql.org/docs/16/release-16.html
Kind Regards,
Peter Smith.
Fujitsu Australia
= InvalidXLogRecPtr;
^
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Remove-shadows-found-in-logical-replication-files.patch
Description: Binary data
Logical replication slot upgrade is only supported for old_cluster >=
PG17. An explicit version check is not necessary here because function
count_old_cluster_logical_slots() will always return 0 for old_cluster
<= PG16.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
a running old_cluster).
This would demonstrate pg_upgrade working successfully even when the
WAL records are not consumed (because LSN checks would be skipped in
check_old_cluster_for_valid_slots function).
--
Kind Regards,
Peter Smith.
Fujitsu Australia
shadow or -Wshadow=local, you can still see a lot of warnings.
>
> Yep. I've addressed on HEAD the ones proposed on this thread.
> --
Thanks for pushing!
3 gone, ~150 remaining :)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
~
[postgres@CentOS7-x64 oss_postgres_misc]$ cat
son* for the start/stop. So, the last sentence
needs some modification
BEFORE
During the upgrade, clusters are started and stopped several times so
that temporary slots will be removed.
SUGGESTION
During the upgrade, clusters are started and stopped several times
causing any temporary slots to be removed.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
1');});
+
+$node_primary->backup('backup');
+
+$node_phys_standby->init_from_backup($node_primary, 'backup',
has_streaming => 1);
+$node_phys_standby->append_conf('postgresql.conf', q{
+synchronize_slot_names = '*'
+primary_slot_name = 'pslot1'
+hot_standby_feedback = off
+});
+$node_phys_standby->start;
+
+$node_primary->safe_psql('postgres', "CREATE TABLE t1 (a int PRIMARY KEY)");
+$node_primary->safe_psql('postgres', "INSERT INTO t1 VALUES (1), (2), (3)");
The comments seem mostly to describe details about what are the
expectations at each test step.
IMO there also needs to be a larger "overview" comment to describe
more generally *what* this is testing, and *how* it is testing it.
e.g. it is hard to understand the test without being already familiar
with the patch.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
t removed the word "leader". Now there are:
"apply worker"
"parallel apply worker"
"table synchronization worker"
PSA patch v2.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
From bac581d9f6843b3df0dd5fc45e318594a7921ee6 Mon Sep 17 00:00:00 2001
From: Peter Smith
Da
odd way to output the heading. Isn't it better to put
this outside the loop?
SUGGESTION
if (slot_arr->nslots > 0)
pg_log(PG_VERBOSE, "Logical replication slots within the database:");
==
src/bin/pg_upgrade/pg_upgrade.c
13.
+/*
+ * setup_new_cluster()
+ *
+ * Starts a new cluster for updating the wal_level in the control fine, then
+ * does final setups. Logical slots are also created here.
+ */
+static void
+setup_new_cluster(void)
typo
/control fine/control file/
--
Kind Regards,
Peter Smith.
Fujitsu Australia
luster_is_empty() do
> similar thing. Please tell me if it should be reverted...
>
Another possible idea is to show all the WARNINGS but only when in verbose mode.
---
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Sep 6, 2023 at 9:49 AM Nathan Bossart wrote:
>
> On Wed, Sep 06, 2023 at 09:02:21AM +1200, Peter Smith wrote:
> > On Sat, Sep 2, 2023 at 7:41 AM Nathan Bossart
> > wrote:
> >> I see that the table refers to "leader apply workers". Would those show
ol conflicting; /* Is the slot usable? */
+} LogicalSlotInfo;
9a.
+ bool caughtup; /* Is confirmed_flush_lsn the same as latest
+ * checkpoint LSN? */
caughtup ==> caught_up
~
9b.
+ bool conflicting; /* Is the slot usable? */
The field name has the opposite meaning of the wording of the comment.
(e.g. it is usable when it is NOT conflicting, right?).
Maybe there needs a better field name, or a better comment, or both.
AFAICT from other code pg_fatal message 'conflicting' is always
interpreted as 'lost' so maybe the field should be called that?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
be interpreted. Needs
rearranging for clarity.
~
12. start_postmaster
(cluster == &new_cluster) ?
- " -c synchronous_commit=off -c fsync=off -c full_page_writes=off" : "",
+ " -c synchronous_commit=off -c fsync=off -c full_page_writes=off -c
max_slot_wal_keep_size=-1 " :
+ " -c max_slot_wal_keep_size=-1",
Instead of putting the same option on both sides of the ternary, I was
wondering if it might be better to hardwire the max_slot_wal_keep_size
just 1 time in the format string?
==
.../pg_upgrade/t/003_logical_replication_slots.pl
13.
# Remove the remained slot
/remained/remaining/
--
Kind Regards,
Peter Smith.
Fujitsu Australia
*) pg_malloc(sizeof(LibraryInfo) *
(array_size));
Those extra parentheses around "(array_size)" seem overkill.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Sep 8, 2023 at 8:28 AM Nathan Bossart wrote:
>
> On Thu, Sep 07, 2023 at 12:36:29PM +1200, Peter Smith wrote:
> > Modified as suggested. PSA v3.
>
> Thanks. I've attached v4 with a couple of small changes. Notably, I've
> moved the worker_type column to be
On Tue, Sep 12, 2023 at 1:44 PM Michael Paquier wrote:
>
> On Tue, Sep 12, 2023 at 01:07:51PM +1000, Peter Smith wrote:
> > The type is only assigned during worker process launch, and during
> > process cleanup [1]. It's only possible to be UNKNOWN after
> &
would be avoided during the upgrade.
SUGGESTION
This setting prevents the invalidation of slots during the upgrade.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Yesterday noticed a TAP test assignment to an unused $result.
PSA patch to remove that.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-remove-redundant-result-assignment.patch
Description: Binary data
On Thu, Sep 14, 2023 at 7:10 PM Amit Kapila wrote:
>
> > Though it is harmless I think we can clean it up. Your patch looks good to
> > me.
> >
>
> Pushed.
>
Thanks!
--
Kind Regards,
Peter Smith.
Fujitsu Australia
he slot in worker pool to which it is attached */
+ /* Count of Database ids it manages */
+ /* dsa_pointer for database ids it manages */
~
43c.
There is inconsistent wording and case in these comments. Just pick
one term to use everywhere.
"Database ids"
"database ids"
"dbids"
~~~
44. GENERAL = restructuring of common structs in worker_internal.h
The field name 'header' is propagated all over the place. It is OK,
and I guess there is no choice, but IMO calling it 'hdr' instead of
'header' might be slightly less intrusive. I think there are lots of
precedents for calling headers as 'hdr'.
==
src/include/storage/lwlock.h
==
src/tools/pgindent/typedefs.list
45.
Missing the the typedef WorkerHeader?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
perhaps
leaving the stats attribute as "table synchronization" (and some
future patch would add "sequence synchronization") is better.
TBH, I am not sure which option is best, so I am happy to go with the consensus.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
quot; and leave it at that.
Changing every other place to consistently say "leader apply" is a
bigger task for another thread because we will find lots more places
to change. For example, there are messages like: "logical replication
apply worker for subscription \"%s\" has started" that perhaps should
say "logical replication leader apply worker for subscription \"%s\"
has started". Such changes don't belong in this stats patch.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Sep 19, 2023 at 1:20 AM Nathan Bossart wrote:
>
> On Mon, Sep 18, 2023 at 04:56:46PM +1000, Peter Smith wrote:
> > On Mon, Sep 18, 2023 at 1:43 PM Amit Kapila wrote:
> >> One simple idea to reduce confusion could be to use (leader) in the
> >> above two plac
the structure fields where possible.
--
[1] https://www.postgresql.org/message-id/ZQk1Ca_eFDTmBiZy%40paquier.xyz
Kind Regards,
Peter Smith.
Fujitsu Australia
change.
>
One question -- the patch comment still says "Bumps catversion.", but
catversion.h change is missing from the v9 patch?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Sep 21, 2023 at 9:34 AM Nathan Bossart wrote:
>
> On Thu, Sep 21, 2023 at 08:14:23AM +0900, Michael Paquier wrote:
> > On Thu, Sep 21, 2023 at 09:01:01AM +1000, Peter Smith wrote:
> >> One question -- the patch comment still says "Bumps catversion.",
apply
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Thanks for pushing.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
s non-superuser the apply worker should be restarted and
+# it should fail with 'password is required' error as password option is not
+# part of the connection string.
/as password option/because the password option/
==
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Tue, Sep 26, 2023 at 11:57 PM vignesh C wrote:
>
> On Tue, 26 Sept 2023 at 13:03, Peter Smith wrote:
> >
> > Here are some comments for patch v2-0001.
> >
> > ==
> > src/backend/replication/logical/worker.c
> >
> > 1. maybe_reread_subsc
synchronization/slot synchronization/ (??) -- there are multiple of these
~
34.
+ /*
+ * Connect to the database specified by user in PrimaryConnInfo. We need
+ * database connection for walrcv_exec to work. Please see comments atop
+ * libpqrcv_exec.
+ */
/database connection/a database connection/
~~~
35.
+ /* Reconnect if primary_conninfo got changed */
+ if (config_reloaded)
+ wrconn = reconnect_if_needed(wrconn, conninfo_prev);
SUGGESTION
Reconnect if GUC primary_conninfo has changed.
~
36.
+ /*
+ * The slot-sync worker must not get here because it will only stop when
+ * it receives a SIGINT from the logical replication launcher, or when
+ * there is an error. None of these cases will allow the code to reach
+ * here.
+ */
+ Assert(false);
36a.
/must not/cannot/
36b.
"None of these cases will allow the code to reach here." <-- redundant sentence
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ill be
replaced with question marks (?).
SUGGESTION
... and anything other than printable ASCII characters will be
replaced with C-style
escaped hexadecimal byte values.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ASCII characters are replaced with C-style escaped hexadecimal byte
+ values.
IIUC all of these 3 places can have exactly the same wording change
(e.g. like Karl's last suggestion [1]).
SUGGESTION
Any bytes that are not printable ASCII characters are replaced with
C-style escaped hexadecimal
byte values.
==
[1]
https://www.postgresql.org/message-id/20230927085924.4198c3d2%40slate.karlpinc.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Sep 28, 2023 at 10:30 AM Karl O. Pinc wrote:
>
> On Thu, 28 Sep 2023 09:49:03 +1000
> Peter Smith wrote:
>
> > On Wed, Sep 27, 2023 at 11:59 PM Karl O. Pinc
> > wrote:
> > >
> > > On Wed, 27 Sep 2023 12:58:54 +
> > > "Hay
On Thu, Sep 28, 2023 at 11:19 AM Tom Lane wrote:
>
> ... trailing bytes that could be mistaken for ASCII are precisely
> the property that causes us to reject an encoding as not backend-safe.
Oh, that is good to know. Thanks for the information.
==
Kind Regards,
Peter Smith
v4 LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ld, since they kind of belong together?
~
2b.
None of the other bool fields here has an 'is' prefix, so you could
consider a shorter field name, like 'ownersuperuser' or
'superuserowner', etc.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi,
I found a link in the docs that referred to the stats "views" section,
instead of the more relevant (IMO) stats "functions" section.
PSA the 1 line patch -- it explains what I mean better than I can
describe in words...
==
Kind Regards,
Peter Smith.
Fujitsu Austra
On Sat, Sep 30, 2023 at 12:04 AM Daniel Gustafsson wrote:
>
> > On 29 Sep 2023, at 10:54, Daniel Gustafsson wrote:
> >
> >> On 29 Sep 2023, at 06:51, Peter Smith wrote:
> >
> >> I found a link in the docs that referred to the stats "views" sectio
uired;
SUGGESTION
must_use_password = sub->passwordrequired && !sub->ownersuperuser;
==
Other than those trivial things, v5 LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
d="monitoring-pg-stat-all-indexes-view" ==> ok
pg_stat_user_functions, linkend="monitoring-pg-stat-user-functions-view" ==> ok
pg_statio_all_tables, linkend="monitoring-pg-statio-all-tables-view" ==> ok
pg_statio_all_indexes, linkend="monitoring-pg-statio-all-indexes-view" ==> ok
pg_statio_all_sequences,
linkend="monitoring-pg-statio-all-sequences-view" ==> ok
~~~
PSA a patch to make these few changes.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Change-some-monitoring-linkends-for-consistency.patch
Description: Binary data
On Tue, Oct 3, 2023 at 5:42 PM vignesh C wrote:
>
> Thanks for the comments, the attached v6 version patch has the changes
> for the same.
>
v6 LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Oct 3, 2023 at 6:30 PM Michael Paquier wrote:
>
> On Tue, Oct 03, 2023 at 01:11:15PM +1100, Peter Smith wrote:
> > I noticed one or two "monitoring" links and linkends that are slightly
> > inconsistent from all the others.
>
> -
> +
>
tring to be %X/%X (not %u/%X like here).
~~~
43.
+ appendStringInfo(&cmd,
+ "SELECT restart_lsn, confirmed_flush_lsn, catalog_xmin"
+ " FROM pg_catalog.pg_replication_slots"
+ " WHERE slot_name = %s",
+ quote_literal_cstr(remote_slot->name));
double space before FROM?
~~~
44. synchronize_one_slot
+ /*
+ * We might not have the WALs retained locally corresponding to
+ * remote's restart_lsn if our local restart_lsn and/or local
+ * catalog_xmin is ahead of remote's one. And thus we can not create
+ * the local slot in sync with primary as that would mean moving local
+ * slot backward. Thus wait for primary's restart_lsn and catalog_xmin
+ * to catch up with the local ones and then do the sync.
+ */
+ if (remote_slot->restart_lsn < MyReplicationSlot->data.restart_lsn ||
+ TransactionIdPrecedes(remote_slot->catalog_xmin,
+ MyReplicationSlot->data.catalog_xmin))
+ {
+ if (!wait_for_primary_slot_catchup(wrconn, remote_slot))
+ {
+ /*
+ * The remote slot didn't catch up to locally reserved
+ * position
+ */
+ ReplicationSlotRelease();
+ CommitTransactionCommand();
+ return;
+ }
SUGGESTION (comment is slightly simplified)
If the local restart_lsn and/or local catalog_xmin is ahead of those
on the remote then we cannot create the local slot in sync with
primary because that would mean moving local slot backwards. In this
case we will wait for primary's restart_lsn and catalog_xmin to catch
up with the local one before attempting the sync.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi Ajin. Thanks for addressing my previous review comments from v19.
I checked all the changes. Below are a few follow-up remarks.
On Thu, Oct 5, 2023 at 7:54 PM Ajin Cherian wrote:
>
> On Wed, Sep 27, 2023 at 2:37 PM Peter Smith wrote:
> >
> > Here are some more review comm
> added more ids
ref/alter_publication.sgml ==> added link to
"sql-altersubscription-refresh-publication"
ref/drop_subscription.sgml ==> added link to "sql-altersubscription"
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Add-more-links.patch
Description: Binary data
On Fri, Oct 6, 2023 at 7:37 PM Alvaro Herrera wrote:
>
> On 2023-Sep-27, Peter Smith wrote:
>
> > 3. get_local_synced_slot_names
> >
> > + for (int i = 0; i < max_replication_slots; i++)
> > + {
> > + ReplicationSlot *s = &ReplicationSlotCtl->re
On Mon, Oct 9, 2023 at 3:32 PM Amit Kapila wrote:
>
> On Fri, Oct 6, 2023 at 12:15 PM Peter Smith wrote:
> >
> > Here is a patch to add the 2 missing references:
> >
> > ref/alter_subscription.sgml ==> added more ids
> > ref/alter_publication.sgml ==>
ble the subscription by
> executing ALTER SUBSCRIPTION ... DISABLE, and then
> disassociate it from the replication slot by executing ALTER
> SUBSCRIPTION ... SET (slot_name = NONE).
>
Hi Vignesh. Thanks for the review comments.
Modified as suggested.
PSA v3.
==
Kind Regards,
Pe
On Mon, Oct 9, 2023 at 9:34 PM shveta malik wrote:
>
> On Wed, Oct 4, 2023 at 8:53 AM Peter Smith wrote:
> >
> > Here are some review comments for v20-0002.
> >
>
> Thanks Peter for the feedback. Comments from 31 till end are addressed
> in v22. First 30 comme
On Tue, Oct 10, 2023 at 11:33 PM Amit Kapila wrote:
>
> On Tue, Oct 10, 2023 at 11:40 AM vignesh C wrote:
> >
> > On Tue, 10 Oct 2023 at 08:47, Peter Smith wrote:
> > > PSA v3.
> >
> > Few more instances in other logical replication relate
ove was in the section "31.5 Conflicts". OK, modified in v4.
~~
Thanks for reporting those. PSA v4.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v4-0001-Add-more-pub-sub-links.patch
Description: Binary data
Probably should be either "StringInfo has" or "StringInfos have"
==
Kind Regards.
Peter Smith.
Fujitsu Australia
On Thu, Oct 12, 2023 at 3:44 PM Amit Kapila wrote:
>
> On Mon, Oct 9, 2023 at 12:15 PM Peter Smith wrote:
> >
> > On Mon, Oct 9, 2023 at 3:32 PM Amit Kapila wrote:
> > >
> >
> > In v1, I used the same pattern as on the CREATE SUBSCRIPTION page,
> >
looks incorrect - the "(isnull: %s)" value
is backwards now.
I guess maybe the intent was to change it something like below:
elog(DEBUG3, "row filter evaluates to %s (isnull: %s)",
isnull ? "false" : DatumGetBool(ret) ? "true" : "false",
isnull ? "true" : "false");
--
Kind Regards,
Peter Smith.
Fujitsu Australia
BREAKS "\t\n@$><=;|&{() "
+#define WORD_BREAKS "\t\n,@$><=;|&{() "
OTOH, this seemed a pretty fundamental change to the 12-year-old (!!)
code so I don't know if it may be too risky and/or could adversely
affect something else?
The tests are all still pas
ogging for logical replication worker.c was DEBUG2.
Perhaps row-filter patch should be using DEBUG2 also?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Jan 20, 2022 at 2:29 PM Amit Kapila wrote:
>
> On Thu, Jan 20, 2022 at 7:51 AM Peter Smith wrote:
> >
> > Here are some review comments for v68-0001.
> >
> >
> > 3. src/backend/replication/pgoutput/pgoutput.c - pgoutput_row_filter_init
> >
> &
3] # expected: '21|1|21'
[21:09:32.183] # Looks like you failed 1 test of 13.
[21:09:32.183] [21:08:49] t/025_rep_changes_for_schema.pl
--
[1] https://cirrus-ci.com/task/6280873841524736?logs=test_world#L3970
Kind Regards,
Peter Smith.
Fujitsu Australia
e RI validation error will happen when a
table is added to the publication at CREATE/ALTER PUBLICATION time.
It says that *subsequent* "UPDATE or DELETE operations will cause an error".
~~
The point is that it is one thing to decide to change something that
was never officially documented, but to change already *documented*
behaviour is much more radical and has the potential to upset some
users.
--
[1] https://www.postgresql.org/docs/devel/logical-replication-publication.
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, Jan 24, 2022 at 4:53 PM Peter Smith wrote:
>
> On Fri, Jan 21, 2022 at 2:04 PM Amit Kapila wrote:
> >
> > On Thu, Jan 20, 2022 at 7:56 PM Alvaro Herrera
> > wrote:
> > >
> > > > > Maybe this was meant to be "validate RF
> >
ctions
* do not include UPDATE or DELETE.
*/
bool rf_invalid_for_upd_del;
AttrNumber invalid_rfcol_upd_del;
PublicationActions pubactions;
} PublicationDesc;
~~~
6. src/tools/pgindent/typedefs.list
Missing the new typedef PublicationDesc
--
Kind Regards,
Peter Smith.
Fujitsu Australia
quot;SELECT count(x)
FROM tab_rf_x");
+is($result, qq(7), 'check table tab_rf_x should not be filtered');
Here I also think the comment maybe should just say something like:
BEFORE:
Similarly, normal filtering after the initial phase will also have not effect.
AFTER:
Similarly, the table filter for tab_rf_x (after the initial phase) has
no effect when combined with the ALL TABLES IN SCHEMA.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Jan 25, 2022 at 2:18 PM houzj.f...@fujitsu.com
wrote:
>
> On Monday, January 24, 2022 4:38 PM Peter Smith
> >
...
> > 5. src/include/catalog/pg_publication.h - typedef struct PublicationDesc
> >
> > +typedef struct PublicationDesc
> > +{
> > + /*
ge to be "boolop" (because the BoolExpr struct has a
boolop member). e.g.
BEFORE
+ * - (Var Op Const) Bool (Var Op Const)
AFTER
+ * - (Var Op Const) boolop (Var Op Const)
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
AFTER
Unchanged toasted replica identity columns are only logged in the old
tuple. Copy this over to the new tuple.
~~~
7. src/test/subscription/t/028_row_filter.pl - COPYRIGHT
This TAP file should have a copyright comment that is consistent with
all the other TAP files.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
;
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, x); -- error
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (b, c); -- error
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, d); -- error
+ALTER PUBLICATION testpub_fortable ADD TABLE testpub_tbl5 (a, c); -- ok
For al
On Tue, Feb 1, 2022 at 12:07 PM Peter Smith wrote:
>
> On Sat, Jan 29, 2022 at 11:31 AM Andres Freund wrote:
> >
> > Hi,
> >
> > Are there any recent performance evaluations of the overhead of row
> > filters? I
> > think it'd be good to get so
701 - 800 of 1980 matches
Mail list logo
