Hi, the patch v4 LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ify that when using pg_logical_slot_get_changes to consume changes from a
+# logical slot with failover enabled, it will also wait for the slots specified
+# in standby_slot_names to catch up.
+##
AFAICT this test is checking only that the function cannot return
while waiting for the stopped standby, but it doesn't seem to check
that it *does* return when the stopped standby comes alive again.
~~~
31.
+$result =
+ $subscriber1->safe_psql('postgres', "SELECT count(*) = 0 FROM tab_int;");
+is($result, 't',
+ "subscriber1 doesn't get data as the sb1_slot doesn't catch up");
Do you think this fragment should have a comment?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
'logical' flag is like a subcategory (for when
'replication' is true). Therefore, won't the modified check be better
to be written the other way around? This will also be consistent with
the way the Assert was written.
SUGGESTION
if (!replication || logical)
{
...
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Feb 28, 2024 at 1:23 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Tuesday, February 27, 2024 3:18 PM Peter Smith
> wrote:
...
> > 20.
> > +#
> > +# | > standby1 (primary_slot_name = sb1_slot) # | > standby2
> > +(primary_slot_name = sb2_slot)
On Tue, Feb 27, 2024 at 11:35 PM Amit Kapila wrote:
>
> On Tue, Feb 27, 2024 at 12:48 PM Peter Smith wrote:
> >
> > Here are some review comments for v99-0001
> >
> > ==
> > 0. GENERAL.
> >
> > +#standby_slot_names = '' #
#LOGICALDECODING-REPLICATION-SLOTS-SYNCHRONIZATION
[3] [1]
https://www.postgresql.org/message-id/OS0PR01MB5716E581B4227DDEB4DE6C30944F2%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Replace-aka-in-docs.patch
Description: Binary data
This if/else/else seems overly difficult to read. IMO it will be
easier if written like:
SUGGESTION
if (target_lsn > flushed_lsn && !(prioritize_stop && got_STOPPING))
{
*wait_event = WAIT_EVENT_WAL_SENDER_WAIT_FOR_WAL;
return true;
}
if (failover_slot && !StandbySlotsHaveCaughtup(flushed_lsn, elevel))
{
*wait_event = WAIT_EVENT_WAIT_FOR_STANDBY_CONFIRMATION;
return true;
}
return false;
--
Kind Regards,
Peter Smith.
Fujitsu Australia
f we don't have this check, it
> might be no problem.
>
Hi, a while ago I asked this same question. See [1 #28] for the response..
--
[1]
https://www.postgresql.org/message-id/OS0PR01MB571646B8186F6A06404BD50194BDA%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Sat, Mar 2, 2024 at 2:51 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Friday, March 1, 2024 12:23 PM Peter Smith wrote:
> >
...
> > ==
> > src/backend/replication/slot.c
> >
> > 2. validate_standby_slots
> >
> > + else if (!ReplicationSlotCtl)
On Fri, Jan 12, 2024 at 4:07 PM Peter Smith wrote:
>
> On Mon, Jan 30, 2023 at 8:36 AM Tom Lane wrote:
> >
> > Zheng Li writes:
> > > The behavior is due to the following code
> > > https://github.com/postgres/postgres/blob/master/src/backend/commands/define.
On Sun, Mar 3, 2024 at 2:56 PM Amit Kapila wrote:
>
> On Sun, Mar 3, 2024 at 5:17 AM Peter Smith wrote:
> >
...
> > 9. NeedToWaitForWal
> >
> > + /*
> > + * Check if the standby slots have caught up to the flushed position. It
> > + * is good to wait u
On Sun, Mar 3, 2024 at 6:51 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Sunday, March 3, 2024 7:47 AM Peter Smith wrote:
> >
> > 3.
> > +
> > +
> > + Value * is not accepted as it is inappropriate
> > to
> > + block lo
On Mon, Mar 4, 2024 at 2:49 PM Amit Kapila wrote:
>
> On Mon, Mar 4, 2024 at 7:25 AM Peter Smith wrote:
> >
> >
> > ==
> > doc/src/sgml/config.sgml
> >
> > 2.
> > +pg_logical_slot_peek_changes,
> > +when used wit
On Mon, Mar 4, 2024 at 2:38 PM Amit Kapila wrote:
>
> On Mon, Mar 4, 2024 at 6:57 AM Peter Smith wrote:
> >
> > On Sun, Mar 3, 2024 at 2:56 PM Amit Kapila wrote:
> > >
> > > On Sun, Mar 3, 2024 at 5:17 AM Peter Smith wrote:
> &g
idea of the currently flushed
+ * position only if we are not waiting for standbys to catch up.
+ */
Regarding that 1st sentence: maybe this logic used to be done
explicitly "within the loop" but IIUC this logic is now hidden inside
NeedToWaitForWal() so the comment should mention that.
-
doesn't really "Make sure" of
anything because the caller does the check whether we need more space.
All that happens here is allocating more space. Maybe this function
comment should say something like "Double the space allocated for
nodes."
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ndex in bh_nodes. This enables the caller to perform
+ * binaryheap_remove_node_ptr(), binaryheap_update_up/down in O(log n).
+ */
+ bool bh_indexed;
+ bh_nodeidx_hash *bh_nodeidx;
} binaryheap;
I'm wondering why the separate 'bh_indexed' is necessary at all. Can't
you just use the bh_nodeidx value? E.g. If bh_nodeidx == NULL then it
means there is no index tracking, otherwise there is.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
IIUIC it is possible that when 'standby_slot_names' has no value, then
standby_slot_config is not NULL but standby_slot_config->slot_num is
0. So shouldn't that be checked too?
Perhaps it is convenient to encapsulate this check using some macro:
#define StandbySlotNamesHasNoValue() (standby_slot_config = NULL ||
standby_slot_config->slot_num == 0)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Mar 7, 2024 at 2:16 PM Masahiko Sawada wrote:
>
> On Tue, Mar 5, 2024 at 3:28 PM Peter Smith wrote:
> >
> > 4a.
> > The comment in simplehash.h says
> > * The following parameters are only relevant when SH_DEFINE is defined:
> > * - SH_KEY -
ame? Won't it
be better to give the field a better name -- e.g. "txn_maxheap" or
similar?
~
12b.
This comment should also say that the heap is ordered by tx size --
(e.g. the comparator is ReorderBufferTXNSizeCompare)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Mar 12, 2024 at 4:23 PM Masahiko Sawada wrote:
>
> On Fri, Mar 8, 2024 at 12:58 PM Peter Smith wrote:
> >
...
> > > > 5.
> > > > + *
> > > > + * If 'indexed' is true, we create a hash table to track of each node's
> > &
On Wed, Mar 13, 2024 at 12:48 PM Masahiko Sawada wrote:
>
> On Wed, Mar 13, 2024 at 10:15 AM Peter Smith wrote:
> >
> > On Tue, Mar 12, 2024 at 4:23 PM Masahiko Sawada
> > wrote:
> > >
> > > On Fri, Mar 8, 2024 at 12:58 PM Peter S
/blob/0b84f5c419a300dc1b1a70cf63b9907208e52643/src/backend/replication/slotfuncs.c#L989
[2]
https://github.com/postgres/postgres/blob/0b84f5c419a300dc1b1a70cf63b9907208e52643/src/backend/replication/logical/slotsync.c#L1258
Kind Regards,
Peter Smith.
Fujitsu Australia
&CASE_EEOP_LAST
};
- StaticAssertStmt(EEOP_LAST + 1 == lengthof(dispatch_table),
+ StaticAssertDecl(EEOP_LAST + 1 == lengthof(dispatch_table),
"dispatch_table out of whack with ExprEvalOp");
Ditto the previous comment.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
fest [1], so I
assumed it would just carry forward to the next one.
Anyway, I've added it again to 2023-01 commitfest [2]. Thanks for telling me.
--
[1] 2022-11 CF - https://commitfest.postgresql.org/40/3959/
[2] 2023-01 CF - https://commitfest.postgresql.org/41/4061/
Kind Regards,
Peter Smith.
Fujitsu Australia.
e those shown below might be more appropriate:
stream_checks_work_mem = true/false
stream_mode_checks_size = true/false
stream_for_large_tx_only = true/false
... etc.
The GUC name length could get a bit unwieldy but isn't it better for
it to have the correct meaning than to have a shorter but slightly
ambiguous name? Anyway, it is a developer option so I guess longer
names are less of a problem.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Dec 13, 2022 at 6:25 AM Alvaro Herrera wrote:
>
> On 2022-Dec-07, samay sharma wrote:
>
> > On Tue, Dec 6, 2022 at 11:12 PM Peter Smith wrote:
>
> > > OK. I copied the tablesync note back to config.sgml definition of
> > > 'max_replication
/a8500750ca0acf6bb95cf9d1ac7f421749b22db7
Kind Regards,
Peter Smith.
Fujitsu Australia
ired by the leader worker so it can update
the lsn_mappings." ??
--
Kind Regards,
Peter Smith.
Fujitsu Australia
er is already
launched, then that will have no effect on the already running
walsender.
Is that understanding correct?
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Tue, Dec 13, 2022 at 2:33 PM Peter Smith wrote:
>
> On Tue, Dec 6, 2022 at 5:23 PM shiy.f...@fujitsu.com
> wrote:
> >
> > Hi hackers,
> >
> > In logical decoding, when logical_decoding_work_mem is exceeded, the
> > changes are
> > sent to ou
K1%2BwyN6zpaHUkCLorEWNx75MG0xhMwcFhvjqm2KURZEAGw%40mail.gmail.com
[2] isolation tests -
https://github.com/postgres/postgres/blob/master/src/test/isolation/README
[3] test_decoding spec tests -
https://github.com/postgres/postgres/tree/master/contrib/test_decoding/specs
Kind Regards,
Peter Smi
On Mon, Dec 19, 2022 at 5:04 PM Tom Lane wrote:
>
> Peter Smith writes:
> > My patch/idea makes a small change to the isolationtester spec
> > grammar. Now each session can optionally specify its own connection
> > string. When specified, this will override any connec
On Mon, Dec 19, 2022 at 5:35 PM Peter Smith wrote:
>
> On Mon, Dec 19, 2022 at 5:04 PM Tom Lane wrote:
> >
> > Peter Smith writes:
> > > My patch/idea makes a small change to the isolationtester spec
> > > grammar. Now each session can optionally specify i
maintaining consistency throughout is best. e.g. I can imagine maybe
someone searching for "Warning" in the docs, and now they are not
going to find this one.
Maybe a safer way to fix this "silly otherwise-empty section title"
would be to just add some explanatory text so it is no longer empty.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Dec 20, 2022 at 2:20 PM Amit Kapila wrote:
>
> On Tue, Dec 20, 2022 at 8:17 AM Peter Smith wrote:
> >
> > Summary
> > ---
> >
> > In summary, everything I have tested so far appeared to be working
> > properly. In other words, for overlapp
On Tue, Dec 20, 2022 at 7:21 PM Alvaro Herrera wrote:
>
> On 2022-Dec-20, Peter Smith wrote:
>
> > If you change this warning title then it becomes the odd one out -
> > every other warning in all the pg docs just says "Warning". IMO
> > maintaining consi
On Tue, Dec 20, 2022 at 2:29 AM Alvaro Herrera wrote:
>
> On 2022-Oct-31, Peter Smith wrote:
>
> > 6. add_policy_clauses
> >
> > + else
> > + {
> > + append_bool_object(policyStmt, "present", false);
> > + }
> >
> > Something s
Looks like a recent commit [1] to add copyrights broke the patch
--
[1]
https://github.com/postgres/postgres/commit/8284cf5f746f84303eda34d213e89c8439a83a42
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Dec 20, 2022 at 5:22 PM Peter Smith wrote:
>
> On Tue, Dec 20, 2022 at 2:20 PM Amit Kapila wrote:
> >
> > On Tue, Dec 20, 2022 at 8:17 AM Peter Smith wrote:
> > >
> > > Summary
> > > ---
> > >
> > > In summary, everyt
ence
SUGGESTION
Force sending any streaming changes to the output plugin immediately
without waiting until logical_decoding_work_mem is exceeded."),
--
[1] https://www.postgresql.org/docs/devel/logical-replication-config.html
[2] GUC declarations -
https://github.com/postgres/postgres/commit/d9d873bac67047cfacc9f5ef96ee488f2cb0f1c3
Kind Regards,
Peter Smith.
Fujitsu Australia
sting and
> debugging but setting logical_decoding_work_mem = 0 doesn't benefit
> users at all, rather brings risks.
>
> I prefer the idea Kuroda-san previously proposed; setting
> logical_decoding_mode = 'immediate' means setting
> logical_decoding_work_mem = 0. We might not need to have it as an enum
> parameter since it has only two values, though.
Did you mean one GUC (logical_decoding_mode) will cause a side-effect
implicit value change on another GUC value
(logical_decoding_work_mem)?
If so, then that seems a like potential source of confusion IMO.
- e.g. actual value is invisibly set differently from what the user
sees in the conf file
- e.g. will it depend on the order they get assigned
Are there any GUC precedents for something like that?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Dec 21, 2022 at 6:59 PM Alvaro Herrera wrote:
>
> On 2022-Dec-21, Peter Smith wrote:
>
> > By "searching" I also meant just scanning visually, although I was
> > thinking more about scanning the PDF.
> >
> > Right now, the intention of any tex
On Wed, Jan 4, 2023 at 6:08 PM vignesh C wrote:
>
> On Mon, 2 Jan 2023 at 13:47, Peter Eisentraut
> wrote:
> >
> > On 08.12.22 03:30, Peter Smith wrote:
> > > PSA patches for v9*
> > >
> > > v9-0001 - Now the table rows are ordered per PeterE
latest_end_time) ON ((st.subid = su.oid)));
+ LEFT JOIN pg_stat_get_subscription(NULL::oid) st(subid, relid,
pid, apply_leader_pid, received_lsn, last_msg_send_time,
last_msg_receipt_time, latest_end_lsn, latest_end_time) ON ((st.subid
= su.oid)));
pg_stat_subscription_stats| SELECT ss.subid,
s.subname,
ss.apply_error_count,
(Same comment as elsewhere)
"apply_leader_pid" --> "leader_apply_pid"
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ostgresql.org/message-id/CAHut%2BPvA10Bp9Jaw9OS2%2BpuKHr7ry_xB3Tf2-bbv5gyxD5E_gw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
not a "parallel apply" worker then it can only be
one of the other 2. So I think it is sufficient and less confusing to
say:
Process ID of the leader apply worker if this process is a parallel
apply worker; NULL if this process is a leader apply worker or a
synchronization worker.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
40OS0PR01MB5716.jpnprd01.prod.outlook.com
[2] Amit -
https://www.postgresql.org/message-id/CAA4eK1J%3D9m-VNRMHCqeG8jpX0CTn3Ciad2o4H-ogrZMDJ3tn4w%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
function here is pretty much in the
correct numerical order except this one, so IMO this code ought to be
relocated to later in this same function.
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Mon, Jan 16, 2023 at 5:41 PM Amit Kapila wrote:
>
> On Mon, Jan 16, 2023 at 10:24 AM Peter Smith wrote:
> >
> > 2.
> >
> > /*
> > + * Return the pid of the leader apply worker if the given pid is the pid
> > of a
> > + * pa
On Tue, Jan 17, 2023 at 1:21 PM houzj.f...@fujitsu.com
wrote:
>
> On Tuesday, January 17, 2023 5:43 AM Peter Smith
> wrote:
> >
> > On Mon, Jan 16, 2023 at 5:41 PM Amit Kapila
> > wrote:
> > >
> > > On Mon, Jan 16, 2023 at 10:24
On Tue, Jan 17, 2023 at 2:37 PM houzj.f...@fujitsu.com
wrote:
>
> On Tuesday, January 17, 2023 11:32 AM Peter Smith
> wrote:
> >
> > On Tue, Jan 17, 2023 at 1:21 PM houzj.f...@fujitsu.com
> > wrote:
> > >
> > > On Tuesday, January 17, 2023 5:43 AM P
l -rn . -e "linkend='" | wc -l
12
(double-quotes)
$ grep --include=*.sgml -rn . -e 'linkend="' | wc -l
5915
~~
PSA patch that makes them all use double quotes.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Replace-linkend-single-quotes-with-double-quotes.patch
Description: Binary data
100,SubscriptionRelationId)
BKI_SHARED_RELATION BKI_ROW
XLogRecPtr subskiplsn; /* All changes finished at this LSN are
* skipped */
+ int64 subminapplydelay; /* Replication apply delay */
+
NameData subname; /* Name of the subscription */
Oid subowner BKI_LOOKUP(pg_authid); /* Owner of the subscription */
SUGGESTION (for comment)
Replication apply delay (ms)
~~
24.
@@ -120,6 +122,7 @@ typedef struct Subscription
* in */
XLogRecPtr skiplsn; /* All changes finished at this LSN are
* skipped */
+ int64 minapplydelay; /* Replication apply delay */
SUGGESTION (for comment)
Replication apply delay (ms)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ut%2BPv5Efz1TLWOLSoFvoyC0mq%2Bs92yFSd534ctWSdjEFtKCw%40mail.gmail.com
[4] DJ how to do it using refentry -
https://www.postgresql.org/message-id/CAKFQuwYkM5UZT%2B6tG%2BNgZvDcd5VavS%2BxNHsGsWC8jS-KJsxh7w%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Jan 18, 2023 at 6:06 PM Peter Smith wrote:
>
> Here are my review comments for the latest patch v16-0001. (excluding
> the test code)
>
...
>
> 8. AlterSubscription (general)
>
> I observed during testing there are 3 different errors….
>
> At subscriptio
On Wed, Jan 18, 2023 at 6:06 PM Peter Smith wrote:
>
> Here are my review comments for the latest patch v16-0001. (excluding
> the test code)
>
And here are some review comments for the v16-0001 test code.
==
src/test/regress/sql/subscription.sql
1. General
For all com
n should that comment be modified to say
/* not strictly necessary for LOGICALREP_COLUMN_BINARY but per
StringInfo practice */
--
Kind Regards,
Peter Smith.
Fujitsu Australia
esting normal logical replication behavior.
#
~
I think you should add a couple of INSERTS for the newly added table/s
also. IMO it is not only better for test completeness, but it causes
readers to question why there are INSERTS for every other table except
these ones.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
y "streaming option").
==
doc/src/sgml/ref/alter_subscription.sgml
The SKIP part says "... enabling two_phase on subscriber.". I thought
there could be a link for "two_phase" here (also "on subscriber" -->
"on the subscriber").
--
Kind Regards,
Peter Smith.
Fujitsu Australia
o the "publish" parameter.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
:
+# tap_pub_parent_sync filter is: (a > 15)
+# tap_pub_child_sync filter is: (a < 15)
Maybe wrapping can be improved in the above comment and a full stop
added to the first sentence.
Otherwise, I have no more comments for v24.
--
Kind Regards,
Peter Smith
Pvzo6%3DKKLqMR6-mAQdM%2Bj_dse0eUreGmrFouL7gbLbv2w%40mail.gmail.com#7da8d0e3b73096375847c16c856b4aed
Kind Regards,
Peter Smith.
Fujitsu Australia
sher or the subscriber.
There is a cut/paste typo here -- it renders badly with "FOR TABLES IN
SCHEMA" appearing 2x.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
>
- originate (see CREATE
PUBLICATION).
+ originate (see publish_via_partition_root
+ of CREATE PUBLICATION).
Hmm, my above-suggested wording was “publish_via_partition_root
parameter “ but it seems you (accidentally?) omitted the word
“parameter”.
Otherwise, the patch v4-0002 also LGTM
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Thanks for this patch.
v5-0001 looks good to me.
v5-0002 looks good to me.
I've marked the CF entry [1] as "ready for committer".
--
[1] https://commitfest.postgresql.org/43/4256/
Kind Regards,
Peter Smith.
Fujitsu Australia
e
a default tableRow[2] that is valid for one case and overwrite it only
for the other case, instead of overwriting it in 2 places.
YMMV.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
UPDATE
or DELETE, so the code would be better to include a sanity Assert.
SUGGESTION
if (change->data.tp.oldtuple)
{
Assert(action == REORDER_BUFFER_CHANGE_UPDATE || action ==
REORDER_BUFFER_CHANGE_DELETE);
...
}
==
6.
I suggest moving the "change->data.tp.oldtuple" check before the
"change->data.tp.newtuple" check. I don't think it makes any
difference, but it seems more natural IMO to have old before new.
--
Kind Regards,
Peter Smith
On Thu, Mar 30, 2023 at 12:57 PM Hayato Kuroda (Fujitsu)
wrote:
>
> Dear hackers,
>
> This thread is motivated from [1]. This patch adds some links that refer
> publication options.
>
Is this the correct attachment?
------
Kind Regards,
Peter Smith,
Fujitsu Australia
"ready for committer"
--
[1]
https://www.postgresql.org/message-id/TYAPR01MB5866879FFE5E0A2726244216F5889%40TYAPR01MB5866.jpnprd01.prod.outlook.com
[2] https://commitfest.postgresql.org/43/4260/
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi Hou-san,
I looked again at v4-0001.
On Thu, Mar 30, 2023 at 2:01 PM houzj.f...@fujitsu.com
wrote:
>
> On Thursday, March 30, 2023 9:15 AM Peter Smith wrote:
> >
...
> >
> > 2.
> > /* Convert tuple if needed. */
> > if (relentry-> attrmap)
> > {
atches are the latest, you will be easily able to unambiguously
refer to them by name in subsequent posts, and when copied to your
local computer they won't clash with any older copied patches.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ON'T
want to get duplicated data from other partitions (because you already
knew about those ones -- like your example does).
So, I am not sure what the answer is, or maybe there isn't one.
At least, we need to check there are sufficient "BE CAREFUL" warnings
in the documentation for scenarios like this.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
users are self-evident from your test cases.
--
[1]
https://docs.timescale.com/use-timescale/latest/hypertables/about-hypertables/
[2] https://www.crunchydata.com/blog/native-partitioning-with-postgres
[3] https://github.com/pgpartman/pg_partman
Kind Regards,
Peter Smith.
Fujitsu Australia
FYI, the WIP patch does not seem to apply cleanly anymore using the latest HEAD.
See the cfbot rebase logs [1].
--
[1] http://cfbot.cputube.org/patch_42_4225.log
Kind Regards,
Peter Smith.
Fujitsu Australia
; option.
~~
AFAICT the associated tab-complete code was accidentally omitted.
PSA patches to add those tab completions.
--
[1]
https://github.com/postgres/postgres/commit/c3afe8cf5a1e465bd71e48e4bc717f5bfdc7a7d6
[2]
https://github.com/postgres/postgres/commit/482675987bcdffb390ae735cfd5
~~
PSA a tiny patch to fix that.
--
[1]
https://github.com/postgres/postgres/commit/1e10d49b65d6c26c61fee07999e4cd59eab2b765
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Fix-comment-typo.patch
Description: Binary data
Thanks for pushing.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
SK)
probably this should be called DB_DUMP_SLOTS_FILE_MASK.
~
20b.
Because the content of this dump/restore file is SQL (not custom
binary) wouldn't a filename suffix ".sql" be better?
==
.../pg_upgrade/t/003_logical_replication.pl
21.
Some parts (formatting, comments, etc) in thi
://cfbot.cputube.org/next.html
[2] https://commitfest.postgresql.org/43/4246/
[3] https://commitfest.postgresql.org/43/4266/
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Apr 11, 2023 at 4:36 PM Thomas Munro wrote:
>
> On Tue, Apr 11, 2023 at 6:16 PM Peter Smith wrote:
> > cfbot [1] is listing some already committed patches under the "Needs
> > Review" category. For example here are some of mine [1][2]. And
> > becau
ot' but
AFAIK the proper option name everywhere else in this patch is
'--include-logical-replication-slots' (with the 's')
~
5b.
I'm not sure that "pg_upgrade for new publisher" makes sense.
It's more like "pg_upgrade of old publisher", or simply "pg_upgrade of
publisher"
--
[1]
https://www.postgresql.org/message-id/TYCPR01MB5870E212F5012FD6272CE1E3F5969%40TYCPR01MB5870.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
adding more checks or an assert
here because... */
OTOH, "Note" is just for highlighting why something is the way it is,
but with no implication that it should be revisited/changed in the
future.
e.g.
/* Note: We deliberately do not test the state here because... */
/* Note: This memory must be zeroed because... */
/* Note: This string has no '\0' terminator so... */
--
Kind Regards,
Peter Smith.
Fujitsu Australia
looks a bit strange that you used it for only some of
the columns but not others.
~~~
3.
+
+ /* FIXME: force dumping */
+ slotinfo[i].dobj.dump = DUMP_COMPONENT_ALL;
Why the "FIXME" here? Are you intending to replace this code with
something else?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
the message can include the names
of the failing subscription and/or the relation that was in the wrong
state. Maybe that means moving all this checking logic into the
pg_dump code?
==
src/bin/pg_upgrade/option.c
16. parseCommandLine
user_opts.transfer_mode = TRANSFER_MODE_COPY;
+ user_opt
out WHAT this is testing or WHY it should still have 2 rows.
~~~
5.
# Refresh the subscription, only the missing row on t2 show be replicated
/show/should/
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
PLICATION SLOT (ID %d NAME
%s)"?
==
.../pg_upgrade/t/003_logical_replication.pl
3.
Should the name of this TAP test file really be 03_logical_replication_slots.pl?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
dlreplok */
+v(CMDTAG_UNKNOWN, "???", false, false, false, false)
Although these are not strictly the same as the PG_CMDTAG macro arg
names, it might be better in this comment to call this "ddl_repl_ok"
instead of "ddlreplok" for consistency with the rest of the comment.
==
src/test/regress/expected/publication.out
21.
The \dRp+ now exposes a new column called "Table DDLS"
I expected to see some tests for t/f values but I did not find any
test where the expected output for this column was 't'.
==
src/tools/pgindent/typedefs.list
22.
LogicalDecodeCommitPreparedCB
+LogicalDecodeDDLMessageCB
+LogicalDecodeStreamDDLMessageCB
LogicalDecodeFilterByOriginCB
The alphabetical order is not correct for LogicalDecodeStreamDDLMessageCB
~~~
23.
ReorderBufferCommitPreparedCB
+ReorderBufferDDLMessageCB
+ReorderBufferStreamDDLMessageCB
ReorderBufferDiskChange
The alphabetical order is not correct for ReorderBufferStreamDDLMessageCB
--
Kind Regards,
Peter Smith.
Fujitsu Australia
X" (e.g. like here
https://www.postgresql.org/about/featurematrix/).
~~~
16.
+
+ The DDL deparser utility is invoked during the replication of
DDLs. The DDL
+ deparser is capable of converting a DDL command into formatted
JSON blob, with
+ the necessary information to reconstruct the DDL commands at
the destination. The
+ benefit of using the deparser output compared to the original
command string
+ includes:
"The benefit ... includes:" --> "The benefits ... include:"
~~~
17.
+ The DDL deparser exposes two SQL functions:
+
I imagine that these SQL functions should be documented elsewhere as well.
Possibly on this page?
https://www.postgresql.org/docs/current/functions-admin.html#FUNCTIONS-REPLICATION
--
Kind Regards,
Peter Smith.
Fujitsu Australia
sizeof(Oid));
+ data += sizeof(Oid);
+ memcpy(data, &change->data.ddl.cmdtype, sizeof(DeparsedCommandType));
+ data += sizeof(int);
+ memcpy(data, change->data.ddl.prefix, prefix_size);
+ data += prefix_size;
Isn't it better to use sizeof(DeparsedCommandType) instead of
sizeof(int)
>exprstate, 0, sizeof(entry->exprstate));
Continually adding to these assignment has got a bit out of control...
IMO the code now would be better written as:
memset(entry->pubactions, 0, sizeof(entry->pubactions));
And doing this would also be consistent with the similar code for
entry->exprstate (just a couple of lines below here).
~~~
35. get_rel_sync_entry
entry->pubactions.pubupdate = false;
entry->pubactions.pubdelete = false;
entry->pubactions.pubtruncate = false;
+ entry->pubactions.pubddl_table = false;
(same as above review comment #35)
IMO all this should be written more simply as:
memset(entry->pubactions, 0, sizeof(entry->pubactions));
--
[1]
https://www.postgresql.org/message-id/CAHut%2BPtzpuuRFrLnjkQePq296ip_0WfmQ4CtydM9JDR6gEf%3DQw%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CAHut%2BPtMkVoweJrd%3DSLw7BfpW883skasdnemoj4N19NnyjrT3Q%40mail.gmail.com
[3]
https://www.postgresql.org/message-id/cahut+pug8j8ua5v-f-o4tczhvfswgg1b8ql+ezo0hjwweey...@mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
/regress/expected/psql.out
38.
IMO the column output is not in a good order. See review comments for describe.c
==
src/test/regress/expected/publication.out
39.
I previously posted a review comment about some missing test cases
([1] #21). The reply was "they will be added in later patches". I
think it's better to put the relevant tests in the same patch that
introduced the functionality.
--
My previous review posts for this patch
[1]
https://www.postgresql.org/message-id/CAHut%2BPtzpuuRFrLnjkQePq296ip_0WfmQ4CtydM9JDR6gEf%3DQw%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CAHut%2BPtMkVoweJrd%3DSLw7BfpW883skasdnemoj4N19NnyjrT3Q%40mail.gmail.com
[3]
https://www.postgresql.org/message-id/cahut+pug8j8ua5v-f-o4tczhvfswgg1b8ql+ezo0hjwweey...@mail.gmail.com
[4]
https://www.postgresql.org/message-id/CAHut%2BPtOODRybaptKRKUWZnGw-PZuLF2BxaitnMSNeAiU8-yPg%40mail.gmail.com
[5] Houz replies to my previous review comments -
https://www.postgresql.org/message-id/OS0PR01MB571690B2DB46B1C4AF61D184946F9%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Sat, May 6, 2023 at 5:28 AM David Zhang wrote:
>
> On 2023-03-16 4:46 p.m., Peter Smith wrote:
> > A rebase was needed due to the recent REPLICA IDENTITY push [1].
> >
> > PSA v2.
> >
> >
> > - A published table must have a replica identity
&
lot by executing ALTER SUBSCRIPTION ... SET (slot_name =
NONE).
SUGGESTION
To proceed in this situation, first DISABLE the subscription, and then
disassociate it from the replication slot by executing ALTER
SUBSCRIPTION ... SET (slot_name = NONE).
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, May 8, 2023 at 1:09 PM Michael Paquier wrote:
>
> On Mon, May 08, 2023 at 10:29:50AM +1000, Peter Smith wrote:
> > Thanks for giving some feedback on my patch.
>
> Looks OK.
>
> While on it, looking at logical-replication.sgml, it seems to me that
> these two
On Mon, May 8, 2023 at 2:05 PM Michael Paquier wrote:
>
> On Mon, May 08, 2023 at 01:57:33PM +1000, Peter Smith wrote:
> > I agree. Do you want me to make a new v4 patch to also do that, or
> > will you handle them in passing?
>
> I'll just go handle them on the wa
ation_slot('test_slot',
'test_decoding', false, true);
2023-05-09 12:19:25.335 AEST [32564] LOG: received immediate shutdown request
2023-05-09 12:19:25.337 AEST [32564] LOG: database system is shut down
~
Is it a bug? Or, if I am doing something wrong please let me know how
to run the test.
~~~
19.
+# Clean up
+rmtree($new_node->data_dir . "/pg_upgrade_output.d");
+$new_node->append_conf('postgresql.conf', "wal_level = 'logical'");
+$new_node->append_conf('postgresql.conf', "max_replication_slots = 0");
I think the last 2 lines are not "clean up". They are preparations for
the subsequent test, so maybe they should be commented as such.
~~~
20.
+# Clean up
+rmtree($new_node->data_dir . "/pg_upgrade_output.d");
+$new_node->append_conf('postgresql.conf', "max_replication_slots = 10");
I think the last line is not "clean up". It is preparation for the
subsequent test, so maybe it should be commented as such.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
e. Probably
they should be consistent.
~
13b.
Something seems amiss. Here the is_error is assigned true; But later
when you test is_error that is for logging the ready-state problem.
Isn't there another missing pg_fatal for this invalid remote_lsn case?
==
src/bin/pg_upgrade/option.c
1
On Mon, Apr 24, 2023 at 4:19 PM Julien Rouhaud wrote:
>
> Hi,
>
> On Thu, Apr 13, 2023 at 03:26:56PM +1000, Peter Smith wrote:
> >
> > 1.
> > All the comments look alike, so it is hard to know what is going on.
> > If each of the main test parts could be high
same info?
==
.../pg_upgrade/t/003_logical_replication_slots.pl
5.
+# Preparations for the subsequent test. The case max_replication_slots is set
+# to 0 is prohibit.
/prohibit/prohibited/
--
[1] My v10 review -
https://www.postgresql.org/message-id/CAHut%2BPtpQaKVfqr-8KUtGZqei1C9gWF0%2BY8
501 - 600 of 1980 matches
Mail list logo
