ring DELETE/,
> > 'unpublished DELETE is filtered');
> >
> > $log_location = -s $node_publisher->logfile;
> > $node_publisher->safe_psql('postgres', "INSERT INTO delete_only_table
> > VALUES (1, 'to be deleted')");
> >
;. Am I missing
> something here?
> I have also added the same comment in point 1. in [2].
>
Fixed.
On Sat, Apr 5, 2025 at 12:34 AM Shlok Kyal wrote:
>
> On Thu, 20 Mar 2025 at 18:09, Ajin Cherian wrote:
> >
> > On Thu, Mar 13, 2025 at 5:49 PM Ajin Cherian wrote:
re a bug on HEAD that this is
solving? If so, can you write a test case to show this? This will make
it more convincing that this change as well is required. Your other
change has a test case to confirm, but this doesn't.
regards,
Ajin Cherian
Fujitsu Australia
#x27;t see an explanation as to why this change was added? Maybe I
missed it in the multiple threads. With this, we are no longer
decoding changes which were in the state SNAPBUILD_FULL_SNAPSHOT. Why
did that change?
regards,
Ajin Cherian
Fujitsu Australia
ges(ctx->reorder, xid,
buf->origptr);
+}
Any reason why you avoided calling SnapBuildProcessNewCid here and
only called ReorderBufferXidSetCatalogChanges? If any, please mention
in the comments the reason.
regards,
Ajin Cherian
Fujitsu Australia
On Thu, Mar 13, 2025 at 5:49 PM Ajin Cherian wrote:
>
> Moving this patch to the next CF as this patch needs more design level
> inputs which may not be feasible in this CF but do continue to review
> the patch.
>
> regards,
> Ajin Cherian
> Fujitsu Australia
Rebased t
Moving this patch to the next CF as this patch needs more design level
inputs which may not be feasible in this CF but do continue to review
the patch.
regards,
Ajin Cherian
Fujitsu Australia
On Mon, Mar 3, 2025 at 9:10 PM Ajin Cherian wrote:
>
> On Thu, Feb 20, 2025 at 8:42 PM Hayato Kuroda (Fujitsu)
> wrote:
> >
I confirmed with tests that with the patch, a lot less disk space is
used when there are lots of unpublished changes and the subscription
is not configured
On Mon, Mar 10, 2025 at 3:58 PM Ashutosh Bapat
wrote:
>
> On Thu, Mar 6, 2025 at 9:18 AM Ajin Cherian wrote:
>
> > + Subscription names, publication names, and replication slot names
> > are
> > + automatically generated. Cannot be used together
iptions (not template databases)
change to "Verify that only user databases have subscriptions"
regards,
Ajin Cherian
Fujitsu Australia
erhead of performing a hash table
> * search for each record, especially when most changes are not filterable.
> */
> -#define CHANGES_THRESHOLD_FOR_FILTER 100
> +#define CHANGES_THRESHOLD_FOR_FILTER 0
>
> Why is this defined as 0? Some accidental residue from performance
&g
On Tue, Feb 25, 2025 at 3:26 PM Ajin Cherian wrote:
>
> On Fri, Feb 21, 2025 at 2:24 PM Amit Kapila wrote:
> >
> > On Fri, Feb 21, 2025 at 7:57 AM Ajin Cherian wrote:
> > > In these tests, I also see an increased performance with the patch
> > > even when al
On Fri, Feb 21, 2025 at 2:24 PM Amit Kapila wrote:
>
> On Fri, Feb 21, 2025 at 7:57 AM Ajin Cherian wrote:
> > In these tests, I also see an increased performance with the patch
> > even when all transactions are published. I will investigate why this
> > happens and up
On Fri, Feb 21, 2025 at 12:57 PM Ajin Cherian wrote:
>
> On Thu, Feb 20, 2025 at 3:08 PM Hayato Kuroda (Fujitsu)
> wrote:
> >
> > Dear Ajin,
> >
> > > I compared the patch 1 which does not employ a hash cache and has the
> > > overhead of starting a
d all transactions on published
tables
Conclusion:
The patched code with 100 transaction throttling significantly
improves performance, reducing execution time by ~69% when no
published transactions are involved, ~43% with partial published
transactions, and ~15% in all published transactions.
Attach
On Thu, Feb 20, 2025 at 1:30 PM Ajin Cherian wrote:
>
> On Fri, Feb 14, 2025 at 6:18 PM Amit Kapila wrote:
> >
> > On Wed, Feb 12, 2025 at 10:41 AM Ajin Cherian wrote:
> > >
> > > On Wed, Jan 29, 2025 at 9:31 AM Peter Smith wrote:
> > >
> >
ottling. I
think we need a way for the user to set this threshold via a GUC and
that can be used for testing.
regards,
Ajin Cherian
Fujitsu Australia
v14-0001-Filter-transactions-that-need-not-be-published.patch
Description: Binary data
v14-0002-Introduce-a-output-plugin-callback-to-filter-cha.patch
Description: Binary data
f concurrent aborts.
2.
I think we can extend the skip mechanism to
UPDATE/DELETE/MultiInsert/SpecConfirm.
Regarding the TRUNCATE, I'm not sure we can handle hte TRUNCATE case
because the we
can't track RelFileLocator anymore.
Updated.
On Tue, Feb 4, 2025 at 2:19 PM vignesh C wrote:
is is the second attempt)
>
>
>
Thanks Hou-san,
Here's a patch-set created based on the design changes proposed by Hou-san.
P.S: It has reached v12 after internal review/rework iterations
Regards,
Ajin Cherian
Fujitsu Australia
v12-0001-Track-transactions-with-internal-snap
pared transactions will be replicated at
COMMIT PREPARED.");
}
I think this code needs to be updated as well.
regards,
Ajin Cherian
Fujitsu Australia
On Fri, Jan 10, 2025 at 9:08 PM Ajin Cherian wrote:
>
> On Fri, Dec 27, 2024 at 5:36 PM Shubham Khanna
> wrote:
> >
> The patch no longer applies on HEAD. Please do rebase.
>
Sorry, I was mistaken. Ignore this. The patch does apply on HEAD.
regards,
Ajin Cherian
On Fri, Dec 27, 2024 at 5:36 PM Shubham Khanna
wrote:
>
The patch no longer applies on HEAD. Please do rebase.
regards,
Ajin Cherian
Fujitsu Australia
On Mon, Nov 4, 2024 at 2:20 PM Amit Kapila wrote:
>
> On Thu, Oct 31, 2024 at 4:26 PM Ajin Cherian wrote:
> >
> > 5. Verified that publications with different column list are disallowed to
> > be subscribed by one subscription
> >a. PUB_A(column list = (a,
On Thu, Oct 31, 2024 at 9:55 PM Ajin Cherian wrote:
> I ran some tests and verified that the patch works with previous versions
> of PG12 and PG17
> 1. Verified with publications with generated columns and without generated
> columns on patched code and subscriptions on P
olumn list, without publish_generated_column) PUB_B(no
column list, with publish_generated_column) - FAIL
Tests did not show any unexpected behaviour.
regards,
Ajin Cherian
Fujitsu Australia
Can we change this to use the new foreach_ptr implementations added:
> + foreach(lc, stmtresolvers)
> + {
> + DefElem*defel = (DefElem *) lfirst(lc);
> + ConflictType type;
> + char *resolver;
>
> to use foreach_ptr like:
> foreach_ptr(DefElem, defel, stmtresolvers)
> {
> + ConflictType type;
> + char *resolver;
>
> }
>
Changed accordingly.
regards,
Ajin Cherian
Fujitsu Australia
t;
> I am not sure if we need to dump default resolvers. Would like to know
> what others think on this.
>
> 3)
> Why in 002_pg_dump.pl we have default resolvers set explicitly?
>
> In 003_pg_dump.pl, default resolvers are not set explicitly, that is the
regexp to check the pg_dum
alculation of resource limits really required here when the same
is already done inside InvalidateObsoleteReplicationSlots()
regards,
Ajin Cherian
Fujitsu Australia
n. This has been the
> approach until v40 patch.
> b) Checkpointer and/or other backends add an autovacuum work item via
> AutoVacuumRequestWork(), and autovacuum when it gets to it will
> invalidate the replication slots. But, what to do for the vacuum
> command here?
>
> Please find the attached v41 patches implementing the idea of vacuum
> doing the invalidation.
>
> Thoughts?
>
> Thanks to Sawada-san for a detailed off-list discussion.
>
The patch no longer applies on HEAD, please rebase.
regards,
Ajin Cherian
Fujitsu Australia
1)
2024-07-01 02:52:59.427 EDT [20304] CONTEXT: processing remote data for
replication origin "pg_16417" during message type "COMMIT" in transaction
763, finished at 0/15E7F68
2024-07-01 02:52:59.427 EDT [20304] WARNING: resource was not closed:
TupleDesc 0x7f8c0439e448 (16402,-1)
On Wed, May 22, 2024 at 2:17 PM Ajin Cherian wrote:
>
> The reason for the crash is that the RelationSyncCache was NULL prior to
> reaching a consistent point.
> Hi li jie, I see that you created a new thread with an updated version of
> this patch [1]. I used that patch and addr
On Tue, Mar 19, 2024 at 1:49 PM Ajin Cherian wrote:
>
>
>> Of course you can, but this will only convert disk space into memory
>> space.
>> For details, please see the case in Email [1].
>>
>> [1]
>> https://www.postgresql.org/message-id/CAGfChW51P944
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_pha
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 K
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
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
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
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
Lock held. Maybe do
this prior to acquiring the spinlock.
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
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
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
, it doesn't fit
well.
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
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
<>
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
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
ithout calling that function. This requires connecting to a database.
regards,
Ajin Cherian
Fujitsu Australia
blank in 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
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
_syncslot = true so they can receive
failover logical slots changes from the primary.
regards,
Ajin Cherian
Fujitsu Australia
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
n_slot('lsub2_slot', 'pgoutput', false, 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
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
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
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
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
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
, 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
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
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
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
Then we took
> > 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
objid)
> + return true;
>
> The comment seems wrong/confused – "Only send this ddl if we don't
> publish ddl message" (??)
>
fixed.
> ==
> src/bin/pg_dump/pg_dump.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
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
*) 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
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
Create Publication",
maybe in future
we'll consider adding it as part of replication slot parameters.
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
ide, then converts these messages to actual
DDL commands and executes them.
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
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
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&
e same commit
messages as previously present.
regards,
Ajin Cherian
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
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
in one of the existing test
> 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
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 second
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
ng and resetting our own context */
> 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 n
worker
as it tries to find the non-existent 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
>output_plugin_private = NULL;
> +
> + /* If no changes were part of this transaction then drop the commit */
> + if (!sent_begin_txn)
> + {
> + elog(DEBUG1, "Skipping replication of an empty transaction in stream
> commit");
> + return;
> + }
>
> (Same as
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 noted
t to break backward compatibility for this small
added optimization.
Amit,
I will work on your comments.
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
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
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
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.
&g
1 - 100 of 266 matches
Mail list logo
