On Tue, Jul 26, 2022 at 2:09 PM Amit Kapila wrote:
>
> On Tue, Jul 26, 2022 at 5:04 AM Peter Smith wrote:
> >
> > On Mon, Jul 25, 2022 at 7:33 PM vignesh C wrote:
> > >
> > > On Mon, Jul 25, 2022 at 12:58 PM Peter Smith
> > > wrote:
> >
with
enabled=false because then the user still has a chance to reconsider,
but otherwise, I don't see what good a warning does if the potentially
harmful initial copy is going to proceed anyway; isn't that like
putting a warning sign at the bottom of a cliff?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
a new primary that has existing table data is not supported.
~~~
11. Generic steps for adding a new node to an existing set of primaries
SUGGESTION (title)
Generic steps for adding a new primary to an existing set of primaries
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, Jul 25, 2022 at 7:33 PM vignesh C wrote:
>
> On Mon, Jul 25, 2022 at 12:58 PM Peter Smith wrote:
> >
> > Firstly, I have some (case-sensitivity) questions about the previous
> > patch which was already pushed [1].
> >
> > Q1. create_subscription d
On Mon, Jul 25, 2022 at 6:54 PM Amit Kapila wrote:
>
> On Mon, Jul 25, 2022 at 12:58 PM Peter Smith wrote:
> >
...
> >
> > Q2. parse_subscription_options
> >
> > Similarly, in the code (parse_subscription_options), I did not
> > understand why the chec
was that for PG15 there might be a whole new section
added to Chapter 31 [1] for describing "Column Lists" (i.e. the Column
List equivalent of the "Row Filters" section)
--
[1] https://www.postgresql.org/docs/15/logical-replication.html
Kind Regards,
Peter Smith.
Fujitsu Australia
al" is not a
special origin name anymore.
--
[1]
https://github.com/postgres/postgres/commit/366283961ac0ed6d8901c6090f3fd02fce0a
Kind Regards,
Peter Smith.
Fujitsu Australia
lations(subid, false);
}
List *
GetSubscriptionNotReadyRelations(Oid subid)
{
return GetSubscriptionRelations(subid, true);
}
--
Kind Regards,
Peter Smith.
Fujitsu Australia
t; > Please don't get me wrong :)
> >
> > I favor a bitmask over an enum here, as you do, with a clean
> > layer for those flags.
> >
>
> Okay, let's see what Peter Smith has to say about this as he was in
> favor of using enum here?
>
I was in
e:
+/*
+ * IsReservedName
+ * True iff name is either "none" or "any".
+ */
+static bool
+IsReservedOriginName(const char *name)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
o introduce a whole new set of
problems.
---
(It used to look like this)
Chapter 53. "System Catalogs"
==
53.1. Overview
53.2. pg_aggregate
53.3. pg_am
53.4. pg_amop
53.5. pg_amproc
...
53.66. System Views <--- 2nd heading just hiding here
53.67. pg_available_extensions
53.68. pg_available_extension_versions
53.69. pg_backend_memory_contexts
53.70. pg_config
...
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
FROM tab_rowfilter_viaroot_part ORDER BY 1");
+is($result, qq(16
+17),
+ 'check replicated rows to tab_rowfilter_viaroot_part'
+);
There is a comment above that code like:
# tab_rowfilter_viaroot_part filter is: (a > 15)
# - INSERT (14)NO, 14 < 15
# - INSERT (15)NO, 15 = 15
#
dea falls apart because IIUC you have no intentions
to permit things like:
(SUBSCRIPTION_REL_STATE_NOT_READY | SUBSCRIPTION_REL_STATE_READY)
IMO using an enum might be a better choice for that parameter.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
is a bit confusing, though. Perhaps this could use a bitmask as
> argument.
+1
(or some enum)
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
(hypothetical) code below can work
as expected, because where is the code updating the value of
MySubscription->retry ???
set_subscription_retry(true);
set_subscription_retry(true);
I think at least there needs to be some detailed comments explaining
what this quick exit is really doing because my guess is that
currently it is not quite working as expected.
~~~
4.5
+ /* reset subretry */
Uppercase comment
--
[1]
https://github.com/postgres/postgres/commit/8445f5a21d40b969673ca03918c74b4fbc882bf4
[2]
https://www.postgresql.org/message-id/OS3PR01MB62755C6C9A75EB09F7218B589E839%40OS3PR01MB6275.jpnprd01.prod.outlook.com
[3]
https://www.postgresql.org/message-id/OS3PR01MB6275120502A4730AB9932FCA9E839%40OS3PR01MB6275.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
is relevant here.
v30-0002
No comments.
--
[1]
https://github.com/postgres/postgres/commit/8445f5a21d40b969673ca03918c74b4fbc882bf4
Kind Regards,
Peter Smith.
Fujitsu Australia
ven".
>
> Peter, I have applied something using your original wording. This is
> a minor issue, but I did not see my suggestion as being better than
> yours.
Thanks for pushing it.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Sat, Jul 9, 2022 at 5:32 AM Bruce Momjian wrote:
>
...
> Attached is a patch to accomplish this. Its output can be seen here:
>
> https://momjian.us/tmp/pgsql/internals.html
>
That output looks good to me.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
eparse_expand_command', prorettype => 'text',
+ proargtypes => 'text', prosrc => 'ddl_deparse_expand_command' },
My 1st impressions was the name "ddl_deparse_expand_command" does not
see to reflext the description very well...
Maybe calling it something like "ddl_json
On Fri, Jul 8, 2022 at 2:17 AM Bruce Momjian wrote:
>
> On Thu, Jul 7, 2022 at 09:29:13AM +1000, Peter Smith wrote:
> > On Thu, Jun 16, 2022 at 10:59 AM Peter Smith wrote:
> > >
> > > On Thu, Jun 9, 2022 at 11:50 PM Tom Lane wrote:
> > > >
> > &
w.postgresql.org/message-id/flat/CALDaNm0gwjY_4HFxvvty01BOT01q_fJLKQ3pWP9%3D9orqubhjcQ%40mail.gmail.com
+static char
+defGetStreamingMode(DefElem *def)
[2]
https://www.postgresql.org/message-id/flat/CAHut%2BPs%2B4iLzJGkPFEatv%3D%2Baa6NUB38-WT050RFKeJqhdcLaGA%40mail.gmail.com#6d43277cbb074071b8e960
On Thu, Jun 16, 2022 at 10:59 AM Peter Smith wrote:
>
> On Thu, Jun 9, 2022 at 11:50 PM Tom Lane wrote:
> >
> > Peter Eisentraut writes:
> > > Initially, that chapter did not document any system views.
> >
> > Maybe we could make the system views a separ
tion
mypub with (
BINARY COPY_DATA DISABLE_ON_ERRORSLOT_NAME
SYNCHRONOUS_COMMIT
CONNECT CREATE_SLOT ENABLED STREAMING
TWO_PHASE
postgres=# create subscription mysub connection 'blah' publication mypub with (
--
Kind Rega
00_bugs.pl .. ok
Test Summary Report
---
t/032_apply_delay.pl (Wstat: 65280 Tests: 0 Failed: 0)
Non-zero exit status: 255
Parse errors: No plan found in TAP output
--
[1]
https://www.postgresql.org/message-id/flat/CAA4eK1%2BwyN6zpaHUkCLorEWNx75MG0xhMwcFhvjqm2KURZEAGw%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/flat/CAHut%2BPucvKZgg_eJzUW--iL6DXHg1Jwj6F09tQziE3kUF67uLg%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
n each test part was using unique data
(e.g. 11,12,13, then 12,22,32, then 13,23,33 etc)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ategory as a
code comment typo patch - do those normally get backpatched? - maybe
follow the same convention here. OTOH, if you think it may be helpful
for future backpatches then I am also fine if you wanted to push it to
PG15.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
-
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Re-order-disable_on_error-in-tab-complete-COMPLET.patch
Description: Binary data
ent like:
# =
--
Kind Regards,
Peter Smith.
Fujitsu Australia
RE trigger_func();
+ALTER TABLE test_tab_partition ENABLE REPLICA TRIGGER insert_trig;
+});
Looks like the wrong comment. I think it should say something like
"Check the trigger on the partition table."
--
Kind Regards,
Peter Smith.
Fujitsu Australia
" is at the end, I have
> changed it to the end.
>
Although it seems it is not a hard rule, mostly the COMPLETE_WITH are
coded using alphabetical order. Anyway, I think that was a clear
intention here too since 13 of 14 parameters were already in
alphabetical order; it is actually only that "disable_on_error"
parameter that was misplaced; not the new "origin" parameter.
Also, in practice, on those completions will get output in
alphabetical order, so IMO it makes more sense for the code to be
consistent with the output:
e.g.
test_sub=# create subscription sub xxx connection '' publication pub WITH (
BINARY DISABLE_ON_ERRORSTREAMING
CONNECT ENABLED SYNCHRONOUS_COMMIT
COPY_DATA ORIGIN TWO_PHASE
CREATE_SLOT SLOT_NAME
test_sub=#
--
Kind Regards,
Peter Smith.
Fujitsu Australia
rker_internal.h
extern int logicalrep_sync_worker_count(Oid subid);
+extern int logicalrep_apply_background_worker_count(Oid subid);
Just wondering if this should be called
"logicalrep_apply_bgworker_count(Oid subid);" for consistency with the
other function naming.
v14-0002
2.1 Commit message
Change all TAP tests using the SUBSCRIPTION "streaming" option, so they
now test both 'on' and 'parallel' values.
"option" -> "parameter"
--
[1]
https://www.postgresql.org/message-id/OS3PR01MB6275DCCDF35B3BBD52CA02CC9EB89%40OS3PR01MB6275.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
hers.
change 2) Adds 'force' value for copy_data parameter.
~
"replicating" -> "replicated"
"change 1)" -> "1)"
"change 2)" -> "2)"
v24-0004
No comments. LGTM
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Document the steps for the following:
a) Creating a two-node bidirectional replication when there is no data
on both nodes.
b) Adding a new node when there is no data on any of the nodes.
c) Adding a new node when data is present on the existing nodes.
d) Generic steps for adding a new node to an existi
PSA trivial patch fixing a harmless #define typo.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Fix-typo-in-pg_publication.c.patch
Description: Binary data
= @_;
versus
+ my ($node_A, $node_B, $node_C, $appname_B, $appname_C, $streaming_mode) =
+ @_;
--
Kind Regards,
Peter Smith.
Fujitsu Australia
does not apply
[postgres@CentOS7-x64 oss_postgres_misc]$
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Jun 22, 2022 at 2:18 PM Amit Kapila wrote:
>
> On Thu, Jun 16, 2022 at 6:07 AM Peter Smith wrote:
>
> >
> > Thank you for your review comments. Those reported mistakes are fixed
> > in the attached patch v3.
> >
>
> This patch looks mostly goo
cations will be published to the first node and then synchronized
+back to the new node while creating the subscription in Step-5. This would
+result in inconsistent data).
+
+
4.3.a
typo: "untilthe" -> "until the"
4.3.b
SUGGESTION (just for the 2nd sentence)
This lock is necessary to prevent any modifications from happening on
the new node. If data modifications occurred after Step-3, there is a
chance they could be published to the first node and then synchronized
back to the new node while creating the subscription in Step-5.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
d transaction %u",
+ prepare_data.xid);
apply_handle_stream_start:
+ elog(DEBUG1, "starting streaming of xid %u", stream_xid);
apply_handle_stream_stop:
+ elog(DEBUG1, "stopped streaming of xid %u, %u changes streamed",
stream_xid, nchanges);
apply_handle_stream_abort:
+ elog(DEBUG1, "[Apply BGW #%u] aborting current transaction xid=%u, subxid=%u",
+ MyParallelState->n, GetCurrentTransactionIdIfAny(),
+ GetCurrentSubTransactionId());
+ elog(DEBUG1, "[Apply BGW #%u] rolling back to savepoint %s",
+ MyParallelState->n, spname);
apply_handle_stream_commit:
+ elog(DEBUG1, "received commit for streamed transaction %u", xid);
Observations:
63a.
Every new introduced message is at level DEBUG1 (not DEBUG). AFAIK
this is OK, because the messages are all protocol related and every
other existing debug message of the current replication worker.c was
also at the same DEBUG1 level.
63b.
The prefix "[Apply BGW #%u]" is used to indicate the bgworker is
executing the code, but it does not seem to be used 100% consistently
- e.g. there are some apply_bgworker_XXX functions not using this
prefix. Is that OK or a mistake?
--
Kind Regards,
Peter Smith.
Fujitsu Austrlia
tch to correct those.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Use-type-integer-instead-of-type-int.patch
Description: Binary data
On Thu, Jun 9, 2022 at 11:50 PM Tom Lane wrote:
>
> Peter Eisentraut writes:
> > Initially, that chapter did not document any system views.
>
> Maybe we could make the system views a separate chapter?
+1
------
Kind Regards,
Peter Smith.
Fujitsu Australia
ur review comments. Those reported mistakes are fixed
in the attached patch v3.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v3-0001-PGDOCS-tablesync-ignores-publish-operation.patch
Description: Binary data
4-Document-bidirectional-logical-replication-steps.patch
Kind Regards,
Peter Smith.
Fujitsu Australia
Clean up
pg_ctl: directory "data_N1" does not exist
pg_ctl: directory "data_N2" does not exist
pg_ctl: directory "data_N3" does not exist
pg_ctl: directory "data_
de. This would result in inconsistent
+data. There is no need to lock the required tables in
+node1 because any data changes made will be synchronized
+while creating the subscription with copy_data = force).
+
"no need to lock the required tables in" -> "no need to lock the
required tables on"
==
8. doc/src/sgml/ref/create_subscription.sgml
@@ -403,7 +403,10 @@ CREATE SUBSCRIPTION subscription_namecopy_data = force.
+ copy_data = force. Refer to
+for how
+ copy_data and origin can be used
+ in bidirectional replication.
"can be used in bidirectional replication" -> "can be used to set up
bidirectional replication"
--
Kind Regards,
Peter Smith.
Fujitsu Australia
bscribed
+ errhint("Use CREATE/ALTER SUBSCRIPTION with copy_data = off or force."));
Saying "off or force" is not consistent with the other message wording
in this patch, which used "/" for multiple enums.
(e.g. "connect = false", "copy_data = true/for
SHARED_RELATION BKI_ROW
/* List of publications subscribed to */
text subpublications[1] BKI_FORCE_NOT_NULL;
+
+ /* Only publish data originating from the specified origin */
+ text suborigin;
#endif
} FormData_pg_subscription;
Perhaps it would be better if this new column was also forced to be NOT NULL.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
FDAA9%40OSZPR01MB6310.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
v2-0001-PGDOCS-tablesync-ignores-publish-operation.patch
Description: Binary data
în timpul rulării intializării Perl"
~~~
(Notice the missing 'i' - "inițializării" versus "intializării")
--
Kind Regards,
Peter Smith.
Fujitsu Australia
in bidirectional replication.
SUGGESTION (keep your formatting)
Refer to for how
copy_data and origin can be used
in bidirectional replication.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
be consistent with the errhint.
SUGGESTION
errdetail("CREATE/ALTER SUBSCRIPTION with origin = local and copy_data
= on is not allowed when the publisher might have replicated data."),
--
[1]
https://www.postgresql.org/message-id/CALDaNm3iLLxP4OV%2ByQHs-c71P6zQ9W8D30DGsve1SQs_1pFsSQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Jun 9, 2022 at 10:00 AM Tom Lane wrote:
>
> Peter Smith writes:
> > e.g.2 What I thought it should look like:
>
> > Chapter 53. "System Catalogs and Views" <-- chapter name change
> > ==
> > 53.1. System Catalogs <-- heading name
time, so perhaps there is some reason for it
being like it is?
Thoughts?
--
[1] https://www.postgresql.org/docs/15/catalogs.html
Kind Regards,
Peter Smith.
Fujitsu Australia
] https://www.postgresql.org/docs/15/catalogs.html
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Jun 8, 2022 at 1:25 PM Justin Pryzby wrote:
>
> On Mon, Jun 06, 2022 at 03:42:31PM +1000, Peter Smith wrote:
> > I noticed the patch "0001-language-fixes-on-HEAD-from-Justin.patch" says:
> >
> > @@ -11673,7 +11673,7 @@
> >pros
.
--
[1]
https://www.postgresql.org/message-id/CAA4eK1L_98LF7Db4yFY1PhKKRzoT83xtN41jTS5X%2B8OeGrAkLw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-PGDOCS-tablesync-ignores-publish-operation.patch
Description: Binary data
by the Oxford dictionary. Or about in brief highlights some
fact ‘on the subject of, or connected with’
The main difference between of and about is that of implies a
possessive quality while about implies concerning or on the subject of
something.
~~~
>From which I guess
1. 'get information of tables in a p
~~~
13. src/test/subscription/t/032_origin.pl
+# 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 ORDER BY 1;");
+is($result, qq(11
+12), 'Remote data originating from another node (not the publisher)
is not replicated when origin option is local'
+);
"option" -> "parameter"
--
Kind Regards,
Peter Smith.
Fujitsu Australia
in data is not returned with only-local option
SUGGESTION
-- Verify the behaviour of the only-local parameter
2b.
+-- Remote origin data returned when only-local option is not set
"option" -> "parameter"
2c.
+-- Remote origin data not returned when only-local option is set
"
help to better understand the
proposal. Also, we thought the ability to experiment with the proposed
API could help people to decide whether LRG is something worth
pursuing or not.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
PSA a patch to fix a spelling mistake that I happened upon...
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Fix-typo.patch
Description: Binary data
e.pl
+# Test replication with publications created using FOR ALL TABLES EXCEPT TABLE
+# option.
+# Create schemas and tables on publisher
"option" -> "clause"
--
[1]
https://www.postgresql.org/message-id/TYCPR01MB83730A2F1D6A5303E9C1416AEDD99%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
on_root option are reset
+\dRp+ testpub_reset
+ALTER PUBLICATION testpub_reset RESET;
+\dRp+ testpub_reset
SUGGESTION
-- Verify that 'publish' and 'publish_via_partition_root' publication
parameters are reset
--
[1] https://www.postgresql.org/docs/current/sql-createpublication.html
Kind Regards,
Peter Smith.
Fujitsu Australia
e
treated as the "new" node that you are attaching to the "first" node1.
IMO the section 31.11.1 example should be reversed so that it obeys
the "generic" pattern.
e.g. It should be doing the CREATE PUBLICATION pub_node2 first (since
that is the "new" node)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
off.
-> that should say "on the remaining node" (plural)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ntil"
33c.
+# Subroutine to create subscription and wait till the initial sync is
completed.
+# Subroutine expects subscriber node, publisher node, subscription name,
+# destination connection string, publication name and the subscription with
+# options to be passed as input parameters.
+sub create_subscription
+{
+ my ($node_subscriber, $node_publisher, $sub_name, $node_connstr,
+ $pub_name, $with_options)
+ = @_;
"subscription with options" => "subscription parameters"
"$with_options" -> "$sub_params"
33d.
+# Specifying only_local 'on' which indicates that the publisher should only
"Specifying" => "Specify"
--
[1] https://www.postgresql.org/docs/15/sql-createsubscription.html
[2] https://www.postgresql.org/docs/current/bgworker.html
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, May 26, 2022 at 3:08 PM Amit Kapila wrote:
>
> On Thu, May 26, 2022 at 7:06 AM Peter Smith wrote:
> >
> >
> > 3. doc/src/sgml/catalogs.sgml
> >
> > +
> > + If true, the subscription will request that the publisher send
> &g
On Thu, May 26, 2022 at 2:20 PM Amit Kapila wrote:
>
> On Thu, May 26, 2022 at 7:06 AM Peter Smith wrote:
> >
> > Here are my review comments for patch v15-0001.
> >
> > ==
> >
> > 1. Commit message
> >
> > Should this also say
ry here. We don't care
where the node_C data came from for this test case.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
--
If the joining node (e.g. a new weather station) already has some
initial local sensor data then sharing that initial data manually with
all the other nodes requires some tricky steps. LRG can hide all this
complexity behind the API, so it is not a user problem anymore.
--
[1]
https://www.postgresql.org/message-id/flat/CALDaNm0gwjY_4HFxvvty01BOT01q_fJLKQ3pWP9%3D9orqubhjcQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
OBJ_EXCEPT_TABLE, /* An Except table */
PUBLICATIONOBJ_TABLES_IN_SCHEMA, /* All tables in schema */
PUBLICATIONOBJ_TABLES_IN_CUR_SCHEMA, /* All tables in first element of
Maybe the comment should be more like:
/* A table to be excluded */
~~~
16. src/test/regress/sql/publication.sql
I did not see any test cases using EXCEPT when the optional TABLE
keyword is omitted.
--
[1]
https://www.postgresql.org/message-id/CAHut%2BPtZDfBJ1d%3D3kSexgM5m%2BP_ok8sdsJXKimsXycaMyqXsNA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, May 20, 2022 at 10:19 AM Peter Smith wrote:
>
> FYI, although the v6-0002 patch applied cleanly, I found that the SGML
> was malformed and so the pg docs build fails.
>
> ~~~
> e.g.
>
> [postgres@CentOS7-x64 sgml]$ make STYLE=website html
>
: refsect1 line 57 and variablelist
^
...
I will work around it locally, but for future patches please check the
SGML builds ok before posting.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
superuser to RESET publication
SET ROLE regress_publication_user;
DROP PUBLICATION testpub_reset;
--
Kind Regards,
Peter Smith.
Fujitsu Australia
xtra #.
Looks better now as just:
# Check initial data was copied to the subscriber
~~~
8. .../subscription/t/023_twophase_stream.pl
+$node_publisher->poll_query_until('postgres',
+ "SELECT pid != $oldpid FROM pg_stat_replication WHERE
application_name = '$appname' AND state = 'streaming';"
+) or die "Timed out while waiting for apply to restart after changing
PUBLICATION";
Should that say "... after changing SUBSCRIPTION"?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
output:
# t
# last actual query output:
#
# with stderr:
timed out waiting for catchup at t/031_column_list.pl line 667.
Kind Regards,
Peter Smith.
Fujitsu Australia
ation/origin.h
@@ -53,7 +53,7 @@ extern XLogRecPtr
replorigin_get_progress(RepOriginId node, bool flush);
extern void replorigin_session_advance(XLogRecPtr remote_commit,
XLogRecPtr local_commit);
-extern void replorigin_session_setup(RepOriginId node);
+extern void replorigin_session_setup(RepOriginId node, bool acquire);
As previously suggested in comment #8 maybe the 2nd parm should be
'must_acquire'.
==
36. src/include/replication/worker_internal.h
@@ -60,6 +60,8 @@ typedef struct LogicalRepWorker
*/
FileSet*stream_fileset;
+ bool subworker;
+
Probably this new member deserves a comment.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, May 17, 2022 at 1:56 PM Amit Kapila wrote:
>
> On Tue, May 17, 2022 at 7:35 AM Peter Smith wrote:
> >
> > Below are my review comments for v5-0002.
> >
> > There may be an overlap with comments recently posted by Osumi-san [1].
> >
> > (I also h
dd except table when publish_via_partition_root option does not
+-- have default value
38e.
+-- can't add except table when the publication options does not have default
+-- values
SUGGESTION
can't add EXCEPT TABLE when the publication options are not the default values
~~~
39. .../t/032_re
#define PUB_DEFAULT_VIA_ROOT false
#define PUB_DEFAULT_ALL_TABLES false
--
[1]
https://www.postgresql.org/message-id/TYCPR01MB8373C3120C2B3112001ED6F1EDCF9%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
hat publish option and publish via root option is reset
SUGGESTED:
+-- Verify that publish options and publish_via_partition_root option are reset
8d.
+-- Verify that only superuser can execute RESET publication
SUGGESTED
+-- Verify that only superuser can reset a publication
--
Kind Regards,
Peter Smith.
Fujitsu Australia
9:59:58.342) is reporting the $reset1 value (19:59:58.402808).
I did not understand how a timestamp saved from the past could be
ahead of the timestamp of the log.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
PSA trivial patch to fix some very old comment typo.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Fix-typo-in-comment.patch
Description: Binary data
;");
-is($result, qq(1), 'transaction is prepared on subscriber');
-
-# 2PC transaction gets aborted
-$node_publisher->safe_psql('postgres', "ROLLBACK PREPARED
'test_prepared_tab';");
-
$node_publisher->wait_for_catchup($appname);
These don't seem to belong here. They are clean up from the prior
test. (same as comment #4)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ALTER PUBLICATION pubname RESET [EXCEPT]
--
Kind Regards,
Peter Smith.
Fujitsu Australia
.
So I have no more review comments. LGTM.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ere is not really any overarching comment that associates these
#defines back to the new 'stream' field so you are just supposed to
guess that's what they are for?
46b. I also feel that using 'o' for ON is not consistent with the 'f'
of OFF. IMO better to use 't/f' for true/false instead of 'o/f'. Also
don't forget update docs, pg_dump.c etc.
46c. Typo: "appied" -> "applied"
47. src/test/regress/expected/subscription.out - missting test
Missing some test cases for all new option values? E.g. Where is the
test using streaming value is set to 'apply'. Same comment as [PSv4]
#81
--
[1]
https://www.postgresql.org/message-id/OS0PR01MB5716E8D536552467EFB512EF94FC9%40OS0PR01MB5716.jpnprd01.prod.outlook.com
[PSv4]
https://www.postgresql.org/message-id/CAHut%2BPuqYP5eD5wcSCtk%3Da6KuMjat2UCzqyGoE7sieCaBsVskQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, May 3, 2022 at 5:16 PM Peter Smith wrote:
>
...
> Avoiding unexpected differences like this is why I suggested the
> option should have to be explicitly enabled instead of being on by
> default as it is in the current patch. See my review comment #14 [1].
> It means the
IMO it would be better if the ALTER syntax could be
unambiguous in the first place. So perhaps the rules should be more
restrictive (e.g. just disallow ALTER ... ADD any table that overlaps
the existing EXCEPT list ??)
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
ransactions in which case
> we might lose all the benefits of doing it via a background worker. Do
> we see any simple way to avoid this?
>
Avoiding unexpected differences like this is why I suggested the
option should have to be explicitly enabled instead of being on by
default as it is in the current patch. See my review comment #14 [1].
It means the user won't have to change their existing code as a
workaround.
--
[1]
https://www.postgresql.org/message-id/CAHut%2BPuqYP5eD5wcSCtk%3Da6KuMjat2UCzqyGoE7sieCaBsVskQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
ve heading?
Wording: "Adding new node ..." -> "Adding a new node ..."
--
[1]
https://www.postgresql.org/message-id/flat/CAHut%2BPuwRAoWY9pz%3DEubps3ooQCOBFiYPU9Yi%3DVB-U%2ByORU7OA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Apr 29, 2022 at 2:16 PM Yura Sokolov wrote:
>
> В Чт, 28/04/2022 в 17:37 +0530, vignesh C пишет:
> > On Thu, Apr 28, 2022 at 4:24 PM Yura Sokolov
> > wrote:
> > > В Чт, 28/04/2022 в 09:49 +1000, Peter Smith пишет:
> > >
> > > > 1.1 ADVA
ous section on this page.
--
[1]
https://github.com/postgres/postgres/commit/d7ab2a9a3c0a2800ab36bb48d1cc9737006e
Kind Regards,
Peter Smith.
Fujitsu Australia
r,
+ '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
ement 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_fJLKQ3pWP9%3D9orqubhjcQ%40mail.gmail.co
s 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
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
xtern
@@ -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
s 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
+-- 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
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.
901 - 1000 of 1527 matches
Mail list logo
