Rebased.
Also, separate thread with some additional explanation is here:
https://www.postgresql.org/message-id/flat/cadzflwxzvmbo11tfs_g2i+6tffvwhu4vuuseoqb+8uqfuoj...@mail.gmail.com
v12-0002-Fix-logical-replication-conflict-detection-durin.patch
Description: Binary data
v12-0001-This-patch-i
Hi!
> Hello, Antonin!
>
> if (using_subtxn)
> {
>RollbackAndReleaseCurrentSubTransaction();
>MemoryContextSwitchTo(ccxt);
>CurrentResourceOwner = cowner;
> }
>
> IIUC memory context is already switched above:
>
> MemoryContext ecxt = MemoryContextSwitchTo(cc
Ranier Vilela :
> SqlServer has similar feature.
> SHRINK
MySQL/MariaDB
OPTIMIZE TABLE table_name
SQL Server
ALTER TABLE table_name REBUILD
DBCC SHRINKFILE
DBCC SHRINKDATABASE
Oracle
ALTER TABLE table_name SHRINK SPACE
SQLite
VACUUM
IBM DB2
REORG TABLE table_name
Sybase ASE
REORG REBUILD tabl
Hello, everyone!
Initially this was discussed in ("[BUG?]
check_exclusion_or_unique_constraint false negative")[0], but as Amit
recommended [1] I decided to write a dedicated email about it, since
the original thread is too deep and has some unrelated discussion.
Commitfest entry [2] contains rep
Hello, Antonin!
if (using_subtxn)
{
RollbackAndReleaseCurrentSubTransaction();
MemoryContextSwitchTo(ccxt);
CurrentResourceOwner = cowner;
}
IIUC memory context is already switched above:
MemoryContext ecxt = MemoryContextSwitchTo(ccxt);
Best regards,
Mikhai
Hello, Amit!
Amit Kapila :
> So, in such cases as we won't be able to detect
> transaction dependencies, it would be better to allow out-of-order
> commits optionally.
I think it is better to enable preserve order by default - for safety reasons.
I also checked the patch for potential issues lik
Hello, Amit!
> Now, I
> would like to know the opinion of others who were involved in the
> initial commit, so added Peter E. to see what he thinks of the same.
Peter answered in [0]:
> I don’t remember. I was just the committer.
I’ve attached a new version of the proposed solution.
The first co
Hello, Álvaro!
Alvaro Herrera :
> If you want to post secondary patches, please rename them to end in
> something like .txt or .nocfbot or whatever. See here:
> https://wiki.postgresql.org/wiki/Cfbot#Which_attachments_are_considered_to_be_patches?
Sorry, I missed that.
But now it is possible to
Hello!
Antonin Houska :
> Are you sure the test is complete? I see no occurrence of the REPACK command
> in it.
Oops, send invalid file. The correct one in attachment.
Add_test_for_REPACK_CONCURRENTLY_with_concurrent_modifications.patch_
Description: Binary data
Hello!
I started an attempt to make a "lightweight" MVCC-safe prototype and
stuck into the "it is not working" issue.
After some debugging I realized Antonin's variant (catalog-mode based)
seems to be broken also...
And after a few more hours I realized non-MVCC is broken as well :)
This is a pa
Hello, Álvaro!
> If others are motivated enough to certify it, maybe we can consider it.
> But I don't think I'm going to have time to get this part reviewed and
> committed in time for 19, so you might need another committer.
I don't think it is realistic to involve another committer - it is
jus
Antonin Houska :
> Do you mean the race related to TransactionIdIsInProgress()? Not sure I
> understand, as you suggested above that you no longer need the function.
The "lightweight" approaches I see so far:
* XactLockTableWait before replay + SnapshotSelf(GetLatestSnapshot?)
* SnapshotDirty + r
Hello, Amit!
Amit Kapila :
> Now, I
> would like to know the opinion of others who were involved in the
> initial commit, so added Peter E. to see what he thinks of the same.
Seems like you added another Peter in [0] - I added Peter Eisentraut :)
> > Hmm Yes - if the TID lands to the page l
Antonin Houska :
> I insist that this is a misuse of TransactionIdIsInProgress(). When dealing
> with logical decoding, only WAL should tell whether particular transaction is
> still running. AFAICS this is how reorderbuffer.c works.
Hm... Maybe, but at the same time we already have SnapshotDirty
Hello, Antonin!
Antonin Houska :
>
> Where exactly should HeapTupleSatisfiesDirty() conclude that the tuple is
> visible? TransactionIdIsCurrentTransactionId() will not do w/o the
> modifications that you proposed earlier [1] and TransactionIdIsInProgress() is
> not suitable as I explained in [2].
Antonin Houska :
> Although it could work, I think it'd be confusing to consider the transactions
> being replayed as "current" from the point of view of the backend that
> executes REPACK CONCURRENTLY.
Just realized SnapshotDirty is the thing that fits into the role - it
respects not-yet committ
Antonin Houska :
> I think the problem is that HeapTupleSatisfiesSelf() uses
> TransactionIdIsInProgress() instead of checking the snapshot:
Yes, some issues might be possible for SnapshotSelf.
Possible solution is to override TransactionIdIsCurrentTransactionId
to true (like you did with nParalle
Hi, Antonin
> How does HeapTupleSatisfiesSelf() recognize the status of any XID w/o using a
> snapshot? Do you mean by checking the commit log (TransactionIdDidCommit) ?
Yes, TransactionIdDidCommit. Another option is just invent a new
snapshot type - SnapshotBelieveEverythingCommitted - for that
Hi, Antonin!
> I assume you are concerned with the patch part 0005 of the v12 patch
> ("Preserve visibility information of the concurrent data changes."), aren't
> you?
Yes, of course. I got an idea while trying to find a way to optimize it.
> Not sure I understand in all details, but I don't th
Amit Kapila :
> What if the new insert happens in a page prior to the current page? I
> mean that the scan won't encounter the page where Insert happens.
Hmm Yes - if the TID lands to the page left of the current
position, we’ll miss it as well.
A lock‑based solution (version in the v10) wou
Hello!
> Why only by luck?
I mean last_write_win provides the same results in the following cases:
* we found the tuple, detected a conflict, and decided to ignore the
update coming from the publisher
* we were unable to find the tuple, logged an error about it, and
ignored the update coming from
Hello, Antonin!
> When posting version 12 of the patch [1] I raised a concern that the the MVCC
> safety is too expensive when it comes to logical decoding. Therefore, I
> abandoned the concept for now, and v13 [2] uses plain heap_insert(). Once we
> implement the MVCC safety, we simply rewrite th
Hello, Andres!
Andres Freund :
>
>As part of that introduce a new lock level in-between share and exclusive
>locks (provisionally called share-exclusive, but I hate it). The new lock
>level allows other share lockers, but can only be held by one backend.
>
>This allows to change th
Amit, a few more explanations related to your message.
> IIUC, the problem you are worried about can happen with DELETE+INSERT
> in the same transaction on the subscriber, right?
Technically, yes - this can occur during a single UPDATE, as well as a
DELETE followed by an INSERT of the same key wi
Oh,
> in index page appear because HOT was applied...
I mean "HOT was NOT applied", sorry for the inconvenience.
Hello, Amit,
> IIUC, the problem you are worried about can happen with DELETE+INSERT
It seems there was some misunderstanding due to my bad explanation and wording.
I wrote "A concurrent transaction deletes a tuple and inserts a new
one with a different TID" - but I mean logical UPDATE causing new
Hello!
Sorry for being noisy - just want to remind: conflict detection system
(including new updated_deleted) is giving invalid reports because of
the issue related to SnapshotDirty vs btree. So, it is not possible to
rely on that at the moment.
You may check TAP tests here [0] and some explanati
Hello,
Added one more test - for invalid "update_deleted" conflict detection.
Best regards,
Mikhail.
v10-0002-Fix-btree-index-scan-concurrency-issues-with-dir.patch
Description: Binary data
v10-0001-This-patch-introduces-new-injection-points-and-T.patch
Description: Binary data
Hello everyone!
Alvaro Herrera :
> Please note that Antonin already implemented this. See his patches
> here:
> https://www.postgresql.org/message-id/77690.1725610115%40antos
> I proposed to leave this part out initially, which is why it hasn't been
> reposted. We can review and discuss after th
Hello, everyone!
Issue description is available at [0] (in few words - SnapshotDirty
scan may miss tuple in index because of race condition with update of
that tuple).
But I have realised there are cases much more severe than invalid
conflict messages for logical replication - lost delete/updates
Hello!
One more thing - I think build_new_indexes and
index_concurrently_create_copy are very close in semantics, so it
might be a good idea to refactor them a bit.
I’m still concerned about MVCC-related issues. For multiple
applications, this is a dealbreaker, because in some cases correctness
i
Hello Álvaro,
Should we skip non-ready indexes in build_new_indexes?
Also, in the current implementation, concurrent mode is marked as
non-MVCC safe. From my point of view, this is a significant limitation
for practical use.
Should we consider an option to exchange non-MVCC issues to short
exclus
Hello, Peter!
> FWIW _hash_readpage has a comment about a stashed LSN, so it seems as
> if this was barely missed by the work on hash indexes around 2017:
I think commit 22c5e735 [0] (Remove lsn from HashScanPosData) is the
thing you are looking for in relation to hash.
Best regards,
Mikhail.
Hello, everyone!
Added patch to the offer book of review marketplace [0].
Best regards,
Mikhail.
[0]: https://wiki.postgresql.org/wiki/Review_Marketplace#Offer_book
Hello!
No, I think the comment is correct, it is about [0].
[0]:
https://github.com/postgres/postgres/blob/f5a987c0e5f6bbf0cc0420228dc57e7aae4d7e8f/src/backend/commands/indexcmds.c#L4217
Hello, everyone!
Rebased.
Best regards,
Mikhail.
v4-0001-amcheck-Fix-bt_index_parent_check-behavior-with-C.patch
Description: Binary data
Hello, Sergey!
> Today I encountered a segmentation fault caused by the patch
> v20-0007-Add-Datum-storage-support-to-tuplestore.patch. During the merge
> phase, I inserted some tuples into the table so that STIR would have data for
> the validation phase. The segfault occurred during a call to
Hello, Sergey!
> In patch v20-0006-Add-STIR-access-method-and-flags-related-to-auxi.patch,
> within the "StirMarkAsSkipInserts" function, a critical section appears to be
> left unclosed. This resulted in an assertion failure during ANALYZE of a
> table containing a leftover STIR index.
Thanks,
Rebased.
v8-0001-Add-an-isolation-test-to-reproduce-a-dirty-snapsh.patch
Description: Binary data
v8-0002-Fix-btree-index-scan-concurrency-issues-with-dirt.patch
Description: Binary data
Hello, Sergey!
> I think it's to avoid duplicate errors when adding tuples from STIP to the
> main index,
> but couldn't we just suppress that error during validation and skip the new
> tuple insertion if it already exists?
In some cases, it is not possible:
– Some index types (GiST, GIN, BRIN)
Hello, Sergey!
This is a great idea, so great as I have implemented it :) Check [0] and
[1] for details.
Review and comments are welcome.
[0]:
https://www.postgresql.org/message-id/flat/CADzfLwVOcZ9mg8gOG%2BKXWurt%3DMHRcqNv3XSECYoXyM3ENrxyfQ%40mail.gmail.com#52c97e004b8f628473124c05e3bf2da1
[1]
Hello!
Rebased\reworked to align with the changes of [0].
Best regards.
[0]:
https://www.postgresql.org/message-id/flat/CAH2-WznZBhWqDBDVGh1VhVBLgLqaYHEkPhmVV7mJCr1Y3ZQhQQ%40mail.gmail.com#d6f1debb1405d1b4a983cbb46b24f41b
From 13934bd0b3588e71a96228f31395ab661e0e749d Mon Sep 17 00:00:00 2001
Fr
Hello, Peter!
>
> I also relocated the code that sets so.drop_pin from
> _bt_preprocess_keys to btrescan. That seems like the more logical
> place for it.
>
I was rebasing [0] and noticed dropPin is not initialized in
btbeginscan, which seems to be suspicious for me.
Is it intended?
[0]: https:/
Hello, Donghang!
> One suggestion to this change is that we might need to update the amcheck doc
> to reflect that
> "This consists of a “dummy” CREATE INDEX CONCURRENTLY operation" rather than
> "CREATE INDEX" operation.
+1, done. Also fixed some typos in the commit message.
Best regards,
Mik
Hello, Álvaro!
> I didn't understand why you have a few "v19" patches and also a separate
> series of "v19-only-part-3-" patches. Is there duplication? How do
> people know which series comes first?
This was explained in the previous email [0]:
> Patch itself contains 4 parts, some of them may
Hello!
Rebased version.
v9-0004-Modify-the-ExecInitPartitionInfo-function-to-cons.patch
Description: Binary data
v9-0001-Specs-to-reproduce-the-issues-with-CREATE-INDEX-C.patch
Description: Binary data
v9-0002-Modify-the-infer_arbiter_indexes-function-to-cons.patch
Description: Binary data
Hello, everyone!
Rebased + fix for compilation due the new INEJCTION_POINT signature.
Best regards,
Mikhail.
v6-0001-Add-an-isolation-test-to-reproduce-a-dirty-snapsh.patch
Description: Binary data
v6-0002-Fix-btree-index-scan-concurrency-issues-with-dirt.patch
Description: Binary data
> I think this LSN is simply LSN where crash recovery ends...
Yes, you are right and we come back to :
> 1. Allow checking standby sync before making data visible after crash recovery
Best regards,
Mikhail.
Hello, everyone!
> On Mon, 12 May 2025 at 18:42, Andrey Borodin wrote:
>> >> Problem: user might try to cancel locally committed transaction and if we
>> >> do so we will show non-replicated data as committed. This leads to
>> >> loosing data with UPSERTs.
>> > >
>> > > Could you explain why sp
Hello, everyone!
> Or might want LSN "flush everything you have written, and return that LSN".
> That will also do the trick, but is not necessary.
I think it is a better option. Less things may go wrong in such a case.
Best regards,
Mikhail.
Hello, everyone.
Not sure if it helps but encountered the same problem in relation to
injection points:
https://www.postgresql.org/message-id/flat/CANtu0oiTgFW47QgpTwrMOVm3Bq4N0Y5bjvTy5sP0gYWLQuVgjw%40mail.gmail.com
Hello, everyone and Peter!
Peter, I have added you because you may be interested in (or already know
about) this btree-related issue.
Short description of the problem:
I noticed a concurrency issue in btree index scans that affects
SnapshotDirty and SnapshotSelf scan types.
When using these non-
Hello!
Noah, I have noticed you already disabled runningcheck for isolation tests
already in injection_points[0].
The whole patch here was about to make it default for all types of tests
for injection_points.
Seems like we may close this entry.
Attached patch is just to put a rebased version her
53 matches
Mail list logo
