On Thu, Apr 18, 2024 at 4:26 PM Ajin Cherian wrote:
>
> Attaching the patch for your review and comments. Big thanks to Kuroda-san
> for also working on the patch.
>
>
Looking at this a bit more, maybe rolling back all prepared transactions on
the subscriber when toggling two_
s decoded as the pending
prepared transactions are prior to the "two_phase_at" LSN. With this patch,
now we are able to handle both pending prepared transactions when altering
two_phase from true to false as well as false to true.
Attaching the patch for your review and comments. Big thanks to Kuroda
ther that me that the
> problem takes place.
>
>
>
Yes, I was able to reproduce the slow catchup of twophase transactions
with pgbench with 20 clients.
regards,
Ajin Cherian
Fujitsu Australia
lution yet.
3. We could mandate that the altering of two_phase state only be done after
disabling the subscription, just like how it is handled for failover option.
Let me know your thoughts.
regards,
Ajin Cherian
Fujitsu Australia
v2-0001-Allow-altering-of-two_phase-option-of-a-SUBSCRIPT.patch
Description: Binary data
o Kuroda-san for working on the patch.
regards,
Ajin Cherian
Fujitsu Australia
v1-0001-Allow-altering-of-two_phase-option-in-subscribers.patch
Description: Binary data
discards/we discard
2. In function ReorderBufferCheckTXNAbort(): I haven't tested this but I
wonder how prepared transactions would be considered, they are neither
committed, nor in progress.
regards,
Ajin Cherian
Fujitsu Australia
amp();
+ if (TimestampDifferenceExceeds(slot->inactive_since, now,
+ slot->data.inactive_timeout * 1000))
+ inavidation_cause = RS_INVAL_INACTIVE_TIMEOUT;
+ }
+
+ if (need_locks)
+ {
+ SpinLockRelease(>mutex);
Here, GetCurrentTimestamp() is still called with SpinLock held. Maybe do
ctive_timeout) {
MyReplicationSlot->data.inactive_timeout = inactive_timeout;
}
if (lock_acquired) {
SpinLockRelease(>mutex);
ReplicationSlotMarkDirty();
ReplicationSlotSave();
}
2. In patch 0005: why change walrcv_alter_slot option? it doesn't seem to
be used anywhere, any use case for it? If required, would the intention be
to add this as a Create Subscription option?
regards,
Ajin Cherian
Fujitsu Australia
e publication publishes "ALL TABLES"?
5. In function: pgoutput_table_filter() - this code appears to be filtering
out not just unpublished tables but also applying row based filters on
published tables as well. Is this really within the scope of the feature?
regards,
Ajin Cherian
Fujitsu Australia
tus so the caller always process this transaction.
+ */
+ if (debug_logical_replication_streaming ==
DEBUG_LOGICAL_REP_STREAMING_IMMEDIATE)
+ return false;
/process/processes
regards,
Ajin Cherian
Fujitsu Australia
, it doesn't fit
well.
regards,
Ajin Cherian
Fujitsu Australia
3 secs
There was no degradation in performance seen.
Thanks Nisha for helping with the testing.
regards,
Ajin Cherian
Fujitsu Australia
failover enabled, physical standby not configured for sync
RUN1 (TPS) RUN2 (TPS) AVERAGE (TPS)
8.18839 8.18839 8.18839-- degradation from first config *-0.17%*
PATCHED code - (v98-0001)
Synchronous_commit=on, hot_standby_feedback=on, standby_slot_names
configured to physical standby, logical subscriber failover enabled,
physical standby configured for sync
RUN1 (TPS) RUN2 (TPS) AVERAGE (TPS)
8.173062 8.068536 8.120799-- degradation from first config* -0.99%*
Overall, I do not see any significant performance degradation with the
patch and sync-slot enabled with one logical subscriber and one physical
standby.
Attaching script for my final test configuration for reference.
regards,
Ajin Cherian
Fujitsu Australia
<>
g_backend_pid();
INSERT INTO test(v) SELECT pg_backend_pid();
PREPARE TRANSACTION $$:mygid$$;
COMMIT PREPARED $$:mygid$$;
regards,
Ajin Cherian
Fujitsu Australia
the information whether it
> is
> intentionally specified or not is discarded. Then,
> GenerateRecoveryConfig() would
> just write down all the connection parameters from PGconn, they cannot
> recognize.
>
> Well, one option is that if a default dbname should be written to the
configuration file, then "postgres' is a better option than "replication"
or "username" as the default option, at least a db of that name exists.
regards,
Ajin Cherian
Fujitsu Australia
ut calling that function. This requires connecting to a database.
regards,
Ajin Cherian
Fujitsu Australia
the generated primary_conninfo string as well.
regards,
Ajin Cherian
Fujitsu Australia
On Sat, Feb 10, 2024 at 2:23 AM Masahiko Sawada
wrote:
> On Fri, Feb 9, 2024 at 7:35 PM Ajin Cherian wrote:
> >
> >
> >
> > On Tue, Feb 6, 2024 at 5:06 PM Masahiko Sawada
> wrote:
> >>
> >>
> >> I've attached the new version patch set.
nction when not necessary? This check was there in
code before the patch.
patch 0003:
+/*
+ * The threshold of the number of transactions in the max-heap
(rb->txn_heap)
+ * to switch the state.
+ */
+#define REORDE_BUFFER_MEM_TRACK_THRESHOLD 1024
Typo: I think you meant REORDER_ and not REORDE_
regards,
Ajin Cherian
Fujitsu Australia
_syncslot = true so they can receive
failover logical slots changes from the primary.
regards,
Ajin Cherian
Fujitsu Australia
without failover
enabled to make sure that the Subscription with failover disabled does not
depend on sync on standby, but this fails because we have failover enabled
by default.
In summary, I don't think these issues are actual bugs but expected
behaviour change.
regards,
Ajin Cherian
Fujitsu Australia
lse, false,
true);
?column?
--
init
(1 row)
Time: 36.125 ms
postgres=# SELECT 'init' FROM
pg_create_logical_replication_slot('lsub1_slot', 'pgoutput', false, false,
false);
?column?
--
init
(1 row)
Time: 53.981 ms
regards,
Ajin Cherian
Fujitsu Australia
he same name exists, then thereafter no new sync slots are
created on standby. Is this expected? I do see that previously created
slots are kept up to date, just that no new slots are created after
that.
regards,
Ajin Cherian
Fujitsu australia
_event_trigger_collect in indexcmds.c
since it is already present in index.c. Add it to index.h and make the
function extern so that it can be accessed in both index.c and
indexcmds.c
regards,
Ajin Cherian
Fujitsu Australia
before_shmem_exit(slotsync_worker_detach, (Datum) 0);
+
+LWLockRelease(SlotSyncWorkerLock);
+}
before_shmem_exit() can error out leaving the lock acquired. Maybe you
should release the lock prior to calling before_shmem_exit() because
you don't need the lock there.
regards,
Ajin Cherian
Fujitsu Australia
is not very good. Neither does it sound like an error, nor is
there clarity on what the underlying problem is or how to correct it.
regards,
Ajin Cherian
Fujitsu Australia
ated locally (either by itself) or by primary_slot
> being invalidated, then we should skip the sync. I will fix this in
> the next version.
Yes, that works.
Another bug I see in my testing is that
pg_get_slot_invalidation_cause() does not release the LOCK if it finds
the slot it is searching f
, then the
local_slot_update() function is never called to set the invalidation
status on the local slot. And for invalidated slots, restart_lsn is
always NULL.
regards,
Ajin Cherian
Fujitsu Australia
r flag "failover_opt_given". Plugins that set this, will be able
to change the failover flag of the slot,
while plugins that do not support this will not set this and the
failover flag of the created slot will remain.
What do you think?
regards,
Ajin Cherian
Fujitsu Australia
s failover flag
using Alter subscription. Currently alter subscription re-establishes
the connection
using START REPLICATION and failover is one of the options passed in along with
START REPLICATION. I am thinking of moving this change to
StartLogicalReplication prior to calling CreateDecodingContext by
parsing the command options in StartReplicationCmd
without adding it to the LogicalDecodingContext.
regards,
Ajin Cherian
Fujitsu Australia
fe to stop this unguarded by SlotSyncWorkerLock locking? Is
> there a window where another dbid decides to reuse this worker at the
> same time this process is about to stop it?
>
==
Only the launcher can do this, and there is only one launcher.
> ~~~
>
> 26. primary_connect
>
> +/*
> + * Connect to primary server for slotsync purpose and return the connection
> + * info. Disconnect previous connection if provided in wrconn_prev.
> + */
>
> /primary server/the primary server/
>
==
fixed
> ~~~
>
> 27.
> + if (!RecoveryInProgress())
> + return NULL;
> +
> + if (max_slotsync_workers == 0)
> + return NULL;
> +
> + if (strcmp(synchronize_slot_names, "") == 0)
> + return NULL;
> +
> + /* The primary_slot_name is not set */
> + if (!WalRcv || WalRcv->slotname[0] == '\0')
> + {
> + ereport(WARNING,
> + errmsg("Skipping slots synchronization as primary_slot_name "
> +"is not set."));
> + return NULL;
> + }
> +
> + /* The hot_standby_feedback must be ON for slot-sync to work */
> + if (!hot_standby_feedback)
> + {
> + ereport(WARNING,
> + errmsg("Skipping slots synchronization as hot_standby_feedback "
> +"is off."));
> + return NULL;
> + }
>
> How come some of these checks giving WARNING that slot synchronization
> will be skipped, but others are just silently returning NULL?
>
==
primary_slot_name and hot_standby_feedback are not GUCs exclusive to
slot synchronization, they
are previously existing - so warning only for them. The others are
specific to slot synchronization,
so if users set them (which shows that the user intends to use sync-slot),
then warning to let the user know that these others also need to be set.
> ~~~
>
> 28. SaveCurrentSlotSyncConfigs
>
> +static void
> +SaveCurrentSlotSyncConfigs()
> +{
> + PrimaryConnInfoPreReload = pstrdup(PrimaryConnInfo);
> + PrimarySlotNamePreReload = pstrdup(WalRcv->slotname);
> + SyncSlotNamesPreReload = pstrdup(synchronize_slot_names);
> +}
>
> Shouldn't this code also do pfree first? Otherwise these will slowly
> leak every time this function is called, right?
>
==
fixed
> ~~~
>
> 29. SlotSyncConfigsChanged
>
> +static bool
> +SlotSyncConfigsChanged()
> +{
> + if (strcmp(PrimaryConnInfoPreReload, PrimaryConnInfo) != 0)
> + return true;
> +
> + if (strcmp(PrimarySlotNamePreReload, WalRcv->slotname) != 0)
> + return true;
> +
> + if (strcmp(SyncSlotNamesPreReload, synchronize_slot_names) != 0)
> + return true;
>
> I felt those can all be combined to have 1 return instead of 3.
>
==
fixed
> ~~~
>
> 30.
> + /*
> + * If we have reached this stage, it means original value of
> + * hot_standby_feedback was 'true', so consider it changed if 'false' now.
> + */
> + if (!hot_standby_feedback)
> + return true;
>
> "If we have reached this stage" seems a bit vague. Can this have some
> more explanation? And, maybe also an Assert(hot_standby_feedback); is
> helpful in the calling code (before the config is reloaded)?
>
==
rewrote this without that comment.
regards,
Ajin Cherian
Fujitsu Australia
gt; > the average (integer value) of this distance over the span of 10 min
> > workload and this is what we got:
> >
>
> I have attached the scripts for schema-setup, running workload and
> capturing lag. Please go through Readme for details.
>
>
I did some more tests for 10,20 and 40 slots to calculate the average
lsn distance
between slots, comparing 1 worker and 3 workers.
My results are as follows:
10 slots
1 worker: 5529.75527426 (average lsn distance between primary and
standby per slot)
3 worker: 2224.57589134
20 slots
1 worker: 9592.87234043
3 worker: 3194.6293
40 slots
1 worker: 20566.093
3 worker: 7885.80952381
90 slots
1 worker: 36706.8405797
3 worker: 10236.6393162
regards,
Ajin Cherian
Fujitsu Australia
mp.c
>
> 29. getEventTriggers
>
> + /* skip internally created event triggers by checking evtisinternal */
> appendPQExpBufferStr(query,
> "SELECT e.tableoid, e.oid, evtname, evtenabled, "
> "evtevent, evtowner, "
>
> Uppercase the comment.
>
fixed.
> ==
> src/include/catalog/pg_event_trigger.h
>
> 33.
> @@ -36,7 +36,7 @@ CATALOG(pg_event_trigger,3466,EventTriggerRelationId)
> * called */
> char evtenabled; /* trigger's firing configuration WRT
> * session_replication_role */
> -
> + bool evtisinternal; /* trigger is system-generated */
> #ifdef CATALOG_VARLEN
> text evttags[1]; /* command TAGs this event trigger targets */
> #endif
>
> ~
>
> This change should not remove the blank line that previously existed
> before the #ifdef CATALOG_VARLEN.
>
fixed.
> ==
> src/include/catalog/pg_publication.
>
> 34.
> +/* Publication trigger events */
> +#define PUB_TRIG_DDL_CMD_START "ddl_command_start"
> +#define PUB_TRIG_DDL_CMD_END "ddl_command_end"
> +#define PUB_TRIG_TBL_REWRITE "table_rewrite"
> +#define PUB_TRIG_TBL_INIT_WRITE "table_init_write"
>
> Elsewhere in PG15 code there are already hardcoded literal strings for
> these triggers, so I am wondering if these constants should really be
> defined in some common place where everybody can make use of them
> instead of having a mixture of string literals and macros for the same
> strings.
>
fixed.
> ==
> src/include/commands/event_trigger.h
>
> 35.
> -extern Oid CreateEventTrigger(CreateEventTrigStmt *stmt);
> +extern Oid CreateEventTrigger(CreateEventTrigStmt *stmt, bool isinternal);
> extern Oid get_event_trigger_oid(const char *trigname, bool missing_ok);
>
> IMO a better name is 'is_internal' (Using a snake-case name matches
> like the other 'missing_ok')
>
fixed.
> ==
> src/include/replication/ddlmessage.h
>
> 36.
> + * Copyright (c) 2022, PostgreSQL Global Development Group
>
> Copyright for the new file should be 2023?
>
fixed.
> ==
> src/include/tcop/ddldeparse.h
>
> 37.
> * ddldeparse.h
> *
> * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
> * Portions Copyright (c) 1994, Regents of the University of California
> *
> * src/include/tcop/ddldeparse.h
>
> ~
>
> I think this is a new file for the feature so why is the copyright
> talking about old dates like 1994,1996 etc?
>
fixed.
regards,
Ajin Cherian
On Mon, May 22, 2023 at 10:36 PM Masahiko Sawada wrote:
>
> On Wed, May 17, 2023 at 10:10 AM Ajin Cherian wrote:
> >
> > On Mon, May 15, 2023 at 10:47 PM Masahiko Sawada
> > wrote:
> > >
> > > On Mon, May 15, 2023 at 5:44 PM Ajin Cherian wrote:
>
On Mon, May 15, 2023 at 10:47 PM Masahiko Sawada wrote:
>
> On Mon, May 15, 2023 at 5:44 PM Ajin Cherian wrote:
> >
> > On Fri, May 12, 2023 at 9:55 PM Ajin Cherian wrote:
> > >
> > > If nobody else is working on this, I can come up with a patch to fix this
&
On Fri, May 12, 2023 at 9:55 PM Ajin Cherian wrote:
>
> If nobody else is working on this, I can come up with a patch to fix this
>
Attaching a patch which attempts to fix this.
regards,
Ajin Cherian
Fujitsu Australia
v1-0001-Fix-issue-where-the-copy-command-does-not-adhere-.patch
De
eans the table copy is being invoked as the
subscription owner and not the table owner.
However, I see subsequent inserts fail on replication with
permission denied error, so the apply worker correctly
applies the inserts as the table owner.
If nobody else is working on this, I can come up with a patch to fix this
regards,
Ajin Cherian
Fujitsu Australia
first(lc1);
^
ddl_deparse.c:8956:31: note: each undeclared identifier is reported
only once for each function it appears in
ddl_deparse.c:8956:48: error: expected expression before ‘)’ token
publication_rel *pub_rel = (publication_rel *) lfirst(lc1);
regards,
Ajin Cheria
IsTransactionState());
> + GetCurrentTransactionId();
>
> Single line comment should be OK here
>
Fixed.
> ~
>
> 23.
>
> + /* trailing zero is critical; see logicalddlmsg_desc */
>
> Uppercase comment
>
fixed.
> ~
>
> 24.
>
> + /* allow origin filtering */
>
> Uppercase comment
>
fixed.
> ==
>
> src/backend/replication/logical/proto.c
>
> 25. logicalrep_read_ddlmessage
>
> + uint8 flags;
> + char *msg;
> +
> + //TODO double check when do we need to get TransactionId.
> +
> + flags = pq_getmsgint(in, 1);
> + if (flags != 0)
> + elog(ERROR, "unrecognized flags %u in ddl message", flags);
> + *lsn = pq_getmsgint64(in);
> + *prefix = pq_getmsgstring(in);
> + *sz = pq_getmsgint(in, 4);
> + msg = (char *) pq_getmsgbytes(in, *sz);
> +
> + return msg;
>
> 25a.
> This code will fail if the associated *write* function has sent a xid.
> Maybe additional param is needed to tell it when to read the xid?
>
removed to not send xid, not required.
> ~
>
> 25b.
> Will be tidier to have a blank line after the elog
>
fixed.
> ~~~
>
> 26. logicalrep_write_ddlmessage
>
> + /* transaction ID (if not valid, we're not streaming) */
> + if (TransactionIdIsValid(xid))
> + pq_sendint32(out, xid);
>
> Perhaps this "write" function should *always* write the xid even if it
> is invalid because then the "read" function will know to always read
> it.
>
changed it to never send xid.
> ==
>
> src/backend/replication/logical/reorderbuffer.c
>
> 27. ReorderBufferQueueDDLMessage
>
> + Assert(xid != InvalidTransactionId);
>
> SUGGESTION
> Assert(TransactionIdIsValid(xid));
>
fixed.
> ~~~
>
> 28. ReorderBufferSerializeChange
>
> + data += sizeof(int);
> + memcpy(data, change->data.ddlmsg.prefix,
> +prefix_size);
> + data += prefix_size;
>
> Unnecessary wrapping of memcpy.
>
fixed.
> ~
>
> 29.
>
> + memcpy(data, >data.ddlmsg.cmdtype, sizeof(int));
> + data += sizeof(int);
>
> Would that be better to write as:
>
> sizeof(DeparsedCommandType) instead of sizeof(int)
>
fixed.
> ~~~
>
> 30. ReorderBufferChangeSize
>
> + case REORDER_BUFFER_CHANGE_DDLMESSAGE:
> + {
> + Size prefix_size = strlen(change->data.ddlmsg.prefix) + 1;
> +
> + sz += prefix_size + change->data.ddlmsg.message_size +
> + sizeof(Size) + sizeof(Size) + sizeof(Oid) + sizeof(int);
>
> sizeof(DeparsedCommandType) instead of sizeof(int)
>
fixed.
Breaking this into two mails, next set of comments in next mail.
regards,
Ajin Cherian
Fujitsu Australia
lication",
maybe in future
we'll consider adding it as part of replication slot parameters.
regards,
Ajin Cherian
Fujitsu Australia
hanks
> Raejsh
>
Currently this feature is only supported using "Create publication".
We have not added
a slot level parameter to trigger this.
regards,
Ajin Cherian
Fujitsu Australia
ide, then converts these messages to actual
DDL commands and executes them.
regards,
Ajin Cherian
Fujitsu Australia
erts a relation to
a view when you create such a rule like the test case does.
So initially the relation had storage, the pgstat_info is linked,
then table is converted to a view, but in init, the previous
relation is not unlinked but when it tries to link a new relation, the
assert fails saying a previous relation is already linked to
pgstat_info
I have made a small patch with a fix, but I am not sure if this is the
right way to fix this.
regards,
Ajin Cherian
Fujitsu Australia
pgstat_assoc_fix_for_views.patch
Description: Binary data
ould you add the changes to patch 1 and patch 2, rather than adding a
new patch?
Otherwise, we'll have a separate patch for each command and it will
take double work to keep it updated
for each new command added.
thanks,
Ajin Cherian
Fujitsu Australia
is worth mentioning:
Skip empty transactions for logical replication.
commit d5a9d86d8ffcadc52ff3729cd00fbd83bc38643c
https://github.com/postgres/postgres/commit/d5a9d86d8ffcadc52ff3729cd00fbd83bc38643c
regards,
Ajin Cherian
Fujitsu Australia
and we shall do that in the next patch unless
> this approach is not worth pursuing.
>
> This POC is prepared by Ajin Cherian, Hou-San, and me.
>
> Thoughts?
>
> [1] -
> https://www.postgresql.org/message-id/20150215044814.GL3391%40alvh.no-ip.org
I have updated Amit'
mit
messages as previously present.
regards,
Ajin Cherian
atches 3 and 4 seem
to be related to the testing of the extension and contrib module
ddl_deparse which is not in this patch-set, so I have left those
patches out, as I have no way of testing the test cases added as part
of these patches.
regards,
Ajin Cherian
Fujitsu Australia
0002-Move-some-
On Wed, Apr 13, 2022 at 2:29 PM Ajin Cherian wrote:
>
>
> This patch-set has not been rebased for some time. I see that there is
> interest in this patch from the logical
> replication of DDL thread [1].
>
Forgot to add the link to the thread.
[1] -
https://www.postgre
e is
interest in this patch from the logical
replication of DDL thread [1].
I will take a stab at rebasing this patch-set, I have already rebased
the first patch and will work on the other
patches in the coming days. Do review and give me feedback.
regards,
Ajin Cherian
Fujitsu Australia
0001-ddl
her review.
>
The patch no longer applies. The patch is a very good attempt, and I
would also like to contribute if required.
I have a few comments but will hold it till a rebased version is available.
regards,
Ajin Cherian
Fujitsu Australia
On Tue, Apr 5, 2022 at 8:32 PM Amit Kapila wrote:
>
> How about something like the attached?
>
LGTM.
regards,
Ajin Cherian
Fujitsu Australia
st
> files(say in 001_rep_changes.pl)?
added a simple test.
> 4. I think we can drop the skip streaming patch as we can't do that for now.
Dropped,
In addition, I have also added a few more comments explaining why the begin send
is delayed in pgoutput_change till row_filter is checked and
On Mon, Mar 7, 2022 at 11:44 PM Ajin Cherian wrote:
>
> Fixed.
>
> regards,
> Ajin Cherian
> Fujitsu Australia
Rebased the patch and fixed some whitespace errors.
regards,
Ajin Cherian
Fujitsu Australia
v25-0001-Skip-empty-transactions-for-logical-replication.patch
Descri
paliveIfNecessary(ctx, false);
+
This reset is called too early, this function might go on to skip
changes because of the row filter, so this
reset fits better once we know for sure that a change is sent out. You
will also need to send keep alive
when the change is skipped due to the row filter.
On Mon, Mar 7, 2022 at 7:50 PM shiy.f...@fujitsu.com
wrote:
>
> On Fri, Mar 4, 2022 9:41 AM Ajin Cherian wrote:
> >
> > I have split the patch into two. I have kept the logic of skipping
> > streaming changes in the second patch.
> > I will work on the secon
I have split the patch into two. I have kept the logic of skipping
streaming changes in the second patch.
I will work on the second patch once we can figure out a solution for
the COMMIT PREPARED after restart problem.
regards,
Ajin Cherian
v23-0001-Skip-empty-transactions-for-logical
ntext */
> old = MemoryContextSwitchTo(data->context);
>
>
> I am not sure if it is suitable to send begin or stream_start here, because
> the
> row filter is not checked yet. That means, empty transactions caused by row
> filter are not skipped.
>
Moved the chec
xistent streamed file. We could add logic
to silently ignore a spurious "stream prepare"
but that might not be ideal. Any thoughts on how to address this? Or
else, we will need to avoid skipping streamed
transactions as well.
regards,
Ajin Cherian
Fujitsu Australia
ing replication of an empty transaction in stream
> commit");
> + return;
> + }
>
> (Same as previous comment #15). I didn't really understand why this
> code is checking the 'sent_begin_txn' member instead of the
> 'sent_stream_start' member?
>
> ~~~
Changed.
>
&g
ts from Peter and Wang in my next patch update.
regards,
Ajin Cherian
Fujitsu Australia
v20-0001-Skip-empty-transactions-for-logical-replication.patch
Description: Binary data
On Thu, Feb 17, 2022 at 9:42 PM Amit Kapila wrote:
>
> On Mon, Jan 31, 2022 at 6:18 PM Ajin Cherian wrote:
> >
>
> Few comments:
> =
> 1. Is there any particular why the patch is not skipping empty xacts
> for streaming (in-progress) transactions as no
ward compatibility for this small
added optimization.
Amit,
I will work on your comments.
regards,
Ajin Cherian
Fujitsu Australia
nt).
Hi Wang,
Some comments:
I see you only track skipped Inserts/Updates and Deletes. What about
DDL operations that are skipped, what about truncate.
What about changes made to unpublished tables? I wonder if you could
create a test script that only did DDL operations
and truncates, would this timeout happen?
regards,
Ajin Cherian
Fujitsu Australia
wed
CREATE PUBLICATION pub_1 FOR TABLE test WHERE (key > 100); -- 0% allowed
DO
$do$
BEGIN
FOR i IN 1..101 BY 4000 LOOP
Alter table test alter column value1 TYPE varchar(30);
INSERT INTO test VALUES(i,'BAH', row_to_json(row(i)));
Alter table test ALTER COLUMN value1 TYPE text;
UPDATE test SET value = 'FOO' WHERE key = i;
COMMIT;
END LOOP;
END
$do$;
regards,
Ajin Cherian
Fujitsu Australia
_json(row(i)));
UPDATE test SET value = 'FOO' WHERE key = i;
IF I % 1000 = 0 THEN
COMMIT;
END IF;
END LOOP;
END
$do$;
regards,
Ajin Cherian
Fujitsu Australia
On Tue, Feb 1, 2022 at 12:07 PM Peter Smith wrote:
>
> On Sat, Jan 29, 2022 at 11:31 AM Andres Freund wrote:
> >
> &
On Sun, Jan 30, 2022 at 7:04 PM osumi.takami...@fujitsu.com
wrote:
>
> On Thursday, January 27, 2022 9:57 PM Ajin Cherian wrote:
> Hi, thanks for your patch update.
>
>
> > On Wed, Jan 26, 2022 at 8:33 PM osumi.takami...@fujitsu.com
> > wrote:
> > >
> &
On Thu, Jan 27, 2022 at 12:16 AM osumi.takami...@fujitsu.com
wrote:
>
> On Tuesday, January 11, 2022 6:43 PM From: Ajin Cherian
> wrote:
> > Minor update to rebase the patch so that it applies clean on HEAD.
> Hi, let me share some additional comments on v16.
On Wed, Jan 26, 2022 at 8:33 PM osumi.takami...@fujitsu.com
wrote:
>
> On Tuesday, January 11, 2022 6:43 PM Ajin Cherian wrote:
> > Minor update to rebase the patch so that it applies clean on HEAD.
> Hi, thanks for you rebase.
>
> Several comments.
>
On Wed, Sep 1, 2021 at 8:57 PM Ajin Cherian wrote:
>
> Thanks for the comments. Addressed them in the attached patch.
>
> regards,
> Ajin Cherian
> Fujitsu Australia
Minor update to rebase the patch so that it applies clean on HEAD.
regards,
Ajin Cherian
regards,
Ajin Cheri
get from the value
>from the virtual tuple.
Storing the old tuple/new tuple in a slot and re-using the slot avoids
the overhead of
continuous deforming of tuple at multiple levels in the code.
regards,
Ajin Cherian
[1] -
https://www.postgresql.org/message-id/CAFiTN-vwBjy+eR+iodkO5UVN5cPv_xx1=s8ehzgcrjza+az...@mail.gmail.com
hile warning him on what the consequences are rather than restricting
him with constraints.
We could explain this in the documentation so that users can better
predict the effect of having pubaction specific filters.
regards,
Ajin Cherian
Fujitsu Australia
update was previously published or not?
I think it's futile for the publisher side to try and figure out the
history of published rows. In fact, if this level of logic is required
then it is best implemented on the subscriber side, which then defeats
the purpose of a publication filter.
regards,
Ajin Cherian
Fujitsu Australia
re we can check the transformed where clause and see if the where
> clause is the same or not. If you are ok with this approach you could
> make similar changes.
thanks for your patch, I have used the same logic with minor changes
and shared it with Peter for v44.
regards,
Ajin Cherian
Fujitsu Australia
On Tue, Oct 12, 2021 at 1:37 AM Dilip Kumar wrote:
>
> On Wed, Oct 6, 2021 at 2:33 PM Ajin Cherian wrote:
> >
> > On Sat, Oct 2, 2021 at 5:44 PM Ajin Cherian wrote:
> > >
> > > I have for now also rebased the patch and merged the first 5 patches
> > &g
On Mon, Sep 27, 2021 at 11:20 AM Ajin Cherian wrote:
>
> On Sat, Sep 25, 2021 at 4:28 AM tushar wrote:
> >
> > On 9/24/21 10:36 PM, Robert Haas wrote:
> > > Here's v9, fixing the issue reported by Fujii Masao.
> >
> > Please refer this scenario where
is restarted. There are logs which indicate that this
has happened. If you could share the
logs (on publisher and subscriber) when this happens, I could have a look.
regards,
Ajin Cherian
Fujitsu Australia
On Wed, Sep 22, 2021 at 1:50 PM Amit Kapila wrote:
>
> On Wed, Sep 22, 2021 at 6:42 AM Ajin Cherian wrote:
> >
>
> Why do you think that the second assumption (if there is an old tuple
> it will contain all RI key fields.) is broken? It seems to me even
> when we
is currently logic in ReorderBufferToastReplace() which already
deforms the new tuple
to detoast changed toasted fields in the new tuple. I think if we can
enhance this logic for our
purpose, then we can avoid an extra deform of the new tuple.
But I think you had earlier indicated that having untoa
can apply a filter expression on individual attributes. The current
mechanism does it
on a tuple. Do let me know if you have any ideas there?
Even if it were done, there would still be the overhead of deforming the tuple.
I will run some performance tests like Amit suggested and see what the
overhead is and
try to minimise it.
regards,
Ajin Cherian
Fujitsu Australia
gt; DELETE
old-row (no match) new row (match) -> INSERT
old-row (match) new row (match) -> UPDATE
old-row (no match) new-row (no match) -> (drop change)
regards,
Ajin Cherian
Fujitsu Australia
t has any external data.
* It's always true in the DELETE case.
After:
* key_required should be false if caller knows that no replica identity
* columns changed value and it doesn't have any external data.
* It's always true in the DELETE case.
regards,
Ajin Cherian
Fujitsu Australia
nd they seem to be working fine.
No review comments, the patch looks good to me.
regards,
Ajin Cherian
Fujitsu Australia
ep alive when skipping transactions in synchronous
> replication mode */
> +static bool force_keepalive_syncrep = false;
>
> =>
> "force" --> "Force"
>
> --
>
> Otherwise, v14-0001 LGTM.
>
Thanks for the comments. Addressed them in the attached patch.
regards,
Ajin Cherian
Fujitsu Australia
v15-0001-Skip-empty-transactions-for-logical-replication.patch
Description: Binary data
On Tue, Aug 31, 2021 at 3:47 PM Masahiko Sawada wrote:
>
> On Tue, Aug 31, 2021 at 12:11 PM Amit Kapila wrote:
> >
> > On Mon, Aug 30, 2021 at 5:48 PM Ajin Cherian wrote:
> > >
> > > On Mon, Aug 30, 2021 at 7:52 PM Amit Kapila
> > > wrote:
>
n_name =
> 'tap_sub';"
> );
> $node_subscriber->safe_psql('postgres',
> "ALTER SUBSCRIPTION tap_sub SET PUBLICATION tap_pub_ins_only
> WITH (copy_data = false)"
> );
> $node_publisher->poll_query_until('postgres',
> "SELECT pid != $oldp
es as well to make it stable and reduce such random
> build farm failures.
I have made the changes in 022_twophase_cascade.pl for HEAD as well as
patches for older branches.
regards,
Ajin Cherian
Fujitsu Australia
HEAD-v2-0001-fix-for-tap-test-failure-seen-in-001_rep_changes.patch
Description:
ears its pid on the shmem.
> 5. the second walsender writes disconnection log.
> 6. the first walsender writes disconneciton log.
I agree with this.
Attaching a patch on head that modifies this particular script to also
consider the state of the walsender.
regards,
Ajin Cherian
Fujitsu A
On Thu, Aug 26, 2021 at 1:54 PM Amit Kapila wrote:
>
> On Thu, Aug 26, 2021 at 9:21 AM Ajin Cherian wrote:
> >
> > On Thu, Aug 26, 2021 at 1:06 PM Amit Kapila wrote:
> >
> > >
> > > You have a point but if we see the below logs, it seems the second
are seeing this problem?
If the pid is cleared in the other order, wouldn't the query [1] return a false?
[1] - " SELECT pid != 16336 FROM pg_stat_replication WHERE
application_name = 'tap_sub';"
regards,
Ajin Cherian
Fujitsu Australia
On Thu, Aug 26, 2021 at 11:02 AM Masahiko Sawada wrote:
>
> On Wed, Aug 25, 2021 at 11:04 PM Ajin Cherian wrote:
> >
> > On Wed, Aug 25, 2021 at 11:17 PM Amit Kapila
> > wrote:
> > >
> > > On Wed, Aug 25, 2021 at 6:10 PM Masahiko Sawada
> &
ts to get the pid of the running walsender for
tap_sub but returns null because both walsender exits.
8. This null return value results in the next query erroring out and
the test failing.
>Can you additionally check the value of 'state' from
>pg_stat_replication for both the old and new walsend
T pid != $oldpid FROM pg_stat_replication WHERE
> application_name = '$appname';"
Yes, the query [1] returns OK with a {f,t} or {t,f}
[1] - "SELECT pid != $oldpid FROM pg_stat_replication WHERE
application_name = '$appname';"
regards,
Ajin Cherian
Fujitsu Australia
On Wed, Aug 25, 2021 at 5:43 PM Ajin Cherian wrote:
>
> On Wed, Aug 25, 2021 at 4:22 PM Amit Kapila wrote:
> >
> > On Wed, Aug 25, 2021 at 8:00 AM Ajin Cherian wrote:
> > >
> > > On Tue, Aug 24, 2021 at 11:12 PM Amit Kapila
> > > wrote:
> >
On Wed, Aug 25, 2021 at 4:22 PM Amit Kapila wrote:
>
> On Wed, Aug 25, 2021 at 8:00 AM Ajin Cherian wrote:
> >
> > On Tue, Aug 24, 2021 at 11:12 PM Amit Kapila
> > wrote:
> >
> > > But will poll function still poll or exit? Have you tried tha
or ERROR" message, there is the
associated STATEMENT message with it.
First there is a LOG "received replication command" followed by the
STATEMENT and then the ERROR "replication slot .. is active.."
followed by the STATEMENT.
I will try with REL_11_STABLE and see if the behaviour is different.
regards,
Ajin Cherian
Fujitsu Australia
here
application_name = 'sub';
?column?
--
(0 rows)
> Also, it seems this failure happens on REL_11_STABLE branch, not sure
> if that matters, but it is better to use the same branch if you are
> using a different branch to reproduce the issue.
>
Ok, I didn't realise this.
l until the query
returns a 'true' (unless specified otherwise).
I don't see in my tests that the polled function exits if the query
returns a NULL. I don't know if differences in the installed perl can
cause this difference in
behaviour. Can a NULL set be construed as a true and cause the poll to exit?
Any suggestions?
regards,
Ajin Cherian
Fujitsu Australia
d_keep_alive && SyncRepEnabled();
>
> Note: Also, that assignment also deserves a big comment to say what it is
> doing.
>
> ~~
changed.
>
> 4b. Change the if/else =>
>
> If you make the change for 4a. then perhaps the keepalive if/else is
> overkill and could be changed.e.g.
>
> if (force_keep_alive_syncrep ||
> MyWalSnd->flush < sentPtr &&
> MyWalSnd->write < sentPtr &&
> !waiting_for_ping_response)
> WalSndKeepalive(false);
>
Changed.
regards,
Ajin Cherian
Fujitsu Australia
v14-0001-Skip-empty-transactions-for-logical-replication.patch
Description: Binary data
On Mon, Aug 2, 2021 at 7:20 PM Amit Kapila wrote:
>
> On Fri, Jul 23, 2021 at 3:39 PM Ajin Cherian wrote:
> >
>
> Let's first split the patch for prepared and non-prepared cases as
> that will help to focus on each of them separately. BTW, why haven't
> you consider
1 - 100 of 234 matches
Mail list logo
