On Wed, Sep 7, 2022 at 8:49 PM Amit Kapila wrote:
>
> On Tue, Sep 6, 2022 at 2:15 PM Amit Kapila wrote:
> >
> > On Tue, Sep 6, 2022 at 5:08 AM Peter Smith wrote:
> > >
> > > You are right - that REFRESH PUBLICATION was not necessary for this
> > &g
Thanks for addressing my previous review comments. I have no more
comments for v46*.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, Sep 5, 2022 at 8:46 PM Amit Kapila wrote:
>
> On Mon, Sep 5, 2022 at 3:46 PM Peter Smith wrote:
> >
> >
> > PSA v7.
> >
>
> For example, if additional columns are added to the table, then
> +(after a REFRESH PUBLICATION) if there was a column
On Mon, Sep 5, 2022 at 1:42 PM shiy.f...@fujitsu.com
wrote:
>
> On Mon, Sep 5, 2022 8:28 AM Peter Smith wrote:
> >
> > I have rebased the remaining patch (v6-0001 is the same as v5-0002)
> >
>
> Thanks for updating the patch. Here are some comments.
>
>
relid);
+
+ if (i > 0)
+ appendStringInfoString(dest, " OR ");
+
+ appendStringInfo(dest, "(N.nspname = '%s' AND C.relname = '%s')",
+ schemaname, tablename);
+ }
--
[1]
https://www.postgresql.org/message-id/CAHut%2BPsku25%2BVjz7HiohWxc2WU07O_ZV4voFG%2BU7WzwKhUzpGQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Sep 2, 2022 at 11:40 PM Amit Kapila wrote:
>
> On Fri, Sep 2, 2022 at 8:45 AM Peter Smith wrote:
> >
> > On Thu, Sep 1, 2022 at 7:53 PM Amit Kapila wrote:
> > >
> > > On Fri, Aug 26, 2022 at 7:33 AM Peter Smith wrote:
> > > >
> > &
On Fri, Sep 2, 2022 at 6:21 PM Peter Smith wrote:
>
> Here are my review comments for the latest patch v44-0001.
>
...
>
> 6. src/backend/commands/subscriptioncmds.c
>
> + if (bsearch(, subrel_local_oids,
> + subrel_count, sizeof(Oid), oid_cmp))
> + isnewtable = false
st))
+ {
I think the proper way to test for non-empty List is
if (publist != NIL) { ... }
or simply
if (publist) { ... }
~~~
8.
+ errmsg("subscription \"%s\" requested origin=NONE but might copy
data that had a different origin",
Do you think this should say "requested copy_data with origin=NONE",
instead of just "requested origin=NONE"?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Sep 1, 2022 at 7:53 PM Amit Kapila wrote:
>
> On Fri, Aug 26, 2022 at 7:33 AM Peter Smith wrote:
> >
>
> Few comments on both the patches:
> v4-0001*
> =
> 1.
> Furthermore, if the table uses
> + REPLICA IDENTITY FULL, specifying a
On Wed, Aug 31, 2022 at 8:36 PM Amit Kapila wrote:
>
> On Wed, Aug 31, 2022 at 11:35 AM Peter Smith wrote:
> >
> > Here are my review comments for v43-0001.
> >
...
> > ==
> >
> > 2. doc/src/sgml/ref/create_subscription.sgml
> >
> >
y warnings using origin=NONE.
Refer to for how copy_data and origin can be used to set up
replication between primaries.
--
[1]
https://www.postgresql.org/message-id/CAHut%2BPvonTd423-cWqoxh0w8Bd_Po3OToqqyxuR1iMNmxSLr_Q%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
e to get the errdetail
message too? Then the test will be more readable for the expected
result and you will also have the bonus of testing the table-name list
in the warning more thoroughly.
--
[1]
https://www.postgresql.org/message-id/CALDaNm1V%2BAvzPsUcq%3DmNYG%2BJfAyovTWBe4vWKTknOgH_ko1E0Q%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
"LogicalRepPartMap", which is not here but is declared static in
relation.c. Maybe the quick/easy fix would be to just change the first
line to say: "Partition map (see LogicalRepPartMap in relation.c)".
OTOH, I'm not sure if some part of this comment still needs to be left
in relation.c (??)
--
[1] https://www.postgresql.org/docs/devel/source-conventions.html
Kind Regards,
Peter Smith.
Fujitsu Australia
t; But the code is unconditionally doing
>> table_slot_create() before it even knows if a tuple was found or not.
>> So what about when it is NOT found - in that case shouldn't there be
>> some cleaning up that (unused?) table slot that got unconditionally
>> created?
>>
>
> This sounds accurate. But I guess it may not have been considered critical as
> we are operating in the ApplyMessageContext? Tha is going to be freed once a
> single tuple is dispatched.
>
> I have a slight preference not to do it in this patch, but if you think
> otherwise let me know.
I agree. Maybe this is not even a leak worth bothering about if it is
only in the short-lived ApplyMessageContext like you say. Anyway,
AFAIK this was already in existing code, so a fix (if any) would
belong in a different patch to this one.
>> 31.
>>
>> + slot_modify_data(remoteslot_part, localslot, entry,
>> newtup);
>>
>> Unnecessary wrapping.
>>
>> ==
>
>
> I think I have not changed this, but fixed anyway
Hmm - I don't see that you changed this, but anyway I guess you
shouldn't be fixing wrapping problems unless this patch caused them.
--
[1]
https://www.postgresql.org/message-id/CACawEhXbw%3D%3DK02v3%3DnHFEAFJqegx0b4r2J%2BFtXtKFkJeE6R95Q%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
ompleted.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
n/t/030_origin.pl
+# have remotely originated data from node_A. We throw a warning, in this case,
+# to draw attention to there being possible remote data.
"throw a warning" -> "log a warning" (this occurs 2x)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
INSERT INTO t1 VALUES(1, 'a-1', 'b-1', 'c-1', 'd-1', 'e-1');
> +INSERT 0 1
>
OK. Modified to say this.
> 4b) In the above, we could mention that the insert should be done on
> the "publisher side" as the previous statements are executed on the
> subscriber side.
OK. Modi
est where the subscriber uses index
# and only touches multiple rows
What does "only ... multiple" mean?
This occurs several times also.
~~~
35.
+# wait for initial table synchronization to finish
+$node_subscriber->wait_for_subscription_sync;
+$node_subscriber->wait_for_subscription_sync;
+$node_subscriber->wait_for_subscription_sync;
That triple wait looks unusual. Is it deliberate?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
be aware that InvalidPid is -1 (not 0), so
replacing any existing code (e.g. in replorigin_session_setup) that
was already checking for 0 has to be done with lots of care.
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Mon, Aug 22, 2022 at 7:01 PM Amit Kapila wrote:
>
> On Mon, Aug 22, 2022 at 4:42 AM Peter Smith wrote:
> >
> > On Fri, Aug 19, 2022 at 7:55 PM Amit Kapila wrote:
> > >
> > > On Fri, Aug 19, 2022 at 3:05 PM Peter Smith wrote:
> > > >
>
On Mon, Aug 22, 2022 at 7:11 PM Erik Rijkers wrote:
>
> Op 22-08-2022 om 10:27 schreef Peter Smith:
> >
> > PSA new set of v2* patches.
>
> Hi,
>
> In the second file a small typo, I think:
>
> "enclosed by parenthesis" should be
> "enclos
/rebase-apply/patch:30: trailing whitespace.
>if the publication publishes only INSERT operations.
> warning: 1 line adds whitespace errors.
>
Fixed.
~~~
PSA the v3* patch set.
--
[1]
https://github.com/postgres/postgres/commit/923def9a533a7d986acfb524139d8b9e5466d0a5
Kind Regards,
Pe
I noticed an accidental ;;
PSA patch to remove the same.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Fix-typo-empty-statement.patch
Description: Binary data
t;t2"
DETAIL: Column list used by the publication does not cover the
replica identity.
I modified the wording for this part of the docs.
~~~
PSA new set of v2* patches.
--
[1] -
https://github.com/postgres/postgres/commit/923def9a533a7d986acfb524139d8b9e5466d0a5
Kind Regard
On Fri, Aug 19, 2022 at 7:55 PM Amit Kapila wrote:
>
> On Fri, Aug 19, 2022 at 3:05 PM Peter Smith wrote:
> >
> > On Fri, Aug 19, 2022 at 7:10 PM Amit Kapila wrote:
> > >
> > > On Fri, Aug 19, 2022 at 2:36 PM Peter Smith wrote:
> > > >
> >
On Fri, Aug 19, 2022 at 7:10 PM Amit Kapila wrote:
>
> On Fri, Aug 19, 2022 at 2:36 PM Peter Smith wrote:
> >
> > Here are my review comments for the v23-0005 patch:
> >
> > ==
> >
> > Commit Message says:
> > main_worker_pid is Proc
', WORKER_TYPE_APPLY='a',
WORKER_TYPE_PARALLEL_APPLY='p'), then use some char column to expose
it? As a bonus, I think the other code (i.e.patch 0001) will also be
improved if a 'type' member is added.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
all this trigger_func_tab1_safe which was
added in patch 0003 is now getting removed in patch 0004. Maybe there
is some good reason, but it doesn't seem right to be adding code in
one patch and then removing it again in the next patch.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
to 'parallel_apply_safe' or 'parallel_safe' etc will
give it the intended meaning.
--
[1]
https://www.postgresql.org/message-id/OS3PR01MB6275739E73E8BEC5D13FB6739E6B9%40OS3PR01MB6275.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
.g. bug
etc...
STEP 2 - Define your rules for how intend to address each of these
kinds of shadows (e.g. just simple rename of the var, use
'foreach_current_index', ...). Hopefully, it will be easy to reach an
agreement now since all instances of the same kind will look pretty
much the same.
STEP 3 - Fix all of the same kinds of shadows per single patch (using
the already agreed fix approach from step 2).
REPEAT STEPS 2,3 until done.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
%2BPuxEQ88PDhFcBftnNY1BAjdj_9G8FYhTvPHKjP8yfacaQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Aug 18, 2022 at 6:57 PM Amit Kapila wrote:
>
> On Thu, Aug 18, 2022 at 11:59 AM Peter Smith wrote:
> >
> > Here are my review comments for patch v21-0001:
> >
> > 4. Commit message
> >
> > In addition, the patch extends the logica
haps it is still useful to include a comment when changing ones
like this?
e.g.
+ TupleTableSlot *tslot; /* TODO - Why declare this at all? Shouldn't
it just re-use the 'slot' at function scope? */
Otherwise, such knowledge will be lost, and nobody will ever know to
revisit them, which feels a bit more like *hiding* the mistake than
fixing it.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi Wang-san,
FYI, I also checked the latest patch v23-0001 but found that the
v21-0001/v23-0001 differences are minimal, so all my v21* review
comments are still applicable for the patch v23-0001.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Oid relid,
+ char *spname, int szsp);
This big block of similarly named externs might as well be in
alphabetical order instead of apparently random.
~~~
53.
+static inline bool
+am_apply_bgworker(void)
+{
+ return MyLogicalRepWorker->main_worker_pid != 0;
+}
This can be simplified/improved using the new macros as previously
suggested in #1d.
SUGGESTION
static inline bool
am_apply_bgworker(void)
{
return isApplyBgWorker(MyLogicalRepWorker);
}
54. src/tools/pgindent/typedefs.list
AppendState
+ApplyBgworkerEntry
+ApplyBgworkerShared
+ApplyBgworkerInfo
+ApplyBgworkerStatus
ApplyErrorCallbackArg
Please rearrange these into alphabetical order.
--
[1]
https://softwareengineering.stackexchange.com/questions/219351/state-or-status-when-should-a-variable-name-contain-the-word-state-and-w#:~:text=status%20is%20used%20to%20describe,(e.g.%20pending%2Fdispatched)
[2]
https://github.com/postgres/postgres/commit/efd0c16becbf45e3b0215e124fde75fee8fcbce4
[3]
https://www.postgresql.org/message-id/OS0PR01MB57169AEA399C6DC370EAF23B94649%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
lat/CAHut%2BPuv4LaQKVQSErtV_%3D3MezUdpipVOMt7tJ3fXHxt_YK-Zw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
the
> newly-added parens on those grounds. I also found a couple more
> spots that could be converted. Pushed with those changes.
>
Thanks for pushing.
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
TICE.
See table [1]. It says WARNING is meant for "warnings of likey
problems". But this is not exactly a "likely" problem - IIUC we really
don't know if there is even any problem at all we only know there
is the *potential* for a problem, but the user has to then judge it
f
anks for the feedback.
PSA patch v3 which now uses explicit comparisons to NIL everywhere,
and also addresses the other review comments.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v3-0001-use-NIL-test-for-empty-List-checks.patch
Description: Binary data
On Tue, Aug 16, 2022 at 11:27 AM Tom Lane wrote:
>
> Peter Smith writes:
> > During a recent code review I was going to suggest that some new code
> > would be more readable if the following:
> > if (list_length(alist) == 0) ...
>
> > was replaced
On Tue, Aug 16, 2022 at 11:27 AM Tom Lane wrote:
>
> Peter Smith writes:
> > During a recent code review I was going to suggest that some new code
> > would be more readable if the following:
> > if (list_length(alist) == 0) ...
>
> > was replaced
_is_empty all
those can be made consistent and often also more readable as to the
code intent.
Patch 0001 adds a new function 'list_is_empty'.
Patch 0002 makes use of it.
Thoughts?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0002-list_is_empty-use-the-new-function.patch
Descriptio
ON-2
Simply remove that one-line comment because the larger comment seems
to be saying the same thing anyhow.
Thoughts?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
=" really
belong in the earlier patch (0003 ?) when this TAP test file was
introduced.
--
[1]
https://www.postgresql.org/message-id/CAHut%2BPvrw%2BtgCEYGxv%2BnKrqg-zbJdYEXee6o4irPAsYoXcuUcw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
test_tab1 ALTER COLUMN b DROP DEFAULT");
Maybe for these tests like this it would be better to test if it works
OK using an immutable DEFAULT function instead of just completely
removing the bad function to make it work.
I think maybe the same was done for TRIGGER tests. There was a test
for a trigger with a bad function, and then the trigger was removed.
What about including a test for the trigger with a good function?
--
[1]
https://www.postgresql.org/message-id/CAHut%2BPv9cKurDQHtk-ygYp45-8LYdE%3D4sMZY-8UmbeDTGgECVg%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
ne just with some #defines
==
15. src/include/replication/worker_internal.h
+ /* proto version of publisher. */
+ uint32 proto_version;
SUGGESTION
Protocol version of publisher
~~~
16.
+ /* id of apply background worker */
+ uint32 worker_id;
Uppercase comment
~~~
17.
+/*
+ * Struct for maintaining an apply background worker.
+ */
+typedef struct ApplyBgworkerState
I'm not sure what this comment means. Perhaps there are some words missing?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
, one of them publishing a parent table
directly, and the other publishing a child table with
publish_via_partition_root = true, then subscribing to both those
publications from one subscription results in two initial
replications. It should only be copied once.
--
Kind Regards,
Peter Smith
.relid = C.oid\n",
+ pub_names.data);
AFAICT the main reason for this check was to decide if you can use the
new version of 'pg_get_publication_tables' that supports the VARIADIC
array of pub names or not. If that is correct, then maybe the comment
should describe that reason, or maybe add another bool var similar to
the 'check_columnlist' one for this.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
PSA patch version v1* for a new "Column Lists" pgdocs section
This is just a first draft, but I wanted to post it as-is, with the
hope that I can get some feedback while continuing to work on it.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Column-List-replic
fmtstr_error_callback
45.1
+/*
+ * Error context callback for JSON format string expansion.
+ *
+ * Possible improvement: indicate which element we're expanding, if applicable.
+ */
Should that "Possible improvement" comment have "XXX" prefix like most
other possible improvement comments have?
45.2
+fmtstr_error_callback(void *arg)
+{
+ errcontext("while expanding format string \"%s\"", (char *) arg);
+
+}
Remove the blank line.
==
46. src/backend/utils/adt/ruleutils.c - pg_get_trigger_whenclause
+char *
+pg_get_trigger_whenclause(Form_pg_trigger trigrec, Node *whenClause,
bool pretty)
Missing function comment
~~~
47. src/backend/utils/adt/ruleutils.c - print_function_sqlbody
@@ -3513,7 +3526,7 @@ pg_get_function_arg_default(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(string_to_text(str));
}
-static void
+void
print_function_sqlbody(StringInfo buf, HeapTuple proctup)
{
Having a function comment is more important now that this is no longer static.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
--
[1]
https://github.com/postgres/postgres/blob/master/src/backend/utils/misc/postgresql.conf.sample
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Aug 2, 2022 at 6:57 PM Amit Kapila wrote:
>
> On Mon, Jul 25, 2022 at 1:27 PM Peter Smith wrote:
> >
> > On Sat, Dec 11, 2021 at 12:24 AM Alvaro Herrera
> > wrote:
> > >
> > > On 2021-Dec-10, Peter Eisentraut wrote:
> > >
> >
On Mon, Aug 1, 2022 at 6:52 PM Amit Kapila wrote:
>
> On Mon, Aug 1, 2022 at 1:32 PM Peter Smith wrote:
> >
> > On Mon, Aug 1, 2022 at 3:27 PM shiy.f...@fujitsu.com
> > wrote:
> > >
> > > On Fri, Jul 29, 2022 1:22 PM vignesh C wrote:
>
ut then there was a suggestion
that the {0} should be implemented as a macro, and the subsequent
discussions about that macro eventually bikeshedded the patch to
death.
It might be a good idea if you check that old thread so you can avoid
the same pitfalls. I hope you have more luck than I did ;-)
--
[1]
https://www.postgresql.org/message-id/flat/2793d0d2-c65f-5db0-4f89-251188438391%40gmail.com#102ee1b34a8341f28758efc347874b8a
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Aug 2, 2022 at 10:03 AM Tom Lane wrote:
>
> Peter Smith writes:
> > This patch tweaks a some tabbing and replaces some spaces with tabs to
> > improve slightly the comment alignment in file
> > 'postgresql.conf.sample'
>
> Hmm ... the parts you want to chang
[2]
https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-STATS-VIEWS
[3]
https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-STATS-FUNCTIONS
Kind Regards,
Peter Smith.
Fujitsu Australia.
This patch tweaks a some tabbing and replaces some spaces with tabs to
improve slightly the comment alignment in file
'postgresql.conf.sample'
PSA.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Align-the-comments.patch
Description: Binary data
uot;, maybe this combination
should throw ERROR, or at least a log a WARNING?
--
KInd Regards,
Peter Smith.
Fujitsu Australia
FYI, I found that the v14-0001 patch does not currently apply [1]. Can
you please rebase it?
--
[1] http://cfbot.cputube.org/patch_38_3595.log
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Jul 29, 2022 at 7:36 PM Alvaro Herrera wrote:
>
> On 2022-Jul-29, Peter Smith wrote:
>
> > PSA v2 of this patch, modified as suggested.
>
> I don't object to doing this, but I think these two functions should
> stay together nonetheless.
Hmm, I think there i
PSA v2 of this patch, modified as suggested.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v2-0001-Functions-is_publishable_class-and-is_publishable.patch
Description: Binary data
AL and
the origin of the data can be identified from the WAL records.
==
5. src/test/subscription/t/030_origin.pl
+ "Refresh publication when the publisher has subscribed for the new table"
SUGGESTION (Just to mention origin = none somehow. Maybe you can
reword it better than this)
Refresh publication when the publisher has subscribed for the new
table, but the subscriber-side wants origin=none
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Jul 29, 2022 at 11:55 AM houzj.f...@fujitsu.com
wrote:
>
> On Friday, July 29, 2022 7:17 AM Peter Smith wrote:
> > During a recent review, I happened to notice that in the file
> > src/backend/catalog/pg_publication.c the two functions
> &g
2f2ae16d99acdbcb3d05/src/backend/catalog/pg_publication.c
[2]
https://github.com/postgres/postgres/commit/5a2832465fd8984d089e8c44c094e6900d987fcd#diff-1ecc273c7808aba21749ea2718482c153cd6c4dc9d90c69124f3a7c5963b2b4a
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Functions-is_pu
e one table for the partitioned table (tab4) here. Because
+# we specify option "publish_via_partition_root" (see pub_all and
+# pub_lower_level above), all data should be replicated to the partitioned
+# table. So we do not need to create table for the partition table.
"replicated to the partitioned table" ??
The entire comment seems a bit misleading because how can we call the
subscriber table a "partitioned" table when it has no partitions?!
SUGGESTION (maybe?)
Note: We only create one table (tab4) here. We specified
publish_via_partition_root = true (see pub_all and pub_lower_level
above), so all data will be replicated to that table.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
/subscription.out
patching file src/test/regress/sql/subscription.sql
patching file src/test/subscription/t/030_origin.pl
patching file src/tools/pgindent/typedefs.list
--
[1] http://cfbot.cputube.org/patch_38_3610.log
Kind Regards,
Peter Smith.
Fujitsu Australia
g mode.
So for all the times when the user did not ask for streaming=parallel
doesn't this just cause unnecessary overhead for every transaction?
--
[1]
https://www.postgresql.org/message-id/OS3PR01MB62758A6AAED27B3A848CEB7A9E8F9%40OS3PR01MB6275.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
apply changes in an apply background
+worker */
+} ParalleApplySafety;
+
3.12a
Typo in enum and typedef names:
"ParalleApplySafety" -> "ParallelApplySafety"
3.12b
I think the values are quite self-explanatory now. Commenting on each
of them separately is not really ad
faZ6g%40mail.gmail.com
[2] https://www.postgresql.org/docs/devel/protocol-logical-replication.html
Kind Reagrds,
Peter Smith.
Fujitsu Australia.
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
801 - 900 of 1496 matches
Mail list logo
