mp;buf,
- "SELECT pubname\n"
+ "SELECT pubname, NULL\n"
"FROM pg_catalog.pg_publication p\n"
"JOIN pg_catalog.pg_publication_rel pr ON p.oid = pr.prpubid\n"
"WHERE pr.prrelid = '%s'\n"
"UNION ALL\n"
- "SELECT pubname\n"
+ "SELECT pubname, NULL\n"
"FROM pg_catalog.pg_publication p\n"
I thought it may be better to reformat to put the NULL columns on a
different line for consistent format with the other SQL just above
this one. e.g.
printfPQExpBuffer(&buf,
"SELECT pubname\n"
+ " , NULL\n"
...
--
[1]
https://www.postgresql.org/message-id/CAA4eK1LApUf%3DagS86KMstoosEBD74GD6%2BPPYGF419kwLw6fvrw%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CAA4eK1KDtwUcuFHOJ4zCCTEY4%2B_-X3fKTjn%3DkyaZwBeeqRF-oA%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
ic name (e.g. repl_common.h? ...) since the
stuff I wanted to put there was not really "slot" related.
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
lse
-- Here the partition does not have a row filter
-- Col "a" is in replica identity.
ALTER PUBLICATION testpub6 SET (PUBLISH_VIA_PARTITION_ROOT=0);
UPDATE rf_tbl_abcd_part_pk SET a = 1;
-- fail - PUBLISH_VIA_PARTITION_ROOT is true
-- Here the root filter will be used, but the "b
ge.postgresql.org/src/backend/utils/cache/relcache.c.gcov.html
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Remove-unreachable-code-in-GetRelationPublication.patch
Description: Binary data
n->rd_pubdesc is NULL at this point?
> >
> > (if it was not-null the function would have returned immediately from the
> > top)
>
> I think it might be better to change this as a separate patch.
OK. I have made a separate thread [1[ for discussing this one.
--
[1]
h
On Thu, Feb 10, 2022 at 11:34 AM Tom Lane wrote:
>
> Peter Smith writes:
> > There appears to be some unreachable code in the relcache function
> > GetRelationPublicationActions.
> > When the 'relation->rd_pubactions' is not NULL then the function
> >
re review comments.
This Row Filter patch v85 LGTM.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
after releasing the
+ * shared memory so that the leader can re-use the fileset for next
+ * streaming transaction.
+ */
+ bool fileset_valid;
+ FileSet fileset;
The comment here seems to need some more work because it is saying
more about what it *isn't*, rather than what it *is*.
Something like
On Mon, Nov 7, 2022 at 5:50 AM Tom Lane wrote:
>
> Peter Smith writes:
> > Sorry, I forgot the attachments in the previous post. PSA.
>
> I spent a bit of time looking at this. I agree that a lot of the
> current ordering choices here look like they were made with the
&g
of "cache lookup failed" and "cache lookup failure"
-- should all be the same.
~
G.8c.
A few above (e.g. role) have a different message text. Shouldn't those
also be "cache lookup failed"?
~~~
G.9 GENERAL - ObjTree variables
Often the ObjTree variable (for the deparse_XXX function return) is
given the name of the statement it is creating.
Although it is good to be descriptive, often there is no need for long
variable names (e.g. 'createTransform' etc), because there is no
ambiguity anyway and it just makes for extra code characters and
unnecessary wrapping. IMO it would be better to just call everything
some short but *consistent* name across every function -- like 'stmt'
or 'json_ddl' or 'root' or 'ret' ... or whatever.
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Fri, Nov 11, 2022 at 3:47 PM Peter Smith wrote:
>
> Here are more review comments for the v32-0001 file ddl_deparse.c
>
> *** NOTE - my review post became too big, so I split it into smaller parts.
THIS IS PART 2 OF 4.
===
src/backend/commands/ddl_
On Fri, Nov 11, 2022 at 4:09 PM Peter Smith wrote:
>
> On Fri, Nov 11, 2022 at 3:47 PM Peter Smith wrote:
> >
> > Here are more review comments for the v32-0001 file ddl_deparse.c
> >
> > *** NOTE - my review post became too big, so I split it into smaller
On Fri, Nov 11, 2022 at 4:17 PM Peter Smith wrote:
>
> On Fri, Nov 11, 2022 at 4:09 PM Peter Smith wrote:
> >
> > On Fri, Nov 11, 2022 at 3:47 PM Peter Smith wrote:
> > >
> > > Here are more review comments for the v32-0001 file ddl_deparse.c
> > >
&
*prefix;
+ Size message_size;
+ char*message;
+ Oid relid;
+ DeparsedCommandType cmdtype;
+ } ddlmsg;
+
Why not call it ddl? -- see general review comment
==
src/test/regress/expected/psql.out
56.
\dRp "no.such.publication"
- List of publications
- Name | Owner | All tables | Inserts | Updates | Deletes | Truncates | Via root
---+---++-+-+-+---+--
+ List of publications
+ Name | Owner | All tables | Inserts | Updates | Deletes | Truncates
| Via root | DDLs
+--+---++-+-+-+---+--+--
(0 rows)
I wondered if "DDLs" belongs adjacent to the
Inserts/Updates/Deletes/Trucates because those are the other "publish"
parameters just like this.
==
src/test/regress/expected/publication.out
57.
(Ditto comment for psql.out)
I wondered if "DDLs" belongs adjacent to the
Inserts/Updates/Deletes/Trucates because those are the other "publish"
parameters just like this.
~~~
58.
Looks like there is a missing regress test case where you actually set
the publish='ddl' and then verify that the DDLs column is correctly
set 't'?
==
59. MISC = typedefs.list
There are missing some typedefs.list changes for this patch. At least
the following:
e.g.
- DeparsedCommandType (from ddlmessage.h)
- xl_logical_ddl_message (from ddlmessage.h)
- LogicalDecodeDDLMessageCB (from output_plugin.h)
- LogicalDecodeStreamDDLMessageCB (from output_plugin.h)
- ReorderBufferDDLMessageCB (from reorderbuffer.h)
- ReorderBufferStreamDDLMessageCB (from reorderbuffer.h)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Sun, Nov 13, 2022 at 11:47 AM vignesh C wrote:
>
> On Mon, 24 Oct 2022 at 13:15, Peter Smith wrote:
> >
> > Hi hackers.
> >
> > There is a docs Logical Replication section "31.10 Configuration
> > Settings" [1] which describes some logical
_stat_gssapi
28.2.12. pg_stat_archiver
28.2.13. pg_stat_bgwriter
28.2.14. pg_stat_wal
28.2.15. pg_stat_database
28.2.16. pg_stat_database_conflicts
28.2.23. pg_stat_slru
28.2.5. pg_stat_replication_slots
28.2.17. pg_stat_all_tables
28.2.18. pg_stat_all_indexes
28.2.19. pg_statio_all_tabl
way users would not need to change anything at all to get the
benefits of parallel streaming.
> Another variant is to have a new subscription parameter for example
> "parallel_workers" parameter that specifies the number of parallel
> workers. That way, users can specify the number of parallel workers
> per subscription.
>
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
"directly in the worker" -> "directly by the worker" (??) 2x
~~~
13. get_worker_name
+/*
+ * Return the name of the logical replication worker.
+ */
+static const char *
+get_worker_name(void)
+{
+ if (am_tablesync_worker())
+ return _("logical replication table synch
On Fri, Nov 18, 2022 at 6:03 PM Peter Smith wrote:
>
> Here are some review comments for v47-0001
>
> (This review is a WIP - I will post more comments for this patch next week)
>
Here are the rest of my comments for v47-0001
==
doc/src/sgml/monitoring.
1.
@@ -1851,6 +1851
gicalrep_worker_wakeup_ptr(LogicalRepWorker *worker);
extern int logicalrep_sync_worker_count(Oid subid);
+extern int logicalrep_parallel_apply_worker_count(Oid subid);
Would it be better to call those new functions using similar shorter
names as done elsewhere?
logicalrep_parallel_apply_worker_stop -> logicalrep_pa_worker_stop
logicalrep_parallel_apply_worker_count -> logicalrep_pa_worker_count
--
[1] Hou-san's reply to my review v47-0001.
https://www.postgresql.org/message-id/OS0PR01MB571680391393F3CB63469F3E940A9%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
same format, we could give a link to
> runtime-config-replication where data type and default is defined for
> publisher configurations max_replication_slots and max_wal_senders.
>
Thanks for your suggestions.
I have included xref links to the original definitions, rather than
defining
* FROM tab;
id | description | subscription
+-+--
1 | one | sub_tenant1
2 | two | sub_tenant1
3 | three | sub_tenant1
(3 rows)
~~
Subscriptions to different tenants would be named differently.
And using other SQL you can map/filter those names however your
application wants.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
t; be added.
>
I am not sure if there is missing functionality, but perhaps there is
some information that is harder to find than it ought to be, so I
would like to help first address that part.
--
[1] conflicts.
https://www.postgresql.org/docs/current/logical-replication-conflicts.html
[2] max_sync_workers_per_subscription.
https://www.postgresql.org/docs/current/runtime-config-replication.html
[3] srsubstate.
https://www.postgresql.org/docs/current/catalog-pg-subscription-rel.html
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, Nov 17, 2022 at 8:46 AM David G. Johnston
wrote:
>
> On Tue, Nov 15, 2022 at 6:39 PM Peter Smith wrote:
>>
>>
>> I was also wondering (but have not yet done) if the content *outside*
>> the tables should be reordered to match the table 28.1/28.2 order.
>
On Wed, Nov 23, 2022 at 9:16 AM Peter Smith wrote:
>
> On Wed, Nov 16, 2022 at 10:24 PM vignesh C wrote:
> >
> ...
>
> > One suggestion:
> > The format of subscribers includes the data type and default values,
> > the format of publishers does not include data
On Fri, Nov 25, 2022 at 2:15 AM Takamichi Osumi (Fujitsu)
wrote:
>
> On Wednesday, October 5, 2022 6:42 PM Peter Smith
> wrote:
...
>
> > ==
> >
> > 5. src/backend/commands/subscriptioncmds.c - SubOpts
> >
> > @@ -89,6 +91,7 @@ typedef struct S
IMO the comment would be better inside the if block
SUGGESTION
if (am_parallel_apply_worker())
{
/* Notify the leader apply worker that we have exited cleanly. */
pq_putmessage('X', NULL, 0);
}
--
[1] Hou-san's reply to my v49-0001 review.
https://www.postgresql.org/message-id/OS0PR01MB5716339FF7CB759E751492CB940D9%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Nov 25, 2022 at 11:09 PM Peter Eisentraut
wrote:
>
> On 23.11.22 09:36, Peter Smith wrote:
> > PSA new patches. Now there are 6 of them. If some of the earlier
> > patches are agreeable can those ones please be committed? (because I
> > think this patch may be susc
On Sat, Nov 26, 2022 at 2:43 PM David G. Johnston
wrote:
>
> On Wed, Nov 23, 2022 at 1:36 AM Peter Smith wrote:
>>
>> On Thu, Nov 17, 2022 at 8:46 AM David G. Johnston
>> wrote:
>>
>> > Also, make it so each view ends up being its own separate page.
&g
the
+ * remaining changes to disk due to timeout when sending data to the
+ * parallel apply worker.
+ */
+ bool serialize_changes;
35a.
I wonder if the comment would be better to also mention "via shared memory".
SUGGESTION
Indicates whether the leader apply worker needs to serialize the
remaining changes to disk due to timeout when attempting to send data
to the parallel apply worker via shared memory.
~
35b.
I wonder if a more informative variable name might be
serialize_remaining_changes?
--
[1]
https://stackoverflow.com/questions/17504316/what-happens-with-an-extern-inline-function
Kind Regards,
Peter Smith.
Fujitsu Australia
might be achieved using a simpler syntax than
what had been previously suggested. But in practice there may be some
problems with this approach -- e.g. how will the initial tablesync
COPY efficiently assign these subscriber name column values?
--
[1]
https://www.postgresql.org/message-id/C
references
and tidies the Chapter 31.10 "Configuration Settings" section.
Meanwhile, the Subscriber GUC descriptions are left on the
config.sgml, where you said they are supposed to be.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v5-0001-Logical-replication-GUCs-links-and-tidy.patch
Description: Binary data
On Sat, Nov 26, 2022 at 2:43 PM David G. Johnston
wrote:
>
> On Wed, Nov 23, 2022 at 1:36 AM Peter Smith wrote:
>>
>> On Thu, Nov 17, 2022 at 8:46 AM David G. Johnston
>> wrote:
>>
>> > Also, make it so each view ends up being its own separate page.
&g
-- Forwarded message -
From: Peter Smith
Date: Sat, Dec 3, 2022 at 8:03 AM
Subject: Re: Perform streaming logical transactions by background
workers and parallel apply
To: Amit Kapila
On Fri, Dec 2, 2022 at 8:57 PM Amit Kapila wrote:
>
> On Fri, Dec 2, 2022 at 2:29 PM
(Resending this because somehow my previous post did not appear in the
mail archives)
-- Forwarded message -
From: Peter Smith
Date: Fri, Dec 2, 2022 at 7:59 PM
Subject: Re: Perform streaming logical transactions by background
workers and parallel apply
To: houzj.f...@fujitsu.com
*
+ * FS_BUSY means that the leader is serializing changes to the file. FS_READY
+ * means that the leader has serialized all changes to the file and the file is
+ * ready to be read by a parallel apply worker.
+ */
+typedef enum PartialFileSetState
"ready to be read" sounded a bit strange.
SUGGESTION
... to the file so it is now OK for a parallel apply worker to read it.
--
[1] Houz reply to my review v51-0002 --
https://www.postgresql.org/message-id/OS0PR01MB5716350729D8C67AA8CE333194129%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
s checking if a delay has occurred by inspecting the
debug logs to see if a certain code path including "logical
replication apply delay" is logged. I guess that is OK, but another
way might be to compare the actual timing values of the published and
replicated rows.
The publisher table can have a column with default now() and the
subscriber side table can have an *additional* column also with
default now(). After replication, those two timestamp values can be
compared to check if the difference exceeds the min_time_delay
parameter specified.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Dec 6, 2022 at 2:51 PM Amit Kapila wrote:
>
> On Tue, Dec 6, 2022 at 5:27 AM Peter Smith wrote:
> >
> > Here are my review comments for patch v55-0002
> >
> ...
> > 4.
> >
> > /*
> > + * Replay the spooled messages in the parallel a
all separated. Putting everything into a single
'logical_replication_mode' might cause difficulties later when/if you
want combinations of the different modes.
For example, instead of
logical_replication_mode = XXX/YYY/ZZZ
maybe something like below will give more flexibility.
logical_replication_dev_XXX = true/false
logical_replication_dev_YYY = true/false
logical_replication_dev_ZZZ = true/false
--
Kind Regards,
Peter Smith.
Fujitsu Australia
/*
* Pick the largest transaction (or subtransaction) and evict it from
IIUC this logic can be simplified quite a lot just by shifting that
"bail out" condition into the loop.
Something like:
while (true)
{
if (!(force_stream && rb->size > 0 || rb->size <
logical_decoding_work_mem * 1024L))
break;
...
}
--
Kind Regards,
Peter Smith.
Fujitsu Australia
I'd like to "fix" this but IIUC there is no consensus yet about what
order is best for patch 0001, right?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Dec 6, 2022 at 5:57 AM samay sharma wrote:
>
> Hi,
>
> On Mon, Oct 24, 2022 at 12:45 AM Peter Smith wrote:
>>
>> Hi hackers.
>>
>> There is a docs Logical Replication section "31.10 Configuration
>> Settings" [1] which describes some
-4246-8d9bf855bc48%40enterprisedb.com
[2]
https://www.postgresql.org/message-id/CAKFQuwby7xWHek8%3D6UPaNXrhGA-i0B2zMOmBoGHgc4yaO8NH_w%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia.
v9-0001-Re-order-Table-28.2-Collected-Statistics-Views.patch
Description: Binary data
v9-000
n workers. Configuration
> > parameter
> > + > linkend="guc-max-worker-processes">max_worker_processes
> > +may need to be adjusted to accommodate for replication workers, at
> > least (
> > + > linkend="guc-max-logical-replication-workers">max_logical_replication_workers
> > ++ 1). Note, some extensions and parallel queries
> > also
> > +take worker slots from max_worker_processes.
> > +
>
> Maybe do max_worker_processes in a new line like the rest.
OK. Done as suggested.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
v7-0001-Logical-replication-GUCs-links-and-tidy.patch
Description: Binary data
we
> need to increase the protocol version for parallel apply feature? If
> sending the additional information is also controlled by an option
> like "streaming", we can decide what we send based on these options,
> no?
>
AFAIK the protocol version defines what protocol message bytes are
transmitted on the wire. So I thought the protocol version should
*always* be updated whenever the message format changes. In other
words, I don't think we ought to be transmitting different protocol
message formats unless it is a different protocol version.
Whether the pub/sub implementation actually needs to check that
protocol version or whether we happen to have some alternative knob we
can check doesn't change what the protocol version is supposed to
mean. And the PGDOCS [1] and [2] currently have clear field notes
about when those fields are present (e.g. "This field is available
since protocol version XXX"), but if hypothetically you don't change
the protocol version for some new fields then now the message format
becomes tied to the built-in implementation of pub/sub -- now what
field note will you say instead to explain that?
--
[1] https://www.postgresql.org/docs/current/protocol-logical-replication.html
[2]
https://www.postgresql.org/docs/current/protocol-logicalrep-message-formats.html
Kind Regards,
Peter Smith.
Fujitsu Australia.
cal content but just re-orders that option list
to be alphabetical.
--
[1] = https://www.postgresql.org/docs/devel/sql-createsubscription.html
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-create-subscription-options-list-order.patch
Description: Binary data
On Mon, Apr 19, 2021 at 2:09 PM Amit Kapila wrote:
>
> On Mon, Apr 19, 2021 at 6:32 AM Euler Taveira wrote:
> >
> > On Sun, Apr 18, 2021, at 8:59 PM, Peter Smith wrote:
> >
> > The CREATE SUBSCRIPTION documentation [1] includes a list of "WITH"
> >
true; can be assigned in just 1 common place
INSIDE the pgoutput_begin function.
--
14. Test Code.
I noticed that there is no test code specifically for seeing if empty
transactions get sent or not. Is it possible to write such a test or
is this traffic improvement only observable using the debugger?
--
[1] https://commitfest.postgresql.org/33/
Kind Regards,
Peter Smith.
Fujitsu Australia
+ }
}
The byte_length was not being checked before, so why is the check needed now?
==
7. test typo "ralation"
+# check query command completion for upper character ralation name
+check_completion("update TAB1 SET \t", qr/update TAB1 SET \af/,
"complete column name for TAB1");
==
8. test typo "case-insensitiveq"
+# check schema query(upper case) which is case-insensitiveq
+check_completion("select oid from Pg_cla\t", qq/select oid from
Pg_cla\b\b\b\b\bG_CLASS /, "complete schema query with uppper case
string");
--
Kind Regards,
Peter Smith.
Fujitsu Australia
.
PSA a patch to correct this.
--
[1]
https://www.postgresql.org/message-id/CAHut%2BPuB07xOgJLnDhvbtp0t_qMDhjDD%2BkO%2B2yB%2Br6tgfaR-5Q%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Fix-concurrent-abort-for-when-streaming.patch
Description: Binary data
Hi,
PSA patch to fix a misnamed function in a comment.
typo: "DecodePreare" --> "DecodePrepare"
------
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Fix-typo-misnamed-function-in-comment.patch
Description: Binary data
On Fri, Apr 23, 2021 at 3:46 PM Ajin Cherian wrote:
>
>
>
> On Mon, Apr 19, 2021 at 6:22 PM Peter Smith wrote:
>>
>>
>> Here are a some review comments:
>>
>> --
>>
>> 1. The patch v3 applied OK but with whitespace warnings
&g
On Mon, Apr 26, 2021 at 9:22 PM vignesh C wrote:
>
> On Wed, Apr 21, 2021 at 12:13 PM Peter Smith wrote:
> >
> > On Tue, Apr 20, 2021 at 3:45 PM Peter Smith wrote:
> > >
> > > Please find attached the latest patch set v73`*
> > >
> > > Di
On Tue, Apr 27, 2021 at 1:41 PM vignesh C wrote:
>
> On Wed, Apr 21, 2021 at 12:13 PM Peter Smith wrote:
> >
> > On Tue, Apr 20, 2021 at 3:45 PM Peter Smith wrote:
> > >
> > > Please find attached the latest patch set v73`*
> > >
> > > Di
On Tue, Apr 27, 2021 at 6:17 PM vignesh C wrote:
>
> On Wed, Apr 21, 2021 at 12:13 PM Peter Smith wrote:
> >
> > On Tue, Apr 20, 2021 at 3:45 PM Peter Smith wrote:
> > >
> > > Please find attached the latest patch set v73`*
> > >
> > > Di
et's see what others has to say.
>
Hmmm - I am not so sure about those goto replacements. I think the
poor goto has a bad reputation, but not all gotos are bad. I've met
some very nice gotos.
Each goto here was doing exactly what it looked like it was doing,
whereas all these boolean replacements have now introduced subtle
differences. e.g. now the *volatility_item = defel; assignment (and
all similar assignments) will happen which previously did not happen
at all. It leaves the reader wondering if assigning to those
references could have any side-effects at the caller. Probably there
are no problems at allbut can you be sure? Meanwhile, those
"inelegant" gotos did not give any cause for such doubts.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ion use a stack variable consistent
with the other nearby functions.
Thoughts?
--
[1]
https://github.com/postgres/postgres/commit/7c4f52409a8c7d85ed169bbbc1f6092274d03920#
Kind Regards,
Peter Smith.
Fujitsu Australia
v1-0001-Fix-wrconn.-Use-stack-variable.patch
Description: Binary data
On Tue, May 4, 2021 at 1:56 PM Bharath Rupireddy
wrote:
>
> On Tue, May 4, 2021 at 5:00 AM Peter Smith wrote:
> >
> > While reviewing some logical replication code I stumbled across a
> > variable usage that looks suspicious to me.
> >
> > Note that the AlterS
On Tue, May 4, 2021 at 2:31 PM Andres Freund wrote:
>
> Hi,
>
> On 2021-05-04 09:29:42 +1000, Peter Smith wrote:
> > While reviewing some logical replication code I stumbled across a
> > variable usage that looks suspicious to me.
>
> > Note that the AlterSubs
PSA v2 of this patch - it has the same content, but an improved commit comment.
I have also added a commitfest entry, https://commitfest.postgresql.org/33/3109/
--
Kind Regards,
Peter Smith
Fujitsu Australia
v2-0001-Fix-wrconn.-Use-stack-variable.patch
Description: Binary data
On Wed, May 5, 2021 at 3:20 PM Bharath Rupireddy
wrote:
>
> On Wed, May 5, 2021 at 8:05 AM Tom Lane wrote:
> >
> > Peter Smith writes:
> > > This patch replaces the global "wrconn" in AlterSubscription_refresh with
> > > a local variable of
On Wed, May 5, 2021 at 12:35 PM Tom Lane wrote:
>
> Peter Smith writes:
> > This patch replaces the global "wrconn" in AlterSubscription_refresh with a
> > local variable of the same name, making it consistent with other functions
> > in subscriptioncmds.
On Thu, May 6, 2021 at 7:18 PM Japin Li wrote:
>
>
> On Thu, 06 May 2021 at 17:08, Peter Smith wrote:
> > On Wed, May 5, 2021 at 12:35 PM Tom Lane wrote:
> >>
> >> Peter Smith writes:
> >> > This patch replaces the global "wrconn" in Alt
On Fri, May 7, 2021 at 7:08 AM Tom Lane wrote:
>
> Alvaro Herrera writes:
> > On 2021-May-06, Peter Smith wrote:
> >> PSA v3 of the patch. Same as before, but now also renames the global
> >> variable from "wrconn" to "lrep_worker_wrconn".
>
t; used is uint32 but documented as int32.
> Attached is a patch which has the fix for the same.
> Thoughts?
If you want to do this then there are more - e.g. Flags should be
Uint8 instead of Int8.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Sun, May 9, 2021 at 11:13 PM Peter Smith wrote:
>
> On Sun, May 9, 2021 at 10:38 PM vignesh C wrote:
> >
> > Hi,
> >
> > For some of the logical replication messages the data type documented
> > was not correct, especially for lsn and xid. For lsn actua
On Fri, May 7, 2021 at 6:09 PM Peter Smith wrote:
>
> On Fri, May 7, 2021 at 7:08 AM Tom Lane wrote:
> >
> > Alvaro Herrera writes:
> > > On 2021-May-06, Peter Smith wrote:
> > >> PSA v3 of the patch. Same as before, but now also renames the
> +
>
Can you please provide more details so I can be sure of the context of
this feedback, e.g. there are multiple places that match that patch
fragment provided. So was this suggestion to change all of them ( 'b',
'P', 'K' , 'r' of patch 0001; and also 'p' of patch 0002) ?
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
PSA v5 of the patch. It is the same as v4 but with the v4-0001 part
omitted because that was already pushed.
(reposted to keep cfbot happy).
--
Kind Regards,
Peter Smith
Fujitsu Australia
v5-0001-Rename-the-logical-replication-global-wrconn.patch
Description: Binary data
Regards,
Peter Smith.
Fujitsu Australia
v1-0001-GetSubscriptionRelations-declare-only-1-scan-key.patch
Description: Binary data
ilter-for-logical-replication.patch:518:
trailing whitespace.
error: patch failed: src/backend/parser/gram.y:426
error: src/backend/parser/gram.y: patch does not apply
error: patch failed: src/backend/replication/logical/worker.c:340
error: src/backend/replication/logical/worker.c: patch does not apply
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Mon, May 10, 2021 at 6:09 PM Bharath Rupireddy
wrote:
>
> On Mon, May 10, 2021 at 12:36 PM Peter Smith wrote:
> >
> > The function GetSubscriptionRelations was declaring ScanKeyData
> > skey[2]; but actually
> > only uses 1 scan key. It seems like the code was c
.g. maybe you could say what C type the bytes
represent when they come off the wire at the other end - something
like below.
BEFORE
Int64
The final LSN of the transaction.
AFTER
Int64
The final LSN (XLogRecPtr) of the transaction
--
[1] https://www.postgresql.org/docs/devel/protocol-message-types.html
[2] https://linux.die.net/man/3/ntohl
Kind Regards,
Peter Smith.
Fujitsu Australia.
quot; already means.
e.g. consider change to just say "Flags (uint8); currently unused."
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, May 12, 2021 at 5:52 PM Michael Paquier wrote:
>
> On Tue, May 11, 2021 at 04:50:46PM +0900, Michael Paquier wrote:
> > And that makes the code slightly easier to follow.
>
> Yeah, that's better this way, so applied.
Thanks!
------
Kind Regards,
Peter Smith.
Fujitsu Australia
On Wed, May 12, 2021 at 11:09 PM vignesh C wrote:
...
>
> Thanks for the comments. Attached v4 patch has the fix for the same.
>
I have not tried this patch so I cannot confirm whether it applies or
renders OK, but just going by the v4 content this now LGTM.
Kind Regards,
Pe
On Mon, May 10, 2021 at 1:31 PM vignesh C wrote:
>
> On Thu, Apr 29, 2021 at 2:23 PM Peter Smith wrote:
> >
> > Please find attached the latest patch set v74*
> >
> > Differences from v73* are:
> >
> > * Rebased to HEAD @ 2 days ago.
> >
> &
c) but the
ExecuteTruncate is using a more powerful AcccessExclusiveLock than the
apply_handle_truncate was using.
~~
PSA a patch to make the apply_handle_truncate use AccessExclusiveLock
same as the ExecuteTruncate function does.
PSA a patch adding a test for this scenario.
Kind Regards,
Peter Smit
On Mon, May 17, 2021 at 8:13 PM Amit Kapila wrote:
>
> On Mon, May 17, 2021 at 3:05 PM Dilip Kumar wrote:
> >
> > On Mon, May 17, 2021 at 12:30 PM Peter Smith wrote:
> >
> > > PSA a patch adding a test for this scenario.
> >
> > I am not sure this t
On Mon, May 17, 2021 at 6:47 PM Amit Kapila wrote:
>
> On Mon, May 17, 2021 at 12:30 PM Peter Smith wrote:
> >
[...]
> > The essence of the trouble seems to be that the apply_handle_truncate
> > function never anticipated it may end up truncating the same table
>
On Fri, May 14, 2021 at 11:16 AM David Rowley wrote:
>
> On Fri, 14 May 2021 at 12:00, Peter Smith wrote:
> > That bug led me to wonder if similar problems might be going
> > undetected elsewhere in the code. There is a gcc compiler option [3]
> > -Wshadow which i
On Sun, May 16, 2021 at 12:07 AM Ajin Cherian wrote:
>
> On Thu, May 13, 2021 at 7:50 PM Peter Smith wrote:
> >
> > Please find attached the latest patch set v75*
> >
> > Differences from v74* are:
> >
> > * Rebased to HEAD @ today.
> >
> > *
On Tue, May 18, 2021 at 9:32 PM Amit Kapila wrote:
>
> On Thu, May 13, 2021 at 3:20 PM Peter Smith wrote:
> >
> > Please find attached the latest patch set v75*
> >
>
> Review comments for v75-0001-Add-support-for-pr
9.
+ /* For DROP PUBLICATION, copy_data option is not supported. */
+ opts->copy_data = isadd ? ©_data : NULL;
The opts struct is already zapped 0/NULL so this code maybe should be:
if (isadd)
opts.copy_data = ©_data;
==
10.
Since the new typedef ParseSubOptions was added by this patch
shouldn't the src/tools/pgindent/typedefs.list file be updated also?
--
Kind Regards,
Peter Smith.
Fujitsu Australia.
On Fri, May 21, 2021 at 9:21 PM Peter Smith wrote:
>
> On Thu, May 20, 2021 at 2:11 PM Bharath Rupireddy
> wrote:
> >
> > On Wed, May 19, 2021 at 6:13 PM Bharath Rupireddy
> > wrote:
> > > Thanks. I will work on the new structure ParseSubOption only for
>
Hi.
The attached PG docs patch about catalog deadlocks was previously
implemented in another thread [1], but it seems more relevant to this
one.
PSA.
--
[1]
https://www.postgresql.org/message-id/CAA4eK1K%2BSeT31pxwL5iTvXq%3DJhZpG_cUJLFhiz-eD%2BJr-WAPeg%40mail.gmail.com
Kind Regards,
Peter
level is 0?
e.g.
if (indent)
{
appendStringInfoCharMacro(out, '\n');
if (level > 0)
appendStringInfoSpaces(out, level * 4);
}
V.
if (indent)
{
appendStringInfoCharMacro(out, '\n');
appendStringInfoSpaces(out, level * 4);
}
--
Kind Regards,
Peter Smith.
Fujitsu Australia
ll OutputPluginUpdateProgress here instead?
e.g.
BEFORE
ctx->update_progress(ctx, ctx->write_location, ctx->write_xid, false);
AFTER
OutputPluginUpdateProgress(ctx, false);
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Fri, Jan 20, 2023 at 3:35 PM Amit Kapila wrote:
>
> On Fri, Jan 20, 2023 at 7:40 AM Peter Smith wrote:
> >
> > Here are some review comments for patch v3-0001.
> >
> > ==
> > src/backend/replication/logical/logical.c
> >
> > 3. forward
");
The use of "doesn't get applied expectedly" (in 2 places here) seemed
strange. Maybe it's better to say like
SUGGESTION
# Confirm disabling the subscription by ALTER DISABLE did not cause
the delayed transaction to be applied.
$result = $node_subscriber->safe_psql('postgres',
"SELECT count(a) FROM test_tab WHERE a = 0;");
is($result, qq(0), "check the delayed transaction was not applied");
--
Kind Regards,
Peter Smith.
Fujitsu Australia
16 ms. Remaining wait time: 142828 ms
DEBUG: time-delayed replication for txid 1234, min_apply_delay =
16 ms. Remaining wait time: 129994 ms
DEBUG: time-delayed replication for txid 1234, min_apply_delay =
16 ms. Remaining wait time: 110001 ms
...
--
Kind Regards,
Peter Smith.
Fujitsu Australia
is insignificant enough
that just leaving the curent code is neater?
LogicalDecodingContext *ctx = rb->private_data;
...
if (ctx->update_progress_txn && (++changes_count >= CHANGES_THRESHOLD))
{
rb->update_progress_txn(rb, txn, change);
changes_count = 0;
}
--
Kind Reagrds,
Peter Smith.
Fujitsu Australia
t;
~
14b.
The log output will be checked later - substantial delay-time case.
I think that needs re-wording to clarify.
e.g1. you have nothing called a "substantial delay-time" case.
e.g2. the word "later" confused me. Originally, I thought you meant it
is not tested yet but that you will check it "later", but now IIUC you
are just referring to the "1 day 5 minutes" test that comes below in
this location TAP file (??)
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, Jan 23, 2023 at 9:44 PM Amit Kapila wrote:
>
> On Mon, Jan 23, 2023 at 1:36 PM Peter Smith wrote:
> >
> > Here are my review comments for v19-0001.
> >
> ...
> >
> > 5. parse_subscription_options
> >
> > + /*
> > + * The combinat
}
--
Kind Regards,
Peter Smith.
Fujitsu Australia
P_MODE_IMMEDIATE, false},
{NULL, 0, false}
};
I noticed this array is still called "logical_decoding_mode_options".
Was that deliberate?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Jan 24, 2023 at 1:45 PM wangw.f...@fujitsu.com
wrote:
>
> On Tues, Jan 24, 2023 at 8:28 AM Peter Smith wrote:
> > Hi Hou-san, Here are my review comments for v5-0001.
>
> Thanks for your comments.
...
>
> Changed as suggested.
>
> Attach the new
s a] delay"
(which is also correct, but it seemed a more awkward way to say it IMO)
~
Perhaps it's better to rename it more fully like
*maybe_delay_the_apply* to remove any ambiguous interpretations.
--
Kind Regards,
Peter Smith.
Fujitsu Australia
de_publisher->safe_psql(
+ 'postgres', q{
+ BEGIN;
+ INSERT INTO test_tab_2 values(1);
+ SAVEPOINT sp;
+ INSERT INTO test_tab_2 values(1);
+ ROLLBACK TO sp;
+ COMMIT;
+ });
Perhaps this should insert 2 different values so then the verification
code can check the correct value remains instead of just checking
COUNT(*)?
--
Kind Regards,
Peter Smith.
Fujitsu Australia
rn PGDLLIMPORT int logical_decoding_work_mem;
-extern PGDLLIMPORT int logical_decoding_mode;
+extern PGDLLIMPORT int logical_replication_mode;
Probably here should also be a comment to say "/* GUC variables */"
--
[1]
https://www.postgresql.org/message-id/OS0PR01MB5716AE9F095F9E7888987BC794C99%40OS0PR01MB5716.jpnprd01.prod.outlook.com
Kind Regards,
Peter Smith.
Fujitsu Australia
801 - 900 of 1980 matches
Mail list logo
