, so having an option seems reasonable. But I
suggest to keep this as a separate last patch. If we can make the core
idea work by default then we can enable it via option in the end.
--
With Regards,
Amit Kapila.
On Fri, Sep 27, 2024 at 10:24 PM Masahiko Sawada wrote:
>
> On Fri, Sep 27, 2024 at 12:39 AM Shlok Kyal wrote:
> >
> > On Mon, 23 Sept 2024 at 09:59, Amit Kapila wrote:
> > >
> > > On Sun, Sep 22, 2024 at 11:27 AM David Rowley
> > > wrote:
> &
On Sun, Sep 29, 2024 at 6:50 AM Bruce Momjian wrote:
>
> On Thu, Sep 26, 2024 at 03:08:52PM +0530, Amit Kapila wrote:
> > On Sat, Sep 21, 2024 at 1:50 AM Bruce Momjian wrote:
> > >
> > > On Fri, Sep 20, 2024 at 04:05:11PM -0400, Tom Lane wrote:
> > > &
cluster," as it may confuse users in other types
> of logical replication with questions such as: a) Which subscriber
> should be upgraded first? b) Which subscriptions should be disabled?
> c) When should each subscription be enabled?
> The attached patch includes a note for the sam
6.blogspot.com/2024/09/online-upgrading-logical-and-physical.html
--
With Regards,
Amit Kapila.
oogle.internal iburst
> ```
>
If NTP already provides a way to configure other time-sync services as
shown by you then I don't think we need to do more at this stage
except to document it with the conflict resolution patch. In the
future, we may want to provide an additional column in the table with
a special meaning that can help in conflict resolution.
--
With Regards,
Amit Kapila.
st
but one can upgrade the subscriber first as well. If so, we can add a
note to the documentation.
--
With Regards,
Amit Kapila.
On Tue, Sep 24, 2024 at 4:20 PM Amit Kapila wrote:
>
> On Fri, Sep 20, 2024 at 5:46 PM vignesh C wrote:
> >
> > I didn’t include a note because each disable/enable statement
> > specifies: a) Disable all subscriptions on the node, b) Enable all
> > subscriptions
7;s upgrade
following these steps have similar problems.
--
With Regards,
Amit Kapila.
On Tue, Sep 24, 2024 at 9:02 AM Zhijie Hou (Fujitsu)
wrote:
>
> On Tuesday, September 24, 2024 5:05 AM Masahiko Sawada
> wrote:
> >
> > Thank you for considering another idea.
>
> Thanks for reviewing the idea!
>
> >
> > On Fri, Sep 20, 2024 at 2:46
ses we can avoid sending messages to the
publisher because with this we only need to send message to a
particular publisher once rather than by all the apply workers
corresponding to same publisher node.
--
With Regards,
Amit Kapila.
ify it.
One possible approach is to extend the syntax of
"synchronized_standby_slots" similar to "synchronous_standby_names" so
that users can directly specify slots similarly and avoid waiting for
more than required standbys.
--
With Regards,
Amit Kapila.
egrity
issues.")
[1] -
https://dev.mysql.com/doc/refman/9.0/en/mysql-cluster-replication-schema.html#ndb-replication-ndb-replication
[2] -
https://docs.oracle.com/en/database/other-databases/timesten/22.1/replication/configuring-timestamp-comparison.html#GUID-C8B0580B-B577-435F-8726-4AF341A09806
[3] -
https://www.oracle.com/cn/a/tech/docs/technical-resources/wp-oracle-goldengate-activeactive-final2-1.pdf
--
With Regards,
Amit Kapila.
On Fri, Sep 20, 2024 at 10:53 PM Masahiko Sawada wrote:
>
> On Thu, Sep 19, 2024 at 10:46 PM Amit Kapila wrote:
> >
> > On Fri, Sep 20, 2024 at 5:13 AM David Rowley wrote:
> > >
> > > On Fri, 20 Sept 2024 at 05:03, Masahiko Sawada
> > > wrote:
>
On Sun, Sep 22, 2024 at 11:27 AM David Rowley wrote:
>
> On Fri, 20 Sept 2024 at 17:46, Amit Kapila wrote:
> >
> > On Fri, Sep 20, 2024 at 5:13 AM David Rowley wrote:
> > > In general, it's a bit annoying to have to code around this
> > > GenerationC
On Mon, Sep 23, 2024 at 4:10 AM Peter Smith wrote:
>
> On Fri, Sep 20, 2024 at 2:26 PM Amit Kapila wrote:
> >
> > On Fri, Sep 20, 2024 at 4:16 AM Peter Smith wrote:
> > >
> > > On Fri, Sep 20, 2024 at 3:26 AM Masahiko Sawada
> > > wrote:
> &g
On Sat, Sep 21, 2024 at 3:19 AM Masahiko Sawada wrote:
>
> On Thu, Sep 19, 2024 at 9:26 PM Amit Kapila wrote:
> >
> > >
> > > OK. Let me give some examples below to help understand this idea.
> > >
> > > Please correct me if
n write steps for one and add a
note to say it has to be done for other subscriptions present.
--
With Regards,
Amit Kapila.
auncher requests one of the apply
workers corresponding to subscriptions pointing to the same publisher
node to get this information; (b) launcher launches another worker to
get the remote wal flush location.
--
With Regards,
Amit Kapila.
holds in that
> struct that could be filled without enlarging the struct.
>
> In general, it's a bit annoying to have to code around this
> GenerationContext fragmentation issue.
>
Right, and I am also slightly afraid that this may not cause some
regression in other cases where defrag wouldn't help.
--
With Regards,
Amit Kapila.
On Thu, Sep 19, 2024 at 10:33 PM Masahiko Sawada wrote:
>
> On Wed, Sep 18, 2024 at 8:55 PM Amit Kapila wrote:
> >
> > On Thu, Sep 19, 2024 at 6:46 AM David Rowley wrote:
> > >
> > > On Thu, 19 Sept 2024 at 11:54, Masahiko Sawada
> > > wrote:
>
patch as per your suggestion.
--
With Regards,
Amit Kapila.
On Fri, Sep 20, 2024 at 4:16 AM Peter Smith wrote:
>
> On Fri, Sep 20, 2024 at 3:26 AM Masahiko Sawada wrote:
> >
> > On Thu, Sep 19, 2024 at 2:32 AM Amit Kapila wrote:
> > >
> > >
> > > Users can use a publication like "create publication pub
d results/grief.
> >
> > It gives a WARNING, and then publishes the specified generated column
> > data (even if publish_generated_column = false)?
I think that the column list should take priority and we should
publish the generated column if it is mentioned in irrespective of
the option.
> > If so, it would mean
> > that specifying the generated column to the column list means to
> > publish its data regardless of the publish_generated_column parameter
> > value.
> >
>
> No. I meant only it can give the WARNING to tell the user user "Hey,
> there is a conflict here because you said publish_generated_column=
> false, but you also specified gencols in the column list".
>
Users can use a publication like "create publication pub1 for table
t1(c1, c2), t2;" where they want t1's generated column to be published
but not for t2. They can specify the generated column name in the
column list of t1 in that case even though the rest of the tables
won't publish generated columns.
--
With Regards,
Amit Kapila.
On Thu, Aug 29, 2024 at 8:44 AM Masahiko Sawada wrote:
>
> On Wed, Aug 28, 2024 at 1:06 AM Amit Kapila wrote:
> >
> > >
> > > As Euler mentioned earlier, I think it's a decision not to replicate
> > > generated columns because we don't know the
On Tue, Sep 17, 2024 at 9:08 AM shveta malik wrote:
>
> On Mon, Sep 16, 2024 at 4:04 PM Amit Kapila wrote:
> >
> > On Mon, Sep 16, 2024 at 2:55 PM shveta malik wrote:
> > >
> > > On Mon, Sep 16, 2024 at 11:13 AM Amit Kapila
> > > wrote:
> >
Let's see what others think.
>
Displaying the 'text' for the state column makes it easy to
understand. So, +1 for doing it that way.
--
With Regards,
Amit Kapila.
problem, however, we can use that or some other constant
to decide the point of defragmentation. The other point we need to
think in this idea is whether we actually need any defragmentation at
all. This will depend on whether there are concurrent transactions
being decoded. This would require benchmarking to see the performance
impact.
--
With Regards,
Amit Kapila.
On Mon, Sep 16, 2024 at 10:41 PM Bharath Rupireddy
wrote:
>
> Thanks for looking into this.
>
> On Mon, Sep 16, 2024 at 4:54 PM Amit Kapila wrote:
> >
> > Why raise the ERROR just for timeout invalidation here and why not if
> > the slot is invalidated for other rea
On Tue, Sep 17, 2024 at 11:24 PM Masahiko Sawada wrote:
>
> On Mon, Sep 16, 2024 at 11:53 PM Amit Kapila wrote:
> >
> > On Tue, Sep 17, 2024 at 6:08 AM Masahiko Sawada
> > wrote:
> >
> > I haven't thought about the implementation details yet but I
On Mon, Sep 16, 2024 at 10:43 PM Masahiko Sawada wrote:
>
> On Fri, Sep 13, 2024 at 3:58 AM Amit Kapila wrote:
> >
> > On Thu, Sep 12, 2024 at 4:03 AM Masahiko Sawada
> > wrote:
> > >
> > > We have several reports that logical decoding uses memory much
On Tue, Sep 17, 2024 at 6:08 AM Masahiko Sawada wrote:
>
> On Fri, Sep 13, 2024 at 12:56 AM shveta malik wrote:
> >
> > On Fri, Sep 13, 2024 at 11:38 AM Amit Kapila
> > wrote:
> > >
> > > > >
> > > > > So in brief, this solution i
ext() which can raise the new ERROR. I am not sure
about how the invalid slots are handled during physical replication,
please check the behavior of that before this patch.
--
With Regards,
Amit Kapila.
On Mon, Sep 16, 2024 at 2:55 PM shveta malik wrote:
>
> On Mon, Sep 16, 2024 at 11:13 AM Amit Kapila wrote:
> >
>
> > Another question aside from the above point, what if someone has
> > specified logical subscribers in synchronous_standby_names? In the
> > case
gt;
> If we don't do something similar, then aren't there chances that we
> keep on waiting on the wrong lsn[mode] for the case when mode =
> SYNC_REP_WAIT_APPLY while sync-rep-wait infrastructure is updating
> different mode's lsn. Is my understanding correct?
>
I think here we always need the lsn values corresponding to
SYNC_REP_WAIT_FLUSH as we want to ensure that the WAL has to be
flushed on physical standby before sending it to the logical
subscriber. See ProcessStandbyReplyMessage() where we always use
flushPtr to advance the physical_slot via
PhysicalConfirmReceivedLocation().
Another question aside from the above point, what if someone has
specified logical subscribers in synchronous_standby_names? In the
case of synchronized_standby_slots, we won't proceed with such slots.
--
With Regards,
Amit Kapila.
On Tue, Sep 10, 2024 at 12:13 AM Bharath Rupireddy
wrote:
>
> On Mon, Sep 9, 2024 at 3:04 PM Amit Kapila wrote:
> >
> > > > > > We should not allow the invalid replication slot to be altered
> > > > > > irrespective of the reason unless there
the same time, I think it would not be a problem.
> Checking performance degradation due to using many memory contexts
> would have to be done.
>
The generation context has been introduced in commit a4ccc1ce which
claims that it has significantly reduced logical decoding's memory
usage and improved its performance. Won't changing it requires us to
validate all the cases which led us to use generation context in the
first place?
--
With Regards,
Amit Kapila.
On Thu, Sep 12, 2024 at 4:24 PM Amit Kapila wrote:
>
> On Wed, Sep 11, 2024 at 8:41 AM shveta malik wrote:
> >
> > On Tue, Sep 10, 2024 at 11:24 PM Bharath Rupireddy
> > wrote:
> > >
> > >
> > > Please find the attached v2 patch also having Sh
WrP789KnKxZydisHajd38rSihWXO8MVBLDwxG1Kg%40mail.gmail.com
[2] -
BEGIN
DBMS_GOLDENGATE_ADM.ALTER_AUTO_CDR(
schema_name => 'hr',
table_name=> 'employees',
tombstone_deletes => TRUE);
END;
/
[3] - https://en.wikipedia.org/wiki/Tombstone_(data_store)
[4] -
https://docs.oracle.com/en/middleware/goldengate/core/19.1/oracle-db/automatic-conflict-detection-and-resolution1.html#GUID-423C6EE8-1C62-4085-899C-8454B8FB9C92
[5] -
https://www.postgresql.org/message-id/e4cdb849-d647-4acf-aabe-7049ae170fbf%40enterprisedb.com
--
With Regards,
Amit Kapila.
me.
>
LGTM as well. I'll push this tomorrow morning unless there are more
comments or suggestions.
--
With Regards,
Amit Kapila.
On Wed, Sep 11, 2024 at 8:32 AM Zhijie Hou (Fujitsu)
wrote:
>
> On Tuesday, September 10, 2024 7:25 PM Amit Kapila
> wrote:
> >
> > One minor comment on 0003
> > ===
> > 1.
> > get_slot_confirmed_flush()
> > {
> > ...
>
On Tue, Sep 10, 2024 at 8:56 PM Bertrand Drouvot
wrote:
>
> On Mon, Sep 09, 2024 at 04:24:09PM +0530, Amit Kapila wrote:
> > On Fri, Aug 30, 2024 at 5:18 PM Bertrand Drouvot
> > wrote:
> > > as we decided not to expose the SnapBuildOnDisk and SnapBuild structs to
On Tue, Sep 10, 2024 at 2:16 PM Amit Kapila wrote:
>
> On Tue, Sep 10, 2024 at 11:36 AM vignesh C wrote:
> >
> > On Mon, 9 Sept 2024 at 13:12, Amit Kapila wrote:
> > >
> > > The second part of the assertion is incomplete. The
> > > IsIndexUsableForR
ck outside of the loop.
+ */
+ LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
+
+ foreach_ptr(String, name, MySubscription->feedback_slots)
+ {
+ XLogRecPtr confirmed_flush;
+ ReplicationSlot *slot;
+
+ slot = ValidateAndGetFeedbackSlot(strVal(name));
Why do we need to validate slots each time here? Isn't it better to do it once?
--
With Regards,
Amit Kapila.
On Tue, Sep 10, 2024 at 11:36 AM vignesh C wrote:
>
> On Mon, 9 Sept 2024 at 13:12, Amit Kapila wrote:
> >
> > The second part of the assertion is incomplete. The
> > IsIndexUsableForReplicaIdentityFull() should be used only when the
> > remote relation has REPLIC
ter,
>
Agreed, I could see a similar case with a message ("cannot alter
invalid database \"%s\"") in the code. Additionally, we should also
include Shveta's suggestion to change the detailed message to other
similar messages in logical.c
--
With Regards,
Amit Kapila.
ven
+ * if other publications have a list).
*/
- if (!pub->alltables)
+ if (!pub->alltables || !pub->pubgencolumns)
Why do we treat pubgencolumns at the same level as the FOR ALL TABLES
case? I thought that if the user has provided a column list, we only
need to publish the specified columns even when the
publish_generated_columns option is set.
--
With Regards,
Amit Kapila.
On Fri, Aug 30, 2024 at 5:18 PM Bertrand Drouvot
wrote:
>
> On Fri, Aug 30, 2024 at 03:43:12PM +0530, Amit Kapila wrote:
> > On Thu, Aug 29, 2024 at 6:33 PM Bharath Rupireddy
> > wrote:
> > >
> > > On Thu, Aug 29, 2024 at 3:44 PM Bertrand Drouvot
> >
ps, track it in a separate thread?
>
> I think so. It does not come under the scope of this thread.
>
It makes sense to me as well. But let's go ahead and get that sorted out first.
--
With Regards,
Amit Kapila.
On Mon, Sep 9, 2024 at 11:44 AM Dilip Kumar wrote:
>
> On Fri, Sep 6, 2024 at 4:48 PM vignesh C wrote:
> >
> > On Mon, 2 Sept 2024 at 18:22, Dilip Kumar wrote:
> > >
> > > On Mon, Sep 2, 2024 at 3:32 PM Amit Kapila
> > > wrote:
> > >
# Alter Publication pub1 drop table delete_test;
ALTER PUBLICATION
postgres=*# Delete from delete_test where id=0;
DELETE 1
postgres=*# commit;
COMMIT
postgres=# select * from delete_test;
id | name
+--
(0 rows)
Subscriber:
=
postgres=# select * from delete_test;
id | name
+---
0 | Nitin
(1 row)
This can also happen when the user has published only 'inserts' but
not 'updates' or 'deletes'.
--
With Regards,
Amit Kapila.
as well just to discuss the clock skew problem
w.r.t resolution strategies like "last_write_wins" strategy. So, we
can discuss clock skew in that thread and keep the focus of this
thread LSN<->timestamp inversion problem.
[1] -
https://www.postgresql.org/message-id/CAJpy0uBWBEveM8LO2b7wNZ47raZ9tVJw3D2_WXd8-b6LSqP6HA%40mail.gmail.com
[2] -
https://www.postgresql.org/message-id/CAJpy0uD0-DpYVMtsxK5R%3DzszXauZBayQMAYET9sWr_w0CNWXxQ%40mail.gmail.com
--
With Regards,
Amit Kapila.
not alter replication slot "mysubnew1_1": ERROR: can no
> longer get changes from replication slot "mysubnew1_1"
> DETAIL: The slot became invalid because it was inactive since
> 2024-09-04 08:54:20.308996+05:30, which is more than 0 seconds ago.
> HINT: You mi
e since
> 2024-09-04 10:06:38.980939+05:30, which is more than 129600 seconds
> ago.
> postgres=# select now();
>now
> ------
> 2024-09-04 10:07:35.201894+05:30
>
> I feel we should change this message itself.
>
+1.
--
With Regards,
Amit Kapila.
em, it won't matter to us. Now, we can discuss whether
"last_write_wins" is a poor strategy or not but if possible, for the
sake of the point of this thread, let's assume that users using the
resolution feature ("last_write_wins") ensure that clocks are synced
or they won't enable this feature and then see if we can think of any
problem w.r.t the current code.
--
With Regards,
Amit Kapila.
t; gets accepted then later we can revisit this, and put all the removed
> extra test cases back in again.
>
I am not convinced that tests that are less useful than others or are
increasing the time are good to be kept under PG_TEST_EXTRA but if
more people advocate such an approach then it is worth considering.
--
With Regards,
Amit Kapila.
for one or two types of conflicts.
*
(see
+ * PgStat_StatSubEntry::conflict_count and PgStat_StatSubEntry::conflict_count)
There is a typo in the above comment.
--
With Regards,
Amit Kapila.
change looks good to me as well. I'll wait for a day or
two before pushing to see if anyone thinks otherwise.
--
With Regards,
Amit Kapila.
Agreed. But this should lead to assertion failure. Did you try testing it?
--
With Regards,
Amit Kapila.
s as 0001 is in itself a complete and
separate patch that can be committed.
--
With Regards,
Amit Kapila.
On Thu, Aug 29, 2024 at 11:31 AM Bharath Rupireddy
wrote:
>
> Thanks for looking into this.
>
> On Mon, Aug 26, 2024 at 4:35 PM Amit Kapila wrote:
> >
> > Few comments on 0001:
> > 1.
> > @@ -651,6 +651,13 @@ synchronize_one_slot(RemoteSlot *remote_slot, Oid
&
he number of
invalidations is more to see the distribution cost in such cases. For
example, Truncate/Drop a table with 100 or 1000 partitions.
--
With Regards,
Amit Kapila.
gt; pg_logicalsnapinspect works as a tool to be helpful even when the
> server is down.
>
We have an example of pageinspect which provides low-level functions
to aid debugging. The proposal for these APIs also seems to fall in
the same category, so why go for the core for these functions?
--
With Regards,
Amit Kapila.
on pub1
with (streaming = on, streaming=off);
ERROR: conflicting or redundant options
LINE 1: ...=postgres' publication pub1 with (streaming = on, streaming=...
So duplicate options are not allowed. If we see any challenges to
follow same for resolvers then we can discuss but it seems better to
follow the existing behavior of other subscription options.
Also, the behavior for CREATE/ALTER should be the same.
--
With Regards,
Amit Kapila.
nge if we plan to *not *dump defaults
> in pg_dump.
>
> Another point about 'defaults' is regarding insertion into the
> pg_subscription_conflict table. We currently do insert default
> resolvers into 'pg_subscription_conflict' even if the user has not
> explicitly configured them.
>
I don't see any problem with it. BTW, if we don't do it, I think
wherever we are referring the resolvers for a conflict, we need some
special handling for default and non-default. Am I missing something?
--
With Regards,
Amit Kapila.
:03.152 IST [195415] WARNING: conflict detection
> could be incomplete due to disabled track_commit_timestamp
> 2024-07-26 09:14:03.152 IST [195415] DETAIL: Conflicts update_differ
> and delete_differ cannot be detected, and the origin and commit
> timestamp for the local row will not be logged.
>
> Thoughts?
>
> If we emit this WARNING during each resolution, then it may flood our
> log files, thus it seems better to emit it during create or alter
> subscription instead of during resolution.
>
Sounds reasonable.
--
With Regards,
Amit Kapila.
On Wed, Aug 28, 2024 at 1:25 AM Bertrand Drouvot
wrote:
>
> On Mon, Aug 26, 2024 at 07:05:27PM +0530, Amit Kapila wrote:
> > On Thu, Aug 22, 2024 at 5:56 PM Bertrand Drouvot
> > wrote:
> > >
> > > 2. The SnapBuildOnDisk and SnapBuild structs are now expo
On Thu, Aug 29, 2024 at 8:44 AM Masahiko Sawada wrote:
>
> On Wed, Aug 28, 2024 at 1:06 AM Amit Kapila wrote:
> >
> > On Mon, May 20, 2024 at 1:49 PM Masahiko Sawada
> > wrote:
> > >
> > > As Euler mentioned earlier, I think it's a decision not
columns.
Yet another option is to have a different view like
pg_stat_subscription_conflicts but that sounds like going too far.
[1] -
https://www.postgresql.org/docs/devel/monitoring-stats.html#MONITORING-PG-STAT-DATABASE-CONFLICTS-VIEW
--
With Regards,
Amit Kapila.
suggestions on this patch.
--
With Regards,
Amit Kapila.
a small patch to
> > > > rename
> > > > as suggested.
> > >
> > > Sorry, I attached the wrong patch. Here is correct one.
> > >
> >
> > LGTM.
> >
>
> LGTM.
>
I'll push this patch tomorrow unless there are any suggestions or comments.
--
With Regards,
Amit Kapila.
be listed
> > here.)
>
> OK, it's enough for me just remove ".. without losing data".
>
The next line related to asynchronous replication is also not
required. See attached.
--
With Regards,
Amit Kapila.
fix_doc_1.patch
Description: Binary data
LS, t2, t3, t4
INCLUDE ..., t5;. I haven't analyzed the feasibility of this, so there
could be some challenges but we can at least investigate it.
Yet another idea is to keep this as a publication option
(include_generated_columns or publish_generated_columns) similar to
"publish_via_partition_root". Normally, "publish_via_partition_root"
is used when tables on either side have different partition
hierarchies which is somewhat the case here.
Thoughts?
--
With Regards,
Amit Kapila.
ps://www.postgresql.org/docs/17/runtime-config-replication.html#GUC-SYNCHRONIZED-STANDBY-SLOTS
--
With Regards,
Amit Kapila.
>
So, will it be okay if we just remove ".. without losing data" from
the sentence? Will that avoid the confusion you have?
With Regards,
Amit Kapila.
t evident from the code or comments in
the patch.
[1] :
+ /* Cache values to reduce contention on lock */
+ for (i = 0; i < NUM_SYNC_REP_WAIT_MODE; i++)
+ {
+ lsn[i] = walsndctl->lsn[i];
+ }
- ss_oldest_flush_lsn = min_restart_lsn;
+ LWLockRelease(SyncRepLock);
- return true;
+ if (lsn[mode] >= wait_for_lsn)
+ return true;
--
With Regards,
Amit Kapila.
On Mon, Aug 26, 2024 at 6:38 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Monday, August 26, 2024 5:37 PM Amit Kapila
> wrote:
> >
> > On Mon, Aug 26, 2024 at 1:30 PM wrote:
> > >
> > > When I read the following documentation related to the
> > "sync
duce.txt).
>
I think you see such a behavior because you have disabled
'synchronized_standby_slots' in your script (# disable
"synchronized_standby_slots"). You need to enable that to avoid data
loss. Considering that, I don't think your proposed text is an
improvement.
--
With Regards,
Amit Kapila.
e to avoid exposing these structures? Can we expose some
function from snapbuild.c that provides the required information?
--
With Regards,
Amit Kapila.
it at the time of shutdown. The
other point is can we try to check the performance impact with 100s of
slots as mentioned in the code comments?
--
With Regards,
Amit Kapila.
On Thu, Aug 22, 2024 at 2:21 PM Amit Kapila wrote:
>
> On Thu, Aug 22, 2024 at 1:33 PM Peter Smith wrote:
> >
> > Do you think the documentation for the 'column_value' parameter of the
> > conflict logging should say that the displayed value might be
> > t
ot; is a bit confusing. Will it make things
clear to me if we remove that part?
I am keeping a few others involved in this feature development in Cc.
--
With Regards,
Amit Kapila.
ction part doesn't go well without being explicit that it is a
resolution method. Another variant could be ON CONFLICT
'insert_exists' USE RESOLUTION [METHOD] 'keep_local'.
I think we can keep all these syntax alternatives either in the form
of comments or in the commit message and discuss more on these once we
agree on the solutions to the key design issues pointed out by Shveta.
--
With Regards,
Amit Kapila.
nflict i.e. we are not
> distinguishing between the missing row and the deleted row. We had
> discussed this in the past a couple of times. If we plan to target it
> in draft 1, I can dig up all old emails and resume discussion on this.
>
This is a separate conflict detection project
On Fri, Aug 23, 2024 at 3:44 PM Masahiko Sawada wrote:
>
> On Tue, Aug 20, 2024 at 9:29 PM Amit Kapila wrote:
> >
>
> I've updated the updated patch with regression tests.
>
LGTM.
--
With Regards,
Amit Kapila.
On Thu, Aug 22, 2024 at 1:33 PM Peter Smith wrote:
>
> Do you think the documentation for the 'column_value' parameter of the
> conflict logging should say that the displayed value might be
> truncated?
>
I updated the patch to mention this and pushed it.
--
With Regards,
Amit Kapila.
t not sure if we want to
cover every code path via tests as each additional test also has some
cost. OTOH, If others think it is important or a good idea to have
this test then I don't have any objection to the same.
--
With Regards,
Amit Kapila.
lay the tuple for conflicts.
>
The current information is displayed keeping in mind that users should
be able to use that to manually resolve conflicts if required. If we
think there is a leak of information (either from a security angle or
otherwise) like tuple data then we can re-consider. However, as we are
displaying tuple information in other places as pointed out by
Hou-San, we thought it is also okay to display in this case.
--
With Regards,
Amit Kapila.
On Tue, Aug 20, 2024 at 2:01 PM shveta malik wrote:
>
> On Tue, Aug 20, 2024 at 11:36 AM Amit Kapila wrote:
> >
> > On Wed, Aug 14, 2024 at 10:26 AM shveta malik
> > wrote:
> > >
> > > >
> > > > Thanks for the detailed analysis. I a
On Wed, Aug 21, 2024 at 12:40 PM Heikki Linnakangas wrote:
>
> On 21/08/2024 09:25, Amit Kapila wrote:
> >>
> >> I think this patch makes sense. I'm not sure we've actually made any
> >> promises on it, but it feels wrong that the slot's LSN mig
On Wed, Aug 21, 2024 at 2:25 AM Heikki Linnakangas wrote:
>
> On 14/08/2024 16:54, Arseny Sher wrote:
> > On 8/13/24 06:35, Amit Kapila wrote:
> >> On Mon, Aug 12, 2024 at 3:43 PM Arseny Sher wrote:
> >>>
> >>> Sorry for the poor formatting of t
gt; test case for now).
>
The patch LGTM except for one minor comment.
+ /* All changes must be returned */
+ Assert(txn->size == 0);
Isn't it better to say: "All changes must be deallocated." in the above comment?
--
With Regards,
Amit Kapila.
But that won't fix the problem reported by Sawada-san in an email [1].
BTW, we should do some performance testing by having a mix of DML and
DDLs to see the performance impact of this patch.
[1] -
https://www.postgresql.org/message-id/CAD21AoAenVqiMjpN-PvGHL1N9DWnHSq673bfgr6phmBUzx=k...@mail.gmail.com
--
With Regards,
Amit Kapila.
stantially less information, and it fits
> a little better with the terse style of the other wait events nearby.
>
+1 for Nathan's version. It is quite close to the previous version,
for which we haven't heard any complaints since they were introduced.
--
With Regards,
Amit Kapila.
ase find v4 attached. Addressed comments in that.
>
The patch looks mostly good to me. I have slightly changed a few of
the comments in the attached. What do you think of the attached?
--
With Regards,
Amit Kapila.
v5-0001-Don-t-advance-origin-during-apply-failure.patch
Description: Binary data
On Mon, Aug 19, 2024 at 4:16 PM Amit Kapila wrote:
>
> > Rest looks good.
> >
>
> Thanks for the review and testing.
>
Pushed.
--
With Regards,
Amit Kapila.
is an extra include, so removed in the attached. Additionally, I
have modified a few comments and commit message.
> Also, there are few long lines in conflict.c (see line 408, 410).
>
I have left these as it is because pgindent doesn't complain about them.
> Rest looks good.
>
On Mon, Aug 19, 2024 at 11:54 AM shveta malik wrote:
>
> On Mon, Aug 19, 2024 at 11:37 AM Amit Kapila wrote:
> >
> > On Mon, Aug 19, 2024 at 9:08 AM shveta malik wrote:
> > >
> > > On Sun, Aug 18, 2024 at 2:27 PM Zhijie Hou (Fujitsu)
> > > wrot
PK seems reasonable. I don't think adding additional
code to distinguish these two cases in the LOG message is worth it. We
can always change such things later if that is what users and or
others prefer.
[1] - https://www.postgresql.org/docs/devel/sql-altertable.html
[2] - https://www.postgresql.org/docs/devel/catalog-pg-class.html
--
With Regards,
Amit Kapila.
t;
> 5)
> For insert/update_exists, the sequence is:
> Key .. ; existing local tuple .. ; remote tuple ...
>
> For rest of the conflicts, sequence is:
> Existing local tuple .. ; remote tuple .. ; replica identity ..
>
> Is it intentional? Shall the 'Key' or 'Replica Identity' be the first
> one to come in all conflicts?
>
This is worth considering but Replica Identity signifies the old tuple
values, that is why it is probably kept at the end. But let's see what
Hou-San or others think about this.
--
With Regards,
Amit Kapila.
1 - 100 of 1708 matches
Mail list logo
