they are specified
in a column list.
==
[1] review v31 18/9 -
https://www.postgresql.org/message-id/flat/CAHv8Rj%2BKOoh58Uf5k2MN-%3DA3VdV60kCVKCh5ftqYxgkdxFSkqg%40mail.gmail.com#f2f3b48080f96ea45e1410f5b1cd9735
[2] review v32 24/9 -
https://www.postgresql.org/message-id/CAHut%2BPu7EcK_JTgWS7GzeStHk6Asb1dmE
comment changes.
==
Please refer to the attachment which implements any nits from above.
==
Kind Regards,
Peter Smith.
Fujitsu Austrlia.
diff --git a/src/test/regress/expected/publication.out
b/src/test/regress/expected/publication.out
index 8bcb79a..e419ca8 100644
--- a/src/test/regress/e
On Thu, Oct 3, 2024 at 10:01 AM David G. Johnston
wrote:
>
> On Wednesday, October 2, 2024, Peter Smith wrote:
>>
>>
>>
>> You can see how confused the current docs are because "failover" is
>> called by both terms even within the same paragraph!
On Thu, Oct 3, 2024 at 10:09 AM Peter Smith wrote:
>
> Hi Shubham,
>
> The different meanings of the terms "parameter" versus "option" were
> discussed in a recent thread [1], and that has made me reconsider this
> generated columns feature.
>
> De
OxOnVJf-_G8OBCLdyqu8jJ8si51d%2BEQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
impacting the understanding badly.
Anyway, since you are already fixing something for "failover", then it
would be good to fix the correct term everywhere for that one (e.g.
call it an "option"), or at least call it an option everywhere on the
CREATE SUBSCRIPTION page.
==
[1]
https://www.postgresql.org/docs/devel/sql-createsubscription.html#SQL-CREATESUBSCRIPTION-PARAMS-WITH-SLOT-NAME
Kind Regards,
Peter Smith.
Fujitsu Australia
ribe?
Would the text code be easier if you just validated the changes by
using the "describe" code (e.g. \dRs+ regress_testsub) instead of SQL
select of the pg_subscription_conflict table?
~
37.
nit - The test cases seemed mostly good to me. My nits are only for
small changes/typos or
On Mon, Sep 30, 2024 at 4:29 PM shveta malik wrote:
>
> On Mon, Sep 30, 2024 at 11:04 AM Peter Smith wrote:
> >
> > On Mon, Sep 30, 2024 at 2:27 PM shveta malik wrote:
> > >
> > > On Fri, Sep 27, 2024 at 1:00 PM Peter Smith wrote:
> > >
> &
HrUQZ8vzyfAcSgeTgQEoO_-f8CrhW4A%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
.org/message-id/CAHv8RjJ5_dmyCH58xQ0StXMdPt9gstemMMWytR79%2BLfOMAHdLw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
't
need to have a 'pub3'.
==
src/test/subscription/t/031_column_list.pl
11.
It is not clear to me -- is there, or is there not yet any test case
for the multiple publication issues that were discussed previously?
e.g. when the same table has gencols but there are multiple
publicati
On Mon, Sep 30, 2024 at 2:27 PM shveta malik wrote:
>
> On Fri, Sep 27, 2024 at 1:00 PM Peter Smith wrote:
...
> >
> > 13. General - ordering of conflict_type.
> >
> > nit - Instead of just some apparent random order, let's put each
> > insert/update/d
I discovered there are 2 resolvers
("error" and "apply_remote") that both claim to be defaults for the
same conflict types.
They both say:
+ It is the default resolver for insert_exists and
+ update_exists.
Anyway, this demonstrates that the current information was hard to re
On Wed, Sep 25, 2024 at 2:51 AM Bertrand Drouvot
wrote:
>
> Hi,
>
> On Mon, Sep 23, 2024 at 12:27:27PM +1000, Peter Smith wrote:
> > My review comments for v8-0001
> >
> > ==
> > contrib/pg_logicalinspect/pg_logicalinspect.c
> >
> > 1.
] my review of v31-0002.
https://www.postgresql.org/message-id/CAHut%2BPusbhvPrL1uN1TKY%3DFd4zu3h63eDebZvsF%3Duy%2BLBKTwgA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/replication/logical/tablesync.c
b/src/backend/replication/logical/tablesync.c
index
this test ought to be saying something more
about what you are doing with the 'publish_generated_columns'
parameters and what behaviour it was expecting.
==
Please refer to the attachment which addresses some of the nit
comments mentioned above.
==
[1] my review of v31-0001:
https:
needs to be done in SnapBuildState.
nit - "...should a change needs to be done" -- the word "needs" is
incorrect here.
How about:
"...if a change needs to be made to SnapBuildState."
"...if a change is made to SnapBuildState."
"...if SnapBuildState i
On Fri, Sep 20, 2024 at 2:26 PM Amit Kapila wrote:
>
> On Fri, Sep 20, 2024 at 4:16 AM Peter Smith wrote:
> >
> > On Fri, Sep 20, 2024 at 3:26 AM Masahiko Sawada
> > wrote:
> > >
> > > On Thu, Sep 19, 2024 at 2:32 AM Amit Kapila
> >
sleading since sometimes
generated columns get published "irrespective of the option".
So, I think the original parameter name 'include_generated_columns'
might be better here because IMO "include" seems more like "add them
if they are not already specified", which is exactly what this idea is
doing.
Thoughts?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
27;isolation_slot');
+ DROP EXTENSION pg_logicalinspect;
+}
Different indentation for the CREATE/DROP EXTENSION?
==
The attached file shows the whitespace nit (#4)
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/contrib/pg_logicalinspect/pg_logicalinspect.c
b/contrib/p
ed immediately after the SnapBuildOnDisk
fields they referred to.
Wasn't that original ordering better than how it is now ordered in
snapshot_internal.h?
==
Please also see the attachment, which implements some of those nits
mentioned above.
==
Kind Regards,
Peter Smith.
ndition just be
replaced with if (!has_pub_with_pubgencols)? It seems equivalent
unless I am mistaken.
==
Please refer to the attachment which implements some of the nits
mentioned above.
==
[1]
https://www.postgresql.org/message-id/CAD21AoBun9crSWaxteMqyu8A_zme2ppa2uJvLJSJC2E3DJxQVA%
San -- e.g. when there are 2
publications for the same table subscribed by just 1 subscription but
having different values of the 'publish_generated_columns' for the
publications.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sg
ecause 2 different people wrote them. It is not making reviews easier
with them split.
==
Please see the attachment which implements some of the nits above.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 2e7804e..cca54bc 1006
On Tue, Sep 17, 2024 at 4:15 PM Masahiko Sawada wrote:
>
> On Mon, Sep 16, 2024 at 8:09 PM Peter Smith wrote:
> >
> > I thought that the option "publish_generated_columns" is more related
> > to "column lists" than "row filters".
> &g
On Tue, Sep 17, 2024 at 7:02 AM Masahiko Sawada wrote:
>
> On Wed, Sep 11, 2024 at 10:30 PM Peter Smith wrote:
> >
> > Because this feature is now being implemented as a PUBLICATION option,
> > there is another scenario that might need consideration; I am thinking
> &
Here are a few comments for the patch v46-0001.
==
src/backend/replication/slot.c
1. ReportSlotInvalidation
On Mon, Sep 16, 2024 at 8:01 PM Bharath Rupireddy
wrote:
>
> On Mon, Sep 9, 2024 at 1:11 PM Peter Smith wrote:
> > 3. ReportSlotInvalidation
> >
> > I did
column will be filled as normal
> > with the
> > + publisher-side computed or default data.
> > +
> >
> > I don't understand this description. Why does this option have no
> > effect if the publisher-side column is a generated column?
> >
>
> The documentation was incorrect. Currently, replicating from a
> publisher table with a generated column to a subscriber table with a
> generated column will result in an error. This has now been updated.
>
> > ---
> > +
> > + This parameter can only be set true if
> > copy_data is
> > + set to false.
> > +
> >
> > If I understand this patch correctly, it doesn't disallow to set
> > copy_data to true when the publish_generated_columns option is
> > specified. But do we want to disallow it? I think it would be more
> > useful and understandable if we allow to use both
> > publish_generated_columns (publisher option) and copy_data (subscriber
> > option) at the same time.
> >
>
> Support for tablesync with generated columns was not included in the
> initial patch, and this was reflected in the documentation. The
> functionality for syncing generated column data has been introduced
> with the 0002 patch.
>
Since nothing was said otherwise, I assumed my v30-0001 comments were
addressed in v31, but the new code seems to have quite a few of my
suggested changes missing. If you haven't addressed my review comments
for patch 0001 yet, please say so. OTOH, please give reasons for any
rejected comments.
> The attached v31 patches contain the changes for the same. I won't be
> posting the test patch for now. I will share it once this patch has
> been stabilized.
How can the patch become "stabilized" without associated tests to
verify the behaviour is not broken? e.g. I can write a stable function
that says 2+2=5.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
html
[2] https://www.postgresql.org/docs/devel/sql-createpublication.html
Kind Regards,
Peter Smith.
Fujitsu Australia
pg_createsubscriber.c:1830:64: warning: declaration of ‘dbinfo’
shadows a global declaration [-Wshadow]
pg_createsubscriber.c:121:31: warning: shadowed declaration is here [-Wshadow]
==
Kind Regards,
Peter Smith.
Fujitsu Australia
controldata_utils.c:52:29: warning: declaration of ‘DataDir’ shadow
plemented again
soon for subscriptions, and then the initial sync tests here can be
properly implemented instead of the placeholders currently in patch
0002.
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPuDJToG%3DV-ogTi9_6fnhhn2S0%2BsVRGPynhcf9mEh0Q%3DLA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
omment #3)
IMO there should be another test case for a WARNING here if the user
attempts to include generated column 'd' in an explicit PUBLICATION
column list while the "publish_generated-columns" is false.
==
[1]
https://www.postgresql.org/message-id/CAD21AoA-tdTz0G-vri8KM2TXeFU8RCDsOpBXUBCgwkfokF7%3DjA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Sep 11, 2024 at 3:54 AM Bharath Rupireddy
wrote:
>
> Hi,
>
> Thanks for reviewing.
>
> On Tue, Sep 10, 2024 at 8:40 AM Peter Smith wrote:
> >
> > Commit message
> >
> > 1.
> > ALTER_REPLICATION_SLOT on invalidated replica
all cases where any old name is still
lurking?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
for pushing the patches 0001 and 0004.
I have rebased the two remaining patches. See v12 attached.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v12-0002-GUC-names-fix-case-datestyle.patch
Description: Binary data
v12-0001-GUC-names-fix-case-intervalstyle.patch
Description: Binary data
#
IMO we should update that "In passing..." sentence to something like:
In passing, ensure that replication slot stats are not removed when
the active slot is invalidated, and check that an error occurs when
attempting to alter the invalid slot.
==
[1] docs
irm it in server log/confirm it in the server log/
~
20.
+{
+ my ($node, $slot, $offset, $inactive_timeout) = @_;
+ my $name = $node->name;
+ my $invalidated = 0;
(same as the other subroutine)
nit - The variable $name seems too vague. How about $node_name?
==
Please refer to the attached
ex);
+
+ if (s->data.invalidated != RS_INVAL_INACTIVE_TIMEOUT)
+ s->inactive_since = *now;
+
+ if (acquire_lock)
+ SpinLockRelease(&s->mutex);
+}
Is the logic correct? What if the slot was already invalid due to some
reason other than RS_INVAL_INACTIVE_TIMEOUT? Is an Assert needed?
On Mon, Sep 9, 2024 at 12:20 PM David G. Johnston
wrote:
>
>
>
> On Sun, Sep 8, 2024, 18:55 Peter Smith wrote:
>>
>> Saying "The time..." is fine, but the suggestions given seem backwards to me:
>> - The time this slot was inactivated
>> - The tim
me inactive" or "was deactivated".
Rather, the 'inactive_since' timestamp field is simply:
- The time the slot was last active.
- The last time the slot was active.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ta,
Yes, I think so. I hadn't bothered to include this in the v1 patch
only because the docs are user-facing, but this code comment isn't.
However, now that you've mentioned it, I made the same change here
also. Thanks. See patch v2.
==
Kind Regards,
Peter Smith.
Fu
keep default test
execution time low).
I hope that if the idea to use PG_TEST_EXTRA for "subscription" tests
gets accepted then later we can revisit this, and put all the removed
extra test cases back in again.
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPsgtnr5BgcqYwD3PSf-AsUtVDE_j799AaZeAjJvE6HGtA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Sep 3, 2024 at 4:35 PM Michael Paquier wrote:
>
> On Tue, Sep 03, 2024 at 12:00:19PM +1000, Peter Smith wrote:
> > Here is the rebased patch set v10*. Everything is the same as before
> > except now there are only 7 patches instead of 8. The previous v9-0001
> >
On Tue, Sep 3, 2024 at 4:35 PM Bertrand Drouvot
wrote:
>
> Hi,
>
> On Tue, Sep 03, 2024 at 10:43:14AM +0530, Amit Kapila wrote:
> > On Mon, Sep 2, 2024 at 9:14 AM shveta malik wrote:
> > >
> > > On Mon, Sep 2, 2024 at 5:47 AM Peter Smith wrote:
> >
10 times.
A comment describing the logic for this subroutine would be helpful.
The most important side-effect of this function is the CHECKPOINT
because without that nothing will ever get invalidated due to
inactivity, but this key point is not obvious from the subroutine
name.
IMO it would be better to name this differently to reflect what it is
really doing:
e.g. "CHECKPOINT_and_wait_for_slot_invalidation_in_server_log"
==
Kind Regards,
Peter Smith.
Fujitsu Australia
D.
I hope these might be pushed soon to avoid further rebasing.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v10-0001-Add-quotes-for-GUCs-int.patch
Description: Binary data
v10-0004-Add-quotes-for-GUCs-enum.patch
Description: Binary data
v10-0003-Add-quotes-for-GUCs-string.patch
D
On Mon, Sep 2, 2024 at 1:28 PM shveta malik wrote:
>
> On Mon, Sep 2, 2024 at 4:20 AM Peter Smith wrote:
> >
> > On Fri, Aug 30, 2024 at 4:24 PM shveta malik wrote:
> > >
> > > On Fri, Aug 30, 2024 at 10:53 AM Peter Smith
> > > wrote:
> > &g
test SELECTs (test/subscription/t/026_stats.pl)
~
Thoughts?
==
[1] docs -
https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-SUBSCRIPTION-STATS
[2] stats for conflicts -
https://www.postgresql.org/message-id/flat/OS0PR01MB57160A07BD575773045FC214948F2%40OS0PR01MB5716.j
ome of the nits
mentioned above.
==
[1] v43 review -
https://www.postgresql.org/message-id/CAHut%2BPuFzCHPCiZbpoQX59kgZbebuWT0gR0O7rOe4t_sdYu%3DOA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 970
esql.org/docs/devel/view-pg-replication-slots.html
[2] thread -
https://www.postgresql.org/message-id/ca+tgmob_ta-t2ty8qrkhbgnnlrf4zycwhghgfsuuofraedw...@mail.gmail.com
[3] push -
https://github.com/postgres/postgres/commit/6d49c8d4b4f4a20eb5b4c501d78cf894fa13c0ea
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Aug 30, 2024 at 4:24 PM shveta malik wrote:
>
> On Fri, Aug 30, 2024 at 10:53 AM Peter Smith wrote:
> >
...
> > 2. Arrange all the counts into an intuitive/natural order
> >
> > There is an intuitive/natural ordering for these counts. For example,
> &g
On Fri, Aug 30, 2024 at 6:36 PM shveta malik wrote:
>
> On Fri, Aug 30, 2024 at 12:15 PM Zhijie Hou (Fujitsu)
> wrote:
> >
> >
> > Here is V5 patch which addressed above and Shveta's[1] comments.
> >
>
> The patch looks good to me.
>
Patch v5 L
left-to-right.
==
[1]
https://www.postgresql.org/docs/devel/logical-replication-conflicts.html#LOGICAL-REPLICATION-CONFLICTS
Kind Regards,
Peter Smith.
Fujitsu Australia
t - that is maybe a bit misleading because IIUC there is no real
"waiting" happening anywhere. Suggest:
Sets the amount of time a replication slot can remain inactive before
it will be invalidated.
==
Please take a look at the attached top-up patches. These include
changes f
; recorded in
pg_stat_subscription_stats?
The 'insert_exists' is either not happening or is not being counted
during table synchronization. Either way, it was not what I was
expecting. If it is not a bug, maybe the docs need to explain more
about the rules for 'insert_exists' during the initial table sync.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
devel/monitoring-stats.html#MONITORING-PG-STAT-DATABASE-CONFLICTS-VIEW
[3]
https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-REPLICATION-SLOTS-VIEW
[4]
https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-ALL-TABLES-VIEW
Kind Regards,
Peter Smith.
Fujitsu Australia
similar to other conflict names 'insert_exists'
> > > > and 'update_exists'.
> > >
> > > Since we reached a consensus on this, I am attaching a small patch to
> > > rename
> > > as suggested.
> >
> > Sorry, I attached the wrong patch. Here is correct one.
> >
>
> LGTM.
>
LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, Aug 26, 2024 at 10:13 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Monday, August 26, 2024 3:30 PM Peter Smith wrote:
> >
> > ==
> > src/include/replication/conflict.h
> >
> > nit - defined 'NUM_CONFLICT_TYPES' inside the enum (I think this
On Mon, Aug 26, 2024 at 7:52 PM Amit Kapila wrote:
>
> On Thu, Aug 22, 2024 at 2:21 PM Amit Kapila wrote:
> >
> > On Thu, Aug 22, 2024 at 1:33 PM Peter Smith wrote:
> > >
> > > Do you think the documentation for the 'column_value' parameter of
quot; comments that did not
mention conflicts
nit - tweak comments wording for update_differ and delete_differ
nit - /both > 0/> 0/
nit - /both 0/0/
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/logical-replication.sgml
b/doc/src/sgml/logical-replication
resolver
etc.
Advantages of my suggestion:
- Close to existing SQL syntax
- No loss of clarity by removing the word "RESOLVER"
- No requirement for new keyword/s
- The commands now read more like English
- Offers more flexibility for any unknown future requirements
- The setup (via create subscription) and the alter/reset all look the same.
==
[1] https://www.postgresql.org/docs/current/sql-insert.html#SQL-ON-CONFLICT
Kind Regards,
Peter Smith.
Fujitsu Australia
lf-explanatory, so some of the test
"messages" were simplified
==
[1]
https://www.postgresql.org/message-id/CALDaNm31LZQfeR8Vv1qNCOREGffvZbgGDrTp%3D3h%3DEHiHTEO2pQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
v29-0001-Enable-support-for-include_generated_columns-opt.
logging should say that the displayed value might be
truncated?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Aug 22, 2024 at 8:54 AM Peter Smith wrote:
>
> On Wed, Aug 21, 2024 at 8:48 PM Amit Kapila wrote:
> >
> > On Fri, Aug 16, 2024 at 9:45 AM vignesh C wrote:
> > >
> > > On Thu, 15 Aug 2024 at 12:55, Peter Smith wrote:
> > > >
> > &
documented what makes these names be shown or not
shown.
~~~
Please see the attachment which implements most of the items mentioned above.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/logical-replication.sgml
b/doc/src/sgml/logical-replication.sgml
index
On Wed, Aug 21, 2024 at 8:48 PM Amit Kapila wrote:
>
> On Fri, Aug 16, 2024 at 9:45 AM vignesh C wrote:
> >
> > On Thu, 15 Aug 2024 at 12:55, Peter Smith wrote:
> > >
> > > Hi Hackers,
> > >
> > > While reviewing another logical replication
less text but much simpler
text too.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi Vignesh, Here are my only review comments for the latest patch set.
v20240820-0003.
nit - missing period for comment in FetchRelationStates
nit - typo in function name 'ProcessSyncingTablesFoSync'
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/r
CFBot reported some failures, so I have attached the rebased patch set v9*.
I'm hopeful the majority of these might be pushed to avoid more rebasing...
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v9-0001-Add-quotes-for-GUCs-bool.patch
Description: Binary data
v9-0005-Add-quote
On Tue, Aug 20, 2024 at 10:18 AM Bruce Momjian wrote:
>
> Great, patch applied to master.
>
Thanks for pushing!
==
Kind Regards,
Peter Smith.
Fujitsu Australia
for this test. Done as suggested in
attached patch v2.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v2-0001-Add-missing-test-case.patch
Description: Binary data
TABLES). I felt they logically belong in sequencesync.c, not here.
- process_syncing_sequences_for_apply(void)
~~~
FetchRelationStates:
nit - the comment change about "not-READY tables" (instead of
relations) should be already in patch 0003.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
uot;PASS".
* The test "# TEST tab_alter" expected empty result also seems
unhelpful. It might be related to the previous bullet.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ed even if it is not
explicitly specified.
nit - The "default" is only like this for "test_decoding" (e.g., the
CREATE SUBSCRIPTION option is the opposite), so let's make the comment
clearer about that.
nit - Use sentence case in the comments.
==
Kind Regards,
Peter
confusion caused by ignoring the name
conventions.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
/mismatched_seqs/
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/commands/subscriptioncmds.c
b/src/backend/commands/subscriptioncmds.c
index 534d385..e799a41 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -800,9
On Tue, Jul 23, 2024 at 9:23 AM Peter Smith wrote:
>
> On Fri, Jul 19, 2024 at 4:01 PM Shlok Kyal wrote:
> >
> > On Thu, 18 Jul 2024 at 13:55, Peter Smith wrote:
> > >
> > > Hi, here are some review comments for v19-0002
> > > ==
> >
.
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPt5FqV7J9GnnWFRNW_Z1KOMMAZXNTRcRNdtFrfMBz_GLA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Add-missing-test-case.patch
Description: Binary data
- variable declaration indent
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/commands/subscriptioncmds.c
b/src/backend/commands/subscriptioncmds.c
index 9fff288..22115bd 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -726,
c/backend/utils/misc/guc_tables.c
nit - /max workers/maximum number of workers/ (for consistency because
all other GUCs are verbose like this; nothing just says "max".)
==
src/test/subscription/t/034_sequences.pl
nit - adjust the expected WARNING message (which was modified above)
On Tue, Aug 13, 2024 at 10:00 PM vignesh C wrote:
>
> On Tue, 13 Aug 2024 at 09:19, Peter Smith wrote:
> >
> > 3.1. GENERAL
> >
> > Hmm. I am guessing this was provided as a separate patch to aid review
> > by showing that existing functions are moved? OTOH y
ization for subscription "sub3", sequence "seq_0569" has
finished
2024-08-13 16:41:47.436 AEST [11735] LOG: logical replication
synchronization for subscription "sub3", sequence "seq_0570" has
finished
2024-08-13 16:41:47.436 AEST [11735] LOG: logical replication
synchronization for subscription "sub3", sequence "seq_0571" has
finished
2024-08-13 16:41:47.436 AEST [11735] LOG: logical replication
synchronization for subscription "sub3", sequence "seq_0582" has
finished
...
Is there a way to refresh sequences in a more natural (e.g.
alphabetical) order to make these logs more readable?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
CYCLE
syntax (plus its description) to be adjacent to MINVALUE/MAXVALUE,
which IMO is where it naturally belongs.
Please see the attached patch.
Thoughts?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
docs_sequence_cycle.diff
Description: Binary data
On Mon, Aug 12, 2024 at 11:07 PM vignesh C wrote:
>
> On Mon, 12 Aug 2024 at 10:40, Peter Smith wrote:
> >
> > Hi Vignesh,
> >
> > I found that when 2 subscriptions are both subscribing to a
> > publication publishing sequences, an ERROR occurs on refre
quot;),
+ gettext_noop("Maximum number of relation synchronization workers per
subscription."),
NULL,
},
I was wondering if "relation synchronization workers" is meaningful to
the user because that seems like new terminology.
Maybe it should say "... of table + sequence sy
AEST [7306] STATEMENT: alter subscription
sub2 refresh publication sequences;
ERROR: tuple already updated by self
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ABLES, SEQUENCES one?
Similarly, there are other combinations not possible
DROP ALL SEQUENCES from a publication that is FOR ALL TABLES, SEQUENCES
DROP ALL TABLES from a publication that is FOR ALL TABLES, SEQUENCES
ADD ALL TABLES to a publication that is FOR ALL SEQUENCES
...
==
Kind Regards,
.), where you can put functions such as
process_syncing_relations() plus any other code common to both
tablesync and sequencesync. That might make more sense then having one
call to the other.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/ref/create_publication.sgml
n my nitpicks attachment)
==
doc/src/sgml/ref/alter_subscription.sgml
nitpick - I reversed the paragraphs to keep the references in a natural order.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Aug 9, 2024 at 1:52 AM vignesh C wrote:
>
> On Thu, 8 Aug 2024 at 08:30, Peter Smith wrot
Hi Vignesh, I reviewed the latest v20240808-0003 patch.
Attached are my minor change suggestions.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/catalog/pg_subscription.c
b/src/backend/catalog/pg_subscription.c
index 4e2f960..a77e810 100644
--- a/src/backend
s test. -- e.g. something like:
+# ERROR: logical replication target relation
"public.tab_nogen_to_gen" is missing
+# replicated column: "b"
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Aug 8, 2024 at 1:55 PM Amit Kapila wrote:
>
> On Wed, Aug 7, 2024 at 10:12 AM Peter Smith wrote:
> >
> > This is mostly a repeat of my previous mail from a while ago [1] but
> > includes some corrections, answers, and more examples. I'm going to
> > tr
in the first place?
==
Also, there are more minor suggestions in the attached nitpicks diff.
==
[1] https://www.postgresql.org/docs/current/logical-replication.html
[2]
file:///usr/local/pg_oss/share/doc/postgresql/html/logical-replication-failover.html
Kind Regards,
Peter Smith.
F
ich has no generated column on the publisher side. So it seems this
is a bug in patch 0001.
FYI, I have included "FIXME" comments in the attached top-up diff
patch to show which test cases I think are expecting wrong results.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
d
er chooses whether to use
"REFRESH PUBLICATION SEQUENCES" versus "REFRESH PUBLICATION WITH
(copy_data=true)". OTHO, the proposed syntax eliminates ambiguity.
ex9. (using default copy_data)
ALTER SUBSCRIPTION sub REFRESH PUBLICATION
- same as ex8
==
[1]
https://www.postgresql
cription/t/034_sequences.pl
nitpick - add some ## comments to highlight the main test
parts to make it easier to read.
nitpick - fix typo /syned/synced/
7. More test cases?
IIUC you can also get a sequence mismatch warning during "ALTER ...
REFRESH PUBLICATION", and &q
ication origin "pg_16390" during message type "INSERT" in
transaction 742, finished at 0/1967BB0
2024-08-05 13:25:24.927 AEST [20039] LOG: background worker "logical
replication apply worker" (PID 11225) exited with exit code 1
...
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPvtT8fKOfvVYr4vANx_fr92vedas%2BZRbQxvMC097rks6w%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
,
Peter Smith.
Fujitsu Australia
diff --git a/src/test/subscription/t/011_generated.pl
b/src/test/subscription/t/011_generated.pl
index 0b596b7..2be06c6 100644
--- a/src/test/subscription/t/011_generated.pl
+++ b/src/test/subscription/t/011_generated.pl
@@ -12,16 +12,25 @@ use Test::More;
my
tioned above.
~~
BTW, For a quicker turnaround and less churning please consider just
posting the v23-0001 by itself instead of waiting to rebase all the
subsequent patches. When 0001 settles down some more then rebase the
others.
~~
Also, please run the indentation tool over this code
test_sub=# SELECT * FROM nextval('s1');
nextval
-
11
(1 row)
(Looking at the description you would expect odd values for this
sequence to be impossible)
test_sub=# \dS+ s1
Sequence "public.s1"
Type | Start | Minimum | Maximum | Increment | Cycle
1 - 100 of 1208 matches
Mail list logo
