On Fri, Jan 31, 2025 at 11:04 AM Masahiko Sawada wrote:
>
> On Wed, Jan 29, 2025 at 9:32 PM Peter Smith wrote:
> >
> > ==
> > .../replication/logical/reorderbuffer.c
> >
> > ReorderBufferCheckAndTruncateAbortedTXN:
> >
> > 2.
> > It
On Fri, Jan 31, 2025 at 11:19 AM Peter Smith wrote:
...
> On Fri, Jan 31, 2025 at 1:02 AM vignesh C wrote:
> >
> > On Thu, 30 Jan 2025 at 16:02, Amit Kapila wrote:
> > >
> > > I have made minor changes in the attached. The main change is to raise
> >
done to avoid a
TRAP, but I am not sure if this is the correct fix. At the point of
that ereport ERROR this slot doesn't yet belong to us because we are
still determining *can* we acquire this slot or not, so I felt it
doesn't seem correct to have the MyReplicationSlot code/comment ("it's
ours now") come before the ERROR.
Furthermore, having the code/comment "We made this slot active, so
it's ours now" ahead of the other code/comment "... but it's already
active in another process..." is contradictory.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Jan 28, 2025 at 9:26 PM Masahiko Sawada wrote:
>
> On Mon, Jan 27, 2025 at 7:01 PM Peter Smith wrote:
> >
...
> > To be honest, I didn't understand the "CLEAR" part of that name. It
> > seems more like it should've been called somet
e commit
log status.
The lag between the commit/abort happening and these flag getting set
seems unintuitive. Should they be named differently -- e.g. maybe
RBTXN_IS_CLOG_COMMITTED, RBTXN_IS_CLOG_ABORTED instead?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
e-id/CAExHW5sQGie7bvS-q7YUYDM2BqYZ%3D%2BxqeqFUS%3DcZGjK_9pnVzQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Jan 29, 2025 at 7:56 PM Amit Kapila wrote:
>
> On Wed, Jan 29, 2025 at 7:50 AM Peter Smith wrote:
> >
> > Some review comments for patch v1-0001.
> >
> > ==
> > src/backend/replication/logical/slotsync.c
> >
> > ReplSlotSyncWorkerMain:
On Wed, Jan 29, 2025 at 2:48 PM Amit Kapila wrote:
>
> On Wed, Jan 29, 2025 at 6:03 AM Peter Smith wrote:
> >
> > On Tue, Jan 28, 2025 at 7:59 PM Amit Kapila wrote:
> > >
> > > On Thu, Jan 23, 2025 at 9:58 AM vignesh C wrote:
> > > >
> >
lly lost? (in
2 places)
Not only that but the accompanying comment modification (to mention
"and idle_replication_slot_timeout is set to 0") is also MIA last seen
in v63 (??)
==
[1]
https://www.postgresql.org/message-id/CABdArM6pBL5hPnSQ%2B5nEVMANcF4FCH7LQmgskXyiLY75TMnKpw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
ose
function comment says "This is exactly like errdetail() except that
strings passed to errdetail_internal are not translated...".
Isn't there some contradiction here?
Perhaps the _() macro is not needed, or perhaps the
errdetail_internal() should be errdetail().
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ow;
}
~
So, should you also assign a 'now' value outside this loop and pass
that timestamp down the calls so they eventually all get assigned the
same? I don't know, but I guess at least that would require much fewer
unnecessary calls to GetCurrentTimestamp so that may be a good thing
entence, shouldn't it be 'output plugin' instead of
> 'plugin output'? We use that way at other places in the docs.
>
Fixed.
~~~
I also modified some whitespace indentations in the SGML file.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v57-0001-DOCS-Generated-Column-Replication.patch
Description: Binary data
at:
/* Does this transaction make changes to the current snapshot? */
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Jan 28, 2025 at 4:31 AM Masahiko Sawada wrote:
>
> On Sun, Jan 26, 2025 at 10:26 PM Amit Kapila wrote:
> >
> > On Fri, Jan 24, 2025 at 12:38 AM Masahiko Sawada
> > wrote:
> > >
> > > On Wed, Jan 22, 2025 at 7:35 PM Peter Smith wrote:
>
ep the internal structure name as 'pubgencols_type'
> > as it is not exposed, unless you prefer to update it to 'pubgencols'
> > as well.
>
> The attached patch has the changes for the same.
>
Hi Vignesh
The changes LGTM.
I was surprised that there was no need to modify any expected test
output. I guess that means there are no tests anywhere directly
looking at the pg_publication catalog column names, but instead, all
tests for that catalog must be going via a publication view or using
psql describe output.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Jan 24, 2025 at 7:28 PM Ashutosh Bapat
wrote:
>
> On Fri, Jan 24, 2025 at 8:14 AM Peter Smith wrote:
> >
> > On Thu, Jan 23, 2025 at 10:33 PM Ashutosh Bapat
> > wrote:
> > >
> > > On Wed, Jan 22, 2025 at 7:29 PM Shubham Khanna
> > >
icitly (e.g. --all-databases).
>
+1 better to be safe.
Instead of a new switch, how about changing the --database switch to
accept a pattern (like pg_dump --schema does [1])
Then "all databases" would be specified something like --database = *
==
[1] https://www.postgresql.org/docs/current/app-pgdump.html
Kind Regards,
Peter Smith.
Fujitsu Australia
Patch v13-0001 LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
oes what it says and
looked ok to me.
However, in hindsight, I am not sure that the column should have been
renamed 'pubgencols_type' in the first place because I cannot find any
other catalogs with an underscore in their column names.
Maybe 'pubgencolstype' or simply leaving it as
tion', 'pub1',
+ '--publication', 'pub2',
+ '--subscription', 'sub1',
+ '--subscription', 'sub2',
+ '--database', $db1,
+ '--database', $db2
],
2a.
+# And, same again to see the output...
Maybe a better comment
Patch v56-0001 LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Patch v14-0001 LGTM
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Jan 23, 2025 at 2:17 PM Amit Kapila wrote:
>
> On Wed, Jan 22, 2025 at 9:21 AM Peter Smith wrote:
> >
> > On Wed, Jan 22, 2025 at 5:36 AM Masahiko Sawada
> > wrote:
> > >
> > > On Sun, Jan 19, 2025 at 7:53 PM Amit Kapila
> > > wrot
etween block
> elements on the same level.
>
Fixed.
==
[1] PeterE v54-0005 review -
https://www.postgresql.org/message-id/18f56f71-ea01-41aa-811e-367b692e9ca4%40eisentraut.org
Kind Regards,
Peter Smith.
Fujitsu Australia
v56-0001-DOCS-Generated-Column-Replication.patch
Description: Binary data
olumns-option-to-use-e.patch
error: patch failed: doc/src/sgml/catalogs.sgml:6396
error: doc/src/sgml/catalogs.sgml: patch does not apply
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ese changes while experimenting with the suggestions.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index e39612f..9391922 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6399,10 +6399,1
Patch v54-0002 LGTM, although it is missing an explanatory commit message.
e.g. Should be something like:
--
Adds the documentation for the 'pubgencols' column of catalog
pg_publication. This was accidentally missing from commit 7054186.
--
==
Kind Regards,
Peter Smit
On Tue, Jan 21, 2025 at 7:28 PM vignesh C wrote:
>
> On Mon, 20 Jan 2025 at 06:14, Peter Smith wrote:
> >
> > Hi Vignesh,
> >
> > Review comments for patch v53-0001:
> >
> >
> > Maybe I have some fundamental misunderstanding here, but I don'
On Wed, Jan 22, 2025 at 5:36 AM Masahiko Sawada wrote:
>
> On Sun, Jan 19, 2025 at 7:53 PM Amit Kapila wrote:
> >
> > On Fri, Jan 17, 2025 at 11:19 PM Masahiko Sawada
> > wrote:
> > >
> > > On Wed, Jan 15, 2025 at 4:43 PM Peter Smith wrote:
> >
1b.
Secondly, is this comment strictly correct? IIUC it's not *always*
going to be invalidated just because the cause is one of those listed.
e.g. the code calls InvalidatePossiblyObsoleteSlot but it might not
end up invalidating the slot having a cause RS_INVAL_IDLE_TIMEOUT.
======
Kind Re
AFAICT the code/comment does not match quite right. The
comment refers to a setting that is "during binary upgrade", yet the
Assert can only pass if IsBinaryUpgrade is false. (??)
I'm unsure of the intent; perhaps it should be like:
if (IsBinaryUpgrade)
Assert(!(*invalidated && SlotIsLogical(s)));
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Jan 21, 2025 at 8:10 AM Dagfinn Ilmari Mannsåker
wrote:
>
> Hi Peter,
>
> Peter Smith writes:
>
> > On Fri, Dec 13, 2024 at 5:03 AM Dagfinn Ilmari Mannsåker
> > wrote:
> >>
> >> On Thu, 12 Dec 2024, at 17:52, Andrew Dunstan wrote:
>
r way is OK for me.
==
[1]
https://www.postgresql.org/message-id/CANhcyEXQ1h%3DoSPFFziCZuU6far6a82DQafL0S85CyVRyEntA%2Bw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
orm of the
publication option introduced in your patch 0003
~~~
2.
Fixed a typo /explicty/explicitly/
==
I've attached a top-up patch to "fix" all issues I found for the
v53-0005 docs patch. Please have a look and apply them if they seem OK
to you.
==
Kind Regards,
Peter Smit
Hi Vignesh.
Here are my review comments for patch v53-0003.
On Sun, Jan 19, 2025 at 11:17 PM vignesh C wrote:
>
> On Fri, 17 Jan 2025 at 11:23, Peter Smith wrote:
> >
> > Hi Vignesh.
> >
> > Some review comments for patch v52-0003
> >
> >
this doc
fragment to change the type from bool to char; it should also itemise
the possible values 'n', 's' saying what those values mean.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
atch.
On Sun, Jan 19, 2025 at 11:07 PM vignesh C wrote:
>
> On Sun, 19 Jan 2025 at 06:39, Peter Smith wrote:
> >
> > Hi Vignesh,
> >
> > I was having some second thoughts about this patch and my previous
> > suggestion.
> >
>
, ", 'f' AS pubviaroot");
~~
Unless I am mistaken this will simplify the subsequent code a lot because:
1. Now you can put the cols in the same order you want to display them
2. Now the tuple result has a fixed number of cols for all server versions
3. Now hardcoding the indexes (1,2,3,4...) is fine because they are
always the same
Thoughts?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi Vignesh.
Some review comments for patch v52-0003
==
1. GENERAL - change to use enum.
On Thu, Jan 16, 2025 at 7:47 PM vignesh C wrote:
>
> On Wed, 15 Jan 2025 at 11:17, Peter Smith wrote:
> > 2.
> > As suggested in more detail below, I think it would be better if you
On Thu, Jan 16, 2025 at 7:47 PM vignesh C wrote:
>
...
>
> v52-0002 - One typo related to a publication name which Peter reported.
Hi Vignesh,
Patch v52-0002 LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ng like this can work?
pubviaroot_idx = ncols;
pubgencols_idx = ncols + 1;
==
Kind Regards,
Peter Smith.
Fujitsu Australia
2
Use the --enable-two-phase switch to enable two_phase.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Patch v11-0001 LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Jan 16, 2025 at 3:26 PM Tom Lane wrote:
>
> Peter Smith writes:
> > During some recent reviews, I came across some comments mentioning "toast"
> > ...
> > TOAST is a PostgreSQL acronym for "The Oversized-Attribute Storage
> > Technique"
"detoast")
~~~
There are many more "toast" examples found in the source code
comments, but I'll first wait to see if this patch is accepted before
looking to address those.
==
[1] TOAST -- https://www.postgresql.org/docs/current/storage-toast.html
[2] toast -- https://en.wikipedia.org/wiki/Toast_(food)
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-TOAST-not-toast.patch
Description: Binary data
On Thu, Jan 16, 2025 at 4:28 AM Masahiko Sawada wrote:
>
> On Sun, Jan 12, 2025 at 10:52 PM Peter Smith wrote:
> >
> > On Fri, Jan 10, 2025 at 8:28 PM Masahiko Sawada
> > wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Jan 7, 2025 a
o.
>
My thoughts are that any consistency improvement is a step in the
right direction so even "don't increase the consistency much" is still
better than nothing.
But if I am outvoted that's OK. It is not a big deal.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
cation slots");
pg_log_warning_detail("Subscriptions will be created with the
two_phase option disabled. "
"Prepared transactions will be
replicated at COMMIT PREPARED.");
pg_log_warning_hint("You can use '--enable-two
On Mon, Jan 13, 2025 at 8:27 PM Amit Kapila wrote:
>
> On Mon, Jan 13, 2025 at 5:25 AM Peter Smith wrote:
> >
> > Future -- there probably need to be further clarifications/emphasis to
> > describe how the generated column replication feature only works for
> >
eter
-# 'publish_generated_columns' is set to false (to confirm existing default
+# 'publish_generated_columns' is set to 'none' (to confirm existing default
# behavior), and is set to true (to confirm replication occurs).
Not fully updated. This is still saying "and is set to true"
~~~
25.
# Verify that the generated column data is not replicated during incremental
# replication when publish_generated_columns is set to false.
The above comment (still present in the file) still refers to the
boolean values of the option.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi Shubham.
Patch v9-0001 LGTM.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Dec 13, 2024 at 12:17 AM Dagfinn Ilmari Mannsåker
wrote:
>
> Dagfinn Ilmari Mannsåker writes:
>
> > Peter Smith writes:
> >
> >> On Thu, Dec 12, 2024 at 2:53 PM Michael Paquier
> >> wrote:
> >> ...
> >>
> >>> &g
: stopping the subscriber
...
--
See the attached diff for the TAP changes I used to expose this
logging. Apply this atop v8.
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPszk61QM%2BcEvq_1-A2y%2BJrAD0USB%2BNvtcidajYOfHDkyw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fu
dant _TXN. e.g. we don't say RBTXN_IS_COMMITTED_TXN etc.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ction status,
allowing logical decoding to skip changes for non-system catalogs if
the transaction is already aborted.
On Tue, Jan 14, 2025 at 5:56 AM Masahiko Sawada wrote:
>
> On Mon, Jan 6, 2025 at 5:52 PM Peter Smith wrote:
> >
> > Hi Sawada-San.
> >
> > Here are
s with replica identity set NOTHING,
> set DEFAULT but with no primary key, or set
> USING INDEX but the index has been
> dropped, cannot be updated or deleted when added to a publication that
> replicates these operations. Attempting to do so will result in an
> error on the publisher."
>
>
I thought Amit's proposed text was mostly OK; it only needed the
"lacking a replica identity" part to be removed. (I've also changed
the e.g. to i.e.)
Like this:
"Tables with an insufficiently defined replica identity (i.e., set to
NOTHING, set to DEFAULT but with no primary key, or set USING INDEX
but the index has been dropped) cannot be updated or ...".
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Patch v3-0001 LGTM
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, Jan 13, 2025 at 5:52 PM vignesh C wrote:
>
> On Thu, 2 Jan 2025 at 15:57, Nisha Moond wrote:
> >
> > On Thu, Jan 2, 2025 at 8:16 AM Peter Smith wrote:
> > >
> > > Hi Nisha,
> > >
> > > Here are some minor review comments for patc
On Fri, Jan 10, 2025 at 8:28 PM Masahiko Sawada wrote:
>
> Hi,
>
> On Tue, Jan 7, 2025 at 11:30 PM Peter Smith wrote:
> >
> > Hi Sawada-San.
> >
> > FWIW, I also thought it was a good idea suggested by Bertrand [1] to
> > "hide" everything
ert
> >> these too (and replace some of the concatenations with interpolation).
> >>
> >> Technically the quotes aren't necessary around single-dash options
> >> before => since unary minus works on strings as well as numbers, but
> >> I'll leav
On Sun, Jan 12, 2025 at 12:58 AM Robert Treat wrote:
>
> On Thu, Jan 9, 2025 at 10:41 PM Peter Smith wrote:
> >
> > Hi Robert.
> >
> > The content and rendering of patch v2 LGTM.
> >
> > Should the word wrapping within the file
> > doc/src/s
On Mon, Jan 13, 2025 at 10:55 AM Peter Smith wrote:
>
> Hi,
>
> Some patches of this thread have fallen though the cracks for more
> than 2 months now, so I am re-posting them so that do not get
> overlooked any longer.
>
> For v49 [1] there were 2 patches:
>
%40mail.gmail.com
[3] pg_publication -
https://www.postgresql.org/message-id/DM4PR84MB1734D79B25D5B427F5B6101AEE5C2%40DM4PR84MB1734.NAMPRD84.PROD.OUTLOOK.COM
Kind Regards,
Peter Smith.
Fujitsu Australia
v51-0002-Add-missing-pubgencols-attribute-docs-for-pg_pub.patch
Description: Binary data
v51-0001
Hi Robert.
The content and rendering of patch v2 LGTM.
Should the word wrapping within the file
doc/src/sgml/logical-replication.sgml be tidied up though?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Sun, Dec 8, 2024 at 10:57 AM Tomas Vondra wrote:
>
> On 8/22/24 05:21, Peter Smith wrote:
> > ...
> >>>
> >>> I also don't see a test for this error condition. However, it is not
> >>> clear to me how important is it to cover this error c
nst "-1". The actual value
is of no real importance to the patch, so going to extra trouble to
extract an int64 that we don't care about seems unnecessary
==
Kind Regards,
Peter Smith.
Fujitsu Australia
keep_size.
But, I can only see a value of -1 in the log file
'tmp_check/log/regress_log_040_pg_createsubscriber', which may not
even be from the same test. Am I looking in the wrong place (???) e.g.
Where is the logging evidence of that publisher's bad GUC (10MB) value
in the logs before the warning is emitted?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Sat, Jan 4, 2025 at 4:23 AM Robert Treat wrote:
>
> On Wed, Dec 18, 2024 at 5:56 AM Amit Kapila wrote:
> >
> > On Thu, Feb 8, 2024 at 9:57 AM Laurenz Albe
> > wrote:
> > >
> > > On Thu, 2024-02-08 at 13:40 +1100, Peter Smith wrote:
> > >
for logical decoding to be activated."
Should this be more like "Waiting for a requested logical decoding
activation to complete."
==
src/include/access/xlog.h
51.
-#define XLogLogicalInfoActive() (wal_level >= WAL_LEVEL_LOGICAL)
+#define XLogLogicalInfoActive() (XLogLogicalInfoEnabled())
Those extra parentheses seem redundant now.
==
src/include/replication/logicalxlog.h
52.
+/*-
+ * logicalxlog.h
+ * Exports from replication/logical/logicalxlog.c.
+ *
+ * Copyright (c) 2012-2024, PostgreSQL Global Development Group
+ *
+ *-
/2024/2025/
~~~
53.
+typedef enum LogicalDecodingStatus
+{
+ LOGICAL_DECODING_STATUS_DISABLED = 0,
+ LOGICAL_DECODING_STATUS_XLOG_LOGICALINFO,
+ LOGICAL_DECODING_STATUS_READY,
+} LogicalDecodingStatus;
Maybe some more comments about these status phases would be helpful.
Specifically, it's not immediately clear if the
LOGICAL_DECODING_STATUS_XLOG_LOGICALINFO is a real state we could find
ourselves permanently, or if it is more like just a temporary
"pending" state while transitioning from DISABLED to READY.
AFAICT it is just transitory, so in that case, maybe
LOGICAL_DECODING_PENDING might be a more appropriate name.
~~~
54.
+typedef struct XLogLogicalInfoCtlData XLogLogicalInfoCtlData;
+extern XLogLogicalInfoCtlData *XLogLogicalInfoCtl;
+extern bool XLogLogicalInfo;
Add a blank line between the typedef and the externs.
==
[1] Bertrand's hide everything idea -
https://www.postgresql.org/message-id/Z3fimYj0fbkLmWJb%40ip-10-97-1-34.eu-west-3.compute.internal
Kind Regards,
Peter Smith.
Fujitsu Australia
?
IMO some minor renaming of the existing constants (and also their
associated macros) might help to make all this more coherent. For
example, perhaps like:
#define RBTXN_IS_PREPARE_NEEDED0x0040
#define RBTXN_IS_PREPARE_SKIPPED 0x0080
#define RBTXN_IS_PREPARE_SENT 0x0200
==
Kind Regards,
Peter Smith.
Fujitsu Australia
equences_alltables FOR ALL
SEQUENCES, ALL TABLES, ALL SEQUENCES;
+ERROR: invalid publication object list
+LINE 1: ...equences_alltables FOR ALL SEQUENCES, ALL TABLES, ALL SEQUEN...
+ ^
+DETAIL: SEQUENCES can be specified only once.
Should the DETAIL message say ALL SEQUENCES instead of just SEQUENCES?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ee attached diff)
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/logical-replication.sgml
b/doc/src/sgml/logical-replication.sgml
index 925e0dd..3b01694 100644
--- a/doc/src/sgml/logical-replication.sgml
+++ b/doc/src/sgml/logical-replication.sgml
@@ -1993,8 +1993,8 @@ CO
to make this change.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/ref/pg_createsubscriber.sgml
b/doc/src/sgml/ref/pg_createsubscriber.sgml
index c5ee5f4..40b8fac 100644
--- a/doc/src/sgml/ref/pg_createsubscriber.sgml
+++ b/doc/src/sgml/ref/pg_createsubscriber.sgml
something like:
Enum for phases of the subscription relations state.
~
2b.
/is no longer valid and/is no longer valid, and/
`
2c.
/that subscription relation state is up-to-date/that the subscription
relation state is up-to-date/
==
Kind Regards,
Peter Smith.
Fujitsu Australia
quired WAL files are not prematurely
removed.
SUGGESTION (pg_log_warning_hint)
Set the configuration parameter \"%s\" to -1 to ensure that required
WAL files are not prematurely removed.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
and the
existing variable 'ncols'.
AFAICT you can just move/replace the declaration of 'ncols' to where
'cols' is declared, and then you can remove the duplicated code below
(because the above code is already doing the same thing).
if (has_pubtruncate)
ncols++;
if (has_pubgencols)
ncols++;
if (has_pubviaroot)
ncols++;
if (has_pubsequence)
ncols++;
==
Kind Regards,
Peter Smith.
Fujitsu Australia
meout not enabled in
PG_TEST_EXTRA";
~
4b.
Should the check be done right at the top of the file (e.g. even
before the "# Testcase start" comment)?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
e pushed to
master by itself, i.e. independent of anything else in this thread
that is being done for the purpose of implementing the timeout
feature?
==
[1]
https://www.postgresql.org/message-id/CABdArM5tcYTQ2zeAPWTciTnea4jj6sPUjVY9M1O-4wWoTBjFgw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Austalia
ce of later replication errors.
So, I think the implementation needs to choose either A or B.
Currently, it seems a mixture.
==
[1] my v3 review -
https://www.postgresql.org/message-id/CAHut%2BPsgEphCa-P-3Q7cORA%3D1TRv15UUsaxN9ZkX56M1-J_QRg%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CALDaNm09cRzke52UN5zx33PT390whU92oXY4gfOSZEo17CLPjw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
saying: "Too bad, it's all broken now; we warned you (only if you
tried a dry run), but you did not listen".
In other words, why not make this an error condition up-front to
entirely remove any chance of this failure?
==
[1] https://www.postgresql.org/docs/17/app-pgcreatesubscriber.html
Kind Regards,
Peter Smith.
Fujitsu Australia
le synchronization worker to continue the
synchronization process. This behaviour ensures that transient errors
do not permanently disrupt the replication setup. See also
wal_retrieve_retry_interval.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
a
right".
There is a third option.
Keep the tests. Because they take excessive time to run, that simply
means you should run them *conditionally* based on the PG_TEST_EXTRA
environment variable so they don't impact the normal BF execution. The
documentation [1] says this env var is for "resource intensive" tests
-- AFAIK this is exactly the scenario we find ourselves in, so is
exactly what this env var was meant for.
Search other *.pl tests for PG_TEST_EXTRA to see some examples.
==
[1] https://www.postgresql.org/docs/17/regress-run.html
Kind Regards,
Peter Smith.
Fujitsu Australia
ation_slot_timeout = 1d # in minutes; 0 disables
I felt it might be better to say 24h here instead of 1d. And, that
would also be consistent with the docs, which said the default was 24
hours.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
atch that implements some of the
above-suggested changes.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 2a99c1f..71c6ae2 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@
equences and handle out-of-sync sequences./on how
to identify and handle out-of-sync sequences./
==
Kind Regards,
Peter Smith.
Fujitsu Australia
er/
~~~
FetchRelationStates:
23.
- * Note: If this function started the transaction (indicated by the parameter)
- * then it is the caller's responsibility to commit it.
+ * Returns true if subscription has 1 or more tables, else false.
*/
bool
-FetchRelationStates(bool *started_tx)
+FetchRelationStates(void)
Partly because of the name (relations), I felt this might be better to
be a void function and the returned value would be passed back by
references (bool *has_tables).
==
src/backend/replication/logical/worker.c
SetupApplyOrSyncWorker:
24.
+
+ if (am_sequencesync_worker())
+ before_shmem_exit(logicalrep_seqsyncworker_failuretime, (Datum) 0);
There should be a comment saying what this callback is for.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
is another place where the function name is "relations", but the
function comment refers to "tables".
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ot be impacting the "All
Sequences" flag value, so the expected value of 't' for "All
sequences" looks wrong to me. I think this might be a manifestation of
that duplicated '9' index mentioned in an earlier review comment #6.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
use the same wording as the documentation.
/has to/must/
==
(The attached NITPICKS patch includes the above suggestions)
==
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index 8b6c34a..aff2c1a 100644
--- a/src/
On Wed, Dec 18, 2024 at 2:50 AM James Coleman wrote:
>
> On Mon, Dec 16, 2024 at 6:17 PM Peter Smith wrote:
> >
> > While revisiting some old threads, I found this one that seemed to
> > reach a conclusion, but then it seemed nothing happened.
> >
> > Aft
whole thing abandoned for some reason?
==
[1] https://www.postgresql.org/docs/devel/logical-replication-publication.html
[2]
https://www.postgresql.org/message-id/CAAaqYe91iO3dfUnVmBs4M-4aUX_zHmPN72ELE7c_8qAO_toPmA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, Dec 16, 2024 at 9:40 PM Nisha Moond wrote:
>
> On Mon, Dec 16, 2024 at 9:58 AM Peter Smith wrote:
> >
...
> > SUGGESTIONS:
> >
> > Docs (idle_replication_slot_timeout): Invalidate replication slots
> > that have remained idle longer than this duration.
e longer than the configured \"%s\" duration.
Msg (errdetail): The slot has remained idle since %s, which is longer
than the configured \"%s\" duration.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Dec 13, 2024 at 8:31 PM vignesh C wrote:
>
> On Fri, 13 Dec 2024 at 10:58, Peter Smith wrote:
> >
> > Hi,
> >
> > While reviewing the pg_createsubscriber [1] docs I found a potentially
> > wrong linkend.
> >
> > This sentence:
> > &
-uLKZWaa6to9x1DurmyJh8Jmu1_1P7hp4Q%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
#x27; states here. (we don't strictly need to wait for the
transition from 'p' to 'e' to occur).
So, SQL like the one below might be the best:
# Verify that all subtwophase states are pending or enabled,
# e.g. there are no subscriptions where subtwophase is disabled ('d').
SELECT count(1) = 0 FROM pg_subscription WHERE subtwophasestate IN ('d')
==
Kind Regards,
Peter Smith.
Fujitsu Australia
tps://www.postgresql.org/docs/current/logical-replication-row-filter.html#LOGICAL-REPLICATION-ROW-FILTER-INITIAL-DATA-SYNC
[3]
https://github.com/postgres/postgres/commit/84db9a0eb10dd1dbee6db509c0e427fa237177dc
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-DOCS-Fix-wrong-linkend-on-pg_createsubscriber-pag.patch
Description: Binary data
app-pgresetwal.html
[8] pg_rewind - https://www.postgresql.org/docs/devel/app-pgrewind.html
[9] pg_test_fsync - https://www.postgresql.org/docs/devel/pgtestfsync.html
[10] pg_test_timing - https://www.postgresql.org/docs/devel/pgtesttiming.html
[11] pg_upgrade - https://www.postgresql.org/docs/devel/p
e them in for consistency.
> >
> > I'd rather get rid of those and just use the long options.
>
> Yeah, that is more self-documenting, so I'll do that while I'm at it.
>
> --
Your fat-comma solution is much better than something I could have
come up with. Thanks for taking this on.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
name char-codes (like 'p') in comments, it
might be helpful to include the equivalent words, saving readers from
having to search the documentation to find the meaning.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
1 - 100 of 1390 matches
Mail list logo
