public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v2] Allow HEAP_XMAX_LOCK_ONLY and HEAP_KEYS_UPDATED combination. 3+ messages / 3 participants [nested] [flat]
* [PATCH v2] Allow HEAP_XMAX_LOCK_ONLY and HEAP_KEYS_UPDATED combination. @ 2021-01-21 16:06 Julien Rouhaud <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Julien Rouhaud @ 2021-01-21 16:06 UTC (permalink / raw) This combination of hint bits was previously detected as a form of corruption, but it can be obtained with some mix of SELECT ... FOR UPDATE and UPDATE queries. Discussion: https://postgr.es/m/20210124061758.GA11756@nol --- contrib/amcheck/t/001_verify_heapam.pl | 14 +++++++++++++- contrib/amcheck/verify_heapam.c | 7 ------- src/backend/access/heap/README.tuplock | 5 +++-- src/backend/access/heap/hio.c | 8 +++----- 4 files changed, 19 insertions(+), 15 deletions(-) diff --git a/contrib/amcheck/t/001_verify_heapam.pl b/contrib/amcheck/t/001_verify_heapam.pl index a2f65b826d..6050feb712 100644 --- a/contrib/amcheck/t/001_verify_heapam.pl +++ b/contrib/amcheck/t/001_verify_heapam.pl @@ -4,7 +4,7 @@ use warnings; use PostgresNode; use TestLib; -use Test::More tests => 79; +use Test::More tests => 80; my ($node, $result); @@ -47,6 +47,9 @@ detects_heap_corruption( # fresh_test_table('test'); $node->safe_psql('postgres', q(VACUUM (FREEZE, DISABLE_PAGE_SKIPPING) test)); +detects_no_corruption( + "verify_heapam('test')", + "all-frozen not corrupted table"); corrupt_first_page('test'); detects_heap_corruption("verify_heapam('test')", "all-frozen corrupted table"); @@ -92,6 +95,15 @@ sub fresh_test_table ALTER TABLE $relname ALTER b SET STORAGE external; INSERT INTO $relname (a, b) (SELECT gs, repeat('b',gs*10) FROM generate_series(1,1000) gs); + BEGIN; + SAVEPOINT s1; + SELECT 1 FROM $relname WHERE a = 42 FOR UPDATE; + UPDATE $relname SET b = b WHERE a = 42; + RELEASE s1; + SAVEPOINT s1; + SELECT 1 FROM $relname WHERE a = 42 FOR UPDATE; + UPDATE $relname SET b = b WHERE a = 42; + COMMIT; )); } diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c index 88ab32490c..49f5ca0ef2 100644 --- a/contrib/amcheck/verify_heapam.c +++ b/contrib/amcheck/verify_heapam.c @@ -608,13 +608,6 @@ check_tuple_header_and_visibilty(HeapTupleHeader tuphdr, HeapCheckContext *ctx) ctx->tuphdr->t_hoff, ctx->lp_len)); header_garbled = true; } - if ((ctx->tuphdr->t_infomask & HEAP_XMAX_LOCK_ONLY) && - (ctx->tuphdr->t_infomask2 & HEAP_KEYS_UPDATED)) - { - report_corruption(ctx, - pstrdup("tuple is marked as only locked, but also claims key columns were updated")); - header_garbled = true; - } if ((ctx->tuphdr->t_infomask & HEAP_XMAX_COMMITTED) && (ctx->tuphdr->t_infomask & HEAP_XMAX_IS_MULTI)) diff --git a/src/backend/access/heap/README.tuplock b/src/backend/access/heap/README.tuplock index d03ddf6cdc..6d357565a8 100644 --- a/src/backend/access/heap/README.tuplock +++ b/src/backend/access/heap/README.tuplock @@ -147,8 +147,9 @@ The following infomask bits are applicable: - HEAP_KEYS_UPDATED This bit lives in t_infomask2. If set, indicates that the XMAX updated - this tuple and changed the key values, or it deleted the tuple. - It's set regardless of whether the XMAX is a TransactionId or a MultiXactId. + this tuple and changed the key values, or it deleted the tuple. It can also + be set in combination of HEAP_XMAX_LOCK_ONLY. It's set regardless of whether + the XMAX is a TransactionId or a MultiXactId. We currently never set the HEAP_XMAX_COMMITTED when the HEAP_XMAX_IS_MULTI bit is set. diff --git a/src/backend/access/heap/hio.c b/src/backend/access/heap/hio.c index fb7ad0bab4..75223c9bea 100644 --- a/src/backend/access/heap/hio.c +++ b/src/backend/access/heap/hio.c @@ -49,12 +49,10 @@ RelationPutHeapTuple(Relation relation, /* * Do not allow tuples with invalid combinations of hint bits to be placed - * on a page. These combinations are detected as corruption by the - * contrib/amcheck logic, so if you disable one or both of these - * assertions, make corresponding changes there. + * on a page. This combination is detected as corruption by the + * contrib/amcheck logic, so if you disable this assertion, make + * corresponding changes there. */ - Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_LOCK_ONLY) && - (tuple->t_data->t_infomask2 & HEAP_KEYS_UPDATED))); Assert(!((tuple->t_data->t_infomask & HEAP_XMAX_COMMITTED) && (tuple->t_data->t_infomask & HEAP_XMAX_IS_MULTI))); -- 2.20.1 --+HP7ph2BbKc20aGI-- ^ permalink raw reply [nested|flat] 3+ messages in thread
* collation-related loose ends before beta2 @ 2023-06-20 09:02 Jeff Davis <[email protected]> 0 siblings, 1 reply; 3+ messages in thread From: Jeff Davis @ 2023-06-20 09:02 UTC (permalink / raw) To: pgsql-hackers; +Cc: Daniel Verite <[email protected]>; Peter Eisentraut <[email protected]>; Andrew Gierth <[email protected]>; Tom Lane <[email protected]>; Joe Conway <[email protected]>; Robert Haas <[email protected]>; Jonathan S. Katz <[email protected]>; Peter Geoghegan <[email protected]>; Thomas Munro <[email protected]> Status on collation loose ends: 1. There's an open item "Switch to ICU for 17". It's a little bit confusing exactly what that means, and the CF entry refers to two items, one of which is the build-time default to --with-icu. As far as I know, building with ICU by default is a settled issue with no objections. The second issue is the initdb default, which is covered by the other open item. So I will just close that open item unless someone thinks I'm missing something. 2. Open item about the unfriendly rules for choosing an ICU locale at initdb time. Tom, Robert, and Daniel Verite have expressed concerns (and at least one objection) to initdb defaulting to icu for --locale- provider. Some of the problems have been addressed, but the issue about C and C.UTF-8 locales is not settled. Even if it were settled I'm not sure we'd have a clear consensus on all the details. I don't think this should proceed to beta2 in this state, so I intend to revert back to libc as the default for initdb. [ I believe we do have a general consensus that ICU is better, but we can signal it other ways: through documentation, packaging, etc. ] 3. The ICU conversion from "C" to "en-US-u-va-posix": cut out this code (it was a small part of a larger change). It's only purpose was consistency between ICU versions, and nobody liked it. It's only here right now to avoid test failures due to an order-of-commits issue; but if the initdb default goes back to libc it won't matter and I can remove it. 4. icu_validation_level WARNING or ERROR: right now an invalid ICU locale raises a WARNING, but Peter Eisentraut would prefer an ERROR. I'm still inclined to leave it as a WARNING for one release and increase it to ERROR later. But if the default collation provider goes back to libc, the risk of ICU validation errors goes way down, so I don't object if Peter would like to change it back to an ERROR. Regards, Jeff Davis ^ permalink raw reply [nested|flat] 3+ messages in thread
* Re: collation-related loose ends before beta2 @ 2023-06-21 17:17 Jonathan S. Katz <[email protected]> parent: Jeff Davis <[email protected]> 0 siblings, 0 replies; 3+ messages in thread From: Jonathan S. Katz @ 2023-06-21 17:17 UTC (permalink / raw) To: Jeff Davis <[email protected]>; pgsql-hackers; +Cc: Daniel Verite <[email protected]>; Peter Eisentraut <[email protected]>; Andrew Gierth <[email protected]>; Tom Lane <[email protected]>; Joe Conway <[email protected]>; Robert Haas <[email protected]>; Peter Geoghegan <[email protected]>; Thomas Munro <[email protected]> On 6/20/23 5:02 AM, Jeff Davis wrote: > > Status on collation loose ends: > > 1. There's an open item "Switch to ICU for 17". It's a little bit > confusing exactly what that means, and the CF entry refers to two > items, one of which is the build-time default to --with-icu. As far as > I know, building with ICU by default is a settled issue with no > objections. The second issue is the initdb default, which is covered by > the other open item. So I will just close that open item unless someone > thinks I'm missing something. [RMT Hat] No objections. The RMT had interpreted this as "Punt on making ICU the building default to v17" but it seems the consensus is to continue to leave it in as the default for v16. > 2. Open item about the unfriendly rules for choosing an ICU locale at > initdb time. Tom, Robert, and Daniel Verite have expressed concerns > (and at least one objection) to initdb defaulting to icu for --locale- > provider. Some of the problems have been addressed, but the issue about > C and C.UTF-8 locales is not settled. Even if it were settled I'm not > sure we'd have a clear consensus on all the details. I don't think this > should proceed to beta2 in this state, so I intend to revert back to > libc as the default for initdb. [ I believe we do have a general > consensus that ICU is better, but we can signal it other ways: through > documentation, packaging, etc. ] [Personal hat] (Building...) I do think this raises a good point: it's really the packaging that will guide what users are using for v16. I don't know if we want to discuss/poll the packagers to see what they are thinking about this? > 3. The ICU conversion from "C" to "en-US-u-va-posix": cut out this code > (it was a small part of a larger change). It's only purpose was > consistency between ICU versions, and nobody liked it. It's only here > right now to avoid test failures due to an order-of-commits issue; but > if the initdb default goes back to libc it won't matter and I can > remove it. > > 4. icu_validation_level WARNING or ERROR: right now an invalid ICU > locale raises a WARNING, but Peter Eisentraut would prefer an ERROR. > I'm still inclined to leave it as a WARNING for one release and > increase it to ERROR later. But if the default collation provider goes > back to libc, the risk of ICU validation errors goes way down, so I > don't object if Peter would like to change it back to an ERROR. [Personal hat] I'd be inclined for "WARNING" until getting a sense of what packagers who do an initdb as part of the installation process decide what collation provider they're going to use. Thanks, Jonathan Attachments: [application/pgp-signature] OpenPGP_signature (840B, ../../[email protected]/2-OpenPGP_signature) download ^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2023-06-21 17:17 UTC | newest] Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-01-21 16:06 [PATCH v2] Allow HEAP_XMAX_LOCK_ONLY and HEAP_KEYS_UPDATED combination. Julien Rouhaud <[email protected]> 2023-06-20 09:02 collation-related loose ends before beta2 Jeff Davis <[email protected]> 2023-06-21 17:17 ` Re: collation-related loose ends before beta2 Jonathan S. Katz <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox