a subscription 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
_syncslot = true so they can receive
failover logical slots changes from the primary.
regards,
Ajin Cherian
Fujitsu Australia
is function 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
check the
+ * transaction status 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
he 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
ilover != failover) {
MyReplicationSlot->data.failover = failover;
}
if (MyReplicationSlot->data.inactive_timeout != inactive_timeout) {
MyReplicationSlot->data.inactive_timeout = inactive_timeout;
}
if (lock_acquired) {
SpinLockRelease(&MyReplicationSlot->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
Lock held. Maybe do
this prior to acquiring the spinlock.
regards,
Ajin Cherian
Fujitsu Australia
herwise return false.
we 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
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
r.
2. No solution 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
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
blank in the generated primary_conninfo string as well.
regards,
Ajin Cherian
Fujitsu Australia
ithout calling that function. This requires connecting to a database.
regards,
Ajin Cherian
Fujitsu Australia
time 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
ROM test WHERE v = pg_backend_pid();
INSERT INTO test(v) SELECT pg_backend_pid();
PREPARE TRANSACTION $$:mygid$$;
COMMIT PREPARED $$:mygid$$;
regards,
Ajin Cherian
Fujitsu Australia
dback=on, standby_slot_names not configured, logical
subscriber not 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
<>
Run2: 627.924183 secs
There was no degradation in performance seen.
Thanks Nisha for helping with the testing.
regards,
Ajin Cherian
Fujitsu Australia
, it doesn't fit
well.
regards,
Ajin Cherian
Fujitsu Australia
t; Missing space before "Note"
>
Fixed.
> ~~~
>
> 22. LogLogicalDDLMessage
>
> + /*
> + * Ensure we have a valid transaction id.
> + */
> + Assert(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, &change->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
*) lfirst(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 C
test should not have access to the
table admin_audit. This means 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
On Tue, Apr 5, 2022 at 8:32 PM Amit Kapila wrote:
>
> How about something like the attached?
>
LGTM.
regards,
Ajin Cherian
Fujitsu Australia
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
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
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
ches 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-som
e same commit
messages as previously present.
regards,
Ajin Cherian
r (widx = 0; widx < max_slotsync_workers; widx++)
> + {
> + SlotSyncWorker *worker = &LogicalRepCtx->ss_workers[widx];
> +
> + if (worker->hdr.in_use && !worker->dbcount)
> + slotsync_worker_stop_internal(worker);
> + }
>
> Is it safe 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
Context will not allow us to change the slot'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
27; flag by adding
another 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
, 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
validated 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 search
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
c = MyProc;
+
+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
plicate reindex_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
of the 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
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 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.
&g
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:
> > >
> &
LUES(i,'BAH', row_to_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 A
0); -- 25% allowed
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
e attachment).
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
t to break backward compatibility for this small
added optimization.
Amit,
I will work on your comments.
regards,
Ajin Cherian
Fujitsu Australia
s 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
ide, then converts these messages to actual
DDL commands and executes them.
regards,
Ajin Cherian
Fujitsu Australia
anding ?
>
> thanks
> 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
Create Publication",
maybe in future
we'll consider adding it as part of replication slot parameters.
regards,
Ajin Cherian
Fujitsu Australia
ansactions are not
sent but also does call OutputPluginUpdateProgress when an empty
transaction is not sent, as a result the confirmed_flush_lsn is kept
moving. I also see no hangs when synchronous_standby is configured.
Do let me know your thoughts on this patch.
regards,
Ajin Cherian
Fujitsu Australia
v
On Thu, Apr 15, 2021 at 1:29 PM Ajin Cherian wrote:
>
> I've rebased the patch and made changes so that the patch supports
> "streaming in-progress transactions" and handling of logical decoding
> messages (transactional and non-transactional).
> I see that this
) on why
the redo, it's not very obvious
to someone reading the code, why we are refetching the index list here.
+ /* Check if we need to redo */
+ newindexoidlist = RelationGetIndexList(relation);
thanks,
Ajin Cherian
Fujitsu Australia
n->xid;
>
> +/* output BEGIN if we haven't yet, avoid for streaming and
> non-transactional messages */
> +if (!data->xact_wrote_changes && !in_streaming && transactional)
> + {
> + pgoutput_begin(ctx, txn);
> + data->xact_wrote_changes = true;
> + }
>
> (same comment as above)
>
> If the variable is renamed as previously suggested then the assignment
> data->sent_BEGIN_txn = true; can be assigned in just 1 common place
> INSIDE the pgoutput_begin function.
>
> --
>
Fixed.
>
> 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?
>
>
The 020_messages.pl actually has a test case for tracking empty messages
even though it is part of the messages test.
regards,
Ajin Cherian
Fujitsu Australia
v4-0001-Skip-empty-transactions-for-logical-replication.patch
Description: Binary data
thing more is required to keep empty transactions
updated as part of synchronous replicas. If my understanding on this
is not correct, let me know.
regards,
Ajin Cherian
Fujitsu Australia
n_txn && !in_streaming)
> + {
> + pgoutput_begin(ctx, txn);
> + }
>
> @@ -725,6 +762,12 @@ pgoutput_message(LogicalDecodingContext *ctx,
> ReorderBufferTXN *txn,
> if (in_streaming)
> xid = txn->xid;
>
> + /* output BEGIN if we haven't yet, avoid
delta, mtime)
VALUES (:tid, :bid, :aid, :delta, CURRENT_TIMESTAMP);\n"
"PREPARE TRANSACTION ':aid:';\n"
"COMMIT PREPARED ':aid:';\n"
The tests ran fine and all 4 cascaded servers replicated the changes
correctly. All the subscriptions were configured with two_phase
enabled.
regards,
Ajin Cherian
Fujitsu Australia
On Tue, Apr 27, 2021 at 1:49 PM Ajin Cherian wrote:
Rebased the patch as it was no longer applying.
regards,
Ajin Cherian
Fujitsu Australia
v6-0001-Skip-empty-transactions-for-logical-replication.patch
Description: Binary data
On Thu, May 27, 2021 at 11:20 AM tanghy.f...@fujitsu.com
wrote:
>
> On Wed, May 26, 2021 10:13 PM Ajin Cherian wrote:
> >
> > I've attached a patch that fixes this issue. Do test and confirm.
> >
>
> Thanks for your patch.
> I have tested and confirmed th
for TRANSFORM.
>
Thanks for the new patch-set.
Could 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
e the change we removed as part of (a).
Attaching two patches:
1. Removes two-phase from CreateReplicationSlotCmd
2. Adds two-phase option in CREATE_REPLICATION_SLOT command.
I will send a patch to update pg_recvlogical next week.
regards,
Ajin Cherian
Fujitsu Australia
v1-0001-Remove-two-ph
On Mon, Jun 7, 2021 at 3:17 PM Amit Kapila wrote:
> Pushed the above patch.
Here's an updated patchset that adds back in the option for two-phase
in CREATE_REPLICATION_SLOT command and a second patch that adds
support for
two-phase decoding in pg_recvlogical.
regards,
Ajin Cherian
On Wed, Jun 9, 2021 at 6:23 AM Jeff Davis wrote:
>
> On Tue, 2021-06-08 at 17:41 +1000, Ajin Cherian wrote:
> > Here's an updated patchset that adds back in the option for two-phase
> > in CREATE_REPLICATION_SLOT command and a second patch that adds
> > support
On Wed, Jun 9, 2021 at 9:57 PM Amit Kapila wrote:
>
> On Wed, Jun 9, 2021 at 4:50 PM Amit Kapila wrote:
> >
> > On Wed, Jun 9, 2021 at 1:53 AM Jeff Davis wrote:
> > >
> > > On Tue, 2021-06-08 at 17:41 +1000, Ajin Cherian wrote:
> > > > Here's
On Fri, Jun 11, 2021 at 8:14 PM Amit Kapila wrote:
>
> On Thu, Jun 10, 2021 at 2:04 PM Ajin Cherian wrote:
> >
>
> The new patches look mostly good apart from the below cosmetic issues.
> I think the question is whether we want to do these for PG-14 or
> postpone them t
>safe_psql('postgres',
> + "BEGIN;INSERT INTO test_table values (11); PREPARE TRANSACTION 'test'");
>
> There is no space after BEGIN but there is a space after INSERT. For
> consistency-sake, I will have space after BEGIN as well.
Changed this.
regards,
Ajin Cherian
Fujitsu Australia
v5-0001-Add-option-to-set-two-phase-in-CREATE_REPLICATION.patch
Description: Binary data
v5-0002-Add-support-for-two-phase-decoding-in-pg_recvlogi.patch
Description: Binary data
tab-complete
has broken it.
regards,
Ajin Cherian
Fujitsu Australia
On Tue, Jun 15, 2021 at 5:34 PM Ajin Cherian wrote:
Since we've decided to not commit this for PG-14, I've added these
patches along with the larger patch-set for
subscriber side 2pc in thread [1]
[1] -
https://www.postgresql.org/message-id/cahut+pujktnrjfre0vbufwmz9bescc__nt+pu
ationTypeTupleValue():
+ tup = heap_modify_tuple(tup, RelationGetDescr(rel), values, nulls,
+ replaces);
+
+ /* Update the catalog. */
+ CatalogTupleUpdate(rel, &tup->t_self, tup);
Not sure if this tup needs to be freed or if the memory context will
take care of it.
=
regards,
Ajin Cherian
Fujitsu Australia
care of it.
>
> I felt this is ok, as the cleanup is handled in the caller function
> "AlterPublication", thoughts?
> /* Cleanup. */
> heap_freetuple(tup);
> table_close(rel, RowExclusiveLock);
that should be fine.
regards,
Ajin Cherian
Fujitsu Australia
On Tue, Jun 29, 2021 at 4:56 PM Amit Kapila wrote:
>
> On Wed, Jun 23, 2021 at 4:10 PM Ajin Cherian wrote:
> >
>
> The first two patches look mostly good to me. I have combined them
> into one and made some minor changes. (a) Removed opt_two_phase and
> related code fro
The patch looks good to me.
regards,
Ajin Cherian
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&
is worth mentioning:
Skip empty transactions for logical replication.
commit d5a9d86d8ffcadc52ff3729cd00fbd83bc38643c
https://github.com/postgres/postgres/commit/d5a9d86d8ffcadc52ff3729cd00fbd83bc38643c
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
> > &
e 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 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
nd walsender clears 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 Cheri
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:
27;postgres',
> "SELECT pid FROM pg_stat_replication WHERE application_name =
> 'tap_sub';"
> );
> $node_subscriber->safe_psql('postgres',
> "ALTER SUBSCRIPTION tap_sub SET PUBLICATION tap_pub_ins_only
> WITH (copy_data = fal
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:
>
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
y seem to be working fine.
No review comments, the patch looks good to me.
regards,
Ajin Cherian
Fujitsu Australia
oesn'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
PDATE
old-row (match) new-row (no match) -> 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
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
is case.
There 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 th
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
and
the apply worker 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 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 w
b-sub [1].
Also I've modified the patch to also skip replicating empty prepared
transactions. Do let me know if you have any comments.
regards,
Ajin Cherian
Fujitsu Australia
[1]-
https://www.postgresql.org/message-id/CAHut+PueG6u3vwG8DU=JhJiWa2TwmZ=bdqpchzkbky7ykza...@mail.gmail.com
v7-0001-Skip-empty-transactions-for-logical-replication.patch
Description: Binary data
t; + if (rbtxn_prepared(txn))
> + pgoutput_begin_prepare(ctx, txn);
> + else
> + pgoutput_begin(ctx, txn);
> + }
> +
>
> 13.c =>
>
> If you introduce the variable (as suggested in 13a) this code becomes
> much simpler:
>
Skipped this. (reason mentioned above)
>
n of an empty transaction");
Fixed this.
I've addressed these comments in version 8 of the patch.
regards,
Ajin Cherian
Fujitsu Australia
t; (NOTE: This same review comment applies in at least 3 places in this
> patch, so please hunt them all down)
>
Updated.
> --
>
> 7. src/backend/replication/pgoutput/pgoutput.c - comment wording
>
> @@ -677,6 +810,18 @@ pgoutput_change(LogicalDecodingContext *ct
On Fri, Jul 23, 2021 at 10:26 AM Greg Nancarrow wrote:
>
> On Thu, Jul 22, 2021 at 11:37 PM Ajin Cherian wrote:
> >
>
> I have some minor comments on the v9 patch:
>
> (1) Several whitespace warnings on patch application
>
Fixed.
> (2) Suggested patch comment cha
uot;messsage" --> "message"
>
> (NOTE this same typo is in 2 places)
>
Fixed.
I have made these changes in v10 of the patch.
regards,
Ajin Cherian
Fujitsu Australia
* This make it possible to skip transactions that are empty.
> + */
>
> =>
>
> typo: "make it possible" --> "makes it possible"
>
fixed.
regards,
Ajin Cherian
Fujitsu Australia
v11-0001-Skip-empty-transactions-for-logical-replication.patch
Description: Binary data
ure. It could also be due to a planned dropping
of subscription or disable of subscription. I have not tested this but
won't the failed stats be updated in this case as well? Is that
correct?
regards,
Ajin Cherian
Fujitsu Australia
t do you think?
I agree with this analysis. The test needs to wait for both
subscriptions to catch up.
Attached is a patch that addresses this issue.
regards,
Ajin Cherian
Fujitsu Australia
v1-0001-Fix-possible-failure-in-021_twophase-tap-test.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.
As a first shot, I have split t
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 c
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
ot an expert in perl but I looked at the perl function used in
the tap test poll_query_until(), this would poll 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
101 - 200 of 294 matches
Mail list logo
