On Tue, Jun 10, 2025 at 6:02 PM Peter Smith wrote:
>
> Hi Timur.
>
> On Wed, Jun 4, 2025 at 11:47 PM Timur Magomedov
> wrote:
> >
> > Hello Peter,
> >
> > I've tried running VCI, the idea looks great to me. Thank you and
> > Iwata-san for working
ous other slightly reworded --clean-something variants were
> > tossed around.
> >
> > Peter Smith raised the concern that "clean" is not an established term
> > and it should by something based on "drop" instead.
> >
> > And then later, this was no
for columns security_pin:
+
+CREATE PUBLICATION users_safe FOR TABLE users EXCEPT (security_pin);
+
+
8a.
Same review comment as previously -- Those tags don't seem correct.
e.g. "users" and "security_pin" are not (???).
Again, are all the other existing tags also
es/commit/626df47ad9db809dc8f93330175ab95b75914721
Kind Regards,
Peter Smith.
Fujitsu Australia
(see
> discussion [2]).
> Can we use leave TupleHashEntryData as is and make new VCI-specific
> struct that contains TupleHashEntryData member and an additional
> pointer or make VCI use TupleHashEntryGetAdditional()?
>
Thanks for the report. This is fixed in v6.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ion will resume in the next cycle. However, if no
> consumer is configured, it is advisable to manually advance the slot
> on the primary using pg_logical_slot_get_changes or
> pg_logical_slot_get_binary_changes, allowing synchronization to
> proceed.
>
> Let me know what you think of above?
>
Phrases like "... it is recommended..." and "... intended for testing
and debugging .. " and "... should be used with caution." and "... it
is advisable to..." seem like indicators that parts of the above
description should be using SGML markup such as or
or instead of just plain text.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
the name of the
subscription option; it might also be the name of an internal
structure member or catalog column name. Furthermore, when you are
referring to the option name I think it ought to be double-quoted for
clarity.
But, unless you are referring specifically to the option/field/column
then IMO it should say two-phase (with the dash).
e.g. The "two_phase" option enables two-phase mode.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Tomas.
Attached is the first version of the README, intended to address the
points above.
For convenience, I’ve included it as a separate file, but the plan is
to integrate it into the 0002 patch in the next update.
Please let me know if you have any feedback or suggestions for
improving the content.
==
Kind Regards,
Peter Smith.
Fujitsu Australia.
README-20250610
Description: Binary data
s versions are next posted.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ostgreSQL. These
VCI patches originated from an older forked source, so it seems this
reversion was inadvertently introduced during the rebasing process.
We’ll aim to correct this in a future patch.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi,
There seems to be some unexpected ">" here:
"E.1.3.7.3. Logical Replication Applications>"
==
Kind Regards,
Peter Smith.
Fujitsu Australia
tion
"sub1" - batch #1 = 100 attempted, 97 succeeded, 3 mismatched, 150
remaining
LOG: Logical replication sequence synchronization for subscription
"sub1" - batch #2 = 100 attempted, 95 succeeded, 5 mismatched, 50
remaining
LOG: Logical replication sequence synchronization for subsc
ECT. Since it fixes
nothing I assumed you just commented the SELECT prompts for
consistency with the other ones, right?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
le =~ qr/Filtering change for relation "insert_only_table"/,
+ 'delete for relation insert_only_table is filtered');
==
Kind Regards,
Peter Smith.
Fujitsu Australia
" multiple times.
SUGGESTION
"Logical replication sequence synchronization for subscription \"%s\"
- total unsynchronized: %d; batch #%d = %d attempted, %d succeeded, %d
mismatched"
==
Kind Regards,
Peter Smith.
Fujitsu Australia
eeded to say "of course" here.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, Apr 28, 2025 at 7:51 PM Amit Kapila wrote:
>
> On Mon, Apr 28, 2025 at 8:08 AM Peter Smith wrote:
> >
> > On Wed, Apr 23, 2025 at 10:33 AM David G. Johnston
> > wrote:
> > ...
> > > In the column list I would stick to mentioning what cannot be spec
stand why it was put there in the first place.
Anyway, PSA patch v2.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v2-0001-DOCS-create-publication-for-table.patch
Description: Binary data
. e.g. what's the point
of gathering publisher sequence lists and setting INIT states for
them, etc, when it won't synchronise anything because copy_data=false?
Everything will be synchronised later anyway when the user does
REFRESH PUBLICATION SEQUENCES.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
:
trailing whitespace.
*
warning: 1 line adds whitespace errors.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ilure time'.
Those details belong at a lower level -- here, it is better to be more
generic.
SUGGESTION:
/* This is a clean exit, so no need for any sequence failure logic. */
==
Kind Regards,
Peter Smith.
Fujitsu Australia
d functions do.
~
4b.
I felt that all the SyncXXX functions exposed from syncutils.c should
be grouped together, and maybe even with a comment like /* from
syncutils.c */
==
Kind Regards,
Peter Smith.
Fujitsu Australia
theses.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi Ajin.
Here are some v17-0003 review comments (aka some v16-0003 comments
that were not yet addressed or rejected)
On Fri, Apr 11, 2025 at 4:05 PM Peter Smith wrote:
...
> ==
> Commit message
>
> 2. missing commit message
Not yet addressed.
> ~~~
>
> 8.
> # In
eady be assigned in both the *if* and the *else* blocks. So, why is
it being overwritten again outside that if/else?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
uded in
this case only if publish_generated_columns is set to stored.
I've attached a patch to implement this suggested wording.
Thoughts?
==
[1]
https://www.postgresql.org/docs/devel/sql-createpublication.html#SQL-CREATEPUBLICATION-PARAMS-FOR-TABLE
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-00
->try_to_filter_change = entry->filterable;
+ return entry->filterable;
+ }
+
Bad indentation.
==
src/include/replication/reorderbuffer.h
3.
+extern bool ReorderBufferCanFilterChanges(ReorderBuffer *rb);
+
Why is this extern here? This function is not implemented in patch 0001.
==
Kind Regar
cussed (earlier in this thread)
changes that are not in this v3* patch set. They haven't been
forgotten; I'll revisit those later after the above (simpler) changes
are dealt with.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v3-0004-DOCS-pg_waldump.-Some-option-tags-are-wrong-s
ight, I have changed that.
PS REPLY 17/4
Yes, the error message, but also I thought 'tblinfo' var and
FindTableByOid function name should refer to relations instead of
tables?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
fferences between the publisher and the subscriber,
+ which might occur when copy_data = true.
+
ditto previous review comment #6.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
copy_data)
12.
+
+ See
+ for recommendations on how to handle any warnings about
differences in
+ the sequence definition between the publisher and the subscriber,
+ which might occur when copy_data = true.
+
Ditto my previous review comment #10.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
~~
29.
+# Check - newly published sequence values are not updated
+$result = $node_subscriber->safe_psql(
+ 'postgres', qq(
+ SELECT last_value, log_cnt, is_called FROM regress_s4;
+));
+is($result, '1|0|f',
+ 'REFRESH PUBLICATION will sync newly published sequence');
+
(This is the copy_data=false test)
29a.
Maybe it is good here also to show the sequence value at the publisher
to see if it is different.
~
29b.
The message 'REFRESH PUBLICATION will sync newly published sequence'
seems wrong because the values are NOT synced when copy_data=false
==
Kind Regards,
Peter Smith.
Fujitsu Australia
)
{
elm->last = next; /* last returned number */
elm->last_valid = true;
error: patch failed: src/backend/commands/sequence.c:994
error: src/backend/commands/sequence.c: patch does not apply
==
Kind Regards,
Peter Smith.
Fujitsu Australia
#x27;t this same code already done a few lines above in the same
function? Maybe I misread something.
======
src/test/regress/sql/publication.sql
5.
+-- check that describe sequence lists all publications the sequence belongs to
Might be clearer to say: "lists both" instead of "lists all"
==
Kind Regards,
Peter Smith.
Fujitsu Australia
32 | t
+(1 row)
+
I think 32 may seem like a surprising value to anybody reading these
results. Perhaps it will help if there can be a comment for this .sql
test to explain why this is the expected value.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
8b.
Change the comment or rearrange the tests so they are in the same
order as described
~
8c.
Looking at the expected logs I wondered if it might be nicer for the
pgoutput_filter_change doing this logging to also emit the relation
name.
~~~
9.
+$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub_all");
+$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub_insert_only");
+$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub_delete_only");
+
9a.
You could combine all those DDL
~
9b.
Add some simple comment like "# cleanup"
~
9c.
Any reason why you dropped the subscriptions but not the publications?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
d be able
to filter INSERTS but not filter DELETES even for the same relation.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
r names might be:
entry->filterable
rb->try_to_filter_change
Also these explanations/distinctions need to be made more clearly in
the commit message and/of file head comments, as well as where those
members are defined.
==
Kind Regards,
Peter Smith.
Fujitsu Australia.
==
[1] https://www.postgresql.org/docs/current/logical-replication-col-lists.html
[2]
https://www.postgresql.org/message-id/flat/CAH2L28vddB_NFdRVpuyRBJEBWjz4BSyTB%3D_ektNRH8NJ1jf95g%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
Relation() to see list of relations that are not
+ * decoded by the reorderbuffer.
/to see list of relations/to see the kinds of relation/
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Apr 9, 2025 at 8:08 PM Amit Kapila wrote:
>
> On Wed, Apr 9, 2025 at 7:48 AM Peter Smith wrote:
> >
> > I was looking at the recent push for the pg_createsubscription "--all"
> > option [1], because I was absent for several weeks prior.
> >
> &g
gt; it, but the main patch can be committed even without this.
>
I've created a separate thread [1] to continue the discussion about
the synopsis.
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPueY_DyuBy6SuvJev2DWJVGtg%3D9WG9WXvYQDJu39gV6TQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
till works)
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPuyBsOJTSygus2-yp60sw_phwYQ-JyC%2BU6fCBMos9x2LA%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CAHv8RjKU24jCHR2fOHocmdSTqhu7ige5UQsUQMkaTZniLc9DbA%40mail.gmail.com
[3]
https://github.com/postgres/postgres/commit/f
s part of the --all description to the
Note/Prerequisites section. (see #1c)
0003. Fixes to code comments. (see #2)
==
[1]
https://github.com/postgres/postgres/commit/fb2ea12f42b9453853be043b8ed107e136e1ccb7
[2] https://www.postgresql.org/docs/current/app-vacuumdb.html
Kind Regards,
Pete
needs to be balanced
by a call to the other function: parallel_vacuum_end_work_phase.
~~~
12.
+static void
+parallel_vacuum_end_worke_phase(ParallelVacuumState *pvs)
Typo (worke)
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPs9yTGk2TWdjgLQEzGfPTWafKT0H-Q6WvYh5UKNW0pCvQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
05 PM Peter Smith wrote:
>
> ==
> src/backend/access/table/tableamapi.c
>
> 2.
> Assert(routine->relation_vacuum != NULL);
> + Assert(routine->parallel_vacuum_compute_workers != NULL);
> Assert(routine->scan_analyze_next_block != NULL);
>
> Is it better
dry-run"
~~~
Please find attached a small patch that implements all these changes.
==
[1]
https://www.postgresql.org/message-id/CAKFQuwbaYnSBc5fgHsoFLW_cUq2u466-3ZpkA%2Bu1Z%3D-medNgwg%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-improvements-for-remove-option-documentation.patch
Description: Binary data
On Thu, Apr 3, 2025 at 10:26 AM Andres Freund wrote:
>
> Hi,
>
> On 2025-04-03 09:58:30 +1100, Peter Smith wrote:
> > I saw that a new errhint_internal() function was recently committed
> > [1]. I had also posted above asking about this same missing function a
> > mo
lict type split into two, something like
'insert_multiple_conflicts' and 'update_multiple_conflicts'?
==
[1]
https://github.com/postgres/postgres/commit/73eba5004a06a744b6b8570e42432b9e9f75997b
Kind Regards,
Peter Smith.
Fujitsu Australia
gresql.org/message-id/CAHut%2BPtDHRif49G%2BbzspOGspETym5oKseD13v0tcBJXWUrTx9A%40mail.gmail.com
[3] https://www.postgresql.org/message-id/547688.1738630459%40sss.pgh.pa.us
Kind Regards,
Peter Smith.
Fujitsu Australia
Thanks for pushing.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, Mar 17, 2025 at 1:50 PM Robert Haas wrote:
>
> On Sun, Mar 16, 2025 at 7:38 PM Peter Smith wrote:
> > If I understand correctly, the summary is:
> > - Tom: +1 for "TOAST table", but changing all the combined forms is
> > maybe not worth the effort.
>
ould be the
best choice in some cases but probably there are many dozens of
candidates so getting consensus on all of those rewrites will be like
herding cats.
Meanwhile, I've moved this CF entry into the next commitfest, because
I don't see how this thread can get resolved by the end of the month.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
Thanks for pushing it.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
on('\x5678'::bytea in '\x1234567890'::bytea) → 3
==
[1] https://www.postgresql.org/docs/17/functions-binarystring.html
Kind Regards,
Peter Smith.
Fujitsu Australia
On Sat, Mar 15, 2025 at 4:52 PM Peter Smith wrote:
>
> On Fri, Mar 14, 2025 at 5:39 PM David G. Johnston
> wrote:
> >
> > On Tuesday, March 11, 2025, Peter Smith wrote:
> >>
> >>
> >> Choice 3. Implement some option that has an argument saying w
On Fri, Mar 14, 2025 at 5:39 PM David G. Johnston
wrote:
>
> On Tuesday, March 11, 2025, Peter Smith wrote:
>>
>>
>> Choice 3. Implement some option that has an argument saying what to delete
>>
>> Implement an option that takes some argument saying wha
time permits I'll try to help you get some of the ("needs review")
threads moving by checking the simple ones first.
Kind Regards,
Peter Smith.
On Wed, Mar 12, 2025 at 3:18 AM David G. Johnston
wrote:
>
> On Thu, Dec 12, 2024 at 8:25 PM Peter Smith wrote:
>>
>> [1] initdb [option...] [ --pgdata | -D ] directory
>> [2] pg_archivecleanup [option...] archivelocation oldestkeptwalfile
>> [3] pg_checksum
2025 at 8:37 PM Nisha Moond wrote:
>
> On Mon, Feb 24, 2025 at 7:39 AM Peter Smith wrote:
> >
> > Hi Nisha.
> >
> > Some review comments for patch v1-0001
> >
> > ==
> > GENERAL
> >
> > 1.
> > This may be a
On Tue, Mar 11, 2025 at 11:32 AM David G. Johnston
wrote:
>
> On Mon, Mar 10, 2025 at 5:00 PM Peter Smith wrote:
>>
>> Hi Shubham.
>>
>> Some review comments for patch v16-0001.
>>
>> ==
>> doc/src/sgml/ref/pg_createsubscriber.sgml
>&
ot specified, a generated name is assigned
to the subscription name.
SUGGEST: If this --subscription is not specified, a generated name is
assigned to the subscription name.
==
[1]
https://www.postgresql.org/message-id/flat/CAHv8RjKhA%3D_h5vAbozzJ1Opnv%3DKXYQHQ-fJyaMfqfRqPpnC2bA%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/flat/CAHut%2BPv%2B96CykSY6-uLKZWaa6to9x1DurmyJh8Jmu1_1P7hp4Q%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
laces, e.g. function cleanup_objects_atexit, this caused
unnecessary code bloat. IMO this function is crying out for a local
variable to simplify the code again.
Please see the attached patch that implements this suggestion.
==
Kind Regards,
Peter Smith
Fujitsu Australia
v1-0001-Add-cleanup_objects_at
e new feature so please separate
them into a different prerequisite patch so we can just focus on what
changes were made just for --drop-all-publications. I know you already
said you are working on it [1-#7], but multiple patch versions have
been posted since you said that.
==
[1] v13 review
https://www.postgresql.org/message-id/CAHv8RjKmf5bAcgTmGToWSn_Fx%2BO2y8qCiLScBXvBTD0D5gX2sw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
frozen_pages = 0;
+ scan_data->rel_pages = orig_rel_pages = RelationGetNumberOfBlocks(rel);
+ vacrel->scan_data = scan_data;
/* dead_items_alloc allocates vacrel->dead_items later on */
That comment about dead_items_alloc seems misplaced now because it is
grouped with all the scan_data stuff.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
nal parallel vacuum callbacks need
to exist, or none.
(this same issue is repeated in multiple places).
==
[1] https://www.postgresql.org/docs/devel/sql-vacuum.html
Kind Regards,
Peter Smith.
Fujitsu Australia
void function, but introduce another parameter
'nworkers_index_p', similar to 'nworkers_table_p'?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ared->work_item == PV_WORK_ITEM_COLLECT_DEAD_ITEMS)"
~~~
12.
+ if (pvs.shared->work_item == PV_WORK_ITEM_COLLECT_DEAD_ITEMS)
+ {
+ /* Scan the table to collect dead items */
+ parallel_vacuum_process_table(&pvs, state);
+ }
+ else
+ {
+ Assert(pvs.shared->work_item == PV_WORK_IT
==
Kind Regards,
Peter Smith.
Fujitsu Australia
on.
~~~
6.
+/*
+ * Perform a parallel vacuums scan to collect dead items.
+ */
6a.
"Perform" or "Execute"?
The comment should match the one in the fwd declaration of this function.
6b.
Typo "vacuums"
==
Kind Regards,
Peter Smith.
Fujitsu Australia
gdata' => $node_s1->data_dir,
'--publisher-server' => $node_p->connstr($db1),
- '--socketdir' => $node_s->host,
- '--subscriber-port' => $node_s->port,
+ '--socketdir' => $node_s1->host,
+ '--subscriber-port' => $node_s1->port,
'--publication' => 'pub1',
'--publication' => 'pub2',
'--replication-slot' => 'replslot1',
'--replication-slot' => 'replslot2',
'--database' => $db1,
'--database' => $db2,
- '--enable-two-phase'
+ '--enable-two-phase',
+ '--cleanup-existing-publications',
],
'run pg_createsubscriber on node S');
Comment and Message should refer to S1, not S.
~~~
15.
+# Create user-defined publications
+$node_p->safe_psql($db3, "CREATE PUBLICATION test_pub3 FOR ALL TABLES;");
+$node_p->safe_psql($db3, "CREATE PUBLICATION test_pub4 FOR ALL TABLES;");
Probably these can be combined.
~~~
16.
+# Drop the newly created publications
+$node_p->safe_psql($db3, "DROP PUBLICATION IF EXISTS test_pub3;");
+$node_p->safe_psql($db3, "DROP PUBLICATION IF EXISTS test_pub4;");
+
Probably these can be combined.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ltiple_unique_conflicts detected during update');
(same as #12)
Won't it be more useful to also log the column name causing the
conflict? Otherwise, if there are 100s of unique columns and just 2 of
them are conflicts how will the user know what to look for when fixing
it?
==
Kind Regards,
Peter Smith.
Fujitsu Australia
you also add different tests where you do operations on a table
that IS partially replicated but only some of the *operations* are not
published. e.g. test the different 'pubactions' of the PUBLICATION
'publish' parameter. Maybe you need different log checks to
distinguish the different causes for the filtering.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
ereport(ERROR,
~
4b.
Unnecessary parentheses in the ereport.
~
4c.
There seems some weird mix of tense "cannot copy" versus "could not
copy" already in this file. But, maybe at least you can be consistent
within the patch and always say "cannot".
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Feb 13, 2025 at 12:58 AM Dagfinn Ilmari Mannsåker
wrote:
>
> Ashutosh Bapat writes:
>
> > On Wed, Feb 12, 2025 at 10:36 AM Peter Smith wrote:
> >>
> >> Hi hackers,
> >>
> >> Recently, while writing some new TAP tests my colleague inadv
21B3390856464C5E27FF5C42%40OSCPR01MB14966.jpnprd01.prod.outlook.com
[2]
https://www.postgresql.org/message-id/CAHut%2BPtrLu%3DYrxo_YQ-LC%2BLSOEUYmuFo2brjCQ18JM9-Vi2DwQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Feb 20, 2025 at 5:44 PM Shubham Khanna
wrote:
>
> On Mon, Feb 17, 2025 at 9:49 AM Peter Smith wrote:
> >
...
> > ==
> > 1 GENERAL. Option Name?
> >
> > Wondering why the patch is introducing more terminology like
> > "cleanup"; if
t; + --all-dbs
> ```
>
> Peter's comment [2] does not say that option name should be changed.
> The scope of his comment is only in the .c file.
Yes, that's correct. My v9 suggestion to change 'all' to 'all_dbs' was
referring only to the field name of struct CreateSubscriberOptions,
nothing else. Not the usage help, not the error messages, not the
docs, not the tests, not the commit message.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
>safe_psql($db1, "INSERT INTO tbl1 VALUES ('replication test');");
+
+$result = $node_s1->safe_psql($db1, "SELECT * FROM tbl1 ORDER BY 1");
+is( $result, qq(first row
+replication test
+second row), "replication successful in $db1");
+
+$node_s1->stop;
# Run pg_createsubscriber on node S. --verbose is used twice
# to show more information.
command_ok(
@@ -431,8 +590,9 @@ $result = $node_s->safe_psql($db1,
is($result, qq(0), 'failover slot was removed');
# Check result in database $db1
-$result = $node_s->safe_psql($db1, 'SELECT * FROM tbl1');
+$result = $node_s->safe_psql($db1, 'SELECT * FROM tbl1 ORDER BY 1');
is( $result, qq(first row
+replication test
I concur with Kuroda-san's comment. Maybe remove all this, because
AFAICT existing code was already testing that replication is working
ok for $db1 and $db2.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
sables logging parameter values."
OR
"-1 disables logging parameter values. 0 means log parameter values in full"
TBH, I am unsure if this one should even mention the value 0 because
there are already examples of "max" GUCs similar to this that say
nothing about 0 since the meaning should be clear.
e.g. log_parameter_max_length doesn't mention 0.
e.g. log_parameter_max_length_on_error doesn't mention 0.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
the commit message
- docs
- test code
==
src/test/recovery/t/035_standby_logical_decoding.pl
3.
+# Attempting to copy an invalidated slot
+($result, $stdout, $stderr) = $node_standby->psql(
/Attempting/Attempt/
==
[1] https://www.postgresql.org/docs/current/functions-admin.htm
");
+$node_p->safe_psql($db1, "CREATE PUBLICATION test_pub2 FOR ALL TABLES;");
+
/it's/their/
~~~
8.
Maybe also do a CREATE PUBLICATION at node_s, prior to the
pg_createsubvscript, so then you can verify that the user-created one
is unaffected by the cleanup of all the others.
==
[1]
https://www.postgresql.org/message-id/CAExHW5t4ew7ZrgcDdTv7YmuG7LVQT1ZaEny_EvtngHtEBNyjcQ%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi Ajin.
FYI - Patch set v13* no longer applies cleanly. Needs rebasing.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
I expect a future patch
to do this might be rejected as being too trivial, so by not changing
now probably these functions are doomed to be inconsistent forever.
Anyway it is just my opinion -- leave it as-is if you wish.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
>
OK. My blank lines patch has been abandoned. Here is just the
'inactive_since' description patch (now called 0001).
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v3-0001-DOCS-improve-pg_replication_slots.inactive_since-.patch
Description: Binary data
filtering */
+ int8 processed_changes;
Maybe 'unfiltered_changes_count' is a better name for this field?
~~~
10.
+extern bool ReorderBufferCanFilterChanges(ReorderBuffer *rb);
Should match the 'can_filter_change' field name, so if you change that
(see comment #8), then change this too.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Feb 14, 2025 at 8:35 AM Nathan Bossart wrote:
>
> On Thu, Feb 13, 2025 at 03:59:45PM +1100, Peter Smith wrote:
> > I noticed the pg_sequences system-view DOCS page [1] has a note about
> > the 'last_value' field. But the note is not within the row for that
l to NULL
- changed true to true
- changed false to false
//
Patch 0002
Modified the text as suggested.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v2-0001-DOCS-pg_replication_slots.-Add-blank-lines.patch
Description: Binary data
v2-0002-DOCS-pg_replication_slots.-Improve-the-active_sin.patch
Description: Binary data
y attached patch just moved the paragraph into the row where I felt
it belonged.
==
[1] https://www.postgresql.org/docs/devel/view-pg-sequences.html
[2]
https://github.com/postgres/postgres/commit/3cb2f13ac500983c9c6b1eef3b3c2091c26f3040
Kind Regards,
Peter Smith.
Fujitsu Australia
v1
make
sense to a user? How are we "converting" the source server databases;
AFAIK we are simply connecting to them and adding publications.
IMO a better choice of adjectives can be found below.
"no suitable databases found..."
"no appropriate databases found..."
text_noop("-1 disables logging statement durations. 0 means log
all statement durations."),
+ gettext_noop("-1 disables logging autovacuum actions. 0 means log
all autovacuum actions."),
==
Kind Regards,
Peter Smith.
Fujitsu Australia
| RBTXN_SENT_PREPARE)
+
AFAICT bitmasks like this are more commonly named with a _MASK suffix.
How about something like:
- RBTXN_PREPARE_MASK
- RBTXN_PREPARE_STATUS_MASK
- RBTXN_PREPARE_FLAGS_MASK
==
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, Feb 12, 2025 at 10:58 AM Peter Smith wrote:
>
> On Tue, Feb 11, 2025 at 9:16 PM Shubham Khanna
> wrote:
> >
> > > #13. Unanswered question "How are tests expecting this even passing?".
> > > Was a reason identified? IOW, how can we be sure
command_fails(). It seems more foolproof.
Thoughts?
(make check-world passes).
==
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Help-prevent-users-from-calling-the-wrong-functio.patch
Description: Binary data
e immediately raised so I expect the
execution overhead to be ~zero.
BTW, your bad option combination tests are only using --all-databases
*after* the other options. Maybe you should mix it up a bit, sometimes
putting it *before* the others as well, because rearranging will cause
different errors.
Everything else now looks good to me.
==
Kind Regards,
Peter Smith.
Fujitsu Australia
I don't have an opinion about the ssl_crl stuff. Everything else looks
good to me.
==
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Wed, Feb 12, 2025 at 12:36 AM Nisha Moond wrote:
>
> On Tue, Feb 11, 2025 at 8:49 AM Peter Smith wrote:
> >
> > Hi Nisha.
> >
> > Some review comments about v74-0001
> >
> > ==
> > src/backend/replication/slot.c
> >
> > 1.
&
~
6b.
Why is the field 'cause' declared as int instead of
ReplicationSlotInvalidationCause?
==
Please the attached top-up patch as a code example of some of my
suggestions above -- in particular the relocating of
RS_INVAL_MAX_CAUSES and the typedef, and the reinstating of the static
quot; (??). Maybe just "--all-databases
option was specified"
==
[1]
https://www.postgresql.org/message-id/CAHut%2BPuYmTyi6kPFnJDTvD%3DaLHd0kyX4VG6iDQVEk-ixov1AwA%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CAHut%2BPuyBsOJTSygus2-yp60sw_phwYQ-JyC%2BU6fCBMos9x2LA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Feb 11, 2025 at 9:25 AM Nathan Bossart wrote:
>
> On Tue, Feb 11, 2025 at 08:29:28AM +1100, Peter Smith wrote:
> > +1 for this. Your wording examples below look good to me..
>
> Okay, how does this look?
>
> --
v2 mostly looked good to me. Just a coup
1 - 100 of 1515 matches
Mail list logo
