Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -43,3 +43,6 @@ lib*.pc
/Release/
/tmp_install/
/portlock/

# local build/install prefix (never commit)
/inst/
20 changes: 20 additions & 0 deletions .local-gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# Local development ignores (not tracked in .gitignore)
# To enable: git config core.excludesFile .local-gitignore
.local-gitignore
Comment on lines +1 to +3

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should not be part of the patch. .local-gitignore is a personal, local-development convenience file (note the comment: "not tracked in .gitignore" and the intent to be wired up via git config core.excludesFile). Committing it violates the minimal-diff discipline: it is unrelated to the stated purpose of the change and would draw an immediate rejection on pgsql-hackers.

Several concrete problems compound this:

  • The file ignores itself (.local-gitignore), which is self-contradictory once it is actually tracked in the repository.
  • It references CLAUDE.md, an AI-assistant artifact that has no business being referenced from the PostgreSQL source tree.
  • Ignore entries like log/, results/, regression.diffs, regression.out, build/, install/ are developer-environment concerns; anything genuinely worth ignoring for all contributors belongs in the tree's real .gitignore files, not a personal excludes file checked into the repo.

Remove this file from the commit entirely and keep it out of version control (e.g., add it to your global/local excludes instead).

Comment on lines +1 to +3

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should not be committed. .local-gitignore is a per-developer convenience file (it is opt-in via git config core.excludesFile .local-gitignore and even lists itself as ignored on line 3). It is unrelated to the stated purpose of the change (the recno/undo access-method work) and the repo already has a comprehensive committed .gitignore for build artifacts. Committing personal-environment configuration is exactly the kind of unrelated change that gets patches rejected on pgsql-hackers. Notably it ignores CLAUDE.md (an AI-assistant artifact), which further confirms this is a local-environment leftover. Please drop this file from the commit; if you rely on it locally, keep it untracked and set core.excludesFile in your own git config, or add the truly-needed patterns to the tracked .gitignore.

