A rebase was needed.
PSA v3*.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
am_parallel_apply_worker();
+}
Patch v40 added the IsLogicalWorker() to the condition, but why is
that extra check necessary?
==
7. src/include/replication/worker_internal.h
+typedef struct ParallelApplyWorkerInfo
+{
+ shm_mq_handle *mq_handle;
+
+ /*
+ * The queue used to transfer messages from the parallel apply worker to
+ * the leader apply worker.
+ */
+ shm_mq_handle *error_mq_handle;
In patch v40 the comment about the NULL error_mq_handle is removed,
but since the code still explicitly set/checks NULL in different
places isn't it still better to have some comment here to describe
what NULL means?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ction are clearly for "logical replication".
Thoughts?
--
[1] 31.10 Configuration Settings -
https://www.postgresql.org/docs/current/logical-replication-config.html
[2] 20.6 Replication -
https://www.postgresql.org/docs/current/runtime-config-replication.html
Kind Regards,
Peter Sm
On Thu, Oct 20, 2022 at 6:52 PM Peter Smith wrote:
>
> On Thu, Oct 20, 2022 at 3:16 PM Justin Pryzby wrote:
> >
> > On Thu, Oct 20, 2022 at 11:56:58AM +1100, Peter Smith wrote:
> > > Patch 0002 adds a sanity-check function called by
> > > InitializeGUCOptions,
this patch now as a prerequisite for my GUC C
var sanity-checker [1].
--
[1]
https://www.postgresql.org/message-id/CAHut%2BPss16YBiYYKyrZBvSp_4uSQfCy7aYfDXU0N8w5VZ5dd_g%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Oct 25, 2022 at 4:09 PM Michael Paquier wrote:
>
> On Tue, Oct 25, 2022 at 02:43:43PM +1100, Peter Smith wrote:
> > This is essentially the same as before except now, utilizing the
> > GUC_DEFAULT_COMPILE flag added by Justin's patch [1], the sanity-check
>
/backend/replication/logical/launcher.c: patch does not apply
--
Kind Regards,
Peter Smith.
Fujitsu Australia
s. For others (mostly the string ones) I left the GUC C var
untouched because the sanity checker function already has a rule not
to complain about int GUC C vars which are 0 or string GUC vars which
are NULL.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v5-0001-GUC-C-variable-sanity-check.patch
Description: Binary data
tch touches (aka its declaration and its default value in
> guc_tables.c). In any case, the patch of this thread still needs some
> adjustments IMO.
OK, I can make that adjustment if it is preferred. I think it is the
same as what I already suggested a while ago [1] ("But probably it is
no problem to just add #defines...")
--
[1] https://www.postgresql.org/message-id/1113448.1665717297%40sss.pgh.pa.us
Kind Regards,
Peter Smith.
Fujitsu Australia.
SA patch v6.
The GUC defaults of guc_tables.c, and the modified GUC C var
declarations now share the same common #define'd value (instead of
cut/paste preprocessor code).
Per Michael's suggestion [1] to use centralized definitions.
--
[1] https://www.postgresql.org/message-id/Y1nuD
en though they are static) would be
better moved out to another file simply to get things down to a more
manageable size.
--
[1]
https://www.postgresql.org/message-id/CAHut%2BPtKiTmcQ7zXs6YvR-qtuMQ9wgffnfamqCAVpM_ETa2LCg%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
that I added back in March:
> https://commitfest.postgresql.org/40/3595/
Sorry, I missed that earlier because I searched only by authors, and
some were missing. Now I saw it has just been updated - thanks.
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
}
After appending the qualstr to buf, should there be a pfree(qualstr)?
~~~
26. pg_get_trigger_whenclause
Missing function comment.
~~~
27. print_function_sqlbody
-static void
+void
print_function_sqlbody(StringInfo buf, HeapTuple proctup)
{
Missing function comment. Probably having a function comment is more
important now that this is not static?
==
src/include/tcop/ddl_deparse.h
28.
+extern char *deparse_utility_command(CollectedCommand *cmd, bool verbose_mode);
+extern char *ddl_deparse_json_to_string(char *jsonb);
+extern char *deparse_drop_command(const char *objidentity, const char
*objecttype,
+ DropBehavior behavior);
Function naming seems inconsistent. ('ddl_deparse_XXX' versus 'deparse_XXX').
--
Kind Regards,
Peter Smith.
Fujitsu Australia
oid)
hash_seq_init(&status, guc_hashtab);
while ((hentry = (GUCHashEntry *) hash_seq_search(&status)) != NULL)
{
+ /* check mapping between initial and default value */
+ Assert(check_GUC_init(hentry->gucvar));
+
Use uppercase.
Minor re-wording.
SUGGESTION
/* Check the GUC default
On Mon, Oct 31, 2022 at 4:02 PM Michael Paquier wrote:
>
> On Mon, Oct 31, 2022 at 12:01:33PM +1100, Peter Smith wrote:
> > SUGGESTION
> > /* Only applicable when prefetching is available */
>
> Thanks for the suggestion. Done this way, then.
>
> > +/* Disab
so in the function
comment.
~~~
12.
+ /*
+ * Special-case crock for types with strange typmod rules where we put
+ * typmod at the middle of name(e.g. TIME(6) with time zone ). We cannot
+ * schema-qualify nor add quotes to the type name in these cases.
+ */
Missing space before '(e.g.'. Extra space before ').'.
~~~
13. FunctionGetDefaults
/*
* Return the defaults values of arguments to a function, as a list of
* deparsed expressions.
*/
"defaults values" -> "default values"
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Nov 2, 2022 at 9:02 PM Amit Kapila wrote:
>
> On Wed, Oct 19, 2022 at 10:10 AM Peter Smith wrote:
> >
> > Thanks for your testing.
> >
>
> LGTM so pushed with a minor change in one of the titles in the Examples
> section.
>
Thanks for pushing.
-
HANDLER", 0);
+ else
Isn't it better to call other function instead of passing zero params
to this one?
~
25b.
+ /* add VALIDATOR clause */
+ if (fdwForm->fdwvalidator == InvalidOid)
+ tmp = new_objtree_VA("NO VALIDATOR", 0);
Ditto #25a
~
25c.
Both above should use OidIsValid macro.
~~~
26. deparse_AlterFdwStmt
26a.
+ /* add HANDLER clause */
+ if (fdwForm->fdwhandler == InvalidOid)
+ tmp = new_objtree_VA("NO HANDLER", 0);
Ditto #25a
~
26b.
+ /* add VALIDATOR clause */
+ if (fdwForm->fdwvalidator == InvalidOid)
+ tmp = new_objtree_VA("NO VALIDATOR", 0);
Ditto #25a
~
26c.
Both above should use OidIsValid macro.
~~~
27. deparse_CreateRangeStmt
+ /* SUBTYPE */
+ tmp = new_objtree_for_qualname_id(TypeRelationId,
+ rangeForm->rngsubtype);
+ tmp = new_objtree_VA("SUBTYPE = %{type}D",
+ 2,
+ "clause", ObjTypeString, "subtype",
+ "type", ObjTypeObject, tmp);
+ definition = lappend(definition, new_object_object(tmp));
The reusing of 'tmp' variable seems a bit sneaky to me. Perhaps using
'tmp' and 'tmp_qualid' might be a more readable way to go here.
~~~
28. deparse_AlterEnumStmt
+ if (node->newValNeighbor)
+ {
+ append_string_object(alterEnum, "%{after_or_before}s",
+ node->newValIsAfter ? "AFTER" : "BEFORE");
+ append_string_object(alterEnum, "%{neighbour}L", node->newValNeighbor);
+ }
Has a mix of US and UK spelling of neighbor/neighbour?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
chTableStates(bool *started_tx)
>
> Can we update comments indicating that if this function starts the
> transaction then the caller is responsible to commit it?
>
> 8.
> (errmsg("logical replication apply worker for subscription \"%s\" will
> restart so two_phase c
when doing the REFRESH
PUBLICATION.
Is that a deliberate functionality, or is it a quirk / bug?
--
[1] https://www.postgresql.org/docs/devel/sql-altersubscription.html
Kind Regards,
Peter Smith.
Fujitsu Australia
ks? Maybe it is just a style thing,
but since there are so many of them I felt it contributed to clutter
and made the code less readable. This pattern was in many places, not
just the example above.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
CT)
if (supported_opts & SUBOPT_ENABLED)
if (supported_opts & SUBOPT_SLOT_NAME)
if (supported_opts & SUBOPT_COPY_DATA)
>
> Can we implement a similar idea for the parse_publication_options
> although there are only two options right now. Option parsing code
> will be consistent for logical replication DDLs and is extensible.
> Thoughts?
I have no strong opinion about it. It seems a trade off between having
a goal of "code consistency", versus "if it aint broke don't fix it".
--
Kind Regards,
Peter Smith.
Fujitsu Australia
; > Is that a deliberate functionality, or is it a quirk / bug?
>
> I don't think it's a bug
...
OK. Thanks to both of you for sharing your thoughts about it.
--
Kind Regards,
Peter Smith
Fujitsu Australia
On Thu, Jun 3, 2021 at 7:56 PM Amit Kapila wrote:
>
> On Wed, Jun 2, 2021 at 4:34 AM Peter Smith wrote:
> >
> > Please find attached the latest patch set v82*
> >
>
> Few comments on 0001:
>
> 1.
> + /*
> + * BeginTr
On Mon, May 10, 2021 at 11:42 PM Euler Taveira wrote:
>
> On Mon, May 10, 2021, at 5:19 AM, Peter Smith wrote:
>
> AFAIK this is the latest patch available, but FYI it no longer applies
> cleanly on HEAD.
>
> Peter, the last patch is broken since f3b141c4825. I'm sti
On Wed, Jun 2, 2021 at 10:41 PM Bharath Rupireddy
wrote:
>
> On Wed, Jun 2, 2021 at 11:43 AM Peter Smith wrote:
> > Yes, it looks better, but (since the masks are all 1 bit) I was only
> > asking why not do like:
> >
> > if (supported_opts & SUBO
On Thu, Jun 10, 2021 at 1:28 AM Bharath Rupireddy
wrote:
>
> On Wed, Jun 9, 2021 at 10:37 AM Peter Smith wrote:
> >
[...]
I checked the v4* patches.
Everything applies and builds and tests OK for me.
> > 2.
> > + /* If connect option is supported, the others also nee
On Thu, Jun 17, 2021 at 6:22 PM Greg Nancarrow wrote:
>
> On Wed, Jun 16, 2021 at 9:08 AM Peter Smith wrote:
> >
> >
> > Please find attached the latest patch set v86*
> >
>
> A couple of comments:
>
> (1) I think one of my suggested changes was missed
521] DETAIL: Key (a)=(1) already exists.
2021-06-18 15:38:29.765 AEST [18521] CONTEXT: COPY test_tab, line 1
2021-06-18 15:38:29.766 AEST [19924] LOG: background worker "logical
replication worker" (PID 18521) exited with exit code 1
etc...
-[ RECORD 1 ]-+
oid | 16399
subdbid | 16384
subname | tap_sub
subowner | 10
subenabled| t
disable_on_error | f
disabled_by_error | f
subbinary | f
substream | f
subconninfo | host=localhost dbname=test_pub application_name=tap_sub
subslotname | tap_sub
subsynccommit | off
suberrmsg |
subpublications | {tap_pub}
--
Kind Regards,
Peter Smith.
Fujitsu Australia
%40mail.gmail.com
[Amit-3]
https://www.postgresql.org/message-id/CAA4eK1Kyx6U9yxC7OXoBD7pHC3bJ4LuNGd%3DOiABmiW6%2BqG%2BvEQ%40mail.gmail.com
[Amit-4]
https://www.postgresql.org/message-id/CAA4eK1Kyx6U9yxC7OXoBD7pHC3bJ4LuNGd%3DOiABmiW6%2BqG%2BvEQ%40mail.gmail.com
[Amit-5]
https://www.postgresql.org/message-id/CAA4eK1Kyx6U9yxC7OXoBD7pHC3bJ4LuNGd%3DOiABmiW6%2BqG%2BvEQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
is done in worker.c.
Thanks for the recent rebase.
- The v15 patch applies OK (albeit with whitespace warning)
- make check is passing OK
- the new TAP tests 020_row_filter is passing OK.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
can affect the ASRP
copy_data [link].
Thoughts?
-
[1] https://www.postgresql.org/docs/devel/sql-altersubscription.html
[2] https://www.postgresql.org/docs/devel/sql-alterpublication.html
[3] https://www.postgresql.org/docs/devel/sql-createpublication.html
Kind Regards,
Peter Smith.
Fujitsu Australia
it is passed at
AlterSubscription_refresh(sub, copy_data);
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
a lot less than what people were
expecting.
Unless there are millions of rows the speedup may be barely noticeable.
[1]
https://www.postgresql.org/message-id/20210128022032.eq2qqc6zxkqn5syt%40alap3.anarazel.de
[2]
https://www.postgresql.org/message-id/CACawEhW_iMnY9XK2tEb1ig%2BA%2BgKeB4cxdJcxMsoCU0
strings of the form "option = value" */
errmsg("subscription with %s must also set %s",
"slot_name = NONE", "create_slot = false")));
This code can be simplified even more than the others mentioned,
because here the "specified_opts" checks were already done in the code
that precedes this.
It can be simplified like this:
- if (enabled && !*enabled_given && *enabled)
+ if (opts->enabled)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
/*- translator: both %s are strings of the form "option = value" */
errmsg("subscription with %s must also set %s",
"slot_name = NONE", "enabled = false")));
- if (create_slot && !create_slot_given && *create_slot)
+ if (opts->create_slot)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
+ /*- translator: both %s are strings of the form "option = value" */
errmsg("subscription with %s must also set %s",
"slot_name = NONE", "create_slot = false")));
//
PSA my patch which includes all the fixes mentioned above.
(Make check, and TAP subscription tests are tested and pass OK)
--
[1]
https://github.com/postgres/postgres/commit/8aafb02616753f5c6c90bbc567636b73c0cbb9d4
Kind Regards,
Peter Smith.
Fujitsu Australia
v11-0001-PS-Fix-v11.patch
Description: Binary data
the form "option = value" */
errmsg("subscription with %s must also set %s",
"slot_name = NONE", "create_slot = false")));
This code can be simplified even more than the others mentioned,
because here the "specified_opts" checks were already done in the code
that precedes this.
It can be simplified like this:
- if (enabled && !*enabled_given && *enabled)
+ if (opts->enabled)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
/*- translator: both %s are strings of the form "option = value" */
errmsg("subscription with %s must also set %s",
"slot_name = NONE", "enabled = false")));
- if (create_slot && !create_slot_given && *create_slot)
+ if (opts->create_slot)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
+ /*- translator: both %s are strings of the form "option = value" */
errmsg("subscription with %s must also set %s",
"slot_name = NONE", "create_slot = false")));
//
PSA my patch which includes all the fixes mentioned above.
(Make check, and TAP subscription tests are tested and pass OK)
--
[1]
https://github.com/postgres/postgres/commit/8aafb02616753f5c6c90bbc567636b73c0cbb9d4
Kind Regards,
Peter Smith.
Fujitsu Australia
v11-0001-PS-Fix-v11.patch
Description: Binary data
On Wed, Jul 7, 2021 at 1:35 PM Bharath Rupireddy
wrote:
>
> On Wed, Jul 7, 2021 at 5:33 AM Peter Smith wrote:
> > PSA my patch which includes all the fixes mentioned above.
>
> I agree with Amit to start a separate thread to discuss these points.
> IMO, we can close thi
e test code either.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Jul 8, 2021 at 10:08 PM vignesh C wrote:
>
> On Thu, Jul 8, 2021 at 11:37 AM Amit Kapila wrote:
> >
> > On Tue, Jul 6, 2021 at 9:58 AM Peter Smith wrote:
> > >
> > > Please find attached the latest patch set v93*
> > >
> >
> > Tha
mypub for all tables with ( "
"create publication mypub for table mytable " TAB --> "create
publication mypub for table mytable WITH ( "
--
[1] https://www.postgresql.org/docs/devel/sql-createpublication.html
Kind Regards,
Peter Smith
Fujitsu Australia
v1-0001-more-auto-complete-for-CREATE-PUBLICATION.patch
Description: Binary data
On Sun, Jul 11, 2021 at 8:20 PM Amit Kapila wrote:
>
> On Fri, Jul 9, 2021 at 4:43 AM Peter Smith wrote:
> >
> > > The patch looks good to me, I don't have any comments.
> >
> > I tried the v95-0001 patch.
> >
> > - The patch applied
something similar in the previous section on this page.
--
[1]
https://github.com/postgres/postgres/commit/d7ab2a9a3c0a2800ab36bb48d1cc9737006e
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
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
d to spill the data for streaming transactions 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
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.
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
M_OFF 'f'
+
+/*
+ * Streaming transactions are written to a temporary file and applied only
+ * after the transaction is committed on upstream.
+ */
+#define SUBSTREAM_ON 'o'
+
+/* Streaming transactions are appied immediately via a background worker */
+#define SUBSTREAM_APPLY 'a'
46a. There 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
.
So I have no more review comments. LGTM.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ALTER PUBLICATION pubname RESET [EXCEPT]
--
Kind Regards,
Peter Smith.
Fujitsu Australia
NSACTION 'test_prepared_tab';});
-
-$node_publisher->wait_for_catchup($appname);
-
-# check that transaction is in prepared state on subscriber
-$result = $node_subscriber->safe_psql('postgres', "SELECT count(*)
FROM pg_prepared_xacts;");
-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
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
> slot_name = 'test_slot'
>
I don't know if this is related, but I noticed that the log timestamp
(19: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
et
8c.
+-- Verify that 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
#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
27;FOR ALL TABLES IN SCHEMA' publication
38d.
+-- can't add 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_rep_changes_except_table.pl
39a.
+# Check the table data does not sync for excluded table
+my $result = $node_subscriber->safe_psql('postgres',
+ "SELECT count(*), min(a), max(a) FROM sch1.tab1");
+is($result, qq(0||), 'check tablesync is excluded for excluded tables');
Maybe the "is" message should say "check there is no initial data
copied for the excluded table"
~~~
40 .../t/032_rep_changes_except_table.pl
+# Insert some data into few tables and verify that inserted data is not
+# replicated
+$node_publisher->safe_psql('postgres',
+ "INSERT INTO sch1.tab1 VALUES(generate_series(11,20))");
The comment is not quite correct. You are inserting into only one
table here - not "few tables".
~~~
41. .../t/032_rep_changes_except_table.pl
+# Alter publication to exclude data changes in public.tab1 and verify that
+# subscriber does not get the new table data.
"new table data" -> "changed data for this table"
--
[1]
https://www.postgresql.org/message-id/TYCPR01MB83737C28187A6E0BADAE98F0EDCF9%40TYCPR01MB8373.jpnprd01.prod.outlook.com
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
ou read from >= 14 but it was still bool so you need to
convert 'false' -> 'f' and 'true' -> 't'?
==
35. src/include/replication/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
list.pl ...
ok 22 - partitions with different replica identities not replicated
correctly Waiting for replication conn sub1's replay_lsn to pass
0/1528CF0 on publisher # poll_query_until timed out executing this
query:
# SELECT '0/1528CF0' <= replay_lsn AND state = 'streaming'
# FROM pg_catalog.pg_stat_replication
# WHERE application_name IN ('sub1', 'walreceiver')
# expecting this 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
TION"?
~~~
7. .../subscription/t/023_twophase_stream.pl
###
# Check initial data was copied to subscriber
###
Perhaps the above comment now looks a bit out-of-place with the extra #####.
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
ERROR: must be superuser to RESET publication
SET ROLE regress_publication_user;
DROP PUBLICATION testpub_reset;
--
Kind Regards,
Peter Smith.
Fujitsu Australia
error : Opening and ending tag
mismatch: 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
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
>
UBLICATIONOBJ_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
n handle initial table data
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
stent.
~~~
7. src/test/subscription/t/032_onlylocal.pl
+my $appname_B2 = 'tap_sub_B2';
+$node_B->safe_psql(
+ 'postgres', "
+ CREATE SUBSCRIPTION tap_sub_B2
+ CONNECTION '$node_C_connstr application_name=$appname_B2'
+ PUBLICATION tap_pub_C
+ WITH (only_local = on)");
+
AFAIK the "WITH (only_local = on)" is unnecessary here. We don't care
where the node_C data came from for this test case.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
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
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
+# Subroutine to create subscription and wait till the initial sync is
completed.
"till" -> "until"
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
option specified as off.
-> that should say "on the remaining node" (plural)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
reated 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
ot 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
7; and 'f' character.
~~~
14. .../t/032_rep_changes_except_table.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
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
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
ta 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
"
E SUBSCRIPTION 'origin' parameter
~~~
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
indicates ‘On the subject of; concerning’ as
defined 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.
~~~
e examples.
--
[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
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
] https://www.postgresql.org/docs/15/catalogs.html
Kind Regards,
Peter Smith.
Fujitsu Australia
me, 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
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
ing "=" instead of "as".
Anyway, it would be more consistent with the errhint. Also, change the
"true" to "on" to 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
can be used
+ 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
în timpul rulării intializării Perl"
~~~
(Notice the missing 'i' - "inițializării" versus "intializării")
--
Kind Regards,
Peter Smith.
Fujitsu Australia
26B8428422EAC1BFB8A4FDAA9%40OSZPR01MB6310.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
v2-0001-PGDOCS-tablesync-ignores-publish-operation.patch
Description: Binary data
n,6100,SubscriptionRelationId)
BKI_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
;).
==
6. src/backend/commands/subscriptioncmds.c - check_pub_table_subscribed
+ 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 "/" f
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
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_N4&qu
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
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
tch to correct those.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Use-type-integer-instead-of-type-int.patch
Description: Binary data
pply_bgworker_set_status:
+ elog(DEBUG1, "[Apply BGW #%u] set status to %d", MyParallelState->n, status);
apply_bgworker_subxact_info_add:
+ elog(DEBUG1, "[Apply BGW #%u] defining savepoint %s",
+ MyParallelState->n, spname);
apply_handle_stream_prepare:
+ elog(DEBUG1, "received prepare for streamed 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
ications occurred after Step-3, there is a chance that
+the modifications 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
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
does not apply
[postgres@CentOS7-x64 oss_postgres_misc]$
--
Kind Regards,
Peter Smith.
Fujitsu Australia
isher, $node_subscriber, $appname, $is_apply) = @_;
versus
+ my ($node_A, $node_B, $node_C, $appname_B, $appname_C, $streaming_mode) =
+ @_;
--
Kind Regards,
Peter Smith.
Fujitsu Australia
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
1001 - 1100 of 2023 matches
Mail list logo
