Thanks for addressing my previous comments. Now I have looked at v19.
On Mon, Feb 21, 2022 at 11:25 AM osumi.takami...@fujitsu.com
wrote:
>
> On Friday, February 18, 2022 3:27 PM Peter Smith
> wrote:
> > Hi. Below are my code review comments for v18.
> Thank you for your
On Mon, Feb 21, 2022 at 11:44 PM osumi.takami...@fujitsu.com
wrote:
>
> On Monday, February 21, 2022 2:56 PM Peter Smith
> wrote:
> > Thanks for addressing my previous comments. Now I have looked at v19.
> >
> > On Mon, Feb 21, 2022 at 11:25 AM osumi.takam
FYI - the latest v18 patch no longer applies due to a recent push [1].
--
[1]
https://github.com/postgres/postgres/commit/52e4f0cd472d39d07732b99559989ea3b615be78
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Feb 22, 2022 at 3:11 PM osumi.takami...@fujitsu.com
wrote:
>
> On Tuesday, February 22, 2022 7:53 AM Peter Smith
> wrote:
> > On Mon, Feb 21, 2022 at 11:44 PM osumi.takami...@fujitsu.com
> > wrote:
> > >
> > > On Monday, February 21, 2022 2:56 PM
- * subworkers is the hash table of PgStat_StatSubWorkerEntry which stores
- * statistics of logical replication workers: apply worker and table sync
- * worker.
+ * tables, functions, and subscription must be last in the struct, because
+ * we don't write the pointers out to the stats file.
*/
Should that say "tables, functions, and subscriptions" (plural)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ot the tablesync worker (which already completed)
"Truncate test_tab1 so that table sync can continue." --> "Truncate
test_tab1 so that the apply worker can continue."
--
[Osumi]
https://www.postgresql.org/message-id/CAD21AoBRt%3DcyKsZP83rcMkHnT498gHH0TEP34fZBrGCxT-Ahwg%40mail.gmail.com
[Tang]
https://www.postgresql.org/message-id/TYCPR01MB612840D018FEBD38268CC83BFB3C9%40TYCPR01MB6128.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
cgi-bin/show_log.pl?nm=komodoensis&dt=2022-02-23%2016%3A12%3A03
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Thu, Feb 24, 2022 at 1:33 PM Amit Kapila wrote:
>
> On Thu, Feb 24, 2022 at 7:57 AM Peter Smith wrote:
> >
> > I noticed that there was a build-farm failure on the machine 'komodoensis'
> > [1]
> >
> > # Failed test 'check r
se some
future unknown stats columns get added? But it means now there is a
corresponding function "pg_stat_reset_subscription_activity", when in
practice you don't really reset activity - what you want to do is
reset some statistics *about* the activity... so it all seems a bit
odd to me.
--
[Tang-v2]
https://www.postgresql.org/message-id/OS0PR01MB6113769B17E90ADC9ACA14B2FB3D9%40OS0PR01MB6113.jpnprd01.prod.outlook.com
[Peter-v1]
https://www.postgresql.org/message-id/CAHut%2BPtH-uN5rbGRh-%3DkCd8xvQYDf_JCcjLcVjW3OXGz6T%2BxCw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
g.2. "One row per subscription, showing statistics about
subscription activity." --> "One row per subscription, showing
statistics about that subscription."
-
[Peter-v2]
https://www.postgresql.org/message-id/CAHut%2BPv%3DVmXtHmPKp4fg8VDF%2BTQP6xWgL91Jn-hrqg5QObfCZA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
SyncRepWaitForLSN' before it was turned
into a function, and there is a long comment in SyncRepWaitForLSN
describing the risks of this logic. e.g.
... If it's true, we need to check it again
* later while holding the lock, to check the flag and operate the sync
* rep queue atomically. This is necessary to avoid the race condition
* described in SyncRepUpdateSyncStandbysDefined().
This same function is now called from walsender.c. I think maybe it is
OK but please confirm it.
Anyway, the point is maybe this SyncRepEnabled function should be
better commented to make some reference about the race concerns of the
original comment. Otherwise some future caller of this function may be
unaware of it and come to grief.
---
Kind Regards,
Peter Smith.
Fujitsu Australia
ats */
+} PgStat_MsgResetsubcounter;
BEFORE
InvalidOid for clearing all subscription stats
SUGGESTED
InvalidOid means reset all subscription stats
--
Kind Regards,
Peter Smith.
Fujitsu Australia
028_disable_on_error.pl - filename
The 028 number needs to be bumped because there is already a TAP test
called 028 now
~~~
9. src/test/subscription/t/028_disable_on_error.pl - missing test
There was no test case for the last combination where the user correct
the apply worker problem: E.g. After a prev
extra blank line can be removed.
~~~
20. Test for the column names.
The patch added a couple of new columns to statistics so I was
surprised there were no regression test updates needed for these? How
can that be? Shouldn’t there be just one regression test that
validates the view column names are what they are expected to be?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
at the previously failing insert was applied OK.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
elation "public.test" in transaction 726 committed at LSN
0/14BFA88 and timestamp 2022-02-28 20:58:27.964238+09 from
replication origin "pg_16395"
After:
CONTEXT: processing remote data during "INSERT" for replication target
relation "public.test" (txid 726, LSN 0/14BFA88, ts 2022-02-28
20:58:27.964238+09, origin "pg_16395")
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
7732b99559989ea3b615be78
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-PG-docs-Logical-Replication-Filtering.patch
Description: Binary data
ersion number for future patch attachments?
--
KInd Regards,
Peter Smith.
Fujitsu Australia
On Wed, Mar 2, 2022 at 8:43 PM Aleksander Alekseev
wrote:
...
> Here is an updated version of the patch.
Thanks for your review comments and fixes. The updated v2 patch looks
good to me.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
, we can always extend the
> existing docs.
>
+1
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Thu, Mar 3, 2022 at 2:15 PM Amit Kapila wrote:
>
> On Wed, Mar 2, 2022 at 8:00 PM Peter Eisentraut
> wrote:
> >
> > On 02.03.22 05:47, Peter Smith wrote:
> > > This patch introduces a new "Filtering" page to give a common place
> > > where
PSA patch v3 to address all comments received so far.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v3-0001-Update-the-documentation-for-logical-replication.patch
Description: Binary data
On Thu, Mar 3, 2022 at 2:15 PM Amit Kapila wrote:
>
> On Wed, Mar 2, 2022 at 8:00 PM Peter Eisentraut
> wrote:
> >
> > On 02.03.22 05:47, Peter Smith wrote:
> > > This patch introduces a new "Filtering" page to give a common place
> > > where
ck if synchronous replication is enabled
+ */
+bool
+SyncRepEnabled(void)
Missing period for that function comment.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
PSA patch to fix a comment typo.
(The 'OR' should not be uppercase - that keyword is irrelevant here).
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Fix-comment-typo-CheckCmdReplicaIdentity.patch
Description: Binary data
e table synchronization failure in an idle state.
> + */
> +HOLD_INTERRUPTS();
> +EmitErrorReport();
> + FlushErrorState();
> +AbortOutOfAnyTransaction();
> +RESUME_INTERRUPTS();
> +pgstat_report_subscription_error(MySubscription->oid, false);
> +
> +if (MySubscription->disableonerr)
> +{
> +DisableSubscriptionOnError();
> +proc_exit(0);
> +}
> +
> +PG_RE_THROW();
>
> If the disableonerr is false, the same error is reported twice. Also,
> the code in PG_CATCH() in both start_apply() and start_table_sync()
> are almost the same. Can we create a common function to do post-error
> processing?
>
> The worker should exit with return code 1.
>
> I've attached a fixup patch for changes to worker.c for your
> reference. Feel free to adopt the changes.
The way that common function is implemented required removal of the
existing PG_RE_THROW logic, which in turn was only possible using
special knowledge that this just happens to be the last try/catch
block for the apply worker. Yes, I believe everything will work ok,
but it just seemed like a step too far for me to change the throw
logic. I feel that once you get to the point of having to write
special comments in the code to explain "why we can get away with
doing this..." then that is an indication that perhaps it's not really
the best way...
Is there some alternative way to share common code, but without having
to change the existing throw error logic to do so?
OTOH, maybe others think it is ok?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
current xid. The similar functions
> logicalmsg_decode() or heap_decode() do call ReorderBufferProcessXid
> even if they decide not to queue or send the change. Is there a reason
> for not doing the same here? However, I am not able to deduce any
> scenario where lack of this will lead to such an Assertion failure.
> Any thoughts?
>
--
[1]
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2022-03-03%2023%3A14%3A26
Kind Regards,
Peter Smith.
Fujitsu Australia
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
why does the patch use syntax option 1?
--
Kind Regards,
Peter Smith
Fujitsu Australia
On Mon, Mar 7, 2022 at 3:56 PM Peter Smith wrote:
>
> Hi Vignesh, I also have not looked at the patch yet, but I have what
> seems like a very fundamental (and possibly dumb) question...
>
> Basically, I do not understand the choice of syntax for setting things up.
>
>
On Mon, Mar 7, 2022 at 4:20 PM vignesh C wrote:
>
> On Mon, Mar 7, 2022 at 10:26 AM Peter Smith wrote:
> >
> > Hi Vignesh, I also have not looked at the patch yet, but I have what
> > seems like a very fundamental (and possibly dumb) question...
> >
> > Basic
t is introduced as option2, however, maybe pub2 can be reused.
> i.e. multiple declaration of publications can be avoided.
>
Yes. Thanks for the example. I had the same observation in my last post [1]
--
[1]
https://www.postgresql.org/message-id/CAHut%2BPtRxiQR_4UFLNThg-NNRV447FvwtcR-BvqMzj
On Mon, Mar 7, 2022 at 6:17 PM vignesh C wrote:
>
> On Mon, Mar 7, 2022 at 11:45 AM Peter Smith wrote:
> >
> > On Mon, Mar 7, 2022 at 4:20 PM vignesh C wrote:
> > >
> > > On Mon, Mar 7, 2022 at 10:26 AM Peter Smith wrote:
> > > >
> > > &
comment.
~~~
15. src/test/regress/sql/subscription.sql
ALTER SUBSCRIPTION test missing?
--
[1]
https://www.postgresql.org/message-id/CAHut%2BPsAWaETh9VMymbBfMrqiE1KuqMq%2BwpBg0s7eMzwLATr%2Bw%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CAHut%2BPvQonJd5epJBM0Yfh1499mL9kTL9a%3DGrMhvnL6Ok05zqw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
d/CAHut%2BPucrizJpqhSyD7dKj1yRkNMskqmiekD_RRqYpdDdusMRQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
nd up with an option that
turned out to be inconsistent looking on the subscriber-side /
publisher-side.
- we should try to avoid accidentally painting ourselves into a corner
(e.g. stuck with a boolean option that cannot be enhanced later on)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Mar 8, 2022 at 4:31 PM Michael Paquier wrote:
>
> On Mon, Mar 07, 2022 at 10:28:08AM +0800, Julien Rouhaud wrote:
> > +1
>
> And done.
> --
> Michael
Thanks!
------
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Mar 8, 2022 at 4:21 PM Amit Kapila wrote:
>
> On Tue, Mar 8, 2022 at 10:31 AM Peter Smith wrote:
> >
> > IIUC the new option may be implemented subscriber-side and/or
> > publisher-side and/or both, and the subscriber-side option may be
> > "enhance
ase just git apply
--reverse that one if it is causing a problem.
Sorry for any inconvenience. I will add the missing functionality to
0009 as soon as I can.
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Dec 3, 2020 at 6:21 PM Peter Smith wrote:
> Sorry for any inconvenience. I will add the missing functionality to
> 0009 as soon as I can.
>
PSA a **replacement** patch for the previous v29-0009.
This should correct the recently reported trouble [1]
[1] =
https://www.postg
gle tx with a temporary
slot as per current code
- Then the "apply" worker would be the *only* worker that actually
applies anything. (as its name suggests)
Thoughts?
---
Kind Regards,
Peter Smith.
Fujitsu Australia
/message-id/flat/CAMGcDxeqEpWj3fTXwqhSwBdXd2RS9jzwWscO-XbeCfso6ts3%2BQ%40mail.gmail.com
---
Kind Regards,
Peter Smith.
Fujitsu Australia
## Not patched
2020-12-07 17:03:45.237 AEDT [26325] LOG: !!>> apply worker: called
process_syncing_tables
2020-12-07 17:03:46.237 AEDT [26325] LOG: !!&g
On Tue, Dec 8, 2020 at 9:14 PM Amit Kapila wrote:
>
> On Tue, Dec 8, 2020 at 11:53 AM Peter Smith wrote:
> >
> > Yes, I observed this same behavior.
> >
> > IIUC the only way for the tablesync worker to go from CATCHUP mode to
> > SYNCDONE is via the call t
On Thu, Dec 10, 2020 at 8:49 PM Peter Smith wrote:
> So I will try to write a patch for the proposed Solution-1.
>
Hi Amit.
FYI, here is my v3 WIP patch for the Solution1.
This patch applies onto the v30 patch set [1] from the other 2PC thread:
[1]
https://www.postgresql.org/mess
DropSubscription. So this might be a problem to cleanup slots
and/or origin tracking belonging to those unknown workers.
* help / comments / cleanup
* There is temporary "!!>>" excessive logging of mine scattered around
which I added to help my testing during development
---
Kind Rega
xcessive logging of mine scattered around
which I added to help my testing during development
* Address review comments
---
Kind Regards,
Peter Smith.
Fujitsu Australia
v5-0001-2PC-change-tablesync-slot-to-use-same-two_phase-m.patch
Description: Binary data
v5-0002-WIP-patch-for-the-Solution1.p
On Sat, Dec 19, 2020 at 5:38 PM Amit Kapila wrote:
>
> On Fri, Dec 18, 2020 at 6:41 PM Peter Smith wrote:
> >
> > TODO / Known Issues:
> >
> > * the current implementation of tablesync drop slot (e.g. from
> > DropSubscription or finish_sync_worker) regenerat
sync slot.
> + */
> + {
> + extern void ReplicationSlotDropAtPubNode(
> + WalReceiverConn *wrconn_given, char *conninfo, char *subname, char
> *slotname);
>
> This is not how we export functions at other places?
Fixed in latest v5 patch -
https://www.postgresql.org/message-id/CAHut%2BPvmDJ_EO11_up%3D_cRbOjhdWCMG-n7kF-mdRhjtCHcjHRA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia.
d around
which I added to help my testing during development
* Address review comments
---
Kind Regards,
Peter Smith.
Fujitsu Australia
v6-0002-WIP-patch-for-the-Solution1.patch
Description: Binary data
v6-0001-2PC-change-tablesync-slot-to-use-same-two_phase-m.patch
Description: Binary data
On Mon, Dec 21, 2020 at 11:36 PM Amit Kapila wrote:
>
> On Mon, Dec 21, 2020 at 3:17 PM Peter Smith wrote:
> >
> > On Mon, Dec 21, 2020 at 4:23 PM Amit Kapila wrote:
> >
> > > Few other comments:
> > > ==
> >
> > Thanks for
temporary "!!>>" excessive logging scattered around which I
added to help my testing during development
* Address review comments
---
Kind Regards,
Peter Smith.
Fujitsu Australia
v7-0002-WIP-patch-for-the-Solution1.patch
Description: Binary data
v7-0001-2PC-change-tablesync-
v6 patch.
TODO / Known Issues:
* Help / comments
* There is temporary "!!>>" excessive logging scattered around which I
added to help my testing during development
* Address review comments
---
Kind Regards,
Peter Smith.
Fujitsu Australia
v8-0001-WIP-patch-for-the-Solution1.p
ring development
* Address review comments
---
Kind Regards,
Peter Smith.
Fujitsu Australia
v9-0001-WIP-patch-for-the-Solution1.patch
Description: Binary data
On Wed, Dec 23, 2020 at 9:07 PM Amit Kapila wrote:
>
> On Tue, Dec 22, 2020 at 4:58 PM Peter Smith wrote:
> >
> > On Mon, Dec 21, 2020 at 11:36 PM Amit Kapila
> > wrote:
> > >
> > > On Mon, Dec 21, 2020 at 3:17 PM Peter Smith wrote:
> > > &g
ly, consider if we error out after the second
> transaction, we won't where to start decoding from. I think all these
> should be done in a single transaction.
Fixed as suggested. Please see latest patch [v9]
---
[v9]
https://www.postgresql.org/message-id/CAHut%2BPv8ShLmrjCriVU%2Btprk_9b2kwBxYK2oGSn5Eb__kWVc7A%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
ments
---
Kind Regards,
Peter Smith.
Fujitsu Australia
v10-0002-Tablesync-extra-logging.patch
Description: Binary data
v10-0001-Tablesync-Solution1.patch
Description: Binary data
se to separate patch.
Fixed in latest patch (v10).
>
> 2. Remove WIP from the commit message and patch name.
>
> --
Fixed in latest patch (v10)
---
v10 =
https://www.postgresql.org/message-id/CAHut%2BPuzPmFzk3p4oL9H3nkiY6utFryV9c5dW6kRhCe_RY%3DgnA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
PSA a trivial patch to correct a comment typo.
Kind Regards,
Peter Smith
Fujitsu Australia.
fix_typo.patch
Description: Binary data
Hi.
I wanted to add a new commitfest entry but although I am logged in I
do not see any "Add" button either in the current or future CF.
https://commitfest.postgresql.org/31/
https://commitfest.postgresql.org/32/
How to add?
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Tue, Jan 5, 2021 at 11:36 AM Tom Lane wrote:
> Hm, there should be a "New patch" button just below the "Commitfest
> 2021-03" title.
Not for me. I see only 2 buttons - "Search/Filter" and "Shortcuts".
---
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Tue, Jan 5, 2021 at 12:46 PM Masahiko Sawada wrote:
>
> On Tue, Jan 5, 2021 at 9:45 AM Tom Lane wrote:
> >
> > Peter Smith writes:
> > > On Tue, Jan 5, 2021 at 11:36 AM Tom Lane wrote:
> > >> Hm, there should be a "New patch" button
tracking is cleaned up during
DropSubscription and/or process_syncing_tables_for_apply.
* the DropSubscription cleanup code was enhanced (v7+) to take care of
crashed sync workers.
* minor updates to PG docs
TODO / Known Issues:
* address review comments
---
Kind Regards,
Peter Smith.
Fujitsu
ginId node,
> LWLockAcquire(&replication_state->lock, LW_EXCLUSIVE);
>
> /* Make sure it's not used by somebody else */
> - if (replication_state->acquired_by != 0)
> + if (replication_state->acquired_by != 0 &&
> replication_state->acquired_by != MyProcPid)
> {
>
TODO
> I think you won't need this change if you do replorigin_advance before
> replorigin_session_setup in your patch.
>
> 8.
> - * that ensures we won't loose knowledge about that after a crash if the
> + * that ensures we won't lose knowledge about that after a crash if the
>
> It is better to submit this as a separate patch.
>
Done. Please see CF entry. https://commitfest.postgresql.org/32/2926/
[v11] =
https://www.postgresql.org/message-id/CAHut%2BPu0A6TUPgYC-L3BKYQfa_ScL31kOV_3RsB3ActdkL1iBQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia.
e =
NONE) workaround becomes ineffectual. Note - the current patch code is
only logging when the user has already disassociated the slot name; of
course normally (when the slot name was not disassociated) table slots
will give ERROR for broken connections.
IMO, if the user has disassociated the slot name then they have
already made their decision that they REALLY DO want to “proceed in
this situation”. So I thought we should let them proceed.
What do you think?
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Wed, Jan 6, 2021 at 4:04 PM Amit Kapila wrote:
>
> On Tue, Jan 5, 2021 at 3:32 PM Peter Smith wrote:
> >
> > On Mon, Jan 4, 2021 at 10:48 PM Amit Kapila wrote:
> > >
> >
> > > 5. Is it possible to write a testcase where we fail (say due to pk
>
ifferent from the macro define.
>
Yes, same was already previously reported [1]
[1]
https://www.postgresql.org/message-id/CAA4eK1Kyi037XZzyrLE71MS2KoMmNSqa6RrQLdSCeeL27gnL%2BA%40mail.gmail.com
It will be fixed in the next patch (v12)
Kind Regards,
Peter Smith.
Fujitsu Australia.
DropSubscription cleanup code was enhanced (v7+) to take care of
any crashed tablesync workers.
* Updates to PG docs
TODO / Known Issues:
* Address review comments
* Patch applies with whitespace warning
---
Kind Regards,
Peter Smith.
Fujitsu Australia
v12-0001-Tablesync-Solution1.patch
Description
On Mon, Jan 4, 2021 at 8:06 PM Amit Kapila wrote:
>
> On Wed, Dec 30, 2020 at 11:51 AM Peter Smith wrote:
> >
> > On Wed, Dec 23, 2020 at 8:43 PM Amit Kapila wrote:
> > >
> > > 1.
> > > + * Rarely, the DropSubscription may be issued when a tablesync
gml
> @@ -7651,6 +7651,7 @@ SCRAM-SHA-256$<iteration
> count>:&l
> State code:
> i = initialize,
> d = data is being copied,
> + C = table data has been copied,
> s = synchronized,
>
Fixed in latest patch [v12]
[v12] =
https://www.postgresql.org/message-id/CAHut%2BPsonJzarxSBWkOM%3DMjoEpaq53ShBJoTT9LHJskwP3OvZA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
/devel/logical-replication-subscription.html
>
PG docs updated in latest patch [v12]
[v12] =
https://www.postgresql.org/message-id/CAHut%2BPsonJzarxSBWkOM%3DMjoEpaq53ShBJoTT9LHJskwP3OvZA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Jan 7, 2021 at 3:20 PM Amit Kapila wrote:
>
> On Wed, Jan 6, 2021 at 3:39 PM Amit Kapila wrote:
> >
> > On Wed, Jan 6, 2021 at 2:13 PM Peter Smith wrote:
> > >
> > > I think it makes sense. If there can be a race between the tablesync
>
. See FIXME comments.
---
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Jan 7, 2021 at 6:52 PM Peter Smith wrote:
>
> Hi Amit.
>
> PSA the v12 patch for the Tablesync Solution1.
>
> Differences from v11:
> + Added PG docs to mention the tablesync slot
> + R
id, albeit maybe unnecessary since in
the current code the tablesync slot
name length is fixed. But I left the older comment here as a safety reminder
in case some future change would want to modify the slot name. What do
you think?
[v13] =
https://www.postgresql.org/message-id/CAHut%2BPvby4zg6kM1RoGd_j-xs9OtPqZPPVhbiC53gCCRWdNSrw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia.
for the "two_phase"
option.
So:
a) should I add a new test per your feedback comment, or
b) should I be consistent with the other similar errors, and not add the test?
Of course it is easy to add a new test if you think option (a) is best.
Thoughts?
-
Kind Regards,
Peter Smith.
Fujitsu Australia
://cfbot.cputube.org/
[2] https://api.cirrus-ci.com/v1/task/5352561114873856/logs/test.log
Kind Regards,
Peter Smith.
Fujitsu Australia
st#L733
Thanks for all the effort spent into looking at this.
Meanwhile, since you pointed out the patch is not applying on the HEAD
tip I can at least address that.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ks!
--
[1] http://cfbot.cputube.org/
[2] https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/32/2914
Kind Regards,
Peter Smith.
Fujitsu Australia
ng_table_states signal handler) only to be
immediately/wrongly overwritten as table_states_valid = true in this
FetchTableStates code.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
-yT5%3D9GDEW84TF%2BA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Sun, Mar 7, 2021 at 3:00 PM Amit Kapila wrote:
>
> On Sun, Mar 7, 2021 at 7:35 AM Peter Smith wrote:
> >
> > Please find attached the latest patch set v51*
> >
>
> Few more comments on v51-0006-Fix-apply-worker-empty-prepare:
> =
On Mon, Mar 8, 2021 at 4:19 PM Amit Kapila wrote:
>
> On Mon, Mar 8, 2021 at 10:04 AM Peter Smith wrote:
> >
> > On Sun, Mar 7, 2021 at 3:00 PM Amit Kapila wrote:
> > >
> > > On Sun, Mar 7, 2021 at 7:35 AM Peter Smith wrote:
> > > >
>
start using numeric instead for the new feature
> getting added?
This may or may not become a problem sometime in the future, but I
think the feedback is not really specific to the current patch set so
I am skipping it at this time.
If you want, maybe create it as a separate thread, Is it OK?
Kind Regards,
Peter Smith.
Fujitsu Australia
On Sun, Mar 7, 2021 at 1:33 PM Amit Kapila wrote:
>
> On Sun, Mar 7, 2021 at 7:26 AM Peter Smith wrote:
> >
> > Hi hackers.
> >
> > I propose a small optimization can be added to the tablesync replication
> > code.
> >
> > This proposa
e
tablesync to achieve SYNCDONE state.
See wait_for_relation_state_change(rstate->relid, SUBREL_STATE_SYNCDONE);
Now, notice the wait_for_relation_state_change already has
CHECK_FOR_INTERRUPTS();
So, AFAIK it isn't necessary to put another CHECK_FOR_INTERRUPTS at
the outer loop.
Thoughts?
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
ther streaming spool file code does - e.g. notice
apply_handle_stream_commit function simply closes its own fd using
BufFileClose; it doesn’t delegate stream_close_file() to do it.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
text applies to the code above or below it)
TIA.
Kind Regards,
Peter Smith.
Fujitsu Australia
ssert(sizeof(hentry->name) == szpath) ' will be better.
>
Thanks for your feedback comment.
But today Amit suggested [ak0315] that the current psf logic should
all be replaced, after which the function you commented about will no
longer exist.
[ak0315]
https://www.postgresql.org/message-id/CAA4eK1LVEdPYnjdajYzu3k6KEii1%2BF0jdQ6sWnYugiHcSGZD6Q%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
ommit/464824323e57dc4b397e8b05854d779908b55304
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Mar 16, 2021 at 7:20 PM Amit Kapila wrote:
>
> On Tue, Mar 16, 2021 at 3:35 AM Peter Smith wrote:
> >
> > I noticed that the PG docs [1] for the catalog pg_subscription doesn't
> > have any mention of the substream column.
> >
> > Accidental omis
On Wed, Mar 17, 2021 at 12:45 AM Amit Kapila wrote:
>
>
> Attached, please find the patch to update the description of substream
> in pg_subscription.
>
I applied your patch and regenerated the PG docs to check the result.
LGTM.
Kind Regards,
Peter Smith.
Fujitsu Australia
rectories?
e.g. Are you only supposed to run "make coverage-html" from the top folder?
Or is it supposed to work but I did something wrong?
--
[1] https://www.postgresql.org/docs/13/regress-coverage.html
Kind Regards.
Peter Smith
Fujitsu Australia.
>
> You can run the "make coverage-html" command in a subdirectory
> if you want a coverage report for only a portion of the code tree.
Thank you for the clarifications and the updated documentation.
Kind Regards,
Peter Smith
Fujitsu Australia
amed. Which is why this
> function will not be called when a streaming transaction is prepared
> as part of a two-phase commit.
AFAIK the last remaining issue now is only about the complexity of the
aforementioned code/comment. If you want to defer changing that until
we can come up with something better, then that is OK by me.
Apart from that I have no other pending review comments at this time.
Kind Regards,
Peter Smith.
Fujitsu Australia
t, txn, prepare_lsn);
else
logicalrep_write_prepare(ctx->out, txn, prepare_lsn);
OutputPluginWrite(ctx, true);
}
===
Kind Regards,
Peter Smith.
Fujitsu Australia
://www.postgresql.org/message-id/CAHut%2BPt6zB-YffCrMo7%2BZOKn7C2yXkNYnuQTdbStEJJJXZZXaw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
v12-patch-test-coverage-20201029.xlsx
Description: MS-Excel 2007 spreadsheet
e switch can make the missing
enum detection easier because then you can use -Wswitch option to
expose the problem (instead of -Wswitch-enum which may give lots of
false positives as well)
===
[1] https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wswitch
Kind Regards,
Peter Smith.
Fujitsu Australia
pgoutput_startup where we are
> disabling the streaming for init phase. But it is always good to once
> test this and ensure the same.
I have tested this scenario and confirmed that even when the
subscriber is capable of streaming it does NOT do any streaming during
its tablesync phase.
Kind Regards
Peter Smith.
Fujitsu Australia
eeds to say
"create OR REPLACE constraint trigger my_trig" to be testing what it
claims to be testing.
I also think there is a missing check in the code - see COMMENT (2) -
for handling this scenario. But since this test case is broken you do
not then notice the code check is missing.
===
Kind Regards,
Peter Smith.
Fujitsu Australia
it(struct LogicalDecodingContext *ctx,
> ReorderBufferTXN *txn,
> XLogRecPtr commit_lsn);
> -
> +static void pgoutput_stream_prepare_txn(LogicalDecodingContext *ctx,
> + ReorderBufferTXN *txn, XLogRecPtr prepare_lsn);
>
> Spurious line removal.
Fixed.
---
Kind Regards,
P
replace constraint trigger my_trig
after insert on my_table
for each row execute procedure funcB(); -- Test 1a. Replace regular
trigger with constraint trigger. Expect ERROR (bad syntax)
drop trigger my_trig on my_table;
create constraint trigger my_trig -- 2. create a constraint trigger. OK
after insert on my_table
for each row execute procedure funcA();
create or replace trigger my_trig
after insert on my_table
for each row execute procedure funcB(); -- Test 2a. Replace
constraint trigger with regular trigger. Expect ERROR (cannot replace
a constraint trigger)
create or replace constraint trigger my_trig
after insert on my_table
for each row execute procedure funcB(); -- Test 2b. Replace
constraint trigger with constraint trigger. Expect ERROR (bad syntax)
drop table my_table;
drop function funcA();
drop function funcB();
--
Kind Regards,
Peter Smith.
Fujitsu Australia
1 - 100 of 1889 matches
Mail list logo
