public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v13 01/20] Remove DLLTOOL, DLLWRAP from configure / Makefile.global.in 10+ messages / 5 participants [nested] [flat]
* [PATCH v13 01/20] Remove DLLTOOL, DLLWRAP from configure / Makefile.global.in @ 2022-08-30 22:33 Andres Freund <[email protected]> 0 siblings, 0 replies; 10+ messages in thread From: Andres Freund @ 2022-08-30 22:33 UTC (permalink / raw) We got rid of the need for them in 4f5f485d10c and 846e91e0223. --- configure | 186 ----------------------------------------- configure.ac | 2 - src/Makefile.global.in | 2 - 3 files changed, 190 deletions(-) diff --git a/configure b/configure index f325bd85b88..aee8a0d3579 100755 --- a/configure +++ b/configure @@ -688,8 +688,6 @@ INSTALL_DATA INSTALL_SCRIPT INSTALL_PROGRAM WINDRES -DLLWRAP -DLLTOOL AR STRIP_SHARED_LIB STRIP_STATIC_LIB @@ -9769,190 +9767,6 @@ else fi if test "$PORTNAME" = "win32"; then - if test -n "$ac_tool_prefix"; then - # Extract the first word of "${ac_tool_prefix}dlltool", so it can be a program name with args. -set dummy ${ac_tool_prefix}dlltool; ac_word=$2 -{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for $ac_word" >&5 -$as_echo_n "checking for $ac_word... " >&6; } -if ${ac_cv_prog_DLLTOOL+:} false; then : - $as_echo_n "(cached) " >&6 -else - if test -n "$DLLTOOL"; then - ac_cv_prog_DLLTOOL="$DLLTOOL" # Let the user override the test. -else -as_save_IFS=$IFS; IFS=$PATH_SEPARATOR -for as_dir in $PATH -do - IFS=$as_save_IFS - test -z "$as_dir" && as_dir=. - for ac_exec_ext in '' $ac_executable_extensions; do - if as_fn_executable_p "$as_dir/$ac_word$ac_exec_ext"; then - ac_cv_prog_DLLTOOL="${ac_tool_prefix}dlltool" - $as_echo "$as_me:${as_lineno-$LINENO}: found $as_dir/$ac_word$ac_exec_ext" >&5 - break 2 - fi -done - done -IFS=$as_save_IFS - -fi -fi -DLLTOOL=$ac_cv_prog_DLLTOOL -if test -n "$DLLTOOL"; then - { $as_echo "$as_me:${as_lineno-$LINENO}: result: $DLLTOOL" >&5 -$as_echo "$DLLTOOL" >&6; } -else - { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5 -$as_echo "no" >&6; } -fi - - -fi -if test -z "$ac_cv_prog_DLLTOOL"; then - ac_ct_DLLTOOL=$DLLTOOL - # Extract the first word of "dlltool", so it can be a program name with args. -set dummy dlltool; ac_word=$2 -{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for $ac_word" >&5 -$as_echo_n "checking for $ac_word... " >&6; } -if ${ac_cv_prog_ac_ct_DLLTOOL+:} false; then : - $as_echo_n "(cached) " >&6 -else - if test -n "$ac_ct_DLLTOOL"; then - ac_cv_prog_ac_ct_DLLTOOL="$ac_ct_DLLTOOL" # Let the user override the test. -else -as_save_IFS=$IFS; IFS=$PATH_SEPARATOR -for as_dir in $PATH -do - IFS=$as_save_IFS - test -z "$as_dir" && as_dir=. - for ac_exec_ext in '' $ac_executable_extensions; do - if as_fn_executable_p "$as_dir/$ac_word$ac_exec_ext"; then - ac_cv_prog_ac_ct_DLLTOOL="dlltool" - $as_echo "$as_me:${as_lineno-$LINENO}: found $as_dir/$ac_word$ac_exec_ext" >&5 - break 2 - fi -done - done -IFS=$as_save_IFS - -fi -fi -ac_ct_DLLTOOL=$ac_cv_prog_ac_ct_DLLTOOL -if test -n "$ac_ct_DLLTOOL"; then - { $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_ct_DLLTOOL" >&5 -$as_echo "$ac_ct_DLLTOOL" >&6; } -else - { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5 -$as_echo "no" >&6; } -fi - - if test "x$ac_ct_DLLTOOL" = x; then - DLLTOOL="dlltool" - else - case $cross_compiling:$ac_tool_warned in -yes:) -{ $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: using cross tools not prefixed with host triplet" >&5 -$as_echo "$as_me: WARNING: using cross tools not prefixed with host triplet" >&2;} -ac_tool_warned=yes ;; -esac - DLLTOOL=$ac_ct_DLLTOOL - fi -else - DLLTOOL="$ac_cv_prog_DLLTOOL" -fi - - if test -n "$ac_tool_prefix"; then - # Extract the first word of "${ac_tool_prefix}dllwrap", so it can be a program name with args. -set dummy ${ac_tool_prefix}dllwrap; ac_word=$2 -{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for $ac_word" >&5 -$as_echo_n "checking for $ac_word... " >&6; } -if ${ac_cv_prog_DLLWRAP+:} false; then : - $as_echo_n "(cached) " >&6 -else - if test -n "$DLLWRAP"; then - ac_cv_prog_DLLWRAP="$DLLWRAP" # Let the user override the test. -else -as_save_IFS=$IFS; IFS=$PATH_SEPARATOR -for as_dir in $PATH -do - IFS=$as_save_IFS - test -z "$as_dir" && as_dir=. - for ac_exec_ext in '' $ac_executable_extensions; do - if as_fn_executable_p "$as_dir/$ac_word$ac_exec_ext"; then - ac_cv_prog_DLLWRAP="${ac_tool_prefix}dllwrap" - $as_echo "$as_me:${as_lineno-$LINENO}: found $as_dir/$ac_word$ac_exec_ext" >&5 - break 2 - fi -done - done -IFS=$as_save_IFS - -fi -fi -DLLWRAP=$ac_cv_prog_DLLWRAP -if test -n "$DLLWRAP"; then - { $as_echo "$as_me:${as_lineno-$LINENO}: result: $DLLWRAP" >&5 -$as_echo "$DLLWRAP" >&6; } -else - { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5 -$as_echo "no" >&6; } -fi - - -fi -if test -z "$ac_cv_prog_DLLWRAP"; then - ac_ct_DLLWRAP=$DLLWRAP - # Extract the first word of "dllwrap", so it can be a program name with args. -set dummy dllwrap; ac_word=$2 -{ $as_echo "$as_me:${as_lineno-$LINENO}: checking for $ac_word" >&5 -$as_echo_n "checking for $ac_word... " >&6; } -if ${ac_cv_prog_ac_ct_DLLWRAP+:} false; then : - $as_echo_n "(cached) " >&6 -else - if test -n "$ac_ct_DLLWRAP"; then - ac_cv_prog_ac_ct_DLLWRAP="$ac_ct_DLLWRAP" # Let the user override the test. -else -as_save_IFS=$IFS; IFS=$PATH_SEPARATOR -for as_dir in $PATH -do - IFS=$as_save_IFS - test -z "$as_dir" && as_dir=. - for ac_exec_ext in '' $ac_executable_extensions; do - if as_fn_executable_p "$as_dir/$ac_word$ac_exec_ext"; then - ac_cv_prog_ac_ct_DLLWRAP="dllwrap" - $as_echo "$as_me:${as_lineno-$LINENO}: found $as_dir/$ac_word$ac_exec_ext" >&5 - break 2 - fi -done - done -IFS=$as_save_IFS - -fi -fi -ac_ct_DLLWRAP=$ac_cv_prog_ac_ct_DLLWRAP -if test -n "$ac_ct_DLLWRAP"; then - { $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_ct_DLLWRAP" >&5 -$as_echo "$ac_ct_DLLWRAP" >&6; } -else - { $as_echo "$as_me:${as_lineno-$LINENO}: result: no" >&5 -$as_echo "no" >&6; } -fi - - if test "x$ac_ct_DLLWRAP" = x; then - DLLWRAP="dllwrap" - else - case $cross_compiling:$ac_tool_warned in -yes:) -{ $as_echo "$as_me:${as_lineno-$LINENO}: WARNING: using cross tools not prefixed with host triplet" >&5 -$as_echo "$as_me: WARNING: using cross tools not prefixed with host triplet" >&2;} -ac_tool_warned=yes ;; -esac - DLLWRAP=$ac_ct_DLLWRAP - fi -else - DLLWRAP="$ac_cv_prog_DLLWRAP" -fi - if test -n "$ac_tool_prefix"; then # Extract the first word of "${ac_tool_prefix}windres", so it can be a program name with args. set dummy ${ac_tool_prefix}windres; ac_word=$2 diff --git a/configure.ac b/configure.ac index 7792ae5bad3..33634ccad9d 100644 --- a/configure.ac +++ b/configure.ac @@ -1128,8 +1128,6 @@ AC_PROG_RANLIB PGAC_CHECK_STRIP AC_CHECK_TOOL(AR, ar, ar) if test "$PORTNAME" = "win32"; then - AC_CHECK_TOOL(DLLTOOL, dlltool, dlltool) - AC_CHECK_TOOL(DLLWRAP, dllwrap, dllwrap) AC_CHECK_TOOL(WINDRES, windres, windres) fi diff --git a/src/Makefile.global.in b/src/Makefile.global.in index 5664c645f82..0963c6d6395 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -284,8 +284,6 @@ ZIC = @ZIC@ # Linking AR = @AR@ -DLLTOOL = @DLLTOOL@ -DLLWRAP = @DLLWRAP@ LIBS = @LIBS@ LDAP_LIBS_FE = @LDAP_LIBS_FE@ LDAP_LIBS_BE = @LDAP_LIBS_BE@ -- 2.37.3.542.gdd3f6c4cae --4uoxke3bx6ymzqvl Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v13-0002-Don-t-hardcode-tmp_check-as-test-directory-for-t.patch" ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. @ 2024-05-10 00:11 Alexander Korotkov <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Alexander Korotkov @ 2024-05-10 00:11 UTC (permalink / raw) To: Noah Misch <[email protected]>; +Cc: Pavel Borisov <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; Mark Dilger <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected] On Wed, May 1, 2024 at 5:26 AM Alexander Korotkov <[email protected]> wrote: > On Wed, May 1, 2024 at 5:24 AM Noah Misch <[email protected]> wrote: > > On Thu, Apr 25, 2024 at 04:59:54PM +0400, Pavel Borisov wrote: > > > 0001: Optimize speed by avoiding heap visibility checking for different > > > non-deduplicated index tuples as proposed by Noah Misch > > > > > > Speed measurements on my laptop using the exact method recommended by Noah > > > upthread: > > > Current master branch: checkunique off: 144s, checkunique on: 419s > > > With patch 0001: checkunique off: 141s, checkunique on: 171s > > > > Where is the CPU time going to make it still be 21% slower w/ checkunique on? > > It's a great improvement vs. current master, but I don't have an obvious > > explanation for the remaining +21%. > > I think there is at least extra index tuples comparison. The revised patchset is attached. I applied cosmetical changes. I'm going to push it if no objections. I don't post the patch with rename of new option. It doesn't seem there is a consensus. I must admit that keeping all the options in the same naming convention makes sense. ------ Regards, Alexander Korotkov Supabase Attachments: [application/octet-stream] v2-0003-Amcheck-Don-t-load-rightpage-into-BtreeCheckState.patch (1.9K, ../../CAPpHfdvxK+KSDqopaVdmCHgbO92AN4ibrbiCJ6i6-+g4oma-6Q@mail.gmail.com/2-v2-0003-Amcheck-Don-t-load-rightpage-into-BtreeCheckState.patch) download | inline diff: From 50f38667510057beea9c88bff8b7ee8cda825939 Mon Sep 17 00:00:00 2001 From: Alexander Korotkov <[email protected]> Date: Fri, 10 May 2024 03:08:15 +0300 Subject: [PATCH v2 3/4] Amcheck: Don't load rightpage into BtreeCheckState For cross-page unique constraint check use a local variable in the similar way as implemented in bt_right_page_check_scankey(). Reported-by: Peter Geoghegan --- contrib/amcheck/verify_nbtree.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c index b433cb33254..977f8b6799d 100644 --- a/contrib/amcheck/verify_nbtree.c +++ b/contrib/amcheck/verify_nbtree.c @@ -1910,6 +1910,8 @@ bt_target_page_check(BtreeCheckState *state) /* The first key on the next page is the same */ if (_bt_compare(state->rel, rightkey, state->target, max) == 0 && !rightkey->anynullkeys) { + Page rightpage; + /* * Do the bt_entry_unique_check() call if it was * postponed. @@ -1918,19 +1920,21 @@ bt_target_page_check(BtreeCheckState *state) bt_entry_unique_check(state, itup, state->targetblock, offset, &lVis); elog(DEBUG2, "cross page equal keys"); - state->target = palloc_btree_page(state, - rightblock_number); - topaque = BTPageGetOpaque(state->target); + rightpage = palloc_btree_page(state, + rightblock_number); + topaque = BTPageGetOpaque(rightpage); if (P_IGNORE(topaque) || !P_ISLEAF(topaque)) break; itemid = PageGetItemIdCareful(state, rightblock_number, - state->target, + rightpage, rightfirstoffset); - itup = (IndexTuple) PageGetItem(state->target, itemid); + itup = (IndexTuple) PageGetItem(rightpage, itemid); bt_entry_unique_check(state, itup, rightblock_number, rightfirstoffset, &lVis); + + pfree(rightpage); } } } -- 2.39.3 (Apple Git-145) [application/octet-stream] v2-0002-amcheck-Refactoring-the-storage-of-the-last-visib.patch (11.0K, ../../CAPpHfdvxK+KSDqopaVdmCHgbO92AN4ibrbiCJ6i6-+g4oma-6Q@mail.gmail.com/3-v2-0002-amcheck-Refactoring-the-storage-of-the-last-visib.patch) download | inline diff: From 9ae515266a041b30d14913000eb00dd9363fc1f8 Mon Sep 17 00:00:00 2001 From: Alexander Korotkov <[email protected]> Date: Fri, 10 May 2024 03:08:07 +0300 Subject: [PATCH v2 2/4] amcheck: Refactoring the storage of the last visible entry This commit introduces a new data structure BtreeLastVisibleEntry comprising information about the last visible heap entry with the current value of key. Usage of this data structure allows us to avoid passing all this information as individual function arguments. Reported-by: Alexander Korotkov Discussion: https://www.postgresql.org/message-id/CAPpHfdsVbB9ToriaB1UHuOKwjKxiZmTFQcEF%3DjuzzC_nby31uA%40mail.gmail.com Author: Pavel Borisov, Alexander Korotkov --- contrib/amcheck/verify_nbtree.c | 125 +++++++++++++++---------------- src/tools/pgindent/typedefs.list | 1 + 2 files changed, 61 insertions(+), 65 deletions(-) diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c index c7be785f88b..b433cb33254 100644 --- a/contrib/amcheck/verify_nbtree.c +++ b/contrib/amcheck/verify_nbtree.c @@ -145,6 +145,19 @@ typedef struct BtreeLevel bool istruerootlevel; } BtreeLevel; +/* + * Information about the last visible entry with current B-tree key. Used + * for validation of the unique constraint. + */ +typedef struct BtreeLastVisibleEntry +{ + BlockNumber blkno; /* Index block */ + OffsetNumber offset; /* Offset on index block */ + int postingIndex; /* Number in the posting list (-1 for + * non-deduplicated tuples) */ + ItemPointer tid; /* Heap tid */ +} BtreeLastVisibleEntry; + PG_FUNCTION_INFO_V1(bt_index_check); PG_FUNCTION_INFO_V1(bt_index_parent_check); @@ -165,17 +178,13 @@ static void bt_recheck_sibling_links(BtreeCheckState *state, BlockNumber btpo_prev_from_target, BlockNumber leftcurrent); static bool heap_entry_is_visible(BtreeCheckState *state, ItemPointer tid); -static void bt_report_duplicate(BtreeCheckState *state, ItemPointer tid, - BlockNumber block, OffsetNumber offset, - int posting, ItemPointer nexttid, +static void bt_report_duplicate(BtreeCheckState *state, BtreeLastVisibleEntry *lVis, + ItemPointer nexttid, BlockNumber nblock, OffsetNumber noffset, int nposting); static void bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup, BlockNumber targetblock, - OffsetNumber offset, int *lVis_i, - ItemPointer *lVis_tid, - OffsetNumber *lVis_offset, - BlockNumber *lVis_block); + OffsetNumber offset, BtreeLastVisibleEntry *lVis); static void bt_target_page_check(BtreeCheckState *state); static BTScanInsert bt_right_page_check_scankey(BtreeCheckState *state, OffsetNumber *rightfirstoffset); @@ -997,8 +1006,7 @@ heap_entry_is_visible(BtreeCheckState *state, ItemPointer tid) */ static void bt_report_duplicate(BtreeCheckState *state, - ItemPointer tid, BlockNumber block, OffsetNumber offset, - int posting, + BtreeLastVisibleEntry *lVis, ItemPointer nexttid, BlockNumber nblock, OffsetNumber noffset, int nposting) { @@ -1010,18 +1018,18 @@ bt_report_duplicate(BtreeCheckState *state, *pnposting = ""; htid = psprintf("tid=(%u,%u)", - ItemPointerGetBlockNumberNoCheck(tid), - ItemPointerGetOffsetNumberNoCheck(tid)); + ItemPointerGetBlockNumberNoCheck(lVis->tid), + ItemPointerGetOffsetNumberNoCheck(lVis->tid)); nhtid = psprintf("tid=(%u,%u)", ItemPointerGetBlockNumberNoCheck(nexttid), ItemPointerGetOffsetNumberNoCheck(nexttid)); - itid = psprintf("tid=(%u,%u)", block, offset); + itid = psprintf("tid=(%u,%u)", lVis->blkno, lVis->offset); - if (nblock != block || noffset != offset) + if (nblock != lVis->blkno || noffset != lVis->offset) nitid = psprintf(" tid=(%u,%u)", nblock, noffset); - if (posting >= 0) - pposting = psprintf(" posting %u", posting); + if (lVis->postingIndex >= 0) + pposting = psprintf(" posting %u", lVis->postingIndex); if (nposting >= 0) pnposting = psprintf(" posting %u", nposting); @@ -1038,9 +1046,7 @@ bt_report_duplicate(BtreeCheckState *state, /* Check if current nbtree leaf entry complies with UNIQUE constraint */ static void bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup, - BlockNumber targetblock, OffsetNumber offset, int *lVis_i, - ItemPointer *lVis_tid, OffsetNumber *lVis_offset, - BlockNumber *lVis_block) + BlockNumber targetblock, OffsetNumber offset, BtreeLastVisibleEntry *lVis) { ItemPointer tid; bool has_visible_entry = false; @@ -1049,7 +1055,7 @@ bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup, /* * Current tuple has posting list. Report duplicate if TID of any posting - * list entry is visible and lVis_tid is valid. + * list entry is visible and lVis->tid is valid. */ if (BTreeTupleIsPosting(itup)) { @@ -1059,11 +1065,10 @@ bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup, if (heap_entry_is_visible(state, tid)) { has_visible_entry = true; - if (ItemPointerIsValid(*lVis_tid)) + if (ItemPointerIsValid(lVis->tid)) { bt_report_duplicate(state, - *lVis_tid, *lVis_block, - *lVis_offset, *lVis_i, + lVis, tid, targetblock, offset, i); } @@ -1073,13 +1078,13 @@ bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup, * between the posting list entries of the first tuple on the * page after cross-page check. */ - if (*lVis_block != targetblock && ItemPointerIsValid(*lVis_tid)) + if (lVis->blkno != targetblock && ItemPointerIsValid(lVis->tid)) return; - *lVis_i = i; - *lVis_tid = tid; - *lVis_offset = offset; - *lVis_block = targetblock; + lVis->blkno = targetblock; + lVis->offset = offset; + lVis->postingIndex = i; + lVis->tid = tid; } } } @@ -1087,7 +1092,7 @@ bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup, /* * Current tuple has no posting list. If TID is visible save info about it * for the next comparisons in the loop in bt_target_page_check(). Report - * duplicate if lVis_tid is already valid. + * duplicate if lVis->tid is already valid. */ else { @@ -1095,37 +1100,38 @@ bt_entry_unique_check(BtreeCheckState *state, IndexTuple itup, if (heap_entry_is_visible(state, tid)) { has_visible_entry = true; - if (ItemPointerIsValid(*lVis_tid)) + if (ItemPointerIsValid(lVis->tid)) { bt_report_duplicate(state, - *lVis_tid, *lVis_block, - *lVis_offset, *lVis_i, + lVis, tid, targetblock, offset, -1); } - *lVis_i = -1; - *lVis_tid = tid; - *lVis_offset = offset; - *lVis_block = targetblock; + + lVis->blkno = targetblock; + lVis->offset = offset; + lVis->tid = tid; + lVis->postingIndex = -1; } } - if (!has_visible_entry && *lVis_block != InvalidBlockNumber && - *lVis_block != targetblock) + if (!has_visible_entry && + lVis->blkno != InvalidBlockNumber && + lVis->blkno != targetblock) { char *posting = ""; - if (*lVis_i >= 0) - posting = psprintf(" posting %u", *lVis_i); + if (lVis->postingIndex >= 0) + posting = psprintf(" posting %u", lVis->postingIndex); ereport(DEBUG1, (errcode(ERRCODE_NO_DATA), errmsg("index uniqueness can not be checked for index tid=(%u,%u) in index \"%s\"", targetblock, offset, RelationGetRelationName(state->rel)), errdetail("It doesn't have visible heap tids and key is equal to the tid=(%u,%u)%s (points to heap tid=(%u,%u)).", - *lVis_block, *lVis_offset, posting, - ItemPointerGetBlockNumberNoCheck(*lVis_tid), - ItemPointerGetOffsetNumberNoCheck(*lVis_tid)), + lVis->blkno, lVis->offset, posting, + ItemPointerGetBlockNumberNoCheck(lVis->tid), + ItemPointerGetOffsetNumberNoCheck(lVis->tid)), errhint("VACUUM the table and repeat the check."))); } } @@ -1372,12 +1378,8 @@ bt_target_page_check(BtreeCheckState *state) OffsetNumber max; BTPageOpaque topaque; - /* last visible entry info for checking indexes with unique constraint */ - int lVis_i = -1; /* the position of last visible item for - * posting tuple. for non-posting tuple (-1) */ - ItemPointer lVis_tid = NULL; - BlockNumber lVis_block = InvalidBlockNumber; - OffsetNumber lVis_offset = InvalidOffsetNumber; + /* Last visible entry info for checking indexes with unique constraint */ + BtreeLastVisibleEntry lVis = {InvalidBlockNumber, InvalidOffsetNumber, -1, NULL}; topaque = BTPageGetOpaque(state->target); max = PageGetMaxOffsetNumber(state->target); @@ -1784,11 +1786,9 @@ bt_target_page_check(BtreeCheckState *state) */ if (state->checkunique && state->indexinfo->ii_Unique && P_ISLEAF(topaque) && !skey->anynullkeys && - (BTreeTupleIsPosting(itup) || ItemPointerIsValid(lVis_tid))) + (BTreeTupleIsPosting(itup) || ItemPointerIsValid(lVis.tid))) { - bt_entry_unique_check(state, itup, state->targetblock, offset, - &lVis_i, &lVis_tid, &lVis_offset, - &lVis_block); + bt_entry_unique_check(state, itup, state->targetblock, offset, &lVis); unique_checked = true; } @@ -1816,17 +1816,16 @@ bt_target_page_check(BtreeCheckState *state) if (_bt_compare(state->rel, skey, state->target, OffsetNumberNext(offset)) != 0 || skey->anynullkeys) { - lVis_i = -1; - lVis_tid = NULL; - lVis_block = InvalidBlockNumber; - lVis_offset = InvalidOffsetNumber; + lVis.blkno = InvalidBlockNumber; + lVis.offset = InvalidOffsetNumber; + lVis.postingIndex = -1; + lVis.tid = NULL; } else if (!unique_checked) { - bt_entry_unique_check(state, itup, state->targetblock, offset, - &lVis_i, &lVis_tid, &lVis_offset, - &lVis_block); + bt_entry_unique_check(state, itup, state->targetblock, offset, &lVis); } + skey->scantid = scantid; /* Restore saved scan key state */ } @@ -1916,9 +1915,7 @@ bt_target_page_check(BtreeCheckState *state) * postponed. */ if (!unique_checked) - bt_entry_unique_check(state, itup, state->targetblock, offset, - &lVis_i, &lVis_tid, &lVis_offset, - &lVis_block); + bt_entry_unique_check(state, itup, state->targetblock, offset, &lVis); elog(DEBUG2, "cross page equal keys"); state->target = palloc_btree_page(state, @@ -1933,9 +1930,7 @@ bt_target_page_check(BtreeCheckState *state) rightfirstoffset); itup = (IndexTuple) PageGetItem(state->target, itemid); - bt_entry_unique_check(state, itup, rightblock_number, rightfirstoffset, - &lVis_i, &lVis_tid, &lVis_offset, - &lVis_block); + bt_entry_unique_check(state, itup, rightblock_number, rightfirstoffset, &lVis); } } } diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 2311f82d81e..7535dc6d692 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -315,6 +315,7 @@ BrinStatsData BrinTuple BrinValues BtreeCheckState +BtreeLastVisibleEntry BtreeLevel Bucket BufFile -- 2.39.3 (Apple Git-145) [application/octet-stream] v2-0001-amcheck-Optimize-speed-of-checking-for-unique-con.patch (3.8K, ../../CAPpHfdvxK+KSDqopaVdmCHgbO92AN4ibrbiCJ6i6-+g4oma-6Q@mail.gmail.com/4-v2-0001-amcheck-Optimize-speed-of-checking-for-unique-con.patch) download | inline diff: From 0c8a911657e68f77255f849897319c227160bd02 Mon Sep 17 00:00:00 2001 From: Alexander Korotkov <[email protected]> Date: Fri, 10 May 2024 03:07:53 +0300 Subject: [PATCH v2 1/4] amcheck: Optimize speed of checking for unique constraint violation Currently, when amcheck validates a unique constraint, it visits the heap for each index tuple. This commit implements skipping keys, which have only one non-dedeuplicated index tuple (quite common case for unique indexes). That gives substantial economy on index checking time. Reported-by: Noah Misch Discussion: https://postgr.es/m/20240325020323.fd.nmisch%40google.com Author: Alexander Korotkov, Pavel Borisov --- contrib/amcheck/verify_nbtree.c | 35 +++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c index 70f65b645a6..c7be785f88b 100644 --- a/contrib/amcheck/verify_nbtree.c +++ b/contrib/amcheck/verify_nbtree.c @@ -1429,6 +1429,13 @@ bt_target_page_check(BtreeCheckState *state) bool lowersizelimit; ItemPointer scantid; + /* + * True if we already called bt_entry_unique_check() for the current + * item. This helps to avoid visiting the heap for keys, which are + * anyway presented only once and can't comprise a unique violation. + */ + bool unique_checked = false; + CHECK_FOR_INTERRUPTS(); itemid = PageGetItemIdCareful(state, state->targetblock, @@ -1771,13 +1778,19 @@ bt_target_page_check(BtreeCheckState *state) /* * If the index is unique verify entries uniqueness by checking the - * heap tuples visibility. + * heap tuples visibility. Immediately check posting tuples and + * tuples with repeated keys. Postpone check for keys, which have the + * first appearance. */ if (state->checkunique && state->indexinfo->ii_Unique && - P_ISLEAF(topaque) && !skey->anynullkeys) + P_ISLEAF(topaque) && !skey->anynullkeys && + (BTreeTupleIsPosting(itup) || ItemPointerIsValid(lVis_tid))) + { bt_entry_unique_check(state, itup, state->targetblock, offset, &lVis_i, &lVis_tid, &lVis_offset, &lVis_block); + unique_checked = true; + } if (state->checkunique && state->indexinfo->ii_Unique && P_ISLEAF(topaque) && OffsetNumberNext(offset) <= max) @@ -1796,6 +1809,9 @@ bt_target_page_check(BtreeCheckState *state) * data (whole index tuple or last posting in index tuple). Key * containing null value does not violate unique constraint and * treated as different to any other key. + * + * If the next key is the same as the previous one, do the + * bt_entry_unique_check() call if it was postponed. */ if (_bt_compare(state->rel, skey, state->target, OffsetNumberNext(offset)) != 0 || skey->anynullkeys) @@ -1805,6 +1821,12 @@ bt_target_page_check(BtreeCheckState *state) lVis_block = InvalidBlockNumber; lVis_offset = InvalidOffsetNumber; } + else if (!unique_checked) + { + bt_entry_unique_check(state, itup, state->targetblock, offset, + &lVis_i, &lVis_tid, &lVis_offset, + &lVis_block); + } skey->scantid = scantid; /* Restore saved scan key state */ } @@ -1889,6 +1911,15 @@ bt_target_page_check(BtreeCheckState *state) /* The first key on the next page is the same */ if (_bt_compare(state->rel, rightkey, state->target, max) == 0 && !rightkey->anynullkeys) { + /* + * Do the bt_entry_unique_check() call if it was + * postponed. + */ + if (!unique_checked) + bt_entry_unique_check(state, itup, state->targetblock, offset, + &lVis_i, &lVis_tid, &lVis_offset, + &lVis_block); + elog(DEBUG2, "cross page equal keys"); state->target = palloc_btree_page(state, rightblock_number); -- 2.39.3 (Apple Git-145) [application/octet-stream] v2-0004-amcheck-Report-an-error-when-the-next-page-to-a-l.patch (2.5K, ../../CAPpHfdvxK+KSDqopaVdmCHgbO92AN4ibrbiCJ6i6-+g4oma-6Q@mail.gmail.com/5-v2-0004-amcheck-Report-an-error-when-the-next-page-to-a-l.patch) download | inline diff: From 9b36cf33f719107c899863f15ed5d0938a99b029 Mon Sep 17 00:00:00 2001 From: Alexander Korotkov <[email protected]> Date: Fri, 10 May 2024 03:07:22 +0300 Subject: [PATCH v2 4/4] amcheck: Report an error when the next page to a leaf is not a leaf This is a very unlikely condition during checking a B-tree unique constraint, meaning that the index structure is violated badly, and we shouldn't continue checking to avoid endless loops, etc. So it's worth immediately throwing an error. Reported-by: Peter Geoghegan Discussion: https://postgr.es/m/CAH2-Wzk%2B2116uOXdOViA27SHcr31WKPgmjsxXLBs_aTxAeThzg%40mail.gmail.com Author: Pavel Borisov --- contrib/amcheck/verify_nbtree.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/contrib/amcheck/verify_nbtree.c b/contrib/amcheck/verify_nbtree.c index 977f8b6799d..e9bbc18c4a5 100644 --- a/contrib/amcheck/verify_nbtree.c +++ b/contrib/amcheck/verify_nbtree.c @@ -1849,7 +1849,6 @@ bt_target_page_check(BtreeCheckState *state) if (offset == max) { BTScanInsert rightkey; - BlockNumber rightblock_number; /* first offset on a right index page (log only) */ OffsetNumber rightfirstoffset = InvalidOffsetNumber; @@ -1894,10 +1893,11 @@ bt_target_page_check(BtreeCheckState *state) * If index has unique constraint make sure that no more than one * found equal items is visible. */ - rightblock_number = topaque->btpo_next; if (state->checkunique && state->indexinfo->ii_Unique && - rightkey && P_ISLEAF(topaque) && rightblock_number != P_NONE) + rightkey && P_ISLEAF(topaque) && !P_RIGHTMOST(topaque)) { + BlockNumber rightblock_number = topaque->btpo_next; + elog(DEBUG2, "check cross page unique condition"); /* @@ -1924,9 +1924,19 @@ bt_target_page_check(BtreeCheckState *state) rightblock_number); topaque = BTPageGetOpaque(rightpage); - if (P_IGNORE(topaque) || !P_ISLEAF(topaque)) - break; - + if (P_IGNORE(topaque)) + { + if (unlikely(!P_ISLEAF(topaque))) + ereport(ERROR, + (errcode(ERRCODE_INDEX_CORRUPTED), + errmsg("right block of leaf block is non-leaf for index \"%s\"", + RelationGetRelationName(state->rel)), + errdetail_internal("Block=%u page lsn=%X/%X.", + state->targetblock, + LSN_FORMAT_ARGS(state->targetlsn)))); + else + break; + } itemid = PageGetItemIdCareful(state, rightblock_number, rightpage, rightfirstoffset); -- 2.39.3 (Apple Git-145) ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. @ 2024-05-10 00:42 Tom Lane <[email protected]> parent: Alexander Korotkov <[email protected]> 0 siblings, 2 replies; 10+ messages in thread From: Tom Lane @ 2024-05-10 00:42 UTC (permalink / raw) To: Alexander Korotkov <[email protected]>; +Cc: Noah Misch <[email protected]>; Pavel Borisov <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; Mark Dilger <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected] Alexander Korotkov <[email protected]> writes: > The revised patchset is attached. I applied cosmetical changes. I'm > going to push it if no objections. Is this really suitable material to be pushing post-feature-freeze? It doesn't look like it's fixing any new-in-v17 issues. regards, tom lane ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. @ 2024-05-10 07:34 Pavel Borisov <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 10+ messages in thread From: Pavel Borisov @ 2024-05-10 07:34 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; Mark Dilger <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected] Hi, Tom! On Fri, 10 May 2024, 04:43 Tom Lane, <[email protected]> wrote: > Alexander Korotkov <[email protected]> writes: > > The revised patchset is attached. I applied cosmetical changes. I'm > > going to push it if no objections. > > Is this really suitable material to be pushing post-feature-freeze? > It doesn't look like it's fixing any new-in-v17 issues. > > regards, tom lane > I think these patches are nice-to-have optimizations and refactorings to make code look better. They are not necessary for the main feature. They don't fix any bugs. But they were requested in the thread, and make sense in my opinion. I really don't know what's the policy of applying code improvements other than bugfixes post feature-freeze. IMO they are safe to be appiled to v17, but they also could be added later. Regards, Pavel Borisov Supabase > ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. @ 2024-05-10 08:39 Alexander Korotkov <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 10+ messages in thread From: Alexander Korotkov @ 2024-05-10 08:39 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Noah Misch <[email protected]>; Pavel Borisov <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; Mark Dilger <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected] On Fri, May 10, 2024 at 3:43 AM Tom Lane <[email protected]> wrote: > Alexander Korotkov <[email protected]> writes: > > The revised patchset is attached. I applied cosmetical changes. I'm > > going to push it if no objections. > > Is this really suitable material to be pushing post-feature-freeze? > It doesn't look like it's fixing any new-in-v17 issues. These are code improvements to the 5ae2087202, which answer critics in the thread. 0001 comprises an optimization, but it's rather small and simple. 0002 and 0003 contain refactoring. 0004 contains better error reporting. For me this looks like pretty similar to what others commit post-FF, isn't it? ------ Regards, Alexander Korotkov Supabase ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. @ 2024-05-10 12:10 Pavel Borisov <[email protected]> parent: Alexander Korotkov <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Pavel Borisov @ 2024-05-10 12:10 UTC (permalink / raw) To: Alexander Korotkov <[email protected]>; +Cc: Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; Mark Dilger <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected] Hi, Alexander! On Fri, 10 May 2024 at 12:39, Alexander Korotkov <[email protected]> wrote: > On Fri, May 10, 2024 at 3:43 AM Tom Lane <[email protected]> wrote: > > Alexander Korotkov <[email protected]> writes: > > > The revised patchset is attached. I applied cosmetical changes. I'm > > > going to push it if no objections. > > > > Is this really suitable material to be pushing post-feature-freeze? > > It doesn't look like it's fixing any new-in-v17 issues. > > These are code improvements to the 5ae2087202, which answer critics in > the thread. 0001 comprises an optimization, but it's rather small and > simple. 0002 and 0003 contain refactoring. 0004 contains better > error reporting. For me this looks like pretty similar to what others > commit post-FF, isn't it? > I've re-checked patches v2. Differences from v1 are in improving naming/pgindent's/commit messages. In 0002 order of variables in struct BtreeLastVisibleEntry changed. This doesn't change code behavior. Patch v2-0003 doesn't contain credits and a discussion link. All other patches do. Overall, patches contain small performance optimization (0001), code refactoring and error reporting changes. IMO they could be pushed post-FF. Regards, Pavel. ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. @ 2024-05-10 17:35 Mark Dilger <[email protected]> parent: Pavel Borisov <[email protected]> 0 siblings, 1 reply; 10+ messages in thread From: Mark Dilger @ 2024-05-10 17:35 UTC (permalink / raw) To: Pavel Borisov <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected] > On May 10, 2024, at 5:10 AM, Pavel Borisov <[email protected]> wrote: > > Hi, Alexander! > > On Fri, 10 May 2024 at 12:39, Alexander Korotkov <[email protected]> wrote: > On Fri, May 10, 2024 at 3:43 AM Tom Lane <[email protected]> wrote: > > Alexander Korotkov <[email protected]> writes: > > > The revised patchset is attached. I applied cosmetical changes. I'm > > > going to push it if no objections. > > > > Is this really suitable material to be pushing post-feature-freeze? > > It doesn't look like it's fixing any new-in-v17 issues. > > These are code improvements to the 5ae2087202, which answer critics in > the thread. 0001 comprises an optimization, but it's rather small and > simple. 0002 and 0003 contain refactoring. 0004 contains better > error reporting. For me this looks like pretty similar to what others > commit post-FF, isn't it? > I've re-checked patches v2. Differences from v1 are in improving naming/pgindent's/commit messages. > In 0002 order of variables in struct BtreeLastVisibleEntry changed. This doesn't change code behavior. > > Patch v2-0003 doesn't contain credits and a discussion link. All other patches do. > > Overall, patches contain small performance optimization (0001), code refactoring and error reporting changes. IMO they could be pushed post-FF. v2-0001's commit message itself says, "This commit implements skipping keys". I take no position on the correctness or value of the improvement, but it seems out of scope post feature freeze. The patch seems to postpone uniqueness checking until later in the scan than what the prior version did, and that kind of change could require more analysis than we have time for at this point in the release cycle. v2-0002 does appear to just be refactoring. I don't care for a small portion of that patch, but I doubt it violates the post feature freeze rules. In particular: + BtreeLastVisibleEntry lVis = {InvalidBlockNumber, InvalidOffsetNumber, -1, NULL}; v2-0003 may be an improvement in some way, but it compounds some preexisting confusion also. There is already a member of the BtreeCheckState called "target" and a memory context in that struct called "targetcontext". That context is used to allocate pages "state->target", "rightpage", "child" and "page", but not "metapage". Perhaps "targetcontext" is a poor choice of name? "notmetacontext" is a terrible name, but closer to describing the purpose of the memory context. Care to propose something sensible? Prior to applying v2-0003, the rightpage was stored in state->target, and continued to be in state->target later when entering /* * * Downlink check * * * Additional check of child items iff this is an internal page and * caller holds a ShareLock. This happens for every downlink (item) * in target excluding the negative-infinity downlink (again, this is * because it has no useful value to compare). */ if (!P_ISLEAF(topaque) && state->readonly) bt_child_check(state, skey, offset); and thereafter. Now, the rightpage of state->target is created, checked, and free'd, and then the old state->target gets processed in the downlink check and thereafter. This is either introducing a bug, or fixing one, but the commit message is totally ambiguous about whether this is a bugfix or a code cleanup or something else? I think this kind of patch should have a super clear commit message about what it thinks it is doing. v2-0004 guards against a real threat, and is reasonable post feature freeze — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. @ 2024-05-10 18:42 Pavel Borisov <[email protected]> parent: Mark Dilger <[email protected]> 0 siblings, 2 replies; 10+ messages in thread From: Pavel Borisov @ 2024-05-10 18:42 UTC (permalink / raw) To: Mark Dilger <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected] Hi, Mark! On Fri, 10 May 2024, 21:35 Mark Dilger, <[email protected]> wrote: > > > > On May 10, 2024, at 5:10 AM, Pavel Borisov <[email protected]> > wrote: > > > > Hi, Alexander! > > > > On Fri, 10 May 2024 at 12:39, Alexander Korotkov <[email protected]> > wrote: > > On Fri, May 10, 2024 at 3:43 AM Tom Lane <[email protected]> wrote: > > > Alexander Korotkov <[email protected]> writes: > > > > The revised patchset is attached. I applied cosmetical changes. I'm > > > > going to push it if no objections. > > > > > > Is this really suitable material to be pushing post-feature-freeze? > > > It doesn't look like it's fixing any new-in-v17 issues. > > > > These are code improvements to the 5ae2087202, which answer critics in > > the thread. 0001 comprises an optimization, but it's rather small and > > simple. 0002 and 0003 contain refactoring. 0004 contains better > > error reporting. For me this looks like pretty similar to what others > > commit post-FF, isn't it? > > I've re-checked patches v2. Differences from v1 are in improving > naming/pgindent's/commit messages. > > In 0002 order of variables in struct BtreeLastVisibleEntry changed. This > doesn't change code behavior. > > > > Patch v2-0003 doesn't contain credits and a discussion link. All other > patches do. > > > > Overall, patches contain small performance optimization (0001), code > refactoring and error reporting changes. IMO they could be pushed post-FF. > > v2-0001's commit message itself says, "This commit implements skipping > keys". I take no position on the correctness or value of the improvement, > but it seems out of scope post feature freeze. The patch seems to postpone > uniqueness checking until later in the scan than what the prior version > did, and that kind of change could require more analysis than we have time > for at this point in the release cycle. > > > v2-0002 does appear to just be refactoring. I don't care for a small > portion of that patch, but I doubt it violates the post feature freeze > rules. In particular: > > + BtreeLastVisibleEntry lVis = {InvalidBlockNumber, > InvalidOffsetNumber, -1, NULL}; > > > v2-0003 may be an improvement in some way, but it compounds some > preexisting confusion also. There is already a member of the > BtreeCheckState called "target" and a memory context in that struct called > "targetcontext". That context is used to allocate pages "state->target", > "rightpage", "child" and "page", but not "metapage". Perhaps > "targetcontext" is a poor choice of name? "notmetacontext" is a terrible > name, but closer to describing the purpose of the memory context. Care to > propose something sensible? > > Prior to applying v2-0003, the rightpage was stored in state->target, and > continued to be in state->target later when entering > > /* > * * Downlink check * > * > * Additional check of child items iff this is an internal page and > * caller holds a ShareLock. This happens for every downlink > (item) > * in target excluding the negative-infinity downlink (again, this > is > * because it has no useful value to compare). > */ > if (!P_ISLEAF(topaque) && state->readonly) > bt_child_check(state, skey, offset); > > and thereafter. Now, the rightpage of state->target is created, checked, > and free'd, and then the old state->target gets processed in the downlink > check and thereafter. This is either introducing a bug, or fixing one, but > the commit message is totally ambiguous about whether this is a bugfix or a > code cleanup or something else? I think this kind of patch should have a > super clear commit message about what it thinks it is doing. > > > v2-0004 guards against a real threat, and is reasonable post feature freeze > > > — > Mark Dilger > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > IMO 0003 doesn't introduce nor fixes a bug. It loads rightpage into a local variable, rather that to a BtreeCheckState that can have another users of state->target afterb uniqueness check in the future, but don't have now. So the original patch is correct, and the goal of this refactoring is to untie rightpage fron state structure as it's used only transiently for cross-page unuque check. It's the same style as already used bt_right_page_check_scankey() that loads rightpage into a local variable. For 0002 I doubt I understand your actual positiob. Could you explain what it violates or doesn't violate? Best regards, Pavel. ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. @ 2024-05-10 18:43 Pavel Borisov <[email protected]> parent: Pavel Borisov <[email protected]> 1 sibling, 0 replies; 10+ messages in thread From: Pavel Borisov @ 2024-05-10 18:43 UTC (permalink / raw) To: Mark Dilger <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected] On Fri, 10 May 2024, 22:42 Pavel Borisov, <[email protected]> wrote: > Hi, Mark! > > > On Fri, 10 May 2024, 21:35 Mark Dilger, <[email protected]> > wrote: > >> >> >> > On May 10, 2024, at 5:10 AM, Pavel Borisov <[email protected]> >> wrote: >> > >> > Hi, Alexander! >> > >> > On Fri, 10 May 2024 at 12:39, Alexander Korotkov <[email protected]> >> wrote: >> > On Fri, May 10, 2024 at 3:43 AM Tom Lane <[email protected]> wrote: >> > > Alexander Korotkov <[email protected]> writes: >> > > > The revised patchset is attached. I applied cosmetical changes. >> I'm >> > > > going to push it if no objections. >> > > >> > > Is this really suitable material to be pushing post-feature-freeze? >> > > It doesn't look like it's fixing any new-in-v17 issues. >> > >> > These are code improvements to the 5ae2087202, which answer critics in >> > the thread. 0001 comprises an optimization, but it's rather small and >> > simple. 0002 and 0003 contain refactoring. 0004 contains better >> > error reporting. For me this looks like pretty similar to what others >> > commit post-FF, isn't it? >> > I've re-checked patches v2. Differences from v1 are in improving >> naming/pgindent's/commit messages. >> > In 0002 order of variables in struct BtreeLastVisibleEntry changed. >> This doesn't change code behavior. >> > >> > Patch v2-0003 doesn't contain credits and a discussion link. All other >> patches do. >> > >> > Overall, patches contain small performance optimization (0001), code >> refactoring and error reporting changes. IMO they could be pushed post-FF. >> >> v2-0001's commit message itself says, "This commit implements skipping >> keys". I take no position on the correctness or value of the improvement, >> but it seems out of scope post feature freeze. The patch seems to postpone >> uniqueness checking until later in the scan than what the prior version >> did, and that kind of change could require more analysis than we have time >> for at this point in the release cycle. >> >> >> v2-0002 does appear to just be refactoring. I don't care for a small >> portion of that patch, but I doubt it violates the post feature freeze >> rules. In particular: >> >> + BtreeLastVisibleEntry lVis = {InvalidBlockNumber, >> InvalidOffsetNumber, -1, NULL}; >> >> >> v2-0003 may be an improvement in some way, but it compounds some >> preexisting confusion also. There is already a member of the >> BtreeCheckState called "target" and a memory context in that struct called >> "targetcontext". That context is used to allocate pages "state->target", >> "rightpage", "child" and "page", but not "metapage". Perhaps >> "targetcontext" is a poor choice of name? "notmetacontext" is a terrible >> name, but closer to describing the purpose of the memory context. Care to >> propose something sensible? >> >> Prior to applying v2-0003, the rightpage was stored in state->target, and >> continued to be in state->target later when entering >> >> /* >> * * Downlink check * >> * >> * Additional check of child items iff this is an internal page >> and >> * caller holds a ShareLock. This happens for every downlink >> (item) >> * in target excluding the negative-infinity downlink (again, >> this is >> * because it has no useful value to compare). >> */ >> if (!P_ISLEAF(topaque) && state->readonly) >> bt_child_check(state, skey, offset); >> >> and thereafter. Now, the rightpage of state->target is created, checked, >> and free'd, and then the old state->target gets processed in the downlink >> check and thereafter. This is either introducing a bug, or fixing one, but >> the commit message is totally ambiguous about whether this is a bugfix or a >> code cleanup or something else? I think this kind of patch should have a >> super clear commit message about what it thinks it is doing. >> >> >> v2-0004 guards against a real threat, and is reasonable post feature >> freeze >> >> >> — >> Mark Dilger >> EnterpriseDB: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> > > IMO 0003 doesn't introduce nor fixes a bug. It loads rightpage into a > local variable, rather that to a BtreeCheckState that can have another > users of state->target afterb uniqueness check in the future, but don't > have now. So the original patch is correct, and the goal of this > refactoring is to untie rightpage fron state structure as it's used only > transiently for cross-page unuque check. It's the same style as already > used bt_right_page_check_scankey() that loads rightpage into a local > variable. > > For 0002 I doubt I understand your actual positiob. Could you explain what > it violates or doesn't violate? > Please forgive many typos in the previous message, I wrote from phone. Pavel. > ^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. @ 2024-05-10 19:27 Mark Dilger <[email protected]> parent: Pavel Borisov <[email protected]> 1 sibling, 0 replies; 10+ messages in thread From: Mark Dilger @ 2024-05-10 19:27 UTC (permalink / raw) To: Pavel Borisov <[email protected]>; +Cc: Alexander Korotkov <[email protected]>; Tom Lane <[email protected]>; Noah Misch <[email protected]>; Peter Eisentraut <[email protected]>; Aleksander Alekseev <[email protected]>; Postgres hackers <[email protected]>; Maxim Orlov <[email protected]>; Andres Freund <[email protected]>; Greg Stark <[email protected]>; Julien Rouhaud <[email protected]>; David Steele <[email protected]>; Peter Geoghegan <[email protected]>; Maxim Orlov <[email protected]>; [email protected] > On May 10, 2024, at 11:42 AM, Pavel Borisov <[email protected]> wrote: > > IMO 0003 doesn't introduce nor fixes a bug. It loads rightpage into a local variable, rather that to a BtreeCheckState that can have another users of state->target afterb uniqueness check in the future, but don't have now. So the original patch is correct, and the goal of this refactoring is to untie rightpage fron state structure as it's used only transiently for cross-page unuque check. It's the same style as already used bt_right_page_check_scankey() that loads rightpage into a local variable. Well, you can put an Assert(false) dead in the middle of the code we're discussing and all the regression tests still pass, so I'd argue the change is getting zero test coverage. This patch introduces a change that stores a new page into variable "rightpage" rather than overwriting "state->target", which the old implementation most certainly did. That means that after returning from bt_target_page_check() into the calling function bt_check_level_from_leftmost() the value in state->target is not what it would have been prior to this patch. Now, that'd be irrelevant if nobody goes on to consult that value, but just 44 lines further down in bt_check_level_from_leftmost() state->target is clearly used. So the behavior at that point is changing between the old and new versions of the code, and I think I'm within reason to ask if it was wrong before the patch, wrong after the patch, or something else? Is this a bug being introduced, being fixed, or ... ? Having a regression test that actually touches this code would go a fair way towards helping the analysis. > For 0002 I doubt I understand your actual positiob. Could you explain what it violates or doesn't violate? v2-0002 is does not violate the post feature freeze restriction on new features so far as I can tell, but I just don't care for the variable initialization because it doesn't name the fields. If anybody refactored the struct they might not notice that the need to reorder this initialization, and depending on various factors including compiler flags they might not get an error. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company ^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2024-05-10 19:27 UTC | newest] Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2022-08-30 22:33 [PATCH v13 01/20] Remove DLLTOOL, DLLWRAP from configure / Makefile.global.in Andres Freund <[email protected]> 2024-05-10 00:11 Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Alexander Korotkov <[email protected]> 2024-05-10 00:42 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Tom Lane <[email protected]> 2024-05-10 07:34 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Pavel Borisov <[email protected]> 2024-05-10 08:39 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Alexander Korotkov <[email protected]> 2024-05-10 12:10 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Pavel Borisov <[email protected]> 2024-05-10 17:35 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Mark Dilger <[email protected]> 2024-05-10 18:42 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Pavel Borisov <[email protected]> 2024-05-10 18:43 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Pavel Borisov <[email protected]> 2024-05-10 19:27 ` Re: [PATCH] Improve amcheck to also check UNIQUE constraint in btree index. Mark Dilger <[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