Comment on lines +1 to +3

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire file does not belong in the patch. .local-gitignore is a personal, developer-local convenience file (as its own comment states, it's activated per-clone via git config core.excludesFile). Committing it into the tree violates the minimal-diff / one-logical-change discipline: it is unrelated to the actual feature and will draw an immediate rejection on -hackers.

Specific problems:

  • Local dev-environment ignores (build-valgrind/, build-asan/, .direnv/, .cache/, .history, test-db/, install-*/) reflect one contributor's setup and are not tree policy. Genuinely shared ignore patterns belong in the tracked .gitignore, not a parallel file every developer must opt into.
  • CLAUDE.md (line 20) is an AI-assistant artifact; its presence here signals leaked personal tooling and should not be referenced in a PostgreSQL patch.
  • A file whose first line is a comment telling you how to git config it is inherently not something the tree should carry — this is exactly the kind of "personal workflow" file the community asks authors to keep out of the diff.

Drop this file from the patch entirely. If any of these patterns are truly project-wide, add them to the existing .gitignore instead.

Comment on lines +1 to +3

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file should not be part of the patch. It is a purely personal, local developer-convenience artifact: its own header notes it is "not tracked in .gitignore" and requires an opt-in git config core.excludesFile .local-gitignore to have any effect, so it provides no shared value to the project and other contributors would never enable it. Committing developer-environment scaffolding (including the reference to CLAUDE.md and other AI/tooling paths) violates the minimal-diff discipline and is a top rejection reason on pgsql-hackers. If any of these ignore patterns are genuinely project-relevant, add them to the tracked .gitignore instead; otherwise drop this file from the change entirely.

Comment on lines +1 to +3

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire file should not be committed. It is a per-developer local artifact -- the header comment itself states it is "not tracked in .gitignore" and is meant to be enabled locally via git config core.excludesFile. Local-only ignore patterns belong in .git/info/exclude or a globally-configured core.excludesFile, never in a tracked file in the repository root. It is unrelated to the stated purpose of the change and violates the minimal-diff rule; a patch posted to pgsql-hackers containing this would be rejected immediately. Additionally, it leaks tool-specific developer scaffolding (CLAUDE.md, .direnv/, .cache/, personal build-*/install-* paths). Please drop this file from the change entirely.

build/
build-valgrind/
build-asan/
install/
install-valgrind/
install-asan/
.direnv/
.cache/
.history
test-db/
log/
results/
regression.diffs
regression.out
*.core
core.*
CLAUDE.md
30 changes: 29 additions & 1 deletion contrib/amcheck/verify_nbtree.c
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ static void bt_child_highkey_check(BtreeCheckState *state,
static void bt_downlink_missing_check(BtreeCheckState *state, bool rightsplit,
BlockNumber blkno, Page page);
static void bt_tuple_present_callback(Relation index, ItemPointer tid,
const RowID *rowid,
Datum *values, bool *isnull,
bool tupleIsAlive, void *checkstate);
static IndexTuple bt_normalize_tuple(BtreeCheckState *state,
Expand Down Expand Up @@ -2779,7 +2780,8 @@ bt_downlink_missing_check(BtreeCheckState *state, bool rightsplit,
* also allows us to detect the corruption in many cases.
*/
static void
bt_tuple_present_callback(Relation index, ItemPointer tid, Datum *values,
bt_tuple_present_callback(Relation index, ItemPointer tid, const RowID *rowid,
Datum *values,
bool *isnull, bool tupleIsAlive, void *checkstate)
{
BtreeCheckState *state = (BtreeCheckState *) checkstate;
Expand Down Expand Up @@ -2860,6 +2862,32 @@ bt_normalize_tuple(BtreeCheckState *state, IndexTuple itup)
/* Caller should only pass "logical" non-pivot tuples here */
Assert(!BTreeTupleIsPosting(itup) && !BTreeTupleIsPivot(itup));

/*
* For a wider-than-TID identity (e.g. RECNO's (TID, gen)) the on-page
* leaf tuples carry a trailing suffix (the generation) that is NOT part
* of the key+TID the heapallindexed scan reconstructs. The suffix must
* be excluded from the fingerprint: an index whose key was not touched by
* an unrelated in-place UPDATE keeps its original-generation entry, while
* the live tuple's shared per-row generation has since advanced -- the
* key+TID still match, only the (irrelevant-here) suffix differs. Reform
* from the key datums so the leaf tuple (with suffix) and the heap-derived
* tuple (without) fingerprint to the identical base tuple. index_getattr
* reads only the key attributes and ignores the trailing suffix.
*/
if (RelationGetIndexRowIdWidth(state->rel) > sizeof(ItemPointerData))
{
Datum widevals[INDEX_MAX_KEYS];
bool widenull[INDEX_MAX_KEYS];

for (i = 0; i < tupleDescriptor->natts; i++)
widevals[i] = index_getattr(itup, TupleDescAttr(tupleDescriptor, i)->attnum,
tupleDescriptor, &widenull[i]);
reformed = index_form_tuple(tupleDescriptor, widevals, widenull);
reformed->t_tid = itup->t_tid;
/* Continue normalization on the suffix-free base tuple. */
itup = reformed;
Comment on lines +2885 to +2888

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memory leak in the wide-RowID reform path. When the tuple has varlena datums and formnewtup becomes true, a second reformed is allocated at line 2980 and the pointer here is overwritten, orphaning the intermediate suffix-stripped tuple -- it is never pfree'd. The scan-side caller (bt_normalize_tuple call at ~line 1513) only frees the returned norm (if (norm != itup) pfree(norm)), and its context (state->targetcontext) is reset only once per page, so this leaks once per non-pivot leaf tuple that has any varlena/compressed/short-4B datum in a wide-RowID index -- potentially the whole page's worth of tuples. Track the intermediate allocation and free it before returning, e.g. keep a local IndexTuple stripped = NULL; set it when reforming here, and if (stripped) pfree(stripped); on the paths that return a different tuple.

}

/* Easy case: It's immediately clear that tuple has no varlena datums */
if (!IndexTupleHasVarwidths(itup))
return itup;
Expand Down
9 changes: 7 additions & 2 deletions contrib/bloom/blinsert.c
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ initCachedPage(BloomBuildState *buildstate)
* Per-tuple callback for table_index_build_scan.
*/
static void
bloomBuildCallback(Relation index, ItemPointer tid, Datum *values,
bloomBuildCallback(Relation index, ItemPointer tid, const RowID *rowid,
Datum *values,
bool *isnull, bool tupleIsAlive, void *state)
{
BloomBuildState *buildstate = (BloomBuildState *) state;
Expand Down Expand Up @@ -176,12 +177,14 @@ blinsert(Relation index, Datum *values, bool *isnull,
ItemPointer ht_ctid, Relation heapRel,
IndexUniqueCheck checkUnique,
bool indexUnchanged,
IndexInfo *indexInfo)
IndexInfo *indexInfo,
const RowID *rowid)
{
BloomState blstate;
BloomTuple *itup;
MemoryContext oldCtx;
MemoryContext insertCtx;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated whitespace churn: this blank line was inserted in the middle of the local-variable declaration block and is not required by the signature change. Per minimal-diff discipline, drop it to keep the patch focused.

Suggested change
MemoryContext insertCtx;
MemoryContext insertCtx;


BloomMetaPageData *metaData;
Buffer buffer,
metaBuffer;
Expand All @@ -191,6 +194,8 @@ blinsert(Relation index, Datum *values, bool *isnull,
OffsetNumber nStart;
GenericXLogState *state;

(void) rowid; /* bloom stores only the heap TID */

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The (void) rowid; cast plus explanatory comment is scaffolding to silence an unused-parameter warning. Other index AMs that ignore rowid presumably do not need this (unused function parameters do not warn in the tree's baseline). This adds noise; recommend dropping it unless a specific warning is actually triggered. Confidence: moderate.


insertCtx = AllocSetContextCreate(CurrentMemoryContext,
"Bloom insert temporary context",
ALLOCSET_DEFAULT_SIZES);
Expand Down
3 changes: 2 additions & 1 deletion contrib/bloom/bloom.h
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,8 @@ extern bool blinsert(Relation index, Datum *values, bool *isnull,
ItemPointer ht_ctid, Relation heapRel,
IndexUniqueCheck checkUnique,
bool indexUnchanged,
struct IndexInfo *indexInfo);
struct IndexInfo *indexInfo,
const RowID *rowid);
extern IndexScanDesc blbeginscan(Relation r, int nkeys, int norderbys);
extern int64 blgetbitmap(IndexScanDesc scan, TIDBitmap *tbm);
extern void blrescan(IndexScanDesc scan, ScanKey scankey, int nscankeys,
Expand Down
9 changes: 6 additions & 3 deletions contrib/pageinspect/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@ OBJS = \
gistfuncs.o \
hashfuncs.o \
heapfuncs.o \
rawpage.o
rawpage.o \
recnofuncs.o

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bumps pageinspect to 1.14 and wires in recnofuncs.o, adding three new SQL-callable functions (recno_page_items, recno_page_stats, recno_tuple_infomask_flags). However, REGRESS is not extended and there is no sql/recno*.sql test for these functions, so the new behavior is entirely untested. Per PostgreSQL patch standards a user-visible change requires regression coverage. Add a recno pageinspect regress test to REGRESS (and the mirror list in meson.build), and update sql/oldextversions.sql to exercise the 1.13->1.14 upgrade path (matching the existing 1.11/1.12 pattern). (moderate confidence)

Comment on lines +13 to +14

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new pageinspect 1.14 upgrade adds three SQL functions (recno_page_items, recno_page_stats, recno_tuple_infomask_flags) backed by recnofuncs.o, but no regression test covers them: the REGRESS list here has no recno entry, meson.build's regress list has none either, and there is no sql/recno.sql under contrib/pageinspect. Per PostgreSQL standards a user-visible change (new inspection functions) must ship with regression tests exercising edge/error paths, not just be installable. Add a recno regress test and list it here (and in meson.build to keep them in sync). (high confidence)


EXTENSION = pageinspect
DATA = pageinspect--1.12--1.13.sql \
DATA = pageinspect--1.13--1.14.sql \
pageinspect--1.12--1.13.sql \
Comment on lines +17 to +18

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This bumps pageinspect to 1.14, adding three new SQL-callable functions (recno_page_items, recno_page_stats, recno_tuple_infomask_flags) in recnofuncs.c, but REGRESS is not extended and there is no new sql/recno*.sql test. Per PostgreSQL patch standards, a user-visible change needs regression coverage. Add a recno pageinspect regress test to REGRESS here (and the mirror list in meson.build), and update sql/oldextversions.sql to exercise the 1.13->1.14 upgrade path (matching the existing 1.11/1.12 pattern). (moderate confidence)

pageinspect--1.11--1.12.sql pageinspect--1.10--1.11.sql \
pageinspect--1.9--1.10.sql pageinspect--1.8--1.9.sql \
pageinspect--1.7--1.8.sql pageinspect--1.6--1.7.sql \
Expand All @@ -32,7 +34,8 @@ REGRESS = \
gin \
gist \
hash \
oldextversions
oldextversions \
hot_updates
Comment on lines +37 to +38

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch's actual new pageinspect functionality is recnofuncs.o + pageinspect--1.13--1.14.sql, which expose recno_page_items(), recno_page_stats(), and recno_tuple_infomask_flags(). But the only REGRESS test added here is hot_updates, whose SQL exercises heap HOT semantics (heap_page_items, pg_stat_get_tuples_hot_updated) and never calls any of the three new recno functions. Net effect: the code this patch adds ships with zero regression coverage. Per PostgreSQL patch discipline, a user-visible new function set without tests is WIP. Add a recno-specific SQL test (e.g. recno_pageinspect) that calls recno_page_items/recno_page_stats/recno_tuple_infomask_flags, and register it here (and in meson.build's regress.sql). Confidence: high.


ifdef USE_PGXS
PG_CONFIG = pg_config
Expand Down
Loading
Loading