=
src/test/subscription/t/021_twophase.pl
nitpick - didn't quite understand the "Since we are..." comment. I
reworded it according to what I thought was the intention.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/commands/subscriptioncmds.c
b/s
org/message-id/CAHut%2BPsqMRS3dcijo5jsTSbgV1-9So-YBC7PH7xg0%2BZ8hA7fDQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index cba6661..24c57fb 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.s
Hi, here is my review of the v18-0003 patch.
==
sgml/ref/alter_subscription.sgml
nitpick - some minor tweaks to the documentation text. I also added a link
back to the two_phase parameter. Please see the attached diffs file.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a
###" comment style as
in patch 0001 to indicate each main TEST scenario.
==
99.
Please refer to the diffs attachment patch, which implements any nitpicks
mentioned above.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/commands/subscriptioncmds.c
o indicate each test part better.
==
99.
Please also see the attached diffs patch which implements any nitpicks
mentioned above.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 3ac4a4b..cba6661 100644
--- a/doc/src/sgm
ces is more consistent
Anyway, it is just my opinion. Maybe there are some pitfalls I'm unaware of.
Thoughts?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ode comments are adjusted appropriately so they are not misleading by
calling these "column lists" still.
BEFORE: BMS 'columns' means "columns of the column list" or NULL if
there was no publication column list
AFTER: BMS 'columns' means "columns to be replicated" or NULL if all
columns are to be replicated
==
Kind Regards,
Peter Smith.
her places in this patch.
==
99.
Please see the attached diffs patch that implements any nitpicks
mentioned above.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/replication/logical/tablesync.c
b/src/backend/replication/logical/tablesync.c
index c2a7d18..5288769
' and 'tab3' to make them more consistent.
==
99.
Please refer to the attached diff patch which implements any nitpicks
described above.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/test/subscription/t/011_generated.pl
b/src/test/subscription/t/011_generate
y isn't it 100?
My guess is that it seems to be related to code in "nextval_internal"
(fetch = log = fetch + SEQ_LOG_VALS;) but it kind of defies
expectations of the test, so if it really is correct then it needs
commentary.
Actually, I found other regression test code that deals with
quot;?
nitpick - changed the varlistentry "id".
==
99.
Please also see the attached diffs patch which implements any nitpicks
mentioned above.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 981ca51..645fc4
On Fri, Jul 5, 2024 at 9:58 PM vignesh C wrote:
>
> On Thu, 4 Jul 2024 at 12:44, Peter Smith wrote:
> >
> > 1.
> > Should there be some new test for the view? Otherwise, AFAICT this
> > patch has no tests that will exercise the new function
&g
can just throw away patch
0004 and everything else (patches 0001,0002,0003) will still be good
to go.
Thoughts?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
er rorqual [1]. but this is probably unrelated to the push because
we found the same failure also occurred in April [2].
==
[1]
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rorqual&dt=2024-07-09%2003%3A46%3A44
[2]
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=rorqual&dt=2024-04-18%2006%3A52%3A07&stg=recovery-check
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi Shlok, here are my review comments for v16-0003.
==
src/backend/replication/logical/proto.c
On Mon, Jul 8, 2024 at 10:04 PM Shlok Kyal wrote:
>
> On Mon, 8 Jul 2024 at 13:20, Peter Smith wrote:
> >
> >
> > 2. logicalrep_write_tuple and logicalrep_write_attrs
&
arameter is true it
should give ERROR.
~~~
5.
-# publisher-side tab3 has generated col 'b' but subscriber-side tab3
has DIFFERENT COMPUTATION generated col 'b'.
+# tab3:
+# publisher-side tab3 has generated col 'b' but
+# subscriber-side tab3 has DIFFERENT COMPUTAT
1_generated.pl
5.
AFAICT there are still multiple comments (e.g. for the "TEST tab"
comments) where it still says "generated" instead of "stored
generated". I did not make a "nitpicks" diff for these because those
comments are inherited from the prior patch 000
ated_columns and copy_data = true
are mutually exclusive" is not necessary because this all falls under
the existing comment "fail - invalid option combinations"
nitpick - let's explicitly put "copy_data = true" in the CREATE
SUBSCRIPTION to make it more obvious
ines for readability
nitpick - typo /subsriber/subscriber/
nitpick - prior to the ALTER test, tab6 is unsubscribed. So add
another test to verify its initial data
nitpick - sometimes the msg 'add a new table to existing publication'
is misplaced
nitpick - the tests for tab6 and tab5 were in o
sequences = (List *) funcctx->user_fctx;
1331
1332 if (funcctx->call_cntr < list_length(sequences))
1333 {
1334 Oid relid = list_nth_oid(sequences, funcctx->call_cntr);
1335
1336 SRF_RETURN_NEXT(funcctx, ObjectIdGetDatum(relid));
1337 }
==
Perhaps now it is time to create a CF entr
The latest (v20240704) patch 0001 LGTM
==
Kind Regards,
Peter Smith.
Fujitsu Australia
eally have a 'regress_' prefix?
==
99.
Please refer to the attached nitpicks diff which has implementation
for the nitpicks cited above.
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPvrk75vSDkaXJVmhhZuuqQSY98btWJV%3DBMZAnyTtKRB4g%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu
.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index d30864e..fb3f973 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19521,7 +19521,7 @@ SELECT setval('myseq', 42, false);
Next nextval
oned above.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 3dd4805..83f87c1 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19521,11 +19521,11 @@ SELECT setval('myseq', 42, false);
Next nextval
I'm not saying this design idea is guaranteed to work, but it might be
worth considering, because if it does work then there is potential to
make the current 0001 patch significantly shorter.
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPsuJfcaeg6zst%3D6PE5uyJv_UxVRHU3ck7W2
comment we can fix
this in passing.
==
99.
Please also see the attached top-up patch for all those nitpicks
identified above.
==
[1] v11-0001 review
https://www.postgresql.org/message-id/CAHut%2BPv45gB4cV%2BSSs6730Kb8urQyqjdZ9PBVgmpwqCycr1Ybg%40mail.gmail.com
Kind Regards,
Peter Smith.
Fu
/publication.sql
17.
+CREATE SEQUENCE testpub_seq0;
+CREATE SEQUENCE pub_test.testpub_seq1;
+
+SET client_min_messages = 'ERROR';
+CREATE PUBLICATION testpub_forallsequences FOR ALL SEQUENCES;
+RESET client_min_messages;
+
+SELECT pubname, puballtables, puballsequences FROM pg_publication
available before retrieving the current timeline. .."
OK, I have changed the code as suggested. Please see the attached v2 patch.
make check-world was successful.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v2-0001-Relocate-the-flushptr-checking-code.patch
Description: Binary data
On Fri, Jun 28, 2024 at 4:18 PM Amit Kapila wrote:
>
> On Fri, Jun 28, 2024 at 5:15 AM Peter Smith wrote:
> >
> > On Thu, Jun 27, 2024 at 3:44 PM Michael Paquier wrote:
> > >
> > > On Wed, Jun 26, 2024 at 02:30:26PM +0530, vignesh C wrote:
> > > >
On Thu, Jun 27, 2024 at 3:44 PM Michael Paquier wrote:
>
> On Wed, Jun 26, 2024 at 02:30:26PM +0530, vignesh C wrote:
> > On Mon, 3 Jun 2024 at 11:21, Peter Smith wrote:
> >> Perhaps the comment should say something like it used to:
> >> /* Fail if there is not
this comment is wrong for the tab2 test because here
col 'b' IS replicated. I have added much more substantial test case
comments in the attached nitpicks diff. PSA.
==
src/test/subscription/t/031_column_list.pl
13.
NITPICK - IMO there is a missing word "list" in the comment. Th
lue' be a better name for what this function is doing?
==
99.
See also my attached diff which is a top-up patch implementing those
nitpicks mentioned above. Please apply any of these that you agree
with.
==
[1] https://www.postgresql.org/docs/devel/functions-sequence.html
Kind Regards,
FYI - I created a CF entry [1] for this because AFAIK the patch is
just waiting for a committer to check if it is OK to be pushed, but
maybe nobody has noticed it.
==
[1] https://commitfest.postgresql.org/48/5075/
Kind Regards,
Peter Smith.
Fujitsu Australia
003 would have to update it again to say something about "STORED".
But all that is missing from the v10* patches.
==
99.
See also my nitpicks diff which is a top-up patch addressing all the
nitpick comments mentioned above. Please apply all of these that you
agree with.
==
[1]
ht
h* for
copy_data=false and also for copy_data=true. IOW, is it really
necessary to have so many tables/tests? For example, I am thinking
some of those tests from patch 0001 can be re-used or just removed now
that copy_data=true works.
~~~
NITPICK - minor comment tweak
~~~
11.
For tab4 and tab6 I saw
01 review -
https://www.postgresql.org/message-id/CAHut%2BPujrRQ63ju8P41tBkdjkQb4X9uEdLK_Wkauxum1MVUdfA%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CAHut%2BPvsRWq9t2tEErt5ZWZCVpNFVZjfZ_owqfdjOhh4yXb_3Q%40mail.gmail.com
[3]
https://www.postgresql.org/message-id/CAHut%2BPsHsT3V1wQ5uoH9ynbmWn4ZQqOe34X%2Bg37LSi7sgE_i
f your new feature, I think that the
"except generated columns" wording is not strictly correct anymore.
IOW that docs page needs updating but AFAICT your patches have not
addressed this yet.
==
[1] https://www.postgresql.org/docs/17/protocol-logicalrep-message-formats.html
Kind Regards,
ringInfo(&cmd, " AND a.attgenerated = ''");
}
}
==
99.
Please refer also to my attached nitpick diffs and apply those if you agree.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/ref/create_subscription.sgml
b/doc/src/sgml/ref/cre
ction note will now need to be modified by your patches.
==
[1]
https://github.com/postgres/postgres/commit/7a089f6e6a14ca3a5dc8822c393c6620279968b9
[2]
Kind Regards,
Peter Smith.
Fujitsu Australia
do explicit "SELECT a,b,c" instead of "SELECT
*", for readability and so there are no surprises.
==
99.
Please also refer to my attached nitpicks diff for numerous cosmetic
changes, and apply if you agree.
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPtAsEc3PEB1
plicit "SELECT a,b" instead of "SELECT *",
for readability and to avoid any surprises.
==
Kind Regards,
Peter Smith.
Fujitsu Australia.
ause the result value of 'b'
is the subscriber-side computed value (which you made deliberately
different to the publisher-side computed value).
==
99.
Please also refer to the attached nitpicks top-up patch for minor
cosmetic stuff.
======
[1]
https://www.postgresql.org/message-i
needed for the versions before that.
~
The attached "HEAD" patch is appropriate for HEAD, PG17, PG16, PG15
The attached "PG14" patch is appropriate for PG14, PG13, PG12
==
[1]
https://www.postgresql.org/message-id/2b291af9-929f-49ab-b378-5cbc029d348f%40eisentraut.org
K
FYI - I applied this latest patch and re-ran the original failing test
script 10 times (e.g. 10 x 100 test iterations; it took 4+ hours).
There were zero failures observed in my environment.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Jun 18, 2024 at 9:40 PM Amit Kapila wrote:
>
> On Tue, Jun 18, 2024 at 12:11 PM Peter Smith wrote:
> >
> > While reviewing another thread that proposes to include "generated
> > columns" support for logical replication [1] I was looking for any
> &
ot;Generated
Columns" section [3]. Perhaps it is enough.
Thoughts?
==
[1]
https://www.postgresql.org/message-id/flat/B80D17B2-2C8E-4C7D-87F2-E5B4BE3C069E%40gmail.com
[2]
https://www.postgresql.org/docs/devel/protocol-logicalrep-message-formats.html
[3] https://www.postgresql.org/docs/devel/dd
27;b'
~~~
9.
+$node_subscriber->safe_psql('postgres',
+ "CREATE SUBSCRIPTION sub4 CONNECTION '$publisher_connstr'
PUBLICATION pub4 WITH (include_generated_columns = true)"
+ );
+
All the publications are created together, and all the subscriptions
are
uot;INSERT INTO tab3 VALUES (4), (5)");
+
+$node_publisher->wait_for_catchup('sub3');
+
+$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab3");
+is( $result, qq(4|24
+5|25), 'generated columns replicated to non-generated column on sub
--> RBTXN_IS_COMMITTED
- RBTXN_ABORTED --> RBTXN_IS_ABORTED
~~~
8.
Similarly, IMO the macros should have the same names as the bitmasks,
like the other nearby ones generally seem to.
rbtxn_did_commit --> rbtxn_is_committed
rbtxn_did_abort --> rbtxn_is_aborted
==
9.
Also, attach
ve-suggested change.
Thoughts?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-walsender-fileheader-comment.patch
Description: Binary data
On Tue, May 28, 2024 at 4:16 PM Peter Smith wrote:
>
...
>
> The new GUC quoting patches are separated by different GUC types only
> to simplify my processing of them.
>
> v7-0001 = Add quotes for GUCs - bool
> v7-0002 = Add quotes for GUCs - int
> v7-0003 = Add quotes f
"?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
/in the case/
/But please note that.../Note that.../
==
Kind Regards,
Peter Smith.
Fujitsu Australia
all seeing any comment or logic for this kind of copy
optimisation in the patch 0003. Is this already accounted for
somewhere and I missed it, or is my understanding wrong?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ot;);
+is( $result, qq(1|2
+2|4
+3|6), 'generated columns initial sync with include_generated_column = true');
Should this say "ORDER BY..." so it will not fail if the row order
happens to be something unanticipated?
==
99.
Also, see the attached file with numerous other ni
there is no value for "a"? I don't know if the
expected *.out here is OK or not, so some test comments may help to
clarify it.
==
[1] https://www.postgresql.org/docs/devel/sql-copy.html
Kind Regards,
Peter Smith.
Fujitsu Australia
exclusive options/,
+ 'cannot use both include_generated_column and copy_data as true');
Isn't this mutual exclusiveness of options something that could have
been tested in the regress test suite instead of TAP tests? e.g. AFAIK
you won't require a connection to test this case.
~~~
17. Missing test?
IIUC there is a missing test scenario. You can add another subscriber
table TAB3 which *already* has generated cols (e.g. generating
different values to the publisher) so then you can verify they are NOT
overwritten, even when the 'include_generated_cols' is true.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Perhaps the comment should say something like it used to:
/* Fail if there is not enough WAL available. This can happen during
shutdown. */
==
[1]
https://github.com/postgres/postgres/commit/fca85f8ef157d4d58dea1fdc8e1f1f957b74ee78
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi, just by visual inspection of the v4/v5 patch diffs, the latest v5 LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, May 29, 2024 at 8:04 PM vignesh C wrote:
>
> On Wed, 22 May 2024 at 14:26, Peter Smith wrote:
> >
> > On Tue, May 21, 2024 at 8:40 PM PG Doc comments form
> > wrote:
> > >
> > > The following documentation comment has been logged on t
he same)
>
Hi Alexander,
FYI, by injecting a lot of logging, I’ve confirmed your findings that
for the failing scenario, the ‘got_SIGUSR2’ flag never gets set to
true, meaning the WalSndLoop() cannot finish. Furthermore, I agree
with your step 8 finding that when it fails the ReadPageInternal
function call (the one in XLogDecodeNextRecord with the comment "Wait
for the next page to become available") constantly returns -1.
I will continue digging next week...
==
Kind Regards,
Peter Smith.
Fujitsu Australia
stigating
> its origins, I discovered that this problem persists across all
> branches up to PG10 (the script needs slight adjustments to run it on
> older versions). It's worth noting that this issue isn't a result of
> recent version changes.
>
Hi,
FWIW using the provided scripting I was also able to reproduce the
problem on HEAD but for me, it was more rare. -- the script passed ok
3 times all 100 iterations; it eventually failed on the 4th run on the
75th iteration.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, May 29, 2024 at 7:53 PM vignesh C wrote:
>
> On Wed, 29 May 2024 at 10:41, Peter Smith wrote:
> >
> > On Thu, Apr 25, 2024 at 6:59 PM vignesh C wrote:
> > >
> >
> > > c) Don't reset the latch at worker attach and allow launcher main to
&g
1678b0d5fa24d2898424243cd6#diff-127f8eb009151ec548d14c877a57a89d67da59e35ea09189411aed529c6341bf
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, May 17, 2024 at 9:57 PM Peter Eisentraut wrote:
>
> On 17.05.24 05:31, Peter Smith wrote:
> >> I think we should accept your two patches
> >>
> >> v6-0001-GUC-names-docs.patch
> >> v6-0002-GUC-names-add-quotes.patch
> >>
> >> whi
not extracted
to another variable (e.g. data->binary).
==
[1] My v1 review -
https://www.postgresql.org/message-id/CAHut+PsuJfcaeg6zst=6pe5uyjv_uxvrhu3ck7w2ahb1uqy...@mail.gmail.com
[2] My v2 review -
https://www.postgresql.org/message-id/CAHut%2BPv4RpOsUgkEaXDX%3DW2rhHAsJLiMWdUrUGZOcoRHuWj5%2BQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
e without
losing any data
+ that has been flushed to the new primary server.
+
Let's word this more like the same sentence top of the page. See
review comment #3c
SUGGESTION
If the result (failover_ready) of both steps above
is true, then existing subscriptions can continue subscribing to
publications now on the new primary server without any loss of data.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
12.
GENERAL - Missing CREATE SUBSCRIPTION test?
GENERAL - Missing ALTER SUBSCRIPTION test?
How come this patch adds a new CREATE SUBSCRIPTION option but does not
seem to include any test case for that option in either the CREATE
SUBSCRIPTION or ALTER SUBSCRIPTION regression tests?
==
[1] My v1 review -
https://www.postgresql.org/message-id/CAHut+PsuJfcaeg6zst=6pe5uyjv_uxvrhu3ck7w2ahb1uqy...@mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
> regards.
>
Hi,
I think it might be better to keep all the discussions about GUC
quoting and related topics like this confined to the main thread here
[1]. Otherwise, we might end up with a bunch of competing patches.
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPv-kSN8SkxSdoHano_wPubqcg5789ejhCDZAcLFceBR-w%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, May 17, 2024 at 9:57 PM Peter Eisentraut wrote:
>
> On 17.05.24 05:31, Peter Smith wrote:
> >> I think we should accept your two patches
> >>
> >> v6-0001-GUC-names-docs.patch
> >> v6-0002-GUC-names-add-quotes.patch
> >>
> >> whi
Hi Kuroda-san,
I did not apply these v12* patches, but I have diff'ed all of them
with the previous v11* patches and confirmed that all of my previous
review comments now seem to be addressed.
I don't have any more comments to make at this time. Thanks!
==
Kind Regards,
Peter Smit
On Thu, May 16, 2024 at 9:35 PM Peter Eisentraut wrote:
>
> On 04.01.24 07:53, Peter Smith wrote:
> >> Now that I read this again, I think this is wrong.
> >>
> >> We should decide the quoting for a category, not the actual content.
> >> Like, q
force_alter parameter for details about when this might
be useful.
==
[1]
https://github.com/postgres/postgres/commit/fa65a022db26bc446fb67ce1d7ac543fa4bb72e4
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, May 16, 2024 at 10:42 PM David Rowley wrote:
>
> On Thu, 16 May 2024 at 19:05, Peter Smith wrote:
> >
> > On Thu, May 16, 2024 at 3:11 PM David Rowley wrote:
> > > If you want to do this, what's the reason to limit it to just this one
> > > page
On Thu, May 16, 2024 at 3:11 PM David Rowley wrote:
>
> On Thu, 16 May 2024 at 12:29, Peter Smith wrote:
> > There are lots of subscription options listed on the CREATE
> > SUBSCRIPTION page [1].
> >
> > Although these boolean options are capable of accepting differ
alse;
data->binary = false;
data->streaming = LOGICALREP_STREAM_OFF;
data->messages = false;
data->two_phase = false;
+ data->publish_generated_column = false;
I think the 1st var should be 'include_generated_columns_option_given'
for consistency with the name of the actual option that was given.
==
src/include/replication/logicalproto.h
7.
(Same as a previous review comment)
For all the API changes the new parameter name should be plural.
/publish_generated_column/publish_generated_columns/
==
src/include/replication/pgoutput.h
8.
bool publish_no_origin;
+ bool publish_generated_column;
} PGOutputData;
/publish_generated_column/publish_generated_columns/
==
Kind Regards,
Peter Smith.
Fujitsu Australia
bscription.html
[3]
https://www.postgresql.org/message-id/flat/8fab8-65d74c80-1-2f28e880%4039088166
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Always-use-true-false-instead-of-on-off-for-boole.patch
Description: Binary data
r aborting the prepared transaction.
>
> Fixed.
>
You wrote "Fixed" for that patch v9-0004 suggestion but I don't think
anything was changed at all. Accidentally missed?
==
Kind Regards,
Peter Smith.
Futjisu Australia
transactions
+ are aborted and the alter will proceed.
+ The default is false.
+
+
+
IMO this will be better broken into multiple paragraphs.
1. Specifies...
2. There is...
3. The default is...
==
src/test/subscription/t/099_twophase_added.pl
(Let's change all the on|off to true|false like you already did in patch 0002.
4.3.
+# Try altering the two_phase option to "off." The command will fail since there
+# is a prepared transaction and the 'force_alter' option is not specified as
+# true.
+my $stdout;
+my $stderr;
/off./false/
==
Kind Regards,
Peter Smith.
Fujitsu Australia
---++-+--
The column heading should be "Force alter", as already mentioned by an
earlier review comment (#4.7)
==
src/test/subscription/t/099_twophase_added.pl
4.11.
+# Alter the two_phase with the force_alter option. Apart from the the last
+# ALTER SUBSCRIPTION command, the command will abort the prepared transaction
+# and succeed.
There is typo "the the" and the wording is a bit strange. Why not just say:
SUGGESTION
Alter the two_phase true to false with the force_alter option enabled.
This command will succeed after aborting the prepared transaction.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
nly to true|false
everywhere for these boolean parameters, instead of sometimes using
different values like on|off.
What do you think?
==
[1] https://www.postgresql.org/docs/devel/sql-createsubscription.html
Kind Regards,
Peter Smith.
Fujitsu Australia
ed transactions exist on the publisher node.
+#
+# Since the two_phase is "off", then normally, this PREPARE will do nothing
+# until the COMMIT PREPARED, but in this test, we toggle the two_phase to "on"
+# again before the COMMIT PREPARED happens.
This is a major test part so IMO this comment should have
# like it had before, to distinguish it from all
the sub-step comments.
==
My v7-0002 review -
https://www.postgresql.org/message-id/CAHut%2BPtu_w_UCGR-5DbenA%2By7wRiA8QPi_ZP%3DCCJ3SGdTn-%3D%3Dg%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia.
e above" mean? Be more explicit.
~~~
9.
+# Verify the prepared transaction are aborted
$result = $node_subscriber->safe_psql('postgres',
"SELECT count(*) FROM pg_prepared_xacts;");
is($result, q(0), "prepared transaction done by worker is aborted");
/transaction are aborted/transaction was aborted/
==
Response to my v7-0004 review --
https://www.postgresql.org/message-id/OSBPR01MB2552F738ACF1DA6838025C4FF5E62%40OSBPR01MB2552.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
d transaction done by worker is aborted");
+
/the prepared transaction are aborted/any prepared transactions are aborted/
==
Kind Regards,
Peter Smith
Fujitsu Australia
ared_xacts;");
is($result, q(0), "prepared transaction done by worker is aborted");
+$node_subscriber->safe_psql('postgres', "ALTER SUBSCRIPTION sub ENABLE;");
+
I felt the ENABLE statement should be above the SELECT statement so
that the code is more like it was before applying the patch.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ows got replicated ok
~~~
8. TAP test - subscription name
It's better to rename the SUBSCRIPTION in this TAP test so you can
avoid getting log warnings like:
psql::4: WARNING: subscriptions created by regression test
cases should have names starting with "regress_"
psql::4: NOTICE: created replication slot "sub" on publisher
==
Kind Regards,
Peter Smith.
Fujitsu Australia
repared transactions exist on the publisher node.
#
# Since two_phase is "off", then normally this PREPARE will do nothing until
# the COMMIT PREPARED, but in this test, we toggle the two_phase to "on" again
# before the COMMIT PREPARED happens.
~~~
7.
Maybe this test case needs
TE),
+ errmsg("cannot disable two_phase when uncommitted prepared
transactions present"),
+ errhint("Resolve these transactions and try again")));
The comment "/* Add error message */" seems unnecessary.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
icated.
/Made sure/Make sure/
/commited/committed/
~~~
21.
+# Make sure that the two-phase is enabled on the subscriber
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT subtwophasestate FROM pg_subscription WHERE subname = 'tap_sub_copy';"
+);
+is($result, qq(e), 'two-phase is disabled');
The 'two-phase is disabled' is the identical message used in the
opposite case earlier, so something is amiss. Maybe this one should
say 'two-phase should be enabled' and the earlier counterpart should
say 'two-phase should be disabled'.
==
Kind Regards,
Peter Smith
Fujitsu Australia
Hi, just by visual inspection of the v3/v4 patch diffs, the latest v4 LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Sun, Mar 31, 2024 at 8:12 PM Andrey M. Borodin wrote:
>
>
>
> > On 15 Aug 2023, at 07:38, Peter Smith wrote:
> >
> > A rebase was needed due to a recent push [1].
> >
> > PSA v3.
>
>
> > On 14 Jan 2024, at 10:43, vignesh C wrote:
> >
/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
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
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
> > &
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 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 -
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
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
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
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.
-
101 - 200 of 1192 matches
Mail list logo
